From: Francesco Dolcini <francesco.dolcini@toradex.com>
Add properties to describe the fan and the PWM controller output.
Link: https://www.ti.com/lit/gpn/amc6821
Signed-off-by: Francesco Dolcini <francesco.dolcini@toradex.com>
---
v3:
- explicitly describe the fan, use standard PWM and FAN bindings
- pwm.yaml cannot be referenced, because of the $nodename pattern that is
enforced there
v2: https://lore.kernel.org/all/20250224180801.128685-2-francesco@dolcini.it/
- no changes
v1: https://lore.kernel.org/all/20250218165633.106867-2-francesco@dolcini.it/
---
.../devicetree/bindings/hwmon/ti,amc6821.yaml | 14 +++++++++++++-
1 file changed, 13 insertions(+), 1 deletion(-)
diff --git a/Documentation/devicetree/bindings/hwmon/ti,amc6821.yaml b/Documentation/devicetree/bindings/hwmon/ti,amc6821.yaml
index 5d33f1a23d03..94aca9c378e6 100644
--- a/Documentation/devicetree/bindings/hwmon/ti,amc6821.yaml
+++ b/Documentation/devicetree/bindings/hwmon/ti,amc6821.yaml
@@ -28,6 +28,13 @@ properties:
i2c-mux:
type: object
+ fan:
+ $ref: fan-common.yaml#
+ unevaluatedProperties: false
+
+ "#pwm-cells":
+ const: 3
+
required:
- compatible
- reg
@@ -50,9 +57,14 @@ examples:
#address-cells = <1>;
#size-cells = <0>;
- fan@18 {
+ fan_controller: fan@18 {
compatible = "ti,amc6821";
reg = <0x18>;
+ #pwm-cells = <3>;
+
+ fan {
+ pwms = <&fan_controller 0 40000 0>;
+ };
};
};
--
2.39.5On Mon, Mar 31, 2025 at 05:52:28PM +0200, Francesco Dolcini wrote: > From: Francesco Dolcini <francesco.dolcini@toradex.com> > > Add properties to describe the fan and the PWM controller output. > > Link: https://www.ti.com/lit/gpn/amc6821 > Signed-off-by: Francesco Dolcini <francesco.dolcini@toradex.com> > --- > v3: > - explicitly describe the fan, use standard PWM and FAN bindings > - pwm.yaml cannot be referenced, because of the $nodename pattern that is > enforced there > v2: https://lore.kernel.org/all/20250224180801.128685-2-francesco@dolcini.it/ > - no changes > v1: https://lore.kernel.org/all/20250218165633.106867-2-francesco@dolcini.it/ > --- > .../devicetree/bindings/hwmon/ti,amc6821.yaml | 14 +++++++++++++- > 1 file changed, 13 insertions(+), 1 deletion(-) > > diff --git a/Documentation/devicetree/bindings/hwmon/ti,amc6821.yaml b/Documentation/devicetree/bindings/hwmon/ti,amc6821.yaml > index 5d33f1a23d03..94aca9c378e6 100644 > --- a/Documentation/devicetree/bindings/hwmon/ti,amc6821.yaml > +++ b/Documentation/devicetree/bindings/hwmon/ti,amc6821.yaml > @@ -28,6 +28,13 @@ properties: > i2c-mux: > type: object > > + fan: > + $ref: fan-common.yaml# > + unevaluatedProperties: false Why do you need the child, instead of referencing fan-common in the top level? Best regards, Krzysztof
On Tue, Apr 01, 2025 at 08:13:14AM +0200, Krzysztof Kozlowski wrote: > On Mon, Mar 31, 2025 at 05:52:28PM +0200, Francesco Dolcini wrote: > > From: Francesco Dolcini <francesco.dolcini@toradex.com> > > > > Add properties to describe the fan and the PWM controller output. > > > > Link: https://www.ti.com/lit/gpn/amc6821 > > Signed-off-by: Francesco Dolcini <francesco.dolcini@toradex.com> > > --- > > v3: > > - explicitly describe the fan, use standard PWM and FAN bindings > > - pwm.yaml cannot be referenced, because of the $nodename pattern that is > > enforced there > > v2: https://lore.kernel.org/all/20250224180801.128685-2-francesco@dolcini.it/ > > - no changes > > v1: https://lore.kernel.org/all/20250218165633.106867-2-francesco@dolcini.it/ > > --- > > .../devicetree/bindings/hwmon/ti,amc6821.yaml | 14 +++++++++++++- > > 1 file changed, 13 insertions(+), 1 deletion(-) > > > > diff --git a/Documentation/devicetree/bindings/hwmon/ti,amc6821.yaml b/Documentation/devicetree/bindings/hwmon/ti,amc6821.yaml > > index 5d33f1a23d03..94aca9c378e6 100644 > > --- a/Documentation/devicetree/bindings/hwmon/ti,amc6821.yaml > > +++ b/Documentation/devicetree/bindings/hwmon/ti,amc6821.yaml > > @@ -28,6 +28,13 @@ properties: > > i2c-mux: > > type: object > > > > + fan: > > + $ref: fan-common.yaml# > > + unevaluatedProperties: false > > Why do you need the child, instead of referencing fan-common in the top > level? Because shockingly a fan controller and a fan are 2 different pieces of h/w. It took *years* for this advancement to come to fruition. Rob
On Tue, Apr 01, 2025 at 08:13:14AM +0200, Krzysztof Kozlowski wrote:
> On Mon, Mar 31, 2025 at 05:52:28PM +0200, Francesco Dolcini wrote:
> > From: Francesco Dolcini <francesco.dolcini@toradex.com>
> >
> > Add properties to describe the fan and the PWM controller output.
> >
> > Link: https://www.ti.com/lit/gpn/amc6821
> > Signed-off-by: Francesco Dolcini <francesco.dolcini@toradex.com>
> > ---
> > v3:
> > - explicitly describe the fan, use standard PWM and FAN bindings
> > - pwm.yaml cannot be referenced, because of the $nodename pattern that is
> > enforced there
> > v2: https://lore.kernel.org/all/20250224180801.128685-2-francesco@dolcini.it/
> > - no changes
> > v1: https://lore.kernel.org/all/20250218165633.106867-2-francesco@dolcini.it/
> > ---
> > .../devicetree/bindings/hwmon/ti,amc6821.yaml | 14 +++++++++++++-
> > 1 file changed, 13 insertions(+), 1 deletion(-)
> >
> > diff --git a/Documentation/devicetree/bindings/hwmon/ti,amc6821.yaml b/Documentation/devicetree/bindings/hwmon/ti,amc6821.yaml
> > index 5d33f1a23d03..94aca9c378e6 100644
> > --- a/Documentation/devicetree/bindings/hwmon/ti,amc6821.yaml
> > +++ b/Documentation/devicetree/bindings/hwmon/ti,amc6821.yaml
> > @@ -28,6 +28,13 @@ properties:
> > i2c-mux:
> > type: object
> >
> > + fan:
> > + $ref: fan-common.yaml#
> > + unevaluatedProperties: false
>
> Why do you need the child, instead of referencing fan-common in the top
> level?
Two small reasons.
First is that the amc6821 is a fan controller, and the fan is just
connected to it. So having the fan as a child seemed the right way to
describe it, and this is done like that in other hwmon binding.
.. but now that you asked I tried to move the fan-common to the top
level and it's not working.
I added
allOf:
- $ref: fan-common.yaml#
at top level, removed the fan child, and moved the pwms up one level in
the example
i2c {
#address-cells = <1>;
#size-cells = <0>;
fan_controller: fan@18 {
compatible = "ti,amc6821";
reg = <0x18>;
#pwm-cells = <2>;
pwms = <&fan_controller 40000 0>;
};
};
and the binding checker is not fine with it
$ make dt_binding_check DT_SCHEMA_FILES=ti,amc6821.yaml
SCHEMA Documentation/devicetree/bindings/processed-schema.json
CHKDT ./Documentation/devicetree/bindings
LINT ./Documentation/devicetree/bindings
DTEX Documentation/devicetree/bindings/hwmon/ti,amc6821.example.dts
DTC [C] Documentation/devicetree/bindings/hwmon/ti,amc6821.example.dtb
/home/francesco/Toradex/sources/linux/Documentation/devicetree/bindings/hwmon/ti,amc6821.example.dtb: fan@18: 'pwms' does not match any of the regexes: 'pinctrl-[0-9]+'
from schema $id: http://devicetree.org/schemas/hwmon/ti,amc6821.yaml#
Any suggestion?
Francesco
On 4/1/25 00:43, Francesco Dolcini wrote:
> On Tue, Apr 01, 2025 at 08:13:14AM +0200, Krzysztof Kozlowski wrote:
>> On Mon, Mar 31, 2025 at 05:52:28PM +0200, Francesco Dolcini wrote:
>>> From: Francesco Dolcini <francesco.dolcini@toradex.com>
>>>
>>> Add properties to describe the fan and the PWM controller output.
>>>
>>> Link: https://www.ti.com/lit/gpn/amc6821
>>> Signed-off-by: Francesco Dolcini <francesco.dolcini@toradex.com>
>>> ---
>>> v3:
>>> - explicitly describe the fan, use standard PWM and FAN bindings
>>> - pwm.yaml cannot be referenced, because of the $nodename pattern that is
>>> enforced there
>>> v2: https://lore.kernel.org/all/20250224180801.128685-2-francesco@dolcini.it/
>>> - no changes
>>> v1: https://lore.kernel.org/all/20250218165633.106867-2-francesco@dolcini.it/
>>> ---
>>> .../devicetree/bindings/hwmon/ti,amc6821.yaml | 14 +++++++++++++-
>>> 1 file changed, 13 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/Documentation/devicetree/bindings/hwmon/ti,amc6821.yaml b/Documentation/devicetree/bindings/hwmon/ti,amc6821.yaml
>>> index 5d33f1a23d03..94aca9c378e6 100644
>>> --- a/Documentation/devicetree/bindings/hwmon/ti,amc6821.yaml
>>> +++ b/Documentation/devicetree/bindings/hwmon/ti,amc6821.yaml
>>> @@ -28,6 +28,13 @@ properties:
>>> i2c-mux:
>>> type: object
>>>
>>> + fan:
>>> + $ref: fan-common.yaml#
>>> + unevaluatedProperties: false
>>
>> Why do you need the child, instead of referencing fan-common in the top
>> level?
>
> Two small reasons.
>
> First is that the amc6821 is a fan controller, and the fan is just
> connected to it. So having the fan as a child seemed the right way to
> describe it, and this is done like that in other hwmon binding.
>
> .. but now that you asked I tried to move the fan-common to the top
> level and it's not working.
>
> I added
>
> allOf:
> - $ref: fan-common.yaml#
>
> at top level, removed the fan child, and moved the pwms up one level in
> the example
>
> i2c {
> #address-cells = <1>;
> #size-cells = <0>;
>
> fan_controller: fan@18 {
> compatible = "ti,amc6821";
> reg = <0x18>;
> #pwm-cells = <2>;
> pwms = <&fan_controller 40000 0>;
devicetree wants to see #pwm-cells one level above pwms. Obviously you can't
put it under the i2c node, so you need the sub-level. Maybe there is a way
around it, but if so I don't know what it might be.
Guenter
On 3/31/25 08:52, Francesco Dolcini wrote:
> From: Francesco Dolcini <francesco.dolcini@toradex.com>
>
> Add properties to describe the fan and the PWM controller output.
>
> Link: https://www.ti.com/lit/gpn/amc6821
> Signed-off-by: Francesco Dolcini <francesco.dolcini@toradex.com>
> ---
> v3:
> - explicitly describe the fan, use standard PWM and FAN bindings
> - pwm.yaml cannot be referenced, because of the $nodename pattern that is
> enforced there
> v2: https://lore.kernel.org/all/20250224180801.128685-2-francesco@dolcini.it/
> - no changes
> v1: https://lore.kernel.org/all/20250218165633.106867-2-francesco@dolcini.it/
> ---
> .../devicetree/bindings/hwmon/ti,amc6821.yaml | 14 +++++++++++++-
> 1 file changed, 13 insertions(+), 1 deletion(-)
>
> diff --git a/Documentation/devicetree/bindings/hwmon/ti,amc6821.yaml b/Documentation/devicetree/bindings/hwmon/ti,amc6821.yaml
> index 5d33f1a23d03..94aca9c378e6 100644
> --- a/Documentation/devicetree/bindings/hwmon/ti,amc6821.yaml
> +++ b/Documentation/devicetree/bindings/hwmon/ti,amc6821.yaml
> @@ -28,6 +28,13 @@ properties:
> i2c-mux:
> type: object
>
> + fan:
> + $ref: fan-common.yaml#
> + unevaluatedProperties: false
> +
> + "#pwm-cells":
> + const: 3
> +
> required:
> - compatible
> - reg
> @@ -50,9 +57,14 @@ examples:
> #address-cells = <1>;
> #size-cells = <0>;
>
> - fan@18 {
> + fan_controller: fan@18 {
> compatible = "ti,amc6821";
> reg = <0x18>;
> + #pwm-cells = <3>;
> +
> + fan {
> + pwms = <&fan_controller 0 40000 0>;
There is no explanation for the parameters. I guess that the first is the index and the
second is the frequency. The index is not used and can be dropped (there is just a single
channel), and the frequency only makes sense if it can actually be configured.
Either case, the fields need to be documented.
Thanks,
Guenter
> + };
> };
> };
>
Hello Guenter,
On Mon, Mar 31, 2025 at 09:01:08AM -0700, Guenter Roeck wrote:
> On 3/31/25 08:52, Francesco Dolcini wrote:
> > From: Francesco Dolcini <francesco.dolcini@toradex.com>
> >
> > Add properties to describe the fan and the PWM controller output.
> >
> > Link: https://www.ti.com/lit/gpn/amc6821
> > Signed-off-by: Francesco Dolcini <francesco.dolcini@toradex.com>
> > ---
> > v3:
> > - explicitly describe the fan, use standard PWM and FAN bindings
> > - pwm.yaml cannot be referenced, because of the $nodename pattern that is
> > enforced there
> > v2: https://lore.kernel.org/all/20250224180801.128685-2-francesco@dolcini.it/
> > - no changes
> > v1: https://lore.kernel.org/all/20250218165633.106867-2-francesco@dolcini.it/
> > ---
> > .../devicetree/bindings/hwmon/ti,amc6821.yaml | 14 +++++++++++++-
> > 1 file changed, 13 insertions(+), 1 deletion(-)
> >
> > diff --git a/Documentation/devicetree/bindings/hwmon/ti,amc6821.yaml b/Documentation/devicetree/bindings/hwmon/ti,amc6821.yaml
> > index 5d33f1a23d03..94aca9c378e6 100644
> > --- a/Documentation/devicetree/bindings/hwmon/ti,amc6821.yaml
> > +++ b/Documentation/devicetree/bindings/hwmon/ti,amc6821.yaml
> > @@ -28,6 +28,13 @@ properties:
> > i2c-mux:
> > type: object
> > + fan:
> > + $ref: fan-common.yaml#
> > + unevaluatedProperties: false
> > +
> > + "#pwm-cells":
> > + const: 3
> > +
> > required:
> > - compatible
> > - reg
> > @@ -50,9 +57,14 @@ examples:
> > #address-cells = <1>;
> > #size-cells = <0>;
> > - fan@18 {
> > + fan_controller: fan@18 {
> > compatible = "ti,amc6821";
> > reg = <0x18>;
> > + #pwm-cells = <3>;
> > +
> > + fan {
> > + pwms = <&fan_controller 0 40000 0>;
>
> There is no explanation for the parameters. I guess that the first is the index and the
> second is the frequency. The index is not used and can be dropped (there is just a single
> channel), and the frequency only makes sense if it can actually be configured.
The PWM period can be configured, the HW supports it, so I think is a
good idea to properly describe the HW now that I am modifying the
binding and not have the situation in which we need it in 1 month.
The instance is just one, it's correct, but there is an assumption
pretty much everywhere that these 3 #cells are
- PWM instance
- PWM period in ns
- PWM flags
I decided to keep the first cell there, with the PWM instance, for this
reason, even as you correctly wrote it would not be required here.
With that said, looking at existing binding, the description is almost
never present.
I would proceed this way for v4:
- keep the first cell as PWM instance
- add the description
Francesco
On 3/31/25 09:16, Francesco Dolcini wrote:
> Hello Guenter,
>
>
> On Mon, Mar 31, 2025 at 09:01:08AM -0700, Guenter Roeck wrote:
>> On 3/31/25 08:52, Francesco Dolcini wrote:
>>> From: Francesco Dolcini <francesco.dolcini@toradex.com>
>>>
>>> Add properties to describe the fan and the PWM controller output.
>>>
>>> Link: https://www.ti.com/lit/gpn/amc6821
>>> Signed-off-by: Francesco Dolcini <francesco.dolcini@toradex.com>
>>> ---
>>> v3:
>>> - explicitly describe the fan, use standard PWM and FAN bindings
>>> - pwm.yaml cannot be referenced, because of the $nodename pattern that is
>>> enforced there
>>> v2: https://lore.kernel.org/all/20250224180801.128685-2-francesco@dolcini.it/
>>> - no changes
>>> v1: https://lore.kernel.org/all/20250218165633.106867-2-francesco@dolcini.it/
>>> ---
>>> .../devicetree/bindings/hwmon/ti,amc6821.yaml | 14 +++++++++++++-
>>> 1 file changed, 13 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/Documentation/devicetree/bindings/hwmon/ti,amc6821.yaml b/Documentation/devicetree/bindings/hwmon/ti,amc6821.yaml
>>> index 5d33f1a23d03..94aca9c378e6 100644
>>> --- a/Documentation/devicetree/bindings/hwmon/ti,amc6821.yaml
>>> +++ b/Documentation/devicetree/bindings/hwmon/ti,amc6821.yaml
>>> @@ -28,6 +28,13 @@ properties:
>>> i2c-mux:
>>> type: object
>>> + fan:
>>> + $ref: fan-common.yaml#
>>> + unevaluatedProperties: false
>>> +
>>> + "#pwm-cells":
>>> + const: 3
>>> +
>>> required:
>>> - compatible
>>> - reg
>>> @@ -50,9 +57,14 @@ examples:
>>> #address-cells = <1>;
>>> #size-cells = <0>;
>>> - fan@18 {
>>> + fan_controller: fan@18 {
>>> compatible = "ti,amc6821";
>>> reg = <0x18>;
>>> + #pwm-cells = <3>;
>>> +
>>> + fan {
>>> + pwms = <&fan_controller 0 40000 0>;
>>
>> There is no explanation for the parameters. I guess that the first is the index and the
>> second is the frequency. The index is not used and can be dropped (there is just a single
>> channel), and the frequency only makes sense if it can actually be configured.
>
> The PWM period can be configured, the HW supports it, so I think is a
> good idea to properly describe the HW now that I am modifying the
> binding and not have the situation in which we need it in 1 month.
>
> The instance is just one, it's correct, but there is an assumption
> pretty much everywhere that these 3 #cells are
> - PWM instance
> - PWM period in ns
> - PWM flags
>
> I decided to keep the first cell there, with the PWM instance, for this
> reason, even as you correctly wrote it would not be required here.
>
> With that said, looking at existing binding, the description is almost
> never present.
>
That is not a valid argument.
> I would proceed this way for v4:
> - keep the first cell as PWM instance
> - add the description
>
We just had another driver where we dropped the pwm instance number because
it is unnecessary. See commit a25633a00906. I very much prefer to keep it that
way if it is not needed. Everything else is just confusing. It is bad enough
that the sub-node is needed. We should not make it worse.
Thanks,
Guenter
© 2016 - 2026 Red Hat, Inc.