[PATCH v2 0/3] Add SARADC support on Sophgo SoC

[PATCH v2 0/3] Add SARADC support on Sophgo SoC

[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 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 SARADC binding documentation
      iio: adc: sophgo-saradc: Add driver for Sophgo SARADC
      riscv: dts: sophgo: Add SARADC configuration

 .../bindings/iio/adc/sophgo,cv18xx-saradc.yaml     |  63 +++++++
 arch/riscv/boot/dts/sophgo/cv1800b.dtsi            |   8 +
 arch/riscv/boot/dts/sophgo/cv18xx.dtsi             |  14 ++
 drivers/iio/adc/Kconfig                            |  10 ++
 drivers/iio/adc/Makefile                           |   1 +
 drivers/iio/adc/sophgo-cv18xx-adc.c                | 195 +++++++++++++++++++++
 6 files changed, 291 insertions(+)
---
base-commit: d20f6b3d747c36889b7ce75ee369182af3decb6b
change-id: 20240527-sg2002-adc-924b862cd3f2

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

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

[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     | 63 ++++++++++++++++++++++
 1 file changed, 63 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..31bd8ac6dfa5
--- /dev/null
+++ b/Documentation/devicetree/bindings/iio/adc/sophgo,cv18xx-saradc.yaml
@@ -0,0 +1,63 @@
+# 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:
+    oneOf:
+      - items:
+          - enum:
+              - sophgo,cv1800b-saradc
+          - const: sophgo,cv18xx-saradc
+
+  reg:
+    maxItems: 1
+
+  interrupts:
+    maxItems: 1
+
+  clocks:
+    description:
+      SARADC will use the presence of this clock to determine if the controller
+      needs to be explicitly clocked by it (Active domain) or if it is part of
+      the No-Die Domain, along with the RTC, which does not require explicit
+      clocking.
+    maxItems: 1
+
+required:
+  - compatible
+  - reg
+
+unevaluatedProperties: false
+
+examples:
+  - |
+    #include <dt-bindings/clock/sophgo,cv1800.h>
+    #include <dt-bindings/interrupt-controller/irq.h>
+    /* ADC in the Active domain */
+    adc@30f0000 {
+        compatible = "sophgo,cv1800b-saradc", "sophgo,cv18xx-saradc";
+        clocks = <&clk CLK_SARADC>;
+        interrupts = <100 IRQ_TYPE_LEVEL_HIGH>;
+        reg = <0x030F0000 0x1000>;
+    };
+  - |
+    #include <dt-bindings/clock/sophgo,cv1800.h>
+    #include <dt-bindings/interrupt-controller/irq.h>
+    /* ADC in the No-Die domain */
+    adc@502c000 {
+        compatible = "sophgo,cv1800b-saradc", "sophgo,cv18xx-saradc";
+        reg = <0x0502C000 0x1000>;
+    };

-- 
2.45.2

[PATCH v2 2/3] iio: adc: sophgo-saradc: Add driver for Sophgo 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 | 195 ++++++++++++++++++++++++++++++++++++
 3 files changed, 206 insertions(+)

diff --git a/drivers/iio/adc/Kconfig b/drivers/iio/adc/Kconfig
index 8db68b80b391..e48d8f7f2873 100644
--- a/drivers/iio/adc/Kconfig
+++ b/drivers/iio/adc/Kconfig
@@ -1122,6 +1122,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_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 edb32ce2af02..3967d3953349 100644
--- a/drivers/iio/adc/Makefile
+++ b/drivers/iio/adc/Makefile
@@ -102,6 +102,7 @@ obj-$(CONFIG_ROCKCHIP_SARADC) += rockchip_saradc.o
 obj-$(CONFIG_RICHTEK_RTQ6056) += rtq6056.o
 obj-$(CONFIG_RZG2L_ADC) += rzg2l_adc.o
 obj-$(CONFIG_SC27XX_ADC) += sc27xx_adc.o
+obj-$(CONFIG_SOPHGO_CV18XX_ADC) += sophgo-cv18xx-adc.o
 obj-$(CONFIG_SPEAR_ADC) += spear_adc.o
 obj-$(CONFIG_SUN4I_GPADC) += sun4i-gpadc-iio.o
 obj-$(CONFIG_SUN20I_GPADC) += sun20i-gpadc-iio.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..dd1188b1923e
--- /dev/null
+++ b/drivers/iio/adc/sophgo-cv18xx-adc.c
@@ -0,0 +1,195 @@
+// 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/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)+4)
+#define CV18XX_ADC_STATUS_REG			0x08
+#define		CV18XX_ADC_BUSY			BIT(0)
+#define CV18XX_ADC_CYC_SET_REG			0x0C
+#define		CV18XX_ADC_DEF_CYC_SETTINGS	0xF1F0F
+#define CV18XX_ADC_CH_RESULT_REG(x)		(0x10+4*(x))
+#define		CV18XX_ADC_CH_RESULT		0xfff
+#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_STA_REG			0x28
+
+#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(1),
+	CV18XX_ADC_CHANNEL(2),
+	CV18XX_ADC_CHANNEL(3),
+};
+
+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;
+		int ret;
+
+		scoped_guard(mutex, &saradc->lock) {
+			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 (!(readl(saradc->regs + CV18XX_ADC_INTR_STA_REG) & BIT(0)))
+		return IRQ_NONE;
+
+	writel(1, 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;
+	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;
+
+
+	if (IS_ERR(devm_clk_get_optional_enabled(&pdev->dev, NULL)))
+		dev_dbg(&pdev->dev, "Can't get clock from device tree, using No-Die domain");
+
+	saradc->regs = devm_platform_ioremap_resource(pdev, 0);
+	if (IS_ERR(saradc->regs)) {
+		ret = PTR_ERR(saradc->regs);
+		return ret;
+	}
+
+	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);
+
+	}
+
+
+	mutex_init(&saradc->lock);
+	platform_set_drvdata(pdev, indio_dev);
+	writel(CV18XX_ADC_DEF_CYC_SETTINGS, saradc->regs + CV18XX_ADC_CYC_SET_REG);
+	ret = devm_iio_device_register(&pdev->dev, indio_dev);
+	if (ret)
+		return ret;
+
+	return 0;
+}
+
+static const struct of_device_id cv18xx_adc_match[] = {
+	{ .compatible = "sophgo,cv18xx-saradc", },
+	{ }
+};
+MODULE_DEVICE_TABLE(of, cv18xx_adc_match);
+
+static struct platform_driver cv18xx_adc_driver = {
+	.driver	= {
+		.name		= "sophgo-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 SARADC driver");
+MODULE_LICENSE("GPL");

-- 
2.45.2

[PATCH v2 3/3] riscv: dts: sophgo: Add SARADC configuration

[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/cv1800b.dtsi |  8 ++++++++
 arch/riscv/boot/dts/sophgo/cv18xx.dtsi  | 14 ++++++++++++++
 2 files changed, 22 insertions(+)

diff --git a/arch/riscv/boot/dts/sophgo/cv1800b.dtsi b/arch/riscv/boot/dts/sophgo/cv1800b.dtsi
index ec9530972ae2..73abbb6e5363 100644
--- a/arch/riscv/boot/dts/sophgo/cv1800b.dtsi
+++ b/arch/riscv/boot/dts/sophgo/cv1800b.dtsi
@@ -25,3 +25,11 @@ &clint {
 &clk {
 	compatible = "sophgo,cv1800-clk";
 };
+
+&saradc_active {
+	compatible = "sophgo,cv1800b-saradc", "sophgo,cv18xx-saradc";
+};
+
+&saradc_nodie {
+	compatible = "sophgo,cv1800b-saradc", "sophgo,cv18xx-saradc";
+};
diff --git a/arch/riscv/boot/dts/sophgo/cv18xx.dtsi b/arch/riscv/boot/dts/sophgo/cv18xx.dtsi
index 891932ae470f..752e14fa3d0c 100644
--- a/arch/riscv/boot/dts/sophgo/cv18xx.dtsi
+++ b/arch/riscv/boot/dts/sophgo/cv18xx.dtsi
@@ -133,6 +133,14 @@ portd: gpio-controller@0 {
 			};
 		};
 
+		saradc_active: adc@30f0000 {
+			compatible = "sophgo,cv18xx-saradc";
+			clocks = <&clk CLK_SARADC>;
+			interrupts = <100 IRQ_TYPE_LEVEL_HIGH>;
+			reg = <0x030F0000 0x1000>;
+			status = "disabled";
+		};
+
 		i2c0: i2c@4000000 {
 			compatible = "snps,designware-i2c";
 			reg = <0x04000000 0x10000>;
@@ -297,6 +305,12 @@ sdhci0: mmc@4310000 {
 			status = "disabled";
 		};
 
+		saradc_nodie: adc@502c000 {
+			compatible = "sophgo,cv18xx-saradc";
+			reg = <0x0502C000 0x1000>;
+			status = "disabled";
+		};
+
 		plic: interrupt-controller@70000000 {
 			reg = <0x70000000 0x4000000>;
 			interrupts-extended = <&cpu0_intc 11>, <&cpu0_intc 9>;

-- 
2.45.2

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

[Conor Dooley]

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

On Fri, Jul 05, 2024 at 03:42:23PM +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     | 63 ++++++++++++++++++++++
>  1 file changed, 63 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..31bd8ac6dfa5
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/iio/adc/sophgo,cv18xx-saradc.yaml
> @@ -0,0 +1,63 @@
> +# 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:
> +    oneOf:
> +      - items:
> +          - enum:
> +              - sophgo,cv1800b-saradc
> +          - const: sophgo,cv18xx-saradc

I don't think the fallback here makes sense. If there's other devices
with a compatible programming model added later, we can fall back to the
cv1800b.

> +
> +  reg:
> +    maxItems: 1
> +
> +  interrupts:
> +    maxItems: 1
> +
> +  clocks:
> +    description:
> +      SARADC will use the presence of this clock to determine if the controller
> +      needs to be explicitly clocked by it (Active domain) or if it is part of
> +      the No-Die Domain, along with the RTC, which does not require explicit
> +      clocking.

What does "explicit clocking" mean? Is it clocked directly (or via
dividers) by a clock on the board or another source?

Cheers,
Conor.

> +    maxItems: 1
> +
> +required:
> +  - compatible
> +  - reg
> +
> +unevaluatedProperties: false
> +
> +examples:
> +  - |
> +    #include <dt-bindings/clock/sophgo,cv1800.h>
> +    #include <dt-bindings/interrupt-controller/irq.h>
> +    /* ADC in the Active domain */
> +    adc@30f0000 {
> +        compatible = "sophgo,cv1800b-saradc", "sophgo,cv18xx-saradc";
> +        clocks = <&clk CLK_SARADC>;
> +        interrupts = <100 IRQ_TYPE_LEVEL_HIGH>;
> +        reg = <0x030F0000 0x1000>;
> +    };
> +  - |
> +    #include <dt-bindings/clock/sophgo,cv1800.h>
> +    #include <dt-bindings/interrupt-controller/irq.h>
> +    /* ADC in the No-Die domain */
> +    adc@502c000 {
> +        compatible = "sophgo,cv1800b-saradc", "sophgo,cv18xx-saradc";
> +        reg = <0x0502C000 0x1000>;
> +    };
> 
> -- 
> 2.45.2
> 

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

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

[Thomas Bonnefille]

On 7/5/24 5:01 PM, Conor Dooley wrote:
> On Fri, Jul 05, 2024 at 03:42:23PM +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     | 63 ++++++++++++++++++++++
>>   1 file changed, 63 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..31bd8ac6dfa5
>> --- /dev/null
>> +++ b/Documentation/devicetree/bindings/iio/adc/sophgo,cv18xx-saradc.yaml
>> @@ -0,0 +1,63 @@
>> +# 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:
>> +    oneOf:
>> +      - items:
>> +          - enum:
>> +              - sophgo,cv1800b-saradc
>> +          - const: sophgo,cv18xx-saradc
> 
> I don't think the fallback here makes sense. If there's other devices
> with a compatible programming model added later, we can fall back to the
> cv1800b.
> 

Ok I'll do that, I wasn't sure if it was a good practice to fallback on 
another SoC specific compatible.

>> +
>> +  reg:
>> +    maxItems: 1
>> +
>> +  interrupts:
>> +    maxItems: 1
>> +
>> +  clocks:
>> +    description:
>> +      SARADC will use the presence of this clock to determine if the controller
>> +      needs to be explicitly clocked by it (Active domain) or if it is part of
>> +      the No-Die Domain, along with the RTC, which does not require explicit
>> +      clocking.
> 
> What does "explicit clocking" mean? Is it clocked directly (or via
> dividers) by a clock on the board or another source?
> 

It means that, if a clock is provided, the driver will work in "Active 
Domain" and will use the clock generator of the SoC to get the right 
clock signal.

However if no clock is provided, the controller will work in "No-Die" 
domain (Always On) and use the RTCSYS subsystem to get its clock signal.

Indeed "explicitly clocked" may not be the right word to describe that, 
maybe some thing like that is better :

"SARADC will use the presence of this clock to determine if the 
controller needs to use the clock generator to get its clock signal 
(Active domain) or if it is part of the No-Die Domain, along with the 
RTC, and does not require the clock generator."

Regards,
Thomas

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

[kernel test robot]

Hi Thomas,

kernel test robot noticed the following build warnings:

[auto build test WARNING on d20f6b3d747c36889b7ce75ee369182af3decb6b]

url:    https://github.com/intel-lab-lkp/linux/commits/Thomas-Bonnefille/dt-bindings-iio-adc-sophgo-cv18xx-saradc-yaml-Add-Sophgo-SARADC-binding-documentation/20240706-040736
base:   d20f6b3d747c36889b7ce75ee369182af3decb6b
patch link:    https://lore.kernel.org/r/20240705-sg2002-adc-v2-2-83428c20a9b2%40bootlin.com
patch subject: [PATCH v2 2/3] iio: adc: sophgo-saradc: Add driver for Sophgo SARADC
config: x86_64-allyesconfig (https://download.01.org/0day-ci/archive/20240706/202407061311.ZEmwMY8m-lkp@intel.com/config)
compiler: clang version 18.1.5 (https://github.com/llvm/llvm-project 617a15a9eac96088ae5e9134248d8236e34b91b1)
reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20240706/202407061311.ZEmwMY8m-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/202407061311.ZEmwMY8m-lkp@intel.com/

All warnings (new ones prefixed by >>):

>> drivers/iio/adc/sophgo-cv18xx-adc.c:85:3: warning: label followed by a declaration is a C23 extension [-Wc23-extensions]
      85 |                 struct cv18xx_adc *saradc = iio_priv(indio_dev);
         |                 ^
   1 warning generated.


vim +85 drivers/iio/adc/sophgo-cv18xx-adc.c

    78	
    79	static int cv18xx_adc_read_raw(struct iio_dev *indio_dev,
    80					  struct iio_chan_spec const *chan,
    81					  int *val, int *val2, long mask)
    82	{
    83		switch (mask) {
    84		case IIO_CHAN_INFO_RAW:
  > 85			struct cv18xx_adc *saradc = iio_priv(indio_dev);
    86			u32 sample;
    87			int ret;
    88	
    89			scoped_guard(mutex, &saradc->lock) {
    90				cv18xx_adc_start_measurement(saradc, chan->scan_index);
    91				ret = cv18xx_adc_wait(saradc);
    92				if (ret < 0)
    93					return ret;
    94	
    95				sample = readl(saradc->regs + CV18XX_ADC_CH_RESULT_REG(chan->scan_index));
    96			}
    97			if (!(sample & CV18XX_ADC_CH_VALID))
    98				return -ENODATA;
    99	
   100			*val = sample & CV18XX_ADC_CH_RESULT;
   101			return IIO_VAL_INT;
   102		case IIO_CHAN_INFO_SCALE:
   103			*val = 3300;
   104			*val2 = 12;
   105			return IIO_VAL_FRACTIONAL_LOG2;
   106		default:
   107			return -EINVAL;
   108		}
   109	}
   110	

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

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

[Jonathan Cameron]

On Sat, 6 Jul 2024 13:16:36 +0800
kernel test robot <lkp@intel.com> wrote:

> Hi Thomas,
> 
> kernel test robot noticed the following build warnings:
> 
> [auto build test WARNING on d20f6b3d747c36889b7ce75ee369182af3decb6b]
> 
> url:    https://github.com/intel-lab-lkp/linux/commits/Thomas-Bonnefille/dt-bindings-iio-adc-sophgo-cv18xx-saradc-yaml-Add-Sophgo-SARADC-binding-documentation/20240706-040736
> base:   d20f6b3d747c36889b7ce75ee369182af3decb6b
> patch link:    https://lore.kernel.org/r/20240705-sg2002-adc-v2-2-83428c20a9b2%40bootlin.com
> patch subject: [PATCH v2 2/3] iio: adc: sophgo-saradc: Add driver for Sophgo SARADC
> config: x86_64-allyesconfig (https://download.01.org/0day-ci/archive/20240706/202407061311.ZEmwMY8m-lkp@intel.com/config)
> compiler: clang version 18.1.5 (https://github.com/llvm/llvm-project 617a15a9eac96088ae5e9134248d8236e34b91b1)
> reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20240706/202407061311.ZEmwMY8m-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/202407061311.ZEmwMY8m-lkp@intel.com/
> 
> All warnings (new ones prefixed by >>):
> 
> >> drivers/iio/adc/sophgo-cv18xx-adc.c:85:3: warning: label followed by a declaration is a C23 extension [-Wc23-extensions]  
>       85 |                 struct cv18xx_adc *saradc = iio_priv(indio_dev);
>          |                 ^
>    1 warning generated.
> 
> 
> vim +85 drivers/iio/adc/sophgo-cv18xx-adc.c
> 
>     78	
>     79	static int cv18xx_adc_read_raw(struct iio_dev *indio_dev,
>     80					  struct iio_chan_spec const *chan,
>     81					  int *val, int *val2, long mask)
>     82	{
>     83		switch (mask) {
>     84		case IIO_CHAN_INFO_RAW:

I guess you figured this out but if not.. Key is you need to define a scope so
			{
>   > 85			struct cv18xx_adc *saradc = iio_priv(indio_dev);  
>     86			u32 sample;
>     87			int ret;
>     88	
>     89			scoped_guard(mutex, &saradc->lock) {
>     90				cv18xx_adc_start_measurement(saradc, chan->scan_index);
>     91				ret = cv18xx_adc_wait(saradc);
>     92				if (ret < 0)
>     93					return ret;
>     94	
>     95				sample = readl(saradc->regs + CV18XX_ADC_CH_RESULT_REG(chan->scan_index));
>     96			}
>     97			if (!(sample & CV18XX_ADC_CH_VALID))
>     98				return -ENODATA;
>     99	
>    100			*val = sample & CV18XX_ADC_CH_RESULT;
>    101			return IIO_VAL_INT;

			}

>    102		case IIO_CHAN_INFO_SCALE:
>    103			*val = 3300;
>    104			*val2 = 12;
>    105			return IIO_VAL_FRACTIONAL_LOG2;
>    106		default:
>    107			return -EINVAL;
>    108		}
>    109	}
>    110	
> 

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

[Jonathan Cameron]

On Fri, 05 Jul 2024 15:42:24 +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>
Some more minor feedback inline.

Thanks,

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..dd1188b1923e
> --- /dev/null
> +++ b/drivers/iio/adc/sophgo-cv18xx-adc.c
> @@ -0,0 +1,195 @@
> +// 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/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)+4)
BIT((x) + 4)

> +#define CV18XX_ADC_STATUS_REG			0x08
> +#define		CV18XX_ADC_BUSY			BIT(0)
> +#define CV18XX_ADC_CYC_SET_REG			0x0C
> +#define		CV18XX_ADC_DEF_CYC_SETTINGS	0xF1F0F

I'd prefer to see that broken up into fields if we have any info
on what they are.

> +#define CV18XX_ADC_CH_RESULT_REG(x)		(0x10+4*(x))
(0x10 + 4 * (x))
code style also applies in macros.

> +#define		CV18XX_ADC_CH_RESULT		0xfff

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_STA_REG			0x28
>
> +
> +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;
> +		int ret;
> +
> +		scoped_guard(mutex, &saradc->lock) {
> +			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 (!(readl(saradc->regs + CV18XX_ADC_INTR_STA_REG) & BIT(0)))

Add a define for that BIT(0) and use FIELD_GET() to extract it.


> +		return IRQ_NONE;
> +
> +	writel(1, saradc->regs + CV18XX_ADC_INTR_CLR_REG);

Add a define for the 1 here (I guess it's BIT(0)?)
as well to show what is logically being cleared rather than simply
the register value.

> +	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;
> +	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;
> +

One blank line is almost always enough for readability and if you use too many
we get less code on the screen and hence it hurts readability a little.

> +
> +	if (IS_ERR(devm_clk_get_optional_enabled(&pdev->dev, NULL)))
> +		dev_dbg(&pdev->dev, "Can't get clock from device tree, using No-Die domain");

Failure to get a clock is an error and you should exit with a suitable
dev_err_probe().
Getting a NULL answer from this call reflects that one wasn't provided
and your handling here would be appropriate for that.

> +
> +	saradc->regs = devm_platform_ioremap_resource(pdev, 0);
> +	if (IS_ERR(saradc->regs)) {
> +		ret = PTR_ERR(saradc->regs);
> +		return ret;

		return PTR_ERR(saradc->regs);
and drop the brackets.

> +	}
> +
> +	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);

Where it isn't limited by line length, preferred style is to align to
just after the opening bracket. e.g.

		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);
> +

Drop this blank line.

> +	}

One blank here is plenty.

> +
> +
> +	mutex_init(&saradc->lock);

Whilst mutex cleanup is of dubious benefit as only helpful if doing particularly forms
of mutex debugging and looking for use after free etc, we do finally have devm_mutex_init()
to make it easy so for new code I'm going to encourage it's use but not insist
on it yet...

	ret = devm_mutex_init(&saradc->lock);
	if (ret)
		return;

> +	platform_set_drvdata(pdev, indio_dev);
> +	writel(CV18XX_ADC_DEF_CYC_SETTINGS, saradc->regs + CV18XX_ADC_CYC_SET_REG);
> +	ret = devm_iio_device_register(&pdev->dev, indio_dev);
> +	if (ret)
> +		return ret;
> +
> +	return 0;

	return devm_iio_device_register().

> +}

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

[Conor Dooley]

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

On Fri, Jul 05, 2024 at 05:24:19PM +0200, Thomas Bonnefille wrote:
> 
> 
> On 7/5/24 5:01 PM, Conor Dooley wrote:
> > On Fri, Jul 05, 2024 at 03:42:23PM +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     | 63 ++++++++++++++++++++++
> > >   1 file changed, 63 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..31bd8ac6dfa5
> > > --- /dev/null
> > > +++ b/Documentation/devicetree/bindings/iio/adc/sophgo,cv18xx-saradc.yaml
> > > @@ -0,0 +1,63 @@
> > > +# 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:
> > > +    oneOf:
> > > +      - items:
> > > +          - enum:
> > > +              - sophgo,cv1800b-saradc
> > > +          - const: sophgo,cv18xx-saradc
> > 
> > I don't think the fallback here makes sense. If there's other devices
> > with a compatible programming model added later, we can fall back to the
> > cv1800b.
> > 
> 
> Ok I'll do that, I wasn't sure if it was a good practice to fallback on
> another SoC specific compatible.
> 
> > > +
> > > +  reg:
> > > +    maxItems: 1
> > > +
> > > +  interrupts:
> > > +    maxItems: 1
> > > +
> > > +  clocks:
> > > +    description:
> > > +      SARADC will use the presence of this clock to determine if the controller
> > > +      needs to be explicitly clocked by it (Active domain) or if it is part of
> > > +      the No-Die Domain, along with the RTC, which does not require explicit
> > > +      clocking.
> > 
> > What does "explicit clocking" mean? Is it clocked directly (or via
> > dividers) by a clock on the board or another source?
> > 
> 
> It means that, if a clock is provided, the driver will work in "Active
> Domain" and will use the clock generator of the SoC to get the right clock
> signal.
> 
> However if no clock is provided, the controller will work in "No-Die" domain
> (Always On) and use the RTCSYS subsystem to get its clock signal.

So it does have a clock, but provided by a different provider. I don't
really understand why that would "excuse" it from having a clocks
property, with the RTCSYS as the provider.

> 
> Indeed "explicitly clocked" may not be the right word to describe that,
> maybe some thing like that is better :
> 
> "SARADC will use the presence of this clock to determine if the controller
> needs to use the clock generator to get its clock signal (Active domain) or
> if it is part of the No-Die Domain, along with the RTC, and does not require
> the clock generator."



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

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

[Miquel Raynal]

Hi Conor,

> > > > +properties:
> > > > +  compatible:
> > > > +    oneOf:
> > > > +      - items:
> > > > +          - enum:
> > > > +              - sophgo,cv1800b-saradc
> > > > +          - const: sophgo,cv18xx-saradc  
> > > 
> > > I don't think the fallback here makes sense. If there's other devices
> > > with a compatible programming model added later, we can fall back to the
> > > cv1800b.

I'm sorry but isn't this slightly disagreeing with the "writing
bindings" doc pointed in v1? It says,

* DO use fallback compatibles when devices are the same as or a subset
  of prior implementations.

I believe we fall in the "devices are the same" category, so I would
have myself wrote a similar binding here with a compatible matching
them all, plus a hardware-implementation-specific compatible as well;
just in case.

Thanks,
Miquèl

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

[Krzysztof Kozlowski]

On 08/07/2024 08:30, Miquel Raynal wrote:
> Hi Conor,
> 
>>>>> +properties:
>>>>> +  compatible:
>>>>> +    oneOf:
>>>>> +      - items:
>>>>> +          - enum:
>>>>> +              - sophgo,cv1800b-saradc
>>>>> +          - const: sophgo,cv18xx-saradc  
>>>>
>>>> I don't think the fallback here makes sense. If there's other devices
>>>> with a compatible programming model added later, we can fall back to the
>>>> cv1800b.
> 
> I'm sorry but isn't this slightly disagreeing with the "writing
> bindings" doc pointed in v1? It says,
> 
> * DO use fallback compatibles when devices are the same as or a subset
>   of prior implementations.
> 
> I believe we fall in the "devices are the same" category, so I would
> have myself wrote a similar binding here with a compatible matching
> them all, plus a hardware-implementation-specific compatible as well;
> just in case.

Fallback from one model to another. There is no "another" model here,
but wildcard. There is no such device as cv18xx, right?

Best regards,
Krzysztof

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

[Miquel Raynal]

Hi Krzysztof,

krzk@kernel.org wrote on Mon, 8 Jul 2024 09:33:04 +0200:

> On 08/07/2024 08:30, Miquel Raynal wrote:
> > Hi Conor,
> >   
> >>>>> +properties:
> >>>>> +  compatible:
> >>>>> +    oneOf:
> >>>>> +      - items:
> >>>>> +          - enum:
> >>>>> +              - sophgo,cv1800b-saradc
> >>>>> +          - const: sophgo,cv18xx-saradc    
> >>>>
> >>>> I don't think the fallback here makes sense. If there's other devices
> >>>> with a compatible programming model added later, we can fall back to the
> >>>> cv1800b.  
> > 
> > I'm sorry but isn't this slightly disagreeing with the "writing
> > bindings" doc pointed in v1? It says,
> > 
> > * DO use fallback compatibles when devices are the same as or a subset
> >   of prior implementations.
> > 
> > I believe we fall in the "devices are the same" category, so I would
> > have myself wrote a similar binding here with a compatible matching
> > them all, plus a hardware-implementation-specific compatible as well;
> > just in case.  
> 
> Fallback from one model to another. There is no "another" model here,
> but wildcard. There is no such device as cv18xx, right?

No there is not. But I don't think there is a "base" model either.
Just multiple SoCs named cv18<something> with apparently the same ADC.

So actually I guess the discussion here is about the wildcard
compatible. It feels strange to me to have no generic compatible either
with a wildcard or with a "base" implementation (because there is
probably none). So I guess the solution here is to just list a single
specific compatible in the end.

Thanks,
Miquèl

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

[Thomas Bonnefille]

On 7/6/24 2:42 PM, Conor Dooley wrote:
> On Fri, Jul 05, 2024 at 05:24:19PM +0200, Thomas Bonnefille wrote:
>>
>>
>> On 7/5/24 5:01 PM, Conor Dooley wrote:
>>> On Fri, Jul 05, 2024 at 03:42:23PM +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     | 63 ++++++++++++++++++++++
>>>>    1 file changed, 63 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..31bd8ac6dfa5
>>>> --- /dev/null
>>>> +++ b/Documentation/devicetree/bindings/iio/adc/sophgo,cv18xx-saradc.yaml
>>>> @@ -0,0 +1,63 @@
>>>> +# 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:
>>>> +    oneOf:
>>>> +      - items:
>>>> +          - enum:
>>>> +              - sophgo,cv1800b-saradc
>>>> +          - const: sophgo,cv18xx-saradc
>>>
>>> I don't think the fallback here makes sense. If there's other devices
>>> with a compatible programming model added later, we can fall back to the
>>> cv1800b.
>>>
>>
>> Ok I'll do that, I wasn't sure if it was a good practice to fallback on
>> another SoC specific compatible.
>>
>>>> +
>>>> +  reg:
>>>> +    maxItems: 1
>>>> +
>>>> +  interrupts:
>>>> +    maxItems: 1
>>>> +
>>>> +  clocks:
>>>> +    description:
>>>> +      SARADC will use the presence of this clock to determine if the controller
>>>> +      needs to be explicitly clocked by it (Active domain) or if it is part of
>>>> +      the No-Die Domain, along with the RTC, which does not require explicit
>>>> +      clocking.
>>>
>>> What does "explicit clocking" mean? Is it clocked directly (or via
>>> dividers) by a clock on the board or another source?
>>>
>>
>> It means that, if a clock is provided, the driver will work in "Active
>> Domain" and will use the clock generator of the SoC to get the right clock
>> signal.
>>
>> However if no clock is provided, the controller will work in "No-Die" domain
>> (Always On) and use the RTCSYS subsystem to get its clock signal.
> 
> So it does have a clock, but provided by a different provider. I don't
> really understand why that would "excuse" it from having a clocks
> property, with the RTCSYS as the provider.

By digging into the datasheet, I discovered that there might be a way to 
use a valid clock as the input of the No-Die domain ADC. I would like to 
ask Inochi about this, as he wrote the clock driver for this SoC.

As I understand it, the SARADC working in the No-Die domain is fed, like 
every other IP in the No-Die domain, by the CLK_SRC_RTC_SYS_0. This 
clock source is either a division of the main oscillator (referred to as 
osc_parents in the clock driver) or "xtal," which is an external 
oscillator. Am I right? What is the role of CLK_RTC_24M?

If I'm correct, this description isn't needed anymore in the bindings, 
and the device tree node for the SARADC in the No-Die domain will need 
this line:

+                       clocks = <&clk CLK_SRC_RTC_SYS_0>;

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

[Jonathan Cameron]

On Mon, 8 Jul 2024 14:23:44 +0200
Miquel Raynal <miquel.raynal@bootlin.com> wrote:

> Hi Krzysztof,
> 
> krzk@kernel.org wrote on Mon, 8 Jul 2024 09:33:04 +0200:
> 
> > On 08/07/2024 08:30, Miquel Raynal wrote:  
> > > Hi Conor,
> > >     
> > >>>>> +properties:
> > >>>>> +  compatible:
> > >>>>> +    oneOf:
> > >>>>> +      - items:
> > >>>>> +          - enum:
> > >>>>> +              - sophgo,cv1800b-saradc
> > >>>>> +          - const: sophgo,cv18xx-saradc      
> > >>>>
> > >>>> I don't think the fallback here makes sense. If there's other devices
> > >>>> with a compatible programming model added later, we can fall back to the
> > >>>> cv1800b.    
> > > 
> > > I'm sorry but isn't this slightly disagreeing with the "writing
> > > bindings" doc pointed in v1? It says,
> > > 
> > > * DO use fallback compatibles when devices are the same as or a subset
> > >   of prior implementations.
> > > 
> > > I believe we fall in the "devices are the same" category, so I would
> > > have myself wrote a similar binding here with a compatible matching
> > > them all, plus a hardware-implementation-specific compatible as well;
> > > just in case.    
> > 
> > Fallback from one model to another. There is no "another" model here,
> > but wildcard. There is no such device as cv18xx, right?  
> 
> No there is not. But I don't think there is a "base" model either.
> Just multiple SoCs named cv18<something> with apparently the same ADC.
> 
> So actually I guess the discussion here is about the wildcard
> compatible. It feels strange to me to have no generic compatible either
> with a wildcard or with a "base" implementation (because there is
> probably none). So I guess the solution here is to just list a single
> specific compatible in the end.

It comes from long experience of silicon vendors not being consistent
with part naming.  Far too often we've had a nice generic wild card
entry and along comes the vendor with a new part in the middle
of that range that is completely incompatible.  Then we end up with
people assuming the wildcard means it will work and a bunch of bug
reports.  Hence no wild cards, just define first supported part as your
'base' and go from there.

It's even more fun when a vendor driver papers over the differences
and so it 'works', but the upstream one doesn't.  In extreme case
because a different driver entirely is required.

So basically we don't trust silicon vendors :)
Speaking as someone who works for one - I think that's entirely
reasonable!!

Jonathan

> 
> Thanks,
> Miquèl
> 
> 

Re: [PATCH v2 0/3] Add SARADC support on Sophgo SoC

[Chen Wang]

Thank you Thomas for your contribution and welcome.

Just FYI, we are maintaining and tracking all PR status about Sophgo 
products here at https://github.com/sophgo/linux/wiki.

Regards,

Chen

On 2024/7/5 21:42, Thomas Bonnefille wrote:
> 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 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 SARADC binding documentation
>        iio: adc: sophgo-saradc: Add driver for Sophgo SARADC
>        riscv: dts: sophgo: Add SARADC configuration
>
>   .../bindings/iio/adc/sophgo,cv18xx-saradc.yaml     |  63 +++++++
>   arch/riscv/boot/dts/sophgo/cv1800b.dtsi            |   8 +
>   arch/riscv/boot/dts/sophgo/cv18xx.dtsi             |  14 ++
>   drivers/iio/adc/Kconfig                            |  10 ++
>   drivers/iio/adc/Makefile                           |   1 +
>   drivers/iio/adc/sophgo-cv18xx-adc.c                | 195 +++++++++++++++++++++
>   6 files changed, 291 insertions(+)
> ---
> base-commit: d20f6b3d747c36889b7ce75ee369182af3decb6b
> change-id: 20240527-sg2002-adc-924b862cd3f2
>
> Best regards,

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

[Miquel Raynal]

Hi Jonathan,

> > > > * DO use fallback compatibles when devices are the same as or a subset
> > > >   of prior implementations.
> > > > 
> > > > I believe we fall in the "devices are the same" category, so I would
> > > > have myself wrote a similar binding here with a compatible matching
> > > > them all, plus a hardware-implementation-specific compatible as well;
> > > > just in case.    
> > > 
> > > Fallback from one model to another. There is no "another" model here,
> > > but wildcard. There is no such device as cv18xx, right?  
> > 
> > No there is not. But I don't think there is a "base" model either.
> > Just multiple SoCs named cv18<something> with apparently the same ADC.
> > 
> > So actually I guess the discussion here is about the wildcard
> > compatible. It feels strange to me to have no generic compatible either
> > with a wildcard or with a "base" implementation (because there is
> > probably none). So I guess the solution here is to just list a single
> > specific compatible in the end.
> 
> It comes from long experience of silicon vendors not being consistent
> with part naming.

Oh, agreed :-)

>  Far too often we've had a nice generic wild card
> entry and along comes the vendor with a new part in the middle
> of that range that is completely incompatible.  Then we end up with
> people assuming the wildcard means it will work and a bunch of bug
> reports.  Hence no wild cards, just define first supported part as your
> 'base' and go from there.

I see what you mean. I must admit I'm not a big fan of naming
compatibles (and drivers) after a working base rather than a good
enough wildcard, but I do understand your point and kind of agree with
it actually.

> It's even more fun when a vendor driver papers over the differences
> and so it 'works', but the upstream one doesn't.  In extreme case
> because a different driver entirely is required.
> 
> So basically we don't trust silicon vendors :)
> Speaking as someone who works for one - I think that's entirely
> reasonable!!

Haha <3

Thanks (once again) for your valuable inputs!
Miquèl