| Message ID | 20240311111347.23067-2-chanh@os.amperecomputing.com |
|---|---|
| State | New |
| Headers | show |
| Series | Update the max31790 driver | expand |
On 11/03/2024 23:55, Krzysztof Kozlowski wrote: > On 11/03/2024 12:13, Chanh Nguyen wrote: >> Add a device tree bindings for max31790 device. > > Subject: drop "driver", bindings are about hardware. > Yes, I'll drop "driver" at v2 updating. > It does not look like you tested the bindings, at least after quick > look. Please run `make dt_binding_check` (see > Documentation/devicetree/bindings/writing-schema.rst for instructions). > Maybe you need to update your dtschema and yamllint. > I tested the binding, I didn't see any warning/error log. Please review my logs as below => make dt_binding_check DT_SCHEMA_FILES=/hwmon/max31790.yaml make[1]: Entering directory '/DISK4T/work/community/linux/out' DTEX Documentation/devicetree/bindings/hwmon/max31790.example.dts DTC_CHK Documentation/devicetree/bindings/hwmon/max31790.example.dtb make[1]: Leaving directory '/DISK4T/work/community/linux/out' >> >> Signed-off-by: Chanh Nguyen <chanh@os.amperecomputing.com> >> --- >> .../devicetree/bindings/hwmon/max31790.yaml | 44 +++++++++++++++++++ >> 1 file changed, 44 insertions(+) >> create mode 100644 Documentation/devicetree/bindings/hwmon/max31790.yaml >> >> diff --git a/Documentation/devicetree/bindings/hwmon/max31790.yaml b/Documentation/devicetree/bindings/hwmon/max31790.yaml >> new file mode 100644 >> index 000000000000..5a93e6bdebda >> --- /dev/null >> +++ b/Documentation/devicetree/bindings/hwmon/max31790.yaml > > Filename like compatible. Yes, I'll update that in v2 > >> @@ -0,0 +1,44 @@ >> +# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause) >> +%YAML 1.2 >> +--- >> +$id: http://devicetree.org/schemas/hwmon/max31790.yaml# >> +$schema: http://devicetree.org/meta-schemas/core.yaml# >> + >> +title: The Maxim MAX31790 Fan Controller >> + >> +maintainers: >> + - Jean Delvare <jdelvare@suse.com> >> + - Guenter Roeck <linux@roeck-us.net> > > You should have here someone responsible for hardware, not subsystem > maintainers. > Hi Krzysztof, I checked the history of the drivers/hwmon/max31790.c and see Guenter Roeck <linux@roeck-us.net> as an important maintainer. I saw many commits from him. So, I add him to maintainer list. >> + >> +description: > >> + The MAX31790 controls the speeds of up to six fans using six >> + independent PWM outputs. The desired fan speeds (or PWM duty cycles) >> + are written through the I2C interface. >> + >> + Datasheets: >> + https://datasheets.maximintegrated.com/en/ds/MAX31790.pdf >> + >> +properties: >> + compatible: >> + const: maxim,max31790 >> + >> + reg: >> + maxItems: 1 > > That's weirdly empty. > Hi Krzysztof, I have not yet understood your comment here. Please help give more details for my missing! Thank Krzysztof! >> + >> +required: >> + - compatible >> + - reg >> + > > You miss allOf: with $ref to fan controller schema. > Thank Krzysztof, I'll add the allOf at v2. >> +additionalProperties: false >> + >> +examples: >> + - | >> + i2c { >> + #address-cells = <1>; >> + #size-cells = <0>; >> + >> + max31790@20 { > > Node names should be generic. See also an explanation and list of > examples (not exhaustive) in DT specification: > https://devicetree-specification.readthedocs.io/en/latest/chapter2-devicetree-basics.html#generic-names-recommendation > I suggest some node names, such as "i2c-fan" or "fan-controller" . Can you please share your ideas with me! > > Best regards, > Krzysztof >
On 18/03/2024 17:00, Krzysztof Kozlowski wrote: > On 18/03/2024 10:51, Chanh Nguyen wrote: >> >>> It does not look like you tested the bindings, at least after quick >>> look. Please run `make dt_binding_check` (see >>> Documentation/devicetree/bindings/writing-schema.rst for instructions). >>> Maybe you need to update your dtschema and yamllint. >>> >> >> >> I tested the binding, I didn't see any warning/error log. Please review >> my logs as below > > Hm, I don't remember what brought my attention to possible error. Maybe > I mistyped my template. > >> >> => make dt_binding_check DT_SCHEMA_FILES=/hwmon/max31790.yaml >> make[1]: Entering directory '/DISK4T/work/community/linux/out' >> DTEX Documentation/devicetree/bindings/hwmon/max31790.example.dts >> DTC_CHK Documentation/devicetree/bindings/hwmon/max31790.example.dtb >> make[1]: Leaving directory '/DISK4T/work/community/linux/out' >> >>>> >>>> Signed-off-by: Chanh Nguyen <chanh@os.amperecomputing.com> >>>> --- >>>> .../devicetree/bindings/hwmon/max31790.yaml | 44 +++++++++++++++++++ >>>> 1 file changed, 44 insertions(+) >>>> create mode 100644 Documentation/devicetree/bindings/hwmon/max31790.yaml >>>> >>>> diff --git a/Documentation/devicetree/bindings/hwmon/max31790.yaml b/Documentation/devicetree/bindings/hwmon/max31790.yaml >>>> new file mode 100644 >>>> index 000000000000..5a93e6bdebda >>>> --- /dev/null >>>> +++ b/Documentation/devicetree/bindings/hwmon/max31790.yaml >>> >>> Filename like compatible. >> >> Yes, I'll update that in v2 >> >>> >>>> @@ -0,0 +1,44 @@ >>>> +# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause) >>>> +%YAML 1.2 >>>> +--- >>>> +$id: http://devicetree.org/schemas/hwmon/max31790.yaml# >>>> +$schema: http://devicetree.org/meta-schemas/core.yaml# >>>> + >>>> +title: The Maxim MAX31790 Fan Controller >>>> + >>>> +maintainers: >>>> + - Jean Delvare <jdelvare@suse.com> >>>> + - Guenter Roeck <linux@roeck-us.net> >>> >>> You should have here someone responsible for hardware, not subsystem >>> maintainers. >>> >> >> Hi Krzysztof, >> I checked the history of the drivers/hwmon/max31790.c and see Guenter >> Roeck <linux@roeck-us.net> as an important maintainer. I saw many >> commits from him. So, I add him to maintainer list. > > OK > >> >>>> + >>>> +description: > >>>> + The MAX31790 controls the speeds of up to six fans using six >>>> + independent PWM outputs. The desired fan speeds (or PWM duty cycles) >>>> + are written through the I2C interface. >>>> + >>>> + Datasheets: >>>> + https://datasheets.maximintegrated.com/en/ds/MAX31790.pdf >>>> + >>>> +properties: >>>> + compatible: >>>> + const: maxim,max31790 >>>> + >>>> + reg: >>>> + maxItems: 1 >>> >>> That's weirdly empty. >>> >> >> Hi Krzysztof, >> I have not yet understood your comment here. Please help give more >> details for my missing! Thank Krzysztof! > > I expect many more properties of a fan controller. Resources (clocks, > PWMs, supplies) and FAN specific properties. > Hi Krzysztof, I'm creating a base binding document for the max31790 driver. I'm basing it on the drivers/hwmon/max31790.c. Currently, the max31790.c driver has not yet implemented other properties, such as clocks, fan-supply, pwms, etc. So I just introduced the "compatible" and "reg" properties. In the near future, if any other properties are necessary, I think we will implement them in drivers/hwmon/max31790.c then update this binding document. I look at other binding documents, I also see something similar. They just introduce the "compatible" and "reg" properties. https://git.kernel.org/pub/scm/linux/kernel/git/next/linux-next.git/tree/Documentation/devicetree/bindings/hwmon/adi,max31760.yaml https://git.kernel.org/pub/scm/linux/kernel/git/next/linux-next.git/tree/Documentation/devicetree/bindings/hwmon/adt7475.yaml https://git.kernel.org/pub/scm/linux/kernel/git/next/linux-next.git/tree/Documentation/devicetree/bindings/hwmon/adi,ad741x.yaml This is only my view. It's a pleasure to hear your advice. Thanks! > >>>> + >>>> +required: >>>> + - compatible >>>> + - reg >>>> + >>> >>> You miss allOf: with $ref to fan controller schema. >>> >> >> Thank Krzysztof, >> I'll add the allOf at v2. >> >>>> +additionalProperties: false >>>> + >>>> +examples: >>>> + - | >>>> + i2c { >>>> + #address-cells = <1>; >>>> + #size-cells = <0>; >>>> + >>>> + max31790@20 { >>> >>> Node names should be generic. See also an explanation and list of >>> examples (not exhaustive) in DT specification: >>> https://devicetree-specification.readthedocs.io/en/latest/chapter2-devicetree-basics.html#generic-names-recommendation >>> >> >> I suggest some node names, such as "i2c-fan" or "fan-controller" . Can >> you please share your ideas with me! > > Look at recent commits and patches for similar type of a device. > Hi Krzysztof, I checked on recent commits and found something of a similar type. adi,max31760.yaml fan-controller@50 { reg = <0x50>; compatible = "adi,max31760"; }; hpe,gxp-fan-ctrl.yaml fan-controller@1000c00 { compatible = "hpe,gxp-fan-ctrl"; reg = <0x1000c00 0x200>, <0xd1000000 0xff>, <0x80200000 0x100000>; reg-names = "base", "pl", "fn2"; }; adi,axi-fan-control.yaml axi_fan_control: axi-fan-control@80000000 { compatible = "adi,axi-fan-control-1.00.a"; reg = <0x0 0x80000000 0x10000>; clocks = <&clk 71>; interrupts = <0 110 0>; pulses-per-revolution = <2>; }; I think "fan-controller" is a good node name. Do you think so? > Best regards, > Krzysztof >
On 25/03/2024 15:32, Krzysztof Kozlowski wrote: > On 22/03/2024 10:53, Chanh Nguyen wrote: >>>>> >>>> >>>> Hi Krzysztof, >>>> I have not yet understood your comment here. Please help give more >>>> details for my missing! Thank Krzysztof! >>> >>> I expect many more properties of a fan controller. Resources (clocks, >>> PWMs, supplies) and FAN specific properties. >>> >> >> Hi Krzysztof, >> >> I'm creating a base binding document for the max31790 driver. I'm basing >> it on the drivers/hwmon/max31790.c. Currently, the max31790.c driver has > > Binding should be based on device (e.g. its datasheet), not the driver. > Thank Krzysztof, I'm reading the writing-bindings.rst and I got it for now. I'll make complete binding in patch v2. I am very pleased to hear your comments. >> not yet implemented other properties, such as clocks, fan-supply, pwms, >> etc. So I just introduced the "compatible" and "reg" properties. >> >> In the near future, if any other properties are necessary, I think we >> will implement them in drivers/hwmon/max31790.c then update this binding >> document. > > Please instead read: > Documentation/devicetree/bindings/writing-bindings.rst > >> >> I look at other binding documents, I also see something similar. They >> just introduce the "compatible" and "reg" properties. >> >> https://git.kernel.org/pub/scm/linux/kernel/git/next/linux-next.git/tree/Documentation/devicetree/bindings/hwmon/adi,max31760.yaml > > Maybe these devices are similar, maybe not. This should not be excuse to > come with really incomplete binding. > > ... > >> I think "fan-controller" is a good node name. Do you think so? >> > > Yes. > > Best regards, > Krzysztof >
diff --git a/Documentation/devicetree/bindings/hwmon/max31790.yaml b/Documentation/devicetree/bindings/hwmon/max31790.yaml new file mode 100644 index 000000000000..5a93e6bdebda --- /dev/null +++ b/Documentation/devicetree/bindings/hwmon/max31790.yaml @@ -0,0 +1,44 @@ +# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause) +%YAML 1.2 +--- +$id: http://devicetree.org/schemas/hwmon/max31790.yaml# +$schema: http://devicetree.org/meta-schemas/core.yaml# + +title: The Maxim MAX31790 Fan Controller + +maintainers: + - Jean Delvare <jdelvare@suse.com> + - Guenter Roeck <linux@roeck-us.net> + +description: > + The MAX31790 controls the speeds of up to six fans using six + independent PWM outputs. The desired fan speeds (or PWM duty cycles) + are written through the I2C interface. + + Datasheets: + https://datasheets.maximintegrated.com/en/ds/MAX31790.pdf + +properties: + compatible: + const: maxim,max31790 + + reg: + maxItems: 1 + +required: + - compatible + - reg + +additionalProperties: false + +examples: + - | + i2c { + #address-cells = <1>; + #size-cells = <0>; + + max31790@20 { + compatible = "maxim,max31790"; + reg = <0x20>; + }; + };
Add a device tree bindings for max31790 device. Signed-off-by: Chanh Nguyen <chanh@os.amperecomputing.com> --- .../devicetree/bindings/hwmon/max31790.yaml | 44 +++++++++++++++++++ 1 file changed, 44 insertions(+) create mode 100644 Documentation/devicetree/bindings/hwmon/max31790.yaml