Message-ID: <20250927190643.61852030@jic23-huawei>
Date: Sat, 27 Sep 2025 19:06:43 +0100
From: Jonathan Cameron <jic23@...nel.org>
To: Nuno Sá <noname.nuno@...il.com>
Cc: Cosmin-Gabriel Tanislav <cosmin-gabriel.tanislav.xa@...esas.com>, 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>, "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>
Subject: Re: [PATCH v2 3/7] iio: adc: add RZ/T2H / RZ/N2H ADC driver

On Fri, 26 Sep 2025 15:41:14 +0100
Nuno Sá <noname.nuno@...il.com> wrote:

> On Fri, 2025-09-26 at 12:41 +0000, Cosmin-Gabriel Tanislav wrote:
> > 
> >   
> > > -----Original Message-----
> > > From: Nuno Sá <noname.nuno@...il.com>
> > > Sent: Friday, September 26, 2025 3:11 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>;
> > > linux-iio@...r.kernel.org; linux-
> > > renesas-soc@...r.kernel.org; devicetree@...r.kernel.org;
> > > linux-kernel@...r.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@...esas.com>
> > > > ---  
> > > 
> > > Just one small nit from me. With it:
> > > 
> > > Reviewed-by: Nuno Sá <nuno.sa@...log.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@...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..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);
> > 
As you've highlighted this bit of code, I'll observe here that mixing goto and
cleanup.h magic is a bad idea.  There is a bunch of text about this in cleanup.h

Rafael has posted a series that will provide cleanup.h magic for runtime PM, but
that still doesn't get you around the mix of goto and not.

> >      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.  

Powered by blists - more mailing lists