Message-ID: <404c05f20becd0fc8e3256425843187b40a3b625.camel@gmail.com>
Date: Wed, 24 Sep 2025 15:34:45 +0100
From: Nuno Sá <noname.nuno@...il.com>
To: Cosmin Tanislav <cosmin-gabriel.tanislav.xa@...esas.com>
Cc: Jonathan Cameron <jic23@...nel.org>, David Lechner
 <dlechner@...libre.com>,  Nuno Sá	 <nuno.sa@...log.com>,
 Andy Shevchenko <andy@...nel.org>, Rob Herring	 <robh@...nel.org>,
 Krzysztof Kozlowski <krzk+dt@...nel.org>, Conor Dooley	
 <conor+dt@...nel.org>, Geert Uytterhoeven <geert+renesas@...der.be>, Magnus
 Damm <magnus.damm@...il.com>, Michael Turquette <mturquette@...libre.com>,
 Stephen Boyd	 <sboyd@...nel.org>, Lad Prabhakar
 <prabhakar.mahadev-lad.rj@...renesas.com>, 	linux-iio@...r.kernel.org,
 linux-renesas-soc@...r.kernel.org, 	devicetree@...r.kernel.org,
 linux-kernel@...r.kernel.org, 	linux-clk@...r.kernel.org
Subject: Re: [PATCH 3/7] iio: adc: add RZ/T2H / RZ/N2H ADC driver

Hi Cosmin,

Some comments/questions from me...
 
On Tue, 2025-09-23 at 19:05 +0300, Cosmin Tanislav wrote:
> Add support for the A/D 12-Bit successive approximation converters found
> in the Renesas RZ/T2H (R9A09G077) and RZ/N2H (R9A09G087) SoCs.
> 
> RZ/T2H has two ADCs with 4 channels and one with 6.
> RZ/N2H has two ADCs with 4 channels and one with 15.
> 
> Conversions can be performed in single or continuous mode. Result of the
> conversion is stored in a 16-bit data register corresponding to each
> channel.
> 
> The conversions can be started by a software trigger, a synchronous
> trigger (from MTU or from ELC) or an asynchronous external trigger (from
> ADTRGn# pin).
> 
> Only single mode with software trigger is supported for now.
> 
> Signed-off-by: Cosmin Tanislav <cosmin-gabriel.tanislav.xa@...esas.com>
> ---
>  MAINTAINERS                 |   1 +
>  drivers/iio/adc/Kconfig     |  10 ++
>  drivers/iio/adc/Makefile    |   1 +
>  drivers/iio/adc/rzt2h_adc.c | 328 ++++++++++++++++++++++++++++++++++++
>  4 files changed, 340 insertions(+)
>  create mode 100644 drivers/iio/adc/rzt2h_adc.c
> 
> diff --git a/MAINTAINERS b/MAINTAINERS
> index 07e0d37cf468..d550399dc390 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -21828,6 +21828,7 @@ L:	linux-iio@...r.kernel.org
>  L:	linux-renesas-soc@...r.kernel.org
>  S:	Supported
>  F:	Documentation/devicetree/bindings/iio/adc/renesas,r9a09g077-adc.yaml
> +F:	drivers/iio/adc/rzt2h_adc.c
>  
>  RENESAS RTCA-3 RTC DRIVER
>  M:	Claudiu Beznea <claudiu.beznea.uj@...renesas.com>
> diff --git a/drivers/iio/adc/Kconfig b/drivers/iio/adc/Kconfig
> index 58a14e6833f6..cab5eeba48fe 100644
> --- a/drivers/iio/adc/Kconfig
> +++ b/drivers/iio/adc/Kconfig
> @@ -1403,6 +1403,16 @@ config RZG2L_ADC
>  	  To compile this driver as a module, choose M here: the
>  	  module will be called rzg2l_adc.
>  
> +config RZT2H_ADC
> +	tristate "Renesas RZ/T2H / RZ/N2H ADC driver"
> +	select IIO_ADC_HELPER
> +	help
> +	  Say yes here to build support for the ADC found in Renesas
> +	  RZ/T2H / RZ/N2H SoCs.
> +
> +	  To compile this driver as a module, choose M here: the
> +	  module will be called rzt2h_adc.
> +
>  config SC27XX_ADC
>  	tristate "Spreadtrum SC27xx series PMICs ADC"
>  	depends on MFD_SC27XX_PMIC || COMPILE_TEST
> diff --git a/drivers/iio/adc/Makefile b/drivers/iio/adc/Makefile
> index d008f78dc010..ed647a734c51 100644
> --- a/drivers/iio/adc/Makefile
> +++ b/drivers/iio/adc/Makefile
> @@ -123,6 +123,7 @@ obj-$(CONFIG_ROHM_BD79112) += rohm-bd79112.o
>  obj-$(CONFIG_ROHM_BD79124) += rohm-bd79124.o
>  obj-$(CONFIG_ROCKCHIP_SARADC) += rockchip_saradc.o
>  obj-$(CONFIG_RZG2L_ADC) += rzg2l_adc.o
> +obj-$(CONFIG_RZT2H_ADC) += rzt2h_adc.o
>  obj-$(CONFIG_SC27XX_ADC) += sc27xx_adc.o
>  obj-$(CONFIG_SD_ADC_MODULATOR) += sd_adc_modulator.o
>  obj-$(CONFIG_SOPHGO_CV1800B_ADC) += sophgo-cv1800b-adc.o
> diff --git a/drivers/iio/adc/rzt2h_adc.c b/drivers/iio/adc/rzt2h_adc.c
> new file mode 100644
> index 000000000000..d855a79b3d96
> --- /dev/null
> +++ b/drivers/iio/adc/rzt2h_adc.c
> @@ -0,0 +1,328 @@
> +// SPDX-License-Identifier: GPL-2.0
> +
> +#include <linux/bitfield.h>
> +#include <linux/cleanup.h>
> +#include <linux/completion.h>
> +#include <linux/delay.h>
> +#include <linux/iio/adc-helpers.h>
> +#include <linux/iio/iio.h>
> +#include <linux/interrupt.h>
> +#include <linux/io.h>
> +#include <linux/iopoll.h>
> +#include <linux/mod_devicetable.h>
> +#include <linux/module.h>
> +#include <linux/platform_device.h>
> +#include <linux/pm_runtime.h>
> +#include <linux/property.h>
> +
> +#define RZT2H_NAME			"rzt2h-adc"
> +
> +#define RZT2H_ADCSR_REG			0x00
> +#define RZT2H_ADCSR_ADIE_MASK		BIT(12)
> +#define RZT2H_ADCSR_ADCS_MASK		GENMASK(14, 13)
> +#define RZT2H_ADCSR_ADCS_SINGLE		0b00
> +#define RZT2H_ADCSR_ADST_MASK		BIT(15)
> +
> +#define RZT2H_ADANSA0_REG		0x04
> +#define RZT2H_ADANSA0_CH_MASK(x)	BIT(x)
> +
> +#define RZT2H_ADDR_REG(x)		(0x20 + 0x2 * (x))
> +
> +#define RZT2H_ADCALCTL_REG		0x1f0
> +#define RZT2H_ADCALCTL_CAL_MASK		BIT(0)
> +#define RZT2H_ADCALCTL_CAL_RDY_MASK	BIT(1)
> +#define RZT2H_ADCALCTL_CAL_ERR_MASK	BIT(2)
> +
> +#define RZT2H_ADC_MAX_CHANNELS		16
> +#define RZT2H_ADC_VREF_MV		1800
> +#define RZT2H_ADC_RESOLUTION		12
> +
> +struct rzt2h_adc {
> +	void __iomem *base;
> +	struct device *dev;
> +
> +	struct completion completion;
> +	/* lock to protect against multiple access to the device */
> +	struct mutex lock;
> +
> +	const struct iio_chan_spec *channels;
> +	unsigned int num_channels;
> +
> +	u16 data[RZT2H_ADC_MAX_CHANNELS];
> +};
> +
> +static void rzt2h_adc_start_stop(struct rzt2h_adc *adc, bool start,
> +				 unsigned int conversion_type)
> +{
> +	u16 mask;
> +	u16 reg;
> +
> +	reg = readw(adc->base + RZT2H_ADCSR_REG);
> +
> +	if (start) {
> +		/* Set conversion type */
> +		reg &= ~RZT2H_ADCSR_ADCS_MASK;
> +		reg |= FIELD_PREP(RZT2H_ADCSR_ADCS_MASK, conversion_type);
> +	}
> +
> +	/* Toggle end of conversion interrupt and start bit. */
> +	mask = RZT2H_ADCSR_ADIE_MASK | RZT2H_ADCSR_ADST_MASK;
> +	if (start)
> +		reg |= mask;
> +	else
> +		reg &= ~mask;
> +
> +	writew(reg, adc->base + RZT2H_ADCSR_REG);
> +}
> +
> +static void rzt2h_adc_start(struct rzt2h_adc *adc, unsigned int
> conversion_type)
> +{
> +	rzt2h_adc_start_stop(adc, true, conversion_type);
> +}
> +
> +static void rzt2h_adc_stop(struct rzt2h_adc *adc)
> +{
> +	rzt2h_adc_start_stop(adc, false, 0);
> +}
> +

I'm not 100% convince the above two helpers add much value given the usage they
have. I do understand that they make things a bit more readable but still...

rzt2h_adc_start_stop(adc, false/true, ...) is already fairly clear about it's
happening. Dont't feel strong about it anyways so up to you.

> +static int rzt2h_adc_read_single(struct rzt2h_adc *adc, unsigned int ch, int
> *val)
> +{
> +	int ret;
> +
> +	ret = pm_runtime_resume_and_get(adc->dev);
> +	if (ret)
> +		return ret;
> +
> +	guard(mutex)(&adc->lock);
> +
> +	reinit_completion(&adc->completion);
> +
> +	/* Enable a single channel */
> +	writew(RZT2H_ADANSA0_CH_MASK(ch), adc->base + RZT2H_ADANSA0_REG);
> +
> +	rzt2h_adc_start(adc, RZT2H_ADCSR_ADCS_SINGLE);

This is the only place where this is called. So we could just pass
RZT2H_ADCSR_ADCS_SINGLE inside the function. Unless this will be extended in the
near future?

> +
> +	/*
> +	 * Datasheet Page 2770, Table 41.1:
> +	 * 0.32us per channel when sample-and-hold circuits are not in use.
> +	 */
> +	ret = wait_for_completion_timeout(&adc->completion,
> usecs_to_jiffies(1));
> +	if (!ret) {
> +		ret = -ETIMEDOUT;
> +		goto disable;
> +	}
> +
> +	*val = adc->data[ch];
> +	ret = IIO_VAL_INT;
> +
> +disable:
> +	rzt2h_adc_stop(adc);
> +
> +	pm_runtime_put_autosuspend(adc->dev);
> +
> +	return ret;
> +}
> +
> +static void rzt2h_adc_set_cal(struct rzt2h_adc *adc, bool cal)
> +{
> +	u16 val;
> +
> +	val = readw(adc->base + RZT2H_ADCALCTL_REG);
> +	if (cal)
> +		val |= RZT2H_ADCALCTL_CAL_MASK;
> +	else
> +		val &= ~RZT2H_ADCALCTL_CAL_MASK;
> +
> +	writew(val, adc->base + RZT2H_ADCALCTL_REG);
> +}
> +
> +static int rzt2h_adc_calibrate(struct rzt2h_adc *adc)
> +{
> +	u16 val;
> +	int ret;
> +
> +	rzt2h_adc_set_cal(adc, true);
> +
> +	ret = read_poll_timeout(readw, val, val &
> RZT2H_ADCALCTL_CAL_RDY_MASK,
> +				200, 1000, true, adc->base +
> RZT2H_ADCALCTL_REG);
> +	if (ret) {
> +		dev_err(adc->dev, "Calibration timed out: %d\n", ret);
> +		return ret;
> +	}
> +
> +	if (val & RZT2H_ADCALCTL_CAL_ERR_MASK) {
> +		dev_err(adc->dev, "Calibration failed\n");
> +		return -EINVAL;
> +	}
> +
> +	rzt2h_adc_set_cal(adc, false);

Should we disable calibrations in the error path (or right after
read_poll_timeout()) or it does not really matter?

> +
> +	return 0;
> +}
> +
> +static int rzt2h_adc_read_raw(struct iio_dev *indio_dev,
> +			      struct iio_chan_spec const *chan,
> +			      int *val, int *val2, long mask)
> +{
> +	struct rzt2h_adc *adc = iio_priv(indio_dev);
> +
> +	switch (mask) {
> +	case IIO_CHAN_INFO_RAW:
> +		return rzt2h_adc_read_single(adc, chan->channel, val);
> +	case IIO_CHAN_INFO_SCALE:
> +		*val = RZT2H_ADC_VREF_MV;
> +		*val2 = RZT2H_ADC_RESOLUTION;
> +		return IIO_VAL_FRACTIONAL_LOG2;
> +	default:
> +		return -EINVAL;
> +	}
> +}
> +
> +static const struct iio_info rzt2h_adc_iio_info = {
> +	.read_raw = rzt2h_adc_read_raw,
> +};
> +
> +static irqreturn_t rzt2h_adc_isr(int irq, void *private)
> +{
> +	struct rzt2h_adc *adc = private;
> +	unsigned long enabled_channels;
> +	unsigned int ch;
> +
> +	enabled_channels = readw(adc->base + RZT2H_ADANSA0_REG);
> +	if (!enabled_channels)
> +		return IRQ_NONE;

Is the above possible at all? In rzt2h_adc_read_single() we do write the same
register...

> +
> +	for_each_set_bit(ch, &enabled_channels, adc->num_channels)
> +		adc->data[ch] = readw(adc->base + RZT2H_ADDR_REG(ch));
> +

Is there any particular reason for reading all enabled channels in the IRQ when
we kind of just care for one channel? If there's nothing non obvious happening
It seems that the IRQ could just do complete(...) and reading the result in 
rzt2h_adc_read_single()

- Nuno Sá

Powered by blists - more mailing lists