lkml.org 
[lkml]   [2020]   [Jan]   [24]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    SubjectRe: [alsa-devel] [PATCH 4/9] ASoC: tegra: add Tegra210 based I2S driver
    From
    Date

    On 23/01/2020 15:16, Dmitry Osipenko wrote:
    > 23.01.2020 12:22, Sameer Pujar пишет:
    >>
    >>
    >> On 1/22/2020 9:57 PM, Dmitry Osipenko wrote:
    >>> External email: Use caution opening links or attachments
    >>>
    >>>
    >>> 22.01.2020 14:52, Jon Hunter пишет:
    >>>> 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.
    >>> Yes, it was kinda actual for the case of unavailable RPM.
    >>
    >>> Anyways, /I think/ variant like this should have been more preferred:
    >>>
    >>> if (!pm_runtime_enabled(&pdev->dev))
    >>>          tegra210_i2s_runtime_suspend(&pdev->dev);
    >>> else
    >>>          pm_runtime_disable(&pdev->dev);
    >>
    >> I think it looks to be similar to what is there already.
    >>
    >> pm_runtime_disable(&pdev->dev); // it would turn out to be a dummy call
    >> if !RPM
    >> if (!pm_runtime_status_suspended(&pdev->dev)) // it is true always if !RPM
    >>         tegra210_i2s_runtime_suspend(&pdev->dev);
    >
    > Maybe this is fine for !RPM, but not really fine in a case of enabled
    > RPM. Device could be in resumed state after pm_runtime_disable() if it
    > wasn't suspended before the disabling.

    I don't see any problem with this for the !RPM case.

    Jon

    --
    nvpublic

    \
     
     \ /
      Last update: 2020-01-24 10:08    [W:2.248 / U:0.036 seconds]
    ©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site