On Tue, 7 Sep 2021 09:57:21 +0800
Cai Huoqing [off-list ref] wrote:

The NXP i.MX 8QuadXPlus SOC has a new ADC IP. These patches add
driver support for this ADC.

Signed-off-by: Cai Huoqing <redacted>

Hi,

A few minor things inline.  This is looking pretty good to me!

Jonathan

....

Not in alphabetical order.

+#include <linux/module.h>
+#include <linux/pm_runtime.h>

...

+static irqreturn_t imx8qxp_adc_isr(int irq, void *dev_id)
+{
+	struct imx8qxp_adc *adc = (struct imx8qxp_adc *)dev_id;

Never any need to explicitly cast from void * in c.

	struct imx8qpx_adc *adc = dev_id;

+
+	u32 fifo_count;
+
+	fifo_count = FIELD_GET(IMX8QXP_ADC_FCTRL_FCOUNT_MASK,
+			       readl(adc->regs + IMX8QXP_ADR_ADC_FCTRL));
+
+	if (fifo_count)
+		complete(&adc->completion);
+
+	return IRQ_HANDLED;
+}
+

...

+
+static int imx8qxp_adc_probe(struct platform_device *pdev)
+{
+	struct imx8qxp_adc *adc;
+	struct iio_dev *indio_dev;
+	struct device *dev = &pdev->dev;
+	int irq;
+	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);
+
+	irq = platform_get_irq(pdev, 0);
+	if (irq < 0)
+		return irq;
+
+	adc->clk = devm_clk_get(dev, "per");
+	if (IS_ERR(adc->clk)) {
+		ret = PTR_ERR(adc->clk);
+		dev_err(dev, "Failed getting clock, err = %d\n", ret);
+		return ret;
+	}
+
+	adc->ipg_clk = devm_clk_get(dev, "ipg");
+	if (IS_ERR(adc->ipg_clk)) {
+		ret = PTR_ERR(adc->ipg_clk);
+		dev_err(dev, "Failed getting clock, err = %d\n", ret);
+		return ret;
+	}
+
+	adc->vref = devm_regulator_get(dev, "vref");
+	if (IS_ERR(adc->vref)) {
+		ret = PTR_ERR(adc->vref);
+		dev_err(dev,

		return dev_err_probe(dev, PTR_ERR(adc->vref), "Failed getting reference voltage\n");

as might be a deferred case and then we don't want errors filling up the log.
This function also deals with reporting error codes etc for you.
Same for anything else that might be deferred because of driver load ordering
(clks for example)

+			"Failed getting reference voltage, err = %d\n", ret);
+		return ret;
+	}
+
+	platform_set_drvdata(pdev, indio_dev);
+
+	init_completion(&adc->completion);
+
+	indio_dev->name = dev_name(dev);
+	indio_dev->info = &imx8qxp_adc_iio_info;
+	indio_dev->modes = INDIO_DIRECT_MODE;
+	indio_dev->channels = imx8qxp_adc_iio_channels;
+	indio_dev->num_channels = ARRAY_SIZE(imx8qxp_adc_iio_channels);
+
+	ret = devm_request_irq(dev, irq, imx8qxp_adc_isr, 0, dev_name(dev), adc);
+	if (ret < 0) {
+		dev_err(dev, "Failed requesting irq, irq = %d\n", irq);
+		return ret;
+	}
+
+	imx8qxp_adc_reset(adc);
+
+	ret = iio_device_register(indio_dev);
+	if (ret) {
+		imx8qxp_adc_disable(adc);
+		dev_err(dev, "Couldn't register the device.\n");
+		return ret;
+	}
+
+	pm_runtime_set_active(dev);
+	pm_runtime_set_autosuspend_delay(dev, 50);
+	pm_runtime_use_autosuspend(dev);
+	pm_runtime_enable(dev);
+
+	return 0;
+}
+

...

+static int imx8qxp_adc_runtime_resume(struct device *dev)
+{
+	struct iio_dev *indio_dev = dev_get_drvdata(dev);
+	struct imx8qxp_adc *adc = iio_priv(indio_dev);
+	int ret;
+
+	ret = regulator_enable(adc->vref);
+	if (ret) {
+		dev_err(dev, "Can't enable adc reference top voltage, err = %d\n", ret);
+		return ret;
+	}
+
+	ret = clk_prepare_enable(adc->clk);
+	if (ret) {
+		dev_err(dev, "Could not prepare or enable clock.\n");
+		goto err_disable_reg;
+	}
+
+	ret = clk_prepare_enable(adc->ipg_clk);
+	if (ret) {
+		dev_err(dev, "Could not prepare or enable clock.\n");
+		goto err_unprepare_clk;
+	}

Blank lines here (to separate statement + error handler from next statement)

+	imx8qxp_adc_reset(adc);

here

+	return 0;

and here would help readability (slightly!)

+err_unprepare_clk:
+	clk_disable_unprepare(adc->clk);
+
+err_disable_reg:
+	regulator_disable(adc->vref);
+
+	return ret;
+}
+

...