| Message ID | 20240705-sg2002-adc-v2-1-83428c20a9b2@bootlin.com (mailing list archive) |
|---|---|
| State | Changes Requested |
| Headers | show |
| Series | Add SARADC support on Sophgo SoC | expand |
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 >
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
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."
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
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
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
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>;
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 > >
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
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>; + };
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(+)