| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID:
<OSZPR01MB8798E25B78C5BB1F2484E523851CA@OSZPR01MB8798.jpnprd01.prod.outlook.com>
Date: Wed, 24 Sep 2025 16:38:01 +0000
From: Cosmin-Gabriel Tanislav <cosmin-gabriel.tanislav.xa@...esas.com>
To: Nuno Sá <noname.nuno@...il.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>,
Prabhakar Mahadev Lad <prabhakar.mahadev-lad.rj@...renesas.com>,
"linux-iio@...r.kernel.org" <linux-iio@...r.kernel.org>,
"linux-renesas-soc@...r.kernel.org" <linux-renesas-soc@...r.kernel.org>,
"devicetree@...r.kernel.org" <devicetree@...r.kernel.org>,
"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
"linux-clk@...r.kernel.org" <linux-clk@...r.kernel.org>
Subject: RE: [PATCH 3/7] iio: adc: add RZ/T2H / RZ/N2H ADC driver
> -----Original Message-----
> From: Nuno Sá <noname.nuno@...il.com>
> Sent: Wednesday, September 24, 2025 5:35 PM
> To: Cosmin-Gabriel 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>; Prabhakar Mahadev Lad <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.
>
Agree. I will inline the contents of rzt2h_adc_start()/rzt2h_adc_stop().
> > +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?
>
We do plan on adding continuous mode support eventually.
> > +
> > + /*
> > + * 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?
>
Yes, we should. I'll do that for the next version.
> > +
> > + 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...
>
No, it shouldn't happen normally.
> > +
> > + 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()
>
For single conversion mode, no, since we can't have multiple enabled
channels. And for continuous conversion we would be using triggers, and
data reading will be in the trigger handler, so I think it is safe to
move the data reading into rzt2h_adc_read_single().
Will do that for next version.
> - Nuno Sá
________________________________
Renesas Electronics Europe GmbH
Registered Office: Arcadiastrasse 10
DE-40472 Duesseldorf
Commercial Registry: Duesseldorf, HRB 3708
Managing Director: Carsten Jauch
VAT-No.: DE 14978647
Tax-ID-No: 105/5839/1793
Legal Disclaimer: This e-mail communication (and any attachment/s) is confidential and contains proprietary information, some or all of which may be legally privileged. It is intended solely for the use of the individual or entity to which it is addressed. Access to this email by anyone else is unauthorized. If you are not the intended recipient, any disclosure, copying, distribution or any action taken or omitted to be taken in reliance on it, is prohibited and may be unlawful.
Powered by blists - more mailing lists