RE: [PATCH v4 1/3] iio: adc: add imx93 adc support

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

 



> -----Original Message-----
> From: Jonathan Cameron <jic23@xxxxxxxxxx>
> Sent: 2022年12月31日 3:16
> 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 v4 1/3] iio: adc: add imx93 adc support
> 
> On Mon, 26 Dec 2022 12:27:17 +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 think there are still improvements to be made in ordering in probe()/remove()
> and also you aren't calling pm_runtime_dont_use_autosuspend() which is a
> requirement if manually handling runtime pm disabling on remove()

Hi Jonathan,

Thanks for your patience and careful review, I will improve this driver according to your suggestion.

Best Regards
Haibo Chen
> 
> Jonathan
> 
> > ---
> >
> > diff --git a/drivers/iio/adc/imx93_adc.c b/drivers/iio/adc/imx93_adc.c
> > new file mode 100644 index 000000000000..677f13a040f8
> > --- /dev/null
> > +++ b/drivers/iio/adc/imx93_adc.c
> > @@ -0,0 +1,477 @@
> 
> 
> 
> > +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) {
> > +		dev_err(dev, "Failed allocating iio device\n");
> > +		return -ENOMEM;
> > +	}
> > +
> > +	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 PTR_ERR(adc->regs);
> > +
> > +	/* 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) {
> > +		dev_err(dev, "Can't enable adc reference top voltage.\n");
> 
> You can use dev_err_probe() for all such handling in probe() whether or not it
> can defer.  That tends to simplify things and avoids the need for reviewers to
> consider if a function can defer of not.
> 
> > +		return ret;
> > +	}
> > +
> > +	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(&pdev->dev, "Could not prepare or enable the 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(dev, "Failed requesting irq, irq = %d\n", adc->irq);
> > +		goto error_ipg_clk_disable;
> > +	}
> > +
> > +	ret = imx93_adc_calibration(adc);
> > +	if (ret < 0)
> > +		goto error_free_adc_irq;
> > +
> > +	imx93_adc_config_ad_clk(adc);
> > +
> > +	ret = iio_device_register(indio_dev);
> > +	if (ret) {
> > +		dev_err(dev, "Couldn't register the 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:
> > +	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;
> > +
> > +	/* adc power down need clock on */
> > +	pm_runtime_get_sync(dev);
> > +
> > +	iio_device_unregister(indio_dev);
> > +	imx93_adc_power_down(adc);
> 
> Why is there no similar power down in the error path in probe for
> iio_device_register() returning an error?
> 
> > +	free_irq(adc->irq, adc);
> > +	clk_disable_unprepare(adc->ipg_clk);
> > +	regulator_disable(adc->vref);
> > +
> > +	pm_runtime_disable(dev);
> > +	pm_runtime_put_noidle(dev);
> 
> I think I caused confusion a bit here by pointing out the device unregister
> needed to be first. That's now fine, but the rest would benefit from a rethink.
> To my mind, the ideal situation is that the remove() is a reverse of the probe()
> function, so I'd expect to see these pm_runtime_disable(),
> pm_runtime_put_noidle() at the start of this function.  Note that you also
> need to call pm_runtime_dont_use_autosuspend() somewhere in here - or take
> all the probe/remove devm_ managed and use
> devm_pm_runtime_enable() which tidies that up for you as needed.
> (see docs in pm_runtime.h)
> 
> > +
> > +	return 0;
> > +}





[Index of Archives]     [Device Tree Compilter]     [Device Tree Spec]     [Linux Driver Backports]     [Video for Linux]     [Linux USB Devel]     [Linux PCI Devel]     [Linux Audio Users]     [Linux Kernel]     [Linux SCSI]     [XFree86]     [Yosemite Backpacking]


  Powered by Linux