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

[PATCH 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>
---
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     |  51 +++++
 MAINTAINERS                                        |   6 +
 arch/riscv/boot/dts/sophgo/cv18xx.dtsi             |  14 ++
 drivers/iio/adc/Kconfig                            |   9 +
 drivers/iio/adc/Makefile                           |   1 +
 drivers/iio/adc/sophgo-adc.c                       | 223 +++++++++++++++++++++
 6 files changed, 304 insertions(+)
---
base-commit: be18997d95c888e7a432d46045644f396286775b
change-id: 20240527-sg2002-adc-924b862cd3f2

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

[PATCH 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     | 51 ++++++++++++++++++++++
 MAINTAINERS                                        |  5 +++
 2 files changed, 56 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..21fd5dc8e24e
--- /dev/null
+++ b/Documentation/devicetree/bindings/iio/adc/sophgo,cv18xx-saradc.yaml
@@ -0,0 +1,51 @@
+# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/iio/adc/sophgo,cv18xx-adc.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: Sophgo 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,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
+
+examples:
+  - |
+    # ADC in the Active domain
+    adc@30f0000 {
+      compatible = "sophgo,cv18xx-saradc";
+      clocks = <&clk CLK_SARADC>;
+      interrupts = <100 IRQ_TYPE_LEVEL_HIGH>;
+      reg = <0x030F0000 0x1000>;
+    };
+  - |
+    # ADC in the No-Die domain
+    adc@502c000 {
+      compatible = "sophgo,cv18xx-saradc";
+      reg = <0x0502C000 0x1000>;
+    };
diff --git a/MAINTAINERS b/MAINTAINERS
index aacccb376c28..dc87898c518c 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -20901,6 +20901,11 @@ S:	Maintained
 N:	sophgo
 K:	sophgo
 
+SOPHGO SARADC DEVICE DRIVER
+M:	Thomas Bonnefille <thomas.bonnefille@bootlin.com>
+S:	Maintained
+F:	Documentation/devicetree/bindings/iio/adc/sophgo,cv18xx-saradc.yaml
+
 SOUND
 M:	Jaroslav Kysela <perex@perex.cz>
 M:	Takashi Iwai <tiwai@suse.com>

-- 
2.45.2

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

[Rob Herring (Arm)]

On Tue, 02 Jul 2024 13:52:21 +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     | 51 ++++++++++++++++++++++
>  MAINTAINERS                                        |  5 +++
>  2 files changed, 56 insertions(+)
> 

My bot found errors running 'make dt_binding_check' on your patch:

yamllint warnings/errors:

dtschema/dtc warnings/errors:
/builds/robherring/dt-review-ci/linux/Documentation/devicetree/bindings/iio/adc/sophgo,cv18xx-saradc.yaml: 'oneOf' conditional failed, one must be fixed:
	'unevaluatedProperties' is a required property
	'additionalProperties' is a required property
	hint: Either unevaluatedProperties or additionalProperties must be present
	from schema $id: http://devicetree.org/meta-schemas/core.yaml#
/builds/robherring/dt-review-ci/linux/Documentation/devicetree/bindings/iio/adc/sophgo,cv18xx-saradc.yaml: $id: Cannot determine base path from $id, relative path/filename doesn't match actual path or filename
 	 $id: http://devicetree.org/schemas/iio/adc/sophgo,cv18xx-adc.yaml
 	file: /builds/robherring/dt-review-ci/linux/Documentation/devicetree/bindings/iio/adc/sophgo,cv18xx-saradc.yaml
Error: Documentation/devicetree/bindings/iio/adc/sophgo,cv18xx-saradc.example.dts:24.11-12 syntax error
FATAL ERROR: Unable to parse input tree
make[2]: *** [scripts/Makefile.lib:427: Documentation/devicetree/bindings/iio/adc/sophgo,cv18xx-saradc.example.dtb] Error 1
make[2]: *** Waiting for unfinished jobs....
make[1]: *** [/builds/robherring/dt-review-ci/linux/Makefile:1430: dt_binding_check] Error 2
make: *** [Makefile:240: __sub-make] Error 2

doc reference errors (make refcheckdocs):

See https://patchwork.ozlabs.org/project/devicetree-bindings/patch/20240702-sg2002-adc-v1-1-ac66e076a756@bootlin.com

The base for the series is generally the latest rc1. A different dependency
should be noted in *this* patch.

If you already ran 'make dt_binding_check' and didn't see the above
error(s), then make sure 'yamllint' is installed and dt-schema is up to
date:

pip3 install dtschema --upgrade

Please check and re-submit after running the above command yourself. Note
that DT_SCHEMA_FILES can be set to your schema file to speed up checking
your schema. However, it must be unset to test all examples with your schema.

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

[Chen Wang]

On 2024/7/2 19:52, 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     | 51 ++++++++++++++++++++++
>   MAINTAINERS                                        |  5 +++
>   2 files changed, 56 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..21fd5dc8e24e
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/iio/adc/sophgo,cv18xx-saradc.yaml
> @@ -0,0 +1,51 @@
> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/iio/adc/sophgo,cv18xx-adc.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: Sophgo 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,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
> +
> +examples:
> +  - |
> +    # ADC in the Active domain
> +    adc@30f0000 {
> +      compatible = "sophgo,cv18xx-saradc";
> +      clocks = <&clk CLK_SARADC>;
> +      interrupts = <100 IRQ_TYPE_LEVEL_HIGH>;
> +      reg = <0x030F0000 0x1000>;
> +    };
> +  - |
> +    # ADC in the No-Die domain
> +    adc@502c000 {
> +      compatible = "sophgo,cv18xx-saradc";
> +      reg = <0x0502C000 0x1000>;
> +    };
> diff --git a/MAINTAINERS b/MAINTAINERS
> index aacccb376c28..dc87898c518c 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -20901,6 +20901,11 @@ S:	Maintained
>   N:	sophgo
>   K:	sophgo
>   
> +SOPHGO SARADC DEVICE DRIVER
> +M:	Thomas Bonnefille <thomas.bonnefille@bootlin.com>
> +S:	Maintained
> +F:	Documentation/devicetree/bindings/iio/adc/sophgo,cv18xx-saradc.yaml
> +
Drop this please, you have mentioned your maintainers info in the 
binding yaml.
>   SOUND
>   M:	Jaroslav Kysela <perex@perex.cz>
>   M:	Takashi Iwai <tiwai@suse.com>
>

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

[Krzysztof Kozlowski]

On 02/07/2024 13:52, 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     | 51 ++++++++++++++++++++++
>  MAINTAINERS                                        |  5 +++
>  2 files changed, 56 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..21fd5dc8e24e
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/iio/adc/sophgo,cv18xx-saradc.yaml
> @@ -0,0 +1,51 @@
> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/iio/adc/sophgo,cv18xx-adc.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: Sophgo 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,cv18xx-saradc

Except that this was never tested... wild-cards are generally not allowed.

Best regards,
Krzysztof

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

[Thomas Bonnefille]

Hello Krzysztof,
Thank you for your feedback

On 7/3/24 7:08 AM, Krzysztof Kozlowski wrote:
> On 02/07/2024 13:52, 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     | 51 ++++++++++++++++++++++
>>   MAINTAINERS                                        |  5 +++
>>   2 files changed, 56 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..21fd5dc8e24e
>> --- /dev/null
>> +++ b/Documentation/devicetree/bindings/iio/adc/sophgo,cv18xx-saradc.yaml
>> @@ -0,0 +1,51 @@
>> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
>> +%YAML 1.2
>> +---
>> +$id: http://devicetree.org/schemas/iio/adc/sophgo,cv18xx-adc.yaml#
>> +$schema: http://devicetree.org/meta-schemas/core.yaml#
>> +
>> +title: Sophgo 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,cv18xx-saradc
> 
> Except that this was never tested... wild-cards are generally not allowed.
> 

I realized I made a mistake when using the "make dt_binding_check" 
command, which led to some errors in this dt_binding. I have now 
addressed all of them, but I'm not sure I understand your comment.

I don't see any wildcards in the YAML file. Could you please provide 
more details on what you expect?

Best regards,
Thomas

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

[Krzysztof Kozlowski]

On 04/07/2024 10:35, Thomas Bonnefille wrote:
> Hello Krzysztof,
> Thank you for your feedback
> 
> On 7/3/24 7:08 AM, Krzysztof Kozlowski wrote:
>> On 02/07/2024 13:52, 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     | 51 ++++++++++++++++++++++
>>>   MAINTAINERS                                        |  5 +++
>>>   2 files changed, 56 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..21fd5dc8e24e
>>> --- /dev/null
>>> +++ b/Documentation/devicetree/bindings/iio/adc/sophgo,cv18xx-saradc.yaml
>>> @@ -0,0 +1,51 @@
>>> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
>>> +%YAML 1.2
>>> +---
>>> +$id: http://devicetree.org/schemas/iio/adc/sophgo,cv18xx-adc.yaml#
>>> +$schema: http://devicetree.org/meta-schemas/core.yaml#
>>> +
>>> +title: Sophgo 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,cv18xx-saradc
>>
>> Except that this was never tested... wild-cards are generally not allowed.
>>
> 
> I realized I made a mistake when using the "make dt_binding_check" 
> command, which led to some errors in this dt_binding. I have now 
> addressed all of them, but I'm not sure I understand your comment.
> 
> I don't see any wildcards in the YAML file. Could you please provide 
> more details on what you expect?

What is "xx"? Sounds like wildcard. I do not see such SoC compatible.

Best regards,
Krzysztof

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

[Thomas Bonnefille]

On 7/4/24 10:39 AM, Krzysztof Kozlowski wrote:
> On 04/07/2024 10:35, Thomas Bonnefille wrote:
>> Hello Krzysztof,
>> Thank you for your feedback
>>
>> On 7/3/24 7:08 AM, Krzysztof Kozlowski wrote:
>>> On 02/07/2024 13:52, 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     | 51 ++++++++++++++++++++++
>>>>    MAINTAINERS                                        |  5 +++
>>>>    2 files changed, 56 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..21fd5dc8e24e
>>>> --- /dev/null
>>>> +++ b/Documentation/devicetree/bindings/iio/adc/sophgo,cv18xx-saradc.yaml
>>>> @@ -0,0 +1,51 @@
>>>> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
>>>> +%YAML 1.2
>>>> +---
>>>> +$id: http://devicetree.org/schemas/iio/adc/sophgo,cv18xx-adc.yaml#
>>>> +$schema: http://devicetree.org/meta-schemas/core.yaml#
>>>> +
>>>> +title: Sophgo 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,cv18xx-saradc
>>>
>>> Except that this was never tested... wild-cards are generally not allowed.
>>>
>>
>> I realized I made a mistake when using the "make dt_binding_check"
>> command, which led to some errors in this dt_binding. I have now
>> addressed all of them, but I'm not sure I understand your comment.
>>
>> I don't see any wildcards in the YAML file. Could you please provide
>> more details on what you expect?
> 
> What is "xx"? Sounds like wildcard. I do not see such SoC compatible.
> 

The point of cv18xx is to have a single compatible for all SoCs of the 
cv18xx series from Sophgo (cv1800b, cv1812h but also sg2000 and sg2002). 
I used that as a reflection of the cv18xx.dtsi used for all those SoCs.

Should I create multiple compatible for all the SoCs of a same brand ?

> Best regards,
> Krzysztof
> 

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

[Inochi Amaoto]

On Thu, Jul 04, 2024 at 10:52:05AM GMT, Thomas Bonnefille wrote:
> 
> 
> On 7/4/24 10:39 AM, Krzysztof Kozlowski wrote:
> > On 04/07/2024 10:35, Thomas Bonnefille wrote:
> > > Hello Krzysztof,
> > > Thank you for your feedback
> > > 
> > > On 7/3/24 7:08 AM, Krzysztof Kozlowski wrote:
> > > > On 02/07/2024 13:52, 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     | 51 ++++++++++++++++++++++
> > > > >    MAINTAINERS                                        |  5 +++
> > > > >    2 files changed, 56 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..21fd5dc8e24e
> > > > > --- /dev/null
> > > > > +++ b/Documentation/devicetree/bindings/iio/adc/sophgo,cv18xx-saradc.yaml
> > > > > @@ -0,0 +1,51 @@
> > > > > +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> > > > > +%YAML 1.2
> > > > > +---
> > > > > +$id: http://devicetree.org/schemas/iio/adc/sophgo,cv18xx-adc.yaml#
> > > > > +$schema: http://devicetree.org/meta-schemas/core.yaml#
> > > > > +
> > > > > +title: Sophgo 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,cv18xx-saradc
> > > > 
> > > > Except that this was never tested... wild-cards are generally not allowed.
> > > > 
> > > 
> > > I realized I made a mistake when using the "make dt_binding_check"
> > > command, which led to some errors in this dt_binding. I have now
> > > addressed all of them, but I'm not sure I understand your comment.
> > > 
> > > I don't see any wildcards in the YAML file. Could you please provide
> > > more details on what you expect?
> > 
> > What is "xx"? Sounds like wildcard. I do not see such SoC compatible.
> > 
> 
> The point of cv18xx is to have a single compatible for all SoCs of the
> cv18xx series from Sophgo (cv1800b, cv1812h but also sg2000 and sg2002). I
> used that as a reflection of the cv18xx.dtsi used for all those SoCs.
> 
> Should I create multiple compatible for all the SoCs of a same brand ?
> 

You can use cv1800-xxx as a base compatible. 
Add new compatible when other SoCs have different features.

See the tutorial I have wrote:
https://github.com/sophgo/linux/wiki/Mainline-patch-How%E2%80%90to

Regards,
Inochi

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

[Krzysztof Kozlowski]

On 04/07/2024 10:52, Thomas Bonnefille wrote:
> 
> 
> On 7/4/24 10:39 AM, Krzysztof Kozlowski wrote:
>> On 04/07/2024 10:35, Thomas Bonnefille wrote:
>>> Hello Krzysztof,
>>> Thank you for your feedback
>>>
>>> On 7/3/24 7:08 AM, Krzysztof Kozlowski wrote:
>>>> On 02/07/2024 13:52, 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     | 51 ++++++++++++++++++++++
>>>>>    MAINTAINERS                                        |  5 +++
>>>>>    2 files changed, 56 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..21fd5dc8e24e
>>>>> --- /dev/null
>>>>> +++ b/Documentation/devicetree/bindings/iio/adc/sophgo,cv18xx-saradc.yaml
>>>>> @@ -0,0 +1,51 @@
>>>>> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
>>>>> +%YAML 1.2
>>>>> +---
>>>>> +$id: http://devicetree.org/schemas/iio/adc/sophgo,cv18xx-adc.yaml#
>>>>> +$schema: http://devicetree.org/meta-schemas/core.yaml#
>>>>> +
>>>>> +title: Sophgo 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,cv18xx-saradc
>>>>
>>>> Except that this was never tested... wild-cards are generally not allowed.
>>>>
>>>
>>> I realized I made a mistake when using the "make dt_binding_check"
>>> command, which led to some errors in this dt_binding. I have now
>>> addressed all of them, but I'm not sure I understand your comment.
>>>
>>> I don't see any wildcards in the YAML file. Could you please provide
>>> more details on what you expect?
>>
>> What is "xx"? Sounds like wildcard. I do not see such SoC compatible.
>>
> 
> The point of cv18xx is to have a single compatible for all SoCs of the 
> cv18xx series from Sophgo (cv1800b, cv1812h but also sg2000 and sg2002). 
> I used that as a reflection of the cv18xx.dtsi used for all those SoCs.
> 
> Should I create multiple compatible for all the SoCs of a same brand ?

See writing bindings (and numerous presentation on this topic).

Best regards,
Krzysztof

[PATCH 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>
---
 MAINTAINERS                  |   1 +
 drivers/iio/adc/Kconfig      |   9 ++
 drivers/iio/adc/Makefile     |   1 +
 drivers/iio/adc/sophgo-adc.c | 223 +++++++++++++++++++++++++++++++++++++++++++
 4 files changed, 234 insertions(+)

diff --git a/MAINTAINERS b/MAINTAINERS
index dc87898c518c..fc74eeaf5678 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -20905,6 +20905,7 @@ SOPHGO SARADC DEVICE DRIVER
 M:	Thomas Bonnefille <thomas.bonnefille@bootlin.com>
 S:	Maintained
 F:	Documentation/devicetree/bindings/iio/adc/sophgo,cv18xx-saradc.yaml
+F:	drivers/iio/adc/sophgo-adc.c
 
 SOUND
 M:	Jaroslav Kysela <perex@perex.cz>
diff --git a/drivers/iio/adc/Kconfig b/drivers/iio/adc/Kconfig
index 8db68b80b391..826871a2e61a 100644
--- a/drivers/iio/adc/Kconfig
+++ b/drivers/iio/adc/Kconfig
@@ -1122,6 +1122,15 @@ config SC27XX_ADC
 	  This driver can also be built as a module. If so, the module
 	  will be called sc27xx_adc.
 
+config SOPHGO_ADC
+	tristate "Sophgo ADC"
+	depends on ARCH_SOPHGO || COMPILE_TEST
+	help
+	  Say yes here to build support for the ADC integrated in Sophgo 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..106a83d50d01 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_ADC) += sophgo-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-adc.c b/drivers/iio/adc/sophgo-adc.c
new file mode 100644
index 000000000000..a94d839d40ec
--- /dev/null
+++ b/drivers/iio/adc/sophgo-adc.c
@@ -0,0 +1,223 @@
+// SPDX-License-Identifier: GPL-2.0-or-later
+/*
+ *  Sophgo 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/module.h>
+#include <linux/mutex.h>
+#include <linux/of.h>
+#include <linux/platform_device.h>
+
+#define SOPHGO_SARADC_CTRL_REG			0x04
+#define		SOPHGO_SARADC_EN		BIT(0)
+#define		SOPHGO_SARADC_SEL(x)		BIT((x)+4)
+#define SOPHGO_SARADC_STATUS_REG		0x08
+#define		SOPHGO_SARADC_BUSY		BIT(0)
+#define SOPHGO_SARADC_CYC_SET_REG		0x0C
+#define		SOPHGO_SARADC_DEF_CYC_SETTINGS	0xF1F0F
+#define SOPHGO_SARADC_CH_RESULT_REG(x)		(0x10+4*(x))
+#define		SARADC_CH_RESULT(x)		((x) & 0xfff)
+#define		SARADC_CH_VALID(x)		((x) & BIT(15))
+#define SOPHGO_SARADC_INTR_EN_REG		0x20
+#define SOPHGO_SARADC_INTR_CLR_REG		0x24
+
+#define SOPHGO_SARADC_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,					\
+		.scan_type = {						\
+			.sign = 'u',					\
+			.realbits = 12,					\
+		},							\
+	}
+
+struct sophgo_saradc {
+	struct completion completion;
+	struct iio_info info;
+	void __iomem *regs;
+	struct mutex lock;
+	struct clk *clk;
+	int irq;
+};
+
+static const struct iio_chan_spec sophgo_channels[] = {
+	SOPHGO_SARADC_CHANNEL(1),
+	SOPHGO_SARADC_CHANNEL(2),
+	SOPHGO_SARADC_CHANNEL(3),
+};
+
+static void sophgo_saradc_start_measurement(struct sophgo_saradc *saradc,
+					    int channel)
+{
+	writel(0, saradc->regs + SOPHGO_SARADC_CTRL_REG);
+	writel(SOPHGO_SARADC_SEL(channel) | SOPHGO_SARADC_EN,
+	       saradc->regs + SOPHGO_SARADC_CTRL_REG);
+}
+
+static int sophgo_saradc_wait(struct sophgo_saradc *saradc)
+{
+	if (saradc->irq < 0) {
+		u32 reg;
+
+		return readl_poll_timeout(saradc->regs + SOPHGO_SARADC_STATUS_REG,
+					  reg, !(reg & SOPHGO_SARADC_BUSY),
+					  500, 1000000);
+	} else {
+		int ret;
+
+		ret = wait_for_completion_timeout(&saradc->completion,
+						  msecs_to_jiffies(1000)) > 0
+						  ? 0 : -ETIMEDOUT;
+		return ret;
+	}
+}
+
+static int sophgo_saradc_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 sophgo_saradc *saradc = iio_priv(indio_dev);
+		u32 sample;
+		int ret;
+
+		mutex_lock(&saradc->lock);
+		sophgo_saradc_start_measurement(saradc, chan->scan_index);
+		ret = sophgo_saradc_wait(saradc);
+		if (ret < 0) {
+			mutex_unlock(&saradc->lock);
+			return ret;
+		}
+
+		sample = readl(saradc->regs + SOPHGO_SARADC_CH_RESULT_REG(chan->scan_index));
+		mutex_unlock(&saradc->lock);
+
+		if (SARADC_CH_VALID(sample)) {
+			*val = SARADC_CH_RESULT(sample);
+			return IIO_VAL_INT;
+		}
+		return -ENODATA;
+	case IIO_CHAN_INFO_SCALE:
+		*val = 3300;
+		*val2 = 12;
+		return IIO_VAL_FRACTIONAL_LOG2;
+	default:
+		return -EINVAL;
+	}
+}
+
+static irqreturn_t sophgo_saradc_interrupt_handler(int irq, void *dev_id)
+{
+	struct sophgo_saradc *saradc = dev_id;
+
+	writel(1, saradc->regs + SOPHGO_SARADC_INTR_CLR_REG);
+	complete(&saradc->completion);
+	return IRQ_HANDLED;
+}
+
+
+static const struct of_device_id sophgo_saradc_match[] = {
+	{ .compatible = "sophgo,cv18xx-saradc", },
+	{ },
+};
+MODULE_DEVICE_TABLE(of, sophgo_saradc_match);
+
+static int sophgo_saradc_probe(struct platform_device *pdev)
+{
+	struct sophgo_saradc *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 SARADC";
+	indio_dev->info = &saradc->info;
+	indio_dev->modes = INDIO_DIRECT_MODE;
+	indio_dev->num_channels = 3;
+	indio_dev->channels = sophgo_channels;
+
+	saradc->clk = devm_clk_get(&pdev->dev, NULL);
+	if (IS_ERR(saradc->clk)) {
+		dev_dbg(&pdev->dev, "Can't get clock from device tree, using No-Die domain");
+	} else {
+		ret = clk_prepare_enable(saradc->clk);
+		if (ret)
+			return ret;
+	}
+
+	saradc->regs = devm_platform_ioremap_resource(pdev, 0);
+	if (IS_ERR(saradc->regs)) {
+		ret = PTR_ERR(saradc->regs);
+		goto error_disable_clock;
+	}
+
+	saradc->irq = platform_get_irq_optional(pdev, 0);
+	if (saradc->irq >= 0) {
+		ret = devm_request_irq(&pdev->dev, saradc->irq,
+				sophgo_saradc_interrupt_handler, 0,
+				dev_name(&pdev->dev), saradc);
+		if (ret)
+			goto error_disable_clock;
+
+		writel(1, saradc->regs + SOPHGO_SARADC_INTR_EN_REG);
+
+		init_completion(&saradc->completion);
+	}
+
+	saradc->info.read_raw = &sophgo_saradc_read_raw;
+
+	mutex_init(&saradc->lock);
+	platform_set_drvdata(pdev, indio_dev);
+	writel(SOPHGO_SARADC_DEF_CYC_SETTINGS, saradc->regs + SOPHGO_SARADC_CYC_SET_REG);
+	ret = devm_iio_device_register(&pdev->dev, indio_dev);
+	if (ret)
+		goto error_disable_clock;
+
+	return 0;
+
+error_disable_clock:
+	if (!IS_ERR(saradc->clk))
+		clk_disable_unprepare(saradc->clk);
+	return ret;
+}
+
+static void sophgo_saradc_remove(struct platform_device *pdev)
+{
+	struct iio_dev *indio_dev = platform_get_drvdata(pdev);
+	struct sophgo_saradc *saradc = iio_priv(indio_dev);
+
+	if (!IS_ERR(saradc->clk))
+		clk_disable_unprepare(saradc->clk);
+}
+
+
+static struct platform_driver sophgo_saradc_driver = {
+	.driver	= {
+		.name		= "sophgo-saradc",
+		.of_match_table	= sophgo_saradc_match,
+	},
+	.probe = sophgo_saradc_probe,
+	.remove_new = sophgo_saradc_remove,
+};
+module_platform_driver(sophgo_saradc_driver);
+
+MODULE_AUTHOR("Thomas Bonnefille <thomas.bonnefille@bootlin.com>");
+MODULE_DESCRIPTION("Sophgo SARADC driver");
+MODULE_LICENSE("GPL");

-- 
2.45.2

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

[Jonathan Cameron]

On Tue, 02 Jul 2024 13:52:22 +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>

Hi Thomas,

Welcome to IIO.

Various comments inline

Thanks,

Jonathan
> diff --git a/drivers/iio/adc/sophgo-adc.c b/drivers/iio/adc/sophgo-adc.c
> new file mode 100644
> index 000000000000..a94d839d40ec
> --- /dev/null
> +++ b/drivers/iio/adc/sophgo-adc.c
> @@ -0,0 +1,223 @@
> +// SPDX-License-Identifier: GPL-2.0-or-later
> +/*
> + *  Sophgo 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/module.h>
> +#include <linux/mutex.h>
> +#include <linux/of.h>
probably want mod_devicetable.h instead.

> +#include <linux/platform_device.h>
> +
> +#define SOPHGO_SARADC_CTRL_REG			0x04
> +#define		SOPHGO_SARADC_EN		BIT(0)
> +#define		SOPHGO_SARADC_SEL(x)		BIT((x)+4)
> +#define SOPHGO_SARADC_STATUS_REG		0x08
> +#define		SOPHGO_SARADC_BUSY		BIT(0)
> +#define SOPHGO_SARADC_CYC_SET_REG		0x0C
> +#define		SOPHGO_SARADC_DEF_CYC_SETTINGS	0xF1F0F
> +#define SOPHGO_SARADC_CH_RESULT_REG(x)		(0x10+4*(x))
> +#define		SARADC_CH_RESULT(x)		((x) & 0xfff)
> +#define		SARADC_CH_VALID(x)		((x) & BIT(15))

Prefer defined masks and FIELD_GET() to actually get the values.
Also use consistent prefix for defines.


> +#define SOPHGO_SARADC_INTR_EN_REG		0x20
> +#define SOPHGO_SARADC_INTR_CLR_REG		0x24
> +
> +#define SOPHGO_SARADC_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,					\
> +		.scan_type = {						\
> +			.sign = 'u',					\
> +			.realbits = 12,					\

No need to define these if not providing buffered capture.


> +		},							\
> +	}
> +
> +struct sophgo_saradc {
> +	struct completion completion;
> +	struct iio_info info;
> +	void __iomem *regs;
> +	struct mutex lock;

Add a comment on what data this lock protects.  

> +	struct clk *clk;
After changes below, probably don't need to hang on to the clk.
> +	int irq;
Rarely need to keep hold of the irq after probe. Use a local variable for
it in instead.

> +};
> +
> +static const struct iio_chan_spec sophgo_channels[] = {
> +	SOPHGO_SARADC_CHANNEL(1),
> +	SOPHGO_SARADC_CHANNEL(2),
> +	SOPHGO_SARADC_CHANNEL(3),
> +};
> +
> +static void sophgo_saradc_start_measurement(struct sophgo_saradc *saradc,
> +					    int channel)
> +{
> +	writel(0, saradc->regs + SOPHGO_SARADC_CTRL_REG);
> +	writel(SOPHGO_SARADC_SEL(channel) | SOPHGO_SARADC_EN,
> +	       saradc->regs + SOPHGO_SARADC_CTRL_REG);
> +}
> +
> +static int sophgo_saradc_wait(struct sophgo_saradc *saradc)
> +{
> +	if (saradc->irq < 0) {
> +		u32 reg;
> +
> +		return readl_poll_timeout(saradc->regs + SOPHGO_SARADC_STATUS_REG,
> +					  reg, !(reg & SOPHGO_SARADC_BUSY),
> +					  500, 1000000);
> +	} else {

Returned above, so no need for else which helps with indent.


> +		int ret;
> +
> +		ret = wait_for_completion_timeout(&saradc->completion,
> +						  msecs_to_jiffies(1000)) > 0
> +						  ? 0 : -ETIMEDOUT;
> +		return ret;
return wait_for_completion.
> +	}
> +}
> +
> +static int sophgo_saradc_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 sophgo_saradc *saradc = iio_priv(indio_dev);
> +		u32 sample;
> +		int ret;
> +
> +		mutex_lock(&saradc->lock);

Use scoped_guard() here so you can return directly on error.

> +		sophgo_saradc_start_measurement(saradc, chan->scan_index);
> +		ret = sophgo_saradc_wait(saradc);
> +		if (ret < 0) {
> +			mutex_unlock(&saradc->lock);
> +			return ret;
> +		}
> +
> +		sample = readl(saradc->regs + SOPHGO_SARADC_CH_RESULT_REG(chan->scan_index));
> +		mutex_unlock(&saradc->lock);
> +
> +		if (SARADC_CH_VALID(sample)) {
> +			*val = SARADC_CH_RESULT(sample);
> +			return IIO_VAL_INT;
> +		}
errors out of line preferred.
		if (!SARADC_CH_VALID(sample))
			return -ENODATA;

		*val...

> +		return -ENODATA;
> +	case IIO_CHAN_INFO_SCALE:
> +		*val = 3300;
> +		*val2 = 12;
> +		return IIO_VAL_FRACTIONAL_LOG2;
> +	default:
> +		return -EINVAL;
> +	}
> +}
> +
> +static irqreturn_t sophgo_saradc_interrupt_handler(int irq, void *dev_id)
> +{
> +	struct sophgo_saradc *saradc = dev_id;

dev_id is an odd name for something that isn't an id.

No register to confirm the interupt status?  Fine if not, but relatively
unusual.

> +
> +	writel(1, saradc->regs + SOPHGO_SARADC_INTR_CLR_REG);
> +	complete(&saradc->completion);
> +	return IRQ_HANDLED;
> +}
> +

Single blank line

> +
> +static const struct of_device_id sophgo_saradc_match[] = {
> +	{ .compatible = "sophgo,cv18xx-saradc", },
> +	{ },
No comma needed on 'null' terminators like this as we won't
ever add anything after them.

> +};
> +MODULE_DEVICE_TABLE(of, sophgo_saradc_match);
> +
> +static int sophgo_saradc_probe(struct platform_device *pdev)
> +{
> +	struct sophgo_saradc *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 SARADC";

Hmm. Whilst the ABI docs don't say it, I don't think we have names with
either capitals or spaces.  sophgo-saradc or similar preferred.

> +	indio_dev->info = &saradc->info;

This is making some static const data dynamic. I can't see why you'd do this

> +	indio_dev->modes = INDIO_DIRECT_MODE;
> +	indio_dev->num_channels = 3;

ARRAY_SIZE(sophgo_channels)

> +	indio_dev->channels = sophgo_channels;
> +
> +	saradc->clk = devm_clk_get(&pdev->dev, NULL);

If it's optional, then use dev_clk_get_optional()

> +	if (IS_ERR(saradc->clk)) {
> +		dev_dbg(&pdev->dev, "Can't get clock from device tree, using No-Die domain");
> +	} else {
> +		ret = clk_prepare_enable(saradc->clk);

Perhaps devm_clk_get_optional_enabled() is appropriate, then
you can use null return to detect that it wasn't present - vs an ERR_PTR() for
it was but didn't enable.

> +		if (ret)
> +			return ret;
> +	}
> +
> +	saradc->regs = devm_platform_ioremap_resource(pdev, 0);
> +	if (IS_ERR(saradc->regs)) {
> +		ret = PTR_ERR(saradc->regs);
> +		goto error_disable_clock;
> +	}
> +
> +	saradc->irq = platform_get_irq_optional(pdev, 0);
> +	if (saradc->irq >= 0) {
> +		ret = devm_request_irq(&pdev->dev, saradc->irq,
> +				sophgo_saradc_interrupt_handler, 0,
> +				dev_name(&pdev->dev), saradc);
> +		if (ret)
> +			goto error_disable_clock;
> +
> +		writel(1, saradc->regs + SOPHGO_SARADC_INTR_EN_REG);
> +
> +		init_completion(&saradc->completion);
Usually best to init a completion that is irq related before requesting
the irq. Avoids any possible races.

> +	}
> +
> +	saradc->info.read_raw = &sophgo_saradc_read_raw;
Why?  
> +
> +	mutex_init(&saradc->lock);
> +	platform_set_drvdata(pdev, indio_dev);
> +	writel(SOPHGO_SARADC_DEF_CYC_SETTINGS, saradc->regs + SOPHGO_SARADC_CYC_SET_REG);
> +	ret = devm_iio_device_register(&pdev->dev, indio_dev);
> +	if (ret)
> +		goto error_disable_clock;
> +
> +	return 0;
> +
> +error_disable_clock:
> +	if (!IS_ERR(saradc->clk))
Don't mix and match devm and non devm management.
As above, I think you can just use devm_clk_get_optional_enabled()

> +		clk_disable_unprepare(saradc->clk);
> +	return ret;
> +}
> +
> +static void sophgo_saradc_remove(struct platform_device *pdev)
> +{
> +	struct iio_dev *indio_dev = platform_get_drvdata(pdev);
> +	struct sophgo_saradc *saradc = iio_priv(indio_dev);
> +
> +	if (!IS_ERR(saradc->clk))
> +		clk_disable_unprepare(saradc->clk);
> +}
> +

Single blank line is enough.

> +
> +static struct platform_driver sophgo_saradc_driver = {
> +	.driver	= {
> +		.name		= "sophgo-saradc",
> +		.of_match_table	= sophgo_saradc_match,
> +	},
> +	.probe = sophgo_saradc_probe,
> +	.remove_new = sophgo_saradc_remove,
> +};
> +module_platform_driver(sophgo_saradc_driver);
> +
> +MODULE_AUTHOR("Thomas Bonnefille <thomas.bonnefille@bootlin.com>");
> +MODULE_DESCRIPTION("Sophgo SARADC driver");
> +MODULE_LICENSE("GPL");
> 

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

[Chen Wang]

On 2024/7/2 19:52, Thomas Bonnefille wrote:

[......]

>   SOUND
>   M:	Jaroslav Kysela <perex@perex.cz>
> diff --git a/drivers/iio/adc/Kconfig b/drivers/iio/adc/Kconfig
> index 8db68b80b391..826871a2e61a 100644
> --- a/drivers/iio/adc/Kconfig
> +++ b/drivers/iio/adc/Kconfig
> @@ -1122,6 +1122,15 @@ config SC27XX_ADC
>   	  This driver can also be built as a module. If so, the module
>   	  will be called sc27xx_adc.
>   
> +config SOPHGO_ADC
> +	tristate "Sophgo ADC"
> +	depends on ARCH_SOPHGO || COMPILE_TEST
> +	help
> +	  Say yes here to build support for the ADC integrated in Sophgo SoCs.
> +
> +	  This driver can also be built as a module. If so, the module
> +	  will be called sophgo_adc.
> +

I believe this adc driver is only for sophgo cv18xx, sophgo has other 
soc chipset, such as sg2024 etc., so it's better use add more limitation 
for this.

>   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..106a83d50d01 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_ADC) += sophgo-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-adc.c b/drivers/iio/adc/sophgo-adc.c

As I mentioned upon, soghgo has many other product code, and 
"sophgo-cv18xx-adc.c" should be more accurate.

[......]

[PATCH 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 SoC.
This patch adds nodes for the two SARADCs presents on 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 | 14 ++++++++++++++
 1 file changed, 14 insertions(+)

diff --git a/arch/riscv/boot/dts/sophgo/cv18xx.dtsi b/arch/riscv/boot/dts/sophgo/cv18xx.dtsi
index 7247c7c3013c..0b996aa7fa31 100644
--- a/arch/riscv/boot/dts/sophgo/cv18xx.dtsi
+++ b/arch/riscv/boot/dts/sophgo/cv18xx.dtsi
@@ -309,5 +309,19 @@ clint: timer@74000000 {
 			reg = <0x74000000 0x10000>;
 			interrupts-extended = <&cpu0_intc 3>, <&cpu0_intc 7>;
 		};
+
+		saradc_active: adc@30f0000 {
+			compatible = "sophgo,cv18xx-saradc";
+			clocks = <&clk CLK_SARADC>;
+			interrupts = <100 IRQ_TYPE_LEVEL_HIGH>;
+			reg = <0x030F0000 0x1000>;
+			status = "disabled";
+		};
+
+		saradc_nodie: adc@502c000 {
+			compatible = "sophgo,cv18xx-saradc";
+			reg = <0x0502C000 0x1000>;
+			status = "disabled";
+		};
 	};
 };

-- 
2.45.2

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

[Krzysztof Kozlowski]

On 02/07/2024 13:52, Thomas Bonnefille wrote:
> Adds SARADC nodes for the common Successive Approximation Analog to
> Digital Converter used in Sophgo SoC.
> This patch adds nodes for the two SARADCs presents on the board, one in
> the Active domain and the other in the No-Die domain.

You are duplicating the first sentence or this does not make sense...

> 
> Signed-off-by: Thomas Bonnefille <thomas.bonnefille@bootlin.com>
> ---
>  arch/riscv/boot/dts/sophgo/cv18xx.dtsi | 14 ++++++++++++++
>  1 file changed, 14 insertions(+)
> 
> diff --git a/arch/riscv/boot/dts/sophgo/cv18xx.dtsi b/arch/riscv/boot/dts/sophgo/cv18xx.dtsi
> index 7247c7c3013c..0b996aa7fa31 100644
> --- a/arch/riscv/boot/dts/sophgo/cv18xx.dtsi
> +++ b/arch/riscv/boot/dts/sophgo/cv18xx.dtsi
> @@ -309,5 +309,19 @@ clint: timer@74000000 {
>  			reg = <0x74000000 0x10000>;
>  			interrupts-extended = <&cpu0_intc 3>, <&cpu0_intc 7>;
>  		};
> +
> +		saradc_active: adc@30f0000 {

Please look at DTS coding style about ordering of nodes.

Best regards,
Krzysztof