22.01.2020 10:16, Sameer Pujar пишет:

On 1/22/2020 11:53 AM, Dmitry Osipenko wrote:

quoted

External email: Use caution opening links or attachments


22.01.2020 07:32, Sameer Pujar пишет:
[snip]

quoted

quoted

quoted

quoted

quoted

+static int tegra210_i2s_remove(struct platform_device *pdev)
+{
+     pm_runtime_disable(&pdev->dev);
+     if (!pm_runtime_status_suspended(&pdev->dev))
+             tegra210_i2s_runtime_suspend(&pdev->dev);

This breaks device's RPM refcounting if it was disabled in the active
state. This code should be removed. At most you could warn about the
unxpected RPM state here, but it shouldn't be necessary.

I guess this was added for safety and explicit suspend keeps clock
disabled.
Not sure if ref-counting of the device matters when runtime PM is
disabled and device is removed.
I see few drivers using this way.

It should matter (if I'm not missing something) because RPM should
be in
a wrecked state once you'll try to re-load the driver's module. Likely
that those few other drivers are wrong.

[snip]

Once the driver is re-loaded and RPM is enabled, I don't think it
would use
the same 'dev' and the corresponding ref count. Doesn't it use the new
counters?
If RPM is not working for some reason, most likely it would be the case
for other
devices. What best driver can do is probably do a force suspend during
removal if
already not done. I would prefer to keep, since multiple drivers still
have it,
unless there is a real harm in doing so.

I took a closer look and looks like the counter actually should be
reset. Still I don't think that it's a good practice to make changes
underneath of RPM, it may strike back.

If RPM is broken, it probably would have been caught during device usage.
I will remove explicit suspend here if no any concerns from other folks.
Thanks.

quoted

quoted

quoted

quoted

quoted

quoted

+     int rx_fifo_th;

Could rx_fifo_th be negative?

rx_fifo_th itself does not take negative values, explicit
typecasting> is avoided in "if" condition by declaring this as "int"

Explicit typecasting isn't needed for integers.

What I meant was, rx_fifo_th is checked against a 'int' variable in an
"if" condition.

What's the problem with comparing of unsigned with signed?

consider this example,
----
unsigned int x = 5;
int y = -1;

(x > y) is false.

Right

----
Hence should be careful while using signed and unsigned comparisons.

quoted

Besides, cif_conf.audio_ch > I2S_RX_FIFO_DEPTH can't be ever true, isn't
it? I2S_RX_FIFO_DEPTH=64, channels_max=16

Yes true.

quoted

Lastly, nothing stops you to make max_th unsigned.

will update.

Thanks