| Message ID | 20230103114359.2663262-1-haibo.chen@nxp.com |
|---|---|
| Headers | show |
| Series | add imx93 adc support | expand |
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? > -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 >
On Tue, 3 Jan 2023 19:43:56 +0800 haibo.chen@nxp.com wrote: > From: Haibo Chen <haibo.chen@nxp.com> > > The ADC in i.mx93 is a total new ADC IP, add a driver to support > this ADC. > > Currently, only support one shot normal conversion triggered by > software. For other mode, will add in future. > > Signed-off-by: Haibo Chen <haibo.chen@nxp.com> Hi Haibo, I'm still not sure about the power handling in remove. One other minor comment inline that would be good to clean up for v6. Thanks, Jonathan > new file mode 100644 > index 000000000000..0c98de438919 > --- /dev/null > +++ b/drivers/iio/adc/imx93_adc.c ... > + > +static int imx93_adc_calibration(struct imx93_adc *adc) > +{ > + u32 mcr, msr; > + int ret; > + > + /* make sure ADC in power down mode */ > + imx93_adc_power_down(adc); > + > + /* config SAR controller operating clock */ > + mcr = readl(adc->regs + IMX93_ADC_MCR); > + mcr &= ~FIELD_PREP(IMX93_ADC_MCR_ADCLKSE_MASK, 1); > + writel(mcr, adc->regs + IMX93_ADC_MCR); > + > + imx93_adc_power_up(adc); I think this function should be side effect free on error to aid easy reviewing / code modularity. Thus if anything after this point fails, the device should be deliberately powered down again to remove that side effect. > + > + /* > + * TODO: we use the default TSAMP/NRSMPL/AVGEN in MCR, > + * can add the setting of these bit if need in future. > + */ > + > + /* run calibration */ > + mcr = readl(adc->regs + IMX93_ADC_MCR); > + mcr |= FIELD_PREP(IMX93_ADC_MCR_CALSTART_MASK, 1); > + writel(mcr, adc->regs + IMX93_ADC_MCR); > + > + /* wait calibration to be finished */ > + ret = readl_poll_timeout(adc->regs + IMX93_ADC_MSR, msr, > + !(msr & IMX93_ADC_MSR_CALBUSY_MASK), 1000, 2000000); > + if (ret == -ETIMEDOUT) { > + dev_warn(adc->dev, "ADC do not finish calibration in 1 min!\n"); > + return ret; > + } > + > + /* check whether calbration is success or not */ > + msr = readl(adc->regs + IMX93_ADC_MSR); > + if (msr & IMX93_ADC_MSR_CALFAIL_MASK) { > + dev_warn(adc->dev, "ADC calibration failed!\n"); > + return -EAGAIN; > + } > + > + return 0; > +} > + ... > +static int imx93_adc_probe(struct platform_device *pdev) > +{ > + struct imx93_adc *adc; > + struct iio_dev *indio_dev; > + struct device *dev = &pdev->dev; > + int ret; > + > + indio_dev = devm_iio_device_alloc(dev, sizeof(*adc)); > + if (!indio_dev) > + return dev_err_probe(dev, -ENOMEM, > + "Failed allocating iio device\n"); > + > + adc = iio_priv(indio_dev); > + adc->dev = dev; > + > + mutex_init(&adc->lock); > + adc->regs = devm_platform_ioremap_resource(pdev, 0); > + if (IS_ERR(adc->regs)) > + return dev_err_probe(dev, PTR_ERR(adc->regs), > + "Failed geting ioremap resource\n"); > + > + /* The third irq is for ADC conversion usage */ > + adc->irq = platform_get_irq(pdev, 2); > + if (adc->irq < 0) > + return adc->irq; > + > + adc->ipg_clk = devm_clk_get(dev, "ipg"); > + if (IS_ERR(adc->ipg_clk)) > + return dev_err_probe(dev, PTR_ERR(adc->ipg_clk), > + "Failed getting clock.\n"); > + > + adc->vref = devm_regulator_get(dev, "vref"); > + if (IS_ERR(adc->vref)) > + return dev_err_probe(dev, PTR_ERR(adc->vref), > + "Failed getting reference voltage.\n"); > + > + ret = regulator_enable(adc->vref); > + if (ret) > + return dev_err_probe(dev, ret, > + "Failed to enable reference voltage.\n"); > + > + platform_set_drvdata(pdev, indio_dev); > + > + init_completion(&adc->completion); > + > + indio_dev->name = "imx93-adc"; > + indio_dev->info = &imx93_adc_iio_info; > + indio_dev->modes = INDIO_DIRECT_MODE; > + indio_dev->channels = imx93_adc_iio_channels; > + indio_dev->num_channels = ARRAY_SIZE(imx93_adc_iio_channels); > + > + ret = clk_prepare_enable(adc->ipg_clk); > + if (ret) { > + dev_err_probe(dev, ret, > + "Failed to enable ipg clock.\n"); > + goto error_regulator_disable; > + } > + > + ret = request_irq(adc->irq, imx93_adc_isr, 0, IMX93_ADC_DRIVER_NAME, adc); > + if (ret < 0) { > + dev_err_probe(dev, ret, > + "Failed requesting irq, irq = %d\n", adc->irq); > + goto error_ipg_clk_disable; > + } > + > + ret = imx93_adc_calibration(adc); > + if (ret < 0) As above, I'd expect the device to be powered down if this function fails and hence need an additional error label. > + goto error_free_adc_irq; > + > + imx93_adc_config_ad_clk(adc); > + > + ret = iio_device_register(indio_dev); > + if (ret) { > + dev_err_probe(dev, ret, > + "Failed to register this iio device.\n"); > + goto error_free_adc_irq; > + } > + > + pm_runtime_set_active(dev); > + pm_runtime_set_autosuspend_delay(dev, 50); > + pm_runtime_use_autosuspend(dev); > + pm_runtime_enable(dev); > + > + return 0; > + > +error_free_adc_irq: > + imx93_adc_power_down(adc); > + free_irq(adc->irq, adc); > +error_ipg_clk_disable: > + clk_disable_unprepare(adc->ipg_clk); > +error_regulator_disable: > + regulator_disable(adc->vref); > + > + return ret; > +} > + > +static int imx93_adc_remove(struct platform_device *pdev) > +{ > + struct iio_dev *indio_dev = platform_get_drvdata(pdev); > + struct imx93_adc *adc = iio_priv(indio_dev); > + struct device *dev = adc->dev; As per reply to cover letter I don't understand logic by which we are definitely runtime resumed at this stage. > + > + pm_runtime_disable(dev); > + pm_runtime_dont_use_autosuspend(dev); > + pm_runtime_put_noidle(dev); This is not balanced with a pm_runtime_get* so I think we underflow (which is protected against in runtime pm ref counting but not a nice thing to do deliberately). > + iio_device_unregister(indio_dev); > + imx93_adc_power_down(adc); > + free_irq(adc->irq, adc); > + clk_disable_unprepare(adc->ipg_clk); > + regulator_disable(adc->vref); > + > + return 0; > +} > +
> -----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. 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 > >
> -----Original Message----- > From: Jonathan Cameron <jic23@kernel.org> > Sent: 2023年1月8日 21:22 > 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 1/3] iio: adc: add imx93 adc support > > On Tue, 3 Jan 2023 19:43:56 +0800 > haibo.chen@nxp.com wrote: > > > From: Haibo Chen <haibo.chen@nxp.com> > > > > The ADC in i.mx93 is a total new ADC IP, add a driver to support this > > ADC. > > > > Currently, only support one shot normal conversion triggered by > > software. For other mode, will add in future. > > > > Signed-off-by: Haibo Chen <haibo.chen@nxp.com> > Hi Haibo, > > I'm still not sure about the power handling in remove. One other minor > comment inline that would be good to clean up for v6. > > Thanks, > > Jonathan > > > new file mode 100644 > > index 000000000000..0c98de438919 > > --- /dev/null > > +++ b/drivers/iio/adc/imx93_adc.c > > ... > > > + > > +static int imx93_adc_calibration(struct imx93_adc *adc) { > > + u32 mcr, msr; > > + int ret; > > + > > + /* make sure ADC in power down mode */ > > + imx93_adc_power_down(adc); > > + > > + /* config SAR controller operating clock */ > > + mcr = readl(adc->regs + IMX93_ADC_MCR); > > + mcr &= ~FIELD_PREP(IMX93_ADC_MCR_ADCLKSE_MASK, 1); > > + writel(mcr, adc->regs + IMX93_ADC_MCR); > > + > > + imx93_adc_power_up(adc); > > I think this function should be side effect free on error to aid easy reviewing / > code modularity. Thus if anything after this point fails, the device should be > deliberately powered down again to remove that side effect. > > > + > > + /* > > + * TODO: we use the default TSAMP/NRSMPL/AVGEN in MCR, > > + * can add the setting of these bit if need in future. > > + */ > > + > > + /* run calibration */ > > + mcr = readl(adc->regs + IMX93_ADC_MCR); > > + mcr |= FIELD_PREP(IMX93_ADC_MCR_CALSTART_MASK, 1); > > + writel(mcr, adc->regs + IMX93_ADC_MCR); > > + > > + /* wait calibration to be finished */ > > + ret = readl_poll_timeout(adc->regs + IMX93_ADC_MSR, msr, > > + !(msr & IMX93_ADC_MSR_CALBUSY_MASK), 1000, 2000000); > > + if (ret == -ETIMEDOUT) { > > + dev_warn(adc->dev, "ADC do not finish calibration in 1 min!\n"); > > + return ret; > > + } > > + > > + /* check whether calbration is success or not */ > > + msr = readl(adc->regs + IMX93_ADC_MSR); > > + if (msr & IMX93_ADC_MSR_CALFAIL_MASK) { > > + dev_warn(adc->dev, "ADC calibration failed!\n"); > > + return -EAGAIN; > > + } > > + > > + return 0; > > +} > > + > > ... > > > +static int imx93_adc_probe(struct platform_device *pdev) { > > + struct imx93_adc *adc; > > + struct iio_dev *indio_dev; > > + struct device *dev = &pdev->dev; > > + int ret; > > + > > + indio_dev = devm_iio_device_alloc(dev, sizeof(*adc)); > > + if (!indio_dev) > > + return dev_err_probe(dev, -ENOMEM, > > + "Failed allocating iio device\n"); > > + > > + adc = iio_priv(indio_dev); > > + adc->dev = dev; > > + > > + mutex_init(&adc->lock); > > + adc->regs = devm_platform_ioremap_resource(pdev, 0); > > + if (IS_ERR(adc->regs)) > > + return dev_err_probe(dev, PTR_ERR(adc->regs), > > + "Failed geting ioremap resource\n"); > > + > > + /* The third irq is for ADC conversion usage */ > > + adc->irq = platform_get_irq(pdev, 2); > > + if (adc->irq < 0) > > + return adc->irq; > > + > > + adc->ipg_clk = devm_clk_get(dev, "ipg"); > > + if (IS_ERR(adc->ipg_clk)) > > + return dev_err_probe(dev, PTR_ERR(adc->ipg_clk), > > + "Failed getting clock.\n"); > > + > > + adc->vref = devm_regulator_get(dev, "vref"); > > + if (IS_ERR(adc->vref)) > > + return dev_err_probe(dev, PTR_ERR(adc->vref), > > + "Failed getting reference voltage.\n"); > > + > > + ret = regulator_enable(adc->vref); > > + if (ret) > > + return dev_err_probe(dev, ret, > > + "Failed to enable reference voltage.\n"); > > + > > + platform_set_drvdata(pdev, indio_dev); > > + > > + init_completion(&adc->completion); > > + > > + indio_dev->name = "imx93-adc"; > > + indio_dev->info = &imx93_adc_iio_info; > > + indio_dev->modes = INDIO_DIRECT_MODE; > > + indio_dev->channels = imx93_adc_iio_channels; > > + indio_dev->num_channels = ARRAY_SIZE(imx93_adc_iio_channels); > > + > > + ret = clk_prepare_enable(adc->ipg_clk); > > + if (ret) { > > + dev_err_probe(dev, ret, > > + "Failed to enable ipg clock.\n"); > > + goto error_regulator_disable; > > + } > > + > > + ret = request_irq(adc->irq, imx93_adc_isr, 0, IMX93_ADC_DRIVER_NAME, > adc); > > + if (ret < 0) { > > + dev_err_probe(dev, ret, > > + "Failed requesting irq, irq = %d\n", adc->irq); > > + goto error_ipg_clk_disable; > > + } > > + > > + ret = imx93_adc_calibration(adc); > > + if (ret < 0) > > As above, I'd expect the device to be powered down if this function fails and > hence need an additional error label. Yes, should add a new error label. > > > + goto error_free_adc_irq; > > + > > + imx93_adc_config_ad_clk(adc); > > + > > + ret = iio_device_register(indio_dev); > > + if (ret) { > > + dev_err_probe(dev, ret, > > + "Failed to register this iio device.\n"); > > + goto error_free_adc_irq; > > + } > > + > > + pm_runtime_set_active(dev); > > + pm_runtime_set_autosuspend_delay(dev, 50); > > + pm_runtime_use_autosuspend(dev); > > + pm_runtime_enable(dev); > > + > > + return 0; > > + > > +error_free_adc_irq: > > + imx93_adc_power_down(adc); > > + free_irq(adc->irq, adc); > > +error_ipg_clk_disable: > > + clk_disable_unprepare(adc->ipg_clk); > > +error_regulator_disable: > > + regulator_disable(adc->vref); > > + > > + return ret; > > +} > > + > > +static int imx93_adc_remove(struct platform_device *pdev) { > > + struct iio_dev *indio_dev = platform_get_drvdata(pdev); > > + struct imx93_adc *adc = iio_priv(indio_dev); > > + struct device *dev = adc->dev; > > As per reply to cover letter I don't understand logic by which we are definitely > runtime resumed at this stage. > > > + > > + pm_runtime_disable(dev); > > + pm_runtime_dont_use_autosuspend(dev); > > + pm_runtime_put_noidle(dev); > > This is not balanced with a pm_runtime_get* so I think we underflow (which is > protected against in runtime pm ref counting but not a nice thing to do > deliberately). Oh, yes, seems still need to call pm_runtime_get_sync() even we still in runtime resume state. Best Regards Haibo Chen > > > + iio_device_unregister(indio_dev); > > + imx93_adc_power_down(adc); > > + free_irq(adc->irq, adc); > > + clk_disable_unprepare(adc->ipg_clk); > > + regulator_disable(adc->vref); > > + > > + return 0; > > +} > > +
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 > > > >
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. -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