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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



> -----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.

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
> >





[Index of Archives]     [Linux USB Devel]     [Video for Linux]     [Linux Audio Users]     [Yosemite News]     [Linux Input]     [Linux Kernel]     [Linux SCSI]     [X.org]

  Powered by Linux