[RFC PATCH v1 0/4] Add support for Allwinner GPADC on D1/T113s/R329 SoCs

[RFC PATCH v1 0/4] Add support for Allwinner GPADC on D1/T113s/R329 SoCs

[Maxim Kiselev]

Hi,

This series adds support for general purpose ADC (GPADC) on new
Allwinner's SoCs, such as D1, T113s and R329. The implemented driver
provides basic functionality for getting ADC channels data.

All of the listed SoCs have the same IP. The only difference is the number
of available channels:
     T113 - 1 channel
     D1   - 2 channels
     R329 - 4 channels

This series is just an RFC and I would be glad to see any comments
about it.


Maxim Kiselev (4):
  iio: adc: Add Allwinner D1/T113s/R329 SoCs GPADC
  dt-bindings: iio: adc: Add Allwinner D1/T113s/R329 SoCs GPADC
  ARM: dts: sun8i: t113s: Add GPADC node
  riscv: dts: allwinner: d1: Add GPADC node

 .../iio/adc/allwinner,sun20i-d1-gpadc.yaml    |  52 ++++
 arch/arm/boot/dts/sun8i-t113s.dtsi            |  12 +
 arch/riscv/boot/dts/allwinner/sun20i-d1.dtsi  |  10 +
 drivers/iio/adc/Kconfig                       |  10 +
 drivers/iio/adc/Makefile                      |   1 +
 drivers/iio/adc/sun20i-gpadc-iio.c            | 275 ++++++++++++++++++
 6 files changed, 360 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/iio/adc/allwinner,sun20i-d1-gpadc.yaml
 create mode 100644 drivers/iio/adc/sun20i-gpadc-iio.c

-- 
2.39.2

[RFC PATCH v1 1/4] iio: adc: Add Allwinner D1/T113s/R329 SoCs GPADC

[Maxim Kiselev]

The General Purpose ADC (GPADC) can convert the external signal into
a certain proportion of digital value, to realize the measurement of
analog signal, which can be applied to power detection and key detection.

D1, T113s and R329 contain this GPADC IP. The only difference between
this SoCs is the number of available channels:

 T113 - 1 channel
 D1   - 2 channels
 R329 - 4 channels

Signed-off-by: Maxim Kiselev <bigunclemax@gmail.com>
---
 drivers/iio/adc/Kconfig            |  10 ++
 drivers/iio/adc/Makefile           |   1 +
 drivers/iio/adc/sun20i-gpadc-iio.c | 275 +++++++++++++++++++++++++++++
 3 files changed, 286 insertions(+)
 create mode 100644 drivers/iio/adc/sun20i-gpadc-iio.c

diff --git a/drivers/iio/adc/Kconfig b/drivers/iio/adc/Kconfig
index eb2b09ef5d5b..988804f46bf6 100644
--- a/drivers/iio/adc/Kconfig
+++ b/drivers/iio/adc/Kconfig
@@ -1123,6 +1123,16 @@ config SUN4I_GPADC
 	  To compile this driver as a module, choose M here: the module will be
 	  called sun4i-gpadc-iio.
 
+config SUN20I_GPADC
+	tristate "Support for the Allwinner SoCs GPADC"
+	depends on ARCH_SUNXI || COMPILE_TEST
+	help
+	  Say yes here to build support for Allwinner (D1, T113 and R329) SoCs
+	  GPADC. This ADC provides up to 4 channels.
+
+	  To compile this driver as a module, choose M here: the module will be
+	  called sun20i-gpadc-iio.
+
 config TI_ADC081C
 	tristate "Texas Instruments ADC081C/ADC101C/ADC121C family"
 	depends on I2C
diff --git a/drivers/iio/adc/Makefile b/drivers/iio/adc/Makefile
index e07e4a3e6237..fc4ef71d5f8f 100644
--- a/drivers/iio/adc/Makefile
+++ b/drivers/iio/adc/Makefile
@@ -95,6 +95,7 @@ obj-$(CONFIG_RZG2L_ADC) += rzg2l_adc.o
 obj-$(CONFIG_SC27XX_ADC) += sc27xx_adc.o
 obj-$(CONFIG_SPEAR_ADC) += spear_adc.o
 obj-$(CONFIG_SUN4I_GPADC) += sun4i-gpadc-iio.o
+obj-$(CONFIG_SUN20I_GPADC) += sun20i-gpadc-iio.o
 obj-$(CONFIG_STM32_ADC_CORE) += stm32-adc-core.o
 obj-$(CONFIG_STM32_ADC) += stm32-adc.o
 obj-$(CONFIG_STM32_DFSDM_CORE) += stm32-dfsdm-core.o
diff --git a/drivers/iio/adc/sun20i-gpadc-iio.c b/drivers/iio/adc/sun20i-gpadc-iio.c
new file mode 100644
index 000000000000..90f3bb2e41cd
--- /dev/null
+++ b/drivers/iio/adc/sun20i-gpadc-iio.c
@@ -0,0 +1,275 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * GPADC driver for sunxi platforms (D1, T113-S3 and R329)
+ * Copyright (c) 2023 Maksim Kiselev <bigunclemax@gmail.com>
+ */
+
+#include <linux/completion.h>
+#include <linux/interrupt.h>
+#include <linux/io.h>
+#include <linux/module.h>
+#include <linux/of.h>
+#include <linux/of_device.h>
+#include <linux/clk.h>
+#include <linux/reset.h>
+#include <linux/platform_device.h>
+#include <linux/regmap.h>
+#include <linux/iio/iio.h>
+
+#define SUN20I_GPADC_SR					0x00
+
+#define SUN20I_GPADC_CTRL				0x04
+#define SUN20I_GPADC_CTRL_ADC_FIRST_DLY			((GENMASK(7, 0) & (x)) << 24)
+#define SUN20I_GPADC_CTRL_ADC_AUTOCALI_EN		BIT(23)
+#define SUN20I_GPADC_CTRL_ADC_OP_BIAS			((GENMASK(1, 0) & (x)) << 20)
+#define SUN20I_GPADC_CTRL_WORK_MODE_SELECT(x)		((GENMASK(1, 0) & (x)) << 18)
+#define SUN20I_GPADC_CTRL_ADC_CALI_EN			BIT(17)
+#define SUN20I_GPADC_CTRL_ADC_EN			BIT(16)
+
+#define SUN20I_GPADC_CS_EN				0x08
+#define SUN20I_GPADC_CS_EN_ADC_CHAN_CMP_EN(x)		((GENMASK(16, 0) & BIT(x)) << 16)
+#define SUN20I_GPADC_CS_EN_ADC_CHAN_SELECT(x)		(GENMASK(16, 0) & BIT(x))
+
+#define SUN20I_GPADC_DATA_INTC				0x28
+#define SUN20I_GPADC_DATA_INTC_CHAN_DATA_IRQ_EN(x)	(GENMASK(16, 0) & BIT(x))
+
+#define SUN20I_GPADC_DATA_INTS				0x38
+#define SUN20I_GPADC_DATA_INTS_DATA_PENDING(x)		(GENMASK(16, 0) & BIT(x))
+#define SUN20I_GPADC_DATA_INTS_DATA_PENDING_MASK	GENMASK(16, 0)
+
+#define SUN20I_GPADC_CHAN_DATA(x)			(0x80 + (x) * 4)
+
+#define SUN20I_GPADC_TIMEOUT				msecs_to_jiffies(100)
+#define SUN20I_GPADC_MAX_CHANNELS			16
+
+struct sun20i_gpadc_configuration {
+	const struct iio_chan_spec	*channels;
+	u8				num_channels;
+};
+
+struct sun20i_gpadc_iio {
+	struct iio_dev			*indio_dev;
+	struct regmap			*regmap;
+	struct completion		completion;
+	struct mutex			lock;
+};
+
+#define SUN20I_GPADC_ADC_CHANNEL(_channel) {			\
+	.type = IIO_VOLTAGE,					\
+	.indexed = 1,						\
+	.channel = _channel,					\
+	.info_mask_separate = BIT(IIO_CHAN_INFO_RAW),		\
+	.info_mask_shared_by_type = BIT(IIO_CHAN_INFO_SCALE),	\
+}
+
+static const struct iio_chan_spec sun8i_t113_gpadc_channels[] = {
+	SUN20I_GPADC_ADC_CHANNEL(0),
+};
+
+static const struct iio_chan_spec sun20i_d1_gpadc_channels[] = {
+	SUN20I_GPADC_ADC_CHANNEL(0),
+	SUN20I_GPADC_ADC_CHANNEL(1),
+};
+
+static const struct iio_chan_spec sun50i_r329_gpadc_channels[] = {
+	SUN20I_GPADC_ADC_CHANNEL(0),
+	SUN20I_GPADC_ADC_CHANNEL(1),
+	SUN20I_GPADC_ADC_CHANNEL(2),
+	SUN20I_GPADC_ADC_CHANNEL(3),
+};
+
+static const struct sun20i_gpadc_configuration sun20i_gpadc_config[] = {
+	{ sun8i_t113_gpadc_channels, ARRAY_SIZE(sun8i_t113_gpadc_channels) },
+	{ sun20i_d1_gpadc_channels, ARRAY_SIZE(sun20i_d1_gpadc_channels) },
+	{ sun50i_r329_gpadc_channels, ARRAY_SIZE(sun50i_r329_gpadc_channels) },
+};
+
+static const struct regmap_config sun20i_gpadc_regmap_config = {
+	.reg_bits = 32,
+	.val_bits = 32,
+	.reg_stride = 4,
+	.fast_io = true,
+};
+
+static int sun20i_gpadc_adc_read(struct sun20i_gpadc_iio *info,
+				 struct iio_chan_spec const *chan, int *val)
+{
+	reinit_completion(&info->completion);
+
+	/* enable the analog input channel */
+	regmap_write(info->regmap, SUN20I_GPADC_CS_EN,
+		     SUN20I_GPADC_CS_EN_ADC_CHAN_SELECT(chan->channel));
+
+	/* enable the data irq for input channel */
+	regmap_write(info->regmap, SUN20I_GPADC_DATA_INTC,
+		     SUN20I_GPADC_DATA_INTC_CHAN_DATA_IRQ_EN(chan->channel));
+
+	/* enable the ADC function */
+	regmap_update_bits(info->regmap, SUN20I_GPADC_CTRL,
+			   SUN20I_GPADC_CTRL_ADC_EN, SUN20I_GPADC_CTRL_ADC_EN);
+
+	if (!wait_for_completion_timeout(&info->completion,
+					 SUN20I_GPADC_TIMEOUT))
+		return -ETIMEDOUT;
+
+	/* read the ADC data */
+	regmap_read(info->regmap, SUN20I_GPADC_CHAN_DATA(chan->channel), val);
+
+	return IIO_VAL_INT;
+}
+
+static int sun20i_gpadc_read_raw(struct iio_dev *indio_dev,
+				 struct iio_chan_spec const *chan, int *val,
+				 int *val2, long mask)
+{
+	struct sun20i_gpadc_iio *info = iio_priv(indio_dev);
+	int ret;
+
+	switch (mask) {
+	case IIO_CHAN_INFO_RAW:
+		mutex_lock(&info->lock);
+		ret = sun20i_gpadc_adc_read(info, chan, val);
+		mutex_unlock(&info->lock);
+		return ret;
+	case IIO_CHAN_INFO_SCALE:
+		/* 1800mV / 4096 * raw */
+		*val = 0;
+		*val2 = 439453125;
+		return IIO_VAL_INT_PLUS_NANO;
+	default:
+		return -EINVAL;
+	}
+}
+
+static irqreturn_t sun20i_gpadc_irq_handler(int irq, void *data)
+{
+	struct sun20i_gpadc_iio *info = data;
+	u32 reg;
+
+	/* clear data interrupt status register */
+	regmap_read(info->regmap, SUN20I_GPADC_DATA_INTS, &reg);
+	regmap_write(info->regmap, SUN20I_GPADC_DATA_INTS, reg);
+
+	complete(&info->completion);
+
+	return IRQ_HANDLED;
+}
+
+static const struct iio_info sun20i_gpadc_iio_info = {
+	.read_raw = sun20i_gpadc_read_raw,
+};
+
+static int sun20i_gpadc_probe(struct platform_device *pdev)
+{
+	const struct sun20i_gpadc_configuration *config;
+	struct iio_dev *indio_dev;
+	struct sun20i_gpadc_iio *info;
+	struct reset_control *rst;
+	void __iomem *base;
+	struct clk *clk;
+	int irq;
+	int ret;
+
+	indio_dev = devm_iio_device_alloc(&pdev->dev, sizeof(*info));
+	if (!indio_dev)
+		return -ENOMEM;
+
+	info = iio_priv(indio_dev);
+	platform_set_drvdata(pdev, indio_dev);
+
+	mutex_init(&info->lock);
+	init_completion(&info->completion);
+
+	info->indio_dev = indio_dev;
+	indio_dev->info = &sun20i_gpadc_iio_info;
+	indio_dev->name = dev_name(&pdev->dev);
+
+	config = of_device_get_match_data(&pdev->dev);
+	if (!config)
+		return -ENODEV;
+
+	/* Sanity check for possible later IP variants with more channels */
+	if (config->num_channels > SUN20I_GPADC_MAX_CHANNELS)
+		return dev_err_probe(&pdev->dev, -EINVAL,
+				     "max channels exceeded\n");
+
+	indio_dev->channels = config->channels;
+	indio_dev->num_channels = config->num_channels;
+
+	base = devm_platform_ioremap_resource(pdev, 0);
+	if (IS_ERR(base))
+		return PTR_ERR(base);
+
+	info->regmap = devm_regmap_init_mmio(&pdev->dev, base,
+					     &sun20i_gpadc_regmap_config);
+	if (IS_ERR(info->regmap))
+		return dev_err_probe(&pdev->dev, PTR_ERR(info->regmap),
+				     "failed to init regmap\n");
+
+	clk = devm_clk_get_enabled(&pdev->dev, NULL);
+	if (IS_ERR(clk))
+		return dev_err_probe(&pdev->dev, PTR_ERR(clk),
+				     "failed to enable bus clock\n");
+
+	rst = devm_reset_control_get_exclusive(&pdev->dev, NULL);
+	if (IS_ERR(rst))
+		return dev_err_probe(&pdev->dev, PTR_ERR(rst),
+				     "failed to get reset control\n");
+
+	ret = reset_control_deassert(rst);
+	if (ret)
+		return dev_err_probe(&pdev->dev, ret,
+				    "failed to deassert reset\n");
+
+	irq = platform_get_irq(pdev, 0);
+	if (irq < 0)
+		return dev_err_probe(&pdev->dev, irq, "failed to get irq\n");
+
+	ret = devm_request_irq(&pdev->dev, irq, sun20i_gpadc_irq_handler,
+			       0, dev_name(&pdev->dev), info);
+	if (ret < 0)
+		return dev_err_probe(&pdev->dev, ret,
+				     "failed requesting irq %d\n", irq);
+
+	/* set the single conversion mode and enable auto calibration*/
+	regmap_write(info->regmap, SUN20I_GPADC_CTRL,
+		     SUN20I_GPADC_CTRL_ADC_AUTOCALI_EN |
+		     SUN20I_GPADC_CTRL_WORK_MODE_SELECT(0));
+
+	ret = devm_iio_device_register(&pdev->dev, indio_dev);
+	if (ret < 0)
+		return dev_err_probe(&pdev->dev, ret,
+				     "could not register the device\n");
+
+	return 0;
+}
+
+static const struct of_device_id sun20i_gpadc_of_id[] = {
+	{
+		.compatible = "allwinner,sun8i-t113-gpadc",
+		.data = &sun20i_gpadc_config[0],
+	},
+	{
+		.compatible = "allwinner,sun20i-d1-gpadc",
+		.data = &sun20i_d1_gpadc_channels[1]
+	},
+	{
+		.compatible = "allwinner,sun50i-r329-gpadc",
+		.data = &sun50i_r329_gpadc_channels[2]
+	},
+	{ /* sentinel */ }
+};
+MODULE_DEVICE_TABLE(of, sun20i_gpadc_of_id);
+
+static struct platform_driver sun20i_gpadc_driver = {
+	.driver = {
+		.name = "sun20i-gpadc-iio",
+		.of_match_table = sun20i_gpadc_of_id,
+	},
+	.probe = sun20i_gpadc_probe,
+};
+module_platform_driver(sun20i_gpadc_driver);
+
+MODULE_DESCRIPTION("ADC driver for sunxi platforms");
+MODULE_AUTHOR("Maksim Kiselev <bigunclemax@gmail.com>");
+MODULE_LICENSE("GPL");
-- 
2.39.2

Re: [RFC PATCH v1 1/4] iio: adc: Add Allwinner D1/T113s/R329 SoCs GPADC

[Andre Przywara]

On Wed, 24 May 2023 11:27:30 +0300
Maxim Kiselev <bigunclemax@gmail.com> wrote:

Hi Maxim,

> The General Purpose ADC (GPADC) can convert the external signal into
> a certain proportion of digital value, to realize the measurement of
> analog signal, which can be applied to power detection and key detection.
> 
> D1, T113s and R329 contain this GPADC IP. The only difference between
> this SoCs is the number of available channels:
> 
>  T113 - 1 channel
>  D1   - 2 channels
>  R329 - 4 channels
> 
> Signed-off-by: Maxim Kiselev <bigunclemax@gmail.com>
> ---
>  drivers/iio/adc/Kconfig            |  10 ++
>  drivers/iio/adc/Makefile           |   1 +
>  drivers/iio/adc/sun20i-gpadc-iio.c | 275 +++++++++++++++++++++++++++++
>  3 files changed, 286 insertions(+)
>  create mode 100644 drivers/iio/adc/sun20i-gpadc-iio.c
> 
> diff --git a/drivers/iio/adc/Kconfig b/drivers/iio/adc/Kconfig
> index eb2b09ef5d5b..988804f46bf6 100644
> --- a/drivers/iio/adc/Kconfig
> +++ b/drivers/iio/adc/Kconfig
> @@ -1123,6 +1123,16 @@ config SUN4I_GPADC
>  	  To compile this driver as a module, choose M here: the module will be
>  	  called sun4i-gpadc-iio.
>  
> +config SUN20I_GPADC
> +	tristate "Support for the Allwinner SoCs GPADC"
> +	depends on ARCH_SUNXI || COMPILE_TEST
> +	help
> +	  Say yes here to build support for Allwinner (D1, T113 and R329) SoCs
> +	  GPADC. This ADC provides up to 4 channels.
> +
> +	  To compile this driver as a module, choose M here: the module will be
> +	  called sun20i-gpadc-iio.
> +
>  config TI_ADC081C
>  	tristate "Texas Instruments ADC081C/ADC101C/ADC121C family"
>  	depends on I2C
> diff --git a/drivers/iio/adc/Makefile b/drivers/iio/adc/Makefile
> index e07e4a3e6237..fc4ef71d5f8f 100644
> --- a/drivers/iio/adc/Makefile
> +++ b/drivers/iio/adc/Makefile
> @@ -95,6 +95,7 @@ obj-$(CONFIG_RZG2L_ADC) += rzg2l_adc.o
>  obj-$(CONFIG_SC27XX_ADC) += sc27xx_adc.o
>  obj-$(CONFIG_SPEAR_ADC) += spear_adc.o
>  obj-$(CONFIG_SUN4I_GPADC) += sun4i-gpadc-iio.o
> +obj-$(CONFIG_SUN20I_GPADC) += sun20i-gpadc-iio.o
>  obj-$(CONFIG_STM32_ADC_CORE) += stm32-adc-core.o
>  obj-$(CONFIG_STM32_ADC) += stm32-adc.o
>  obj-$(CONFIG_STM32_DFSDM_CORE) += stm32-dfsdm-core.o
> diff --git a/drivers/iio/adc/sun20i-gpadc-iio.c b/drivers/iio/adc/sun20i-gpadc-iio.c
> new file mode 100644
> index 000000000000..90f3bb2e41cd
> --- /dev/null
> +++ b/drivers/iio/adc/sun20i-gpadc-iio.c
> @@ -0,0 +1,275 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * GPADC driver for sunxi platforms (D1, T113-S3 and R329)
> + * Copyright (c) 2023 Maksim Kiselev <bigunclemax@gmail.com>
> + */
> +
> +#include <linux/completion.h>
> +#include <linux/interrupt.h>
> +#include <linux/io.h>
> +#include <linux/module.h>
> +#include <linux/of.h>
> +#include <linux/of_device.h>
> +#include <linux/clk.h>
> +#include <linux/reset.h>
> +#include <linux/platform_device.h>
> +#include <linux/regmap.h>
> +#include <linux/iio/iio.h>
> +
> +#define SUN20I_GPADC_SR					0x00
> +
> +#define SUN20I_GPADC_CTRL				0x04
> +#define SUN20I_GPADC_CTRL_ADC_FIRST_DLY			((GENMASK(7, 0) & (x)) << 24)
> +#define SUN20I_GPADC_CTRL_ADC_AUTOCALI_EN		BIT(23)
> +#define SUN20I_GPADC_CTRL_ADC_OP_BIAS			((GENMASK(1, 0) & (x)) << 20)
> +#define SUN20I_GPADC_CTRL_WORK_MODE_SELECT(x)		((GENMASK(1, 0) & (x)) << 18)
> +#define SUN20I_GPADC_CTRL_ADC_CALI_EN			BIT(17)
> +#define SUN20I_GPADC_CTRL_ADC_EN			BIT(16)
> +
> +#define SUN20I_GPADC_CS_EN				0x08
> +#define SUN20I_GPADC_CS_EN_ADC_CHAN_CMP_EN(x)		((GENMASK(16, 0) & BIT(x)) << 16)
> +#define SUN20I_GPADC_CS_EN_ADC_CHAN_SELECT(x)		(GENMASK(16, 0) & BIT(x))
> +
> +#define SUN20I_GPADC_DATA_INTC				0x28
> +#define SUN20I_GPADC_DATA_INTC_CHAN_DATA_IRQ_EN(x)	(GENMASK(16, 0) & BIT(x))
> +
> +#define SUN20I_GPADC_DATA_INTS				0x38
> +#define SUN20I_GPADC_DATA_INTS_DATA_PENDING(x)		(GENMASK(16, 0) & BIT(x))
> +#define SUN20I_GPADC_DATA_INTS_DATA_PENDING_MASK	GENMASK(16, 0)
> +
> +#define SUN20I_GPADC_CHAN_DATA(x)			(0x80 + (x) * 4)
> +
> +#define SUN20I_GPADC_TIMEOUT				msecs_to_jiffies(100)
> +#define SUN20I_GPADC_MAX_CHANNELS			16
> +
> +struct sun20i_gpadc_configuration {
> +	const struct iio_chan_spec	*channels;
> +	u8				num_channels;
> +};
> +
> +struct sun20i_gpadc_iio {
> +	struct iio_dev			*indio_dev;
> +	struct regmap			*regmap;
> +	struct completion		completion;
> +	struct mutex			lock;
> +};
> +
> +#define SUN20I_GPADC_ADC_CHANNEL(_channel) {			\
> +	.type = IIO_VOLTAGE,					\
> +	.indexed = 1,						\
> +	.channel = _channel,					\
> +	.info_mask_separate = BIT(IIO_CHAN_INFO_RAW),		\
> +	.info_mask_shared_by_type = BIT(IIO_CHAN_INFO_SCALE),	\
> +}
> +
> +static const struct iio_chan_spec sun8i_t113_gpadc_channels[] = {
> +	SUN20I_GPADC_ADC_CHANNEL(0),
> +};
> +
> +static const struct iio_chan_spec sun20i_d1_gpadc_channels[] = {
> +	SUN20I_GPADC_ADC_CHANNEL(0),
> +	SUN20I_GPADC_ADC_CHANNEL(1),
> +};
> +
> +static const struct iio_chan_spec sun50i_r329_gpadc_channels[] = {
> +	SUN20I_GPADC_ADC_CHANNEL(0),
> +	SUN20I_GPADC_ADC_CHANNEL(1),
> +	SUN20I_GPADC_ADC_CHANNEL(2),
> +	SUN20I_GPADC_ADC_CHANNEL(3),
> +};
> +
> +static const struct sun20i_gpadc_configuration sun20i_gpadc_config[] = {
> +	{ sun8i_t113_gpadc_channels, ARRAY_SIZE(sun8i_t113_gpadc_channels) },
> +	{ sun20i_d1_gpadc_channels, ARRAY_SIZE(sun20i_d1_gpadc_channels) },
> +	{ sun50i_r329_gpadc_channels, ARRAY_SIZE(sun50i_r329_gpadc_channels) },
> +};

It looks a bit odd to have an array here, when those are separate devices.
I think other drivers just spell those devices out, one by one:

static const struct sun20i_gpadc_configuration sun20i_d1_gpadc_config = {
	sun20i_d1_gpadc_channels, ARRAY_SIZE(sun20i_d1_gpadc_channels)
};
static const struct sun20i_gpadc_configuration sun8i_t113_gpadc_config = {
	sun8i_t113_gpadc_channels, ARRAY_SIZE(sun8i_t113_gpadc_channels)
};
....

Though I believe it would be easier to either allocate the required number
of channels at probe time, or just use one predefined array with four
channels (like the r329 version above), and then just register less for
the other SoCs, by just setting indio_dev->num_channels to the number read
from match_data.

Or we follow the other idea of reading the number of channels from a DT
property, which means we would already support every IP with up to 16
channels, without further driver changes in the future.

> +
> +static const struct regmap_config sun20i_gpadc_regmap_config = {
> +	.reg_bits = 32,
> +	.val_bits = 32,
> +	.reg_stride = 4,
> +	.fast_io = true,
> +};

Is there any particular reason you chose a regmap to model this here?
Isn't that just straight-forward MMIO, which we could just drive using
readl()/writel()?

> +static int sun20i_gpadc_adc_read(struct sun20i_gpadc_iio *info,
> +				 struct iio_chan_spec const *chan, int *val)
> +{
> +	reinit_completion(&info->completion);
> +
> +	/* enable the analog input channel */
> +	regmap_write(info->regmap, SUN20I_GPADC_CS_EN,
> +		     SUN20I_GPADC_CS_EN_ADC_CHAN_SELECT(chan->channel));
> +
> +	/* enable the data irq for input channel */
> +	regmap_write(info->regmap, SUN20I_GPADC_DATA_INTC,
> +		     SUN20I_GPADC_DATA_INTC_CHAN_DATA_IRQ_EN(chan->channel));

I wonder if this should be either moved out to some multiplexer: the DT
bindings suggest that such a thing could be independent.
But at least we could cater for the possibility that this channel is
already selected, and skip this part then?

> +
> +	/* enable the ADC function */
> +	regmap_update_bits(info->regmap, SUN20I_GPADC_CTRL,
> +			   SUN20I_GPADC_CTRL_ADC_EN, SUN20I_GPADC_CTRL_ADC_EN);
> +
> +	if (!wait_for_completion_timeout(&info->completion,
> +					 SUN20I_GPADC_TIMEOUT))
> +		return -ETIMEDOUT;
> +
> +	/* read the ADC data */
> +	regmap_read(info->regmap, SUN20I_GPADC_CHAN_DATA(chan->channel), val);
> +
> +	return IIO_VAL_INT;
> +}
> +
> +static int sun20i_gpadc_read_raw(struct iio_dev *indio_dev,
> +				 struct iio_chan_spec const *chan, int *val,
> +				 int *val2, long mask)
> +{
> +	struct sun20i_gpadc_iio *info = iio_priv(indio_dev);
> +	int ret;
> +
> +	switch (mask) {
> +	case IIO_CHAN_INFO_RAW:
> +		mutex_lock(&info->lock);

Couldn't taking the lock go into the function? It seems to be only one
caller, and we need the lock in any case, it seems?

> +		ret = sun20i_gpadc_adc_read(info, chan, val);
> +		mutex_unlock(&info->lock);
> +		return ret;
> +	case IIO_CHAN_INFO_SCALE:
> +		/* 1800mV / 4096 * raw */
> +		*val = 0;
> +		*val2 = 439453125;
> +		return IIO_VAL_INT_PLUS_NANO;
> +	default:
> +		return -EINVAL;
> +	}
> +}
> +
> +static irqreturn_t sun20i_gpadc_irq_handler(int irq, void *data)
> +{
> +	struct sun20i_gpadc_iio *info = data;
> +	u32 reg;
> +
> +	/* clear data interrupt status register */
> +	regmap_read(info->regmap, SUN20I_GPADC_DATA_INTS, &reg);
> +	regmap_write(info->regmap, SUN20I_GPADC_DATA_INTS, reg);
> +
> +	complete(&info->completion);
> +
> +	return IRQ_HANDLED;
> +}
> +
> +static const struct iio_info sun20i_gpadc_iio_info = {
> +	.read_raw = sun20i_gpadc_read_raw,
> +};
> +
> +static int sun20i_gpadc_probe(struct platform_device *pdev)
> +{
> +	const struct sun20i_gpadc_configuration *config;
> +	struct iio_dev *indio_dev;
> +	struct sun20i_gpadc_iio *info;
> +	struct reset_control *rst;
> +	void __iomem *base;
> +	struct clk *clk;
> +	int irq;
> +	int ret;
> +
> +	indio_dev = devm_iio_device_alloc(&pdev->dev, sizeof(*info));
> +	if (!indio_dev)
> +		return -ENOMEM;
> +
> +	info = iio_priv(indio_dev);
> +	platform_set_drvdata(pdev, indio_dev);
> +
> +	mutex_init(&info->lock);
> +	init_completion(&info->completion);
> +
> +	info->indio_dev = indio_dev;
> +	indio_dev->info = &sun20i_gpadc_iio_info;
> +	indio_dev->name = dev_name(&pdev->dev);
> +
> +	config = of_device_get_match_data(&pdev->dev);
> +	if (!config)
> +		return -ENODEV;
> +
> +	/* Sanity check for possible later IP variants with more channels */
> +	if (config->num_channels > SUN20I_GPADC_MAX_CHANNELS)
> +		return dev_err_probe(&pdev->dev, -EINVAL,
> +				     "max channels exceeded\n");
> +
> +	indio_dev->channels = config->channels;
> +	indio_dev->num_channels = config->num_channels;
> +
> +	base = devm_platform_ioremap_resource(pdev, 0);
> +	if (IS_ERR(base))
> +		return PTR_ERR(base);
> +
> +	info->regmap = devm_regmap_init_mmio(&pdev->dev, base,
> +					     &sun20i_gpadc_regmap_config);
> +	if (IS_ERR(info->regmap))
> +		return dev_err_probe(&pdev->dev, PTR_ERR(info->regmap),
> +				     "failed to init regmap\n");
> +
> +	clk = devm_clk_get_enabled(&pdev->dev, NULL);
> +	if (IS_ERR(clk))
> +		return dev_err_probe(&pdev->dev, PTR_ERR(clk),
> +				     "failed to enable bus clock\n");
> +
> +	rst = devm_reset_control_get_exclusive(&pdev->dev, NULL);
> +	if (IS_ERR(rst))
> +		return dev_err_probe(&pdev->dev, PTR_ERR(rst),
> +				     "failed to get reset control\n");
> +
> +	ret = reset_control_deassert(rst);
> +	if (ret)
> +		return dev_err_probe(&pdev->dev, ret,
> +				    "failed to deassert reset\n");
> +
> +	irq = platform_get_irq(pdev, 0);
> +	if (irq < 0)
> +		return dev_err_probe(&pdev->dev, irq, "failed to get irq\n");
> +
> +	ret = devm_request_irq(&pdev->dev, irq, sun20i_gpadc_irq_handler,
> +			       0, dev_name(&pdev->dev), info);
> +	if (ret < 0)
> +		return dev_err_probe(&pdev->dev, ret,
> +				     "failed requesting irq %d\n", irq);
> +
> +	/* set the single conversion mode and enable auto calibration*/
> +	regmap_write(info->regmap, SUN20I_GPADC_CTRL,
> +		     SUN20I_GPADC_CTRL_ADC_AUTOCALI_EN |
> +		     SUN20I_GPADC_CTRL_WORK_MODE_SELECT(0));
> +
> +	ret = devm_iio_device_register(&pdev->dev, indio_dev);
> +	if (ret < 0)
> +		return dev_err_probe(&pdev->dev, ret,
> +				     "could not register the device\n");
> +
> +	return 0;
> +}
> +
> +static const struct of_device_id sun20i_gpadc_of_id[] = {
> +	{
> +		.compatible = "allwinner,sun8i-t113-gpadc",
> +		.data = &sun20i_gpadc_config[0],
> +	},
> +	{
> +		.compatible = "allwinner,sun20i-d1-gpadc",
> +		.data = &sun20i_d1_gpadc_channels[1]

Regardless of what I said above, this looks wrong?
Shouldn't that be read &sun20i_gpadc_config[1] here?

> +	},
> +	{
> +		.compatible = "allwinner,sun50i-r329-gpadc",
> +		.data = &sun50i_r329_gpadc_channels[2]

Same as above.

Cheers,
Andre


> +	},
> +	{ /* sentinel */ }
> +};
> +MODULE_DEVICE_TABLE(of, sun20i_gpadc_of_id);
> +
> +static struct platform_driver sun20i_gpadc_driver = {
> +	.driver = {
> +		.name = "sun20i-gpadc-iio",
> +		.of_match_table = sun20i_gpadc_of_id,
> +	},
> +	.probe = sun20i_gpadc_probe,
> +};
> +module_platform_driver(sun20i_gpadc_driver);
> +
> +MODULE_DESCRIPTION("ADC driver for sunxi platforms");
> +MODULE_AUTHOR("Maksim Kiselev <bigunclemax@gmail.com>");
> +MODULE_LICENSE("GPL");

Re: [RFC PATCH v1 1/4] iio: adc: Add Allwinner D1/T113s/R329 SoCs GPADC

[Andy Shevchenko]

On Wed, May 24, 2023 at 11:01:05AM +0100, Andre Przywara wrote:
> On Wed, 24 May 2023 11:27:30 +0300
> Maxim Kiselev <bigunclemax@gmail.com> wrote:

...

> > +static const struct regmap_config sun20i_gpadc_regmap_config = {
> > +	.reg_bits = 32,
> > +	.val_bits = 32,
> > +	.reg_stride = 4,
> > +	.fast_io = true,
> > +};
> 
> Is there any particular reason you chose a regmap to model this here?
> Isn't that just straight-forward MMIO, which we could just drive using
> readl()/writel()?

Even though regmap adds a few nice features that might be used.
For example, locking. But I dunno if this driver actually uses it
OR uses it correctly.

...

> > +	config = of_device_get_match_data(&pdev->dev);

Please, avoid using OF-centric APIs in the new IIO drivers.

	config = device_get_match_data(&pdev->dev);

should suffice.

> > +	if (!config)
> > +		return -ENODEV;

...

> > +	irq = platform_get_irq(pdev, 0);
> > +	if (irq < 0)
> > +		return dev_err_probe(&pdev->dev, irq, "failed to get irq\n");

We should not repeat the message that printed by platform core.

...

> > +	ret = devm_request_irq(&pdev->dev, irq, sun20i_gpadc_irq_handler,
> > +			       0, dev_name(&pdev->dev), info);

You can simplify your life with

	struct device *dev = &pdev->dev;

at the definition block of the function.

> > +	if (ret < 0)
> > +		return dev_err_probe(&pdev->dev, ret,
> > +				     "failed requesting irq %d\n", irq);

...

> > +		.data = &sun20i_d1_gpadc_channels[1]

Also, leave comma here.

...

> > +		.data = &sun50i_r329_gpadc_channels[2]

Same as above.

-- 
With Best Regards,
Andy Shevchenko

Re: [RFC PATCH v1 1/4] iio: adc: Add Allwinner D1/T113s/R329 SoCs GPADC

[Maxim Kiselev]

Andre Przywara <andre.przywara@arm.com> wrote:
> Or we follow the other idea of reading the number of channels from a DT
> property, which means we would already support every IP with up to 16
> channels, without further driver changes in the future.

I like this idea. This will rid us from duplicating code, and we could also
use a common DT node for D1 and T113.

> Is there any particular reason you chose a regmap to model this here?
> Isn't that just straight-forward MMIO, which we could just drive using
> readl()/writel()?

Actually there is no special reason. I just made it in the same way as
in the sun4i-gpadc-iio driver.

> I wonder if this should be either moved out to some multiplexer: the DT
> bindings suggest that such a thing could be independent.

Sorry I didn't quite get it :)

> But at least we could cater for the possibility that this channel is
> already selected, and skip this part then?

Thanks for that suggestion.

> Couldn't taking the lock go into the function? It seems to be only one
> caller,

Yes, we could. Thanks for the remark.

> and we need the lock in any case, it seems?

This lock is used in others ADC drivers to protects a read access from
ocurring before another one has finished.


Andy Shevchenko <andriy.shevchenko@linux.intel.com> wrote:
> Even though regmap adds a few nice features that might be used.
> For example, locking. But I dunno if this driver actually uses it
> OR uses it correctly.

Just I wrote above, I made it in the same way as in the
sun4i-gpadc-iio driver

> > > + config = of_device_get_match_data(&pdev->dev);
>
> Please, avoid using OF-centric APIs in the new IIO drivers.
>
> config = device_get_match_data(&pdev->dev);
>
> should suffice.

> > + irq = platform_get_irq(pdev, 0);
> > + if (irq < 0)
> > + return dev_err_probe(&pdev->dev, irq, "failed to get irq\n");
>
> > > We should not repeat the message that printed by platform core.

> > > + ret = devm_request_irq(&pdev->dev, irq, sun20i_gpadc_irq_handler,
> > > + 0, dev_name(&pdev->dev), info);
>
> You can simplify your life with
>
> struct device *dev = &pdev->dev;
>
> at the definition block of the function.
> > > + .data = &sun20i_d1_gpadc_channels[1]
>
> Also, leave comma here.

Thanks for the remarks, I'll fix this in the next version.

Re: [RFC PATCH v1 1/4] iio: adc: Add Allwinner D1/T113s/R329 SoCs GPADC

[Jonathan Cameron]

On Wed, 24 May 2023 11:27:30 +0300
Maxim Kiselev <bigunclemax@gmail.com> wrote:

> The General Purpose ADC (GPADC) can convert the external signal into
> a certain proportion of digital value, to realize the measurement of
> analog signal, which can be applied to power detection and key detection.
> 
> D1, T113s and R329 contain this GPADC IP. The only difference between
> this SoCs is the number of available channels:
> 
>  T113 - 1 channel
>  D1   - 2 channels
>  R329 - 4 channels
> 
> Signed-off-by: Maxim Kiselev <bigunclemax@gmail.com>

Hi Maxim,

Various comments inline.

Thanks,

Jonathan

> ---
>  drivers/iio/adc/Kconfig            |  10 ++
>  drivers/iio/adc/Makefile           |   1 +
>  drivers/iio/adc/sun20i-gpadc-iio.c | 275 +++++++++++++++++++++++++++++
>  3 files changed, 286 insertions(+)
>  create mode 100644 drivers/iio/adc/sun20i-gpadc-iio.c
> 
> diff --git a/drivers/iio/adc/Kconfig b/drivers/iio/adc/Kconfig
> index eb2b09ef5d5b..988804f46bf6 100644
> --- a/drivers/iio/adc/Kconfig
> +++ b/drivers/iio/adc/Kconfig
> @@ -1123,6 +1123,16 @@ config SUN4I_GPADC
>  	  To compile this driver as a module, choose M here: the module will be
>  	  called sun4i-gpadc-iio.
>  
> +config SUN20I_GPADC
> +	tristate "Support for the Allwinner SoCs GPADC"
> +	depends on ARCH_SUNXI || COMPILE_TEST
> +	help
> +	  Say yes here to build support for Allwinner (D1, T113 and R329) SoCs
> +	  GPADC. This ADC provides up to 4 channels.

Given you wrote code that clamps at 16 channels I'd avoid saying only 4 here as
it will then need updating if another device turns up with more than 4.

> +
> +	  To compile this driver as a module, choose M here: the module will be
> +	  called sun20i-gpadc-iio.
> +
>  config TI_ADC081C
>  	tristate "Texas Instruments ADC081C/ADC101C/ADC121C family"
>  	depends on I2C

> diff --git a/drivers/iio/adc/sun20i-gpadc-iio.c b/drivers/iio/adc/sun20i-gpadc-iio.c
> new file mode 100644
> index 000000000000..90f3bb2e41cd
> --- /dev/null
> +++ b/drivers/iio/adc/sun20i-gpadc-iio.c
> @@ -0,0 +1,275 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * GPADC driver for sunxi platforms (D1, T113-S3 and R329)
> + * Copyright (c) 2023 Maksim Kiselev <bigunclemax@gmail.com>
> + */
> +
> +#include <linux/completion.h>
> +#include <linux/interrupt.h>
> +#include <linux/io.h>
> +#include <linux/module.h>
> +#include <linux/of.h>
> +#include <linux/of_device.h>
Use generic versions from property.h
> +#include <linux/clk.h>

Alphabetical order preferred. Fine to separate out the iio one as
a more specific group at the end though.

> +#include <linux/reset.h>
> +#include <linux/platform_device.h>

I'd expect to see mod_devicetable.h included.

> +#include <linux/regmap.h>
> +#include <linux/iio/iio.h>
> +
> +#define SUN20I_GPADC_SR					0x00
> +
> +#define SUN20I_GPADC_CTRL				0x04
> +#define SUN20I_GPADC_CTRL_ADC_FIRST_DLY			((GENMASK(7, 0) & (x)) << 24)

define the mask, and use FIELD_PREP to place the data in it.
Same for other cases that look like this.

> +#define SUN20I_GPADC_CTRL_ADC_AUTOCALI_EN		BIT(23)
> +#define SUN20I_GPADC_CTRL_ADC_OP_BIAS			((GENMASK(1, 0) & (x)) << 20)
> +#define SUN20I_GPADC_CTRL_WORK_MODE_SELECT(x)		((GENMASK(1, 0) & (x)) << 18)
> +#define SUN20I_GPADC_CTRL_ADC_CALI_EN			BIT(17)
> +#define SUN20I_GPADC_CTRL_ADC_EN			BIT(16)
> +
> +#define SUN20I_GPADC_CS_EN				0x08
> +#define SUN20I_GPADC_CS_EN_ADC_CHAN_CMP_EN(x)		((GENMASK(16, 0) & BIT(x)) << 16)

> +#define SUN20I_GPADC_CS_EN_ADC_CHAN_SELECT(x)		(GENMASK(16, 0) & BIT(x))
> +
> +#define SUN20I_GPADC_DATA_INTC				0x28
> +#define SUN20I_GPADC_DATA_INTC_CHAN_DATA_IRQ_EN(x)	(GENMASK(16, 0) & BIT(x))
> +
> +#define SUN20I_GPADC_DATA_INTS				0x38
> +#define SUN20I_GPADC_DATA_INTS_DATA_PENDING(x)		(GENMASK(16, 0) & BIT(x))
> +#define SUN20I_GPADC_DATA_INTS_DATA_PENDING_MASK	GENMASK(16, 0)
> +
> +#define SUN20I_GPADC_CHAN_DATA(x)			(0x80 + (x) * 4)
> +
> +#define SUN20I_GPADC_TIMEOUT				msecs_to_jiffies(100)

I'd either document why that time here, or push this down to were it's called.
It's a real world value rather than a magic number so doesn't need a name unless
more information is provided with that.

> +#define SUN20I_GPADC_MAX_CHANNELS			16
> +


...

> +
> +static int sun20i_gpadc_read_raw(struct iio_dev *indio_dev,
> +				 struct iio_chan_spec const *chan, int *val,
> +				 int *val2, long mask)
> +{
> +	struct sun20i_gpadc_iio *info = iio_priv(indio_dev);
> +	int ret;
> +
> +	switch (mask) {
> +	case IIO_CHAN_INFO_RAW:
> +		mutex_lock(&info->lock);
> +		ret = sun20i_gpadc_adc_read(info, chan, val);
> +		mutex_unlock(&info->lock);
> +		return ret;
> +	case IIO_CHAN_INFO_SCALE:
> +		/* 1800mV / 4096 * raw */

Not * raw, because that bit is in userspace - or express this as
value in mv = 1800mV / 4096  raw

Also we have IIO_VAL_FRACTIONAL_LOG_2 - the advantage of that being that
a consumer of this channel can do more efficient maths in some cases 
if it gets the values in that form.

> +		*val = 0;
> +		*val2 = 439453125;
> +		return IIO_VAL_INT_PLUS_NANO;
> +	default:
> +		return -EINVAL;
> +	}
> +}
> +
> +static irqreturn_t sun20i_gpadc_irq_handler(int irq, void *data)
> +{
> +	struct sun20i_gpadc_iio *info = data;
> +	u32 reg;
> +
> +	/* clear data interrupt status register */
> +	regmap_read(info->regmap, SUN20I_GPADC_DATA_INTS, &reg);
> +	regmap_write(info->regmap, SUN20I_GPADC_DATA_INTS, reg);
> +
> +	complete(&info->completion);
> +
> +	return IRQ_HANDLED;
> +}
> +
> +static const struct iio_info sun20i_gpadc_iio_info = {
> +	.read_raw = sun20i_gpadc_read_raw,
> +};
> +
> +static int sun20i_gpadc_probe(struct platform_device *pdev)
> +{
> +	const struct sun20i_gpadc_configuration *config;
> +	struct iio_dev *indio_dev;
> +	struct sun20i_gpadc_iio *info;
> +	struct reset_control *rst;
> +	void __iomem *base;
> +	struct clk *clk;
> +	int irq;
> +	int ret;
> +
> +	indio_dev = devm_iio_device_alloc(&pdev->dev, sizeof(*info));
> +	if (!indio_dev)
> +		return -ENOMEM;
> +
> +	info = iio_priv(indio_dev);
> +	platform_set_drvdata(pdev, indio_dev);

Why set this?  I'm not immediately spotting it being used.

> +
> +	mutex_init(&info->lock);
> +	init_completion(&info->completion);
> +
> +	info->indio_dev = indio_dev;

This is very rarely a good idea. It normally implies that
the level of abstraction has gone wrong somewhere in the driver.
Here I don't think you even use it - hence drop that pointer from
the structure.

> +	indio_dev->info = &sun20i_gpadc_iio_info;
> +	indio_dev->name = dev_name(&pdev->dev);

dev_name() has a habit of not reflecting the part number of the device,
which is what should be used for indio_dev->name.
Usual fix is put it in your device type specific structure - which is another
reason you may want to keep the explicit compatibles.

> +
> +	config = of_device_get_match_data(&pdev->dev);

I think this already got pointed out.  In IIO we are trying to use
the generic firmware interfaces for everything.  Whilst there are devices
where it is unlikely that anyone will ever use them with other firmware
types, using the generic interfaces is more consistent for reviewers
of the wide class of devices that are used with ACPI etc.

> +	if (!config)
> +		return -ENODEV;
> +
> +	/* Sanity check for possible later IP variants with more channels */
> +	if (config->num_channels > SUN20I_GPADC_MAX_CHANNELS)
> +		return dev_err_probe(&pdev->dev, -EINVAL,
> +				     "max channels exceeded\n");

This should be either by design or a compile time check if you really
want to check it.

> +
> +	indio_dev->channels = config->channels;
> +	indio_dev->num_channels = config->num_channels;
> +
> +	base = devm_platform_ioremap_resource(pdev, 0);
> +	if (IS_ERR(base))
> +		return PTR_ERR(base);
> +
> +	info->regmap = devm_regmap_init_mmio(&pdev->dev, base,
> +					     &sun20i_gpadc_regmap_config);
> +	if (IS_ERR(info->regmap))
> +		return dev_err_probe(&pdev->dev, PTR_ERR(info->regmap),
> +				     "failed to init regmap\n");
> +
> +	clk = devm_clk_get_enabled(&pdev->dev, NULL);
> +	if (IS_ERR(clk))
> +		return dev_err_probe(&pdev->dev, PTR_ERR(clk),
> +				     "failed to enable bus clock\n");
> +
> +	rst = devm_reset_control_get_exclusive(&pdev->dev, NULL);
> +	if (IS_ERR(rst))
> +		return dev_err_probe(&pdev->dev, PTR_ERR(rst),
> +				     "failed to get reset control\n");
> +
> +	ret = reset_control_deassert(rst);
> +	if (ret)
> +		return dev_err_probe(&pdev->dev, ret,
> +				    "failed to deassert reset\n");
> +
> +	irq = platform_get_irq(pdev, 0);
> +	if (irq < 0)
> +		return dev_err_probe(&pdev->dev, irq, "failed to get irq\n");
> +
> +	ret = devm_request_irq(&pdev->dev, irq, sun20i_gpadc_irq_handler,
> +			       0, dev_name(&pdev->dev), info);
> +	if (ret < 0)
> +		return dev_err_probe(&pdev->dev, ret,
> +				     "failed requesting irq %d\n", irq);
> +
> +	/* set the single conversion mode and enable auto calibration*/
> +	regmap_write(info->regmap, SUN20I_GPADC_CTRL,
> +		     SUN20I_GPADC_CTRL_ADC_AUTOCALI_EN |
> +		     SUN20I_GPADC_CTRL_WORK_MODE_SELECT(0));
> +
> +	ret = devm_iio_device_register(&pdev->dev, indio_dev);
> +	if (ret < 0)
> +		return dev_err_probe(&pdev->dev, ret,
> +				     "could not register the device\n");
> +
> +	return 0;
> +}
> +
> +static const struct of_device_id sun20i_gpadc_of_id[] = {
> +	{
> +		.compatible = "allwinner,sun8i-t113-gpadc",
> +		.data = &sun20i_gpadc_config[0],
> +	},
> +	{
> +		.compatible = "allwinner,sun20i-d1-gpadc",
> +		.data = &sun20i_d1_gpadc_channels[1]
> +	},
> +	{
> +		.compatible = "allwinner,sun50i-r329-gpadc",
> +		.data = &sun50i_r329_gpadc_channels[2]
I agree with the other reviewer who suggested the array was unnecessary
- if you did want to keep it for some reason the introduce an enum so
you can reference it by named index.

Add commas to these lines, both for consistency and because maybe we will
one day set more in that structure and it will reduce the noise level
of such a patch.
> +	},
> +	{ /* sentinel */ }
> +};
> +MODULE_DEVICE_TABLE(of, sun20i_gpadc_of_id);

[RFC PATCH v1 2/4] dt-bindings: iio: adc: Add Allwinner D1/T113s/R329 SoCs GPADC

[Maxim Kiselev]

Allwinner's D1, T113s and R329 SoCs have a new general purpose ADC.
This ADC is the same for all of this SoCs. The only difference is
the number of available channels.

Signed-off-by: Maxim Kiselev <bigunclemax@gmail.com>
---
 .../iio/adc/allwinner,sun20i-d1-gpadc.yaml    | 52 +++++++++++++++++++
 1 file changed, 52 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/iio/adc/allwinner,sun20i-d1-gpadc.yaml

diff --git a/Documentation/devicetree/bindings/iio/adc/allwinner,sun20i-d1-gpadc.yaml b/Documentation/devicetree/bindings/iio/adc/allwinner,sun20i-d1-gpadc.yaml
new file mode 100644
index 000000000000..a79b874750dd
--- /dev/null
+++ b/Documentation/devicetree/bindings/iio/adc/allwinner,sun20i-d1-gpadc.yaml
@@ -0,0 +1,52 @@
+# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/iio/adc/allwinner,sun20i-d1-gpadc.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: Allwinner D1 General Purpose ADC
+
+properties:
+  "#io-channel-cells":
+    const: 0
+
+  clocks:
+    maxItems: 1
+
+  compatible:
+    enum:
+      - allwinner,sun20i-d1-gpadc
+      - allwinner,sun8i-t113-gpadc
+      - allwinner,sun50i-r329-gpadc
+
+  interrupts:
+    maxItems: 1
+
+  reg:
+    maxItems: 1
+
+  resets:
+    maxItems: 1
+
+required:
+  - "#io-channel-cells"
+  - clocks
+  - compatible
+  - interrupts
+  - reg
+  - resets
+
+additionalProperties: false
+
+examples:
+  - |
+    gpadc: adc@2009000 {
+        compatible = "allwinner,sun20i-d1-gpadc";
+        reg = <0x2009000 0x1000>;
+        clocks = <&ccu CLK_BUS_GPADC>;
+        resets = <&ccu RST_BUS_GPADC>;
+        interrupts = <SOC_PERIPHERAL_IRQ(57) IRQ_TYPE_LEVEL_HIGH>;
+        #io-channel-cells = <0>;
+    };
+
+...
-- 
2.39.2

Re: [RFC PATCH v1 2/4] dt-bindings: iio: adc: Add Allwinner D1/T113s/R329 SoCs GPADC

[Rob Herring]

On Wed, 24 May 2023 11:27:31 +0300, Maxim Kiselev wrote:
> Allwinner's D1, T113s and R329 SoCs have a new general purpose ADC.
> This ADC is the same for all of this SoCs. The only difference is
> the number of available channels.
> 
> Signed-off-by: Maxim Kiselev <bigunclemax@gmail.com>
> ---
>  .../iio/adc/allwinner,sun20i-d1-gpadc.yaml    | 52 +++++++++++++++++++
>  1 file changed, 52 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/iio/adc/allwinner,sun20i-d1-gpadc.yaml
> 

My bot found errors running 'make DT_CHECKER_FLAGS=-m dt_binding_check'
on your patch (DT_CHECKER_FLAGS is new in v5.13):

yamllint warnings/errors:

dtschema/dtc warnings/errors:
/builds/robherring/dt-review-ci/linux/Documentation/devicetree/bindings/iio/adc/allwinner,sun20i-d1-gpadc.yaml: 'maintainers' is a required property
	hint: Metaschema for devicetree binding documentation
	from schema $id: http://devicetree.org/meta-schemas/base.yaml#
Error: Documentation/devicetree/bindings/iio/adc/allwinner,sun20i-d1-gpadc.example.dts:27.28-29 syntax error
FATAL ERROR: Unable to parse input tree
make[1]: *** [scripts/Makefile.lib:419: Documentation/devicetree/bindings/iio/adc/allwinner,sun20i-d1-gpadc.example.dtb] Error 1
make[1]: *** Waiting for unfinished jobs....
make: *** [Makefile:1512: dt_binding_check] Error 2

doc reference errors (make refcheckdocs):

See https://patchwork.ozlabs.org/project/devicetree-bindings/patch/20230524082744.3215427-3-bigunclemax@gmail.com

The base for the series is generally the latest rc1. A different dependency
should be noted in *this* patch.

If you already ran 'make dt_binding_check' and didn't see the above
error(s), then make sure 'yamllint' is installed and dt-schema is up to
date:

pip3 install dtschema --upgrade

Please check and re-submit after running the above command yourself. Note
that DT_SCHEMA_FILES can be set to your schema file to speed up checking
your schema. However, it must be unset to test all examples with your schema.

Re: [RFC PATCH v1 2/4] dt-bindings: iio: adc: Add Allwinner D1/T113s/R329 SoCs GPADC

[Jonathan Cameron]

On Wed, 24 May 2023 11:27:31 +0300
Maxim Kiselev <bigunclemax@gmail.com> wrote:

> Allwinner's D1, T113s and R329 SoCs have a new general purpose ADC.
> This ADC is the same for all of this SoCs. The only difference is
> the number of available channels.
> 
> Signed-off-by: Maxim Kiselev <bigunclemax@gmail.com>
> ---
>  .../iio/adc/allwinner,sun20i-d1-gpadc.yaml    | 52 +++++++++++++++++++
>  1 file changed, 52 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/iio/adc/allwinner,sun20i-d1-gpadc.yaml
> 
> diff --git a/Documentation/devicetree/bindings/iio/adc/allwinner,sun20i-d1-gpadc.yaml b/Documentation/devicetree/bindings/iio/adc/allwinner,sun20i-d1-gpadc.yaml
> new file mode 100644
> index 000000000000..a79b874750dd
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/iio/adc/allwinner,sun20i-d1-gpadc.yaml
> @@ -0,0 +1,52 @@
> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/iio/adc/allwinner,sun20i-d1-gpadc.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: Allwinner D1 General Purpose ADC
> +
> +properties:
> +  "#io-channel-cells":
> +    const: 0

That's odd for a device with more than one channel.  How do you reference
which channel you want to consume?

> +
> +  clocks:
> +    maxItems: 1
> +
> +  compatible:
> +    enum:
> +      - allwinner,sun20i-d1-gpadc
> +      - allwinner,sun8i-t113-gpadc
> +      - allwinner,sun50i-r329-gpadc
> +
> +  interrupts:
> +    maxItems: 1
> +
> +  reg:
> +    maxItems: 1
> +
> +  resets:
> +    maxItems: 1
> +
> +required:
> +  - "#io-channel-cells"
> +  - clocks
> +  - compatible
> +  - interrupts
> +  - reg
> +  - resets
> +
> +additionalProperties: false
> +
> +examples:
> +  - |
> +    gpadc: adc@2009000 {
> +        compatible = "allwinner,sun20i-d1-gpadc";
> +        reg = <0x2009000 0x1000>;
> +        clocks = <&ccu CLK_BUS_GPADC>;
> +        resets = <&ccu RST_BUS_GPADC>;
> +        interrupts = <SOC_PERIPHERAL_IRQ(57) IRQ_TYPE_LEVEL_HIGH>;

I guess the bot probably got this, but to use these macros you need some includes.

> +        #io-channel-cells = <0>;
> +    };
> +
> +...

[RFC PATCH v1 3/4] ARM: dts: sun8i: t113s: Add GPADC node

[Maxim Kiselev]

This patch adds declaration of the general purpose ADC with one channel
for T113s SoC.

Signed-off-by: Maxim Kiselev <bigunclemax@gmail.com>
---
 arch/arm/boot/dts/sun8i-t113s.dtsi | 12 ++++++++++++
 1 file changed, 12 insertions(+)

diff --git a/arch/arm/boot/dts/sun8i-t113s.dtsi b/arch/arm/boot/dts/sun8i-t113s.dtsi
index 804aa197a24f..d269fdde330d 100644
--- a/arch/arm/boot/dts/sun8i-t113s.dtsi
+++ b/arch/arm/boot/dts/sun8i-t113s.dtsi
@@ -42,6 +42,18 @@ gic: interrupt-controller@1c81000 {
 		#interrupt-cells = <3>;
 	};
 
+	soc {
+		gpadc: adc@2009000 {
+			compatible = "allwinner,sun8i-t113-gpadc";
+			reg = <0x2009000 0x1000>;
+			clocks = <&ccu CLK_BUS_GPADC>;
+			resets = <&ccu RST_BUS_GPADC>;
+			interrupts = <SOC_PERIPHERAL_IRQ(57) IRQ_TYPE_LEVEL_HIGH>;
+			status = "disabled";
+			#io-channel-cells = <0>;
+		};
+	};
+
 	timer {
 		compatible = "arm,armv7-timer";
 		interrupts = <GIC_PPI 13 (GIC_CPU_MASK_SIMPLE(4) | IRQ_TYPE_LEVEL_LOW)>,
-- 
2.39.2

[RFC PATCH v1 4/4] riscv: dts: allwinner: d1: Add GPADC node

[Maxim Kiselev]

This patch adds declaration of the general purpose ADC with two channel
for D1 SoC.

Signed-off-by: Maxim Kiselev <bigunclemax@gmail.com>
---
 arch/riscv/boot/dts/allwinner/sun20i-d1.dtsi | 10 ++++++++++
 1 file changed, 10 insertions(+)

diff --git a/arch/riscv/boot/dts/allwinner/sun20i-d1.dtsi b/arch/riscv/boot/dts/allwinner/sun20i-d1.dtsi
index 97e7cbb32597..b21825429cca 100644
--- a/arch/riscv/boot/dts/allwinner/sun20i-d1.dtsi
+++ b/arch/riscv/boot/dts/allwinner/sun20i-d1.dtsi
@@ -6,6 +6,16 @@
 
 / {
 	soc {
+		gpadc: adc@2009000 {
+			compatible = "allwinner,sun20i-d1-gpadc";
+			reg = <0x2009000 0x1000>;
+			clocks = <&ccu CLK_BUS_GPADC>;
+			resets = <&ccu RST_BUS_GPADC>;
+			interrupts = <SOC_PERIPHERAL_IRQ(57) IRQ_TYPE_LEVEL_HIGH>;
+			status = "disabled";
+			#io-channel-cells = <0>;
+		};
+
 		lradc: keys@2009800 {
 			compatible = "allwinner,sun20i-d1-lradc",
 				     "allwinner,sun50i-r329-lradc";
-- 
2.39.2

Re: [RFC PATCH v1 0/4] Add support for Allwinner GPADC on D1/T113s/R329 SoCs

[Andre Przywara]

On Wed, 24 May 2023 11:27:29 +0300
Maxim Kiselev <bigunclemax@gmail.com> wrote:

Hi Maxim,

many thanks for your work on this and for posting it!

> This series adds support for general purpose ADC (GPADC) on new
> Allwinner's SoCs, such as D1, T113s and R329. The implemented driver
> provides basic functionality for getting ADC channels data.
> 
> All of the listed SoCs have the same IP. The only difference is the number
> of available channels:
>      T113 - 1 channel
>      D1   - 2 channels
>      R329 - 4 channels

This may sound kind of obvious, but wouldn't it be easier to model this
with one compatible string, and have the number of channels as a DT
property?
Or, alternatively, using iio/multiplexer/io-channel-mux.yaml, since it's
only one ADC anyway?

And btw: it seems that the T507 (the H616 die with a different pinout) has
the same IP, with four channels:
http://dl.linux-sunxi.org/T507/

Cheers,
Andre

> This series is just an RFC and I would be glad to see any comments
> about it.
> 
> 
> Maxim Kiselev (4):
>   iio: adc: Add Allwinner D1/T113s/R329 SoCs GPADC
>   dt-bindings: iio: adc: Add Allwinner D1/T113s/R329 SoCs GPADC
>   ARM: dts: sun8i: t113s: Add GPADC node
>   riscv: dts: allwinner: d1: Add GPADC node
> 
>  .../iio/adc/allwinner,sun20i-d1-gpadc.yaml    |  52 ++++
>  arch/arm/boot/dts/sun8i-t113s.dtsi            |  12 +
>  arch/riscv/boot/dts/allwinner/sun20i-d1.dtsi  |  10 +
>  drivers/iio/adc/Kconfig                       |  10 +
>  drivers/iio/adc/Makefile                      |   1 +
>  drivers/iio/adc/sun20i-gpadc-iio.c            | 275 ++++++++++++++++++
>  6 files changed, 360 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/iio/adc/allwinner,sun20i-d1-gpadc.yaml
>  create mode 100644 drivers/iio/adc/sun20i-gpadc-iio.c
> 

Re: [RFC PATCH v1 0/4] Add support for Allwinner GPADC on D1/T113s/R329 SoCs

[Maxim Kiselev]

Hi Andre,

thanks for you comments

> This may sound kind of obvious, but wouldn't it be easier to model this
> with one compatible string, and have the number of channels as a DT
> property?

Yes, I completely agree that using separate config for each SoCs is looks
overcomplicated because the only difference is the number of channels.
I thought about a DT property with channels number but I didn't find
another ADC driver with the same approach (except i2c ADC's with child nodes).

> Or, alternatively, using iio/multiplexer/io-channel-mux.yaml, since it's
> only one ADC anyway?
I'm sorry, I didn't quite understand what you're suggesting.

> And btw: it seems that the T507 (the H616 die with a different pinout) has
> the same IP, with four channels:
> http://dl.linux-sunxi.org/T507/

Oh, thanks for pointing that. I'll add it to the list in the next version.

Re: [RFC PATCH v1 0/4] Add support for Allwinner GPADC on D1/T113s/R329 SoCs

[Jonathan Cameron]

On Wed, 24 May 2023 14:36:28 +0300
Maxim Kiselev <bigunclemax@gmail.com> wrote:

> Hi Andre,
> 
> thanks for you comments
> 
> > This may sound kind of obvious, but wouldn't it be easier to model this
> > with one compatible string, and have the number of channels as a DT
> > property?  
> 
> Yes, I completely agree that using separate config for each SoCs is looks
> overcomplicated because the only difference is the number of channels.
> I thought about a DT property with channels number but I didn't find
> another ADC driver with the same approach (except i2c ADC's with child nodes).
If you are 100% sure that that devices are either
1) Detectable at runtime
2) Identical in functionality.

So that in neither case will any changes on driver support expose differences
in the future then a single compatible is fine.

The back up is that you use fallback compatibles - list more than one.
Whilst it doesn't matter (as no differences found) the driver can use
the first one.  If differences become apparent later, others may be used.

I'm not however keen on a simple channel count parameter.  If you want
to go that way, it's better to provide the fine control of individual channel
child nodes (see Documentation/devicetree/bindings/iio/adc/adc.yaml)

That way the control is on which channels are wired to something useful, rather
than whether the device can read them or not (which is pointless if no one
wired them up.


> 
> > Or, alternatively, using iio/multiplexer/io-channel-mux.yaml, since it's
> > only one ADC anyway?  
> I'm sorry, I didn't quite understand what you're suggesting.

That's normally only used for a separate MUX where we need a separate driver
to handle it.  If used on a device like this it would expose additional complexity
to userspace with no benefits in generality etc.

> 
> > And btw: it seems that the T507 (the H616 die with a different pinout) has
> > the same IP, with four channels:
> > http://dl.linux-sunxi.org/T507/  
> 
> Oh, thanks for pointing that. I'll add it to the list in the next version.

Re: [RFC PATCH v1 0/4] Add support for Allwinner GPADC on D1/T113s/R329 SoCs

[Jonathan Cameron]

On Sun, 28 May 2023 16:22:57 +0100
Jonathan Cameron <jic23@kernel.org> wrote:

> On Wed, 24 May 2023 14:36:28 +0300
> Maxim Kiselev <bigunclemax@gmail.com> wrote:
> 
> > Hi Andre,
> > 
> > thanks for you comments
> >   
> > > This may sound kind of obvious, but wouldn't it be easier to model this
> > > with one compatible string, and have the number of channels as a DT
> > > property?    
> > 
> > Yes, I completely agree that using separate config for each SoCs is looks
> > overcomplicated because the only difference is the number of channels.
> > I thought about a DT property with channels number but I didn't find
> > another ADC driver with the same approach (except i2c ADC's with child nodes).  
> If you are 100% sure that that devices are either
> 1) Detectable at runtime
> 2) Identical in functionality.
> 
> So that in neither case will any changes on driver support expose differences
> in the future then a single compatible is fine.
> 
> The back up is that you use fallback compatibles - list more than one.
> Whilst it doesn't matter (as no differences found) the driver can use
> the first one.  If differences become apparent later, others may be used.
> 
> I'm not however keen on a simple channel count parameter.  If you want
> to go that way, it's better to provide the fine control of individual channel
> child nodes (see Documentation/devicetree/bindings/iio/adc/adc.yaml)
> 
> That way the control is on which channels are wired to something useful, rather
> than whether the device can read them or not (which is pointless if no one
> wired them up.

Another thing to note is that with generic compatibles you loose the ability
to have the dt-schmema validate that sets of parameters make sense. If it's
just the channels available that may not matter however.


> 
> 
> >   
> > > Or, alternatively, using iio/multiplexer/io-channel-mux.yaml, since it's
> > > only one ADC anyway?    
> > I'm sorry, I didn't quite understand what you're suggesting.  
> 
> That's normally only used for a separate MUX where we need a separate driver
> to handle it.  If used on a device like this it would expose additional complexity
> to userspace with no benefits in generality etc.
> 
> >   
> > > And btw: it seems that the T507 (the H616 die with a different pinout) has
> > > the same IP, with four channels:
> > > http://dl.linux-sunxi.org/T507/    
> > 
> > Oh, thanks for pointing that. I'll add it to the list in the next version.  
> 

Re: [RFC PATCH v1 0/4] Add support for Allwinner GPADC on D1/T113s/R329 SoCs

[Jonathan Cameron]

On Wed, 24 May 2023 11:27:29 +0300
Maxim Kiselev <bigunclemax@gmail.com> wrote:

> Hi,
> 
> This series adds support for general purpose ADC (GPADC) on new
> Allwinner's SoCs, such as D1, T113s and R329. The implemented driver
> provides basic functionality for getting ADC channels data.
> 
> All of the listed SoCs have the same IP. The only difference is the number
> of available channels:
>      T113 - 1 channel
>      D1   - 2 channels
>      R329 - 4 channels
> 
> This series is just an RFC and I would be glad to see any comments
> about it.
Why 'just an RFC'?  Normal to call out aspects that mean it doesn't yet
make sense for it to be picked up for upstream.

Looks pretty good to me in general rather than RFC material so perhaps
I'm missing something.

Jonathan

> 
> 
> Maxim Kiselev (4):
>   iio: adc: Add Allwinner D1/T113s/R329 SoCs GPADC
>   dt-bindings: iio: adc: Add Allwinner D1/T113s/R329 SoCs GPADC
>   ARM: dts: sun8i: t113s: Add GPADC node
>   riscv: dts: allwinner: d1: Add GPADC node
> 
>  .../iio/adc/allwinner,sun20i-d1-gpadc.yaml    |  52 ++++
>  arch/arm/boot/dts/sun8i-t113s.dtsi            |  12 +
>  arch/riscv/boot/dts/allwinner/sun20i-d1.dtsi  |  10 +
>  drivers/iio/adc/Kconfig                       |  10 +
>  drivers/iio/adc/Makefile                      |   1 +
>  drivers/iio/adc/sun20i-gpadc-iio.c            | 275 ++++++++++++++++++
>  6 files changed, 360 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/iio/adc/allwinner,sun20i-d1-gpadc.yaml
>  create mode 100644 drivers/iio/adc/sun20i-gpadc-iio.c
>