Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Message-ID: <07452438-f7e7-70d7-7a38-567f0f224fa1@collabora.com>
Date:   Tue, 19 Jul 2022 16:11:31 +0300
MIME-Version: 1.0
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:91.0) Gecko/20100101
 Thunderbird/91.10.0
Subject: Re: [PATCH v9 2/2] iio: light: Add support for ltrf216a sensor
Content-Language: en-US
To:     Jonathan Cameron <jic23@jic23.retrosnub.co.uk>
Cc:     Shreeya Patel <shreeya.patel@collabora.com>, lars@metafoo.de,
        robh+dt@kernel.org, Zhigang.Shi@liteon.com, krisman@collabora.com,
        linux-iio@vger.kernel.org, devicetree@vger.kernel.org,
        linux-kernel@vger.kernel.org, kernel@collabora.com,
        alvaro.soliverez@collabora.com, andy.shevchenko@gmail.com
References: <20220715111626.1066513-1-shreeya.patel@collabora.com>
 <20220715111626.1066513-3-shreeya.patel@collabora.com>
 <20220718182547.360e5cf2@jic23-huawei>
 <1e880d3f-758b-56a8-d468-dcb06f4cbc18@collabora.com>
 <20220719131808.7899acd4@jic23-huawei>
From:   Dmitry Osipenko <dmitry.osipenko@collabora.com>
In-Reply-To: <20220719131808.7899acd4@jic23-huawei>
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 7bit
Precedence: bulk

On 7/19/22 15:19, Jonathan Cameron wrote:
> On Tue, 19 Jul 2022 14:56:51 +0300
> Dmitry Osipenko <dmitry.osipenko@collabora.com> wrote:
> 
>> On 7/18/22 20:25, Jonathan Cameron wrote:
>>> What turns this off again?  I'd expect to see a devm_add_action_or_reset()
>>> to do that in the !CONFIG_PM case.
>>>
>>> This is also an unusual pattern. As far as I can tell it works.
>>> Normal trick for ensuring !CONFIG_PM works is to:
>>>
>>> 1) Unconditionally turn device on.
>>> 2) Register unconditional device off devm_callback. Very rarely harmful even if device already off
>>>    due to runtime pm.  
>>
>> If CONFIG_PM is disabled, do we really need to care about the power
>> management on removal?
>>
> 
> Best effort + in general if we do something probe(), we want to do the
> reverse in remove().  Sure it's not super important, but it's a nice
> to have.  This tends to get 'fixed' by people revisiting the driver
> after it has merged.
> 
>>> 3) Then call pm_runtime_set_active() so the state tracking matches.  
>>
>> We can add pm_runtime_set_active() before h/w is touched for more
>> consistency. On Steam Deck supplies are always enabled, but this may be
>> not true for other devices.
> 
> Generally set it wherever you 'enable' the device as you are indicating
> the state after that has happened. That might be really early though.
> 
>>
>>> 4) Call 	
>>>   pm_runtime_mark_last_busy(dev);
>>>   pm_runtime_put_autosuspend(dev);
>>>   (here you have a function to do this anyway)
>>>   to let runtime_pm use same path as normal to autosuspend
>>>
>>> the upshot of this is that if !CONFIG_PM 3 and 4 do nothing and device
>>> is left turned on.  Is there something I'm missing that makes that cycle
>>> inappropriate here?  The main reason to do this is it then looks exactly
>>> like any other runtime_pm calls elsewhere in the driver, so easier to review.  
>>
>> It's appropriate, although caring about PM when it's disabled in kernel
>> config could be unnecessary, IMO. It was my suggestion to keep the h/w
>> enabled on driver's removal with !CONFIG_PM, minimizing the code.
>>
> For the cost of about 4-8 lines of code, I think it's worth having, but can
> also see why you decided against.

Alright, thank you for the review. Shreeya will address it all and
prepare the v10.

-- 
Best regards,
Dmitry