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

Commit Message

Maksim Kiselev May 24, 2023, 8:27 a.m. UTC

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 <[e-mail]>
---
 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

Comments

Andre Przywara May 24, 2023, 10:01 a.m. UTC | #1

On Wed, 24 May 2023 11:27:30 +0300
Maxim Kiselev <[e-mail]> 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 <[e-mail]>
> ---
>  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 <[e-mail]>
> + */
> +
> +#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 <[e-mail]>");
> +MODULE_LICENSE("GPL");

Andy Shevchenko May 24, 2023, 11:06 a.m. UTC | #2

On Wed, May 24, 2023 at 11:01:05AM +0100, Andre Przywara wrote:
> On Wed, 24 May 2023 11:27:30 +0300
> Maxim Kiselev <[e-mail]> 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.

Maksim Kiselev May 24, 2023, 12:14 p.m. UTC | #3

Andre Przywara <[e-mail]> 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 <[e-mail]> 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.

Jonathan Cameron May 28, 2023, 4:38 p.m. UTC | #4

On Wed, 24 May 2023 11:27:30 +0300
Maxim Kiselev <[e-mail]> 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 <[e-mail]>

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 <[e-mail]>
> + */
> +
> +#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);

Patch

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 <[e-mail]>
+ */
+
+#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 <[e-mail]>");
+MODULE_LICENSE("GPL");