Re: [PATCH v5 0/3] add imx93 adc support

[Jonathan Cameron <Jonathan.Cameron@Huawei.com>]

On Tue, 17 Jan 2023 09:00:50 +0000
Bough Chen <haibo.chen@nxp.com> wrote:

> > -----Original Message-----
> > From: Jonathan Cameron <jic23@kernel.org>
> > Sent: 2023年1月8日 21:15
> > To: Bough Chen <haibo.chen@nxp.com>
> > Cc: lars@metafoo.de; robh+dt@kernel.org; krzysztof.kozlowski+dt@linaro.org;
> > shawnguo@kernel.org; s.hauer@pengutronix.de; kernel@pengutronix.de;
> > festevam@gmail.com; dl-linux-imx <linux-imx@nxp.com>;
> > linux-iio@vger.kernel.org; devicetree@vger.kernel.org
> > Subject: Re: [PATCH v5 0/3] add imx93 adc support
> > 
> > On Tue,  3 Jan 2023 19:43:55 +0800
> > haibo.chen@nxp.com wrote:
> >   
> > > From: Haibo Chen <haibo.chen@nxp.com>
> > >
> > > V5:
> > >   -For ADC driver, use dev_err_probe() to replace dev_err() in  
> > dev_err_probe().  
> > >   -Add imx93_adc_power_down() in the probe error path.
> > >   -Re-order the function in imx93_adc_remove(), make them inverse in  
> > probe().  
> > >   -Remove the pm_runtime_get_sync(dev) in imx93_adc_remove(), because  
> > this driver  
> > >    enable the pm_runtime autosuspend feature, and config the delay as  
> > 50ms. So when  
> > >    called imx93_adc_remove(), this device still in runtime resume state, no  
> > need to  
> > >    force resume the device back.  
> > I don't follow this point.  Perhaps talk me through in more detail on why the
> > device will be in a runtime resumed state when ever we hit remove?  
> 
> Hi Jonathan,
> 
> Sorry for delay.
> 
> This driver use module_platform_driver, so when do rmmod or unbind operation
> The function call steps are as belowing:
> platform_driver_unregister
>   --> driver_unregister
>      --> bus_remove_driver
>         --> driver_detach
>            --> device_release_driver_internal
>               --> __device_release_driver  
> 
> In __device_release_driver {
>         pm_runtime_get_sync(dev);
>         ...
>         pm_runtime_put_sync(dev);
>         device_remove(dev);     -> call imx93_adc_remove()
>         ...
> }
> 
> Since in this imx93 adc driver, we use 50ms auto suspend dealy,
>       pm_runtime_set_autosuspend_delay(dev, 50);
> 
> and here is the description of this API  (Documentation/power/runtime_pm.rst):
>   `void pm_runtime_set_autosuspend_delay(struct device *dev, int delay);`
>     - set the power.autosuspend_delay value to 'delay' (expressed in
>       milliseconds); if 'delay' is negative then runtime suspends are
>       prevented; if power.use_autosuspend is set, pm_runtime_get_sync may be
>       called or the device's usage counter may be decremented and
>       pm_runtime_idle called depending on if power.autosuspend_delay is
>       changed to or from a negative value; if power.use_autosuspend is clear,
>       pm_runtime_idle is called
> 
> and the description of pm_runtime_put_sync.
> /**
>  * pm_runtime_put_sync - Drop device usage counter and run "idle check" if 0.
>  * @dev: Target device.
>  *
>  * Decrement the runtime PM usage counter of @dev and if it turns out to be
>  * equal to 0, invoke the "idle check" callback of @dev and, depending on its
>  * return value, set up autosuspend of @dev or suspend it (depending on whether
>  * or not autosuspend has been enabled for it).
>  *
>  * The possible return values of this function are the same as for
>  * pm_runtime_idle() and the runtime PM usage counter of @dev remains
>  * decremented in all cases, even if it returns an error code.
>  */
> static inline int pm_runtime_put_sync(struct device *dev)
> {
>         return __pm_runtime_idle(dev, RPM_GET_PUT);
> }
> 
> This means after call the pm_runtime_put_sync in __device_release_driver(), imx93_adc will not call imx93_adc_runtime_suspend() immediately, will do it after 50ms, but just then, call the imx93_adc_remove(), so this means when imx93_adc_remove() execute, the ADC related clocks keep on.

If I follow correctly, that means we are relying on a race?
I don't think it is valid to assume that device_remove will be called within the 50 msec
window even though it is extremely likely.  We should be incrementing the reference
counter appropriately to ensure autosuspend doesn't happen.

Jonathan

> 
> Best Regards
> Haibo Chen
> >   
> > >   -no changes for binding doc and dts.
> > >
> > > V4:
> > >   For ADC driver, re-define the ADC status show the relation to specific  
> > register bit.  
> > >   Redo the imx93_adc_remove(), change the return error sequence in  
> > imx93_adc_read_raw(),  
> > >   and use a direct string for indio_dev->name.
> > >   For dt-bings, change the commit title and add maintainer's reviewed by  
> > tag  
> > >   For dts, no change.
> > >
> > > V3:
> > >   For dt-bings, add some change according to review comments, and pass  
> > dt_binding_check.  
> > >   For dts, add #io-channel-cells = <1>; to pass dtbs_check
> > >   For ADC driver, no change.
> > >
> > > V2:
> > >   For ADC driver, add change according to matainer's commets.
> > >
> > > Haibo Chen (3):
> > >   iio: adc: add imx93 adc support
> > >   dt-bindings: iio: adc: Add NXP IMX93 ADC
> > >   arm64: dts: imx93: add ADC support
> > >
> > >  .../bindings/iio/adc/nxp,imx93-adc.yaml       |  81 +++
> > >  MAINTAINERS                                   |   4 +-
> > >  .../boot/dts/freescale/imx93-11x11-evk.dts    |  12 +
> > >  arch/arm64/boot/dts/freescale/imx93.dtsi      |  13 +
> > >  drivers/iio/adc/Kconfig                       |  10 +
> > >  drivers/iio/adc/Makefile                      |   1 +
> > >  drivers/iio/adc/imx93_adc.c                   | 477  
> > ++++++++++++++++++  
> > >  7 files changed, 597 insertions(+), 1 deletion(-)  create mode 100644
> > > Documentation/devicetree/bindings/iio/adc/nxp,imx93-adc.yaml
> > >  create mode 100644 drivers/iio/adc/imx93_adc.c
> > >  
>