On Tue, 17 Jan 2023 09:00:50 +0000
Bough Chen <haibo.chen@xxxxxxx> wrote:
> > -----Original Message-----
> > From: Jonathan Cameron <jic23@xxxxxxxxxx>
> > Sent: 2023年1月8日 21:15
> > To: Bough Chen <haibo.chen@xxxxxxx>
> > Cc: lars@xxxxxxxxxx; robh+dt@xxxxxxxxxx; krzysztof.kozlowski+dt@xxxxxxxxxx;
> > shawnguo@xxxxxxxxxx; s.hauer@xxxxxxxxxxxxxx; kernel@xxxxxxxxxxxxxx;
> > festevam@xxxxxxxxx; dl-linux-imx <linux-imx@xxxxxxx>;
> > linux-iio@xxxxxxxxxxxxxxx; devicetree@xxxxxxxxxxxxxxx
> > Subject: Re: [PATCH v5 0/3] add imx93 adc support
> >
> > On Tue, 3 Jan 2023 19:43:55 +0800
> > haibo.chen@xxxxxxx wrote:
> >
> > > From: Haibo Chen <haibo.chen@xxxxxxx>
> > >
> > > 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
> > >
>
