22.01.2020 07:32, Sameer Pujar пишет: [snip] >>>>> +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. >>>>> + 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? 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 Lastly, nothing stops you to make max_th unsigned. _______________________________________________ Alsa-devel mailing list Alsa-devel@xxxxxxxxxxxxxxxx https://mailman.alsa-project.org/mailman/listinfo/alsa-devel