| Message ID | 20240731-sg2002-adc-v3-1-5ac40a518c0a@bootlin.com (mailing list archive) |
|---|---|
| State | Changes Requested |
| Headers | show |
| Series | Add SARADC support on Sophgo SoC | expand |
On Wed, Jul 31, 2024 at 02:24:14PM GMT, 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 | 48 ++++++++++++++++++++++ > 1 file changed, 48 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..79d8cb52279f > --- /dev/null > +++ b/Documentation/devicetree/bindings/iio/adc/sophgo,cv18xx-saradc.yaml > @@ -0,0 +1,48 @@ > +# 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: > + - const: sophgo,cv1800b-saradc There is no need to use "oneOF" and "items" Suggestions: add a compatible like "cv1800-saradc" as fallback and add use "sophgo,cv1800b-saradc" as specific compatible. Use the "cv1800-saradc" in the cv18xx.dtsi and override the compatible with specific one if necessary. For example: - items: - enum: - sophgo,cv1800b-saradc - const: sophgo,cv1800-saradc - const: sophgo,cv1800b-saradc Regards, Inochi
On 31/07/2024 14:24, 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> A nit, subject: drop second/last, redundant "binding documentation". The "dt-bindings" prefix is already stating that these are bindings and this is documentation. Cannot be anything else. See also: https://elixir.bootlin.com/linux/v6.7-rc8/source/Documentation/devicetree/bindings/submitting-patches.rst#L18 > --- > .../bindings/iio/adc/sophgo,cv18xx-saradc.yaml | 48 ++++++++++++++++++++++ > 1 file changed, 48 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..79d8cb52279f > --- /dev/null > +++ b/Documentation/devicetree/bindings/iio/adc/sophgo,cv18xx-saradc.yaml > @@ -0,0 +1,48 @@ > +# 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: Drop > + - items: Drop, use const directly. > + - const: sophgo,cv1800b-saradc > + > + reg: > + maxItems: 1 > + > + interrupts: > + maxItems: 1 > + > + clocks: > + maxItems: 1 > + > +required: > + - clocks > + - compatible > + - reg Odd order. compatible is always first. Anyway, list here has the same order as "properties:". > + > +unevaluatedProperties: false I don't see any other $ref. Don't you miss adc.yaml? Or channels? Or some more properties? This looks incomplete for ADC. > + > +examples: > + - | > + #include <dt-bindings/clock/sophgo,cv1800.h> > + #include <dt-bindings/interrupt-controller/irq.h> > + adc@30f0000 { > + compatible = "sophgo,cv1800b-saradc"; > + clocks = <&clk CLK_SARADC>; > + interrupts = <100 IRQ_TYPE_LEVEL_HIGH>; > + reg = <0x030F0000 0x1000>; Hex is *always* lower case, see DTS coding style. Order the properties correctly, so again, please read DTS coding style. > + }; > Best regards, Krzysztof
On 7/31/24 2:41 PM, Inochi Amaoto wrote: > On Wed, Jul 31, 2024 at 02:24:14PM GMT, 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 | 48 ++++++++++++++++++++++ >> 1 file changed, 48 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..79d8cb52279f >> --- /dev/null >> +++ b/Documentation/devicetree/bindings/iio/adc/sophgo,cv18xx-saradc.yaml >> @@ -0,0 +1,48 @@ >> +# 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: >> + - const: sophgo,cv1800b-saradc > > There is no need to use "oneOF" and "items" > Thank you very much, I'll do that. > Suggestions: add a compatible like "cv1800-saradc" as fallback > and add use "sophgo,cv1800b-saradc" as specific compatible. > Use the "cv1800-saradc" in the cv18xx.dtsi and override the > compatible with specific one if necessary. > If I understand correctly, maintainers doesn't want the use of wildcards as generic compatibles [1]. They prefer to use the most basic SoC as the generic compatible. Is the CV1800 a real SoC or is it just a kind of wildcard to say CV18* ? > For example: > - items: > - enum: > - sophgo,cv1800b-saradc > - const: sophgo,cv1800-saradc > - const: sophgo,cv1800b-saradc > To avoid the issue of falling back on a wildcard I planned to do this instead: properties: compatible: const: sophgo,cv1800b-saradc > Regards, > Inochi [1] : https://lore.kernel.org/all/20240708165719.000021b9@Huawei.com/ Thank you for your comments :) Thomas
On Wed, Jul 31, 2024 at 04:48:01PM GMT, Thomas Bonnefille wrote: > > > On 7/31/24 2:41 PM, Inochi Amaoto wrote: > > On Wed, Jul 31, 2024 at 02:24:14PM GMT, 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 | 48 ++++++++++++++++++++++ > > > 1 file changed, 48 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..79d8cb52279f > > > --- /dev/null > > > +++ b/Documentation/devicetree/bindings/iio/adc/sophgo,cv18xx-saradc.yaml > > > @@ -0,0 +1,48 @@ > > > +# 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: > > > + - const: sophgo,cv1800b-saradc > > > > There is no need to use "oneOF" and "items" > > > > Thank you very much, I'll do that. > > > Suggestions: add a compatible like "cv1800-saradc" as fallback > > and add use "sophgo,cv1800b-saradc" as specific compatible. > > Use the "cv1800-saradc" in the cv18xx.dtsi and override the > > compatible with specific one if necessary. > > > > If I understand correctly, maintainers doesn't want the use of wildcards as > generic compatibles [1]. They prefer to use the most basic SoC as the > generic compatible. > Is the CV1800 a real SoC or is it just a kind of wildcard to say CV18* ? Yeah, that is the wildcard too. > > > For example: > > - items: > > - enum: > > - sophgo,cv1800b-saradc > > - const: sophgo,cv1800-saradc > > - const: sophgo,cv1800b-saradc > > > > To avoid the issue of falling back on a wildcard I planned to do this > instead: > properties: > compatible: > const: sophgo,cv1800b-saradc > I have read the link, it seems the upstream may prefer this solution. Let's use cv1800b as the basic fallback. > > > > Regards, > > Inochi > > [1] : https://lore.kernel.org/all/20240708165719.000021b9@Huawei.com/ > > Thank you for your comments :) > Thomas
On Wed, 31 Jul 2024 15:57:27 +0200 Krzysztof Kozlowski <krzk@kernel.org> wrote: > On 31/07/2024 14:24, 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> > > A nit, subject: drop second/last, redundant "binding documentation". The > "dt-bindings" prefix is already stating that these are bindings and this > is documentation. Cannot be anything else. > See also: > https://elixir.bootlin.com/linux/v6.7-rc8/source/Documentation/devicetree/bindings/submitting-patches.rst#L18 > > > > --- > > .../bindings/iio/adc/sophgo,cv18xx-saradc.yaml | 48 ++++++++++++++++++++++ > > 1 file changed, 48 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..79d8cb52279f > > --- /dev/null > > +++ b/Documentation/devicetree/bindings/iio/adc/sophgo,cv18xx-saradc.yaml > > @@ -0,0 +1,48 @@ > > +# 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: > > Drop > > > + - items: > > Drop, use const directly. > > > > + - const: sophgo,cv1800b-saradc > > + > > + reg: > > + maxItems: 1 > > + > > + interrupts: > > + maxItems: 1 > > + > > + clocks: > > + maxItems: 1 > > + > > +required: > > + - clocks > > + - compatible > > + - reg > > Odd order. compatible is always first. Anyway, list here has the same > order as "properties:". > > > + > > +unevaluatedProperties: false > > I don't see any other $ref. Don't you miss adc.yaml? Or channels? Or > some more properties? This looks incomplete for ADC. It's acceptable on ADCs in particular (but more generally) to just assume all channels are exposed. They may all be wired to internal power lines anyway, in which case what is there is a chip feature. This only works if their isn't any channel specific configuration, then not providing the channels child nodes is fine. However, that requires us to be fairly sure there won't ever be a per channel thing that needs configuring from DT. That's generally only the case in simple devices. So might be better to put the channels nodes in there and deal with dynamic channel configuration (so don't present any without a channel node) from the start as it's more future proof. > > > + > > +examples: > > + - | > > + #include <dt-bindings/clock/sophgo,cv1800.h> > > + #include <dt-bindings/interrupt-controller/irq.h> > > + adc@30f0000 { > > + compatible = "sophgo,cv1800b-saradc"; > > + clocks = <&clk CLK_SARADC>; > > + interrupts = <100 IRQ_TYPE_LEVEL_HIGH>; > > + reg = <0x030F0000 0x1000>; > > Hex is *always* lower case, see DTS coding style. > > Order the properties correctly, so again, please read DTS coding style. > > > + }; > > > > Best regards, > Krzysztof >
On 03/08/2024 12:39, Jonathan Cameron wrote: >> >>> + >>> +unevaluatedProperties: false >> >> I don't see any other $ref. Don't you miss adc.yaml? Or channels? Or >> some more properties? This looks incomplete for ADC. > > It's acceptable on ADCs in particular (but more generally) > to just assume all channels are exposed. They may all be wired > to internal power lines anyway, in which case what is there is > a chip feature. This only works if their isn't any channel specific > configuration, then not providing the channels child nodes is fine. > > However, that requires us to be fairly sure there won't ever be > a per channel thing that needs configuring from DT. > That's generally only the case in simple devices. > > So might be better to put the channels nodes in there and deal with > dynamic channel configuration (so don't present any without > a channel node) from the start as it's more future proof. OK. Then anyway this should be additionalProperties: false (unless I missed somewhere $ref?). Best regards, Krzysztof
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..79d8cb52279f --- /dev/null +++ b/Documentation/devicetree/bindings/iio/adc/sophgo,cv18xx-saradc.yaml @@ -0,0 +1,48 @@ +# 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: + - const: sophgo,cv1800b-saradc + + reg: + maxItems: 1 + + interrupts: + maxItems: 1 + + clocks: + maxItems: 1 + +required: + - clocks + - compatible + - reg + +unevaluatedProperties: false + +examples: + - | + #include <dt-bindings/clock/sophgo,cv1800.h> + #include <dt-bindings/interrupt-controller/irq.h> + adc@30f0000 { + compatible = "sophgo,cv1800b-saradc"; + clocks = <&clk CLK_SARADC>; + interrupts = <100 IRQ_TYPE_LEVEL_HIGH>; + reg = <0x030F0000 0x1000>; + };
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 | 48 ++++++++++++++++++++++ 1 file changed, 48 insertions(+)