[PATCH v5 0/3] add imx93 adc support

[PATCH v5 0/3] add imx93 adc support

[haibo.chen]

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

-- 
2.34.1

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

[haibo.chen]

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>
---
 MAINTAINERS                 |   4 +-
 drivers/iio/adc/Kconfig     |  10 +
 drivers/iio/adc/Makefile    |   1 +
 drivers/iio/adc/imx93_adc.c | 477 ++++++++++++++++++++++++++++++++++++
 4 files changed, 491 insertions(+), 1 deletion(-)
 create mode 100644 drivers/iio/adc/imx93_adc.c

diff --git a/MAINTAINERS b/MAINTAINERS
index 30e032abd196..37431a02cd60 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -15020,14 +15020,16 @@ S:	Maintained
 F:	Documentation/devicetree/bindings/iio/adc/nxp,imx8qxp-adc.yaml
 F:	drivers/iio/adc/imx8qxp-adc.c
 
-NXP i.MX 7D/6SX/6UL AND VF610 ADC DRIVER
+NXP i.MX 7D/6SX/6UL/93 AND VF610 ADC DRIVER
 M:	Haibo Chen <haibo.chen@nxp.com>
 L:	linux-iio@vger.kernel.org
 L:	linux-imx@nxp.com
 S:	Maintained
 F:	Documentation/devicetree/bindings/iio/adc/fsl,imx7d-adc.yaml
 F:	Documentation/devicetree/bindings/iio/adc/fsl,vf610-adc.yaml
+F:	Documentation/devicetree/bindings/iio/adc/nxp,imx93-adc.yaml
 F:	drivers/iio/adc/imx7d_adc.c
+F:	drivers/iio/adc/imx93_adc.c
 F:	drivers/iio/adc/vf610_adc.c
 
 NXP PF8100/PF8121A/PF8200 PMIC REGULATOR DEVICE DRIVER
diff --git a/drivers/iio/adc/Kconfig b/drivers/iio/adc/Kconfig
index 46c4fc2fc534..4fc826f52b8c 100644
--- a/drivers/iio/adc/Kconfig
+++ b/drivers/iio/adc/Kconfig
@@ -565,6 +565,16 @@ config IMX8QXP_ADC
 	  This driver can also be built as a module. If so, the module will be
 	  called imx8qxp-adc.
 
+config IMX93_ADC
+	tristate "IMX93 ADC driver"
+	depends on ARCH_MXC || COMPILE_TEST
+	depends on HAS_IOMEM
+	help
+	  Say yes here to build support for IMX93 ADC.
+
+	  This driver can also be built as a module. If so, the module will be
+	  called imx93_adc.
+
 config LP8788_ADC
 	tristate "LP8788 ADC driver"
 	depends on MFD_LP8788
diff --git a/drivers/iio/adc/Makefile b/drivers/iio/adc/Makefile
index 6e08415c3f3a..df8c0f26307d 100644
--- a/drivers/iio/adc/Makefile
+++ b/drivers/iio/adc/Makefile
@@ -49,6 +49,7 @@ obj-$(CONFIG_HI8435) += hi8435.o
 obj-$(CONFIG_HX711) += hx711.o
 obj-$(CONFIG_IMX7D_ADC) += imx7d_adc.o
 obj-$(CONFIG_IMX8QXP_ADC) += imx8qxp-adc.o
+obj-$(CONFIG_IMX93_ADC) += imx93_adc.o
 obj-$(CONFIG_INA2XX_ADC) += ina2xx-adc.o
 obj-$(CONFIG_INGENIC_ADC) += ingenic-adc.o
 obj-$(CONFIG_INTEL_MRFLD_ADC) += intel_mrfld_adc.o
diff --git a/drivers/iio/adc/imx93_adc.c b/drivers/iio/adc/imx93_adc.c
new file mode 100644
index 000000000000..0c98de438919
--- /dev/null
+++ b/drivers/iio/adc/imx93_adc.c
@@ -0,0 +1,477 @@
+// SPDX-License-Identifier: GPL-2.0+
+/*
+ * NXP i.MX93 ADC driver
+ *
+ * Copyright 2022 NXP
+ */
+
+#include <linux/bitfield.h>
+#include <linux/clk.h>
+#include <linux/completion.h>
+#include <linux/err.h>
+#include <linux/iio/iio.h>
+#include <linux/interrupt.h>
+#include <linux/io.h>
+#include <linux/iopoll.h>
+#include <linux/mod_devicetable.h>
+#include <linux/module.h>
+#include <linux/platform_device.h>
+#include <linux/pm_runtime.h>
+#include <linux/regulator/consumer.h>
+
+#define IMX93_ADC_DRIVER_NAME	"imx93-adc"
+
+/* Register map definition */
+#define IMX93_ADC_MCR		0x00
+#define IMX93_ADC_MSR		0x04
+#define IMX93_ADC_ISR		0x10
+#define IMX93_ADC_IMR		0x20
+#define IMX93_ADC_CIMR0		0x24
+#define IMX93_ADC_CTR0		0x94
+#define IMX93_ADC_NCMR0		0xA4
+#define IMX93_ADC_PCDR0		0x100
+#define IMX93_ADC_PCDR1		0x104
+#define IMX93_ADC_PCDR2		0x108
+#define IMX93_ADC_PCDR3		0x10c
+#define IMX93_ADC_PCDR4		0x110
+#define IMX93_ADC_PCDR5		0x114
+#define IMX93_ADC_PCDR6		0x118
+#define IMX93_ADC_PCDR7		0x11c
+#define IMX93_ADC_CALSTAT	0x39C
+
+/* ADC bit shift */
+#define IMX93_ADC_MCR_MODE_MASK			BIT(29)
+#define IMX93_ADC_MCR_NSTART_MASK		BIT(24)
+#define IMX93_ADC_MCR_CALSTART_MASK		BIT(14)
+#define IMX93_ADC_MCR_ADCLKSE_MASK		BIT(8)
+#define IMX93_ADC_MCR_PWDN_MASK			BIT(0)
+#define IMX93_ADC_MSR_CALFAIL_MASK		BIT(30)
+#define IMX93_ADC_MSR_CALBUSY_MASK		BIT(29)
+#define IMX93_ADC_MSR_ADCSTATUS_MASK		GENMASK(2, 0)
+#define IMX93_ADC_ISR_ECH_MASK			BIT(0)
+#define IMX93_ADC_ISR_EOC_MASK			BIT(1)
+#define IMX93_ADC_ISR_EOC_ECH_MASK		(IMX93_ADC_ISR_EOC_MASK | \
+						 IMX93_ADC_ISR_ECH_MASK)
+#define IMX93_ADC_IMR_JEOC_MASK			BIT(3)
+#define IMX93_ADC_IMR_JECH_MASK			BIT(2)
+#define IMX93_ADC_IMR_EOC_MASK			BIT(1)
+#define IMX93_ADC_IMR_ECH_MASK			BIT(0)
+#define IMX93_ADC_PCDR_CDATA_MASK		GENMASK(11, 0)
+
+/* ADC status */
+#define IMX93_ADC_MSR_ADCSTATUS_IDLE			0
+#define IMX93_ADC_MSR_ADCSTATUS_POWER_DOWN		1
+#define IMX93_ADC_MSR_ADCSTATUS_WAIT_STATE		2
+#define IMX93_ADC_MSR_ADCSTATUS_BUSY_IN_CALIBRATION	3
+#define IMX93_ADC_MSR_ADCSTATUS_SAMPLE			4
+#define IMX93_ADC_MSR_ADCSTATUS_CONVERSION		6
+
+#define IMX93_ADC_TIMEOUT		msecs_to_jiffies(100)
+
+struct imx93_adc {
+	struct device *dev;
+	void __iomem *regs;
+	struct clk *ipg_clk;
+	int irq;
+	struct regulator *vref;
+	/* lock to protect against multiple access to the device */
+	struct mutex lock;
+	struct completion completion;
+};
+
+#define IMX93_ADC_CHAN(_idx) {					\
+	.type = IIO_VOLTAGE,					\
+	.indexed = 1,						\
+	.channel = (_idx),					\
+	.info_mask_separate = BIT(IIO_CHAN_INFO_RAW),		\
+	.info_mask_shared_by_type = BIT(IIO_CHAN_INFO_SCALE) |	\
+				BIT(IIO_CHAN_INFO_SAMP_FREQ),	\
+}
+
+static const struct iio_chan_spec imx93_adc_iio_channels[] = {
+	IMX93_ADC_CHAN(0),
+	IMX93_ADC_CHAN(1),
+	IMX93_ADC_CHAN(2),
+	IMX93_ADC_CHAN(3),
+};
+
+static void imx93_adc_power_down(struct imx93_adc *adc)
+{
+	u32 mcr, msr;
+	int ret;
+
+	mcr = readl(adc->regs + IMX93_ADC_MCR);
+	mcr |= FIELD_PREP(IMX93_ADC_MCR_PWDN_MASK, 1);
+	writel(mcr, adc->regs + IMX93_ADC_MCR);
+
+	ret = readl_poll_timeout(adc->regs + IMX93_ADC_MSR, msr,
+				 ((msr & IMX93_ADC_MSR_ADCSTATUS_MASK) ==
+				  IMX93_ADC_MSR_ADCSTATUS_POWER_DOWN),
+				 1, 50);
+	if (ret == -ETIMEDOUT)
+		dev_warn(adc->dev,
+			 "ADC do not in power down mode, current MSR is %x\n",
+			 msr);
+}
+
+static void imx93_adc_power_up(struct imx93_adc *adc)
+{
+	u32 mcr;
+
+	/* bring ADC out of power down state, in idle state */
+	mcr = readl(adc->regs + IMX93_ADC_MCR);
+	mcr &= ~FIELD_PREP(IMX93_ADC_MCR_PWDN_MASK, 1);
+	writel(mcr, adc->regs + IMX93_ADC_MCR);
+}
+
+static void imx93_adc_config_ad_clk(struct imx93_adc *adc)
+{
+	u32 mcr;
+
+	/* put adc in power down mode */
+	imx93_adc_power_down(adc);
+
+	/* config the AD_CLK equal to bus 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);
+}
+
+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);
+
+	/*
+	 * 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_read_channel_conversion(struct imx93_adc *adc,
+						int channel_number,
+						int *result)
+{
+	u32 channel;
+	u32 imr, mcr, pcda;
+	long ret;
+
+	reinit_completion(&adc->completion);
+
+	/* config channel mask register */
+	channel = 1 << channel_number;
+	writel(channel, adc->regs + IMX93_ADC_NCMR0);
+
+	/* TODO: can config desired sample time in CTRn if need */
+
+	/* config interrupt mask */
+	imr = FIELD_PREP(IMX93_ADC_IMR_EOC_MASK, 1);
+	writel(imr, adc->regs + IMX93_ADC_IMR);
+	writel(channel, adc->regs + IMX93_ADC_CIMR0);
+
+	/* config one-shot mode */
+	mcr = readl(adc->regs + IMX93_ADC_MCR);
+	mcr &= ~FIELD_PREP(IMX93_ADC_MCR_MODE_MASK, 1);
+	writel(mcr, adc->regs + IMX93_ADC_MCR);
+
+	/* start normal conversion */
+	mcr = readl(adc->regs + IMX93_ADC_MCR);
+	mcr |= FIELD_PREP(IMX93_ADC_MCR_NSTART_MASK, 1);
+	writel(mcr, adc->regs + IMX93_ADC_MCR);
+
+	ret = wait_for_completion_interruptible_timeout(&adc->completion,
+							IMX93_ADC_TIMEOUT);
+	if (ret == 0)
+		return -ETIMEDOUT;
+
+	if (ret < 0)
+		return ret;
+
+	pcda = readl(adc->regs + IMX93_ADC_PCDR0 + channel_number * 4);
+
+	*result = FIELD_GET(IMX93_ADC_PCDR_CDATA_MASK, pcda);
+
+	return ret;
+}
+
+static int imx93_adc_read_raw(struct iio_dev *indio_dev,
+				struct iio_chan_spec const *chan,
+				int *val, int *val2, long mask)
+{
+	struct imx93_adc *adc = iio_priv(indio_dev);
+	struct device *dev = adc->dev;
+	long ret;
+	u32 vref_uv;
+
+	switch (mask) {
+	case IIO_CHAN_INFO_RAW:
+		pm_runtime_get_sync(dev);
+		mutex_lock(&adc->lock);
+		ret = imx93_adc_read_channel_conversion(adc, chan->channel, val);
+		mutex_unlock(&adc->lock);
+		pm_runtime_mark_last_busy(dev);
+		pm_runtime_put_sync_autosuspend(dev);
+		if (ret < 0)
+			return ret;
+
+		return IIO_VAL_INT;
+
+	case IIO_CHAN_INFO_SCALE:
+		ret = vref_uv = regulator_get_voltage(adc->vref);
+		if (ret < 0)
+			return ret;
+		*val = vref_uv / 1000;
+		*val2 = 12;
+		return IIO_VAL_FRACTIONAL_LOG2;
+
+	case IIO_CHAN_INFO_SAMP_FREQ:
+		*val = clk_get_rate(adc->ipg_clk);
+		return IIO_VAL_INT;
+
+	default:
+		return -EINVAL;
+	}
+}
+
+static irqreturn_t imx93_adc_isr(int irq, void *dev_id)
+{
+	struct imx93_adc *adc = dev_id;
+	u32 isr, eoc, unexpected;
+
+	isr = readl(adc->regs + IMX93_ADC_ISR);
+
+	if (FIELD_GET(IMX93_ADC_ISR_EOC_ECH_MASK, isr)) {
+		eoc = isr & IMX93_ADC_ISR_EOC_ECH_MASK;
+		writel(eoc, adc->regs + IMX93_ADC_ISR);
+		complete(&adc->completion);
+	}
+
+	unexpected = isr & ~IMX93_ADC_ISR_EOC_ECH_MASK;
+	if (unexpected) {
+		writel(unexpected, adc->regs + IMX93_ADC_ISR);
+		dev_err(adc->dev, "Unexpected interrupt 0x%08x.\n", unexpected);
+		return IRQ_NONE;
+	}
+
+	return IRQ_HANDLED;
+}
+
+static const struct iio_info imx93_adc_iio_info = {
+	.read_raw = &imx93_adc_read_raw,
+};
+
+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)
+		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;
+
+	pm_runtime_disable(dev);
+	pm_runtime_dont_use_autosuspend(dev);
+	pm_runtime_put_noidle(dev);
+	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;
+}
+
+static int imx93_adc_runtime_suspend(struct device *dev)
+{
+	struct iio_dev *indio_dev = dev_get_drvdata(dev);
+	struct imx93_adc *adc = iio_priv(indio_dev);
+
+	imx93_adc_power_down(adc);
+	clk_disable_unprepare(adc->ipg_clk);
+	regulator_disable(adc->vref);
+
+	return 0;
+}
+
+static int imx93_adc_runtime_resume(struct device *dev)
+{
+	struct iio_dev *indio_dev = dev_get_drvdata(dev);
+	struct imx93_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->ipg_clk);
+	if (ret) {
+		dev_err(dev, "Could not prepare or enable clock.\n");
+		goto err_disable_reg;
+	}
+
+	imx93_adc_power_up(adc);
+
+	return 0;
+
+err_disable_reg:
+	regulator_disable(adc->vref);
+
+	return ret;
+}
+
+static DEFINE_RUNTIME_DEV_PM_OPS(imx93_adc_pm_ops,
+				 imx93_adc_runtime_suspend,
+				 imx93_adc_runtime_resume, NULL);
+
+static const struct of_device_id imx93_adc_match[] = {
+	{ .compatible = "nxp,imx93-adc", },
+	{ /* sentinel */ }
+};
+MODULE_DEVICE_TABLE(of, imx93_adc_match);
+
+static struct platform_driver imx93_adc_driver = {
+	.probe		= imx93_adc_probe,
+	.remove		= imx93_adc_remove,
+	.driver		= {
+		.name	= IMX93_ADC_DRIVER_NAME,
+		.of_match_table = imx93_adc_match,
+		.pm	= pm_ptr(&imx93_adc_pm_ops),
+	},
+};
+
+module_platform_driver(imx93_adc_driver);
+
+MODULE_DESCRIPTION("NXP i.MX93 ADC driver");
+MODULE_AUTHOR("Haibo Chen <haibo.chen@nxp.com>");
+MODULE_LICENSE("GPL");
-- 
2.34.1

Re: [PATCH v5 1/3] iio: adc: add imx93 adc support

[Jonathan Cameron]

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;
> +}
> +

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

[Bough Chen]

> -----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;
> > +}
> > +

[PATCH v5 2/3] dt-bindings: iio: adc: Add NXP IMX93 ADC

[haibo.chen]

From: Haibo Chen <haibo.chen@nxp.com>

The IMX93 SoC has a new ADC IP, so add binding documentation
for NXP IMX93 ADC.

Reviewed-by: Rob Herring <robh@kernel.org>
Signed-off-by: Haibo Chen <haibo.chen@nxp.com>
---
 .../bindings/iio/adc/nxp,imx93-adc.yaml       | 81 +++++++++++++++++++
 1 file changed, 81 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/iio/adc/nxp,imx93-adc.yaml

diff --git a/Documentation/devicetree/bindings/iio/adc/nxp,imx93-adc.yaml b/Documentation/devicetree/bindings/iio/adc/nxp,imx93-adc.yaml
new file mode 100644
index 000000000000..dacc526dc695
--- /dev/null
+++ b/Documentation/devicetree/bindings/iio/adc/nxp,imx93-adc.yaml
@@ -0,0 +1,81 @@
+# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause)
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/iio/adc/nxp,imx93-adc.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: NXP iMX93 ADC
+
+maintainers:
+  - Haibo Chen <haibo.chen@nxp.com>
+
+description:
+  The ADC on iMX93 is a 8-channel 12-bit 1MS/s ADC with 4 channels
+  connected to pins. it support normal and inject mode, include
+  One-Shot and Scan (continuous) conversions. Programmable DMA
+  enables for each channel  Also this ADC contain alternate analog
+  watchdog thresholds, select threshold through input ports. And
+  also has Self-test logic and Software-initiated calibration.
+
+properties:
+  compatible:
+    const: nxp,imx93-adc
+
+  reg:
+    maxItems: 1
+
+  interrupts:
+    items:
+      - description: WDGnL, watchdog threshold interrupt requests.
+      - description: WDGnH, watchdog threshold interrupt requests.
+      - description: normal conversion, include EOC (End of Conversion),
+          ECH (End of Chain), JEOC (End of Injected Conversion) and
+          JECH (End of injected Chain).
+      - description: Self-testing Interrupts.
+
+  clocks:
+    maxItems: 1
+
+  clock-names:
+    const: ipg
+
+  vref-supply:
+    description:
+      The reference voltage which used to establish channel scaling.
+
+  "#io-channel-cells":
+    const: 1
+
+required:
+  - compatible
+  - reg
+  - interrupts
+  - clocks
+  - clock-names
+  - vref-supply
+  - "#io-channel-cells"
+
+additionalProperties: false
+
+examples:
+  - |
+    #include <dt-bindings/interrupt-controller/irq.h>
+    #include <dt-bindings/clock/imx93-clock.h>
+    #include <dt-bindings/interrupt-controller/arm-gic.h>
+    soc {
+        #address-cells = <1>;
+        #size-cells = <1>;
+        adc@44530000 {
+            compatible = "nxp,imx93-adc";
+            reg = <0x44530000 0x10000>;
+            interrupts = <GIC_SPI 217 IRQ_TYPE_LEVEL_HIGH>,
+                         <GIC_SPI 218 IRQ_TYPE_LEVEL_HIGH>,
+                         <GIC_SPI 219 IRQ_TYPE_LEVEL_HIGH>,
+                         <GIC_SPI 268 IRQ_TYPE_LEVEL_HIGH>;
+            clocks = <&clk IMX93_CLK_ADC1_GATE>;
+            clock-names = "ipg";
+            vref-supply = <&reg_vref_1v8>;
+            #io-channel-cells = <1>;
+        };
+    };
+...
-- 
2.34.1

[PATCH v5 3/3] arm64: dts: imx93: add ADC support

[haibo.chen]

From: Haibo Chen <haibo.chen@nxp.com>

Add ADC support for imx93-11x11-evk board.

Signed-off-by: Haibo Chen <haibo.chen@nxp.com>
---
 arch/arm64/boot/dts/freescale/imx93-11x11-evk.dts | 12 ++++++++++++
 arch/arm64/boot/dts/freescale/imx93.dtsi          | 13 +++++++++++++
 2 files changed, 25 insertions(+)

diff --git a/arch/arm64/boot/dts/freescale/imx93-11x11-evk.dts b/arch/arm64/boot/dts/freescale/imx93-11x11-evk.dts
index 69786c326db0..cab5f4d66bf9 100644
--- a/arch/arm64/boot/dts/freescale/imx93-11x11-evk.dts
+++ b/arch/arm64/boot/dts/freescale/imx93-11x11-evk.dts
@@ -15,6 +15,13 @@ chosen {
 		stdout-path = &lpuart1;
 	};
 
+	reg_vref_1v8: regulator-adc-vref {
+		compatible = "regulator-fixed";
+		regulator-name = "vref_1v8";
+		regulator-min-microvolt = <1800000>;
+		regulator-max-microvolt = <1800000>;
+	};
+
 	reg_usdhc2_vmmc: regulator-usdhc2 {
 		compatible = "regulator-fixed";
 		pinctrl-names = "default";
@@ -27,6 +34,11 @@ reg_usdhc2_vmmc: regulator-usdhc2 {
 	};
 };
 
+&adc1 {
+	vref-supply = <&reg_vref_1v8>;
+	status = "okay";
+};
+
 &mu1 {
 	status = "okay";
 };
diff --git a/arch/arm64/boot/dts/freescale/imx93.dtsi b/arch/arm64/boot/dts/freescale/imx93.dtsi
index 5d79663b3b84..a0d39dd34591 100644
--- a/arch/arm64/boot/dts/freescale/imx93.dtsi
+++ b/arch/arm64/boot/dts/freescale/imx93.dtsi
@@ -266,6 +266,19 @@ anatop: anatop@44480000 {
 				compatible = "fsl,imx93-anatop", "syscon";
 				reg = <0x44480000 0x10000>;
 			};
+
+			adc1: adc@44530000 {
+				compatible = "nxp,imx93-adc";
+				reg = <0x44530000 0x10000>;
+				interrupts = <GIC_SPI 217 IRQ_TYPE_LEVEL_HIGH>,
+					     <GIC_SPI 218 IRQ_TYPE_LEVEL_HIGH>,
+					     <GIC_SPI 219 IRQ_TYPE_LEVEL_HIGH>,
+					     <GIC_SPI 268 IRQ_TYPE_LEVEL_HIGH>;
+				clocks = <&clk IMX93_CLK_ADC1_GATE>;
+				clock-names = "ipg";
+				#io-channel-cells = <1>;
+				status = "disabled";
+			};
 		};
 
 		aips2: bus@42000000 {
-- 
2.34.1

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

[Jonathan Cameron]

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
> 

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

[Bough Chen]

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

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

[Jonathan Cameron]

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