Re: [PATCH v2 3/7] iio: adc: add RZ/T2H / RZ/N2H ADC driver

["Nuno Sá" <noname.nuno@gmail.com>]

On Fri, 2025-09-26 at 12:41 +0000, Cosmin-Gabriel Tanislav wrote:
> 
> 
> > -----Original Message-----
> > From: Nuno Sá <noname.nuno@gmail.com>
> > Sent: Friday, September 26, 2025 3:11 PM
> > To: Cosmin-Gabriel Tanislav <cosmin-gabriel.tanislav.xa@renesas.com>
> > Cc: Jonathan Cameron <jic23@kernel.org>; David Lechner
> > <dlechner@baylibre.com>; Nuno Sá
> > <nuno.sa@analog.com>; Andy Shevchenko <andy@kernel.org>; Rob Herring
> > <robh@kernel.org>; Krzysztof
> > Kozlowski <krzk+dt@kernel.org>; Conor Dooley <conor+dt@kernel.org>; Geert
> > Uytterhoeven
> > <geert+renesas@glider.be>; magnus.damm <magnus.damm@gmail.com>;
> > linux-iio@vger.kernel.org; linux-
> > renesas-soc@vger.kernel.org; devicetree@vger.kernel.org;
> > linux-kernel@vger.kernel.org
> > Subject: Re: [PATCH v2 3/7] iio: adc: add RZ/T2H / RZ/N2H ADC driver
> > 
> > On Fri, 2025-09-26 at 01:40 +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@renesas.com>
> > > ---
> > 
> > Just one small nit from me. With it:
> > 
> > Reviewed-by: Nuno Sá <nuno.sa@analog.com>
> > 
> > >  MAINTAINERS                 |   1 +
> > >  drivers/iio/adc/Kconfig     |  10 ++
> > >  drivers/iio/adc/Makefile    |   1 +
> > >  drivers/iio/adc/rzt2h_adc.c | 306 ++++++++++++++++++++++++++++++++++++
> > >  4 files changed, 318 insertions(+)
> > >  create mode 100644 drivers/iio/adc/rzt2h_adc.c
> > > 
> > > diff --git a/MAINTAINERS b/MAINTAINERS
> > > index eed08d25cb7a..220d17039084 100644
> > > --- a/MAINTAINERS
> > > +++ b/MAINTAINERS
> > > @@ -21837,6 +21837,7 @@ L:  linux-iio@vger.kernel.org
> > >  L: linux-renesas-soc@vger.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@bp.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..6a49788a5c67
> > > --- /dev/null
> > > +++ b/drivers/iio/adc/rzt2h_adc.c
> > > @@ -0,0 +1,306 @@
> > > +// 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>
> > > +
> > 
> > ...
> > 
> > > 
> > > +
> > > +static int rzt2h_adc_pm_runtime_resume(struct device *dev)
> > > +{
> > > +   struct iio_dev *indio_dev = dev_get_drvdata(dev);
> > > +   struct rzt2h_adc *adc = iio_priv(indio_dev);
> > 
> > Not seeing the point of the pointer arithmetic. You can pass your device
> > pointer
> > (adc) directly in platform_set_drvdata()
> > 
> 
> Thanks Nuno, I'll do that. I also have another change to make to the driver so
> I will have to send a new version and you'll have to give your Reviewed-by
> again.
> 
> Here's the change I'm planning to make, maybe I could keep the Reviewed-by
> if you agree.
> 
> Without this change, pm_runtime_resume_and_get() is inside the mutex,
> while pm_runtime_put_autosuspend() is outside of it. This is mostly for

I guess you meant the other way around.

> symmetry, although it's not excluded for some subtle bugs to be able to
> occur without it.
> 

Fell free to keep my tag.

- Nuno Sá

> diff --git a/drivers/iio/adc/rzt2h_adc.c b/drivers/iio/adc/rzt2h_adc.c
> index 708029dc8949..79053bbc71c9 100644
> --- a/drivers/iio/adc/rzt2h_adc.c
> +++ b/drivers/iio/adc/rzt2h_adc.c
> @@ -81,9 +81,9 @@ static int rzt2h_adc_read_single(struct rzt2h_adc *adc,
> unsigned int ch, int *va
>      ret = pm_runtime_resume_and_get(adc->dev);
>      if (ret)
>          return ret;
> 
> -    guard(mutex)(&adc->lock);
> +    mutex_lock(&adc->lock);
> 
>      reinit_completion(&adc->completion);
> 
>      /* Enable a single channel */
> @@ -106,8 +106,10 @@ static int rzt2h_adc_read_single(struct rzt2h_adc *adc,
> unsigned int ch, int *va
> 
>  disable:
>      rzt2h_adc_start_stop(adc, false, 0);
> 
> +    mutex_unlock(&adc->lock);
> +
>      pm_runtime_put_autosuspend(adc->dev);
> 
>      return ret;
>  }
> 
> > - 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.