[PATCH v4 0/3] Add SARADC support on Sophgo CV18XX series

[PATCH v4 0/3] Add SARADC support on Sophgo CV18XX series

[Thomas Bonnefille]

This patchset adds initial ADC support for Sophgo SoC. This driver can
work with or without interrupt and in "Active" and "No-Die" domains
depending on if a clock is provided.

Link: https://github.com/sophgo/sophgo-doc/releases/download/sg2002-trm-v1.0/sg2002_trm_en.pdf

Signed-off-by: Thomas Bonnefille <thomas.bonnefille@bootlin.com>
---
Changes in v4:
- Lowercase register hexadecimal value in dts
- Reorder properties in dts
- Use only a const in the compatible property of the device tree bindings
- Specify the series of SoC in the driver to avoid confusing with other
  Sophgo SoCs
- Add channel description in the bindings
- Use FIELD_PREP in the default configuration
- Index channels from 0
- Return PTR_ERR instead of IS_ERR
- Link to v3: https://lore.kernel.org/r/20240731-sg2002-adc-v3-0-5ac40a518c0a@bootlin.com

Changes in v3:
- Subdivide default cycle configuration into multiple elementary
  configurations
- Fix formatting in the driver
- Use devm_mutex_init
- Use devm_clk_get_enabled now because the clock is no more optional
- Remove handling of Saradc in No-Die Domain as RTC isn't implemented yet
- Use cv1800-saradc as default compatible instead of a wildcard
- Link to v2: https://lore.kernel.org/r/20240705-sg2002-adc-v2-0-83428c20a9b2@bootlin.com

Changes in v2:
- Drop modifications in MAINTAINERS file
- Rename the ADC from "sophgo-adc" to "sophgo-cv18xx-adc" to avoid
  conflict with ADCs available in future Sophgo SoCs.
- Reorder nodes in DT to match DTS coding style
- Switch from including <linux/of.h> to <linux/mod_devicetable.h>
- Use scoped_guard instead of mutex_lock/unlock
- Check IRQ Status in the handler
- Change IIO device name
- Use devm_clk_get_optional_enabled instead of a clock variable
- Init completion before the IRQ request
- Removed unnecessary iio_info structure in the private data of the
  driver
- Use SoC specific compatible in the bindings and device trees
- Link to v1: https://lore.kernel.org/r/20240702-sg2002-adc-v1-0-ac66e076a756@bootlin.com

---
Thomas Bonnefille (3):
      dt-bindings: iio: adc: sophgo,cv18xx-saradc.yaml: Add Sophgo CV18XX SARADC binding
      iio: adc: sophgo-saradc: Add driver for Sophgo CV18XX series SARADC
      riscv: dts: sophgo: Add SARADC description for Sophgo CV18XX

 .../bindings/iio/adc/sophgo,cv18xx-saradc.yaml     |  85 +++++++++
 arch/riscv/boot/dts/sophgo/cv18xx.dtsi             |  20 ++
 drivers/iio/adc/Kconfig                            |  10 +
 drivers/iio/adc/Makefile                           |   1 +
 drivers/iio/adc/sophgo-cv18xx-adc.c                | 208 +++++++++++++++++++++
 5 files changed, 324 insertions(+)
---
base-commit: 7c626ce4bae1ac14f60076d00eafe71af30450ba
change-id: 20240527-sg2002-adc-924b862cd3f2

Best regards,
-- 
Thomas Bonnefille <thomas.bonnefille@bootlin.com>

[PATCH v4 1/3] dt-bindings: iio: adc: sophgo,cv18xx-saradc.yaml: Add Sophgo CV18XX SARADC binding

[Thomas Bonnefille]

The Sophgo SARADC is a Successive Approximation ADC that can be found in
the Sophgo SoC.

Signed-off-by: Thomas Bonnefille <thomas.bonnefille@bootlin.com>
---
 .../bindings/iio/adc/sophgo,cv18xx-saradc.yaml     | 85 ++++++++++++++++++++++
 1 file changed, 85 insertions(+)

diff --git a/Documentation/devicetree/bindings/iio/adc/sophgo,cv18xx-saradc.yaml b/Documentation/devicetree/bindings/iio/adc/sophgo,cv18xx-saradc.yaml
new file mode 100644
index 000000000000..846590808e5f
--- /dev/null
+++ b/Documentation/devicetree/bindings/iio/adc/sophgo,cv18xx-saradc.yaml
@@ -0,0 +1,85 @@
+# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/iio/adc/sophgo,cv18xx-saradc.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title:
+  Sophgo CV18XX SoC series 3 channels Successive Approximation Analog to
+  Digital Converters
+
+maintainers:
+  - Thomas Bonnefille <thomas.bonnefille@bootlin.com>
+
+description:
+  Datasheet at https://github.com/sophgo/sophgo-doc/releases
+
+properties:
+  compatible:
+    const: sophgo,cv1800b-saradc
+
+  reg:
+    maxItems: 1
+
+  interrupts:
+    maxItems: 1
+
+  clocks:
+    maxItems: 1
+
+  '#address-cells':
+    const: 1
+
+  '#size-cells':
+    const: 0
+
+patternProperties:
+  "^channel@[0-3]+$":
+    $ref: adc.yaml
+
+    description: |
+      Represents the channels of the ADC.
+
+    properties:
+      reg:
+        description: |
+          The channel number. It can have up to 3 channels numbered from 0 to 2.
+        items:
+          - minimum: 0
+            maximum: 2
+
+    required:
+      - reg
+
+    additionalProperties: false
+
+
+required:
+  - compatible
+  - reg
+  - clocks
+
+unevaluatedProperties: false
+
+examples:
+  - |
+    #include <dt-bindings/clock/sophgo,cv1800.h>
+    #include <dt-bindings/interrupt-controller/irq.h>
+    adc@30f0000 {
+        compatible = "sophgo,cv1800b-saradc";
+        reg = <0x030f0000 0x1000>;
+        clocks = <&clk CLK_SARADC>;
+        interrupts = <100 IRQ_TYPE_LEVEL_HIGH>;
+        #address-cells = <1>;
+        #size-cells = <0>;
+
+        channel@0 {
+            reg = <0>;
+        };
+        channel@1 {
+            reg = <1>;
+        };
+        channel@2 {
+            reg = <2>;
+        };
+    };

-- 
2.46.0

[PATCH v4 2/3] iio: adc: sophgo-saradc: Add driver for Sophgo CV18XX series SARADC

[Thomas Bonnefille]

This adds a driver for the common Sophgo SARADC.

Signed-off-by: Thomas Bonnefille <thomas.bonnefille@bootlin.com>
---
 drivers/iio/adc/Kconfig             |  10 ++
 drivers/iio/adc/Makefile            |   1 +
 drivers/iio/adc/sophgo-cv18xx-adc.c | 208 ++++++++++++++++++++++++++++++++++++
 3 files changed, 219 insertions(+)

diff --git a/drivers/iio/adc/Kconfig b/drivers/iio/adc/Kconfig
index f60fe85a30d5..b10bf26d8e86 100644
--- a/drivers/iio/adc/Kconfig
+++ b/drivers/iio/adc/Kconfig
@@ -1156,6 +1156,16 @@ config SC27XX_ADC
 	  This driver can also be built as a module. If so, the module
 	  will be called sc27xx_adc.
 
+config SOPHGO_CV18XX_ADC
+	tristate "Sophgo CV18XX series SARADC"
+	depends on ARCH_SOPHGO || COMPILE_TEST
+	help
+	  Say yes here to build support for the SARADC integrated inside
+	  the Sophgo CV18XX series SoCs.
+
+	  This driver can also be built as a module. If so, the module
+	  will be called sophgo_cv18xx_adc.
+
 config SPEAR_ADC
 	tristate "ST SPEAr ADC"
 	depends on PLAT_SPEAR || COMPILE_TEST
diff --git a/drivers/iio/adc/Makefile b/drivers/iio/adc/Makefile
index d370e066544e..24c241b12ef0 100644
--- a/drivers/iio/adc/Makefile
+++ b/drivers/iio/adc/Makefile
@@ -105,6 +105,7 @@ obj-$(CONFIG_ROCKCHIP_SARADC) += rockchip_saradc.o
 obj-$(CONFIG_RZG2L_ADC) += rzg2l_adc.o
 obj-$(CONFIG_SC27XX_ADC) += sc27xx_adc.o
 obj-$(CONFIG_SD_ADC_MODULATOR) += sd_adc_modulator.o
+obj-$(CONFIG_SOPHGO_CV18XX_ADC) += sophgo-cv18xx-adc.o
 obj-$(CONFIG_SPEAR_ADC) += spear_adc.o
 obj-$(CONFIG_STM32_ADC_CORE) += stm32-adc-core.o
 obj-$(CONFIG_STM32_ADC) += stm32-adc.o
diff --git a/drivers/iio/adc/sophgo-cv18xx-adc.c b/drivers/iio/adc/sophgo-cv18xx-adc.c
new file mode 100644
index 000000000000..ab7ee0f482cc
--- /dev/null
+++ b/drivers/iio/adc/sophgo-cv18xx-adc.c
@@ -0,0 +1,208 @@
+// SPDX-License-Identifier: GPL-2.0-or-later
+/*
+ *  Sophgo CV18XX series SARADC Driver
+ *
+ *  Copyright (C) Bootlin 2024
+ *  Author: Thomas Bonnefille <thomas.bonnefille@bootlin.com>
+ */
+
+#include "linux/err.h"
+#include <linux/bitfield.h>
+#include <linux/clk.h>
+#include <linux/completion.h>
+#include <linux/dev_printk.h>
+#include <linux/interrupt.h>
+#include <linux/iio/iio.h>
+#include <linux/iopoll.h>
+#include <linux/mod_devicetable.h>
+#include <linux/module.h>
+#include <linux/mutex.h>
+#include <linux/platform_device.h>
+
+#define CV18XX_ADC_CTRL_REG				0x04
+#define		CV18XX_ADC_EN				BIT(0)
+#define		CV18XX_ADC_SEL(x)			BIT((x) + 5)
+#define CV18XX_ADC_STATUS_REG				0x08
+#define		CV18XX_ADC_BUSY				BIT(0)
+#define CV18XX_ADC_CYC_SET_REG				0x0C
+/* The default cycle configuration is set to maximize the accuracy */
+#define		CV18XX_ADC_DEF_STARTUP_CYCLE_MASK	0x1F
+#define		CV18XX_ADC_DEF_SAMPLE_WINDOW_MASK	0xF00
+#define		CV18XX_ADC_DEF_CLOCK_DIVIDER_MASK	0xF000
+#define		CV18XX_ADC_DEF_COMPARE_CYCLE_MASK	0xF0000
+#define CV18XX_ADC_CH_RESULT_REG(x)			(0x14 + 4 * (x))
+#define		CV18XX_ADC_CH_RESULT			GENMASK(11, 0)
+#define		CV18XX_ADC_CH_VALID			BIT(15)
+#define CV18XX_ADC_INTR_EN_REG				0x20
+#define CV18XX_ADC_INTR_CLR_REG				0x24
+#define		CV18XX_ADC_INTR_CLR_BIT			BIT(0)
+#define CV18XX_ADC_INTR_STA_REG				0x28
+#define		CV18XX_ADC_INTR_STA_BIT			BIT(0)
+
+#define CV18XX_ADC_CHANNEL(index)					\
+	{								\
+		.type = IIO_VOLTAGE,					\
+		.indexed = 1,						\
+		.channel = index,					\
+		.info_mask_separate = BIT(IIO_CHAN_INFO_RAW),		\
+		.info_mask_shared_by_type = BIT(IIO_CHAN_INFO_SCALE),	\
+		.scan_index = index,					\
+	}
+
+struct cv18xx_adc {
+	struct completion completion;
+	void __iomem *regs;
+	struct mutex lock; /* ADC Control and Result register */
+	int irq;
+};
+
+static const struct iio_chan_spec sophgo_channels[] = {
+	CV18XX_ADC_CHANNEL(0),
+	CV18XX_ADC_CHANNEL(1),
+	CV18XX_ADC_CHANNEL(2),
+};
+
+static void cv18xx_adc_start_measurement(struct cv18xx_adc *saradc,
+					    int channel)
+{
+	writel(0, saradc->regs + CV18XX_ADC_CTRL_REG);
+	writel(CV18XX_ADC_SEL(channel) | CV18XX_ADC_EN,
+	       saradc->regs + CV18XX_ADC_CTRL_REG);
+}
+
+static int cv18xx_adc_wait(struct cv18xx_adc *saradc)
+{
+	if (saradc->irq < 0) {
+		u32 reg;
+
+		return readl_poll_timeout(saradc->regs + CV18XX_ADC_STATUS_REG,
+					  reg, !(reg & CV18XX_ADC_BUSY),
+					  500, 1000000);
+	}
+	return wait_for_completion_timeout(&saradc->completion,
+					  msecs_to_jiffies(1000)) > 0
+					  ? 0 : -ETIMEDOUT;
+}
+
+static int cv18xx_adc_read_raw(struct iio_dev *indio_dev,
+				  struct iio_chan_spec const *chan,
+				  int *val, int *val2, long mask)
+{
+
+	switch (mask) {
+	case IIO_CHAN_INFO_RAW:{
+		struct cv18xx_adc *saradc = iio_priv(indio_dev);
+		u32 sample;
+
+		scoped_guard(mutex, &saradc->lock) {
+			int ret;
+
+			cv18xx_adc_start_measurement(saradc, chan->scan_index);
+			ret = cv18xx_adc_wait(saradc);
+			if (ret < 0)
+				return ret;
+
+			sample = readl(saradc->regs + CV18XX_ADC_CH_RESULT_REG(chan->scan_index));
+		}
+		if (!(sample & CV18XX_ADC_CH_VALID))
+			return -ENODATA;
+
+		*val = sample & CV18XX_ADC_CH_RESULT;
+		return IIO_VAL_INT;
+		}
+	case IIO_CHAN_INFO_SCALE:
+		*val = 3300;
+		*val2 = 12;
+		return IIO_VAL_FRACTIONAL_LOG2;
+	default:
+		return -EINVAL;
+	}
+}
+
+static irqreturn_t cv18xx_adc_interrupt_handler(int irq, void *private)
+{
+	struct cv18xx_adc *saradc = private;
+
+	if (!(FIELD_GET(CV18XX_ADC_INTR_STA_BIT,
+			readl(saradc->regs + CV18XX_ADC_INTR_STA_REG))))
+		return IRQ_NONE;
+
+	writel(CV18XX_ADC_INTR_CLR_BIT, saradc->regs + CV18XX_ADC_INTR_CLR_REG);
+	complete(&saradc->completion);
+	return IRQ_HANDLED;
+}
+
+static const struct iio_info cv18xx_adc_info = {
+	.read_raw = &cv18xx_adc_read_raw,
+};
+
+static int cv18xx_adc_probe(struct platform_device *pdev)
+{
+	struct cv18xx_adc *saradc;
+	struct iio_dev *indio_dev;
+	struct clk *clk;
+	int ret;
+
+	indio_dev = devm_iio_device_alloc(&pdev->dev, sizeof(*saradc));
+	if (!indio_dev)
+		return -ENOMEM;
+
+	saradc = iio_priv(indio_dev);
+	indio_dev->name = "sophgo-cv18xx-adc";
+	indio_dev->modes = INDIO_DIRECT_MODE;
+	indio_dev->info = &cv18xx_adc_info;
+	indio_dev->num_channels = ARRAY_SIZE(sophgo_channels);
+	indio_dev->channels = sophgo_channels;
+
+	clk = devm_clk_get_enabled(&pdev->dev, NULL);
+	if (IS_ERR(clk))
+		return PTR_ERR(clk);
+
+	saradc->regs = devm_platform_ioremap_resource(pdev, 0);
+	if (IS_ERR(saradc->regs))
+		return PTR_ERR(saradc->regs);
+
+	saradc->irq = platform_get_irq_optional(pdev, 0);
+	if (saradc->irq >= 0) {
+		init_completion(&saradc->completion);
+		ret = devm_request_irq(&pdev->dev, saradc->irq,
+				       cv18xx_adc_interrupt_handler, 0,
+				       dev_name(&pdev->dev), saradc);
+		if (ret)
+			return ret;
+
+		writel(1, saradc->regs + CV18XX_ADC_INTR_EN_REG);
+	}
+
+	ret = devm_mutex_init(&pdev->dev, &saradc->lock);
+	if (ret)
+		return ret;
+
+	platform_set_drvdata(pdev, indio_dev);
+	writel(FIELD_PREP(CV18XX_ADC_DEF_STARTUP_CYCLE_MASK, 0xF) |
+	       FIELD_PREP(CV18XX_ADC_DEF_SAMPLE_WINDOW_MASK, 0xF) |
+	       FIELD_PREP(CV18XX_ADC_DEF_CLOCK_DIVIDER_MASK, 0x1) |
+	       FIELD_PREP(CV18XX_ADC_DEF_COMPARE_CYCLE_MASK, 0xF),
+	       saradc->regs + CV18XX_ADC_CYC_SET_REG);
+
+	return devm_iio_device_register(&pdev->dev, indio_dev);
+}
+
+static const struct of_device_id cv18xx_adc_match[] = {
+	{ .compatible = "sophgo,cv1800b-saradc", },
+	{ }
+};
+MODULE_DEVICE_TABLE(of, cv18xx_adc_match);
+
+static struct platform_driver cv18xx_adc_driver = {
+	.driver	= {
+		.name		= "sophgo-cv18xx-saradc",
+		.of_match_table	= cv18xx_adc_match,
+	},
+	.probe = cv18xx_adc_probe,
+};
+module_platform_driver(cv18xx_adc_driver);
+
+MODULE_AUTHOR("Thomas Bonnefille <thomas.bonnefille@bootlin.com>");
+MODULE_DESCRIPTION("Sophgo CV18XX series SARADC driver");
+MODULE_LICENSE("GPL");

-- 
2.46.0

[PATCH v4 3/3] riscv: dts: sophgo: Add SARADC description for Sophgo CV18XX

[Thomas Bonnefille]

Adds SARADC nodes for the common Successive Approximation Analog to
Digital Converter used in Sophgo CV18xx series SoC.
This patch adds two nodes for the two controllers the board, one in
the Active domain and the other in the No-Die domain.

Signed-off-by: Thomas Bonnefille <thomas.bonnefille@bootlin.com>
---
 arch/riscv/boot/dts/sophgo/cv18xx.dtsi | 20 ++++++++++++++++++++
 1 file changed, 20 insertions(+)

diff --git a/arch/riscv/boot/dts/sophgo/cv18xx.dtsi b/arch/riscv/boot/dts/sophgo/cv18xx.dtsi
index 891932ae470f..71a2618852fa 100644
--- a/arch/riscv/boot/dts/sophgo/cv18xx.dtsi
+++ b/arch/riscv/boot/dts/sophgo/cv18xx.dtsi
@@ -133,6 +133,26 @@ portd: gpio-controller@0 {
 			};
 		};
 
+		saradc: adc@30f0000 {
+			compatible = "sophgo,cv1800b-saradc";
+			reg = <0x030f0000 0x1000>;
+			clocks = <&clk CLK_SARADC>;
+			interrupts = <100 IRQ_TYPE_LEVEL_HIGH>;
+			#address-cells = <1>;
+			#size-cells = <0>;
+			status = "disabled";
+
+			channel@0 {
+				reg = <0>;
+			};
+			channel@1 {
+				reg = <1>;
+			};
+			channel@2 {
+				reg = <2>;
+			};
+		};
+
 		i2c0: i2c@4000000 {
 			compatible = "snps,designware-i2c";
 			reg = <0x04000000 0x10000>;

-- 
2.46.0

Re: [PATCH v4 1/3] dt-bindings: iio: adc: sophgo,cv18xx-saradc.yaml: Add Sophgo CV18XX SARADC binding

[Conor Dooley]

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

On Mon, Aug 12, 2024 at 05:00:55PM +0200, Thomas Bonnefille wrote:
> The Sophgo SARADC is a Successive Approximation ADC that can be found in
> the Sophgo SoC.
> 
> Signed-off-by: Thomas Bonnefille <thomas.bonnefille@bootlin.com>
> ---
>  .../bindings/iio/adc/sophgo,cv18xx-saradc.yaml     | 85 ++++++++++++++++++++++
>  1 file changed, 85 insertions(+)
> 
> diff --git a/Documentation/devicetree/bindings/iio/adc/sophgo,cv18xx-saradc.yaml b/Documentation/devicetree/bindings/iio/adc/sophgo,cv18xx-saradc.yaml
> new file mode 100644
> index 000000000000..846590808e5f
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/iio/adc/sophgo,cv18xx-saradc.yaml
> @@ -0,0 +1,85 @@
> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/iio/adc/sophgo,cv18xx-saradc.yaml#

Filename matching the compatible please.

> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title:
> +  Sophgo CV18XX SoC series 3 channels Successive Approximation Analog to
> +  Digital Converters
> +
> +maintainers:
> +  - Thomas Bonnefille <thomas.bonnefille@bootlin.com>
> +
> +description:
> +  Datasheet at https://github.com/sophgo/sophgo-doc/releases
> +
> +properties:
> +  compatible:
> +    const: sophgo,cv1800b-saradc
> +
> +  reg:
> +    maxItems: 1
> +
> +  interrupts:
> +    maxItems: 1
> +
> +  clocks:
> +    maxItems: 1
> +
> +  '#address-cells':
> +    const: 1
> +
> +  '#size-cells':
> +    const: 0
> +
> +patternProperties:
> +  "^channel@[0-3]+$":
> +    $ref: adc.yaml
> +
> +    description: |

This | is not required.

> +      Represents the channels of the ADC.
> +
> +    properties:
> +      reg:
> +        description: |
> +          The channel number. It can have up to 3 channels numbered from 0 to 2.
> +        items:
> +          - minimum: 0
> +            maximum: 2

Is this sufficient to limit the number of channels to 3? Aren't you relying
on the unique unit addresses warning in dtc to limit it, rather than
actually limiting with min/maxItems?

Otherwise, looks fine to me.

Cheers,
Conor.

> +
> +    required:
> +      - reg
> +
> +    additionalProperties: false
> +
> +
> +required:
> +  - compatible
> +  - reg
> +  - clocks
> +
> +unevaluatedProperties: false
> +
> +examples:
> +  - |
> +    #include <dt-bindings/clock/sophgo,cv1800.h>
> +    #include <dt-bindings/interrupt-controller/irq.h>
> +    adc@30f0000 {
> +        compatible = "sophgo,cv1800b-saradc";
> +        reg = <0x030f0000 0x1000>;
> +        clocks = <&clk CLK_SARADC>;
> +        interrupts = <100 IRQ_TYPE_LEVEL_HIGH>;
> +        #address-cells = <1>;
> +        #size-cells = <0>;
> +
> +        channel@0 {
> +            reg = <0>;
> +        };
> +        channel@1 {
> +            reg = <1>;
> +        };
> +        channel@2 {
> +            reg = <2>;
> +        };
> +    };
> 
> -- 
> 2.46.0
> 

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

Re: [PATCH v4 2/3] iio: adc: sophgo-saradc: Add driver for Sophgo CV18XX series SARADC

[Chen Wang]

On 2024/8/12 23:00, Thomas Bonnefille wrote:
> This adds a driver for the common Sophgo SARADC.
Not common. Please double check the whole patchset.
>
> Signed-off-by: Thomas Bonnefille <thomas.bonnefille@bootlin.com>
> ---
>   drivers/iio/adc/Kconfig             |  10 ++
>   drivers/iio/adc/Makefile            |   1 +
>   drivers/iio/adc/sophgo-cv18xx-adc.c | 208 ++++++++++++++++++++++++++++++++++++
>   3 files changed, 219 insertions(+)
>
> diff --git a/drivers/iio/adc/Kconfig b/drivers/iio/adc/Kconfig
> index f60fe85a30d5..b10bf26d8e86 100644
> --- a/drivers/iio/adc/Kconfig
> +++ b/drivers/iio/adc/Kconfig
> @@ -1156,6 +1156,16 @@ config SC27XX_ADC
>   	  This driver can also be built as a module. If so, the module
>   	  will be called sc27xx_adc.
>   
> +config SOPHGO_CV18XX_ADC
> +	tristate "Sophgo CV18XX series SARADC"
> +	depends on ARCH_SOPHGO || COMPILE_TEST
> +	help
> +	  Say yes here to build support for the SARADC integrated inside
> +	  the Sophgo CV18XX series SoCs.
> +
> +	  This driver can also be built as a module. If so, the module
> +	  will be called sophgo_cv18xx_adc.
> +
>   config SPEAR_ADC
>   	tristate "ST SPEAr ADC"
>   	depends on PLAT_SPEAR || COMPILE_TEST
> diff --git a/drivers/iio/adc/Makefile b/drivers/iio/adc/Makefile
> index d370e066544e..24c241b12ef0 100644
> --- a/drivers/iio/adc/Makefile
> +++ b/drivers/iio/adc/Makefile
> @@ -105,6 +105,7 @@ obj-$(CONFIG_ROCKCHIP_SARADC) += rockchip_saradc.o
>   obj-$(CONFIG_RZG2L_ADC) += rzg2l_adc.o
>   obj-$(CONFIG_SC27XX_ADC) += sc27xx_adc.o
>   obj-$(CONFIG_SD_ADC_MODULATOR) += sd_adc_modulator.o
> +obj-$(CONFIG_SOPHGO_CV18XX_ADC) += sophgo-cv18xx-adc.o
>   obj-$(CONFIG_SPEAR_ADC) += spear_adc.o
>   obj-$(CONFIG_STM32_ADC_CORE) += stm32-adc-core.o
>   obj-$(CONFIG_STM32_ADC) += stm32-adc.o
> diff --git a/drivers/iio/adc/sophgo-cv18xx-adc.c b/drivers/iio/adc/sophgo-cv18xx-adc.c
> new file mode 100644
> index 000000000000..ab7ee0f482cc
> --- /dev/null
> +++ b/drivers/iio/adc/sophgo-cv18xx-adc.c
> @@ -0,0 +1,208 @@
> +// SPDX-License-Identifier: GPL-2.0-or-later
> +/*
> + *  Sophgo CV18XX series SARADC Driver
> + *
> + *  Copyright (C) Bootlin 2024
> + *  Author: Thomas Bonnefille <thomas.bonnefille@bootlin.com>
> + */
> +
> +#include "linux/err.h"
<linux/err.h> ? And please sort with the file name alphabeta.
> +#include <linux/bitfield.h>
> +#include <linux/clk.h>
> +#include <linux/completion.h>
> +#include <linux/dev_printk.h>
> +#include <linux/interrupt.h>
> +#include <linux/iio/iio.h>
> +#include <linux/iopoll.h>
> +#include <linux/mod_devicetable.h>
> +#include <linux/module.h>
> +#include <linux/mutex.h>
> +#include <linux/platform_device.h>
> +
> +#define CV18XX_ADC_CTRL_REG				0x04
> +#define		CV18XX_ADC_EN				BIT(0)

Should be one space between "#define" and macro-name. Please check this 
for the whole file.

Run "./scripts/checkpatch.pl --strict" if it can help you check this 
format error?

> +#define		CV18XX_ADC_SEL(x)			BIT((x) + 5)
> +#define CV18XX_ADC_STATUS_REG				0x08
> +#define		CV18XX_ADC_BUSY				BIT(0)
> +#define CV18XX_ADC_CYC_SET_REG				0x0C
> +/* The default cycle configuration is set to maximize the accuracy */
> +#define		CV18XX_ADC_DEF_STARTUP_CYCLE_MASK	0x1F
> +#define		CV18XX_ADC_DEF_SAMPLE_WINDOW_MASK	0xF00
> +#define		CV18XX_ADC_DEF_CLOCK_DIVIDER_MASK	0xF000
> +#define		CV18XX_ADC_DEF_COMPARE_CYCLE_MASK	0xF0000
> +#define CV18XX_ADC_CH_RESULT_REG(x)			(0x14 + 4 * (x))
> +#define		CV18XX_ADC_CH_RESULT			GENMASK(11, 0)
> +#define		CV18XX_ADC_CH_VALID			BIT(15)
> +#define CV18XX_ADC_INTR_EN_REG				0x20
> +#define CV18XX_ADC_INTR_CLR_REG				0x24
> +#define		CV18XX_ADC_INTR_CLR_BIT			BIT(0)
> +#define CV18XX_ADC_INTR_STA_REG				0x28
> +#define		CV18XX_ADC_INTR_STA_BIT			BIT(0)
> +
> +#define CV18XX_ADC_CHANNEL(index)					\
> +	{								\
> +		.type = IIO_VOLTAGE,					\
> +		.indexed = 1,						\
> +		.channel = index,					\
> +		.info_mask_separate = BIT(IIO_CHAN_INFO_RAW),		\
> +		.info_mask_shared_by_type = BIT(IIO_CHAN_INFO_SCALE),	\
> +		.scan_index = index,					\
> +	}
> +
> +struct cv18xx_adc {
> +	struct completion completion;
> +	void __iomem *regs;
> +	struct mutex lock; /* ADC Control and Result register */
> +	int irq;
> +};
> +
> +static const struct iio_chan_spec sophgo_channels[] = {
> +	CV18XX_ADC_CHANNEL(0),
> +	CV18XX_ADC_CHANNEL(1),
> +	CV18XX_ADC_CHANNEL(2),
> +};
> +
> +static void cv18xx_adc_start_measurement(struct cv18xx_adc *saradc,
> +					    int channel)
> +{
> +	writel(0, saradc->regs + CV18XX_ADC_CTRL_REG);
> +	writel(CV18XX_ADC_SEL(channel) | CV18XX_ADC_EN,
> +	       saradc->regs + CV18XX_ADC_CTRL_REG);
> +}
> +
> +static int cv18xx_adc_wait(struct cv18xx_adc *saradc)
> +{
> +	if (saradc->irq < 0) {
> +		u32 reg;
> +
> +		return readl_poll_timeout(saradc->regs + CV18XX_ADC_STATUS_REG,
> +					  reg, !(reg & CV18XX_ADC_BUSY),
> +					  500, 1000000);
> +	}
> +	return wait_for_completion_timeout(&saradc->completion,
> +					  msecs_to_jiffies(1000)) > 0
> +					  ? 0 : -ETIMEDOUT;
> +}
> +
> +static int cv18xx_adc_read_raw(struct iio_dev *indio_dev,
> +				  struct iio_chan_spec const *chan,
> +				  int *val, int *val2, long mask)
> +{
> +
> +	switch (mask) {
> +	case IIO_CHAN_INFO_RAW:{
> +		struct cv18xx_adc *saradc = iio_priv(indio_dev);
> +		u32 sample;
> +
> +		scoped_guard(mutex, &saradc->lock) {
> +			int ret;
> +
> +			cv18xx_adc_start_measurement(saradc, chan->scan_index);
> +			ret = cv18xx_adc_wait(saradc);
> +			if (ret < 0)
> +				return ret;
> +
> +			sample = readl(saradc->regs + CV18XX_ADC_CH_RESULT_REG(chan->scan_index));
> +		}
> +		if (!(sample & CV18XX_ADC_CH_VALID))
> +			return -ENODATA;
> +
> +		*val = sample & CV18XX_ADC_CH_RESULT;
> +		return IIO_VAL_INT;
> +		}
> +	case IIO_CHAN_INFO_SCALE:
> +		*val = 3300;
> +		*val2 = 12;
> +		return IIO_VAL_FRACTIONAL_LOG2;
> +	default:
> +		return -EINVAL;
> +	}
> +}
> +
> +static irqreturn_t cv18xx_adc_interrupt_handler(int irq, void *private)
> +{
> +	struct cv18xx_adc *saradc = private;
> +
> +	if (!(FIELD_GET(CV18XX_ADC_INTR_STA_BIT,
> +			readl(saradc->regs + CV18XX_ADC_INTR_STA_REG))))
> +		return IRQ_NONE;
> +
> +	writel(CV18XX_ADC_INTR_CLR_BIT, saradc->regs + CV18XX_ADC_INTR_CLR_REG);
> +	complete(&saradc->completion);
> +	return IRQ_HANDLED;
> +}
> +
> +static const struct iio_info cv18xx_adc_info = {
> +	.read_raw = &cv18xx_adc_read_raw,
> +};
> +
> +static int cv18xx_adc_probe(struct platform_device *pdev)
> +{
> +	struct cv18xx_adc *saradc;
> +	struct iio_dev *indio_dev;
> +	struct clk *clk;
> +	int ret;
> +
> +	indio_dev = devm_iio_device_alloc(&pdev->dev, sizeof(*saradc));
> +	if (!indio_dev)
> +		return -ENOMEM;
> +
> +	saradc = iio_priv(indio_dev);
> +	indio_dev->name = "sophgo-cv18xx-adc";
> +	indio_dev->modes = INDIO_DIRECT_MODE;
> +	indio_dev->info = &cv18xx_adc_info;
> +	indio_dev->num_channels = ARRAY_SIZE(sophgo_channels);
> +	indio_dev->channels = sophgo_channels;
> +
> +	clk = devm_clk_get_enabled(&pdev->dev, NULL);
> +	if (IS_ERR(clk))
> +		return PTR_ERR(clk);
> +
> +	saradc->regs = devm_platform_ioremap_resource(pdev, 0);
> +	if (IS_ERR(saradc->regs))
> +		return PTR_ERR(saradc->regs);
> +
> +	saradc->irq = platform_get_irq_optional(pdev, 0);
> +	if (saradc->irq >= 0) {
> +		init_completion(&saradc->completion);
> +		ret = devm_request_irq(&pdev->dev, saradc->irq,
> +				       cv18xx_adc_interrupt_handler, 0,
> +				       dev_name(&pdev->dev), saradc);
> +		if (ret)
> +			return ret;
> +
> +		writel(1, saradc->regs + CV18XX_ADC_INTR_EN_REG);
> +	}
> +
> +	ret = devm_mutex_init(&pdev->dev, &saradc->lock);
> +	if (ret)
> +		return ret;
> +
> +	platform_set_drvdata(pdev, indio_dev);
> +	writel(FIELD_PREP(CV18XX_ADC_DEF_STARTUP_CYCLE_MASK, 0xF) |
> +	       FIELD_PREP(CV18XX_ADC_DEF_SAMPLE_WINDOW_MASK, 0xF) |
> +	       FIELD_PREP(CV18XX_ADC_DEF_CLOCK_DIVIDER_MASK, 0x1) |
> +	       FIELD_PREP(CV18XX_ADC_DEF_COMPARE_CYCLE_MASK, 0xF),
> +	       saradc->regs + CV18XX_ADC_CYC_SET_REG);
> +
> +	return devm_iio_device_register(&pdev->dev, indio_dev);
> +}
> +
> +static const struct of_device_id cv18xx_adc_match[] = {
> +	{ .compatible = "sophgo,cv1800b-saradc", },
> +	{ }
> +};
> +MODULE_DEVICE_TABLE(of, cv18xx_adc_match);
> +
> +static struct platform_driver cv18xx_adc_driver = {
> +	.driver	= {
> +		.name		= "sophgo-cv18xx-saradc",
> +		.of_match_table	= cv18xx_adc_match,
> +	},
> +	.probe = cv18xx_adc_probe,
> +};
> +module_platform_driver(cv18xx_adc_driver);
> +
> +MODULE_AUTHOR("Thomas Bonnefille <thomas.bonnefille@bootlin.com>");
> +MODULE_DESCRIPTION("Sophgo CV18XX series SARADC driver");
> +MODULE_LICENSE("GPL");
>

Re: [PATCH v4 3/3] riscv: dts: sophgo: Add SARADC description for Sophgo CV18XX

[Chen Wang]

On 2024/8/12 23:00, Thomas Bonnefille wrote:
> Adds SARADC nodes for the common Successive Approximation Analog to
> Digital Converter used in Sophgo CV18xx series SoC.
> This patch adds two nodes for the two controllers the board, one in
> the Active domain and the other in the No-Die domain.
Where is the node for the No-die domain?
>
> Signed-off-by: Thomas Bonnefille <thomas.bonnefille@bootlin.com>
> ---
>   arch/riscv/boot/dts/sophgo/cv18xx.dtsi | 20 ++++++++++++++++++++
>   1 file changed, 20 insertions(+)
>
> diff --git a/arch/riscv/boot/dts/sophgo/cv18xx.dtsi b/arch/riscv/boot/dts/sophgo/cv18xx.dtsi
> index 891932ae470f..71a2618852fa 100644
> --- a/arch/riscv/boot/dts/sophgo/cv18xx.dtsi
> +++ b/arch/riscv/boot/dts/sophgo/cv18xx.dtsi
> @@ -133,6 +133,26 @@ portd: gpio-controller@0 {
>   			};
>   		};
>   
> +		saradc: adc@30f0000 {
> +			compatible = "sophgo,cv1800b-saradc";
> +			reg = <0x030f0000 0x1000>;
> +			clocks = <&clk CLK_SARADC>;
> +			interrupts = <100 IRQ_TYPE_LEVEL_HIGH>;
> +			#address-cells = <1>;
> +			#size-cells = <0>;
> +			status = "disabled";
> +
> +			channel@0 {
> +				reg = <0>;
> +			};
> +			channel@1 {
> +				reg = <1>;
> +			};
> +			channel@2 {
> +				reg = <2>;
> +			};
> +		};
> +
>   		i2c0: i2c@4000000 {
>   			compatible = "snps,designware-i2c";
>   			reg = <0x04000000 0x10000>;
>

Re: [PATCH v4 3/3] riscv: dts: sophgo: Add SARADC description for Sophgo CV18XX

[Inochi Amaoto]

On Tue, Aug 13, 2024 at 09:45:53AM GMT, Chen Wang wrote:
> 
> On 2024/8/12 23:00, Thomas Bonnefille wrote:
> > Adds SARADC nodes for the common Successive Approximation Analog to
> > Digital Converter used in Sophgo CV18xx series SoC.
> > This patch adds two nodes for the two controllers the board, one in
> > the Active domain and the other in the No-Die domain.
> Where is the node for the No-die domain?

I have suggested Thomas not add the node for the RTC domain.
It is not clear that which clock is used for the RTC domain,
it will good to add this node after the RTC is implemented.

> > 
> > Signed-off-by: Thomas Bonnefille <thomas.bonnefille@bootlin.com>
> > ---
> >   arch/riscv/boot/dts/sophgo/cv18xx.dtsi | 20 ++++++++++++++++++++
> >   1 file changed, 20 insertions(+)
> > 
> > diff --git a/arch/riscv/boot/dts/sophgo/cv18xx.dtsi b/arch/riscv/boot/dts/sophgo/cv18xx.dtsi
> > index 891932ae470f..71a2618852fa 100644
> > --- a/arch/riscv/boot/dts/sophgo/cv18xx.dtsi
> > +++ b/arch/riscv/boot/dts/sophgo/cv18xx.dtsi
> > @@ -133,6 +133,26 @@ portd: gpio-controller@0 {
> >   			};
> >   		};
> > +		saradc: adc@30f0000 {
> > +			compatible = "sophgo,cv1800b-saradc";
> > +			reg = <0x030f0000 0x1000>;
> > +			clocks = <&clk CLK_SARADC>;
> > +			interrupts = <100 IRQ_TYPE_LEVEL_HIGH>;
> > +			#address-cells = <1>;
> > +			#size-cells = <0>;
> > +			status = "disabled";
> > +
> > +			channel@0 {
> > +				reg = <0>;
> > +			};
> > +			channel@1 {
> > +				reg = <1>;
> > +			};
> > +			channel@2 {
> > +				reg = <2>;
> > +			};
> > +		};
> > +
> >   		i2c0: i2c@4000000 {
> >   			compatible = "snps,designware-i2c";
> >   			reg = <0x04000000 0x10000>;
> > 

Re: [PATCH v4 1/3] dt-bindings: iio: adc: sophgo,cv18xx-saradc.yaml: Add Sophgo CV18XX SARADC binding

[Krzysztof Kozlowski]

On 12/08/2024 17:00, Thomas Bonnefille wrote:
> The Sophgo SARADC is a Successive Approximation ADC that can be found in
> the Sophgo SoC.
> 
> Signed-off-by: Thomas Bonnefille <thomas.bonnefille@bootlin.com>


<form letter>
This is a friendly reminder during the review process.

It seems my or other reviewer's previous comments were not fully
addressed. Maybe the feedback got lost between the quotes, maybe you
just forgot to apply it. Please go back to the previous discussion and
either implement all requested changes or keep discussing them.

Thank you.
</form letter>

.. and more ignored comments further.

> ---
>  .../bindings/iio/adc/sophgo,cv18xx-saradc.yaml     | 85 ++++++++++++++++++++++
>  1 file changed, 85 insertions(+)
> 
> diff --git a/Documentation/devicetree/bindings/iio/adc/sophgo,cv18xx-saradc.yaml b/Documentation/devicetree/bindings/iio/adc/sophgo,cv18xx-saradc.yaml
> new file mode 100644
> index 000000000000..846590808e5f
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/iio/adc/sophgo,cv18xx-saradc.yaml
> @@ -0,0 +1,85 @@
> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/iio/adc/sophgo,cv18xx-saradc.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title:
> +  Sophgo CV18XX SoC series 3 channels Successive Approximation Analog to
> +  Digital Converters
> +
> +maintainers:
> +  - Thomas Bonnefille <thomas.bonnefille@bootlin.com>
> +
> +description:
> +  Datasheet at https://github.com/sophgo/sophgo-doc/releases
> +
> +properties:
> +  compatible:
> +    const: sophgo,cv1800b-saradc
> +
> +  reg:
> +    maxItems: 1
> +
> +  interrupts:
> +    maxItems: 1
> +
> +  clocks:
> +    maxItems: 1
> +
> +  '#address-cells':
> +    const: 1
> +
> +  '#size-cells':
> +    const: 0
> +
> +patternProperties:
> +  "^channel@[0-3]+$":
> +    $ref: adc.yaml
> +
> +    description: |
> +      Represents the channels of the ADC.
> +
> +    properties:
> +      reg:
> +        description: |
> +          The channel number. It can have up to 3 channels numbered from 0 to 2.
> +        items:
> +          - minimum: 0
> +            maximum: 2
> +
> +    required:
> +      - reg
> +
> +    additionalProperties: false
> +
> +

Just one blank line.

> +required:
> +  - compatible
> +  - reg
> +  - clocks
> +
> +unevaluatedProperties: false

No, how did this appear here? This must be additionalProperties.

I already commented on this.

<form letter>
This is a friendly reminder during the review process.

It seems my or other reviewer's previous comments were not fully
addressed. Maybe the feedback got lost between the quotes, maybe you
just forgot to apply it. Please go back to the previous discussion and
either implement all requested changes or keep discussing them.

Thank you.
</form letter>

Best regards,
Krzysztof

Re: [PATCH v4 3/3] riscv: dts: sophgo: Add SARADC description for Sophgo CV18XX

[Chen Wang]

On 2024/8/13 9:50, Inochi Amaoto wrote:
> On Tue, Aug 13, 2024 at 09:45:53AM GMT, Chen Wang wrote:
>> On 2024/8/12 23:00, Thomas Bonnefille wrote:
>>> Adds SARADC nodes for the common Successive Approximation Analog to
>>> Digital Converter used in Sophgo CV18xx series SoC.
>>> This patch adds two nodes for the two controllers the board, one in
>>> the Active domain and the other in the No-Die domain.
>> Where is the node for the No-die domain?
> I have suggested Thomas not add the node for the RTC domain.
> It is not clear that which clock is used for the RTC domain,
> it will good to add this node after the RTC is implemented.
OK，so please update the commit message and mention this.
>
>>> Signed-off-by: Thomas Bonnefille <thomas.bonnefille@bootlin.com>
>>> ---
>>>    arch/riscv/boot/dts/sophgo/cv18xx.dtsi | 20 ++++++++++++++++++++
>>>    1 file changed, 20 insertions(+)
>>>
>>> diff --git a/arch/riscv/boot/dts/sophgo/cv18xx.dtsi b/arch/riscv/boot/dts/sophgo/cv18xx.dtsi
>>> index 891932ae470f..71a2618852fa 100644
>>> --- a/arch/riscv/boot/dts/sophgo/cv18xx.dtsi
>>> +++ b/arch/riscv/boot/dts/sophgo/cv18xx.dtsi
>>> @@ -133,6 +133,26 @@ portd: gpio-controller@0 {
>>>    			};
>>>    		};
>>> +		saradc: adc@30f0000 {
>>> +			compatible = "sophgo,cv1800b-saradc";
>>> +			reg = <0x030f0000 0x1000>;
>>> +			clocks = <&clk CLK_SARADC>;
>>> +			interrupts = <100 IRQ_TYPE_LEVEL_HIGH>;
>>> +			#address-cells = <1>;
>>> +			#size-cells = <0>;
>>> +			status = "disabled";
>>> +
>>> +			channel@0 {
>>> +				reg = <0>;
>>> +			};
>>> +			channel@1 {
>>> +				reg = <1>;
>>> +			};
>>> +			channel@2 {
>>> +				reg = <2>;
>>> +			};
>>> +		};
>>> +
>>>    		i2c0: i2c@4000000 {
>>>    			compatible = "snps,designware-i2c";
>>>    			reg = <0x04000000 0x10000>;
>>>

Re: [PATCH v4 2/3] iio: adc: sophgo-saradc: Add driver for Sophgo CV18XX series SARADC

[Jonathan Cameron]

On Mon, 12 Aug 2024 17:00:56 +0200
Thomas Bonnefille <thomas.bonnefille@bootlin.com> wrote:

> This adds a driver for the common Sophgo SARADC.
> 
> Signed-off-by: Thomas Bonnefille <thomas.bonnefille@bootlin.com>
Similar comment on wild cards to the one you resolved in the dt binding
applies to the driver.

So normally convention is pick a part and name after that for
all function names, parameters etc.

cv1800b seems appropriate given the binding.

Other than that, just one trivial comment from me.

Jonathan


> diff --git a/drivers/iio/adc/sophgo-cv18xx-adc.c b/drivers/iio/adc/sophgo-cv18xx-adc.c
> new file mode 100644
> index 000000000000..ab7ee0f482cc
> --- /dev/null
> +++ b/drivers/iio/adc/sophgo-cv18xx-adc.c
> @@ -0,0 +1,208 @@

> +static int cv18xx_adc_probe(struct platform_device *pdev)
> +{
> +	struct cv18xx_adc *saradc;
> +	struct iio_dev *indio_dev;
> +	struct clk *clk;
> +	int ret;
> +
> +	indio_dev = devm_iio_device_alloc(&pdev->dev, sizeof(*saradc));
> +	if (!indio_dev)
> +		return -ENOMEM;
> +
> +	saradc = iio_priv(indio_dev);
> +	indio_dev->name = "sophgo-cv18xx-adc";
This definitely doesn't want to be wildcard based

> +	indio_dev->modes = INDIO_DIRECT_MODE;
> +	indio_dev->info = &cv18xx_adc_info;
> +	indio_dev->num_channels = ARRAY_SIZE(sophgo_channels);
> +	indio_dev->channels = sophgo_channels;
> +
> +	clk = devm_clk_get_enabled(&pdev->dev, NULL);
> +	if (IS_ERR(clk))
> +		return PTR_ERR(clk);
> +
> +	saradc->regs = devm_platform_ioremap_resource(pdev, 0);
> +	if (IS_ERR(saradc->regs))
> +		return PTR_ERR(saradc->regs);
> +
> +	saradc->irq = platform_get_irq_optional(pdev, 0);
> +	if (saradc->irq >= 0) {
> +		init_completion(&saradc->completion);
> +		ret = devm_request_irq(&pdev->dev, saradc->irq,
> +				       cv18xx_adc_interrupt_handler, 0,
> +				       dev_name(&pdev->dev), saradc);
> +		if (ret)
> +			return ret;
> +
> +		writel(1, saradc->regs + CV18XX_ADC_INTR_EN_REG);
> +	}
> +
> +	ret = devm_mutex_init(&pdev->dev, &saradc->lock);
> +	if (ret)
> +		return ret;
> +
> +	platform_set_drvdata(pdev, indio_dev);

Is this used?

> +	writel(FIELD_PREP(CV18XX_ADC_DEF_STARTUP_CYCLE_MASK, 0xF) |
> +	       FIELD_PREP(CV18XX_ADC_DEF_SAMPLE_WINDOW_MASK, 0xF) |
> +	       FIELD_PREP(CV18XX_ADC_DEF_CLOCK_DIVIDER_MASK, 0x1) |
> +	       FIELD_PREP(CV18XX_ADC_DEF_COMPARE_CYCLE_MASK, 0xF),
> +	       saradc->regs + CV18XX_ADC_CYC_SET_REG);
> +
> +	return devm_iio_device_register(&pdev->dev, indio_dev);
> +}

Re: [PATCH v4 1/3] dt-bindings: iio: adc: sophgo,cv18xx-saradc.yaml: Add Sophgo CV18XX SARADC binding

[Thomas Bonnefille]

Hello Conor,

On 8/12/24 5:53 PM, Conor Dooley wrote:
> On Mon, Aug 12, 2024 at 05:00:55PM +0200, Thomas Bonnefille wrote:
>> The Sophgo SARADC is a Successive Approximation ADC that can be found in
>> the Sophgo SoC.
>>
>> Signed-off-by: Thomas Bonnefille <thomas.bonnefille@bootlin.com>
>> ---
>>   .../bindings/iio/adc/sophgo,cv18xx-saradc.yaml     | 85 ++++++++++++++++++++++
>>   1 file changed, 85 insertions(+)
>>
>> diff --git a/Documentation/devicetree/bindings/iio/adc/sophgo,cv18xx-saradc.yaml b/Documentation/devicetree/bindings/iio/adc/sophgo,cv18xx-saradc.yaml
>> new file mode 100644
>> index 000000000000..846590808e5f
>> --- /dev/null
>> +++ b/Documentation/devicetree/bindings/iio/adc/sophgo,cv18xx-saradc.yaml
>> @@ -0,0 +1,85 @@
>> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
>> +%YAML 1.2
>> +---
>> +$id: http://devicetree.org/schemas/iio/adc/sophgo,cv18xx-saradc.yaml#
> 
> Filename matching the compatible please.
> 
>> +$schema: http://devicetree.org/meta-schemas/core.yaml#
>> +
>> +title:
>> +  Sophgo CV18XX SoC series 3 channels Successive Approximation Analog to
>> +  Digital Converters
>> +
>> +maintainers:
>> +  - Thomas Bonnefille <thomas.bonnefille@bootlin.com>
>> +
>> +description:
>> +  Datasheet at https://github.com/sophgo/sophgo-doc/releases
>> +
>> +properties:
>> +  compatible:
>> +    const: sophgo,cv1800b-saradc
>> +
>> +  reg:
>> +    maxItems: 1
>> +
>> +  interrupts:
>> +    maxItems: 1
>> +
>> +  clocks:
>> +    maxItems: 1
>> +
>> +  '#address-cells':
>> +    const: 1
>> +
>> +  '#size-cells':
>> +    const: 0
>> +
>> +patternProperties:
>> +  "^channel@[0-3]+$":
>> +    $ref: adc.yaml
>> +
>> +    description: |
> 
> This | is not required.
> 
>> +      Represents the channels of the ADC.
>> +
>> +    properties:
>> +      reg:
>> +        description: |
>> +          The channel number. It can have up to 3 channels numbered from 0 to 2.
>> +        items:
>> +          - minimum: 0
>> +            maximum: 2
> 
> Is this sufficient to limit the number of channels to 3? Aren't you relying
> on the unique unit addresses warning in dtc to limit it, rather than
> actually limiting with min/maxItems?
> 
It seems like I can't use min/maxItems on this property. I think that it 
is using size-cells + address-cells to deduce that the number of items 
should be equal to 1.
Looking at the dtschema repository it seems to be the case in reg.yaml 
with address-cells/size-cells = 2/2, 1/1 and 2/1.
If I try to use maxItems here :

     properties:
       reg:
         maxItems: 1
         items:
           - minimum: 0
             maximum: 2

I get this strange error message from `make dt_binding_check`:

DTEX 
Documentation/devicetree/bindings/iio/adc/sophgo,cv1800b-saradc.example.dts
/home/thomas/linux/Documentation/devicetree/bindings/iio/adc/sophgo,cv1800b-saradc.yaml: 
patternProperties:^channel@[0-2]+$:properties:reg: {'maxItems': 1, 
'items': [{'minimum': 0, 'maximum': 2}]} should not be valid under 
{'required': ['maxItems']}
	hint: "maxItems" is not needed with an "items" list
	from schema $id: http://devicetree.org/meta-schemas/items.yaml#
/home/thomas/linux/Documentation/devicetree/bindings/iio/adc/sophgo,cv1800b-saradc.yaml: 
patternProperties:^channel@[0-2]+$:properties:reg: 'anyOf' conditional 
failed, one must be fixed:
	'items' is not one of ['maxItems', 'description', 'deprecated']
		hint: Only "maxItems" is required for a single entry if there are no 
constraints defined for the values.
	'maxItems' is not one of ['description', 'deprecated', 'const', 'enum', 
'minimum', 'maximum', 'multipleOf', 'default', '$ref', 'oneOf']
	'items' is not one of ['description', 'deprecated', 'const', 'enum', 
'minimum', 'maximum', 'multipleOf', 'default', '$ref', 'oneOf']
	1 is less than the minimum of 2
		hint: Arrays must be described with a combination of 
minItems/maxItems/items
	hint: cell array properties must define how many entries and what the 
entries are when there is more than one entry.
	from schema $id: http://devicetree.org/meta-schemas/core.yaml#

Isn't it okay to just use minimum and maximum and rely on 
address-cells/size-cells for the number of items allowed ?

Re: [PATCH v4 1/3] dt-bindings: iio: adc: sophgo,cv18xx-saradc.yaml: Add Sophgo CV18XX SARADC binding

[Conor Dooley]

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

On Tue, Aug 20, 2024 at 06:21:07PM +0200, Thomas Bonnefille wrote:
> Hello Conor,
> 
> On 8/12/24 5:53 PM, Conor Dooley wrote:
> > On Mon, Aug 12, 2024 at 05:00:55PM +0200, Thomas Bonnefille wrote:
> > > The Sophgo SARADC is a Successive Approximation ADC that can be found in
> > > the Sophgo SoC.
> > > 
> > > Signed-off-by: Thomas Bonnefille <thomas.bonnefille@bootlin.com>
> > > ---
> > >   .../bindings/iio/adc/sophgo,cv18xx-saradc.yaml     | 85 ++++++++++++++++++++++
> > >   1 file changed, 85 insertions(+)
> > > 
> > > diff --git a/Documentation/devicetree/bindings/iio/adc/sophgo,cv18xx-saradc.yaml b/Documentation/devicetree/bindings/iio/adc/sophgo,cv18xx-saradc.yaml
> > > new file mode 100644
> > > index 000000000000..846590808e5f
> > > --- /dev/null
> > > +++ b/Documentation/devicetree/bindings/iio/adc/sophgo,cv18xx-saradc.yaml
> > > @@ -0,0 +1,85 @@
> > > +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> > > +%YAML 1.2
> > > +---
> > > +$id: http://devicetree.org/schemas/iio/adc/sophgo,cv18xx-saradc.yaml#
> > 
> > Filename matching the compatible please.
> > 
> > > +$schema: http://devicetree.org/meta-schemas/core.yaml#
> > > +
> > > +title:
> > > +  Sophgo CV18XX SoC series 3 channels Successive Approximation Analog to
> > > +  Digital Converters
> > > +
> > > +maintainers:
> > > +  - Thomas Bonnefille <thomas.bonnefille@bootlin.com>
> > > +
> > > +description:
> > > +  Datasheet at https://github.com/sophgo/sophgo-doc/releases
> > > +
> > > +properties:
> > > +  compatible:
> > > +    const: sophgo,cv1800b-saradc
> > > +
> > > +  reg:
> > > +    maxItems: 1
> > > +
> > > +  interrupts:
> > > +    maxItems: 1
> > > +
> > > +  clocks:
> > > +    maxItems: 1
> > > +
> > > +  '#address-cells':
> > > +    const: 1
> > > +
> > > +  '#size-cells':
> > > +    const: 0
> > > +
> > > +patternProperties:
> > > +  "^channel@[0-3]+$":
> > > +    $ref: adc.yaml
> > > +
> > > +    description: |
> > 
> > This | is not required.
> > 
> > > +      Represents the channels of the ADC.
> > > +
> > > +    properties:
> > > +      reg:
> > > +        description: |
> > > +          The channel number. It can have up to 3 channels numbered from 0 to 2.
> > > +        items:
> > > +          - minimum: 0
> > > +            maximum: 2
> > 
> > Is this sufficient to limit the number of channels to 3? Aren't you relying
> > on the unique unit addresses warning in dtc to limit it, rather than
> > actually limiting with min/maxItems?
> > 
> It seems like I can't use min/maxItems on this property. I think that it is
> using size-cells + address-cells to deduce that the number of items should
> be equal to 1.

I think I was mistaken in talking about mix/max items here. I had the
right idea, but mentioned an incorrect solution - sorry about that. I
wasn't talking about the number of elements in the reg property, what I
meant was limiting the number of channel nodes in the first place -
something which min/maxItems cannot do. As examples of the problem I was
thinking of, see the below two examples:

    adc@30f0000 {
        compatible = "sophgo,cv1800b-saradc";
        reg = <0x030f0000 0x1000>;
        clocks = <&clk CLK_SARADC>;
        interrupts = <100 IRQ_TYPE_LEVEL_HIGH>;
        #address-cells = <1>;
        #size-cells = <0>;

        channel@0 {
            reg = <0>;
        };
        channel@2 {
            reg = <2>;
        };
        channel@22 {
            reg = <2>;
        };
    };

    adc@30f0000 {
        compatible = "sophgo,cv1800b-saradc";
        reg = <0x030f0000 0x1000>;
        clocks = <&clk CLK_SARADC>;
        interrupts = <100 IRQ_TYPE_LEVEL_HIGH>;
        #address-cells = <1>;
        #size-cells = <0>;

        channel@0 {
            reg = <0>;
        };
        channel@2 {
            reg = <2>;
        };
        channel@22 {
            reg = <2>;
        };
    };

The solution is simple, remove the + from the regex. Sorry for sending
you on the wrong track Thomas.

Thanks,
Conor.

> Looking at the dtschema repository it seems to be the case in reg.yaml with
> address-cells/size-cells = 2/2, 1/1 and 2/1.
> If I try to use maxItems here :
> 
>     properties:
>       reg:
>         maxItems: 1
>         items:
>           - minimum: 0
>             maximum: 2
> 
> I get this strange error message from `make dt_binding_check`:
> 
> DTEX
> Documentation/devicetree/bindings/iio/adc/sophgo,cv1800b-saradc.example.dts
> /home/thomas/linux/Documentation/devicetree/bindings/iio/adc/sophgo,cv1800b-saradc.yaml:
> patternProperties:^channel@[0-2]+$:properties:reg: {'maxItems': 1, 'items':
> [{'minimum': 0, 'maximum': 2}]} should not be valid under {'required':
> ['maxItems']}
> 	hint: "maxItems" is not needed with an "items" list
> 	from schema $id: http://devicetree.org/meta-schemas/items.yaml#
> /home/thomas/linux/Documentation/devicetree/bindings/iio/adc/sophgo,cv1800b-saradc.yaml:
> patternProperties:^channel@[0-2]+$:properties:reg: 'anyOf' conditional
> failed, one must be fixed:
> 	'items' is not one of ['maxItems', 'description', 'deprecated']
> 		hint: Only "maxItems" is required for a single entry if there are no
> constraints defined for the values.
> 	'maxItems' is not one of ['description', 'deprecated', 'const', 'enum',
> 'minimum', 'maximum', 'multipleOf', 'default', '$ref', 'oneOf']
> 	'items' is not one of ['description', 'deprecated', 'const', 'enum',
> 'minimum', 'maximum', 'multipleOf', 'default', '$ref', 'oneOf']
> 	1 is less than the minimum of 2
> 		hint: Arrays must be described with a combination of
> minItems/maxItems/items
> 	hint: cell array properties must define how many entries and what the
> entries are when there is more than one entry.
> 	from schema $id: http://devicetree.org/meta-schemas/core.yaml#
> 
> Isn't it okay to just use minimum and maximum and rely on
> address-cells/size-cells for the number of items allowed ?
> 

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

Re: [PATCH v4 1/3] dt-bindings: iio: adc: sophgo,cv18xx-saradc.yaml: Add Sophgo CV18XX SARADC binding

[Miquel Raynal]

Hello,

> > > > +      Represents the channels of the ADC.
> > > > +
> > > > +    properties:
> > > > +      reg:
> > > > +        description: |
> > > > +          The channel number. It can have up to 3 channels numbered from 0 to 2.
> > > > +        items:
> > > > +          - minimum: 0
> > > > +            maximum: 2  
> > > 
> > > Is this sufficient to limit the number of channels to 3? Aren't you relying
> > > on the unique unit addresses warning in dtc to limit it, rather than
> > > actually limiting with min/maxItems?
> > >   
> > It seems like I can't use min/maxItems on this property. I think that it is
> > using size-cells + address-cells to deduce that the number of items should
> > be equal to 1.  

Looking at dt-schema, I couldn't personally understand from where did
the error messages reported by Thomas came from. There are clear
constraints over minItems/maxItems regarding the use of {#address-cells,
#sizez-cells} being {1, 1}, {2, 2} and {2, 1} (in reg.yaml), but nothing
explicit regarding the other situations, namely {1, 0} in this case
which enforces maxItems to 1 is not clearly stated in any of the core
yaml files. Any idea where to look at? Although, I'm convinced there is
something defined because renaming the property from 'reg' to 'foo'
silences these warnings.

> I think I was mistaken in talking about mix/max items here. I had the
> right idea, but mentioned an incorrect solution - sorry about that. I
> wasn't talking about the number of elements in the reg property, what I
> meant was limiting the number of channel nodes in the first place -
> something which min/maxItems cannot do. As examples of the problem I was
> thinking of, see the below two examples:
> 
>     adc@30f0000 {
>         compatible = "sophgo,cv1800b-saradc";
>         reg = <0x030f0000 0x1000>;
>         clocks = <&clk CLK_SARADC>;
>         interrupts = <100 IRQ_TYPE_LEVEL_HIGH>;
>         #address-cells = <1>;
>         #size-cells = <0>;
> 
>         channel@0 {
>             reg = <0>;
>         };
>         channel@2 {
>             reg = <2>;
>         };
>         channel@22 {
>             reg = <2>;
>         };
>     };
> 
>     adc@30f0000 {
>         compatible = "sophgo,cv1800b-saradc";
>         reg = <0x030f0000 0x1000>;
>         clocks = <&clk CLK_SARADC>;
>         interrupts = <100 IRQ_TYPE_LEVEL_HIGH>;
>         #address-cells = <1>;
>         #size-cells = <0>;
> 
>         channel@0 {
>             reg = <0>;
>         };
>         channel@2 {
>             reg = <2>;
>         };
>         channel@22 {
>             reg = <2>;
>         };
>     };
> 
> The solution is simple, remove the + from the regex. Sorry for sending
> you on the wrong track Thomas.

Ah! Thanks Conor for the details, now it makes full sense :-) BTW Thomas
the regex is

	^channel@[0-3]+$

and I guess it should instead be

	^channel@[0-2]$
                    ^

in order to fully match the real indexing constraints you're enforcing
with minimum/maximum.

Thanks,
Miquèl

Re: [PATCH v4 1/3] dt-bindings: iio: adc: sophgo,cv18xx-saradc.yaml: Add Sophgo CV18XX SARADC binding

[Conor Dooley]

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

On Wed, Aug 21, 2024 at 09:41:50AM +0200, Miquel Raynal wrote:
> > > > > +      Represents the channels of the ADC.
> > > > > +
> > > > > +    properties:
> > > > > +      reg:
> > > > > +        description: |
> > > > > +          The channel number. It can have up to 3 channels numbered from 0 to 2.
> > > > > +        items:
> > > > > +          - minimum: 0
> > > > > +            maximum: 2  
> > > > 
> > > > Is this sufficient to limit the number of channels to 3? Aren't you relying
> > > > on the unique unit addresses warning in dtc to limit it, rather than
> > > > actually limiting with min/maxItems?
> > > >   
> > > It seems like I can't use min/maxItems on this property. I think that it is
> > > using size-cells + address-cells to deduce that the number of items should
> > > be equal to 1.  
> 
> Looking at dt-schema, I couldn't personally understand from where did
> the error messages reported by Thomas came from. There are clear

I think the complaints are on a more meta level than that. He provided
an items list
     properties:
       reg:
         maxItems: 1
         items:
           - minimum: 0
             maximum: 2
but this list only has one entry as there's one -. The first complaint
from dt_binding_check is that having maxItems is not needed with an
items list, because the items list contains the maximum number of
elements.

The second one comes from cell.yaml:
https://github.com/devicetree-org/dt-schema/blob/main/dtschema/meta-schemas/cell.yaml

It either allows a single item, with maxItems: 1 or multiple items, in
which case maxitems must be greater than 1. That's where the "anyOf
conditonal failed" and "1 is less than the minimum of 2" stuff comes
from.

I hope that helps?

> constraints over minItems/maxItems regarding the use of {#address-cells,
> #sizez-cells} being {1, 1}, {2, 2} and {2, 1} (in reg.yaml), but nothing
> explicit regarding the other situations, namely {1, 0} in this case
> which enforces maxItems to 1 is not clearly stated in any of the core
> yaml files. Any idea where to look at? Although, I'm convinced there is
> something defined because renaming the property from 'reg' to 'foo'
> silences these warnings.
> 
> > I think I was mistaken in talking about mix/max items here. I had the
> > right idea, but mentioned an incorrect solution - sorry about that. I
> > wasn't talking about the number of elements in the reg property, what I
> > meant was limiting the number of channel nodes in the first place -
> > something which min/maxItems cannot do. As examples of the problem I was
> > thinking of, see the below two examples:
> > 
> >     adc@30f0000 {
> >         compatible = "sophgo,cv1800b-saradc";
> >         reg = <0x030f0000 0x1000>;
> >         clocks = <&clk CLK_SARADC>;
> >         interrupts = <100 IRQ_TYPE_LEVEL_HIGH>;
> >         #address-cells = <1>;
> >         #size-cells = <0>;
> > 
> >         channel@0 {
> >             reg = <0>;
> >         };
> >         channel@2 {
> >             reg = <2>;
> >         };
> >         channel@22 {
> >             reg = <2>;
> >         };
> >     };
> > 
> >     adc@30f0000 {
> >         compatible = "sophgo,cv1800b-saradc";
> >         reg = <0x030f0000 0x1000>;
> >         clocks = <&clk CLK_SARADC>;
> >         interrupts = <100 IRQ_TYPE_LEVEL_HIGH>;
> >         #address-cells = <1>;
> >         #size-cells = <0>;
> > 
> >         channel@0 {
> >             reg = <0>;
> >         };
> >         channel@2 {
> >             reg = <2>;
> >         };
> >         channel@22 {
> >             reg = <2>;
> >         };

I noticed that I pasted two of the same example. I must have just
yoinked the latter to a vim buffer rather than to my clipboard. At least
it didn't matter in the end.

Cheers,
Conor.

> >     };
> > 
> > The solution is simple, remove the + from the regex. Sorry for sending
> > you on the wrong track Thomas.
> 
> Ah! Thanks Conor for the details, now it makes full sense :-) BTW Thomas
> the regex is
> 
> 	^channel@[0-3]+$
> 
> and I guess it should instead be
> 
> 	^channel@[0-2]$
>                     ^
> 
> in order to fully match the real indexing constraints you're enforcing
> with minimum/maximum.
> 
> Thanks,
> Miquèl

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

Re: [PATCH v4 1/3] dt-bindings: iio: adc: sophgo,cv18xx-saradc.yaml: Add Sophgo CV18XX SARADC binding

[Miquel Raynal]

Hi Conor,

conor@kernel.org wrote on Wed, 21 Aug 2024 16:29:42 +0100:

> On Wed, Aug 21, 2024 at 09:41:50AM +0200, Miquel Raynal wrote:
> > > > > > +      Represents the channels of the ADC.
> > > > > > +
> > > > > > +    properties:
> > > > > > +      reg:
> > > > > > +        description: |
> > > > > > +          The channel number. It can have up to 3 channels numbered from 0 to 2.
> > > > > > +        items:
> > > > > > +          - minimum: 0
> > > > > > +            maximum: 2    
> > > > > 
> > > > > Is this sufficient to limit the number of channels to 3? Aren't you relying
> > > > > on the unique unit addresses warning in dtc to limit it, rather than
> > > > > actually limiting with min/maxItems?
> > > > >     
> > > > It seems like I can't use min/maxItems on this property. I think that it is
> > > > using size-cells + address-cells to deduce that the number of items should
> > > > be equal to 1.    
> > 
> > Looking at dt-schema, I couldn't personally understand from where did
> > the error messages reported by Thomas came from. There are clear  
> 
> I think the complaints are on a more meta level than that. He provided
> an items list
>      properties:
>        reg:
>          maxItems: 1
>          items:
>            - minimum: 0
>              maximum: 2
> but this list only has one entry as there's one -. The first complaint
> from dt_binding_check is that having maxItems is not needed with an
> items list, because the items list contains the maximum number of
> elements.
> 
> The second one comes from cell.yaml:
> https://github.com/devicetree-org/dt-schema/blob/main/dtschema/meta-schemas/cell.yaml
> 
> It either allows a single item, with maxItems: 1 or multiple items, in
> which case maxitems must be greater than 1. That's where the "anyOf
> conditonal failed" and "1 is less than the minimum of 2" stuff comes
> from.
> 
> I hope that helps?

Ah yeah, makes sense. Thanks a lot for your feedback!

Miquèl