[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <3f93a0dad6ba5dea8db84973ae1518bbb98d3aed.camel@gmail.com>
Date: Fri, 26 Sep 2025 15:41:14 +0100
From: Nuno Sá <noname.nuno@...il.com>
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-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, 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);
>
> 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