[PATCH 0/4] Add support for the Renesas RZ/N1 ADC

[PATCH 0/4] Add support for the Renesas RZ/N1 ADC

[Herve Codina (Schneider Electric)]

Hi,

The Renesas RZ/N1 ADC controller is the ADC controller available in the
Renesas RZ/N1 SoCs family.

It can use up to two internal ACD cores (ADC1 and ADC2) those internal
cores are handled through ADC controller virtual channels.

Best regards,
Herve Codina

Herve Codina (Schneider Electric) (4):
  dt-bindings: iio: adc: Add the Renesas RZ/N1 ADC
  iio: adc: Add support for the Renesas RZ/N1 ADC
  ARM: dts: renesas: r9a06g032: Add the ADC device
  MAINTAINERS: Add the Renesas RZ/N1 ADC driver entry

 .../bindings/iio/adc/renesas,rzn1-adc.yaml    | 120 ++++
 MAINTAINERS                                   |   8 +
 arch/arm/boot/dts/renesas/r9a06g032.dtsi      |  10 +
 drivers/iio/adc/Kconfig                       |  10 +
 drivers/iio/adc/Makefile                      |   1 +
 drivers/iio/adc/rzn1-adc.c                    | 626 ++++++++++++++++++
 6 files changed, 775 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/iio/adc/renesas,rzn1-adc.yaml
 create mode 100644 drivers/iio/adc/rzn1-adc.c

-- 
2.51.0

[PATCH 1/4] dt-bindings: iio: adc: Add the Renesas RZ/N1 ADC

[Herve Codina (Schneider Electric)]

The Renesas RZ/N1 ADC controller is the ADC controller available in the
Renesas RZ/N1 SoCs family.

Signed-off-by: Herve Codina (Schneider Electric) <herve.codina@bootlin.com>
---
 .../bindings/iio/adc/renesas,rzn1-adc.yaml    | 120 ++++++++++++++++++
 1 file changed, 120 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/iio/adc/renesas,rzn1-adc.yaml

diff --git a/Documentation/devicetree/bindings/iio/adc/renesas,rzn1-adc.yaml b/Documentation/devicetree/bindings/iio/adc/renesas,rzn1-adc.yaml
new file mode 100644
index 000000000000..73a08eef28d9
--- /dev/null
+++ b/Documentation/devicetree/bindings/iio/adc/renesas,rzn1-adc.yaml
@@ -0,0 +1,120 @@
+# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/iio/adc/renesas,rzn1-adc.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: Renesas RZ/N1 Analog to Digital Converter (ADC)
+
+maintainers:
+  - Herve Codina <herve.codina@bootlin.com>
+
+description:
+  The Renesas RZ/N1 ADC controller available in the Renesas RZ/N1 SoCs family
+  can use up to two internal ACD cores (ADC1 and ADC2) those internal cores are
+  handled through ADC controller virtual channels.
+
+properties:
+  compatible:
+    items:
+      - enum:
+          - renesas,r9a06g032-adc   # RZ/N1D
+      - const: renesas,rzn1-adc
+
+  reg:
+    maxItems: 1
+
+  clocks:
+    items:
+      - description: APB internal bus clock
+      - description: ADC clock
+
+  clock-names:
+    items:
+      - const: pclk
+      - const: adc-clk
+
+  power-domains:
+    maxItems: 1
+
+  adc1-avdd-supply:
+    description:
+      ADC1 analog power supply.
+
+  adc1-vref-supply:
+    description:
+      ADC1 reference voltage supply.
+
+  adc2-avdd-supply:
+    description:
+      ADC2 analog power supply.
+
+  adc2-vref-supply:
+    description:
+      ADC2 reference voltage supply.
+
+  '#io-channel-cells':
+    const: 1
+    description: |
+      Channels numbers available:
+        if ADC1 is used (i.e. adc1-{avdd,vref}-supply present):
+          - 0: ADC1 IN0
+          - 1: ADC1 IN1
+          - 2: ADC1 IN2
+          - 3: ADC1 IN3
+          - 4: ADC1 IN4
+          - 5: ADC1 IN6
+          - 6: ADC1 IN7
+          - 7: ADC1 IN8
+        if ADC2 is used (i.e. adc2-{avdd,vref}-supply present):
+          - 8: ADC2 IN0
+          - 9: ADC2 IN1
+          - 10: ADC2 IN2
+          - 11: ADC2 IN3
+          - 12: ADC2 IN4
+          - 13: ADC2 IN6
+          - 14: ADC2 IN7
+          - 15: ADC2 IN8
+
+additionalProperties: false
+
+required:
+  - compatible
+  - reg
+  - clocks
+  - clock-names
+  - power-domains
+  - '#io-channel-cells'
+
+dependencies:
+  # None or both adc1-avdd-supply / adc1-vref-supply should be present
+  adc1-avdd-supply: [ adc1-vref-supply ]
+  adc1-vref-supply: [ adc1-avdd-supply ]
+  # None or both adc2-avdd-supply / adc2-vref-supply should be present
+  adc2-avdd-supply: [ adc2-vref-supply ]
+  adc2-vref-supply: [ adc2-avdd-supply ]
+
+# At least one of avvd/vref supplies
+anyOf:
+  - required:
+      - adc1-vref-supply
+      - adc1-avdd-supply
+  - required:
+      - adc2-vref-supply
+      - adc2-avdd-supply
+
+examples:
+  - |
+    #include <dt-bindings/clock/r9a06g032-sysctrl.h>
+
+    adc: adc@40065000 {
+      compatible = "renesas,r9a06g032-adc", "renesas,rzn1-adc";
+      reg = <0x40065000 0x200>;
+      clocks = <&sysctrl R9A06G032_HCLK_ADC>, <&sysctrl R9A06G032_CLK_ADC>;
+      clock-names = "pclk", "adc-clk";
+      power-domains = <&sysctrl>;
+      adc1-avdd-supply = <&adc1_avdd>;
+      adc1-vref-supply = <&adc1_vref>;
+      #io-channel-cells = <1>;
+    };
+...
-- 
2.51.0

Re: [PATCH 1/4] dt-bindings: iio: adc: Add the Renesas RZ/N1 ADC

[Krzysztof Kozlowski]

On 15/10/2025 16:28, Herve Codina (Schneider Electric) wrote:
> +  clocks:
> +    items:
> +      - description: APB internal bus clock
> +      - description: ADC clock
> +
> +  clock-names:
> +    items:
> +      - const: pclk
> +      - const: adc-clk

Just 'adc'

clk is redundant

> +
> +  power-domains:
> +    maxItems: 1
> +
> +  adc1-avdd-supply:
> +    description:
> +      ADC1 analog power supply.
> +
> +  adc1-vref-supply:
> +    description:
> +      ADC1 reference voltage supply.
> +
> +  adc2-avdd-supply:
> +    description:
> +      ADC2 analog power supply.
> +
> +  adc2-vref-supply:
> +    description:
> +      ADC2 reference voltage supply.
> +
> +  '#io-channel-cells':
> +    const: 1
> +    description: |
> +      Channels numbers available:
> +        if ADC1 is used (i.e. adc1-{avdd,vref}-supply present):
> +          - 0: ADC1 IN0
> +          - 1: ADC1 IN1
> +          - 2: ADC1 IN2
> +          - 3: ADC1 IN3
> +          - 4: ADC1 IN4
> +          - 5: ADC1 IN6
> +          - 6: ADC1 IN7
> +          - 7: ADC1 IN8
> +        if ADC2 is used (i.e. adc2-{avdd,vref}-supply present):
> +          - 8: ADC2 IN0
> +          - 9: ADC2 IN1
> +          - 10: ADC2 IN2
> +          - 11: ADC2 IN3
> +          - 12: ADC2 IN4
> +          - 13: ADC2 IN6
> +          - 14: ADC2 IN7
> +          - 15: ADC2 IN8
> +
> +additionalProperties: false

This goes just before example
> +
> +required:
> +  - compatible
> +  - reg
> +  - clocks
> +  - clock-names
> +  - power-domains
> +  - '#io-channel-cells'
> +
> +dependencies:
> +  # None or both adc1-avdd-supply / adc1-vref-supply should be present
> +  adc1-avdd-supply: [ adc1-vref-supply ]
> +  adc1-vref-supply: [ adc1-avdd-supply ]
> +  # None or both adc2-avdd-supply / adc2-vref-supply should be present
> +  adc2-avdd-supply: [ adc2-vref-supply ]
> +  adc2-vref-supply: [ adc2-avdd-supply ]

Above seems unnecessary. The anyOf below should already enforce that, no?

> +
> +# At least one of avvd/vref supplies
> +anyOf:
> +  - required:
> +      - adc1-vref-supply
> +      - adc1-avdd-supply
> +  - required:
> +      - adc2-vref-supply
> +      - adc2-avdd-supply
> +



Best regards,
Krzysztof

Re: [PATCH 1/4] dt-bindings: iio: adc: Add the Renesas RZ/N1 ADC

[Herve Codina]

Hi Krzysztof,

On Thu, 16 Oct 2025 17:49:33 +0200
Krzysztof Kozlowski <krzk@kernel.org> wrote:

> On 15/10/2025 16:28, Herve Codina (Schneider Electric) wrote:
> > +  clocks:
> > +    items:
> > +      - description: APB internal bus clock
> > +      - description: ADC clock
> > +
> > +  clock-names:
> > +    items:
> > +      - const: pclk
> > +      - const: adc-clk  
> 
> Just 'adc'
> 
> clk is redundant

Ok, will be update.

...

> > +
> > +additionalProperties: false  
> 
> This goes just before example

Ok, will be update

> > +
> > +required:
> > +  - compatible
> > +  - reg
> > +  - clocks
> > +  - clock-names
> > +  - power-domains
> > +  - '#io-channel-cells'
> > +
> > +dependencies:
> > +  # None or both adc1-avdd-supply / adc1-vref-supply should be present
> > +  adc1-avdd-supply: [ adc1-vref-supply ]
> > +  adc1-vref-supply: [ adc1-avdd-supply ]
> > +  # None or both adc2-avdd-supply / adc2-vref-supply should be present
> > +  adc2-avdd-supply: [ adc2-vref-supply ]
> > +  adc2-vref-supply: [ adc2-avdd-supply ]  
> 
> Above seems unnecessary. The anyOf below should already enforce that, no?

Yes, I will remove the above dependencies and keep only the anyOf.

> 
> > +
> > +# At least one of avvd/vref supplies
> > +anyOf:
> > +  - required:
> > +      - adc1-vref-supply
> > +      - adc1-avdd-supply
> > +  - required:
> > +      - adc2-vref-supply
> > +      - adc2-avdd-supply
> > +  
> 

Best regards,
Hervé

Re: [PATCH 1/4] dt-bindings: iio: adc: Add the Renesas RZ/N1 ADC

[Wolfram Sang]

[-- Attachment #1: Type: text/plain, Size: 576 bytes --]


> +description:
> +  The Renesas RZ/N1 ADC controller available in the Renesas RZ/N1 SoCs family
> +  can use up to two internal ACD cores (ADC1 and ADC2) those internal cores are

ADC cores?

> +  handled through ADC controller virtual channels.
> +
> +properties:
> +  compatible:
> +    items:
> +      - enum:
> +          - renesas,r9a06g032-adc   # RZ/N1D
> +      - const: renesas,rzn1-adc

Do you know of other SoCs with this IP core? If it is only RZ/N for now,
we could go with const for N1D. All other N1 variants cannot run Linux
because of no SDRAM controller.


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH 1/4] dt-bindings: iio: adc: Add the Renesas RZ/N1 ADC

[Herve Codina]

On Thu, 16 Oct 2025 19:17:30 +0200
Wolfram Sang <wsa+renesas@sang-engineering.com> wrote:

> > +description:
> > +  The Renesas RZ/N1 ADC controller available in the Renesas RZ/N1 SoCs family
> > +  can use up to two internal ACD cores (ADC1 and ADC2) those internal cores are  
> 
> ADC cores?

Sure, will be fixed.

> 
> > +  handled through ADC controller virtual channels.
> > +
> > +properties:
> > +  compatible:
> > +    items:
> > +      - enum:
> > +          - renesas,r9a06g032-adc   # RZ/N1D
> > +      - const: renesas,rzn1-adc  
> 
> Do you know of other SoCs with this IP core? If it is only RZ/N for now,
> we could go with const for N1D. All other N1 variants cannot run Linux
> because of no SDRAM controller.
> 

I know only about RZ/N1 family.

I will keep only "renesas,r9a06g032-adc" in the next iteration.

Best regards,
Hervé

Re: [PATCH 1/4] dt-bindings: iio: adc: Add the Renesas RZ/N1 ADC

[Herve Codina]

Hi Wolfram, Geert,

On Fri, 17 Oct 2025 09:07:42 +0200
Herve Codina <herve.codina@bootlin.com> wrote:

> > > +
> > > +properties:
> > > +  compatible:
> > > +    items:
> > > +      - enum:
> > > +          - renesas,r9a06g032-adc   # RZ/N1D
> > > +      - const: renesas,rzn1-adc    
> > 
> > Do you know of other SoCs with this IP core? If it is only RZ/N for now,
> > we could go with const for N1D. All other N1 variants cannot run Linux
> > because of no SDRAM controller.
> >   
> 
> I know only about RZ/N1 family.
> 
> I will keep only "renesas,r9a06g032-adc" in the next iteration.
> 

May be I misunderstood.

Most of other bindings related to rzn1d have the both r9a06g032 and
rzn1 compatible string.


Would you expect:

  - a) renesas,r9a06g032-adc and renesas,rzn1-adc

    compatible:
      items:
        - const: renesas,r9a06g032-adc   # RZ/N1D
        - const: renesas,rzn1-adc

or

 - b) renesas,r9a06g032-adc only

    compatible:
      const: renesas,r9a06g032-adc

Can you confirm your expectation?

Best regards,
Hervé

Re: [PATCH 1/4] dt-bindings: iio: adc: Add the Renesas RZ/N1 ADC

[Wolfram Sang]

[-- Attachment #1: Type: text/plain, Size: 201 bytes --]


>     compatible:
>       items:
>         - const: renesas,r9a06g032-adc   # RZ/N1D
>         - const: renesas,rzn1-adc

This. Just switch from a forever-single enum to a const for the first
entry.


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

[PATCH 2/4] iio: adc: Add support for the Renesas RZ/N1 ADC

[Herve Codina (Schneider Electric)]

The Renesas RZ/N1 ADC controller is the ADC controller available in the
Renesas RZ/N1 SoCs family. It can use up to two internal ACD cores (ADC1
and ADC2) those internal cores are not directly accessed but are handled
through ADC controller virtual channels.

Signed-off-by: Herve Codina (Schneider Electric) <herve.codina@bootlin.com>
---
 drivers/iio/adc/Kconfig    |  10 +
 drivers/iio/adc/Makefile   |   1 +
 drivers/iio/adc/rzn1-adc.c | 626 +++++++++++++++++++++++++++++++++++++
 3 files changed, 637 insertions(+)
 create mode 100644 drivers/iio/adc/rzn1-adc.c

diff --git a/drivers/iio/adc/Kconfig b/drivers/iio/adc/Kconfig
index 58a14e6833f6..113f6a5c9745 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 RZN1_ADC
+	tristate "Renesas RZ/N1 ADC driver"
+	depends on ARCH_RZN1 || COMPILE_TEST
+	help
+	  Say yes here to build support for the ADC found in Renesas
+	  RZ/N1 family.
+
+	  To compile this driver as a module, choose M here: the
+	  module will be called rzn1-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..ba7a8a63d070 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_RZN1_ADC) += rzn1-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/rzn1-adc.c b/drivers/iio/adc/rzn1-adc.c
new file mode 100644
index 000000000000..f5e16b9cdf17
--- /dev/null
+++ b/drivers/iio/adc/rzn1-adc.c
@@ -0,0 +1,626 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * Renesas RZ/N1 ADC driver
+ *
+ * Copyright (C) 2025 Schneider-Electric
+ *
+ * Author: Herve Codina <herve.codina@bootlin.com>
+ */
+
+#include <linux/kernel.h>
+#include <linux/slab.h>
+#include <linux/module.h>
+#include <linux/platform_device.h>
+#include <linux/interrupt.h>
+#include <linux/mutex.h>
+#include <linux/completion.h>
+#include <linux/iio/iio.h>
+#include <linux/iio/machine.h>
+#include <linux/iio/driver.h>
+#include <linux/io.h>
+#include <linux/iopoll.h>
+#include <linux/clk.h>
+#include <linux/delay.h>
+#include <linux/bits.h>
+#include <linux/of.h>
+#include <linux/regulator/consumer.h>
+#include <linux/pm_runtime.h>
+
+/*                  ADC1 ADC2
+ * RZ/N1D, BGA 400   y    y
+ * RZ/N1D, BGA 324   y    n
+ * RZ/N1S, BGA 324   y    n
+ * RZ/N1S, BGA 196   y    n
+ * RZ/N1L, BGA 196   y    n
+ */
+
+#define RZN1_ADC_CONTROL_REG	0x2c
+#define RZN1_ADC_CONTROL_ADC_BUSY	BIT(6)
+#define RZN1_ADC_FORCE_REG		0x30
+#define RZN1_ADC_SET_FORCE_REG		0x34
+#define RZN1_ADC_CLEAR_FORCE_REG	0x38
+#define RZN1_ADC_FORCE_VC(_n)		BIT(_n)
+
+#define RZN1_ADC_CONFIG_REG	0x40
+#define RZN1_ADC_CONFIG_ADC_POWER_DOWN	BIT(3)
+
+#define RZN1_ADC_VC_REG(_n)	(0xc0 + 0x4 * (_n))
+#define RZN1_ADC_VC_ADC2_ENABLE		BIT(16)
+#define RZN1_ADC_VC_ADC1_ENABLE		BIT(15)
+#define RZN1_ADC_VC_ADC2_CHANNEL_SEL_MASK	GENMASK(5, 3)
+#define RZN1_ADC_VC_ADC2_CHANNEL_SEL(_c)	FIELD_PREP(RZN1_ADC_VC_ADC2_CHANNEL_SEL_MASK, _c)
+#define RZN1_ADC_VC_ADC1_CHANNEL_SEL_MASK	GENMASK(2, 0)
+#define RZN1_ADC_VC_ADC1_CHANNEL_SEL(_c)	FIELD_PREP(RZN1_ADC_VC_ADC1_CHANNEL_SEL_MASK, _c)
+
+#define RZN1_ADC_ADC1_DATA_REG(_n)	(0x100 + 0x4 * (_n))
+#define RZN1_ADC_ADC2_DATA_REG(_n)	(0x140 + 0x4 * (_n))
+#define RZN1_ADC_ADCX_DATA_DATA_MASK	GENMASK(11, 0)
+#define RZN1_ADC_ADCX_GET_DATA(_reg)	FIELD_GET(RZN1_ADC_ADCX_DATA_DATA_MASK, _reg)
+
+#define RZN1_ADC_CHANNEL_SHARED_SCALE(_ch, _ds_name) {		\
+	.type = IIO_VOLTAGE,					\
+	.indexed = 1,						\
+	.channel = (_ch),					\
+	.info_mask_separate = BIT(IIO_CHAN_INFO_RAW),		\
+	.info_mask_shared_by_type = BIT(IIO_CHAN_INFO_SCALE),   \
+	.datasheet_name = (_ds_name),				\
+}
+
+#define RZN1_ADC_CHANNEL_SEPARATED_SCALE(_ch, _ds_name) {	\
+	.type = IIO_VOLTAGE,					\
+	.indexed = 1,						\
+	.channel = (_ch),					\
+	.info_mask_separate = BIT(IIO_CHAN_INFO_RAW) |		\
+			      BIT(IIO_CHAN_INFO_SCALE),		\
+	.datasheet_name = (_ds_name),				\
+}
+
+/*
+ * 8 ADC1_IN signals existed numbered 0..4, 6..8
+ * ADCx_IN5 doesn't exist in RZ/N1 datasheet
+ */
+static struct iio_chan_spec rzn1_adc1_channels[] = {
+	RZN1_ADC_CHANNEL_SHARED_SCALE(0, "ADC1_IN0"),
+	RZN1_ADC_CHANNEL_SHARED_SCALE(1, "ADC1_IN1"),
+	RZN1_ADC_CHANNEL_SHARED_SCALE(2, "ADC1_IN2"),
+	RZN1_ADC_CHANNEL_SHARED_SCALE(3, "ADC1_IN3"),
+	RZN1_ADC_CHANNEL_SHARED_SCALE(4, "ADC1_IN4"),
+	RZN1_ADC_CHANNEL_SHARED_SCALE(5, "ADC1_IN6"),
+	RZN1_ADC_CHANNEL_SHARED_SCALE(6, "ADC1_IN7"),
+	RZN1_ADC_CHANNEL_SHARED_SCALE(7, "ADC1_IN8"),
+};
+
+static struct iio_chan_spec rzn1_adc2_channels[] = {
+	RZN1_ADC_CHANNEL_SHARED_SCALE(8,  "ADC2_IN0"),
+	RZN1_ADC_CHANNEL_SHARED_SCALE(9,  "ADC2_IN1"),
+	RZN1_ADC_CHANNEL_SHARED_SCALE(10, "ADC2_IN2"),
+	RZN1_ADC_CHANNEL_SHARED_SCALE(11, "ADC2_IN3"),
+	RZN1_ADC_CHANNEL_SHARED_SCALE(12, "ADC2_IN4"),
+	RZN1_ADC_CHANNEL_SHARED_SCALE(13, "ADC2_IN6"),
+	RZN1_ADC_CHANNEL_SHARED_SCALE(14, "ADC2_IN7"),
+	RZN1_ADC_CHANNEL_SHARED_SCALE(15, "ADC2_IN8"),
+};
+
+/*
+ * If both ADCs core are used, scale cannot be common. Indeed, scale is
+ * based on Vref connected on each ADC core.
+ */
+static struct iio_chan_spec rzn1_adc1_adc2_channels[] = {
+	RZN1_ADC_CHANNEL_SEPARATED_SCALE(0, "ADC1_IN0"),
+	RZN1_ADC_CHANNEL_SEPARATED_SCALE(1, "ADC1_IN1"),
+	RZN1_ADC_CHANNEL_SEPARATED_SCALE(2, "ADC1_IN2"),
+	RZN1_ADC_CHANNEL_SEPARATED_SCALE(3, "ADC1_IN3"),
+	RZN1_ADC_CHANNEL_SEPARATED_SCALE(4, "ADC1_IN4"),
+	RZN1_ADC_CHANNEL_SEPARATED_SCALE(5, "ADC1_IN6"),
+	RZN1_ADC_CHANNEL_SEPARATED_SCALE(6, "ADC1_IN7"),
+	RZN1_ADC_CHANNEL_SEPARATED_SCALE(7, "ADC1_IN8"),
+	RZN1_ADC_CHANNEL_SEPARATED_SCALE(8,  "ADC2_IN0"),
+	RZN1_ADC_CHANNEL_SEPARATED_SCALE(9,  "ADC2_IN1"),
+	RZN1_ADC_CHANNEL_SEPARATED_SCALE(10, "ADC2_IN2"),
+	RZN1_ADC_CHANNEL_SEPARATED_SCALE(11, "ADC2_IN3"),
+	RZN1_ADC_CHANNEL_SEPARATED_SCALE(12, "ADC2_IN4"),
+	RZN1_ADC_CHANNEL_SEPARATED_SCALE(13, "ADC2_IN6"),
+	RZN1_ADC_CHANNEL_SEPARATED_SCALE(14, "ADC2_IN7"),
+	RZN1_ADC_CHANNEL_SEPARATED_SCALE(15, "ADC2_IN8"),
+};
+
+struct rzn1_adc_core {
+	int is_used;
+	struct regulator *avdd;
+	struct regulator *vref;
+};
+
+struct rzn1_adc {
+	struct device *dev;
+	void __iomem *regs;
+	struct mutex lock; /* ADC lock */
+	struct clk *pclk;
+	struct clk *adc_clk;
+	struct rzn1_adc_core adc_core[2]; /* ADC1 and ADC2 */
+};
+
+static int rzn1_adc_core_power_on(struct rzn1_adc_core *adc_core)
+{
+	int ret;
+
+	if (!adc_core->is_used)
+		return 0;
+
+	ret = regulator_enable(adc_core->avdd);
+	if (ret)
+		return ret;
+
+	ret = regulator_enable(adc_core->vref);
+	if (ret) {
+		regulator_disable(adc_core->avdd);
+		return ret;
+	}
+
+	return 0;
+}
+
+static void rzn1_adc_core_power_off(struct rzn1_adc_core *adc_core)
+{
+	if (!adc_core->is_used)
+		return;
+
+	regulator_disable(adc_core->avdd);
+	regulator_disable(adc_core->vref);
+}
+
+static int rzn1_adc_core_get_regulators(struct rzn1_adc *rzn1_adc,
+					struct rzn1_adc_core *adc_core,
+					const char *avdd_name, const char *vref_name)
+{
+	struct device *dev = rzn1_adc->dev;
+	int ret;
+
+	adc_core->avdd = devm_regulator_get_optional(dev, avdd_name);
+	if (IS_ERR(adc_core->avdd)) {
+		ret = PTR_ERR(adc_core->avdd);
+		if (ret != -ENODEV)
+			return dev_err_probe(dev, ret,
+					     "Failed to get '%s' regulator\n",
+					     avdd_name);
+		adc_core->avdd = NULL;
+	}
+
+	adc_core->vref = devm_regulator_get_optional(dev, vref_name);
+	if (IS_ERR(adc_core->vref)) {
+		ret = PTR_ERR(adc_core->vref);
+		if (ret != -ENODEV)
+			return dev_err_probe(dev, ret,
+					     "Failed to get '%s' regulator\n",
+					     vref_name);
+		adc_core->vref = NULL;
+	}
+
+	/*
+	 * Both regulators (avdd and vref) need to be available to have the
+	 * related adc_core used.
+	 */
+	adc_core->is_used = adc_core->vref && adc_core->avdd;
+	return 0;
+}
+
+static int rzn1_adc_core_get_vref_mv(struct rzn1_adc_core *adc_core)
+{
+	int vref_uv;
+
+	if (!adc_core->vref)
+		return -ENODEV;
+
+	vref_uv = regulator_get_voltage(adc_core->vref);
+	if (vref_uv < 0)
+		return vref_uv;
+
+	return vref_uv / 1000;
+}
+
+static int rzn1_adc_power(struct rzn1_adc *rzn1_adc, bool power)
+{
+	u32 v;
+
+	writel(power ? 0 : RZN1_ADC_CONFIG_ADC_POWER_DOWN,
+	       rzn1_adc->regs + RZN1_ADC_CONFIG_REG);
+
+	/*
+	 * Wait for the ADC_BUSY to clear.
+	 *
+	 * On timeout, ret is -ETIMEDOUT, otherwise it will be 0.
+	 */
+	return readl_poll_timeout_atomic(rzn1_adc->regs + RZN1_ADC_CONTROL_REG,
+					 v, !(v & RZN1_ADC_CONTROL_ADC_BUSY),
+					 0, 500);
+}
+
+static void rzn1_adc_vc_setup_conversion(struct rzn1_adc *rzn1_adc, u32 ch,
+					 int adc1_ch, int adc2_ch)
+{
+	u32 vc = 0;
+
+	if (adc1_ch != -1)
+		vc |= RZN1_ADC_VC_ADC1_ENABLE | RZN1_ADC_VC_ADC1_CHANNEL_SEL(adc1_ch);
+
+	if (adc2_ch != -1)
+		vc |= RZN1_ADC_VC_ADC2_ENABLE | RZN1_ADC_VC_ADC2_CHANNEL_SEL(adc2_ch);
+
+	writel(vc, rzn1_adc->regs + RZN1_ADC_VC_REG(ch));
+}
+
+static int rzn1_adc_vc_start_conversion(struct rzn1_adc *rzn1_adc, u32 ch)
+{
+	u32 val;
+
+	val = readl(rzn1_adc->regs + RZN1_ADC_FORCE_REG);
+	if (val & RZN1_ADC_FORCE_VC(ch))
+		return -EBUSY;
+
+	writel(RZN1_ADC_FORCE_VC(ch), rzn1_adc->regs + RZN1_ADC_SET_FORCE_REG);
+
+	return 0;
+}
+
+static void rzn1_adc_vc_stop_conversion(struct rzn1_adc *rzn1_adc, u32 ch)
+{
+	writel(RZN1_ADC_FORCE_VC(ch), rzn1_adc->regs + RZN1_ADC_CLEAR_FORCE_REG);
+}
+
+static int rzn1_adc_vc_wait_conversion(struct rzn1_adc *rzn1_adc, u32 ch,
+				       u32 *adc1_data, u32 *adc2_data)
+{
+	u32 data_reg;
+	int ret;
+	u32 v;
+
+	/*
+	 * When a VC is selected, it needs 20 ADC clocks to perform the
+	 * conversion.
+	 *
+	 * The worst case is when the 16 VCs need to perform a conversion and
+	 * our VC is the lowest in term of priority.
+	 *
+	 * In that case, the conversion is performed in 16 * 20 ADC clocks.
+	 *
+	 * The ADC clock can be set from 4MHz to 20MHz. This leads to a worst
+	 * case of  16 * 20 * 1/4Mhz = 80us.
+	 *
+	 * Round it up to 100us
+	 */
+
+	/*
+	 * Wait for the ADC_FORCE_VC(n) to clear.
+	 *
+	 * On timeout, ret is -ETIMEDOUT, otherwise it will be 0.
+	 */
+	ret = readl_poll_timeout_atomic(rzn1_adc->regs + RZN1_ADC_FORCE_REG,
+					v, !(v & RZN1_ADC_FORCE_VC(ch)),
+					0, 100);
+	if (ret)
+		return ret;
+
+	if (adc1_data) {
+		data_reg = readl(rzn1_adc->regs + RZN1_ADC_ADC1_DATA_REG(ch));
+		*adc1_data = RZN1_ADC_ADCX_GET_DATA(data_reg);
+	}
+
+	if (adc2_data) {
+		data_reg = readl(rzn1_adc->regs + RZN1_ADC_ADC2_DATA_REG(ch));
+		*adc2_data = RZN1_ADC_ADCX_GET_DATA(data_reg);
+	}
+
+	return 0;
+}
+
+static int rzn1_adc_read_raw_ch(struct rzn1_adc *rzn1_adc, unsigned int chan, int *val)
+{
+	u32 *adc1_data, *adc2_data;
+	int adc1_ch, adc2_ch;
+	u32 adc_data;
+	int ret;
+
+	if (chan < 8) {
+		/* chan 0..7 used to get ADC1 ch 0..7 */
+		adc1_ch = chan;
+		adc1_data = &adc_data;
+		adc2_ch = -1;
+		adc2_data = NULL;
+	} else if (chan < 16) {
+		/* chan 8..15 used to get ADC2 ch 0..7 */
+		adc1_ch = -1;
+		adc1_data = NULL;
+		adc2_ch = chan - 8;
+		adc2_data = &adc_data;
+	} else {
+		return -EINVAL;
+	}
+
+	ret = pm_runtime_resume_and_get(rzn1_adc->dev);
+	if (ret < 0)
+		return ret;
+
+	mutex_lock(&rzn1_adc->lock);
+
+	rzn1_adc_vc_setup_conversion(rzn1_adc, chan, adc1_ch, adc2_ch);
+
+	ret = rzn1_adc_vc_start_conversion(rzn1_adc, chan);
+	if (ret)
+		goto end;
+
+	ret = rzn1_adc_vc_wait_conversion(rzn1_adc, chan, adc1_data, adc2_data);
+	if (ret) {
+		rzn1_adc_vc_stop_conversion(rzn1_adc, chan);
+		goto end;
+	}
+
+	*val = adc_data;
+	ret = IIO_VAL_INT;
+
+end:
+	mutex_unlock(&rzn1_adc->lock);
+
+	pm_runtime_mark_last_busy(rzn1_adc->dev);
+	pm_runtime_put_autosuspend(rzn1_adc->dev);
+
+	return ret;
+}
+
+static int rzn1_adc_get_vref_mv(struct rzn1_adc *rzn1_adc, unsigned int chan)
+{
+	struct rzn1_adc_core *adc_core;
+
+	/*
+	 * chan 0..7 use ADC1 ch 0..7. Vref related to ADC1 core
+	 * chan 8..15 use ADC2 ch 0..7. Vref related to ADC2 core
+	 */
+	if (chan < 8)
+		adc_core = &rzn1_adc->adc_core[0];
+	else if (chan < 16)
+		adc_core = &rzn1_adc->adc_core[1];
+	else
+		return -EINVAL;
+
+	return rzn1_adc_core_get_vref_mv(adc_core);
+}
+
+static int rzn1_adc_read_raw(struct iio_dev *indio_dev, struct iio_chan_spec const *chan,
+			     int *val, int *val2, long mask)
+{
+	struct rzn1_adc *rzn1_adc = iio_priv(indio_dev);
+	int ret;
+
+	switch (mask) {
+	case IIO_CHAN_INFO_RAW:
+		ret = rzn1_adc_read_raw_ch(rzn1_adc, chan->channel, val);
+		if (ret)
+			return ret;
+		return IIO_VAL_INT;
+
+	case IIO_CHAN_INFO_SCALE:
+		ret = rzn1_adc_get_vref_mv(rzn1_adc, chan->channel);
+		if (ret < 0)
+			return ret;
+		*val = ret;
+		*val2 = 12;
+		return IIO_VAL_FRACTIONAL_LOG2;
+
+	default:
+		break;
+	}
+
+	return -EINVAL;
+}
+
+static const struct iio_info rzn1_adc_info = {
+	.read_raw = &rzn1_adc_read_raw
+};
+
+static int rzn1_adc_enable(struct rzn1_adc *rzn1_adc)
+{
+	int ret;
+
+	ret = rzn1_adc_core_power_on(&rzn1_adc->adc_core[0]);
+	if (ret)
+		return ret;
+
+	ret = rzn1_adc_core_power_on(&rzn1_adc->adc_core[1]);
+	if (ret)
+		goto poweroff_adc_core0;
+
+	ret = clk_prepare_enable(rzn1_adc->pclk);
+	if (ret)
+		goto poweroff_adc_core1;
+
+	ret = clk_prepare_enable(rzn1_adc->adc_clk);
+	if (ret)
+		goto disable_pclk;
+
+	ret = rzn1_adc_power(rzn1_adc, true);
+	if (ret)
+		goto disable_adc_clk;
+
+	return 0;
+
+disable_adc_clk:
+	clk_disable_unprepare(rzn1_adc->adc_clk);
+disable_pclk:
+	clk_disable_unprepare(rzn1_adc->pclk);
+poweroff_adc_core1:
+	rzn1_adc_core_power_off(&rzn1_adc->adc_core[1]);
+poweroff_adc_core0:
+	rzn1_adc_core_power_off(&rzn1_adc->adc_core[0]);
+	return ret;
+}
+
+static void rzn1_adc_disable(struct rzn1_adc *rzn1_adc)
+{
+	rzn1_adc_power(rzn1_adc, false);
+
+	clk_disable_unprepare(rzn1_adc->adc_clk);
+	clk_disable_unprepare(rzn1_adc->pclk);
+
+	rzn1_adc_core_power_off(&rzn1_adc->adc_core[1]);
+	rzn1_adc_core_power_off(&rzn1_adc->adc_core[0]);
+}
+
+static int rzn1_adc_set_iio_dev_channels(struct rzn1_adc *rzn1_adc,
+					 struct iio_dev *indio_dev)
+{
+	int adc_used;
+
+	adc_used = rzn1_adc->adc_core[0].is_used ? 0x01 : 0x00;
+	adc_used |= rzn1_adc->adc_core[1].is_used ? 0x02 : 0x00;
+
+	switch (adc_used) {
+	case 0x01:
+		indio_dev->channels = rzn1_adc1_channels;
+		indio_dev->num_channels = ARRAY_SIZE(rzn1_adc1_channels);
+		return 0;
+	case 0x02:
+		indio_dev->channels = rzn1_adc2_channels;
+		indio_dev->num_channels = ARRAY_SIZE(rzn1_adc2_channels);
+		return 0;
+	case 0x03:
+		indio_dev->channels = rzn1_adc1_adc2_channels;
+		indio_dev->num_channels = ARRAY_SIZE(rzn1_adc1_adc2_channels);
+		return 0;
+	default:
+		break;
+	}
+
+	dev_err(rzn1_adc->dev, "Failed to set IIO channels, no ADC core used\n");
+	return -ENODEV;
+}
+
+static int rzn1_adc_probe(struct platform_device *pdev)
+{
+	struct device *dev = &pdev->dev;
+	struct iio_dev *indio_dev;
+	struct rzn1_adc *rzn1_adc;
+	int ret;
+
+	indio_dev = devm_iio_device_alloc(dev, sizeof(*rzn1_adc));
+	if (!indio_dev)
+		return -ENOMEM;
+
+	rzn1_adc = iio_priv(indio_dev);
+	rzn1_adc->dev = dev;
+	mutex_init(&rzn1_adc->lock);
+
+	rzn1_adc->regs = devm_platform_ioremap_resource(pdev, 0);
+	if (IS_ERR(rzn1_adc->regs))
+		return PTR_ERR(rzn1_adc->regs);
+
+	rzn1_adc->pclk = devm_clk_get(dev, "pclk");
+	if (IS_ERR(rzn1_adc->pclk))
+		return dev_err_probe(dev, PTR_ERR(rzn1_adc->pclk), "Failed to get pclk\n");
+
+	rzn1_adc->adc_clk = devm_clk_get(dev, "adc-clk");
+	if (IS_ERR(rzn1_adc->pclk))
+		return dev_err_probe(dev, PTR_ERR(rzn1_adc->pclk), "Failed to get adc-clk\n");
+
+	ret = rzn1_adc_core_get_regulators(rzn1_adc, &rzn1_adc->adc_core[0],
+					   "adc1-avdd", "adc1-vref");
+	if (ret)
+		return ret;
+
+	ret = rzn1_adc_core_get_regulators(rzn1_adc, &rzn1_adc->adc_core[1],
+					   "adc2-avdd", "adc2-vref");
+	if (ret)
+		return ret;
+
+	platform_set_drvdata(pdev, indio_dev);
+
+	indio_dev->name = dev_name(dev);
+	indio_dev->info = &rzn1_adc_info;
+	indio_dev->modes = INDIO_DIRECT_MODE;
+	ret = rzn1_adc_set_iio_dev_channels(rzn1_adc, indio_dev);
+	if (ret)
+		return ret;
+
+	ret = rzn1_adc_enable(rzn1_adc);
+	if (ret)
+		return ret;
+
+	pm_runtime_set_autosuspend_delay(dev, 500);
+	pm_runtime_use_autosuspend(dev);
+	pm_runtime_get_noresume(dev);
+	pm_runtime_set_active(dev);
+	pm_runtime_enable(dev);
+
+	ret = devm_iio_device_register(dev, indio_dev);
+	if (ret)
+		goto disable;
+
+	pm_runtime_mark_last_busy(dev);
+	pm_runtime_put_autosuspend(dev);
+
+	return 0;
+
+disable:
+	pm_runtime_disable(dev);
+	pm_runtime_put_noidle(dev);
+	pm_runtime_set_suspended(dev);
+	pm_runtime_dont_use_autosuspend(dev);
+
+	rzn1_adc_disable(rzn1_adc);
+	return ret;
+}
+
+static void rzn1_adc_remove(struct platform_device *pdev)
+{
+	struct iio_dev *indio_dev = platform_get_drvdata(pdev);
+	struct rzn1_adc *rzn1_adc = iio_priv(indio_dev);
+
+	pm_runtime_disable(rzn1_adc->dev);
+	pm_runtime_set_suspended(rzn1_adc->dev);
+	pm_runtime_dont_use_autosuspend(rzn1_adc->dev);
+
+	rzn1_adc_disable(rzn1_adc);
+}
+
+static int rzn1_adc_pm_runtime_suspend(struct device *dev)
+{
+	struct iio_dev *indio_dev = dev_get_drvdata(dev);
+	struct rzn1_adc *rzn1_adc = iio_priv(indio_dev);
+
+	rzn1_adc_disable(rzn1_adc);
+
+	return 0;
+}
+
+static int rzn1_adc_pm_runtime_resume(struct device *dev)
+{
+	struct iio_dev *indio_dev = dev_get_drvdata(dev);
+	struct rzn1_adc *rzn1_adc = iio_priv(indio_dev);
+
+	return rzn1_adc_enable(rzn1_adc);
+}
+
+static const struct dev_pm_ops rzn1_adc_pm_ops = {
+	SET_SYSTEM_SLEEP_PM_OPS(pm_runtime_force_suspend, pm_runtime_force_resume)
+	SET_RUNTIME_PM_OPS(rzn1_adc_pm_runtime_suspend, rzn1_adc_pm_runtime_resume, NULL)
+};
+
+static const struct of_device_id rzn1_adc_of_match[] = {
+	{ .compatible = "renesas,rzn1-adc" },
+	{ /* sentinel */ },
+};
+
+MODULE_DEVICE_TABLE(of, rzn1_adc_of_match);
+
+static struct platform_driver rzn1_adc_driver = {
+	.probe = rzn1_adc_probe,
+	.remove = rzn1_adc_remove,
+	.driver = {
+		.name = "rzn1-adc",
+		.of_match_table = of_match_ptr(rzn1_adc_of_match),
+		.pm = pm_ptr(&rzn1_adc_pm_ops),
+	},
+};
+
+module_platform_driver(rzn1_adc_driver);
+
+MODULE_AUTHOR("Herve Codina <herve.codina@bootlin.com>");
+MODULE_DESCRIPTION("Renesas RZ/N1 ADC Driver");
+MODULE_LICENSE("GPL");
-- 
2.51.0

Re: [PATCH 2/4] iio: adc: Add support for the Renesas RZ/N1 ADC

[Andy Shevchenko]

On Wed, Oct 15, 2025 at 04:28:14PM +0200, Herve Codina (Schneider Electric) wrote:

Thanks for the contribution, my comments below.

TL;DR: seems like somebody just submitted a driver without doing an internal
review with the kernel developers familiar with the upstream (and I believe
there are such in your company). This is not good. You, guys, should try your
best and not put a burden on the maintainer's/reviewers' shoulders.

> The Renesas RZ/N1 ADC controller is the ADC controller available in the
> Renesas RZ/N1 SoCs family. It can use up to two internal ACD cores (ADC1
> and ADC2) those internal cores are not directly accessed but are handled
> through ADC controller virtual channels.

...

> +#include <linux/kernel.h>

Definitely big no for this one.

> +#include <linux/slab.h>
> +#include <linux/module.h>
> +#include <linux/platform_device.h>
> +#include <linux/interrupt.h>
> +#include <linux/mutex.h>
> +#include <linux/completion.h>
> +#include <linux/iio/iio.h>
> +#include <linux/iio/machine.h>
> +#include <linux/iio/driver.h>
> +#include <linux/io.h>
> +#include <linux/iopoll.h>
> +#include <linux/clk.h>
> +#include <linux/delay.h>
> +#include <linux/bits.h>

> +#include <linux/of.h>

And simple "no" to this one.

> +#include <linux/regulator/consumer.h>
> +#include <linux/pm_runtime.h>

In IIO we do
1) follow IWYU principle;
2) keep headers ordered (with possible grouping of iio/* ones or others if more
than one and specific to a domain);

...

> +#define RZN1_ADC_CONTROL_REG	0x2c
> +#define RZN1_ADC_CONTROL_ADC_BUSY	BIT(6)
> +#define RZN1_ADC_FORCE_REG		0x30
> +#define RZN1_ADC_SET_FORCE_REG		0x34
> +#define RZN1_ADC_CLEAR_FORCE_REG	0x38
> +#define RZN1_ADC_FORCE_VC(_n)		BIT(_n)

Please, make sure _REG definition values are on the column, same for bitfield
defs (some people like indent them slightly differently to the regs).

> +#define RZN1_ADC_CONFIG_REG	0x40
> +#define RZN1_ADC_CONFIG_ADC_POWER_DOWN	BIT(3)

...

> +#define RZN1_ADC_VC_REG(_n)	(0xc0 + 0x4 * (_n))

0x4 --> 4 as it's just a stride. Same for the rest of the similar cases.

...

> +struct rzn1_adc_core {
> +	int is_used;
> +	struct regulator *avdd;
> +	struct regulator *vref;

Possible waste of 4 bytes on some architectures. Please, run `pahole` and see
the opportunities to improve all data structure types in the driver.

> +};

...

> +static int rzn1_adc_core_power_on(struct rzn1_adc_core *adc_core)
> +{
> +	int ret;
> +
> +	if (!adc_core->is_used)
> +		return 0;
> +
> +	ret = regulator_enable(adc_core->avdd);
> +	if (ret)
> +		return ret;
> +
> +	ret = regulator_enable(adc_core->vref);
> +	if (ret) {
> +		regulator_disable(adc_core->avdd);
> +		return ret;
> +	}

Don't we have bulk API? Can't it be used here? Why?

> +	return 0;
> +}

...

> +static int rzn1_adc_core_get_regulators(struct rzn1_adc *rzn1_adc,
> +					struct rzn1_adc_core *adc_core,
> +					const char *avdd_name, const char *vref_name)
> +{
> +	struct device *dev = rzn1_adc->dev;
> +	int ret;
> +
> +	adc_core->avdd = devm_regulator_get_optional(dev, avdd_name);

	ret = PTR_ERR_OR_ZERO(...);

might simplify the below.

> +	if (IS_ERR(adc_core->avdd)) {
> +		ret = PTR_ERR(adc_core->avdd);
> +		if (ret != -ENODEV)
> +			return dev_err_probe(dev, ret,
> +					     "Failed to get '%s' regulator\n",
> +					     avdd_name);
> +		adc_core->avdd = NULL;
> +	}

Like
	if (ret == -ENODEV)
		...
	else if (ret)
		return dev_err_probe(...);

> +	adc_core->vref = devm_regulator_get_optional(dev, vref_name);
> +	if (IS_ERR(adc_core->vref)) {
> +		ret = PTR_ERR(adc_core->vref);
> +		if (ret != -ENODEV)
> +			return dev_err_probe(dev, ret,
> +					     "Failed to get '%s' regulator\n",
> +					     vref_name);
> +		adc_core->vref = NULL;
> +	}

Ditto. (However see above about bulk API)

I'm so much wondering when somebody provides a wrapper that _optional() for
regulator will be optional from device source code pattern so we won't see
anymore these checks for ENODEV (with chance > 0 of a mistake in some cases as
it's usually an unintuitive for use API if you need to test for the specific
error code).

> +	/*
> +	 * Both regulators (avdd and vref) need to be available to have the
> +	 * related adc_core used.
> +	 */
> +	adc_core->is_used = adc_core->vref && adc_core->avdd;
> +	return 0;
> +}

...

> +static int rzn1_adc_core_get_vref_mv(struct rzn1_adc_core *adc_core)
> +{
> +	int vref_uv;

_uV ?

> +
> +	if (!adc_core->vref)
> +		return -ENODEV;
> +
> +	vref_uv = regulator_get_voltage(adc_core->vref);
> +	if (vref_uv < 0)
> +		return vref_uv;
> +
> +	return vref_uv / 1000;

Hmm... (MICRO/MILLI) ?

> +}

...

> +static int rzn1_adc_power(struct rzn1_adc *rzn1_adc, bool power)
> +{
> +	u32 v;
> +
> +	writel(power ? 0 : RZN1_ADC_CONFIG_ADC_POWER_DOWN,
> +	       rzn1_adc->regs + RZN1_ADC_CONFIG_REG);
> +
> +	/*
> +	 * Wait for the ADC_BUSY to clear.

> +	 * On timeout, ret is -ETIMEDOUT, otherwise it will be 0.

Useless comment. It might be something else in some cases, we don't need to
explain this in every caller.

> +	 */
> +	return readl_poll_timeout_atomic(rzn1_adc->regs + RZN1_ADC_CONTROL_REG,
> +					 v, !(v & RZN1_ADC_CONTROL_ADC_BUSY),
> +					 0, 500);
> +}

...

> +static void rzn1_adc_vc_setup_conversion(struct rzn1_adc *rzn1_adc, u32 ch,
> +					 int adc1_ch, int adc2_ch)
> +{
> +	u32 vc = 0;
> +
> +	if (adc1_ch != -1)

The >= 0 is more robust in case the value gets an error pointer or -err code.

> +		vc |= RZN1_ADC_VC_ADC1_ENABLE | RZN1_ADC_VC_ADC1_CHANNEL_SEL(adc1_ch);
> +
> +	if (adc2_ch != -1)
> +		vc |= RZN1_ADC_VC_ADC2_ENABLE | RZN1_ADC_VC_ADC2_CHANNEL_SEL(adc2_ch);
> +
> +	writel(vc, rzn1_adc->regs + RZN1_ADC_VC_REG(ch));
> +}

> +static int rzn1_adc_vc_wait_conversion(struct rzn1_adc *rzn1_adc, u32 ch,
> +				       u32 *adc1_data, u32 *adc2_data)
> +{
> +	u32 data_reg;
> +	int ret;
> +	u32 v;
> +
> +	/*
> +	 * When a VC is selected, it needs 20 ADC clocks to perform the
> +	 * conversion.
> +	 *
> +	 * The worst case is when the 16 VCs need to perform a conversion and
> +	 * our VC is the lowest in term of priority.
> +	 *
> +	 * In that case, the conversion is performed in 16 * 20 ADC clocks.
> +	 *
> +	 * The ADC clock can be set from 4MHz to 20MHz. This leads to a worst
> +	 * case of  16 * 20 * 1/4Mhz = 80us.
> +	 *
> +	 * Round it up to 100us

Missing period.

> +	 */
> +
> +	/*
> +	 * Wait for the ADC_FORCE_VC(n) to clear.
> +	 *
> +	 * On timeout, ret is -ETIMEDOUT, otherwise it will be 0.
> +	 */
> +	ret = readl_poll_timeout_atomic(rzn1_adc->regs + RZN1_ADC_FORCE_REG,
> +					v, !(v & RZN1_ADC_FORCE_VC(ch)),
> +					0, 100);
> +	if (ret)
> +		return ret;
> +
> +	if (adc1_data) {
> +		data_reg = readl(rzn1_adc->regs + RZN1_ADC_ADC1_DATA_REG(ch));
> +		*adc1_data = RZN1_ADC_ADCX_GET_DATA(data_reg);
> +	}
> +
> +	if (adc2_data) {
> +		data_reg = readl(rzn1_adc->regs + RZN1_ADC_ADC2_DATA_REG(ch));
> +		*adc2_data = RZN1_ADC_ADCX_GET_DATA(data_reg);
> +	}
> +
> +	return 0;
> +}

I almost stopped here, see TL;DR above why.

...

> +static const struct of_device_id rzn1_adc_of_match[] = {
> +	{ .compatible = "renesas,rzn1-adc" },
> +	{ /* sentinel */ },

No comments, no commas in terminator entry. This is a common style in IIO.

> +};

> +

Unneeded blank line.

> +MODULE_DEVICE_TABLE(of, rzn1_adc_of_match);

...

> +static struct platform_driver rzn1_adc_driver = {
> +	.probe = rzn1_adc_probe,
> +	.remove = rzn1_adc_remove,
> +	.driver = {
> +		.name = "rzn1-adc",
> +		.of_match_table = of_match_ptr(rzn1_adc_of_match),

New code shouldn't use of_match_ptr().

> +		.pm = pm_ptr(&rzn1_adc_pm_ops),
> +	},
> +};

> +

Unneeded blank line.

> +module_platform_driver(rzn1_adc_driver);

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH 2/4] iio: adc: Add support for the Renesas RZ/N1 ADC

[Nuno Sá]

On Wed, 2025-10-15 at 16:28 +0200, Herve Codina (Schneider Electric) wrote:
> The Renesas RZ/N1 ADC controller is the ADC controller available in the
> Renesas RZ/N1 SoCs family. It can use up to two internal ACD cores (ADC1
> and ADC2) those internal cores are not directly accessed but are handled
> through ADC controller virtual channels.
> 
> Signed-off-by: Herve Codina (Schneider Electric) <herve.codina@bootlin.com>
> ---
>  drivers/iio/adc/Kconfig    |  10 +
>  drivers/iio/adc/Makefile   |   1 +
>  drivers/iio/adc/rzn1-adc.c | 626 +++++++++++++++++++++++++++++++++++++
>  3 files changed, 637 insertions(+)
>  create mode 100644 drivers/iio/adc/rzn1-adc.c
> 
> diff --git a/drivers/iio/adc/Kconfig b/drivers/iio/adc/Kconfig
> index 58a14e6833f6..113f6a5c9745 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 RZN1_ADC
> +	tristate "Renesas RZ/N1 ADC driver"
> +	depends on ARCH_RZN1 || COMPILE_TEST
> +	help
> +	  Say yes here to build support for the ADC found in Renesas
> +	  RZ/N1 family.
> +
> +	  To compile this driver as a module, choose M here: the
> +	  module will be called rzn1-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..ba7a8a63d070 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_RZN1_ADC) += rzn1-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/rzn1-adc.c b/drivers/iio/adc/rzn1-adc.c
> new file mode 100644
> index 000000000000..f5e16b9cdf17
> --- /dev/null
> +++ b/drivers/iio/adc/rzn1-adc.c
> @@ -0,0 +1,626 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * Renesas RZ/N1 ADC driver
> + *
> + * Copyright (C) 2025 Schneider-Electric
> + *
> + * Author: Herve Codina <herve.codina@bootlin.com>
> + */
> +
> +#include <linux/kernel.h>
> +#include <linux/slab.h>
> +#include <linux/module.h>
> +#include <linux/platform_device.h>
> +#include <linux/interrupt.h>
> +#include <linux/mutex.h>
> +#include <linux/completion.h>
> +#include <linux/iio/iio.h>
> +#include <linux/iio/machine.h>
> +#include <linux/iio/driver.h>
> +#include <linux/io.h>
> +#include <linux/iopoll.h>
> +#include <linux/clk.h>
> +#include <linux/delay.h>
> +#include <linux/bits.h>
> +#include <linux/of.h>
> +#include <linux/regulator/consumer.h>
> +#include <linux/pm_runtime.h>
> +
> +/*                  ADC1 ADC2
> + * RZ/N1D, BGA 400   y    y
> + * RZ/N1D, BGA 324   y    n
> + * RZ/N1S, BGA 324   y    n
> + * RZ/N1S, BGA 196   y    n
> + * RZ/N1L, BGA 196   y    n
> + */
> +
> +#define RZN1_ADC_CONTROL_REG	0x2c
> +#define RZN1_ADC_CONTROL_ADC_BUSY	BIT(6)
> +#define RZN1_ADC_FORCE_REG		0x30
> +#define RZN1_ADC_SET_FORCE_REG		0x34
> +#define RZN1_ADC_CLEAR_FORCE_REG	0x38
> +#define RZN1_ADC_FORCE_VC(_n)		BIT(_n)
> +
> +#define RZN1_ADC_CONFIG_REG	0x40
> +#define RZN1_ADC_CONFIG_ADC_POWER_DOWN	BIT(3)
> +
> 

...

> +static int rzn1_adc_enable(struct rzn1_adc *rzn1_adc)
> +{
> +	int ret;
> +
> +	ret = rzn1_adc_core_power_on(&rzn1_adc->adc_core[0]);
> +	if (ret)
> +		return ret;
> +
> +	ret = rzn1_adc_core_power_on(&rzn1_adc->adc_core[1]);
> +	if (ret)
> +		goto poweroff_adc_core0;
> +
> +	ret = clk_prepare_enable(rzn1_adc->pclk);
> +	if (ret)
> +		goto poweroff_adc_core1;
> +
> +	ret = clk_prepare_enable(rzn1_adc->adc_clk);
> +	if (ret)
> +		goto disable_pclk;
> +
> +	ret = rzn1_adc_power(rzn1_adc, true);
> +	if (ret)
> +		goto disable_adc_clk;

Can we use devm_actions() on the above to avoid the complex error path plus the
.remove() callback?

> +
> +	return 0;
> +
> +disable_adc_clk:
> +	clk_disable_unprepare(rzn1_adc->adc_clk);
> +disable_pclk:
> +	clk_disable_unprepare(rzn1_adc->pclk);
> +poweroff_adc_core1:
> +	rzn1_adc_core_power_off(&rzn1_adc->adc_core[1]);
> +poweroff_adc_core0:
> +	rzn1_adc_core_power_off(&rzn1_adc->adc_core[0]);
> +	return ret;
> +}
> +
> +static void rzn1_adc_disable(struct rzn1_adc *rzn1_adc)
> +{
> +	rzn1_adc_power(rzn1_adc, false);
> +
> +	clk_disable_unprepare(rzn1_adc->adc_clk);
> +	clk_disable_unprepare(rzn1_adc->pclk);
> +
> +	rzn1_adc_core_power_off(&rzn1_adc->adc_core[1]);
> +	rzn1_adc_core_power_off(&rzn1_adc->adc_core[0]);
> +}
> +
> +static int rzn1_adc_set_iio_dev_channels(struct rzn1_adc *rzn1_adc,
> +					 struct iio_dev *indio_dev)
> +{
> +	int adc_used;
> +
> +	adc_used = rzn1_adc->adc_core[0].is_used ? 0x01 : 0x00;
> +	adc_used |= rzn1_adc->adc_core[1].is_used ? 0x02 : 0x00;
> +
> +	switch (adc_used) {
> +	case 0x01:
> +		indio_dev->channels = rzn1_adc1_channels;
> +		indio_dev->num_channels = ARRAY_SIZE(rzn1_adc1_channels);
> +		return 0;
> +	case 0x02:
> +		indio_dev->channels = rzn1_adc2_channels;
> +		indio_dev->num_channels = ARRAY_SIZE(rzn1_adc2_channels);
> +		return 0;
> +	case 0x03:
> +		indio_dev->channels = rzn1_adc1_adc2_channels;
> +		indio_dev->num_channels =
> ARRAY_SIZE(rzn1_adc1_adc2_channels);
> +		return 0;
> +	default:
> +		break;
> +	}
> +
> +	dev_err(rzn1_adc->dev, "Failed to set IIO channels, no ADC core
> used\n");
> +	return -ENODEV;

dev_err_probe()?

> +}
> +
> +static int rzn1_adc_probe(struct platform_device *pdev)
> +{
> +	struct device *dev = &pdev->dev;
> +	struct iio_dev *indio_dev;
> +	struct rzn1_adc *rzn1_adc;
> +	int ret;
> +
> +	indio_dev = devm_iio_device_alloc(dev, sizeof(*rzn1_adc));
> +	if (!indio_dev)
> +		return -ENOMEM;
> +
> +	rzn1_adc = iio_priv(indio_dev);
> +	rzn1_adc->dev = dev;
> +	mutex_init(&rzn1_adc->lock);

devm_mutex_init()

> +
> +	rzn1_adc->regs = devm_platform_ioremap_resource(pdev, 0);
> +	if (IS_ERR(rzn1_adc->regs))
> +		return PTR_ERR(rzn1_adc->regs);
> +
> +	rzn1_adc->pclk = devm_clk_get(dev, "pclk");
> +	if (IS_ERR(rzn1_adc->pclk))
> +		return dev_err_probe(dev, PTR_ERR(rzn1_adc->pclk), "Failed to
> get pclk\n");
> +
> +	rzn1_adc->adc_clk = devm_clk_get(dev, "adc-clk");
> +	if (IS_ERR(rzn1_adc->pclk))
> +		return dev_err_probe(dev, PTR_ERR(rzn1_adc->pclk), "Failed to
> get adc-clk\n");
> +
> +	ret = rzn1_adc_core_get_regulators(rzn1_adc, &rzn1_adc->adc_core[0],
> +					   "adc1-avdd", "adc1-vref");
> +	if (ret)
> +		return ret;
> +
> +	ret = rzn1_adc_core_get_regulators(rzn1_adc, &rzn1_adc->adc_core[1],
> +					   "adc2-avdd", "adc2-vref");
> +	if (ret)
> +		return ret;

Hmm, is avdd really an optional regulator? I mean can the ADC power up at all
without a supply in AVDD? Even vref seems to be mandatory as we can't properly
scale the sample without it.

Also, can't we have getting and enabling the regulator together? Then, we could
use some of the modern helpers to simplify the code (ok I see you use them in
the PM callbacks).

> +
> +	platform_set_drvdata(pdev, indio_dev);
> +
> +	indio_dev->name = dev_name(dev);

dev_name() should not be used for the above. It's typically the part name so I
guess in here "rzn1-adc" would be the appropriate one.

> +	indio_dev->info = &rzn1_adc_info;
> +	indio_dev->modes = INDIO_DIRECT_MODE;
> +	ret = rzn1_adc_set_iio_dev_channels(rzn1_adc, indio_dev);
> +	if (ret)
> +		return ret;
> +
> +	ret = rzn1_adc_enable(rzn1_adc);
> +	if (ret)
> +		return ret;
> +
> +	pm_runtime_set_autosuspend_delay(dev, 500);
> +	pm_runtime_use_autosuspend(dev);
> +	pm_runtime_get_noresume(dev);
> +	pm_runtime_set_active(dev);
> +	pm_runtime_enable(dev);

There's a devm_pm_runtime_enable() API now.

> +
> +	ret = devm_iio_device_register(dev, indio_dev);
> +	if (ret)
> +		goto disable;
> +
> +	pm_runtime_mark_last_busy(dev);
> +	pm_runtime_put_autosuspend(dev);
> +
> +	return 0;
> +
> +disable:
> +	pm_runtime_disable(dev);
> +	pm_runtime_put_noidle(dev);
> +	pm_runtime_set_suspended(dev);
> +	pm_runtime_dont_use_autosuspend(dev);
> +
> +	rzn1_adc_disable(rzn1_adc);
> +	return ret;
> +}
> +
> +static void rzn1_adc_remove(struct platform_device *pdev)
> +{
> +	struct iio_dev *indio_dev = platform_get_drvdata(pdev);
> +	struct rzn1_adc *rzn1_adc = iio_priv(indio_dev);
> +
> +	pm_runtime_disable(rzn1_adc->dev);
> +	pm_runtime_set_suspended(rzn1_adc->dev);
> +	pm_runtime_dont_use_autosuspend(rzn1_adc->dev);
> +
> +	rzn1_adc_disable(rzn1_adc);
> +}

I'm fairly confident we can sanely go without .remove().
 
> +
> +static int rzn1_adc_pm_runtime_suspend(struct device *dev)
> +{
> +	struct iio_dev *indio_dev = dev_get_drvdata(dev);
> +	struct rzn1_adc *rzn1_adc = iio_priv(indio_dev);
> +
> +	rzn1_adc_disable(rzn1_adc);
> +
> +	return 0;
> +}
> +
> +static int rzn1_adc_pm_runtime_resume(struct device *dev)
> +{
> +	struct iio_dev *indio_dev = dev_get_drvdata(dev);
> +	struct rzn1_adc *rzn1_adc = iio_priv(indio_dev);
> +
> +	return rzn1_adc_enable(rzn1_adc);
> +}
> +
> +static const struct dev_pm_ops rzn1_adc_pm_ops = {
> +	SET_SYSTEM_SLEEP_PM_OPS(pm_runtime_force_suspend,
> pm_runtime_force_resume)
> +	SET_RUNTIME_PM_OPS(rzn1_adc_pm_runtime_suspend,
> rzn1_adc_pm_runtime_resume, NULL)
> +};
> +
> +static const struct of_device_id rzn1_adc_of_match[] = {
> +	{ .compatible = "renesas,rzn1-adc" },
> +	{ /* sentinel */ },
> +};

We typically don't add the sentinel comment in IIO.

> +
> +MODULE_DEVICE_TABLE(of, rzn1_adc_of_match);
> +
> +static struct platform_driver rzn1_adc_driver = {
> +	.probe = rzn1_adc_probe,
> +	.remove = rzn1_adc_remove,
> +	.driver = {
> +		.name = "rzn1-adc",
> +		.of_match_table = of_match_ptr(rzn1_adc_of_match),

Drop of_match_ptr().

- Nuno Sá

Re: [PATCH 2/4] iio: adc: Add support for the Renesas RZ/N1 ADC

[Herve Codina]

Hi Nuno,

On Wed, 15 Oct 2025 16:21:09 +0100
Nuno Sá <noname.nuno@gmail.com> wrote:

...
> 
> > +static int rzn1_adc_enable(struct rzn1_adc *rzn1_adc)
> > +{
> > +	int ret;
> > +
> > +	ret = rzn1_adc_core_power_on(&rzn1_adc->adc_core[0]);
> > +	if (ret)
> > +		return ret;
> > +
> > +	ret = rzn1_adc_core_power_on(&rzn1_adc->adc_core[1]);
> > +	if (ret)
> > +		goto poweroff_adc_core0;
> > +
> > +	ret = clk_prepare_enable(rzn1_adc->pclk);
> > +	if (ret)
> > +		goto poweroff_adc_core1;
> > +
> > +	ret = clk_prepare_enable(rzn1_adc->adc_clk);
> > +	if (ret)
> > +		goto disable_pclk;
> > +
> > +	ret = rzn1_adc_power(rzn1_adc, true);
> > +	if (ret)
> > +		goto disable_adc_clk;  
> 
> Can we use devm_actions() on the above to avoid the complex error path plus the
> .remove() callback?

rzn1_adc_enable() is used by the driver pm_runtime_resume() function.

I don't think that devm_add_actions_or_reset() will help here.

In my understanding, devm_* functions are use to perform some operations
automatically on device removal.

The purpose of the error path here is to restore a correct state if
rzn1_adc_enable() failed when it is called from pm_runtime_resume().

In that case no device removal is involved to trig any action set by
devm_add_actions_or_reset().

Maybe I am wrong. Did I miss something?

> 
> > +
> > +	return 0;
> > +
> > +disable_adc_clk:
> > +	clk_disable_unprepare(rzn1_adc->adc_clk);
> > +disable_pclk:
> > +	clk_disable_unprepare(rzn1_adc->pclk);
> > +poweroff_adc_core1:
> > +	rzn1_adc_core_power_off(&rzn1_adc->adc_core[1]);
> > +poweroff_adc_core0:
> > +	rzn1_adc_core_power_off(&rzn1_adc->adc_core[0]);
> > +	return ret;
> > +}
> > +

...

> > +static int rzn1_adc_set_iio_dev_channels(struct rzn1_adc *rzn1_adc,
> > +					 struct iio_dev *indio_dev)
> > +{
> > +	int adc_used;
> > +
> > +	adc_used = rzn1_adc->adc_core[0].is_used ? 0x01 : 0x00;
> > +	adc_used |= rzn1_adc->adc_core[1].is_used ? 0x02 : 0x00;
> > +
> > +	switch (adc_used) {
> > +	case 0x01:
> > +		indio_dev->channels = rzn1_adc1_channels;
> > +		indio_dev->num_channels = ARRAY_SIZE(rzn1_adc1_channels);
> > +		return 0;
> > +	case 0x02:
> > +		indio_dev->channels = rzn1_adc2_channels;
> > +		indio_dev->num_channels = ARRAY_SIZE(rzn1_adc2_channels);
> > +		return 0;
> > +	case 0x03:
> > +		indio_dev->channels = rzn1_adc1_adc2_channels;
> > +		indio_dev->num_channels =
> > ARRAY_SIZE(rzn1_adc1_adc2_channels);
> > +		return 0;
> > +	default:
> > +		break;
> > +	}
> > +
> > +	dev_err(rzn1_adc->dev, "Failed to set IIO channels, no ADC core
> > used\n");
> > +	return -ENODEV;  
> 
> dev_err_probe()?

Why? the error returned is a well known value: -ENODEV.

dev_err_probe() should be involved when -EPROBE_DEFER is a potential error
code.

IMHO, dev_err() here is correct.

> 
> > +}
> > +
> > +static int rzn1_adc_probe(struct platform_device *pdev)
> > +{
> > +	struct device *dev = &pdev->dev;
> > +	struct iio_dev *indio_dev;
> > +	struct rzn1_adc *rzn1_adc;
> > +	int ret;
> > +
> > +	indio_dev = devm_iio_device_alloc(dev, sizeof(*rzn1_adc));
> > +	if (!indio_dev)
> > +		return -ENOMEM;
> > +
> > +	rzn1_adc = iio_priv(indio_dev);
> > +	rzn1_adc->dev = dev;
> > +	mutex_init(&rzn1_adc->lock);  
> 
> devm_mutex_init()

Yes, I will update in the next iteration.

> 
> > +
> > +	rzn1_adc->regs = devm_platform_ioremap_resource(pdev, 0);
> > +	if (IS_ERR(rzn1_adc->regs))
> > +		return PTR_ERR(rzn1_adc->regs);
> > +
> > +	rzn1_adc->pclk = devm_clk_get(dev, "pclk");
> > +	if (IS_ERR(rzn1_adc->pclk))
> > +		return dev_err_probe(dev, PTR_ERR(rzn1_adc->pclk), "Failed to
> > get pclk\n");
> > +
> > +	rzn1_adc->adc_clk = devm_clk_get(dev, "adc-clk");
> > +	if (IS_ERR(rzn1_adc->pclk))
> > +		return dev_err_probe(dev, PTR_ERR(rzn1_adc->pclk), "Failed to
> > get adc-clk\n");
> > +
> > +	ret = rzn1_adc_core_get_regulators(rzn1_adc, &rzn1_adc->adc_core[0],
> > +					   "adc1-avdd", "adc1-vref");
> > +	if (ret)
> > +		return ret;
> > +
> > +	ret = rzn1_adc_core_get_regulators(rzn1_adc, &rzn1_adc->adc_core[1],
> > +					   "adc2-avdd", "adc2-vref");
> > +	if (ret)
> > +		return ret;  
> 
> Hmm, is avdd really an optional regulator? I mean can the ADC power up at all
> without a supply in AVDD? Even vref seems to be mandatory as we can't properly
> scale the sample without it.

Where do you see that avdd is an optional regulator?

> 
> Also, can't we have getting and enabling the regulator together? Then, we could
> use some of the modern helpers to simplify the code (ok I see you use them in
> the PM callbacks).

Yes, I rely on PM callbacks to handle those regulators.

> 
> > +
> > +	platform_set_drvdata(pdev, indio_dev);
> > +
> > +	indio_dev->name = dev_name(dev);  
> 
> dev_name() should not be used for the above. It's typically the part name so I
> guess in here "rzn1-adc" would be the appropriate one.

I thought it was more related to the instance and so having a different name
for each instance was better.

Some other IIO drivers use dev_name() here.

But well, if you confirm that a fixed string should be used and so all
instances have the same string, no problem, I will update my indio_dev->name.

> 
> > +	indio_dev->info = &rzn1_adc_info;
> > +	indio_dev->modes = INDIO_DIRECT_MODE;
> > +	ret = rzn1_adc_set_iio_dev_channels(rzn1_adc, indio_dev);
> > +	if (ret)
> > +		return ret;
> > +
> > +	ret = rzn1_adc_enable(rzn1_adc);
> > +	if (ret)
> > +		return ret;
> > +
> > +	pm_runtime_set_autosuspend_delay(dev, 500);
> > +	pm_runtime_use_autosuspend(dev);
> > +	pm_runtime_get_noresume(dev);
> > +	pm_runtime_set_active(dev);
> > +	pm_runtime_enable(dev);  
> 
> There's a devm_pm_runtime_enable() API now.

Will look to use it in the next iteration.

> 
> > +
> > +	ret = devm_iio_device_register(dev, indio_dev);
> > +	if (ret)
> > +		goto disable;
> > +
> > +	pm_runtime_mark_last_busy(dev);
> > +	pm_runtime_put_autosuspend(dev);
> > +
> > +	return 0;
> > +
> > +disable:
> > +	pm_runtime_disable(dev);
> > +	pm_runtime_put_noidle(dev);
> > +	pm_runtime_set_suspended(dev);
> > +	pm_runtime_dont_use_autosuspend(dev);
> > +
> > +	rzn1_adc_disable(rzn1_adc);
> > +	return ret;
> > +}
> > +
> > +static void rzn1_adc_remove(struct platform_device *pdev)
> > +{
> > +	struct iio_dev *indio_dev = platform_get_drvdata(pdev);
> > +	struct rzn1_adc *rzn1_adc = iio_priv(indio_dev);
> > +
> > +	pm_runtime_disable(rzn1_adc->dev);
> > +	pm_runtime_set_suspended(rzn1_adc->dev);
> > +	pm_runtime_dont_use_autosuspend(rzn1_adc->dev);
> > +
> > +	rzn1_adc_disable(rzn1_adc);
> > +}  
> 
> I'm fairly confident we can sanely go without .remove().

Will see what I can be do for the next iteration.

Maybe I will ask some questions if I need some clarification around
pm_runtime but let me first try to go further in that direction.

>  
> > +
> > +static int rzn1_adc_pm_runtime_suspend(struct device *dev)
> > +{
> > +	struct iio_dev *indio_dev = dev_get_drvdata(dev);
> > +	struct rzn1_adc *rzn1_adc = iio_priv(indio_dev);
> > +
> > +	rzn1_adc_disable(rzn1_adc);
> > +
> > +	return 0;
> > +}
> > +
> > +static int rzn1_adc_pm_runtime_resume(struct device *dev)
> > +{
> > +	struct iio_dev *indio_dev = dev_get_drvdata(dev);
> > +	struct rzn1_adc *rzn1_adc = iio_priv(indio_dev);
> > +
> > +	return rzn1_adc_enable(rzn1_adc);
> > +}
> > +
> > +static const struct dev_pm_ops rzn1_adc_pm_ops = {
> > +	SET_SYSTEM_SLEEP_PM_OPS(pm_runtime_force_suspend,
> > pm_runtime_force_resume)
> > +	SET_RUNTIME_PM_OPS(rzn1_adc_pm_runtime_suspend,
> > rzn1_adc_pm_runtime_resume, NULL)
> > +};
> > +
> > +static const struct of_device_id rzn1_adc_of_match[] = {
> > +	{ .compatible = "renesas,rzn1-adc" },
> > +	{ /* sentinel */ },
> > +};  
> 
> We typically don't add the sentinel comment in IIO.

Ok, will be removed.

> 
> > +
> > +MODULE_DEVICE_TABLE(of, rzn1_adc_of_match);
> > +
> > +static struct platform_driver rzn1_adc_driver = {
> > +	.probe = rzn1_adc_probe,
> > +	.remove = rzn1_adc_remove,
> > +	.driver = {
> > +		.name = "rzn1-adc",
> > +		.of_match_table = of_match_ptr(rzn1_adc_of_match),  
> 
> Drop of_match_ptr().

Ok, will be removed.


Thanks for your review.

Best regards,
Hervé

Re: [PATCH 2/4] iio: adc: Add support for the Renesas RZ/N1 ADC

[Nuno Sá]

On Wed, 2025-10-15 at 21:14 +0200, Herve Codina wrote:
> Hi Nuno,
> 
> On Wed, 15 Oct 2025 16:21:09 +0100
> Nuno Sá <noname.nuno@gmail.com> wrote:
> 
> ...
> > 
> > > +static int rzn1_adc_enable(struct rzn1_adc *rzn1_adc)
> > > +{
> > > +	int ret;
> > > +
> > > +	ret = rzn1_adc_core_power_on(&rzn1_adc->adc_core[0]);
> > > +	if (ret)
> > > +		return ret;
> > > +
> > > +	ret = rzn1_adc_core_power_on(&rzn1_adc->adc_core[1]);
> > > +	if (ret)
> > > +		goto poweroff_adc_core0;
> > > +
> > > +	ret = clk_prepare_enable(rzn1_adc->pclk);
> > > +	if (ret)
> > > +		goto poweroff_adc_core1;
> > > +
> > > +	ret = clk_prepare_enable(rzn1_adc->adc_clk);
> > > +	if (ret)
> > > +		goto disable_pclk;
> > > +
> > > +	ret = rzn1_adc_power(rzn1_adc, true);
> > > +	if (ret)
> > > +		goto disable_adc_clk;  
> > 
> > Can we use devm_actions() on the above to avoid the complex error path plus
> > the
> > .remove() callback?
> 
> rzn1_adc_enable() is used by the driver pm_runtime_resume() function.
> 
> I don't think that devm_add_actions_or_reset() will help here.
> 
> In my understanding, devm_* functions are use to perform some operations
> automatically on device removal.
> 
> The purpose of the error path here is to restore a correct state if
> rzn1_adc_enable() failed when it is called from pm_runtime_resume().
> 
> In that case no device removal is involved to trig any action set by
> devm_add_actions_or_reset().
> 
> Maybe I am wrong. Did I miss something?

Nope, I see now what's your intent.

> 
> > 
> > > +
> > > +	return 0;
> > > +
> > > +disable_adc_clk:
> > > +	clk_disable_unprepare(rzn1_adc->adc_clk);
> > > +disable_pclk:
> > > +	clk_disable_unprepare(rzn1_adc->pclk);
> > > +poweroff_adc_core1:
> > > +	rzn1_adc_core_power_off(&rzn1_adc->adc_core[1]);
> > > +poweroff_adc_core0:
> > > +	rzn1_adc_core_power_off(&rzn1_adc->adc_core[0]);
> > > +	return ret;
> > > +}
> > > +
> 
> ...
> 
> > > +static int rzn1_adc_set_iio_dev_channels(struct rzn1_adc *rzn1_adc,
> > > +					 struct iio_dev *indio_dev)
> > > +{
> > > +	int adc_used;
> > > +
> > > +	adc_used = rzn1_adc->adc_core[0].is_used ? 0x01 : 0x00;
> > > +	adc_used |= rzn1_adc->adc_core[1].is_used ? 0x02 : 0x00;
> > > +
> > > +	switch (adc_used) {
> > > +	case 0x01:
> > > +		indio_dev->channels = rzn1_adc1_channels;
> > > +		indio_dev->num_channels = ARRAY_SIZE(rzn1_adc1_channels);
> > > +		return 0;
> > > +	case 0x02:
> > > +		indio_dev->channels = rzn1_adc2_channels;
> > > +		indio_dev->num_channels = ARRAY_SIZE(rzn1_adc2_channels);
> > > +		return 0;
> > > +	case 0x03:
> > > +		indio_dev->channels = rzn1_adc1_adc2_channels;
> > > +		indio_dev->num_channels =
> > > ARRAY_SIZE(rzn1_adc1_adc2_channels);
> > > +		return 0;
> > > +	default:
> > > +		break;
> > > +	}
> > > +
> > > +	dev_err(rzn1_adc->dev, "Failed to set IIO channels, no ADC core
> > > used\n");
> > > +	return -ENODEV;  
> > 
> > dev_err_probe()?
> 
> Why? the error returned is a well known value: -ENODEV.
> 
> dev_err_probe() should be involved when -EPROBE_DEFER is a potential error
> code.
> 
> IMHO, dev_err() here is correct.

If I'm not missing nothing this function is called during probe so I do think
dev_err_probe() should be used. Not only unifies logging style during probe it
also has the small benefit of doing:

return dev_err_probe(...) saving a line of code.

You can see that, at least in IIO, we even have some patches just converting
drivers probe() to use dev_err_probe().

> 
> > 
> > > +}
> > > +
> > > +static int rzn1_adc_probe(struct platform_device *pdev)
> > > +{
> > > +	struct device *dev = &pdev->dev;
> > > +	struct iio_dev *indio_dev;
> > > +	struct rzn1_adc *rzn1_adc;
> > > +	int ret;
> > > +
> > > +	indio_dev = devm_iio_device_alloc(dev, sizeof(*rzn1_adc));
> > > +	if (!indio_dev)
> > > +		return -ENOMEM;
> > > +
> > > +	rzn1_adc = iio_priv(indio_dev);
> > > +	rzn1_adc->dev = dev;
> > > +	mutex_init(&rzn1_adc->lock);  
> > 
> > devm_mutex_init()
> 
> Yes, I will update in the next iteration.
> 
> > 
> > > +
> > > +	rzn1_adc->regs = devm_platform_ioremap_resource(pdev, 0);
> > > +	if (IS_ERR(rzn1_adc->regs))
> > > +		return PTR_ERR(rzn1_adc->regs);
> > > +
> > > +	rzn1_adc->pclk = devm_clk_get(dev, "pclk");
> > > +	if (IS_ERR(rzn1_adc->pclk))
> > > +		return dev_err_probe(dev, PTR_ERR(rzn1_adc->pclk),
> > > "Failed to
> > > get pclk\n");
> > > +
> > > +	rzn1_adc->adc_clk = devm_clk_get(dev, "adc-clk");
> > > +	if (IS_ERR(rzn1_adc->pclk))
> > > +		return dev_err_probe(dev, PTR_ERR(rzn1_adc->pclk),
> > > "Failed to
> > > get adc-clk\n");
> > > +
> > > +	ret = rzn1_adc_core_get_regulators(rzn1_adc, &rzn1_adc-
> > > >adc_core[0],
> > > +					   "adc1-avdd", "adc1-vref");
> > > +	if (ret)
> > > +		return ret;
> > > +
> > > +	ret = rzn1_adc_core_get_regulators(rzn1_adc, &rzn1_adc-
> > > >adc_core[1],
> > > +					   "adc2-avdd", "adc2-vref");
> > > +	if (ret)
> > > +		return ret;  
> > 
> > Hmm, is avdd really an optional regulator? I mean can the ADC power up at
> > all
> > without a supply in AVDD? Even vref seems to be mandatory as we can't
> > properly
> > scale the sample without it.
> 
> Where do you see that avdd is an optional regulator?

You are using devm_regulator_get_optional(). That's for optional regulators.

> 
> > 
> > Also, can't we have getting and enabling the regulator together? Then, we
> > could
> > use some of the modern helpers to simplify the code (ok I see you use them
> > in
> > the PM callbacks).
> 
> Yes, I rely on PM callbacks to handle those regulators.
> 
> > 
> > > +
> > > +	platform_set_drvdata(pdev, indio_dev);
> > > +
> > > +	indio_dev->name = dev_name(dev);  
> > 
> > dev_name() should not be used for the above. It's typically the part name so
> > I
> > guess in here "rzn1-adc" would be the appropriate one.
> 
> I thought it was more related to the instance and so having a different name
> for each instance was better.
> 
> Some other IIO drivers use dev_name() here.
> 
> But well, if you confirm that a fixed string should be used and so all
> instances have the same string, no problem, I will update my indio_dev->name.

It is a fixed string, typically the part name. David Lechner not that long ago
actually sent some patch or documented somewhere why not to use dev_name(). To
identify different instances we have a 'label' property.

> 
> > 
> > > +	indio_dev->info = &rzn1_adc_info;
> > > +	indio_dev->modes = INDIO_DIRECT_MODE;
> > > +	ret = rzn1_adc_set_iio_dev_channels(rzn1_adc, indio_dev);
> > > +	if (ret)
> > > +		return ret;
> > > +
> > > +	ret = rzn1_adc_enable(rzn1_adc);
> > > +	if (ret)
> > > +		return ret;
> > > +
> > > +	pm_runtime_set_autosuspend_delay(dev, 500);
> > > +	pm_runtime_use_autosuspend(dev);
> > > +	pm_runtime_get_noresume(dev);
> > > +	pm_runtime_set_active(dev);
> > > +	pm_runtime_enable(dev);  
> > 
> > There's a devm_pm_runtime_enable() API now.
> 
> Will look to use it in the next iteration.
> 
> > 
> > > +
> > > +	ret = devm_iio_device_register(dev, indio_dev);
> > > +	if (ret)
> > > +		goto disable;
> > > +
> > > +	pm_runtime_mark_last_busy(dev);
> > > +	pm_runtime_put_autosuspend(dev);
> > > +
> > > +	return 0;
> > > +
> > > +disable:
> > > +	pm_runtime_disable(dev);
> > > +	pm_runtime_put_noidle(dev);
> > > +	pm_runtime_set_suspended(dev);
> > > +	pm_runtime_dont_use_autosuspend(dev);
> > > +
> > > +	rzn1_adc_disable(rzn1_adc);
> > > +	return ret;
> > > +}
> > > +
> > > +static void rzn1_adc_remove(struct platform_device *pdev)
> > > +{
> > > +	struct iio_dev *indio_dev = platform_get_drvdata(pdev);
> > > +	struct rzn1_adc *rzn1_adc = iio_priv(indio_dev);
> > > +
> > > +	pm_runtime_disable(rzn1_adc->dev);
> > > +	pm_runtime_set_suspended(rzn1_adc->dev);
> > > +	pm_runtime_dont_use_autosuspend(rzn1_adc->dev);
> > > +
> > > +	rzn1_adc_disable(rzn1_adc);
> > > +}  
> > 
> > I'm fairly confident we can sanely go without .remove().
> 
> Will see what I can be do for the next iteration.
> 
> Maybe I will ask some questions if I need some clarification around
> pm_runtime but let me first try to go further in that direction.

Yeah, maybe you can come up with something but given how you use pm to
enable/disable stuff I'm also not sure the above is easily doable.

- Nuno Sá

Re: [PATCH 2/4] iio: adc: Add support for the Renesas RZ/N1 ADC

[Herve Codina]

Hi Nuno,

On Thu, 16 Oct 2025 10:24:36 +0100
Nuno Sá <noname.nuno@gmail.com> wrote:

> On Wed, 2025-10-15 at 21:14 +0200, Herve Codina wrote:
> > Hi Nuno,
> > 
> > On Wed, 15 Oct 2025 16:21:09 +0100
> > Nuno Sá <noname.nuno@gmail.com> wrote:
> > 
> > ...  
> > >   
> > > > +static int rzn1_adc_enable(struct rzn1_adc *rzn1_adc)
> > > > +{
> > > > +	int ret;
> > > > +
> > > > +	ret = rzn1_adc_core_power_on(&rzn1_adc->adc_core[0]);
> > > > +	if (ret)
> > > > +		return ret;
> > > > +
> > > > +	ret = rzn1_adc_core_power_on(&rzn1_adc->adc_core[1]);
> > > > +	if (ret)
> > > > +		goto poweroff_adc_core0;
> > > > +
> > > > +	ret = clk_prepare_enable(rzn1_adc->pclk);
> > > > +	if (ret)
> > > > +		goto poweroff_adc_core1;
> > > > +
> > > > +	ret = clk_prepare_enable(rzn1_adc->adc_clk);
> > > > +	if (ret)
> > > > +		goto disable_pclk;
> > > > +
> > > > +	ret = rzn1_adc_power(rzn1_adc, true);
> > > > +	if (ret)
> > > > +		goto disable_adc_clk;    
> > > 
> > > Can we use devm_actions() on the above to avoid the complex error path plus
> > > the
> > > .remove() callback?  
> > 
> > rzn1_adc_enable() is used by the driver pm_runtime_resume() function.
> > 
> > I don't think that devm_add_actions_or_reset() will help here.
> > 
> > In my understanding, devm_* functions are use to perform some operations
> > automatically on device removal.
> > 
> > The purpose of the error path here is to restore a correct state if
> > rzn1_adc_enable() failed when it is called from pm_runtime_resume().
> > 
> > In that case no device removal is involved to trig any action set by
> > devm_add_actions_or_reset().
> > 
> > Maybe I am wrong. Did I miss something?  
> 
> Nope, I see now what's your intent.

Ok, no change planned for the next iteration related to this error path.

> 
> >   
> > >   
> > > > +
> > > > +	return 0;
> > > > +
> > > > +disable_adc_clk:
> > > > +	clk_disable_unprepare(rzn1_adc->adc_clk);
> > > > +disable_pclk:
> > > > +	clk_disable_unprepare(rzn1_adc->pclk);
> > > > +poweroff_adc_core1:
> > > > +	rzn1_adc_core_power_off(&rzn1_adc->adc_core[1]);
> > > > +poweroff_adc_core0:
> > > > +	rzn1_adc_core_power_off(&rzn1_adc->adc_core[0]);
> > > > +	return ret;
> > > > +}
> > > > +  
> > 
> > ...
> >   
> > > > +static int rzn1_adc_set_iio_dev_channels(struct rzn1_adc *rzn1_adc,
> > > > +					 struct iio_dev *indio_dev)
> > > > +{
> > > > +	int adc_used;
> > > > +
> > > > +	adc_used = rzn1_adc->adc_core[0].is_used ? 0x01 : 0x00;
> > > > +	adc_used |= rzn1_adc->adc_core[1].is_used ? 0x02 : 0x00;
> > > > +
> > > > +	switch (adc_used) {
> > > > +	case 0x01:
> > > > +		indio_dev->channels = rzn1_adc1_channels;
> > > > +		indio_dev->num_channels = ARRAY_SIZE(rzn1_adc1_channels);
> > > > +		return 0;
> > > > +	case 0x02:
> > > > +		indio_dev->channels = rzn1_adc2_channels;
> > > > +		indio_dev->num_channels = ARRAY_SIZE(rzn1_adc2_channels);
> > > > +		return 0;
> > > > +	case 0x03:
> > > > +		indio_dev->channels = rzn1_adc1_adc2_channels;
> > > > +		indio_dev->num_channels =
> > > > ARRAY_SIZE(rzn1_adc1_adc2_channels);
> > > > +		return 0;
> > > > +	default:
> > > > +		break;
> > > > +	}
> > > > +
> > > > +	dev_err(rzn1_adc->dev, "Failed to set IIO channels, no ADC core
> > > > used\n");
> > > > +	return -ENODEV;    
> > > 
> > > dev_err_probe()?  
> > 
> > Why? the error returned is a well known value: -ENODEV.
> > 
> > dev_err_probe() should be involved when -EPROBE_DEFER is a potential error
> > code.
> > 
> > IMHO, dev_err() here is correct.  
> 
> If I'm not missing nothing this function is called during probe so I do think
> dev_err_probe() should be used. Not only unifies logging style during probe it
> also has the small benefit of doing:
> 
> return dev_err_probe(...) saving a line of code.
> 
> You can see that, at least in IIO, we even have some patches just converting
> drivers probe() to use dev_err_probe().

Right, I will use dev_err_probe() in the next iteration.

> 
> >   
> > >   
> > > > +}
> > > > +
> > > > +static int rzn1_adc_probe(struct platform_device *pdev)
> > > > +{
> > > > +	struct device *dev = &pdev->dev;
> > > > +	struct iio_dev *indio_dev;
> > > > +	struct rzn1_adc *rzn1_adc;
> > > > +	int ret;
> > > > +
> > > > +	indio_dev = devm_iio_device_alloc(dev, sizeof(*rzn1_adc));
> > > > +	if (!indio_dev)
> > > > +		return -ENOMEM;
> > > > +
> > > > +	rzn1_adc = iio_priv(indio_dev);
> > > > +	rzn1_adc->dev = dev;
> > > > +	mutex_init(&rzn1_adc->lock);    
> > > 
> > > devm_mutex_init()  
> > 
> > Yes, I will update in the next iteration.
> >   
> > >   
> > > > +
> > > > +	rzn1_adc->regs = devm_platform_ioremap_resource(pdev, 0);
> > > > +	if (IS_ERR(rzn1_adc->regs))
> > > > +		return PTR_ERR(rzn1_adc->regs);
> > > > +
> > > > +	rzn1_adc->pclk = devm_clk_get(dev, "pclk");
> > > > +	if (IS_ERR(rzn1_adc->pclk))
> > > > +		return dev_err_probe(dev, PTR_ERR(rzn1_adc->pclk),
> > > > "Failed to
> > > > get pclk\n");
> > > > +
> > > > +	rzn1_adc->adc_clk = devm_clk_get(dev, "adc-clk");
> > > > +	if (IS_ERR(rzn1_adc->pclk))
> > > > +		return dev_err_probe(dev, PTR_ERR(rzn1_adc->pclk),
> > > > "Failed to
> > > > get adc-clk\n");
> > > > +
> > > > +	ret = rzn1_adc_core_get_regulators(rzn1_adc, &rzn1_adc-  
> > > > >adc_core[0],  
> > > > +					   "adc1-avdd", "adc1-vref");
> > > > +	if (ret)
> > > > +		return ret;
> > > > +
> > > > +	ret = rzn1_adc_core_get_regulators(rzn1_adc, &rzn1_adc-  
> > > > >adc_core[1],  
> > > > +					   "adc2-avdd", "adc2-vref");
> > > > +	if (ret)
> > > > +		return ret;    
> > > 
> > > Hmm, is avdd really an optional regulator? I mean can the ADC power up at
> > > all
> > > without a supply in AVDD? Even vref seems to be mandatory as we can't
> > > properly
> > > scale the sample without it.  
> > 
> > Where do you see that avdd is an optional regulator?  
> 
> You are using devm_regulator_get_optional(). That's for optional regulators.
> 

Indeed I use devm_regulator_get_optional().

We have two similar function to get regulators:
- devm_regulator_get() and
- devm_regulator_get_optional().

devm_regulator_get() returns a dummy regulator if the regulator is not
described in the device-tree. The calling code has no way to known if
the regulator was present or not.

On the other hand, devm_regulator_get_optional() returns -ENODEV when the
regulator is not described.

That's pretty confusing but it is the reality.

I use devm_regulator_get_optional() but check for -ENODEV to see if the
regulator is provided or not.

In order to use the ADC core (is_used flag), I need both the AVDD and the
VREF regulator available.

> >   
> > > 
> > > Also, can't we have getting and enabling the regulator together? Then, we
> > > could
> > > use some of the modern helpers to simplify the code (ok I see you use them
> > > in
> > > the PM callbacks).  
> > 
> > Yes, I rely on PM callbacks to handle those regulators.
> >   
> > >   
> > > > +
> > > > +	platform_set_drvdata(pdev, indio_dev);
> > > > +
> > > > +	indio_dev->name = dev_name(dev);    
> > > 
> > > dev_name() should not be used for the above. It's typically the part name so
> > > I
> > > guess in here "rzn1-adc" would be the appropriate one.  
> > 
> > I thought it was more related to the instance and so having a different name
> > for each instance was better.
> > 
> > Some other IIO drivers use dev_name() here.
> > 
> > But well, if you confirm that a fixed string should be used and so all
> > instances have the same string, no problem, I will update my indio_dev->name.  
> 
> It is a fixed string, typically the part name. David Lechner not that long ago
> actually sent some patch or documented somewhere why not to use dev_name(). To
> identify different instances we have a 'label' property.

Right, I will set indio_dev->name to the "rzn1-adc" fixed string.

> 
> >   
> > >   
> > > > +	indio_dev->info = &rzn1_adc_info;
> > > > +	indio_dev->modes = INDIO_DIRECT_MODE;
> > > > +	ret = rzn1_adc_set_iio_dev_channels(rzn1_adc, indio_dev);
> > > > +	if (ret)
> > > > +		return ret;
> > > > +
> > > > +	ret = rzn1_adc_enable(rzn1_adc);
> > > > +	if (ret)
> > > > +		return ret;
> > > > +
> > > > +	pm_runtime_set_autosuspend_delay(dev, 500);
> > > > +	pm_runtime_use_autosuspend(dev);
> > > > +	pm_runtime_get_noresume(dev);
> > > > +	pm_runtime_set_active(dev);
> > > > +	pm_runtime_enable(dev);    
> > > 
> > > There's a devm_pm_runtime_enable() API now.  
> > 
> > Will look to use it in the next iteration.
> >   
> > >   
> > > > +
> > > > +	ret = devm_iio_device_register(dev, indio_dev);
> > > > +	if (ret)
> > > > +		goto disable;
> > > > +
> > > > +	pm_runtime_mark_last_busy(dev);
> > > > +	pm_runtime_put_autosuspend(dev);
> > > > +
> > > > +	return 0;
> > > > +
> > > > +disable:
> > > > +	pm_runtime_disable(dev);
> > > > +	pm_runtime_put_noidle(dev);
> > > > +	pm_runtime_set_suspended(dev);
> > > > +	pm_runtime_dont_use_autosuspend(dev);
> > > > +
> > > > +	rzn1_adc_disable(rzn1_adc);
> > > > +	return ret;
> > > > +}
> > > > +
> > > > +static void rzn1_adc_remove(struct platform_device *pdev)
> > > > +{
> > > > +	struct iio_dev *indio_dev = platform_get_drvdata(pdev);
> > > > +	struct rzn1_adc *rzn1_adc = iio_priv(indio_dev);
> > > > +
> > > > +	pm_runtime_disable(rzn1_adc->dev);
> > > > +	pm_runtime_set_suspended(rzn1_adc->dev);
> > > > +	pm_runtime_dont_use_autosuspend(rzn1_adc->dev);
> > > > +
> > > > +	rzn1_adc_disable(rzn1_adc);
> > > > +}    
> > > 
> > > I'm fairly confident we can sanely go without .remove().  
> > 
> > Will see what I can be do for the next iteration.
> > 
> > Maybe I will ask some questions if I need some clarification around
> > pm_runtime but let me first try to go further in that direction.  
> 
> Yeah, maybe you can come up with something but given how you use pm to
> enable/disable stuff I'm also not sure the above is easily doable.
> 

Hum, do you think it's worth a try?

Best regards,
Hervé

Re: [PATCH 2/4] iio: adc: Add support for the Renesas RZ/N1 ADC

[Nuno Sá]

On Thu, 2025-10-16 at 16:02 +0200, Herve Codina wrote:
> Hi Nuno,
> 
> On Thu, 16 Oct 2025 10:24:36 +0100
> Nuno Sá <noname.nuno@gmail.com> wrote:
> 
> > On Wed, 2025-10-15 at 21:14 +0200, Herve Codina wrote:
> > > Hi Nuno,
> > > 
> > > On Wed, 15 Oct 2025 16:21:09 +0100
> > > Nuno Sá <noname.nuno@gmail.com> wrote:
> > > 
> > > ...  
> > > >   
> > > > > +static int rzn1_adc_enable(struct rzn1_adc *rzn1_adc)
> > > > > +{
> > > > > +	int ret;
> > > > > +
> > > > > +	ret = rzn1_adc_core_power_on(&rzn1_adc->adc_core[0]);
> > > > > +	if (ret)
> > > > > +		return ret;
> > > > > +
> > > > > +	ret = rzn1_adc_core_power_on(&rzn1_adc->adc_core[1]);
> > > > > +	if (ret)
> > > > > +		goto poweroff_adc_core0;
> > > > > +
> > > > > +	ret = clk_prepare_enable(rzn1_adc->pclk);
> > > > > +	if (ret)
> > > > > +		goto poweroff_adc_core1;
> > > > > +
> > > > > +	ret = clk_prepare_enable(rzn1_adc->adc_clk);
> > > > > +	if (ret)
> > > > > +		goto disable_pclk;
> > > > > +
> > > > > +	ret = rzn1_adc_power(rzn1_adc, true);
> > > > > +	if (ret)
> > > > > +		goto disable_adc_clk;    
> > > > 
> > > > Can we use devm_actions() on the above to avoid the complex error path
> > > > plus
> > > > the
> > > > .remove() callback?  
> > > 
> > > rzn1_adc_enable() is used by the driver pm_runtime_resume() function.
> > > 
> > > I don't think that devm_add_actions_or_reset() will help here.
> > > 
> > > In my understanding, devm_* functions are use to perform some operations
> > > automatically on device removal.
> > > 
> > > The purpose of the error path here is to restore a correct state if
> > > rzn1_adc_enable() failed when it is called from pm_runtime_resume().
> > > 
> > > In that case no device removal is involved to trig any action set by
> > > devm_add_actions_or_reset().
> > > 
> > > Maybe I am wrong. Did I miss something?  
> > 
> > Nope, I see now what's your intent.
> 
> Ok, no change planned for the next iteration related to this error path.
> 
> > 
> > >   
> > > >   
> > > > > +
> > > > > +	return 0;
> > > > > +
> > > > > +disable_adc_clk:
> > > > > +	clk_disable_unprepare(rzn1_adc->adc_clk);
> > > > > +disable_pclk:
> > > > > +	clk_disable_unprepare(rzn1_adc->pclk);
> > > > > +poweroff_adc_core1:
> > > > > +	rzn1_adc_core_power_off(&rzn1_adc->adc_core[1]);
> > > > > +poweroff_adc_core0:
> > > > > +	rzn1_adc_core_power_off(&rzn1_adc->adc_core[0]);
> > > > > +	return ret;
> > > > > +}
> > > > > +  
> > > 
> > > ...
> > >   
> > > > > +static int rzn1_adc_set_iio_dev_channels(struct rzn1_adc *rzn1_adc,
> > > > > +					 struct iio_dev *indio_dev)
> > > > > +{
> > > > > +	int adc_used;
> > > > > +
> > > > > +	adc_used = rzn1_adc->adc_core[0].is_used ? 0x01 : 0x00;
> > > > > +	adc_used |= rzn1_adc->adc_core[1].is_used ? 0x02 : 0x00;
> > > > > +
> > > > > +	switch (adc_used) {
> > > > > +	case 0x01:
> > > > > +		indio_dev->channels = rzn1_adc1_channels;
> > > > > +		indio_dev->num_channels =
> > > > > ARRAY_SIZE(rzn1_adc1_channels);
> > > > > +		return 0;
> > > > > +	case 0x02:
> > > > > +		indio_dev->channels = rzn1_adc2_channels;
> > > > > +		indio_dev->num_channels =
> > > > > ARRAY_SIZE(rzn1_adc2_channels);
> > > > > +		return 0;
> > > > > +	case 0x03:
> > > > > +		indio_dev->channels = rzn1_adc1_adc2_channels;
> > > > > +		indio_dev->num_channels =
> > > > > ARRAY_SIZE(rzn1_adc1_adc2_channels);
> > > > > +		return 0;
> > > > > +	default:
> > > > > +		break;
> > > > > +	}
> > > > > +
> > > > > +	dev_err(rzn1_adc->dev, "Failed to set IIO channels, no ADC
> > > > > core
> > > > > used\n");
> > > > > +	return -ENODEV;    
> > > > 
> > > > dev_err_probe()?  
> > > 
> > > Why? the error returned is a well known value: -ENODEV.
> > > 
> > > dev_err_probe() should be involved when -EPROBE_DEFER is a potential error
> > > code.
> > > 
> > > IMHO, dev_err() here is correct.  
> > 
> > If I'm not missing nothing this function is called during probe so I do
> > think
> > dev_err_probe() should be used. Not only unifies logging style during probe
> > it
> > also has the small benefit of doing:
> > 
> > return dev_err_probe(...) saving a line of code.
> > 
> > You can see that, at least in IIO, we even have some patches just converting
> > drivers probe() to use dev_err_probe().
> 
> Right, I will use dev_err_probe() in the next iteration.
> 
> > 
> > >   
> > > >   
> > > > > +}
> > > > > +
> > > > > +static int rzn1_adc_probe(struct platform_device *pdev)
> > > > > +{
> > > > > +	struct device *dev = &pdev->dev;
> > > > > +	struct iio_dev *indio_dev;
> > > > > +	struct rzn1_adc *rzn1_adc;
> > > > > +	int ret;
> > > > > +
> > > > > +	indio_dev = devm_iio_device_alloc(dev, sizeof(*rzn1_adc));
> > > > > +	if (!indio_dev)
> > > > > +		return -ENOMEM;
> > > > > +
> > > > > +	rzn1_adc = iio_priv(indio_dev);
> > > > > +	rzn1_adc->dev = dev;
> > > > > +	mutex_init(&rzn1_adc->lock);    
> > > > 
> > > > devm_mutex_init()  
> > > 
> > > Yes, I will update in the next iteration.
> > >   
> > > >   
> > > > > +
> > > > > +	rzn1_adc->regs = devm_platform_ioremap_resource(pdev, 0);
> > > > > +	if (IS_ERR(rzn1_adc->regs))
> > > > > +		return PTR_ERR(rzn1_adc->regs);
> > > > > +
> > > > > +	rzn1_adc->pclk = devm_clk_get(dev, "pclk");
> > > > > +	if (IS_ERR(rzn1_adc->pclk))
> > > > > +		return dev_err_probe(dev, PTR_ERR(rzn1_adc->pclk),
> > > > > "Failed to
> > > > > get pclk\n");
> > > > > +
> > > > > +	rzn1_adc->adc_clk = devm_clk_get(dev, "adc-clk");
> > > > > +	if (IS_ERR(rzn1_adc->pclk))
> > > > > +		return dev_err_probe(dev, PTR_ERR(rzn1_adc->pclk),
> > > > > "Failed to
> > > > > get adc-clk\n");
> > > > > +
> > > > > +	ret = rzn1_adc_core_get_regulators(rzn1_adc, &rzn1_adc-  
> > > > > > adc_core[0],  
> > > > > +					   "adc1-avdd", "adc1-vref");
> > > > > +	if (ret)
> > > > > +		return ret;
> > > > > +
> > > > > +	ret = rzn1_adc_core_get_regulators(rzn1_adc, &rzn1_adc-  
> > > > > > adc_core[1],  
> > > > > +					   "adc2-avdd", "adc2-vref");
> > > > > +	if (ret)
> > > > > +		return ret;    
> > > > 
> > > > Hmm, is avdd really an optional regulator? I mean can the ADC power up
> > > > at
> > > > all
> > > > without a supply in AVDD? Even vref seems to be mandatory as we can't
> > > > properly
> > > > scale the sample without it.  
> > > 
> > > Where do you see that avdd is an optional regulator?  
> > 
> > You are using devm_regulator_get_optional(). That's for optional regulators.
> > 
> 
> Indeed I use devm_regulator_get_optional().
> 
> We have two similar function to get regulators:
> - devm_regulator_get() and
> - devm_regulator_get_optional().
> 
> devm_regulator_get() returns a dummy regulator if the regulator is not
> described in the device-tree. The calling code has no way to known if
> the regulator was present or not.

Yeah because it's mandatory and the part cannot work without power :). So we
should not be allowed to operate without a regulator.

> 
> On the other hand, devm_regulator_get_optional() returns -ENODEV when the
> regulator is not described.
> 
> That's pretty confusing but it is the reality.
> 
> I use devm_regulator_get_optional() but check for -ENODEV to see if the
> regulator is provided or not.
> 
> In order to use the ADC core (is_used flag), I need both the AVDD and the
> VREF regulator available.

And that is why I don't get why are we allowed to proceed if there's no
regulators? That seems wrong to me. 

So I think the regulators should be mandatory in the bindings and a dummy
regulator should also not be allowed in this case because that should get you 
-EINVAL when calling regulator_get_voltage().

> 
> > >   
> > > > 
> > > > Also, can't we have getting and enabling the regulator together? Then,
> > > > we
> > > > could
> > > > use some of the modern helpers to simplify the code (ok I see you use
> > > > them
> > > > in
> > > > the PM callbacks).  
> > > 
> > > Yes, I rely on PM callbacks to handle those regulators.
> > >   
> > > >   
> > > > > +
> > > > > +	platform_set_drvdata(pdev, indio_dev);
> > > > > +
> > > > > +	indio_dev->name = dev_name(dev);    
> > > > 
> > > > dev_name() should not be used for the above. It's typically the part
> > > > name so
> > > > I
> > > > guess in here "rzn1-adc" would be the appropriate one.  
> > > 
> > > I thought it was more related to the instance and so having a different
> > > name
> > > for each instance was better.
> > > 
> > > Some other IIO drivers use dev_name() here.
> > > 
> > > But well, if you confirm that a fixed string should be used and so all
> > > instances have the same string, no problem, I will update my indio_dev-
> > > >name.  
> > 
> > It is a fixed string, typically the part name. David Lechner not that long
> > ago
> > actually sent some patch or documented somewhere why not to use dev_name().
> > To
> > identify different instances we have a 'label' property.
> 
> Right, I will set indio_dev->name to the "rzn1-adc" fixed string.
> 
> > 
> > >   
> > > >   
> > > > > +	indio_dev->info = &rzn1_adc_info;
> > > > > +	indio_dev->modes = INDIO_DIRECT_MODE;
> > > > > +	ret = rzn1_adc_set_iio_dev_channels(rzn1_adc, indio_dev);
> > > > > +	if (ret)
> > > > > +		return ret;
> > > > > +
> > > > > +	ret = rzn1_adc_enable(rzn1_adc);
> > > > > +	if (ret)
> > > > > +		return ret;
> > > > > +
> > > > > +	pm_runtime_set_autosuspend_delay(dev, 500);
> > > > > +	pm_runtime_use_autosuspend(dev);
> > > > > +	pm_runtime_get_noresume(dev);
> > > > > +	pm_runtime_set_active(dev);
> > > > > +	pm_runtime_enable(dev);    
> > > > 
> > > > There's a devm_pm_runtime_enable() API now.  
> > > 
> > > Will look to use it in the next iteration.
> > >   
> > > >   
> > > > > +
> > > > > +	ret = devm_iio_device_register(dev, indio_dev);
> > > > > +	if (ret)
> > > > > +		goto disable;
> > > > > +
> > > > > +	pm_runtime_mark_last_busy(dev);
> > > > > +	pm_runtime_put_autosuspend(dev);
> > > > > +
> > > > > +	return 0;
> > > > > +
> > > > > +disable:
> > > > > +	pm_runtime_disable(dev);
> > > > > +	pm_runtime_put_noidle(dev);
> > > > > +	pm_runtime_set_suspended(dev);
> > > > > +	pm_runtime_dont_use_autosuspend(dev);
> > > > > +
> > > > > +	rzn1_adc_disable(rzn1_adc);
> > > > > +	return ret;
> > > > > +}
> > > > > +
> > > > > +static void rzn1_adc_remove(struct platform_device *pdev)
> > > > > +{
> > > > > +	struct iio_dev *indio_dev = platform_get_drvdata(pdev);
> > > > > +	struct rzn1_adc *rzn1_adc = iio_priv(indio_dev);
> > > > > +
> > > > > +	pm_runtime_disable(rzn1_adc->dev);
> > > > > +	pm_runtime_set_suspended(rzn1_adc->dev);
> > > > > +	pm_runtime_dont_use_autosuspend(rzn1_adc->dev);
> > > > > +
> > > > > +	rzn1_adc_disable(rzn1_adc);
> > > > > +}    
> > > > 
> > > > I'm fairly confident we can sanely go without .remove().  
> > > 
> > > Will see what I can be do for the next iteration.
> > > 
> > > Maybe I will ask some questions if I need some clarification around
> > > pm_runtime but let me first try to go further in that direction.  
> > 
> > Yeah, maybe you can come up with something but given how you use pm to
> > enable/disable stuff I'm also not sure the above is easily doable.
> > 
> 
> Hum, do you think it's worth a try?

Not sure. But it got me thinking about all this handling in the pm runtime
routines. So if in the resume() call you fail at some point and then disable
stuff in your return path and then we get an unbind won't things (clocks and
regulators) be unbalanced leading to splats? In fact by just looking at the
unbind path [1] I can see:

1. We call pm_runtime_get_sync(dev) which can fail;
2. Later on we call pm_runtime_put_sync(dev).

Not really sure if there's special handling in the pm core to be aware that
resuming failed (the refcount seems to be incremented [2] before resuming so...)

Maybe I would keep it simple and get and enable clocks/regulators during probe
and only care of rzn1_adc_power() in the runtime routines. My 2 cents.

[1]: https://elixir.bootlin.com/linux/v6.17.1/source/drivers/base/dd.c#L1249
[2]: https://elixir.bootlin.com/linux/v6.17.1/source/drivers/base/power/runtime.c#L1189

- Nuno Sá

Re: [PATCH 2/4] iio: adc: Add support for the Renesas RZ/N1 ADC

[Herve Codina]

Hi Nuno,

On Thu, 16 Oct 2025 16:26:28 +0100
Nuno Sá <noname.nuno@gmail.com> wrote:

...

...
> > > > > > +
> > > > > > +	ret = rzn1_adc_core_get_regulators(rzn1_adc, &rzn1_adc-    
> > > > > > > adc_core[0],    
> > > > > > +					   "adc1-avdd", "adc1-vref");
> > > > > > +	if (ret)
> > > > > > +		return ret;
> > > > > > +
> > > > > > +	ret = rzn1_adc_core_get_regulators(rzn1_adc, &rzn1_adc-    
> > > > > > > adc_core[1],    
> > > > > > +					   "adc2-avdd", "adc2-vref");
> > > > > > +	if (ret)
> > > > > > +		return ret;      
> > > > > 
> > > > > Hmm, is avdd really an optional regulator? I mean can the ADC power up
> > > > > at
> > > > > all
> > > > > without a supply in AVDD? Even vref seems to be mandatory as we can't
> > > > > properly
> > > > > scale the sample without it.    
> > > > 
> > > > Where do you see that avdd is an optional regulator?    
> > > 
> > > You are using devm_regulator_get_optional(). That's for optional regulators.
> > >   
> > 
> > Indeed I use devm_regulator_get_optional().
> > 
> > We have two similar function to get regulators:
> > - devm_regulator_get() and
> > - devm_regulator_get_optional().
> > 
> > devm_regulator_get() returns a dummy regulator if the regulator is not
> > described in the device-tree. The calling code has no way to known if
> > the regulator was present or not.  
> 
> Yeah because it's mandatory and the part cannot work without power :). So we
> should not be allowed to operate without a regulator.
> 
> > 
> > On the other hand, devm_regulator_get_optional() returns -ENODEV when the
> > regulator is not described.
> > 
> > That's pretty confusing but it is the reality.
> > 
> > I use devm_regulator_get_optional() but check for -ENODEV to see if the
> > regulator is provided or not.
> > 
> > In order to use the ADC core (is_used flag), I need both the AVDD and the
> > VREF regulator available.  
> 
> And that is why I don't get why are we allowed to proceed if there's no
> regulators? That seems wrong to me. 
> 
> So I think the regulators should be mandatory in the bindings and a dummy
> regulator should also not be allowed in this case because that should get you 
> -EINVAL when calling regulator_get_voltage().
> 

I have 4 regulators: avdd1, vref1, avvd2, vref2.

The ADC controller can work with 2 internal ADC core (adc_core[0] and adc_core[1])
in the driver. Those internal core are not directly accessed by the driver. Only
the ADC controller is accesses.

Those cores have an AVDD and a VREF power supply.

We can use either adc_core[0] only, adc_core[1] only or both adc cores.

Depending on regulator described, the driver uses one or two adc cores.

This choice is done by:
--- 8< ---
static int rzn1_adc_set_iio_dev_channels(struct rzn1_adc *rzn1_adc,
					 struct iio_dev *indio_dev)
{
	int adc_used;

	adc_used = rzn1_adc->adc_core[0].is_used ? 0x01 : 0x00;
	adc_used |= rzn1_adc->adc_core[1].is_used ? 0x02 : 0x00;

	switch (adc_used) {
	case 0x01:
		indio_dev->channels = rzn1_adc1_channels;
		indio_dev->num_channels = ARRAY_SIZE(rzn1_adc1_channels);
		return 0;
	case 0x02:
		indio_dev->channels = rzn1_adc2_channels;
		indio_dev->num_channels = ARRAY_SIZE(rzn1_adc2_channels);
		return 0;
	case 0x03:
		indio_dev->channels = rzn1_adc1_adc2_channels;
		indio_dev->num_channels = ARRAY_SIZE(rzn1_adc1_adc2_channels);
		return 0;
	default:
		break;
	}
--- 8< ---

In rzn1_adc_core_get_regulators(), looking at one core I do the
following:
 - Try to get AVDD (using get_optional)
 - Try to get VREF (using get_optional)
 - Core is used only if both regulators are present.

For one core to be used, both regulators have to be present.

Regulators are mandatory but adc core usage is optional.

This optional usage depends on related regulator presence.


> > 
> > Hum, do you think it's worth a try?  
> 
> Not sure. But it got me thinking about all this handling in the pm runtime
> routines. So if in the resume() call you fail at some point and then disable
> stuff in your return path and then we get an unbind won't things (clocks and
> regulators) be unbalanced leading to splats? In fact by just looking at the
> unbind path [1] I can see:
> 
> 1. We call pm_runtime_get_sync(dev) which can fail;
> 2. Later on we call pm_runtime_put_sync(dev).
> 
> Not really sure if there's special handling in the pm core to be aware that
> resuming failed (the refcount seems to be incremented [2] before resuming so...)
> 
> Maybe I would keep it simple and get and enable clocks/regulators during probe
> and only care of rzn1_adc_power() in the runtime routines. My 2 cents.
> 
> [1]: https://elixir.bootlin.com/linux/v6.17.1/source/drivers/base/dd.c#L1249
> [2]: https://elixir.bootlin.com/linux/v6.17.1/source/drivers/base/power/runtime.c#L1189
> 

I see, thanks for those clarifications.
I will simplify and, as you proposed, I will only take care of rzn1_adc_power().

Best regards,
Hervé

Re: [PATCH 2/4] iio: adc: Add support for the Renesas RZ/N1 ADC

[Nuno Sá]

On Fri, 2025-10-17 at 08:59 +0200, Herve Codina wrote:
> Hi Nuno,
> 
> On Thu, 16 Oct 2025 16:26:28 +0100
> Nuno Sá <noname.nuno@gmail.com> wrote:
> 
> ...
> 
> ...
> > > > > > > +
> > > > > > > +	ret = rzn1_adc_core_get_regulators(rzn1_adc, &rzn1_adc-
> > > > > > >     
> > > > > > > > adc_core[0],    
> > > > > > > +					   "adc1-avdd", "adc1-
> > > > > > > vref");
> > > > > > > +	if (ret)
> > > > > > > +		return ret;
> > > > > > > +
> > > > > > > +	ret = rzn1_adc_core_get_regulators(rzn1_adc, &rzn1_adc-
> > > > > > >     
> > > > > > > > adc_core[1],    
> > > > > > > +					   "adc2-avdd", "adc2-
> > > > > > > vref");
> > > > > > > +	if (ret)
> > > > > > > +		return ret;      
> > > > > > 
> > > > > > Hmm, is avdd really an optional regulator? I mean can the ADC power
> > > > > > up
> > > > > > at
> > > > > > all
> > > > > > without a supply in AVDD? Even vref seems to be mandatory as we
> > > > > > can't
> > > > > > properly
> > > > > > scale the sample without it.    
> > > > > 
> > > > > Where do you see that avdd is an optional regulator?    
> > > > 
> > > > You are using devm_regulator_get_optional(). That's for optional
> > > > regulators.
> > > >   
> > > 
> > > Indeed I use devm_regulator_get_optional().
> > > 
> > > We have two similar function to get regulators:
> > > - devm_regulator_get() and
> > > - devm_regulator_get_optional().
> > > 
> > > devm_regulator_get() returns a dummy regulator if the regulator is not
> > > described in the device-tree. The calling code has no way to known if
> > > the regulator was present or not.  
> > 
> > Yeah because it's mandatory and the part cannot work without power :). So we
> > should not be allowed to operate without a regulator.
> > 
> > > 
> > > On the other hand, devm_regulator_get_optional() returns -ENODEV when the
> > > regulator is not described.
> > > 
> > > That's pretty confusing but it is the reality.
> > > 
> > > I use devm_regulator_get_optional() but check for -ENODEV to see if the
> > > regulator is provided or not.
> > > 
> > > In order to use the ADC core (is_used flag), I need both the AVDD and the
> > > VREF regulator available.  
> > 
> > And that is why I don't get why are we allowed to proceed if there's no
> > regulators? That seems wrong to me. 
> > 
> > So I think the regulators should be mandatory in the bindings and a dummy
> > regulator should also not be allowed in this case because that should get
> > you 
> > -EINVAL when calling regulator_get_voltage().
> > 
> 
> I have 4 regulators: avdd1, vref1, avvd2, vref2.
> 
> The ADC controller can work with 2 internal ADC core (adc_core[0] and
> adc_core[1])
> in the driver. Those internal core are not directly accessed by the driver.
> Only
> the ADC controller is accesses.
> 
> Those cores have an AVDD and a VREF power supply.
> 
> We can use either adc_core[0] only, adc_core[1] only or both adc cores.
> 
> Depending on regulator described, the driver uses one or two adc cores.
> 
> This choice is done by:
> --- 8< ---
> static int rzn1_adc_set_iio_dev_channels(struct rzn1_adc *rzn1_adc,
> 					 struct iio_dev *indio_dev)
> {
> 	int adc_used;
> 
> 	adc_used = rzn1_adc->adc_core[0].is_used ? 0x01 : 0x00;
> 	adc_used |= rzn1_adc->adc_core[1].is_used ? 0x02 : 0x00;
> 
> 	switch (adc_used) {
> 	case 0x01:
> 		indio_dev->channels = rzn1_adc1_channels;
> 		indio_dev->num_channels = ARRAY_SIZE(rzn1_adc1_channels);
> 		return 0;
> 	case 0x02:
> 		indio_dev->channels = rzn1_adc2_channels;
> 		indio_dev->num_channels = ARRAY_SIZE(rzn1_adc2_channels);
> 		return 0;
> 	case 0x03:
> 		indio_dev->channels = rzn1_adc1_adc2_channels;
> 		indio_dev->num_channels =
> ARRAY_SIZE(rzn1_adc1_adc2_channels);
> 		return 0;
> 	default:
> 		break;
> 	}
> --- 8< ---
> 
> In rzn1_adc_core_get_regulators(), looking at one core I do the
> following:
>  - Try to get AVDD (using get_optional)
>  - Try to get VREF (using get_optional)
>  - Core is used only if both regulators are present.
> 
> For one core to be used, both regulators have to be present.
> 
> Regulators are mandatory but adc core usage is optional.
> 
> This optional usage depends on related regulator presence.
> 

Ok, then we could flip the logic and have boolean properties for the adc core
usage and depending on that, requesting the regulators. To me, the intent would
be more clear (at the expense of more FW properties).

Having said that, the above helps a lot in understanding what's going on and
explains the get_optional() usage. I'm not still 100% convinced but bah, fine :)

I would still argue that you should have a comment (likely in get_regulators())
explaining the logic and the optional usage.

Given the above I think you could also remove:

if (!adc_core->vref)
	return -ENODEV;

from rzn1_adc_core_get_vref_mv() since the channels are only exposed in case the
regulators are present.


- Nuno Sá

Re: [PATCH 2/4] iio: adc: Add support for the Renesas RZ/N1 ADC

[Herve Codina]

I Nuno,

On Fri, 17 Oct 2025 09:26:26 +0100
Nuno Sá <noname.nuno@gmail.com> wrote:

> On Fri, 2025-10-17 at 08:59 +0200, Herve Codina wrote:
> > Hi Nuno,
> > 
> > On Thu, 16 Oct 2025 16:26:28 +0100
> > Nuno Sá <noname.nuno@gmail.com> wrote:
> > 
> > ...
> > 
> > ...  
> > > > > > > > +
> > > > > > > > +	ret = rzn1_adc_core_get_regulators(rzn1_adc, &rzn1_adc-
> > > > > > > >       
> > > > > > > > > adc_core[0],      
> > > > > > > > +					   "adc1-avdd", "adc1-
> > > > > > > > vref");
> > > > > > > > +	if (ret)
> > > > > > > > +		return ret;
> > > > > > > > +
> > > > > > > > +	ret = rzn1_adc_core_get_regulators(rzn1_adc, &rzn1_adc-
> > > > > > > >       
> > > > > > > > > adc_core[1],      
> > > > > > > > +					   "adc2-avdd", "adc2-
> > > > > > > > vref");
> > > > > > > > +	if (ret)
> > > > > > > > +		return ret;        
> > > > > > > 
> > > > > > > Hmm, is avdd really an optional regulator? I mean can the ADC power
> > > > > > > up
> > > > > > > at
> > > > > > > all
> > > > > > > without a supply in AVDD? Even vref seems to be mandatory as we
> > > > > > > can't
> > > > > > > properly
> > > > > > > scale the sample without it.      
> > > > > > 
> > > > > > Where do you see that avdd is an optional regulator?      
> > > > > 
> > > > > You are using devm_regulator_get_optional(). That's for optional
> > > > > regulators.
> > > > >     
> > > > 
> > > > Indeed I use devm_regulator_get_optional().
> > > > 
> > > > We have two similar function to get regulators:
> > > > - devm_regulator_get() and
> > > > - devm_regulator_get_optional().
> > > > 
> > > > devm_regulator_get() returns a dummy regulator if the regulator is not
> > > > described in the device-tree. The calling code has no way to known if
> > > > the regulator was present or not.    
> > > 
> > > Yeah because it's mandatory and the part cannot work without power :). So we
> > > should not be allowed to operate without a regulator.
> > >   
> > > > 
> > > > On the other hand, devm_regulator_get_optional() returns -ENODEV when the
> > > > regulator is not described.
> > > > 
> > > > That's pretty confusing but it is the reality.
> > > > 
> > > > I use devm_regulator_get_optional() but check for -ENODEV to see if the
> > > > regulator is provided or not.
> > > > 
> > > > In order to use the ADC core (is_used flag), I need both the AVDD and the
> > > > VREF regulator available.    
> > > 
> > > And that is why I don't get why are we allowed to proceed if there's no
> > > regulators? That seems wrong to me. 
> > > 
> > > So I think the regulators should be mandatory in the bindings and a dummy
> > > regulator should also not be allowed in this case because that should get
> > > you 
> > > -EINVAL when calling regulator_get_voltage().
> > >   
> > 
> > I have 4 regulators: avdd1, vref1, avvd2, vref2.
> > 
> > The ADC controller can work with 2 internal ADC core (adc_core[0] and
> > adc_core[1])
> > in the driver. Those internal core are not directly accessed by the driver.
> > Only
> > the ADC controller is accesses.
> > 
> > Those cores have an AVDD and a VREF power supply.
> > 
> > We can use either adc_core[0] only, adc_core[1] only or both adc cores.
> > 
> > Depending on regulator described, the driver uses one or two adc cores.
> > 
> > This choice is done by:
> > --- 8< ---
> > static int rzn1_adc_set_iio_dev_channels(struct rzn1_adc *rzn1_adc,
> > 					 struct iio_dev *indio_dev)
> > {
> > 	int adc_used;
> > 
> > 	adc_used = rzn1_adc->adc_core[0].is_used ? 0x01 : 0x00;
> > 	adc_used |= rzn1_adc->adc_core[1].is_used ? 0x02 : 0x00;
> > 
> > 	switch (adc_used) {
> > 	case 0x01:
> > 		indio_dev->channels = rzn1_adc1_channels;
> > 		indio_dev->num_channels = ARRAY_SIZE(rzn1_adc1_channels);
> > 		return 0;
> > 	case 0x02:
> > 		indio_dev->channels = rzn1_adc2_channels;
> > 		indio_dev->num_channels = ARRAY_SIZE(rzn1_adc2_channels);
> > 		return 0;
> > 	case 0x03:
> > 		indio_dev->channels = rzn1_adc1_adc2_channels;
> > 		indio_dev->num_channels =
> > ARRAY_SIZE(rzn1_adc1_adc2_channels);
> > 		return 0;
> > 	default:
> > 		break;
> > 	}
> > --- 8< ---
> > 
> > In rzn1_adc_core_get_regulators(), looking at one core I do the
> > following:
> >  - Try to get AVDD (using get_optional)
> >  - Try to get VREF (using get_optional)
> >  - Core is used only if both regulators are present.
> > 
> > For one core to be used, both regulators have to be present.
> > 
> > Regulators are mandatory but adc core usage is optional.
> > 
> > This optional usage depends on related regulator presence.
> >   
> 
> Ok, then we could flip the logic and have boolean properties for the adc core
> usage and depending on that, requesting the regulators. To me, the intent would
> be more clear (at the expense of more FW properties).

This introduces a new property and duplicates the information:
- flag to tell if adc core is used
- regulators described only if used

And so, the possible flag set to "adc core used" but regulators not
described. This is error prone.


I have chosen to rely only on regulators description to avoid the
information redundancy.
  - regulators described -> adc core used
  - regulators not described -> adc core not used

> 
> Having said that, the above helps a lot in understanding what's going on and
> explains the get_optional() usage. I'm not still 100% convinced but bah, fine :)
> 
> I would still argue that you should have a comment (likely in get_regulators())
> explaining the logic and the optional usage.
> 
> Given the above I think you could also remove:
> 
> if (!adc_core->vref)
> 	return -ENODEV;
> 
> from rzn1_adc_core_get_vref_mv() since the channels are only exposed in case the
> regulators are present.

Yes indeed, I will remove the adc_core->vref check.

Best regards,
Hervé

Re: [PATCH 2/4] iio: adc: Add support for the Renesas RZ/N1 ADC

[Nuno Sá]

On Fri, 2025-10-17 at 17:43 +0200, Herve Codina wrote:
> I Nuno,
> 
> On Fri, 17 Oct 2025 09:26:26 +0100
> Nuno Sá <noname.nuno@gmail.com> wrote:
> 
> > On Fri, 2025-10-17 at 08:59 +0200, Herve Codina wrote:
> > > Hi Nuno,
> > > 
> > > On Thu, 16 Oct 2025 16:26:28 +0100
> > > Nuno Sá <noname.nuno@gmail.com> wrote:
> > > 
> > > ...
> > > 
> > > ...  
> > > > > > > > > +
> > > > > > > > > +	ret = rzn1_adc_core_get_regulators(rzn1_adc,
> > > > > > > > > &rzn1_adc-
> > > > > > > > >       
> > > > > > > > > > adc_core[0],      
> > > > > > > > > +					   "adc1-avdd",
> > > > > > > > > "adc1-
> > > > > > > > > vref");
> > > > > > > > > +	if (ret)
> > > > > > > > > +		return ret;
> > > > > > > > > +
> > > > > > > > > +	ret = rzn1_adc_core_get_regulators(rzn1_adc,
> > > > > > > > > &rzn1_adc-
> > > > > > > > >       
> > > > > > > > > > adc_core[1],      
> > > > > > > > > +					   "adc2-avdd",
> > > > > > > > > "adc2-
> > > > > > > > > vref");
> > > > > > > > > +	if (ret)
> > > > > > > > > +		return ret;        
> > > > > > > > 
> > > > > > > > Hmm, is avdd really an optional regulator? I mean can the ADC
> > > > > > > > power
> > > > > > > > up
> > > > > > > > at
> > > > > > > > all
> > > > > > > > without a supply in AVDD? Even vref seems to be mandatory as we
> > > > > > > > can't
> > > > > > > > properly
> > > > > > > > scale the sample without it.      
> > > > > > > 
> > > > > > > Where do you see that avdd is an optional regulator?      
> > > > > > 
> > > > > > You are using devm_regulator_get_optional(). That's for optional
> > > > > > regulators.
> > > > > >     
> > > > > 
> > > > > Indeed I use devm_regulator_get_optional().
> > > > > 
> > > > > We have two similar function to get regulators:
> > > > > - devm_regulator_get() and
> > > > > - devm_regulator_get_optional().
> > > > > 
> > > > > devm_regulator_get() returns a dummy regulator if the regulator is not
> > > > > described in the device-tree. The calling code has no way to known if
> > > > > the regulator was present or not.    
> > > > 
> > > > Yeah because it's mandatory and the part cannot work without power :).
> > > > So we
> > > > should not be allowed to operate without a regulator.
> > > >   
> > > > > 
> > > > > On the other hand, devm_regulator_get_optional() returns -ENODEV when
> > > > > the
> > > > > regulator is not described.
> > > > > 
> > > > > That's pretty confusing but it is the reality.
> > > > > 
> > > > > I use devm_regulator_get_optional() but check for -ENODEV to see if
> > > > > the
> > > > > regulator is provided or not.
> > > > > 
> > > > > In order to use the ADC core (is_used flag), I need both the AVDD and
> > > > > the
> > > > > VREF regulator available.    
> > > > 
> > > > And that is why I don't get why are we allowed to proceed if there's no
> > > > regulators? That seems wrong to me. 
> > > > 
> > > > So I think the regulators should be mandatory in the bindings and a
> > > > dummy
> > > > regulator should also not be allowed in this case because that should
> > > > get
> > > > you 
> > > > -EINVAL when calling regulator_get_voltage().
> > > >   
> > > 
> > > I have 4 regulators: avdd1, vref1, avvd2, vref2.
> > > 
> > > The ADC controller can work with 2 internal ADC core (adc_core[0] and
> > > adc_core[1])
> > > in the driver. Those internal core are not directly accessed by the
> > > driver.
> > > Only
> > > the ADC controller is accesses.
> > > 
> > > Those cores have an AVDD and a VREF power supply.
> > > 
> > > We can use either adc_core[0] only, adc_core[1] only or both adc cores.
> > > 
> > > Depending on regulator described, the driver uses one or two adc cores.
> > > 
> > > This choice is done by:
> > > --- 8< ---
> > > static int rzn1_adc_set_iio_dev_channels(struct rzn1_adc *rzn1_adc,
> > > 					 struct iio_dev *indio_dev)
> > > {
> > > 	int adc_used;
> > > 
> > > 	adc_used = rzn1_adc->adc_core[0].is_used ? 0x01 : 0x00;
> > > 	adc_used |= rzn1_adc->adc_core[1].is_used ? 0x02 : 0x00;
> > > 
> > > 	switch (adc_used) {
> > > 	case 0x01:
> > > 		indio_dev->channels = rzn1_adc1_channels;
> > > 		indio_dev->num_channels = ARRAY_SIZE(rzn1_adc1_channels);
> > > 		return 0;
> > > 	case 0x02:
> > > 		indio_dev->channels = rzn1_adc2_channels;
> > > 		indio_dev->num_channels = ARRAY_SIZE(rzn1_adc2_channels);
> > > 		return 0;
> > > 	case 0x03:
> > > 		indio_dev->channels = rzn1_adc1_adc2_channels;
> > > 		indio_dev->num_channels =
> > > ARRAY_SIZE(rzn1_adc1_adc2_channels);
> > > 		return 0;
> > > 	default:
> > > 		break;
> > > 	}
> > > --- 8< ---
> > > 
> > > In rzn1_adc_core_get_regulators(), looking at one core I do the
> > > following:
> > >  - Try to get AVDD (using get_optional)
> > >  - Try to get VREF (using get_optional)
> > >  - Core is used only if both regulators are present.
> > > 
> > > For one core to be used, both regulators have to be present.
> > > 
> > > Regulators are mandatory but adc core usage is optional.
> > > 
> > > This optional usage depends on related regulator presence.
> > >   
> > 
> > Ok, then we could flip the logic and have boolean properties for the adc
> > core
> > usage and depending on that, requesting the regulators. To me, the intent
> > would
> > be more clear (at the expense of more FW properties).
> 
> This introduces a new property and duplicates the information:
> - flag to tell if adc core is used
> - regulators described only if used
> 
> And so, the possible flag set to "adc core used" but regulators not
> described. This is error prone.
> 
> 
> I have chosen to rely only on regulators description to avoid the
> information redundancy.
>   - regulators described -> adc core used
>   - regulators not described -> adc core not used

I'll leave it up to you but while I know it introduces new properties, you could
still do it in a way that minimizes errors:

 - In the bindings, if the property is set, then the regulators are a
__required__;
 - In the driver, if the boolean is true, then use devm_regulator_get()

- Nuno Sá

Re: [PATCH 2/4] iio: adc: Add support for the Renesas RZ/N1 ADC

[Jonathan Cameron]

On Fri, 17 Oct 2025 17:29:34 +0100
Nuno Sá <noname.nuno@gmail.com> wrote:

> On Fri, 2025-10-17 at 17:43 +0200, Herve Codina wrote:
> > I Nuno,
> > 
> > On Fri, 17 Oct 2025 09:26:26 +0100
> > Nuno Sá <noname.nuno@gmail.com> wrote:
> >   
> > > On Fri, 2025-10-17 at 08:59 +0200, Herve Codina wrote:  
> > > > Hi Nuno,
> > > > 
> > > > On Thu, 16 Oct 2025 16:26:28 +0100
> > > > Nuno Sá <noname.nuno@gmail.com> wrote:
> > > > 
> > > > ...
> > > > 
> > > > ...    
> > > > > > > > > > +
> > > > > > > > > > +	ret = rzn1_adc_core_get_regulators(rzn1_adc,
> > > > > > > > > > &rzn1_adc-
> > > > > > > > > >         
> > > > > > > > > > > adc_core[0],        
> > > > > > > > > > +					   "adc1-avdd",
> > > > > > > > > > "adc1-
> > > > > > > > > > vref");
> > > > > > > > > > +	if (ret)
> > > > > > > > > > +		return ret;
> > > > > > > > > > +
> > > > > > > > > > +	ret = rzn1_adc_core_get_regulators(rzn1_adc,
> > > > > > > > > > &rzn1_adc-
> > > > > > > > > >         
> > > > > > > > > > > adc_core[1],        
> > > > > > > > > > +					   "adc2-avdd",
> > > > > > > > > > "adc2-
> > > > > > > > > > vref");
> > > > > > > > > > +	if (ret)
> > > > > > > > > > +		return ret;          
> > > > > > > > > 
> > > > > > > > > Hmm, is avdd really an optional regulator? I mean can the ADC
> > > > > > > > > power
> > > > > > > > > up
> > > > > > > > > at
> > > > > > > > > all
> > > > > > > > > without a supply in AVDD? Even vref seems to be mandatory as we
> > > > > > > > > can't
> > > > > > > > > properly
> > > > > > > > > scale the sample without it.        
> > > > > > > > 
> > > > > > > > Where do you see that avdd is an optional regulator?        
> > > > > > > 
> > > > > > > You are using devm_regulator_get_optional(). That's for optional
> > > > > > > regulators.
> > > > > > >       
> > > > > > 
> > > > > > Indeed I use devm_regulator_get_optional().
> > > > > > 
> > > > > > We have two similar function to get regulators:
> > > > > > - devm_regulator_get() and
> > > > > > - devm_regulator_get_optional().
> > > > > > 
> > > > > > devm_regulator_get() returns a dummy regulator if the regulator is not
> > > > > > described in the device-tree. The calling code has no way to known if
> > > > > > the regulator was present or not.      
> > > > > 
> > > > > Yeah because it's mandatory and the part cannot work without power :).
> > > > > So we
> > > > > should not be allowed to operate without a regulator.
> > > > >     
> > > > > > 
> > > > > > On the other hand, devm_regulator_get_optional() returns -ENODEV when
> > > > > > the
> > > > > > regulator is not described.
> > > > > > 
> > > > > > That's pretty confusing but it is the reality.
> > > > > > 
> > > > > > I use devm_regulator_get_optional() but check for -ENODEV to see if
> > > > > > the
> > > > > > regulator is provided or not.
> > > > > > 
> > > > > > In order to use the ADC core (is_used flag), I need both the AVDD and
> > > > > > the
> > > > > > VREF regulator available.      
> > > > > 
> > > > > And that is why I don't get why are we allowed to proceed if there's no
> > > > > regulators? That seems wrong to me. 
> > > > > 
> > > > > So I think the regulators should be mandatory in the bindings and a
> > > > > dummy
> > > > > regulator should also not be allowed in this case because that should
> > > > > get
> > > > > you 
> > > > > -EINVAL when calling regulator_get_voltage().
> > > > >     
> > > > 
> > > > I have 4 regulators: avdd1, vref1, avvd2, vref2.
> > > > 
> > > > The ADC controller can work with 2 internal ADC core (adc_core[0] and
> > > > adc_core[1])
> > > > in the driver. Those internal core are not directly accessed by the
> > > > driver.
> > > > Only
> > > > the ADC controller is accesses.
> > > > 
> > > > Those cores have an AVDD and a VREF power supply.
> > > > 
> > > > We can use either adc_core[0] only, adc_core[1] only or both adc cores.
> > > > 
> > > > Depending on regulator described, the driver uses one or two adc cores.
> > > > 
> > > > This choice is done by:
> > > > --- 8< ---
> > > > static int rzn1_adc_set_iio_dev_channels(struct rzn1_adc *rzn1_adc,
> > > > 					 struct iio_dev *indio_dev)
> > > > {
> > > > 	int adc_used;
> > > > 
> > > > 	adc_used = rzn1_adc->adc_core[0].is_used ? 0x01 : 0x00;
> > > > 	adc_used |= rzn1_adc->adc_core[1].is_used ? 0x02 : 0x00;
> > > > 
> > > > 	switch (adc_used) {
> > > > 	case 0x01:
> > > > 		indio_dev->channels = rzn1_adc1_channels;
> > > > 		indio_dev->num_channels = ARRAY_SIZE(rzn1_adc1_channels);
> > > > 		return 0;
> > > > 	case 0x02:
> > > > 		indio_dev->channels = rzn1_adc2_channels;
> > > > 		indio_dev->num_channels = ARRAY_SIZE(rzn1_adc2_channels);
> > > > 		return 0;
> > > > 	case 0x03:
> > > > 		indio_dev->channels = rzn1_adc1_adc2_channels;
> > > > 		indio_dev->num_channels =
> > > > ARRAY_SIZE(rzn1_adc1_adc2_channels);
> > > > 		return 0;
> > > > 	default:
> > > > 		break;
> > > > 	}
> > > > --- 8< ---
> > > > 
> > > > In rzn1_adc_core_get_regulators(), looking at one core I do the
> > > > following:
> > > >  - Try to get AVDD (using get_optional)
> > > >  - Try to get VREF (using get_optional)
> > > >  - Core is used only if both regulators are present.
> > > > 
> > > > For one core to be used, both regulators have to be present.
> > > > 
> > > > Regulators are mandatory but adc core usage is optional.
> > > > 
> > > > This optional usage depends on related regulator presence.
> > > >     
> > > 
> > > Ok, then we could flip the logic and have boolean properties for the adc
> > > core
> > > usage and depending on that, requesting the regulators. To me, the intent
> > > would
> > > be more clear (at the expense of more FW properties).  
> > 
> > This introduces a new property and duplicates the information:
> > - flag to tell if adc core is used
> > - regulators described only if used
> > 
> > And so, the possible flag set to "adc core used" but regulators not
> > described. This is error prone.
> > 
> > 
> > I have chosen to rely only on regulators description to avoid the
> > information redundancy.
> >   - regulators described -> adc core used
> >   - regulators not described -> adc core not used  
> 
> I'll leave it up to you but while I know it introduces new properties, you could
> still do it in a way that minimizes errors:
> 
>  - In the bindings, if the property is set, then the regulators are a
> __required__;
>  - In the driver, if the boolean is true, then use devm_regulator_get()
> 
> - Nuno Sá

I'd add a question on this under the --- in the next version of the binding
doc. This is a fairly unusual situation. I think the regulators presence is
sufficient but it may surprise people enough to make it worth calling out.

Jonathan

Re: [PATCH 2/4] iio: adc: Add support for the Renesas RZ/N1 ADC

[kernel test robot]

Hi Herve,

kernel test robot noticed the following build warnings:

[auto build test WARNING on jic23-iio/togreg]
[also build test WARNING on geert-renesas-devel/next linus/master v6.18-rc1 next-20251015]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]

url:    https://github.com/intel-lab-lkp/linux/commits/Herve-Codina-Schneider-Electric/dt-bindings-iio-adc-Add-the-Renesas-RZ-N1-ADC/20251015-223254
base:   https://git.kernel.org/pub/scm/linux/kernel/git/jic23/iio.git togreg
patch link:    https://lore.kernel.org/r/20251015142816.1274605-3-herve.codina%40bootlin.com
patch subject: [PATCH 2/4] iio: adc: Add support for the Renesas RZ/N1 ADC
config: alpha-allyesconfig (https://download.01.org/0day-ci/archive/20251016/202510162035.gngVSgxu-lkp@intel.com/config)
compiler: alpha-linux-gcc (GCC) 15.1.0
reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20251016/202510162035.gngVSgxu-lkp@intel.com/reproduce)

If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <lkp@intel.com>
| Closes: https://lore.kernel.org/oe-kbuild-all/202510162035.gngVSgxu-lkp@intel.com/

All warnings (new ones prefixed by >>):

   drivers/iio/adc/rzn1-adc.c: In function 'rzn1_adc_vc_setup_conversion':
   drivers/iio/adc/rzn1-adc.c:53:49: error: implicit declaration of function 'FIELD_PREP' [-Wimplicit-function-declaration]
      53 | #define RZN1_ADC_VC_ADC1_CHANNEL_SEL(_c)        FIELD_PREP(RZN1_ADC_VC_ADC1_CHANNEL_SEL_MASK, _c)
         |                                                 ^~~~~~~~~~
   drivers/iio/adc/rzn1-adc.c:243:49: note: in expansion of macro 'RZN1_ADC_VC_ADC1_CHANNEL_SEL'
     243 |                 vc |= RZN1_ADC_VC_ADC1_ENABLE | RZN1_ADC_VC_ADC1_CHANNEL_SEL(adc1_ch);
         |                                                 ^~~~~~~~~~~~~~~~~~~~~~~~~~~~
   drivers/iio/adc/rzn1-adc.c: In function 'rzn1_adc_vc_wait_conversion':
   drivers/iio/adc/rzn1-adc.c:58:41: error: implicit declaration of function 'FIELD_GET' [-Wimplicit-function-declaration]
      58 | #define RZN1_ADC_ADCX_GET_DATA(_reg)    FIELD_GET(RZN1_ADC_ADCX_DATA_DATA_MASK, _reg)
         |                                         ^~~~~~~~~
   drivers/iio/adc/rzn1-adc.c:304:30: note: in expansion of macro 'RZN1_ADC_ADCX_GET_DATA'
     304 |                 *adc1_data = RZN1_ADC_ADCX_GET_DATA(data_reg);
         |                              ^~~~~~~~~~~~~~~~~~~~~~
   drivers/iio/adc/rzn1-adc.c: At top level:
>> drivers/iio/adc/rzn1-adc.c:592:12: warning: 'rzn1_adc_pm_runtime_resume' defined but not used [-Wunused-function]
     592 | static int rzn1_adc_pm_runtime_resume(struct device *dev)
         |            ^~~~~~~~~~~~~~~~~~~~~~~~~~
>> drivers/iio/adc/rzn1-adc.c:582:12: warning: 'rzn1_adc_pm_runtime_suspend' defined but not used [-Wunused-function]
     582 | static int rzn1_adc_pm_runtime_suspend(struct device *dev)
         |            ^~~~~~~~~~~~~~~~~~~~~~~~~~~


vim +/rzn1_adc_pm_runtime_resume +592 drivers/iio/adc/rzn1-adc.c

   581	
 > 582	static int rzn1_adc_pm_runtime_suspend(struct device *dev)
   583	{
   584		struct iio_dev *indio_dev = dev_get_drvdata(dev);
   585		struct rzn1_adc *rzn1_adc = iio_priv(indio_dev);
   586	
   587		rzn1_adc_disable(rzn1_adc);
   588	
   589		return 0;
   590	}
   591	
 > 592	static int rzn1_adc_pm_runtime_resume(struct device *dev)
   593	{
   594		struct iio_dev *indio_dev = dev_get_drvdata(dev);
   595		struct rzn1_adc *rzn1_adc = iio_priv(indio_dev);
   596	
   597		return rzn1_adc_enable(rzn1_adc);
   598	}
   599	

-- 
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki

Re: [PATCH 2/4] iio: adc: Add support for the Renesas RZ/N1 ADC

[kernel test robot]

Hi Herve,

kernel test robot noticed the following build errors:

[auto build test ERROR on jic23-iio/togreg]
[also build test ERROR on geert-renesas-devel/next linus/master v6.18-rc1 next-20251015]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]

url:    https://github.com/intel-lab-lkp/linux/commits/Herve-Codina-Schneider-Electric/dt-bindings-iio-adc-Add-the-Renesas-RZ-N1-ADC/20251015-223254
base:   https://git.kernel.org/pub/scm/linux/kernel/git/jic23/iio.git togreg
patch link:    https://lore.kernel.org/r/20251015142816.1274605-3-herve.codina%40bootlin.com
patch subject: [PATCH 2/4] iio: adc: Add support for the Renesas RZ/N1 ADC
config: hexagon-allmodconfig (https://download.01.org/0day-ci/archive/20251016/202510162222.Fe1rY5aB-lkp@intel.com/config)
compiler: clang version 17.0.6 (https://github.com/llvm/llvm-project 6009708b4367171ccdbf4b5905cb6a803753fe18)
reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20251016/202510162222.Fe1rY5aB-lkp@intel.com/reproduce)

If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <lkp@intel.com>
| Closes: https://lore.kernel.org/oe-kbuild-all/202510162222.Fe1rY5aB-lkp@intel.com/

All errors (new ones prefixed by >>):

>> drivers/iio/adc/rzn1-adc.c:243:35: error: call to undeclared function 'FIELD_PREP'; ISO C99 and later do not support implicit function declarations [-Wimplicit-function-declaration]
     243 |                 vc |= RZN1_ADC_VC_ADC1_ENABLE | RZN1_ADC_VC_ADC1_CHANNEL_SEL(adc1_ch);
         |                                                 ^
   drivers/iio/adc/rzn1-adc.c:53:42: note: expanded from macro 'RZN1_ADC_VC_ADC1_CHANNEL_SEL'
      53 | #define RZN1_ADC_VC_ADC1_CHANNEL_SEL(_c)        FIELD_PREP(RZN1_ADC_VC_ADC1_CHANNEL_SEL_MASK, _c)
         |                                                 ^
>> drivers/iio/adc/rzn1-adc.c:304:16: error: call to undeclared function 'FIELD_GET'; ISO C99 and later do not support implicit function declarations [-Wimplicit-function-declaration]
     304 |                 *adc1_data = RZN1_ADC_ADCX_GET_DATA(data_reg);
         |                              ^
   drivers/iio/adc/rzn1-adc.c:58:38: note: expanded from macro 'RZN1_ADC_ADCX_GET_DATA'
      58 | #define RZN1_ADC_ADCX_GET_DATA(_reg)    FIELD_GET(RZN1_ADC_ADCX_DATA_DATA_MASK, _reg)
         |                                         ^
   drivers/iio/adc/rzn1-adc.c:309:16: error: call to undeclared function 'FIELD_GET'; ISO C99 and later do not support implicit function declarations [-Wimplicit-function-declaration]
     309 |                 *adc2_data = RZN1_ADC_ADCX_GET_DATA(data_reg);
         |                              ^
   drivers/iio/adc/rzn1-adc.c:58:38: note: expanded from macro 'RZN1_ADC_ADCX_GET_DATA'
      58 | #define RZN1_ADC_ADCX_GET_DATA(_reg)    FIELD_GET(RZN1_ADC_ADCX_DATA_DATA_MASK, _reg)
         |                                         ^
   3 errors generated.


vim +/FIELD_PREP +243 drivers/iio/adc/rzn1-adc.c

   236	
   237	static void rzn1_adc_vc_setup_conversion(struct rzn1_adc *rzn1_adc, u32 ch,
   238						 int adc1_ch, int adc2_ch)
   239	{
   240		u32 vc = 0;
   241	
   242		if (adc1_ch != -1)
 > 243			vc |= RZN1_ADC_VC_ADC1_ENABLE | RZN1_ADC_VC_ADC1_CHANNEL_SEL(adc1_ch);
   244	
   245		if (adc2_ch != -1)
   246			vc |= RZN1_ADC_VC_ADC2_ENABLE | RZN1_ADC_VC_ADC2_CHANNEL_SEL(adc2_ch);
   247	
   248		writel(vc, rzn1_adc->regs + RZN1_ADC_VC_REG(ch));
   249	}
   250	
   251	static int rzn1_adc_vc_start_conversion(struct rzn1_adc *rzn1_adc, u32 ch)
   252	{
   253		u32 val;
   254	
   255		val = readl(rzn1_adc->regs + RZN1_ADC_FORCE_REG);
   256		if (val & RZN1_ADC_FORCE_VC(ch))
   257			return -EBUSY;
   258	
   259		writel(RZN1_ADC_FORCE_VC(ch), rzn1_adc->regs + RZN1_ADC_SET_FORCE_REG);
   260	
   261		return 0;
   262	}
   263	
   264	static void rzn1_adc_vc_stop_conversion(struct rzn1_adc *rzn1_adc, u32 ch)
   265	{
   266		writel(RZN1_ADC_FORCE_VC(ch), rzn1_adc->regs + RZN1_ADC_CLEAR_FORCE_REG);
   267	}
   268	
   269	static int rzn1_adc_vc_wait_conversion(struct rzn1_adc *rzn1_adc, u32 ch,
   270					       u32 *adc1_data, u32 *adc2_data)
   271	{
   272		u32 data_reg;
   273		int ret;
   274		u32 v;
   275	
   276		/*
   277		 * When a VC is selected, it needs 20 ADC clocks to perform the
   278		 * conversion.
   279		 *
   280		 * The worst case is when the 16 VCs need to perform a conversion and
   281		 * our VC is the lowest in term of priority.
   282		 *
   283		 * In that case, the conversion is performed in 16 * 20 ADC clocks.
   284		 *
   285		 * The ADC clock can be set from 4MHz to 20MHz. This leads to a worst
   286		 * case of  16 * 20 * 1/4Mhz = 80us.
   287		 *
   288		 * Round it up to 100us
   289		 */
   290	
   291		/*
   292		 * Wait for the ADC_FORCE_VC(n) to clear.
   293		 *
   294		 * On timeout, ret is -ETIMEDOUT, otherwise it will be 0.
   295		 */
   296		ret = readl_poll_timeout_atomic(rzn1_adc->regs + RZN1_ADC_FORCE_REG,
   297						v, !(v & RZN1_ADC_FORCE_VC(ch)),
   298						0, 100);
   299		if (ret)
   300			return ret;
   301	
   302		if (adc1_data) {
   303			data_reg = readl(rzn1_adc->regs + RZN1_ADC_ADC1_DATA_REG(ch));
 > 304			*adc1_data = RZN1_ADC_ADCX_GET_DATA(data_reg);
   305		}
   306	
   307		if (adc2_data) {
   308			data_reg = readl(rzn1_adc->regs + RZN1_ADC_ADC2_DATA_REG(ch));
   309			*adc2_data = RZN1_ADC_ADCX_GET_DATA(data_reg);
   310		}
   311	
   312		return 0;
   313	}
   314	

-- 
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki

Re: [PATCH 2/4] iio: adc: Add support for the Renesas RZ/N1 ADC

[Wolfram Sang]

[-- Attachment #1: Type: text/plain, Size: 1506 bytes --]

Hi Herve,

On Wed, Oct 15, 2025 at 04:28:14PM +0200, Herve Codina (Schneider Electric) wrote:
> The Renesas RZ/N1 ADC controller is the ADC controller available in the
> Renesas RZ/N1 SoCs family. It can use up to two internal ACD cores (ADC1

ADC cores

> and ADC2) those internal cores are not directly accessed but are handled
> through ADC controller virtual channels.
> 
> Signed-off-by: Herve Codina (Schneider Electric) <herve.codina@bootlin.com>

Very high level review.

> +/*                  ADC1 ADC2
> + * RZ/N1D, BGA 400   y    y
> + * RZ/N1D, BGA 324   y    n
> + * RZ/N1S, BGA 324   y    n
> + * RZ/N1S, BGA 196   y    n
> + * RZ/N1L, BGA 196   y    n
> + */

I think this table can go. N1D is the only variant supported by Linux
because others have no SDRAM controller. Maybe a comment after the
copyright is helpful stating that the second ADC core is utilized when
the adc2-* bindings are supplied?

> +static void rzn1_adc_vc_setup_conversion(struct rzn1_adc *rzn1_adc, u32 ch,
> +					 int adc1_ch, int adc2_ch)
> +{
> +	u32 vc = 0;
> +
> +	if (adc1_ch != -1)
> +		vc |= RZN1_ADC_VC_ADC1_ENABLE | RZN1_ADC_VC_ADC1_CHANNEL_SEL(adc1_ch);
> +
> +	if (adc2_ch != -1)
> +		vc |= RZN1_ADC_VC_ADC2_ENABLE | RZN1_ADC_VC_ADC2_CHANNEL_SEL(adc2_ch);

Are you open to either use an errno (maybe EACCES) or define something
custom (maybe RZN1_ADC_NO_CHANNEL) instead of hardcoded -1? I think I
like the latter a tad more.

Happy hacking,

   Wolfram


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH 2/4] iio: adc: Add support for the Renesas RZ/N1 ADC

[Herve Codina]

Hi Wolfram,

On Fri, 17 Oct 2025 08:28:16 +0200
Wolfram Sang <wsa+renesas@sang-engineering.com> wrote:

> Hi Herve,
> 
> On Wed, Oct 15, 2025 at 04:28:14PM +0200, Herve Codina (Schneider Electric) wrote:
> > The Renesas RZ/N1 ADC controller is the ADC controller available in the
> > Renesas RZ/N1 SoCs family. It can use up to two internal ACD cores (ADC1  
> 
> ADC cores

Yes, indeed.

> 
> > and ADC2) those internal cores are not directly accessed but are handled
> > through ADC controller virtual channels.
> > 
> > Signed-off-by: Herve Codina (Schneider Electric) <herve.codina@bootlin.com>  
> 
> Very high level review.
> 
> > +/*                  ADC1 ADC2
> > + * RZ/N1D, BGA 400   y    y
> > + * RZ/N1D, BGA 324   y    n
> > + * RZ/N1S, BGA 324   y    n
> > + * RZ/N1S, BGA 196   y    n
> > + * RZ/N1L, BGA 196   y    n
> > + */  
> 
> I think this table can go. N1D is the only variant supported by Linux
> because others have no SDRAM controller. Maybe a comment after the
> copyright is helpful stating that the second ADC core is utilized when
> the adc2-* bindings are supplied?

Yes, with only RZ/N1D supported, this table doesn't bring any additional
information.

As you suggested, I will add information about ADC cores in the header
part of this .c file.

> 
> > +static void rzn1_adc_vc_setup_conversion(struct rzn1_adc *rzn1_adc, u32 ch,
> > +					 int adc1_ch, int adc2_ch)
> > +{
> > +	u32 vc = 0;
> > +
> > +	if (adc1_ch != -1)
> > +		vc |= RZN1_ADC_VC_ADC1_ENABLE | RZN1_ADC_VC_ADC1_CHANNEL_SEL(adc1_ch);
> > +
> > +	if (adc2_ch != -1)
> > +		vc |= RZN1_ADC_VC_ADC2_ENABLE | RZN1_ADC_VC_ADC2_CHANNEL_SEL(adc2_ch);  
> 
> Are you open to either use an errno (maybe EACCES) or define something
> custom (maybe RZN1_ADC_NO_CHANNEL) instead of hardcoded -1? I think I
> like the latter a tad more.

I prefer RZN1_ADC_NO_CHANNEL too instead of an error code and I will use
that instead of -1 in the next iteration.

Best regards,
Hervé

Re: [PATCH 2/4] iio: adc: Add support for the Renesas RZ/N1 ADC

[Geert Uytterhoeven]

Hi Hervé,

On Fri, 17 Oct 2025 at 09:37, Herve Codina <herve.codina@bootlin.com> wrote:
> Wolfram Sang <wsa+renesas@sang-engineering.com> wrote:
> > On Wed, Oct 15, 2025 at 04:28:14PM +0200, Herve Codina (Schneider Electric) wrote:
> > > +static void rzn1_adc_vc_setup_conversion(struct rzn1_adc *rzn1_adc, u32 ch,
> > > +                                    int adc1_ch, int adc2_ch)
> > > +{
> > > +   u32 vc = 0;
> > > +
> > > +   if (adc1_ch != -1)
> > > +           vc |= RZN1_ADC_VC_ADC1_ENABLE | RZN1_ADC_VC_ADC1_CHANNEL_SEL(adc1_ch);
> > > +
> > > +   if (adc2_ch != -1)
> > > +           vc |= RZN1_ADC_VC_ADC2_ENABLE | RZN1_ADC_VC_ADC2_CHANNEL_SEL(adc2_ch);
> >
> > Are you open to either use an errno (maybe EACCES) or define something
> > custom (maybe RZN1_ADC_NO_CHANNEL) instead of hardcoded -1? I think I
> > like the latter a tad more.
>
> I prefer RZN1_ADC_NO_CHANNEL too instead of an error code and I will use
> that instead of -1 in the next iteration.

Or just -ENODEV or -ENOENT, and change the checks above to
"if (adc1_ch >= 0)"?

Gr{oetje,eeting}s,

                        Geert

-- 
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

Re: [PATCH 2/4] iio: adc: Add support for the Renesas RZ/N1 ADC

[Herve Codina]

Hi Geert,

On Fri, 17 Oct 2025 09:40:42 +0200
Geert Uytterhoeven <geert@linux-m68k.org> wrote:

> Hi Hervé,
> 
> On Fri, 17 Oct 2025 at 09:37, Herve Codina <herve.codina@bootlin.com> wrote:
> > Wolfram Sang <wsa+renesas@sang-engineering.com> wrote:  
> > > On Wed, Oct 15, 2025 at 04:28:14PM +0200, Herve Codina (Schneider Electric) wrote:  
> > > > +static void rzn1_adc_vc_setup_conversion(struct rzn1_adc *rzn1_adc, u32 ch,
> > > > +                                    int adc1_ch, int adc2_ch)
> > > > +{
> > > > +   u32 vc = 0;
> > > > +
> > > > +   if (adc1_ch != -1)
> > > > +           vc |= RZN1_ADC_VC_ADC1_ENABLE | RZN1_ADC_VC_ADC1_CHANNEL_SEL(adc1_ch);
> > > > +
> > > > +   if (adc2_ch != -1)
> > > > +           vc |= RZN1_ADC_VC_ADC2_ENABLE | RZN1_ADC_VC_ADC2_CHANNEL_SEL(adc2_ch);  
> > >
> > > Are you open to either use an errno (maybe EACCES) or define something
> > > custom (maybe RZN1_ADC_NO_CHANNEL) instead of hardcoded -1? I think I
> > > like the latter a tad more.  
> >
> > I prefer RZN1_ADC_NO_CHANNEL too instead of an error code and I will use
> > that instead of -1 in the next iteration.  
> 
> Or just -ENODEV or -ENOENT, and change the checks above to
> "if (adc1_ch >= 0)"?

I prefer to avoid mixing channel numbers with error code.

The specific RZN1_ADC_NO_CHANNEL value looks good to me meaning "No channel
to use".

Best regards,
Hervé

Re: [PATCH 2/4] iio: adc: Add support for the Renesas RZ/N1 ADC

[Wolfram Sang]

[-- Attachment #1: Type: text/plain, Size: 176 bytes --]


> The specific RZN1_ADC_NO_CHANNEL value looks good to me meaning "No channel
> to use".

I think I have an even better idea, will respond to the original patch
for context.


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH 2/4] iio: adc: Add support for the Renesas RZ/N1 ADC

[Wolfram Sang]

[-- Attachment #1: Type: text/plain, Size: 1649 bytes --]


> +static int rzn1_adc_read_raw_ch(struct rzn1_adc *rzn1_adc, unsigned int chan, int *val)
> +{
> +	u32 *adc1_data, *adc2_data;
> +	int adc1_ch, adc2_ch;
> +	u32 adc_data;
> +	int ret;
> +
> +	if (chan < 8) {
> +		/* chan 0..7 used to get ADC1 ch 0..7 */
> +		adc1_ch = chan;
> +		adc1_data = &adc_data;
> +		adc2_ch = -1;
> +		adc2_data = NULL;
> +	} else if (chan < 16) {
> +		/* chan 8..15 used to get ADC2 ch 0..7 */
> +		adc1_ch = -1;
> +		adc1_data = NULL;
> +		adc2_ch = chan - 8;
> +		adc2_data = &adc_data;
> +	} else {
> +		return -EINVAL;
> +	}

How about putting part of the logic into the setup function? So, here
only:

	if (chan >= 16)
		return -EINVAL

> +
> +	ret = pm_runtime_resume_and_get(rzn1_adc->dev);
> +	if (ret < 0)
> +		return ret;
> +
> +	mutex_lock(&rzn1_adc->lock);
> +
> +	rzn1_adc_vc_setup_conversion(rzn1_adc, chan, adc1_ch, adc2_ch);

	rzn1_adc_vc_setup_conversion(rzn1_adc, chan);

And in that function:

> +static void rzn1_adc_vc_setup_conversion(struct rzn1_adc *rzn1_adc, u32 ch,
> +					 int adc1_ch, int adc2_ch)
> +{
> +	u32 vc = 0;
> +
> +	if (adc1_ch != -1)
> +		vc |= RZN1_ADC_VC_ADC1_ENABLE | RZN1_ADC_VC_ADC1_CHANNEL_SEL(adc1_ch);
> +
> +	if (adc2_ch != -1)
> +		vc |= RZN1_ADC_VC_ADC2_ENABLE | RZN1_ADC_VC_ADC2_CHANNEL_SEL(adc2_ch);
> +
> +	writel(vc, rzn1_adc->regs + RZN1_ADC_VC_REG(ch));
> +}

	if (ch < 8)
		vc |= RZN1_ADC_VC_ADC1_ENABLE | RZN1_ADC_VC_ADC1_CHANNEL_SEL(ch);
	else
		vc |= RZN1_ADC_VC_ADC2_ENABLE | RZN1_ADC_VC_ADC2_CHANNEL_SEL(ch - 8);

And a similar simplification for rzn1_adc_vc_wait_conversion().

Should work and the code is even more readable, I'd say. And has less
lines.


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH 2/4] iio: adc: Add support for the Renesas RZ/N1 ADC

[Herve Codina]

Hi Wolfram,

On Fri, 17 Oct 2025 11:11:49 +0200
Wolfram Sang <wsa+renesas@sang-engineering.com> wrote:

> > +static int rzn1_adc_read_raw_ch(struct rzn1_adc *rzn1_adc, unsigned int chan, int *val)
> > +{
> > +	u32 *adc1_data, *adc2_data;
> > +	int adc1_ch, adc2_ch;
> > +	u32 adc_data;
> > +	int ret;
> > +
> > +	if (chan < 8) {
> > +		/* chan 0..7 used to get ADC1 ch 0..7 */
> > +		adc1_ch = chan;
> > +		adc1_data = &adc_data;
> > +		adc2_ch = -1;
> > +		adc2_data = NULL;
> > +	} else if (chan < 16) {
> > +		/* chan 8..15 used to get ADC2 ch 0..7 */
> > +		adc1_ch = -1;
> > +		adc1_data = NULL;
> > +		adc2_ch = chan - 8;
> > +		adc2_data = &adc_data;
> > +	} else {
> > +		return -EINVAL;
> > +	}  
> 
> How about putting part of the logic into the setup function? So, here
> only:
> 
> 	if (chan >= 16)
> 		return -EINVAL
> 
> > +
> > +	ret = pm_runtime_resume_and_get(rzn1_adc->dev);
> > +	if (ret < 0)
> > +		return ret;
> > +
> > +	mutex_lock(&rzn1_adc->lock);
> > +
> > +	rzn1_adc_vc_setup_conversion(rzn1_adc, chan, adc1_ch, adc2_ch);  
> 
> 	rzn1_adc_vc_setup_conversion(rzn1_adc, chan);
> 
> And in that function:
> 
> > +static void rzn1_adc_vc_setup_conversion(struct rzn1_adc *rzn1_adc, u32 ch,
> > +					 int adc1_ch, int adc2_ch)
> > +{
> > +	u32 vc = 0;
> > +
> > +	if (adc1_ch != -1)
> > +		vc |= RZN1_ADC_VC_ADC1_ENABLE | RZN1_ADC_VC_ADC1_CHANNEL_SEL(adc1_ch);
> > +
> > +	if (adc2_ch != -1)
> > +		vc |= RZN1_ADC_VC_ADC2_ENABLE | RZN1_ADC_VC_ADC2_CHANNEL_SEL(adc2_ch);
> > +
> > +	writel(vc, rzn1_adc->regs + RZN1_ADC_VC_REG(ch));
> > +}  
> 
> 	if (ch < 8)
> 		vc |= RZN1_ADC_VC_ADC1_ENABLE | RZN1_ADC_VC_ADC1_CHANNEL_SEL(ch);
> 	else
> 		vc |= RZN1_ADC_VC_ADC2_ENABLE | RZN1_ADC_VC_ADC2_CHANNEL_SEL(ch - 8);
> 
> And a similar simplification for rzn1_adc_vc_wait_conversion().
> 
> Should work and the code is even more readable, I'd say. And has less
> lines.
> 

That was what I did on my first driver draft before sending this first
iteration. I moved to adc1 and adc2 channel numbers in parameters to avoid
adding this logic here.

I think it was better to decouple the IIO chan number from the adc core
channel used.

I don't know if it will be relevant but we can image a future improvement
where new IIO chans use both the ADC core 1 and 2. It could make sense.

IMHO, I think the solution you proposed is similar in term of complexity
to the RZN1_ADC_NO_CHANNEL approach. On my side, I would prefer the
RZN1_ADC_NO_CHANNEL approach to keep the decoupling between IIO chan and
ADC core chans.

That's said, I am still open to move in your direction if you still think
it is more relevant than the RZN1_ADC_NO_CHANNEL approach. Just tell me.

Best regards,
Hervé

Re: [PATCH 2/4] iio: adc: Add support for the Renesas RZ/N1 ADC

[Wolfram Sang]

[-- Attachment #1: Type: text/plain, Size: 678 bytes --]


> IMHO, I think the solution you proposed is similar in term of complexity
> to the RZN1_ADC_NO_CHANNEL approach. On my side, I would prefer the
> RZN1_ADC_NO_CHANNEL approach to keep the decoupling between IIO chan and
> ADC core chans.
> 
> That's said, I am still open to move in your direction if you still think
> it is more relevant than the RZN1_ADC_NO_CHANNEL approach. Just tell me.

Well, in deed, I like "my" approach a tad better, but I am not demanding
it. It is your driver and you have reasons to do it like you implemented
it - you chose the way, I am fine with both. But maybe add a comment
(mention the decoupling) why it was decided this way.


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH 2/4] iio: adc: Add support for the Renesas RZ/N1 ADC

[Jonathan Cameron]

On Wed, 15 Oct 2025 16:28:14 +0200
"Herve Codina (Schneider Electric)" <herve.codina@bootlin.com> wrote:

> The Renesas RZ/N1 ADC controller is the ADC controller available in the
> Renesas RZ/N1 SoCs family. It can use up to two internal ACD cores (ADC1
> and ADC2) those internal cores are not directly accessed but are handled
> through ADC controller virtual channels.
> 
> Signed-off-by: Herve Codina (Schneider Electric) <herve.codina@bootlin.com>
Hi Herve,

A few additional things inline from me.  I may well have overlapped with
the review Nuno did but tried to avoid doing it too much!

Thanks,

Jonathan

> diff --git a/drivers/iio/adc/rzn1-adc.c b/drivers/iio/adc/rzn1-adc.c
> new file mode 100644
> index 000000000000..f5e16b9cdf17
> --- /dev/null
> +++ b/drivers/iio/adc/rzn1-adc.c
> @@ -0,0 +1,626 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * Renesas RZ/N1 ADC driver
> + *
> + * Copyright (C) 2025 Schneider-Electric
> + *
> + * Author: Herve Codina <herve.codina@bootlin.com>
> + */
> +
> +#include <linux/kernel.h>
> +#include <linux/slab.h>
> +#include <linux/module.h>
> +#include <linux/platform_device.h>
> +#include <linux/interrupt.h>
> +#include <linux/mutex.h>
> +#include <linux/completion.h>
> +#include <linux/iio/iio.h>
> +#include <linux/iio/machine.h>

I didn't see anything around iio_maps in here so
don't think you need this or driver.h

In general check these follow the slightly vague
version of Include What you Use (IWYU) that we try to follow for
kernel drivers.

> +#include <linux/iio/driver.h>
> +#include <linux/io.h>
> +#include <linux/iopoll.h>
> +#include <linux/clk.h>
> +#include <linux/delay.h>
> +#include <linux/bits.h>
> +#include <linux/of.h>
once of_match_ptr() is gone, this probably doesn't want to be here.
mod_devicetable.h does though for the id table.

> +#include <linux/regulator/consumer.h>
> +#include <linux/pm_runtime.h>
I'm not sure if there is a specific order to these headers.
If not, go with alphabetical.

> +
> +/*                  ADC1 ADC2
> + * RZ/N1D, BGA 400   y    y
> + * RZ/N1D, BGA 324   y    n
> + * RZ/N1S, BGA 324   y    n
> + * RZ/N1S, BGA 196   y    n
> + * RZ/N1L, BGA 196   y    n
> + */
> +
> +#define RZN1_ADC_CONTROL_REG	0x2c
> +#define RZN1_ADC_CONTROL_ADC_BUSY	BIT(6)
> +#define RZN1_ADC_FORCE_REG		0x30
> +#define RZN1_ADC_SET_FORCE_REG		0x34
> +#define RZN1_ADC_CLEAR_FORCE_REG	0x38
> +#define RZN1_ADC_FORCE_VC(_n)		BIT(_n)
> +
> +#define RZN1_ADC_CONFIG_REG	0x40
> +#define RZN1_ADC_CONFIG_ADC_POWER_DOWN	BIT(3)
> +
> +#define RZN1_ADC_VC_REG(_n)	(0xc0 + 0x4 * (_n))
> +#define RZN1_ADC_VC_ADC2_ENABLE		BIT(16)
> +#define RZN1_ADC_VC_ADC1_ENABLE		BIT(15)
> +#define RZN1_ADC_VC_ADC2_CHANNEL_SEL_MASK	GENMASK(5, 3)
> +#define RZN1_ADC_VC_ADC2_CHANNEL_SEL(_c)	FIELD_PREP(RZN1_ADC_VC_ADC2_CHANNEL_SEL_MASK, _c)
> +#define RZN1_ADC_VC_ADC1_CHANNEL_SEL_MASK	GENMASK(2, 0)
> +#define RZN1_ADC_VC_ADC1_CHANNEL_SEL(_c)	FIELD_PREP(RZN1_ADC_VC_ADC1_CHANNEL_SEL_MASK, _c)

Similar to below. I'd just have the slightly longer FIELD_PREP() inline

> +
> +#define RZN1_ADC_ADC1_DATA_REG(_n)	(0x100 + 0x4 * (_n))
> +#define RZN1_ADC_ADC2_DATA_REG(_n)	(0x140 + 0x4 * (_n))
> +#define RZN1_ADC_ADCX_DATA_DATA_MASK	GENMASK(11, 0)
> +#define RZN1_ADC_ADCX_GET_DATA(_reg)	FIELD_GET(RZN1_ADC_ADCX_DATA_DATA_MASK, _reg)
For this one and some of the others I'm not convinced the extra macro
adds value over just using FIELD_GET() inline.
Your defines have good names, so it is pretty clear what is going on from
just the field name.


> +static int rzn1_adc_core_get_vref_mv(struct rzn1_adc_core *adc_core)
Is this realistically going to change at runtime?

Long time back we always wrote drivers to allow for that happening
but in reality it turned out people very rarely use vrefs that are
adjusted after power up, so these days it's generally acceptable to grab
voltages at power up and use those for all time.

> +{
> +	int vref_uv;
> +
> +	if (!adc_core->vref)
> +		return -ENODEV;
> +
> +	vref_uv = regulator_get_voltage(adc_core->vref);
> +	if (vref_uv < 0)
> +		return vref_uv;
> +
> +	return vref_uv / 1000;
> +}

> +
> +static int rzn1_adc_read_raw_ch(struct rzn1_adc *rzn1_adc, unsigned int chan, int *val)
> +{
> +	u32 *adc1_data, *adc2_data;
> +	int adc1_ch, adc2_ch;
> +	u32 adc_data;
> +	int ret;
> +
> +	if (chan < 8) {
> +		/* chan 0..7 used to get ADC1 ch 0..7 */
> +		adc1_ch = chan;
> +		adc1_data = &adc_data;
> +		adc2_ch = -1;
> +		adc2_data = NULL;
> +	} else if (chan < 16) {
> +		/* chan 8..15 used to get ADC2 ch 0..7 */
> +		adc1_ch = -1;
> +		adc1_data = NULL;
> +		adc2_ch = chan - 8;
> +		adc2_data = &adc_data;
> +	} else {
> +		return -EINVAL;
> +	}
> +
> +	ret = pm_runtime_resume_and_get(rzn1_adc->dev);
There is a new toy to simplify code flow like this (see recent commits
to pm_runtime.h for more info). Note this went in during the merge window
and so has very few users yet.  I plan to look at how thoroughly we
can use it in other drivers when I get some time.  The ACQUIRE() stuff
can also probably work for some other things like claiming direct mode.
Given how new it is, I'm not going to insist on anyone using it for
runtime pm just yet.

	ACQUIRE(pm_runtime_active_auto_try_enabled, pm)(dev);
	ret = ACQUIRE_ERR(pm_runtime_active_auto_try_enabled, &pm);
	if (ret < 0)
		return ret;

	guard(mutex)(&rzn1_adc->lock);

Then you can return directly in all error cases rather than needing the goto end;
Not that this is only a good idea in cases like this where all gotos become unnecessary/
Mixing the two is a bad idea (as cleanup.h docs call out)

> +	if (ret < 0)
> +		return ret;
> +
> +	mutex_lock(&rzn1_adc->lock);
> +
> +	rzn1_adc_vc_setup_conversion(rzn1_adc, chan, adc1_ch, adc2_ch);
> +
> +	ret = rzn1_adc_vc_start_conversion(rzn1_adc, chan);
> +	if (ret)
> +		goto end;
> +
> +	ret = rzn1_adc_vc_wait_conversion(rzn1_adc, chan, adc1_data, adc2_data);
> +	if (ret) {
> +		rzn1_adc_vc_stop_conversion(rzn1_adc, chan);
> +		goto end;
> +	}
> +
> +	*val = adc_data;
> +	ret = IIO_VAL_INT;
> +
> +end:
> +	mutex_unlock(&rzn1_adc->lock);
> +
> +	pm_runtime_mark_last_busy(rzn1_adc->dev);

As below. This no longer needs to be explicitly called.

> +	pm_runtime_put_autosuspend(rzn1_adc->dev);
> +
> +	return ret;
> +}



> +
> +static int rzn1_adc_set_iio_dev_channels(struct rzn1_adc *rzn1_adc,
> +					 struct iio_dev *indio_dev)
> +{
> +	int adc_used;
> +
> +	adc_used = rzn1_adc->adc_core[0].is_used ? 0x01 : 0x00;
> +	adc_used |= rzn1_adc->adc_core[1].is_used ? 0x02 : 0x00;
> +
> +	switch (adc_used) {
> +	case 0x01:
> +		indio_dev->channels = rzn1_adc1_channels;
> +		indio_dev->num_channels = ARRAY_SIZE(rzn1_adc1_channels);
> +		return 0;
> +	case 0x02:
> +		indio_dev->channels = rzn1_adc2_channels;
> +		indio_dev->num_channels = ARRAY_SIZE(rzn1_adc2_channels);
> +		return 0;
> +	case 0x03:
> +		indio_dev->channels = rzn1_adc1_adc2_channels;
> +		indio_dev->num_channels = ARRAY_SIZE(rzn1_adc1_adc2_channels);

Are we likely to see more complex sets?  If not the bitmap seems like an
unnecessary bit of extra complexity.

> +		return 0;
> +	default:
> +		break;
> +	}
> +
> +	dev_err(rzn1_adc->dev, "Failed to set IIO channels, no ADC core used\n");
> +	return -ENODEV;
> +}
> +
> +static int rzn1_adc_probe(struct platform_device *pdev)
> +{
> +	struct device *dev = &pdev->dev;
> +	struct iio_dev *indio_dev;
> +	struct rzn1_adc *rzn1_adc;
> +	int ret;
> +
> +	indio_dev = devm_iio_device_alloc(dev, sizeof(*rzn1_adc));
> +	if (!indio_dev)
> +		return -ENOMEM;
> +
> +	rzn1_adc = iio_priv(indio_dev);
> +	rzn1_adc->dev = dev;
> +	mutex_init(&rzn1_adc->lock);
For new code small preference for enabling the debug stuff that
	ret = devm_mutex_init()
	if (ret)
		return ret;

ends up registering.  I'm not sure it will often be useful but it is
now cheap to turn on.

> +
> +	rzn1_adc->regs = devm_platform_ioremap_resource(pdev, 0);
> +	if (IS_ERR(rzn1_adc->regs))
> +		return PTR_ERR(rzn1_adc->regs);
> +
> +	rzn1_adc->pclk = devm_clk_get(dev, "pclk");
> +	if (IS_ERR(rzn1_adc->pclk))
> +		return dev_err_probe(dev, PTR_ERR(rzn1_adc->pclk), "Failed to get pclk\n");
> +
> +	rzn1_adc->adc_clk = devm_clk_get(dev, "adc-clk");
> +	if (IS_ERR(rzn1_adc->pclk))
> +		return dev_err_probe(dev, PTR_ERR(rzn1_adc->pclk), "Failed to get adc-clk\n");
> +
> +	ret = rzn1_adc_core_get_regulators(rzn1_adc, &rzn1_adc->adc_core[0],
> +					   "adc1-avdd", "adc1-vref");
> +	if (ret)
> +		return ret;
> +
> +	ret = rzn1_adc_core_get_regulators(rzn1_adc, &rzn1_adc->adc_core[1],
> +					   "adc2-avdd", "adc2-vref");
> +	if (ret)
> +		return ret;
> +
> +	platform_set_drvdata(pdev, indio_dev);
> +
> +	indio_dev->name = dev_name(dev);
> +	indio_dev->info = &rzn1_adc_info;
> +	indio_dev->modes = INDIO_DIRECT_MODE;
> +	ret = rzn1_adc_set_iio_dev_channels(rzn1_adc, indio_dev);
> +	if (ret)
> +		return ret;
> +
> +	ret = rzn1_adc_enable(rzn1_adc);
> +	if (ret)
> +		return ret;
> +
> +	pm_runtime_set_autosuspend_delay(dev, 500);
> +	pm_runtime_use_autosuspend(dev);
> +	pm_runtime_get_noresume(dev);
> +	pm_runtime_set_active(dev);
> +	pm_runtime_enable(dev);
> +
> +	ret = devm_iio_device_register(dev, indio_dev);
> +	if (ret)
> +		goto disable;
> +
> +	pm_runtime_mark_last_busy(dev);

No longer need this.  It is called inside pm_runtime_put_autosuspend()

> +	pm_runtime_put_autosuspend(dev);
> +
> +	return 0;
> +
> +disable:
> +	pm_runtime_disable(dev);
> +	pm_runtime_put_noidle(dev);
> +	pm_runtime_set_suspended(dev);
> +	pm_runtime_dont_use_autosuspend(dev);
> +
> +	rzn1_adc_disable(rzn1_adc);

So this came up in Nuno's review and I think you said you were looking at it,,
but at this level moving the whole lot to devm seems likely to simplify things
a little at least.

> +	return ret;
> +}
> +
> +static void rzn1_adc_remove(struct platform_device *pdev)
> +{
> +	struct iio_dev *indio_dev = platform_get_drvdata(pdev);
> +	struct rzn1_adc *rzn1_adc = iio_priv(indio_dev);
> +
> +	pm_runtime_disable(rzn1_adc->dev);
> +	pm_runtime_set_suspended(rzn1_adc->dev);
> +	pm_runtime_dont_use_autosuspend(rzn1_adc->dev);
> +
> +	rzn1_adc_disable(rzn1_adc);
> +}

> +
> +static const struct dev_pm_ops rzn1_adc_pm_ops = {
> +	SET_SYSTEM_SLEEP_PM_OPS(pm_runtime_force_suspend, pm_runtime_force_resume)
> +	SET_RUNTIME_PM_OPS(rzn1_adc_pm_runtime_suspend, rzn1_adc_pm_runtime_resume, NULL)

Isn't this opencoded DEFINE_RUNTIME_DEV_PM_OPS()?

> +};
> +
> +static const struct of_device_id rzn1_adc_of_match[] = {
> +	{ .compatible = "renesas,rzn1-adc" },
> +	{ /* sentinel */ },
No trailing comma and the comment can go.  We kind of standardized
on
	{ }

in IIO for these a while back in the interests of consistency rather
than any strong reason to pick particular formatting over another.

> +};
> +
> +MODULE_DEVICE_TABLE(of, rzn1_adc_of_match);
> +
> +static struct platform_driver rzn1_adc_driver = {
> +	.probe = rzn1_adc_probe,
> +	.remove = rzn1_adc_remove,
> +	.driver = {
> +		.name = "rzn1-adc",
> +		.of_match_table = of_match_ptr(rzn1_adc_of_match),
I think Nuno already got this but drop of_match_ptr().
Sure we may not care about the other firmware types that can use
of_match_table (but are not OF) but in general I'd rather not have
examples of this in tree and in practice saving space for someone who
is building with COMPILE_TEST and no DT support is not a goal I care about ;)

> +		.pm = pm_ptr(&rzn1_adc_pm_ops),
> +	},
> +};

[PATCH 3/4] ARM: dts: renesas: r9a06g032: Add the ADC device

[Herve Codina (Schneider Electric)]

The ADC available in the r9a06g032 SoC can use up to two internal ACD
cores (ADC1 and ADC2) those internal cores are handled through ADC
controller virtual channels.

Describe this device.

Signed-off-by: Herve Codina (Schneider Electric) <herve.codina@bootlin.com>
---
 arch/arm/boot/dts/renesas/r9a06g032.dtsi | 10 ++++++++++
 1 file changed, 10 insertions(+)

diff --git a/arch/arm/boot/dts/renesas/r9a06g032.dtsi b/arch/arm/boot/dts/renesas/r9a06g032.dtsi
index 13a60656b044..2bc07372bafa 100644
--- a/arch/arm/boot/dts/renesas/r9a06g032.dtsi
+++ b/arch/arm/boot/dts/renesas/r9a06g032.dtsi
@@ -290,6 +290,16 @@ i2c2: i2c@40064000 {
 			status = "disabled";
 		};
 
+		adc: adc@40065000 {
+			compatible = "renesas,r9a06g032-adc", "renesas,rzn1-adc";
+			reg = <0x40065000 0x200>;
+			clocks = <&sysctrl R9A06G032_HCLK_ADC>, <&sysctrl R9A06G032_CLK_ADC>;
+			clock-names = "pclk", "adc-clk";
+			power-domains = <&sysctrl>;
+			#io-channel-cells = <1>;
+			status = "disabled";
+		};
+
 		pinctrl: pinctrl@40067000 {
 			compatible = "renesas,r9a06g032-pinctrl", "renesas,rzn1-pinctrl";
 			reg = <0x40067000 0x1000>, <0x51000000 0x480>;
-- 
2.51.0

Re: [PATCH 3/4] ARM: dts: renesas: r9a06g032: Add the ADC device

[Wolfram Sang]

[-- Attachment #1: Type: text/plain, Size: 520 bytes --]

On Wed, Oct 15, 2025 at 04:28:15PM +0200, Herve Codina (Schneider Electric) wrote:
> The ADC available in the r9a06g032 SoC can use up to two internal ACD
> cores (ADC1 and ADC2) those internal cores are handled through ADC
> controller virtual channels.
> 
> Describe this device.
> 
> Signed-off-by: Herve Codina (Schneider Electric) <herve.codina@bootlin.com>

Looks good given the current bindings. Despite clock-names will need an update:

Reviewed-by: Wolfram Sang <wsa+renesas@sang-engineering.com>


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

[PATCH 4/4] MAINTAINERS: Add the Renesas RZ/N1 ADC driver entry

[Herve Codina (Schneider Electric)]

After contributing the driver, add myself as the maintainer for the
Renesas RZ/N1 ADC driver.

Signed-off-by: Herve Codina (Schneider Electric) <herve.codina@bootlin.com>
---
 MAINTAINERS | 8 ++++++++
 1 file changed, 8 insertions(+)

diff --git a/MAINTAINERS b/MAINTAINERS
index 46126ce2f968..40af68e4c9e9 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -21874,6 +21874,14 @@ F:	include/dt-bindings/net/pcs-rzn1-miic.h
 F:	include/linux/pcs-rzn1-miic.h
 F:	net/dsa/tag_rzn1_a5psw.c
 
+RENESAS RZ/N1 ADC DRIVER
+M:	Herve Codina <herve.codina@bootlin.com>
+L:	linux-iio@vger.kernel.org
+L:	linux-renesas-soc@vger.kernel.org
+S:	Supported
+F:	Documentation/devicetree/bindings/iio/adc/renesas,rzn1-adc.yaml
+F:	drivers/iio/adc/rzn1-adc.c
+
 RENESAS RZ/N1 DWMAC GLUE LAYER
 M:	Romain Gantois <romain.gantois@bootlin.com>
 S:	Maintained
-- 
2.51.0

Re: [PATCH 4/4] MAINTAINERS: Add the Renesas RZ/N1 ADC driver entry

[Wolfram Sang]

[-- Attachment #1: Type: text/plain, Size: 296 bytes --]

Hi Herve,

thanks for stepping up to be the maintainer!

> +RENESAS RZ/N1 ADC DRIVER
> +M:	Herve Codina <herve.codina@bootlin.com>
> +L:	linux-iio@vger.kernel.org

The iio list can go because it will be added by the generic IIO entry
which handles all "drivers/iio".

Happy hacking,

   Wolfram


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]