| Message ID | SG2PR01MB42189977B4172405F5704CC4D7F82@SG2PR01MB4218.apcprd01.prod.exchangelabs.com |
|---|---|
| State | New |
| Headers | show |
| Series | riscv: sophgo: add thermal sensor support for cv180x/sg200x SoCs | expand |
On Tue, Jun 04, 2024 at 12:54:19PM GMT, Haylen Chu wrote: > Add devicetree binding documentation for thermal sensors integrated in > Sophgo CV180X SoCs. > > Signed-off-by: Haylen Chu <heylenay@outlook.com> > --- > .../thermal/sophgo,cv180x-thermal.yaml | 82 +++++++++++++++++++ > 1 file changed, 82 insertions(+) > create mode 100644 Documentation/devicetree/bindings/thermal/sophgo,cv180x-thermal.yaml > > diff --git a/Documentation/devicetree/bindings/thermal/sophgo,cv180x-thermal.yaml b/Documentation/devicetree/bindings/thermal/sophgo,cv180x-thermal.yaml > new file mode 100644 > index 000000000000..1c3a6f74ff1d > --- /dev/null > +++ b/Documentation/devicetree/bindings/thermal/sophgo,cv180x-thermal.yaml > @@ -0,0 +1,82 @@ > +# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause) > +%YAML 1.2 > +--- > +$id: http://devicetree.org/schemas/thermal/sophgo,cv180x-thermal.yaml# > +$schema: http://devicetree.org/meta-schemas/core.yaml# > + > +title: Sophgo CV180x on-SoC Thermal Sensor > + > +maintainers: > + - Haylen Chu <heylenay@outlook.com> > + > +description: Binding for Sophgo CV180x on-SoC thermal sensor > + > +properties: > + compatible: > + enum: > + - sophgo,cv1800-thermal > + - sophgo,cv180x-thermal > + Is this necessary? I don't find any change between the sensor of these. > + reg: > + maxItems: 1 > + > + clocks: > + description: The thermal sensor clock > + > + clock-names: > + const: clk_tempsen > + > + accumulation-period: > + $ref: /schemas/types.yaml#/definitions/uint32 > + description: Accumulation period for a sample > + oneOf: > + - const: 0 > + description: 512 ticks > + - const: 1 > + description: 1024 ticks > + - const: 2 > + description: 2048 ticks > + - const: 3 > + description: 4096 ticks > + default: 2 > + > + chop-period: > + $ref: /schemas/types.yaml#/definitions/uint32 > + description: ADC chop period > + oneOf: > + - const: 0 > + description: 128 ticks > + - const: 1 > + description: 256 ticks > + - const: 2 > + description: 512 ticks > + - const: 3 > + description: 1024 ticks > + default: 3 > + > + sample-cycle-us: > + description: Period between samples > + default: 1000000 > + > + '#thermal-sensor-cells': > + const: 0 > + > +required: > + - compatible > + - reg > + - clocks > + - clock-names > + > +additionalProperties: false > + > +examples: > + - | > + #include <dt-bindings/clock/sophgo,cv1800.h> > + thermal-sensor@30e0000 { > + compatible = "sophgo,cv180x-thermal"; > + reg = <0x30e0000 0x100>; > + clocks = <&clk CLK_TEMPSEN>; > + clock-names = "clk_tempsen"; > + #thermal-sensor-cells = <0>; > + }; > +... Where is the interrupt number? The sensors does support the interrupt, but I don't see you describe it in the binding. > -- > 2.45.2 >
On Wed, Jun 05, 2024 at 11:40:32AM +0800, Inochi Amaoto wrote: > On Tue, Jun 04, 2024 at 12:54:19PM GMT, Haylen Chu wrote: > > Add devicetree binding documentation for thermal sensors integrated in > > Sophgo CV180X SoCs. > > > > Signed-off-by: Haylen Chu <heylenay@outlook.com> > > --- > > .../thermal/sophgo,cv180x-thermal.yaml | 82 +++++++++++++++++++ > > 1 file changed, 82 insertions(+) > > create mode 100644 Documentation/devicetree/bindings/thermal/sophgo,cv180x-thermal.yaml > > > > diff --git a/Documentation/devicetree/bindings/thermal/sophgo,cv180x-thermal.yaml b/Documentation/devicetree/bindings/thermal/sophgo,cv180x-thermal.yaml > > new file mode 100644 > > index 000000000000..1c3a6f74ff1d > > --- /dev/null > > +++ b/Documentation/devicetree/bindings/thermal/sophgo,cv180x-thermal.yaml > > @@ -0,0 +1,82 @@ > > +# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause) > > +%YAML 1.2 > > +--- > > +$id: http://devicetree.org/schemas/thermal/sophgo,cv180x-thermal.yaml# > > +$schema: http://devicetree.org/meta-schemas/core.yaml# > > + > > +title: Sophgo CV180x on-SoC Thermal Sensor > > + > > +maintainers: > > + - Haylen Chu <heylenay@outlook.com> > > + > > +description: Binding for Sophgo CV180x on-SoC thermal sensor > > + > > +properties: > > + compatible: > > + enum: > > + - sophgo,cv1800-thermal > > + - sophgo,cv180x-thermal > > + > > Is this necessary? I don't find any change between the sensor of these. "cv180x" isn't even a real device. Either we have a compatible that matches an actual SoC and use it everywhere, or we add ones for each SoC and have a fallback to cv1800. > > + reg: > > + maxItems: 1 > > + > > + clocks: > > + description: The thermal sensor clock > > + > > + clock-names: > > + const: clk_tempsen clock-names is not useful here as there's only one clock. "clk_tempsen" sounds more like the name for this clock at the provider than at the consumer anyway. > > + > > + accumulation-period: > > + $ref: /schemas/types.yaml#/definitions/uint32 > > + description: Accumulation period for a sample > > + oneOf: > > + - const: 0 > > + description: 512 ticks > > + - const: 1 > > + description: 1024 ticks > > + - const: 2 > > + description: 2048 ticks > > + - const: 3 > > + description: 4096 ticks > > + default: 2 > > + > > + chop-period: > > + $ref: /schemas/types.yaml#/definitions/uint32 > > + description: ADC chop period What's a "chop" and why is either this or the accumulation-period a fixed property of the hardware? Shouldn't this choice really be up to the user? > > + oneOf: > > + - const: 0 > > + description: 128 ticks > > + - const: 1 > > + description: 256 ticks > > + - const: 2 > > + description: 512 ticks > > + - const: 3 > > + description: 1024 ticks Can we just make the number of ticks the unit here, and above? Also, a "oneOf: - const" structure is just an enum. > > + default: 3 > > + > > + sample-cycle-us: > > + description: Period between samples > > + default: 1000000 No constraints? Thanks, Conor. > > + > > + '#thermal-sensor-cells': > > + const: 0 > > + > > +required: > > + - compatible > > + - reg > > + - clocks > > + - clock-names > > + > > +additionalProperties: false > > + > > +examples: > > + - | > > + #include <dt-bindings/clock/sophgo,cv1800.h> > > + thermal-sensor@30e0000 { > > + compatible = "sophgo,cv180x-thermal"; > > + reg = <0x30e0000 0x100>; > > + clocks = <&clk CLK_TEMPSEN>; > > + clock-names = "clk_tempsen"; > > + #thermal-sensor-cells = <0>; > > + }; > > +... > > Where is the interrupt number? The sensors does support the interrupt, > but I don't see you describe it in the binding. > > > -- > > 2.45.2 > >
On Wed, Jun 05, 2024 at 06:54:17PM +0100, Conor Dooley wrote: > > > + accumulation-period: > > > + $ref: /schemas/types.yaml#/definitions/uint32 > > > + description: Accumulation period for a sample > > > + oneOf: > > > + - const: 0 > > > + description: 512 ticks > > > + - const: 1 > > > + description: 1024 ticks > > > + - const: 2 > > > + description: 2048 ticks > > > + - const: 3 > > > + description: 4096 ticks > > > + default: 2 > > > + > > > + chop-period: > > > + $ref: /schemas/types.yaml#/definitions/uint32 > > > + description: ADC chop period > > What's a "chop" and why is either this or the accumulation-period a > fixed property of the hardware? Shouldn't this choice really be up to > the user? The chop-period is an ADC parameter. Both accumulation-period and chop-period specify how the sensor measures temperature. Making these parameters up to end users brings extra unnecessary code complexity. Being configurable for each board should be enough and other thermal drivers have been doing things in this way. > > > > + oneOf: > > > + - const: 0 > > > + description: 128 ticks > > > + - const: 1 > > > + description: 256 ticks > > > + - const: 2 > > > + description: 512 ticks > > > + - const: 3 > > > + description: 1024 ticks > > Can we just make the number of ticks the unit here, and above? > Also, a "oneOf: - const" structure is just an enum. I do not catch your idea. These values directly map to raw register configuration, which simplify the implementation a lot. > > > > + default: 3 > > > + > > > + sample-cycle-us: > > > + description: Period between samples > > > + default: 1000000 > No constraints? Sample cycle is more flexible because of hardware designing. Best reguards, Haylen Chu
On Thu, Jun 06, 2024 at 01:32:46PM +0000, Haylen Chu wrote: > On Wed, Jun 05, 2024 at 06:54:17PM +0100, Conor Dooley wrote: > > > > + accumulation-period: > > > > + $ref: /schemas/types.yaml#/definitions/uint32 > > > > + description: Accumulation period for a sample > > > > + oneOf: > > > > + - const: 0 > > > > + description: 512 ticks > > > > + - const: 1 > > > > + description: 1024 ticks > > > > + - const: 2 > > > > + description: 2048 ticks > > > > + - const: 3 > > > > + description: 4096 ticks > > > > + default: 2 > > > > + > > > > + chop-period: > > > > + $ref: /schemas/types.yaml#/definitions/uint32 > > > > + description: ADC chop period > > > > What's a "chop" and why is either this or the accumulation-period a > > fixed property of the hardware? Shouldn't this choice really be up to > > the user? > > The chop-period is an ADC parameter. > > Both accumulation-period and chop-period specify how the sensor > measures temperature. Making these parameters up to end users brings > extra unnecessary code complexity. Being configurable for each board > should be enough and other thermal drivers have been doing things in > this way. Other systems may well have properties for this, but something being done in the past doesn't mean it might be the right thing to do now. I don't really buy that this is something you set to a fixed value per board, but rather the use case of a particular board would factor into whether or not you would want to use a shorter or longer accumulation period. > > > > + oneOf: > > > > + - const: 0 > > > > + description: 128 ticks > > > > + - const: 1 > > > > + description: 256 ticks > > > > + - const: 2 > > > > + description: 512 ticks > > > > + - const: 3 > > > > + description: 1024 ticks > > > > Can we just make the number of ticks the unit here, and above? > > Also, a "oneOf: - const" structure is just an enum. > > I do not catch your idea. These values directly map to raw register > configuration, which simplify the implementation a lot. It should be trivial to convert them to register values in your driver. > > > > + default: 3 > > > > + > > > > + sample-cycle-us: > > > > + description: Period between samples > > > > + default: 1000000 > > No constraints? > > Sample cycle is more flexible because of hardware designing. It quite likely has constraints, flexible or not. Is the hardware capable of both 1 us and uint32_max us? Thanks, Conor.
On Thu, Jun 06, 2024 at 06:05:30PM +0100, Conor Dooley wrote: > On Thu, Jun 06, 2024 at 01:32:46PM +0000, Haylen Chu wrote: > > Both accumulation-period and chop-period specify how the sensor > > measures temperature. Making these parameters up to end users brings > > extra unnecessary code complexity. Being configurable for each board > > should be enough and other thermal drivers have been doing things in > > this way. > > Other systems may well have properties for this, but something being > done in the past doesn't mean it might be the right thing to do now. > I don't really buy that this is something you set to a fixed value per > board, but rather the use case of a particular board would factor into > whether or not you would want to use a shorter or longer accumulation > period. Accumulation period and chop period do only affect the accuracy of its result, in a range about 1 Celsius degree. Changing these parameters does not mean much to end users, as this is only a thermal sensor and 1 Celsius is quite good for its usage. And users could always switch to another configuration with a dt-overlay. > > I do not catch your idea. These values directly map to raw register > > configuration, which simplify the implementation a lot. > > It should be trivial to convert them to register values in your driver. Okay, I will do this. > > > > > + default: 3 > > > > > + > > > > > + sample-cycle-us: > > > > > + description: Period between samples > > > > > + default: 1000000 > > > No constraints? > > > > Sample cycle is more flexible because of hardware designing. > > It quite likely has constraints, flexible or not. Is the hardware > capable of both 1 us and uint32_max us? It should be a value between 524 and 2^24 - 1. Will document this in next revision. > Thanks, > Conor. Best regards, Haylen Chu
diff --git a/Documentation/devicetree/bindings/thermal/sophgo,cv180x-thermal.yaml b/Documentation/devicetree/bindings/thermal/sophgo,cv180x-thermal.yaml new file mode 100644 index 000000000000..1c3a6f74ff1d --- /dev/null +++ b/Documentation/devicetree/bindings/thermal/sophgo,cv180x-thermal.yaml @@ -0,0 +1,82 @@ +# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause) +%YAML 1.2 +--- +$id: http://devicetree.org/schemas/thermal/sophgo,cv180x-thermal.yaml# +$schema: http://devicetree.org/meta-schemas/core.yaml# + +title: Sophgo CV180x on-SoC Thermal Sensor + +maintainers: + - Haylen Chu <heylenay@outlook.com> + +description: Binding for Sophgo CV180x on-SoC thermal sensor + +properties: + compatible: + enum: + - sophgo,cv1800-thermal + - sophgo,cv180x-thermal + + reg: + maxItems: 1 + + clocks: + description: The thermal sensor clock + + clock-names: + const: clk_tempsen + + accumulation-period: + $ref: /schemas/types.yaml#/definitions/uint32 + description: Accumulation period for a sample + oneOf: + - const: 0 + description: 512 ticks + - const: 1 + description: 1024 ticks + - const: 2 + description: 2048 ticks + - const: 3 + description: 4096 ticks + default: 2 + + chop-period: + $ref: /schemas/types.yaml#/definitions/uint32 + description: ADC chop period + oneOf: + - const: 0 + description: 128 ticks + - const: 1 + description: 256 ticks + - const: 2 + description: 512 ticks + - const: 3 + description: 1024 ticks + default: 3 + + sample-cycle-us: + description: Period between samples + default: 1000000 + + '#thermal-sensor-cells': + const: 0 + +required: + - compatible + - reg + - clocks + - clock-names + +additionalProperties: false + +examples: + - | + #include <dt-bindings/clock/sophgo,cv1800.h> + thermal-sensor@30e0000 { + compatible = "sophgo,cv180x-thermal"; + reg = <0x30e0000 0x100>; + clocks = <&clk CLK_TEMPSEN>; + clock-names = "clk_tempsen"; + #thermal-sensor-cells = <0>; + }; +...
Add devicetree binding documentation for thermal sensors integrated in Sophgo CV180X SoCs. Signed-off-by: Haylen Chu <heylenay@outlook.com> --- .../thermal/sophgo,cv180x-thermal.yaml | 82 +++++++++++++++++++ 1 file changed, 82 insertions(+) create mode 100644 Documentation/devicetree/bindings/thermal/sophgo,cv180x-thermal.yaml