[v4,1/3] dt-bindings: thermal: sophgo,cv1800-thermal: Add Sophgo CV1800 thermal

Commit Message

Haylen Chu July 16, 2024, 9:42 a.m. UTC

Add devicetree binding documentation for thermal sensors integrated in
Sophgo CV180X SoCs.

Signed-off-by: Haylen Chu <[e-mail]>
---
 .../thermal/sophgo,cv1800-thermal.yaml        | 55 +++++++++++++++++++
 1 file changed, 55 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/thermal/sophgo,cv1800-thermal.yaml

Comments

Chen Wang July 16, 2024, 12:43 p.m. UTC | #1

On 2024/7/16 17:42, Haylen Chu wrote:
> Add devicetree binding documentation for thermal sensors integrated in
> Sophgo CV180X SoCs.
>
> Signed-off-by: Haylen Chu <[e-mail]>
> ---
>   .../thermal/sophgo,cv1800-thermal.yaml        | 55 +++++++++++++++++++

I see sometimes you call it cv1800, and in patch 3, the file name is 
cv180x_thermal.c, and for dts changes, you changed cv18xx.dtsi. Please 
unify it.

I think sg200x is new name for cv181x serias, so if you want to cover 
cv180x/sg200x, is cv18xx better?

>   1 file changed, 55 insertions(+)
>   create mode 100644 Documentation/devicetree/bindings/thermal/sophgo,cv1800-thermal.yaml
>
> diff --git a/Documentation/devicetree/bindings/thermal/sophgo,cv1800-thermal.yaml b/Documentation/devicetree/bindings/thermal/sophgo,cv1800-thermal.yaml
> new file mode 100644
> index 000000000000..58bd4432cd10
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/thermal/sophgo,cv1800-thermal.yaml
> @@ -0,0 +1,55 @@
> +# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause)
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/thermal/sophgo,cv1800-thermal.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: Sophgo CV1800 on-SoC Thermal Sensor
> +
> +maintainers:
> +  - Haylen Chu <[e-mail]>
> +
> +description: Sophgo CV1800 on-SoC thermal sensor
> +
> +properties:
> +  compatible:
> +    enum:
> +      - sophgo,cv1800-thermal
cv18xx-thermal ?
> +
> +  reg:
> +    maxItems: 1
> +
> +  clocks:
> +    description: The thermal sensor clock
> +
> +  interrupts:
> +    maxItems: 1
> +
> +  sample-rate-hz:
> +    minimum: 1
> +    maximum: 1908
> +    default: 1
> +
> +  '#thermal-sensor-cells':
> +    const: 0
> +
> +required:
> +  - compatible
> +  - reg
> +  - clocks
> +  - interrupts
> +
> +additionalProperties: false
> +
> +examples:
> +  - |
> +        #include <dt-bindings/clock/sophgo,cv1800.h>
> +        #include <dt-bindings/interrupt-controller/irq.h>
> +        thermal-sensor@30e0000 {
> +            compatible = "sophgo,cv1800-thermal";
> +            reg = <0x30e0000 0x100>;
> +            clocks = <&clk CLK_TEMPSEN>;
> +            interrupts = <16 IRQ_TYPE_LEVEL_HIGH>;
> +            #thermal-sensor-cells = <0>;
> +        };
> +...

Conor Dooley July 16, 2024, 3:48 p.m. UTC | #2

On Tue, Jul 16, 2024 at 08:43:19PM +0800, Chen Wang wrote:
> 
> On 2024/7/16 17:42, Haylen Chu wrote:
> > Add devicetree binding documentation for thermal sensors integrated in
> > Sophgo CV180X SoCs.
> > 
> > Signed-off-by: Haylen Chu <[e-mail]>
> > ---
> >   .../thermal/sophgo,cv1800-thermal.yaml        | 55 +++++++++++++++++++
> 
> I see sometimes you call it cv1800, and in patch 3, the file name is
> cv180x_thermal.c, and for dts changes, you changed cv18xx.dtsi. Please unify
> it.
> 
> I think sg200x is new name for cv181x serias, so if you want to cover
> cv180x/sg200x, is cv18xx better?
> 
> >   1 file changed, 55 insertions(+)
> >   create mode 100644 Documentation/devicetree/bindings/thermal/sophgo,cv1800-thermal.yaml
> > 
> > diff --git a/Documentation/devicetree/bindings/thermal/sophgo,cv1800-thermal.yaml b/Documentation/devicetree/bindings/thermal/sophgo,cv1800-thermal.yaml
> > new file mode 100644
> > index 000000000000..58bd4432cd10
> > --- /dev/null
> > +++ b/Documentation/devicetree/bindings/thermal/sophgo,cv1800-thermal.yaml
> > @@ -0,0 +1,55 @@
> > +# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause)
> > +%YAML 1.2
> > +---
> > +$id: http://devicetree.org/schemas/thermal/sophgo,cv1800-thermal.yaml#
> > +$schema: http://devicetree.org/meta-schemas/core.yaml#
> > +
> > +title: Sophgo CV1800 on-SoC Thermal Sensor
> > +
> > +maintainers:
> > +  - Haylen Chu <[e-mail]>
> > +
> > +description: Sophgo CV1800 on-SoC thermal sensor
> > +
> > +properties:
> > +  compatible:
> > +    enum:
> > +      - sophgo,cv1800-thermal
> cv18xx-thermal ?

Please, no wildcards in compatibles :/

> > +
> > +  reg:
> > +    maxItems: 1
> > +
> > +  clocks:
> > +    description: The thermal sensor clock
> > +
> > +  interrupts:
> > +    maxItems: 1
> > +
> > +  sample-rate-hz:
> > +    minimum: 1
> > +    maximum: 1908
> > +    default: 1

I still don't think this belongs in the devicetree, but is actually
software policy.

Cheers,
Conor.

> > +
> > +  '#thermal-sensor-cells':
> > +    const: 0
> > +
> > +required:
> > +  - compatible
> > +  - reg
> > +  - clocks
> > +  - interrupts
> > +
> > +additionalProperties: false
> > +
> > +examples:
> > +  - |
> > +        #include <dt-bindings/clock/sophgo,cv1800.h>
> > +        #include <dt-bindings/interrupt-controller/irq.h>
> > +        thermal-sensor@30e0000 {
> > +            compatible = "sophgo,cv1800-thermal";
> > +            reg = <0x30e0000 0x100>;
> > +            clocks = <&clk CLK_TEMPSEN>;
> > +            interrupts = <16 IRQ_TYPE_LEVEL_HIGH>;
> > +            #thermal-sensor-cells = <0>;
> > +        };
> > +...

Krzysztof Kozlowski July 16, 2024, 3:56 p.m. UTC | #3

On 16/07/2024 11:42, Haylen Chu wrote:
> Add devicetree binding documentation for thermal sensors integrated in
> Sophgo CV180X SoCs.
> 
> Signed-off-by: Haylen Chu <[e-mail]>
> ---
>  .../thermal/sophgo,cv1800-thermal.yaml        | 55 +++++++++++++++++++
>  1 file changed, 55 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/thermal/sophgo,cv1800-thermal.yaml
> 
> diff --git a/Documentation/devicetree/bindings/thermal/sophgo,cv1800-thermal.yaml b/Documentation/devicetree/bindings/thermal/sophgo,cv1800-thermal.yaml
> new file mode 100644
> index 000000000000..58bd4432cd10
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/thermal/sophgo,cv1800-thermal.yaml
> @@ -0,0 +1,55 @@
> +# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause)
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/thermal/sophgo,cv1800-thermal.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: Sophgo CV1800 on-SoC Thermal Sensor
> +
> +maintainers:
> +  - Haylen Chu <[e-mail]>
> +
> +description: Sophgo CV1800 on-SoC thermal sensor
> +

Missing $ref to thermal-sensor.yaml


> +properties:
> +  compatible:
> +    enum:
> +      - sophgo,cv1800-thermal
> +
> +  reg:
> +    maxItems: 1
> +
> +  clocks:
> +    description: The thermal sensor clock

Description is useless. You miss constraints on the other hand, so maxItems.

> +
> +  interrupts:
> +    maxItems: 1
> +
> +  sample-rate-hz:
> +    minimum: 1
> +    maximum: 1908
> +    default: 1
> +
> +  '#thermal-sensor-cells':
> +    const: 0
> +
> +required:
> +  - compatible
> +  - reg
> +  - clocks
> +  - interrupts
> +
> +additionalProperties: false
> +
> +examples:
> +  - |
> +        #include <dt-bindings/clock/sophgo,cv1800.h>
> +        #include <dt-bindings/interrupt-controller/irq.h>

Use 4 spaces for example indentation.



Best regards,
Krzysztof

Chen Wang July 17, 2024, 12:05 a.m. UTC | #4

On 2024/7/16 23:48, Conor Dooley wrote:
> On Tue, Jul 16, 2024 at 08:43:19PM +0800, Chen Wang wrote:
>> On 2024/7/16 17:42, Haylen Chu wrote:
>>> Add devicetree binding documentation for thermal sensors integrated in
>>> Sophgo CV180X SoCs.
>>>
>>> Signed-off-by: Haylen Chu <[e-mail]>
>>> ---
>>>    .../thermal/sophgo,cv1800-thermal.yaml        | 55 +++++++++++++++++++
>> I see sometimes you call it cv1800, and in patch 3, the file name is
>> cv180x_thermal.c, and for dts changes, you changed cv18xx.dtsi. Please unify
>> it.
>>
>> I think sg200x is new name for cv181x serias, so if you want to cover
>> cv180x/sg200x, is cv18xx better?
>>
>>>    1 file changed, 55 insertions(+)
>>>    create mode 100644 Documentation/devicetree/bindings/thermal/sophgo,cv1800-thermal.yaml
>>>
>>> diff --git a/Documentation/devicetree/bindings/thermal/sophgo,cv1800-thermal.yaml b/Documentation/devicetree/bindings/thermal/sophgo,cv1800-thermal.yaml
>>> new file mode 100644
>>> index 000000000000..58bd4432cd10
>>> --- /dev/null
>>> +++ b/Documentation/devicetree/bindings/thermal/sophgo,cv1800-thermal.yaml
>>> @@ -0,0 +1,55 @@
>>> +# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause)
>>> +%YAML 1.2
>>> +---
>>> +$id: http://devicetree.org/schemas/thermal/sophgo,cv1800-thermal.yaml#
>>> +$schema: http://devicetree.org/meta-schemas/core.yaml#
>>> +
>>> +title: Sophgo CV1800 on-SoC Thermal Sensor
>>> +
>>> +maintainers:
>>> +  - Haylen Chu <[e-mail]>
>>> +
>>> +description: Sophgo CV1800 on-SoC thermal sensor
>>> +
>>> +properties:
>>> +  compatible:
>>> +    enum:
>>> +      - sophgo,cv1800-thermal
>> cv18xx-thermal ?
> Please, no wildcards in compatibles :/

Sorry for my confusion.

Haylen, so you want a compatible that matches an actual SoC and use it 
everywhere?

Or we can add ones for each SoC and have a fallback to cv1800.


[......]

Inochi Amaoto July 17, 2024, 1:27 a.m. UTC | #5

On Wed, Jul 17, 2024 at 08:05:10AM GMT, Chen Wang wrote:
> 
> On 2024/7/16 23:48, Conor Dooley wrote:
> > On Tue, Jul 16, 2024 at 08:43:19PM +0800, Chen Wang wrote:
> > > On 2024/7/16 17:42, Haylen Chu wrote:
> > > > Add devicetree binding documentation for thermal sensors integrated in
> > > > Sophgo CV180X SoCs.
> > > > 
> > > > Signed-off-by: Haylen Chu <[e-mail]>
> > > > ---
> > > >    .../thermal/sophgo,cv1800-thermal.yaml        | 55 +++++++++++++++++++
> > > I see sometimes you call it cv1800, and in patch 3, the file name is
> > > cv180x_thermal.c, and for dts changes, you changed cv18xx.dtsi. Please unify
> > > it.
> > > 
> > > I think sg200x is new name for cv181x serias, so if you want to cover
> > > cv180x/sg200x, is cv18xx better?
> > > 
> > > >    1 file changed, 55 insertions(+)
> > > >    create mode 100644 Documentation/devicetree/bindings/thermal/sophgo,cv1800-thermal.yaml
> > > > 
> > > > diff --git a/Documentation/devicetree/bindings/thermal/sophgo,cv1800-thermal.yaml b/Documentation/devicetree/bindings/thermal/sophgo,cv1800-thermal.yaml
> > > > new file mode 100644
> > > > index 000000000000..58bd4432cd10
> > > > --- /dev/null
> > > > +++ b/Documentation/devicetree/bindings/thermal/sophgo,cv1800-thermal.yaml
> > > > @@ -0,0 +1,55 @@
> > > > +# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause)
> > > > +%YAML 1.2
> > > > +---
> > > > +$id: http://devicetree.org/schemas/thermal/sophgo,cv1800-thermal.yaml#
> > > > +$schema: http://devicetree.org/meta-schemas/core.yaml#
> > > > +
> > > > +title: Sophgo CV1800 on-SoC Thermal Sensor
> > > > +
> > > > +maintainers:
> > > > +  - Haylen Chu <[e-mail]>
> > > > +
> > > > +description: Sophgo CV1800 on-SoC thermal sensor
> > > > +
> > > > +properties:
> > > > +  compatible:
> > > > +    enum:
> > > > +      - sophgo,cv1800-thermal
> > > cv18xx-thermal ?
> > Please, no wildcards in compatibles :/
> 
> Sorry for my confusion.
> 
> Haylen, so you want a compatible that matches an actual SoC and use it
> everywhere?
> 

This should depend. If this peripheral is SoC specific, it is OK
for using SoC specific compatible. Otherwise, it should be series
specific.

For thermal sensors, I suggest using series-based compatible name
as this peripheral is the same across the whole series IIRC.

> Or we can add ones for each SoC and have a fallback to cv1800.

SoC specific compatible means most of the SoC have different part
for this peripheral. For safety, it may not use the fallback 
generic compatible.

Regards,
Inochi

Chen Wang July 17, 2024, 2:14 a.m. UTC | #6

On 2024/7/17 9:27, Inochi Amaoto wrote:
> On Wed, Jul 17, 2024 at 08:05:10AM GMT, Chen Wang wrote:
>> On 2024/7/16 23:48, Conor Dooley wrote:
>>> On Tue, Jul 16, 2024 at 08:43:19PM +0800, Chen Wang wrote:
>>>> On 2024/7/16 17:42, Haylen Chu wrote:
>>>>> Add devicetree binding documentation for thermal sensors integrated in
>>>>> Sophgo CV180X SoCs.
>>>>>
>>>>> Signed-off-by: Haylen Chu <[e-mail]>
>>>>> ---
>>>>>     .../thermal/sophgo,cv1800-thermal.yaml        | 55 +++++++++++++++++++
>>>> I see sometimes you call it cv1800, and in patch 3, the file name is
>>>> cv180x_thermal.c, and for dts changes, you changed cv18xx.dtsi. Please unify
>>>> it.
>>>>
>>>> I think sg200x is new name for cv181x serias, so if you want to cover
>>>> cv180x/sg200x, is cv18xx better?
>>>>
>>>>>     1 file changed, 55 insertions(+)
>>>>>     create mode 100644 Documentation/devicetree/bindings/thermal/sophgo,cv1800-thermal.yaml
>>>>>
>>>>> diff --git a/Documentation/devicetree/bindings/thermal/sophgo,cv1800-thermal.yaml b/Documentation/devicetree/bindings/thermal/sophgo,cv1800-thermal.yaml
>>>>> new file mode 100644
>>>>> index 000000000000..58bd4432cd10
>>>>> --- /dev/null
>>>>> +++ b/Documentation/devicetree/bindings/thermal/sophgo,cv1800-thermal.yaml
>>>>> @@ -0,0 +1,55 @@
>>>>> +# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause)
>>>>> +%YAML 1.2
>>>>> +---
>>>>> +$id: http://devicetree.org/schemas/thermal/sophgo,cv1800-thermal.yaml#
>>>>> +$schema: http://devicetree.org/meta-schemas/core.yaml#
>>>>> +
>>>>> +title: Sophgo CV1800 on-SoC Thermal Sensor
>>>>> +
>>>>> +maintainers:
>>>>> +  - Haylen Chu <[e-mail]>
>>>>> +
>>>>> +description: Sophgo CV1800 on-SoC thermal sensor
>>>>> +
>>>>> +properties:
>>>>> +  compatible:
>>>>> +    enum:
>>>>> +      - sophgo,cv1800-thermal
>>>> cv18xx-thermal ?
>>> Please, no wildcards in compatibles :/
>> Sorry for my confusion.
>>
>> Haylen, so you want a compatible that matches an actual SoC and use it
>> everywhere?
>>
> This should depend. If this peripheral is SoC specific, it is OK
> for using SoC specific compatible. Otherwise, it should be series
> specific.
>
> For thermal sensors, I suggest using series-based compatible name
> as this peripheral is the same across the whole series IIRC.

What's the  "series-based compatible name" do you mean? Can you give an 
example?

And allow me clarify, what I said "a compatible that matches an actual 
SoC and use it everywhere" means to define "sophgo,cv1800-thermal" just 
as Haylen did and use it for all cv18xx SoC chips.

Anyway, as Conor suggested, we'd better not use wildcards (char 'x') in 
compatibles.

Thanks,

Chen

>> Or we can add ones for each SoC and have a fallback to cv1800.
> SoC specific compatible means most of the SoC have different part
> for this peripheral. For safety, it may not use the fallback
> generic compatible.
>
> Regards,
> Inochi

Inochi Amaoto July 17, 2024, 2:27 a.m. UTC | #7

On Wed, Jul 17, 2024 at 10:14:46AM GMT, Chen Wang wrote:
> 
> On 2024/7/17 9:27, Inochi Amaoto wrote:
> > On Wed, Jul 17, 2024 at 08:05:10AM GMT, Chen Wang wrote:
> > > On 2024/7/16 23:48, Conor Dooley wrote:
> > > > On Tue, Jul 16, 2024 at 08:43:19PM +0800, Chen Wang wrote:
> > > > > On 2024/7/16 17:42, Haylen Chu wrote:
> > > > > > Add devicetree binding documentation for thermal sensors integrated in
> > > > > > Sophgo CV180X SoCs.
> > > > > > 
> > > > > > Signed-off-by: Haylen Chu <[e-mail]>
> > > > > > ---
> > > > > >     .../thermal/sophgo,cv1800-thermal.yaml        | 55 +++++++++++++++++++
> > > > > I see sometimes you call it cv1800, and in patch 3, the file name is
> > > > > cv180x_thermal.c, and for dts changes, you changed cv18xx.dtsi. Please unify
> > > > > it.
> > > > > 
> > > > > I think sg200x is new name for cv181x serias, so if you want to cover
> > > > > cv180x/sg200x, is cv18xx better?
> > > > > 
> > > > > >     1 file changed, 55 insertions(+)
> > > > > >     create mode 100644 Documentation/devicetree/bindings/thermal/sophgo,cv1800-thermal.yaml
> > > > > > 
> > > > > > diff --git a/Documentation/devicetree/bindings/thermal/sophgo,cv1800-thermal.yaml b/Documentation/devicetree/bindings/thermal/sophgo,cv1800-thermal.yaml
> > > > > > new file mode 100644
> > > > > > index 000000000000..58bd4432cd10
> > > > > > --- /dev/null
> > > > > > +++ b/Documentation/devicetree/bindings/thermal/sophgo,cv1800-thermal.yaml
> > > > > > @@ -0,0 +1,55 @@
> > > > > > +# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause)
> > > > > > +%YAML 1.2
> > > > > > +---
> > > > > > +$id: http://devicetree.org/schemas/thermal/sophgo,cv1800-thermal.yaml#
> > > > > > +$schema: http://devicetree.org/meta-schemas/core.yaml#
> > > > > > +
> > > > > > +title: Sophgo CV1800 on-SoC Thermal Sensor
> > > > > > +
> > > > > > +maintainers:
> > > > > > +  - Haylen Chu <[e-mail]>
> > > > > > +
> > > > > > +description: Sophgo CV1800 on-SoC thermal sensor
> > > > > > +
> > > > > > +properties:
> > > > > > +  compatible:
> > > > > > +    enum:
> > > > > > +      - sophgo,cv1800-thermal
> > > > > cv18xx-thermal ?
> > > > Please, no wildcards in compatibles :/
> > > Sorry for my confusion.
> > > 
> > > Haylen, so you want a compatible that matches an actual SoC and use it
> > > everywhere?
> > > 
> > This should depend. If this peripheral is SoC specific, it is OK
> > for using SoC specific compatible. Otherwise, it should be series
> > specific.
> > 
> > For thermal sensors, I suggest using series-based compatible name
> > as this peripheral is the same across the whole series IIRC.
> 
> What's the  "series-based compatible name" do you mean? Can you give an
> example?
> 

You can take clk as an exmaple. "cv1800-clk", "cv1810-clk" and 
"sg2000-clk". As you already know, sophgo use the same clk tree
for the same series.

> And allow me clarify, what I said "a compatible that matches an actual SoC
> and use it everywhere" means to define "sophgo,cv1800-thermal" just as
> Haylen did and use it for all cv18xx SoC chips.
> 

I know it. But there may be something like the pinctrl. They share the same
logic, but have different data and layout. In fact, I am sure the thermal 
is not suitable for this case.

> Anyway, as Conor suggested, we'd better not use wildcards (char 'x') in
> compatibles.
> 

Yeah, I agree.

> Thanks,
> 
> Chen
> 
> > > Or we can add ones for each SoC and have a fallback to cv1800.
> > SoC specific compatible means most of the SoC have different part
> > for this peripheral. For safety, it may not use the fallback
> > generic compatible.
> > 
> > Regards,
> > Inochi

Haylen Chu July 17, 2024, 5:19 a.m. UTC | #8

On Wed, Jul 17, 2024 at 08:05:10AM +0800, Chen Wang wrote:
> Haylen, so you want a compatible that matches an actual SoC and use it
> everywhere?
> 
> Or we can add ones for each SoC and have a fallback to cv1800.

I would prefer "sophgo,cv1800-thermal" and use it everywhere. I don't
see any difference on thermal sensors between cv18xx-series SoCs.

Best regards,
Haylen

Haylen Chu July 17, 2024, 5:30 a.m. UTC | #9

On Wed, Jul 17, 2024 at 10:27:35AM +0800, Inochi Amaoto wrote:
> > And allow me clarify, what I said "a compatible that matches an actual SoC
> > and use it everywhere" means to define "sophgo,cv1800-thermal" just as
> > Haylen did and use it for all cv18xx SoC chips.
> > 
> 
> I know it. But there may be something like the pinctrl. They share the same
> logic, but have different data and layout. In fact, I am sure the thermal 
> is not suitable for this case.

But for the thermal sensor, they are exactly the same, I don't think
it's worth to add compatible strings for each SoC. Even though later
SoCs do come with a different design, we could add a new one with
little pain.

Best regards,
Haylen

> > Anyway, as Conor suggested, we'd better not use wildcards (char 'x') in
> > compatibles.
> > 
> 
> Yeah, I agree.
> 
> > Thanks,
> > 
> > Chen
> > 
> > > > Or we can add ones for each SoC and have a fallback to cv1800.
> > > SoC specific compatible means most of the SoC have different part
> > > for this peripheral. For safety, it may not use the fallback
> > > generic compatible.
> > > 
> > > Regards,
> > > Inochi

Krzysztof Kozlowski July 17, 2024, 9:12 a.m. UTC | #10

On 17/07/2024 07:19, Haylen Chu wrote:
> On Wed, Jul 17, 2024 at 08:05:10AM +0800, Chen Wang wrote:
>> Haylen, so you want a compatible that matches an actual SoC and use it
>> everywhere?
>>
>> Or we can add ones for each SoC and have a fallback to cv1800.
> 
> I would prefer "sophgo,cv1800-thermal" and use it everywhere. I don't
> see any difference on thermal sensors between cv18xx-series SoCs.

Please use proper fallbacks - there is a very specific rule, repeated
many times:

https://elixir.bootlin.com/linux/v6.10-rc1/source/Documentation/devicetree/bindings/writing-bindings.rst#L42

Best regards,
Krzysztof

Haylen Chu July 18, 2024, 7:04 a.m. UTC | #11

On Wed, Jul 17, 2024 at 11:12:52AM +0200, Krzysztof Kozlowski wrote:
> On 17/07/2024 07:19, Haylen Chu wrote:
> > On Wed, Jul 17, 2024 at 08:05:10AM +0800, Chen Wang wrote:
> >> Haylen, so you want a compatible that matches an actual SoC and use it
> >> everywhere?
> >>
> >> Or we can add ones for each SoC and have a fallback to cv1800.
> > 
> > I would prefer "sophgo,cv1800-thermal" and use it everywhere. I don't
> > see any difference on thermal sensors between cv18xx-series SoCs.
> 
> Please use proper fallbacks - there is a very specific rule, repeated
> many times:
> 
> https://elixir.bootlin.com/linux/v6.10-rc1/source/Documentation/devicetree/bindings/writing-bindings.rst#L42

Just in case I misunderstood,

You would prefer different SoC-specific compatible strings like
"sophgo,cv1800-thermal" "sophgo,sg2002-thermal" added to the driver,
and each thermal-sensor node contains two compatible strings, one
matches the SoC exactly and one is "sophgo,cv1800-thermal" just as a
fallback, right?

Best regards,
Haylen Chu

Krzysztof Kozlowski July 18, 2024, 10:53 a.m. UTC | #12

On 18/07/2024 09:04, Haylen Chu wrote:
> On Wed, Jul 17, 2024 at 11:12:52AM +0200, Krzysztof Kozlowski wrote:
>> On 17/07/2024 07:19, Haylen Chu wrote:
>>> On Wed, Jul 17, 2024 at 08:05:10AM +0800, Chen Wang wrote:
>>>> Haylen, so you want a compatible that matches an actual SoC and use it
>>>> everywhere?
>>>>
>>>> Or we can add ones for each SoC and have a fallback to cv1800.
>>>
>>> I would prefer "sophgo,cv1800-thermal" and use it everywhere. I don't
>>> see any difference on thermal sensors between cv18xx-series SoCs.
>>
>> Please use proper fallbacks - there is a very specific rule, repeated
>> many times:
>>
>> https://elixir.bootlin.com/linux/v6.10-rc1/source/Documentation/devicetree/bindings/writing-bindings.rst#L42
> 
> Just in case I misunderstood,
> 
> You would prefer different SoC-specific compatible strings like
> "sophgo,cv1800-thermal" "sophgo,sg2002-thermal" added to the driver,

There is nothing in above - comment or guideline - mentioning drivers.

> and each thermal-sensor node contains two compatible strings, one
> matches the SoC exactly and one is "sophgo,cv1800-thermal" just as a
> fallback, right?



Best regards,
Krzysztof

Patch

diff --git a/Documentation/devicetree/bindings/thermal/sophgo,cv1800-thermal.yaml b/Documentation/devicetree/bindings/thermal/sophgo,cv1800-thermal.yaml
new file mode 100644
index 000000000000..58bd4432cd10
--- /dev/null
+++ b/Documentation/devicetree/bindings/thermal/sophgo,cv1800-thermal.yaml
@@ -0,0 +1,55 @@ 
+# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause)
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/thermal/sophgo,cv1800-thermal.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: Sophgo CV1800 on-SoC Thermal Sensor
+
+maintainers:
+  - Haylen Chu <[e-mail]>
+
+description: Sophgo CV1800 on-SoC thermal sensor
+
+properties:
+  compatible:
+    enum:
+      - sophgo,cv1800-thermal
+
+  reg:
+    maxItems: 1
+
+  clocks:
+    description: The thermal sensor clock
+
+  interrupts:
+    maxItems: 1
+
+  sample-rate-hz:
+    minimum: 1
+    maximum: 1908
+    default: 1
+
+  '#thermal-sensor-cells':
+    const: 0
+
+required:
+  - compatible
+  - reg
+  - clocks
+  - interrupts
+
+additionalProperties: false
+
+examples:
+  - |
+        #include <dt-bindings/clock/sophgo,cv1800.h>
+        #include <dt-bindings/interrupt-controller/irq.h>
+        thermal-sensor@30e0000 {
+            compatible = "sophgo,cv1800-thermal";
+            reg = <0x30e0000 0x100>;
+            clocks = <&clk CLK_TEMPSEN>;
+            interrupts = <16 IRQ_TYPE_LEVEL_HIGH>;
+            #thermal-sensor-cells = <0>;
+        };
+...