> -----Original Message-----
> From: Jonathan Cameron <jic23@xxxxxxxxxx>
> Sent: 2023年1月8日 21:22
> 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 1/3] iio: adc: add imx93 adc support
>
> On Tue, 3 Jan 2023 19:43:56 +0800
> haibo.chen@xxxxxxx wrote:
>
> > From: Haibo Chen <haibo.chen@xxxxxxx>
> >
> > 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@xxxxxxx>
> 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;
> > +}
> > +
