On 22/01/2020 07:16, Sameer Pujar wrote: ... >>>>>>> +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. I recall that this was the preferred way of doing this from the RPM folks. Tegra30 I2S driver does the same and Stephen had pointed me to this as a reference. I believe that this is meant to ensure that the device is always powered-off regardless of it RPM is enabled or not and what the current state is. Now for Tegra210 (or actually 64-bit Tegra) RPM is always enabled and so we don't need to worry about the !RPM case. However, I still don't see the harm in this. Jon -- nvpublic _______________________________________________ Alsa-devel mailing list Alsa-devel@xxxxxxxxxxxxxxxx https://mailman.alsa-project.org/mailman/listinfo/alsa-devel