| Message ID | 20240918103212.591204-1-chanh@os.amperecomputing.com (mailing list archive) |
|---|---|
| State | Changes Requested |
| Headers | show |
| Series | dt-bindings: hwmon: Add adt7462 | expand |
On 18/09/2024 12:32, Chanh Nguyen wrote: > Add device tree binding and example for adt7462 device. > > Signed-off-by: Chanh Nguyen <chanh@os.amperecomputing.com> > --- > .../bindings/hwmon/onnn,adt7462.yaml | 51 +++++++++++++++++++ > 1 file changed, 51 insertions(+) Where is any user? This is supposed to be sent along driver change implementing this compatible. > create mode 100644 Documentation/devicetree/bindings/hwmon/onnn,adt7462.yaml > > diff --git a/Documentation/devicetree/bindings/hwmon/onnn,adt7462.yaml b/Documentation/devicetree/bindings/hwmon/onnn,adt7462.yaml > new file mode 100644 > index 000000000000..4a980cca419a Binding looks ok. Best regards, Krzysztof
On 18/09/2024 20:09, Krzysztof Kozlowski wrote: > On 18/09/2024 12:32, Chanh Nguyen wrote: >> Add device tree binding and example for adt7462 device. >> >> Signed-off-by: Chanh Nguyen <chanh@os.amperecomputing.com> >> --- >> .../bindings/hwmon/onnn,adt7462.yaml | 51 +++++++++++++++++++ >> 1 file changed, 51 insertions(+) > > Where is any user? This is supposed to be sent along driver change > implementing this compatible. > I'm using this device on my platform, and I'm preparing upstream my device tree. So, I pushed the dt binding before. I'm also quite surprised that there aren't any platforms using adt7462 yet. >> create mode 100644 Documentation/devicetree/bindings/hwmon/onnn,adt7462.yaml >> >> diff --git a/Documentation/devicetree/bindings/hwmon/onnn,adt7462.yaml b/Documentation/devicetree/bindings/hwmon/onnn,adt7462.yaml >> new file mode 100644 >> index 000000000000..4a980cca419a > > Binding looks ok. > Thank Krzysztof! > > Best regards, > Krzysztof >
On 9/18/24 09:07, Chanh Nguyen wrote: > > > On 18/09/2024 20:09, Krzysztof Kozlowski wrote: >> On 18/09/2024 12:32, Chanh Nguyen wrote: >>> Add device tree binding and example for adt7462 device. >>> >>> Signed-off-by: Chanh Nguyen <chanh@os.amperecomputing.com> >>> --- >>> .../bindings/hwmon/onnn,adt7462.yaml | 51 +++++++++++++++++++ >>> 1 file changed, 51 insertions(+) >> >> Where is any user? This is supposed to be sent along driver change >> implementing this compatible. >> > > I'm using this device on my platform, and I'm preparing upstream my device tree. So, I pushed the dt binding before. > > I'm also quite surprised that there aren't any platforms using adt7462 yet. > I am sure there are (or used to be) platforms using it, only there are possibly no _devicetree_ based platforms using it. After all, the chip is old. The driver was added back in 2008, and the first version of the datasheet was published in 2006. Guenter
On Wed, Sep 18, 2024 at 10:32:12AM +0000, Chanh Nguyen wrote: > Add device tree binding and example for adt7462 device. > > Signed-off-by: Chanh Nguyen <chanh@os.amperecomputing.com> > --- > .../bindings/hwmon/onnn,adt7462.yaml | 51 +++++++++++++++++++ > 1 file changed, 51 insertions(+) > create mode 100644 Documentation/devicetree/bindings/hwmon/onnn,adt7462.yaml > > diff --git a/Documentation/devicetree/bindings/hwmon/onnn,adt7462.yaml b/Documentation/devicetree/bindings/hwmon/onnn,adt7462.yaml > new file mode 100644 > index 000000000000..4a980cca419a > --- /dev/null > +++ b/Documentation/devicetree/bindings/hwmon/onnn,adt7462.yaml > @@ -0,0 +1,51 @@ > +# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause) > +%YAML 1.2 > +--- > +$id: http://devicetree.org/schemas/hwmon/onnn,adt7462.yaml# > +$schema: http://devicetree.org/meta-schemas/core.yaml# > + > +title: ON Semiconductor ADT7462 Temperature, Voltage Monitor and Fan Controller > + > +maintainers: > + - Chanh Nguyen <chanh@os.amperecomputing.com> > + > +description: | > + The ADT7462 has temperature monitors, voltage monitors and multiple PWN Fan > + controllers. > + > + The ADT7462 supports monitoring and controlling up to four PWM Fan Drive > + Outputs and eight TACH Inputs Measures. The ADT7462 supports reading a single > + on chip temperature sensor and three remote temperature sensors. There are up > + to 13 voltage monitoring inputs, ranging from 12V to 0.9V. > + > + Datasheets: > + https://www.onsemi.com/pub/Collateral/ADT7462-D.PDF > + > +properties: > + compatible: > + const: onnn,adt7462 > + > + reg: > + maxItems: 1 > + > + resets: > + maxItems: 1 How would this work? 'resets' generally is used for on-chip devices and a reset controller. That doesn't exist at the board level. A standalone device typically uses a GPIO lines if there's a s/w controlled reset. That would be the 'reset-gpios' property. Rob
On Wed, Sep 18, 2024 at 03:09:23PM +0200, Krzysztof Kozlowski wrote: > On 18/09/2024 12:32, Chanh Nguyen wrote: > > Add device tree binding and example for adt7462 device. > > > > Signed-off-by: Chanh Nguyen <chanh@os.amperecomputing.com> > > --- > > .../bindings/hwmon/onnn,adt7462.yaml | 51 +++++++++++++++++++ > > 1 file changed, 51 insertions(+) > > Where is any user? This is supposed to be sent along driver change > implementing this compatible. Since I2C subsystem will strip the vendor prefix and match "adt7462" to the existing i2c_device_id, no driver change is needed for it to work. Though for good hygene, an of_device_id entry should be added. Rob
On 19/09/2024 05:05, Rob Herring wrote: > On Wed, Sep 18, 2024 at 10:32:12AM +0000, Chanh Nguyen wrote: >> Add device tree binding and example for adt7462 device. >> >> Signed-off-by: Chanh Nguyen <chanh@os.amperecomputing.com> >> --- >> .../bindings/hwmon/onnn,adt7462.yaml | 51 +++++++++++++++++++ >> 1 file changed, 51 insertions(+) >> create mode 100644 Documentation/devicetree/bindings/hwmon/onnn,adt7462.yaml >> >> diff --git a/Documentation/devicetree/bindings/hwmon/onnn,adt7462.yaml b/Documentation/devicetree/bindings/hwmon/onnn,adt7462.yaml >> new file mode 100644 >> index 000000000000..4a980cca419a >> --- /dev/null >> +++ b/Documentation/devicetree/bindings/hwmon/onnn,adt7462.yaml >> @@ -0,0 +1,51 @@ >> +# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause) >> +%YAML 1.2 >> +--- >> +$id: http://devicetree.org/schemas/hwmon/onnn,adt7462.yaml# >> +$schema: http://devicetree.org/meta-schemas/core.yaml# >> + >> +title: ON Semiconductor ADT7462 Temperature, Voltage Monitor and Fan Controller >> + >> +maintainers: >> + - Chanh Nguyen <chanh@os.amperecomputing.com> >> + >> +description: | >> + The ADT7462 has temperature monitors, voltage monitors and multiple PWN Fan >> + controllers. >> + >> + The ADT7462 supports monitoring and controlling up to four PWM Fan Drive >> + Outputs and eight TACH Inputs Measures. The ADT7462 supports reading a single >> + on chip temperature sensor and three remote temperature sensors. There are up >> + to 13 voltage monitoring inputs, ranging from 12V to 0.9V. >> + >> + Datasheets: >> + https://www.onsemi.com/pub/Collateral/ADT7462-D.PDF >> + >> +properties: >> + compatible: >> + const: onnn,adt7462 >> + >> + reg: >> + maxItems: 1 >> + >> + resets: >> + maxItems: 1 > > How would this work? 'resets' generally is used for on-chip devices and > a reset controller. That doesn't exist at the board level. A standalone > device typically uses a GPIO lines if there's a s/w controlled reset. > That would be the 'reset-gpios' property. > Thank Rob for your comments! The ADT7462 includes an active low reset pin (Pin #14). I'll change 'resets' into the 'reset-gpios' property. The example in the binding will be examples: - | i2c { #address-cells = <1>; #size-cells = <0>; hwmon@5c { compatible = "onnn,adt7462"; reg = <0x5c>; reset-gpios = <&gpio0 1 GPIO_ACTIVE_LOW>; }; }; > Rob
On 19/09/2024 11:43, Chanh Nguyen wrote: >>> +properties: >>> + compatible: >>> + const: onnn,adt7462 >>> + >>> + reg: >>> + maxItems: 1 >>> + >>> + resets: >>> + maxItems: 1 >> >> How would this work? 'resets' generally is used for on-chip devices and >> a reset controller. That doesn't exist at the board level. A standalone >> device typically uses a GPIO lines if there's a s/w controlled reset. >> That would be the 'reset-gpios' property. >> > > Thank Rob for your comments! The ADT7462 includes an active low reset > pin (Pin #14). > > I'll change 'resets' into the 'reset-gpios' property. > > The example in the binding will be The question how did it work in the first place is still valid... I think we might benefit from asking people to post their upstreamed DTS. Otherwise we will take broken or half-baked bindings, because we never saw the bigger picture. :( Best regards, Krzysztof
On 19/09/2024 17:39, Krzysztof Kozlowski wrote: > [EXTERNAL EMAIL NOTICE: This email originated from an external sender. Please be mindful of safe email handling and proprietary information protection practices.] > > > On 19/09/2024 11:43, Chanh Nguyen wrote: >>>> +properties: >>>> + compatible: >>>> + const: onnn,adt7462 >>>> + >>>> + reg: >>>> + maxItems: 1 >>>> + >>>> + resets: >>>> + maxItems: 1 >>> >>> How would this work? 'resets' generally is used for on-chip devices and >>> a reset controller. That doesn't exist at the board level. A standalone >>> device typically uses a GPIO lines if there's a s/w controlled reset. >>> That would be the 'reset-gpios' property. >>> >> >> Thank Rob for your comments! The ADT7462 includes an active low reset >> pin (Pin #14). >> >> I'll change 'resets' into the 'reset-gpios' property. >> >> The example in the binding will be > > The question how did it work in the first place is still valid... I > think we might benefit from asking people to post their upstreamed DTS. > Otherwise we will take broken or half-baked bindings, because we never > saw the bigger picture. :( > Thank Krzysztof, I saw the ADT7462 includes an active low reset pin (Pin #14) to reset device via I/O pin. So, I introduced a reset property follow the device datasheet. But the adt7462 driver has not yet implemented this property. My platform also doesn't design this pin on board, so I don't need it at least now. Anyway, I hope Rob, Guenter and Krzysztof can give me advice on whether I should add this property to the binding? Thanks, Chanh > Best regards, > Krzysztof >
On 9/19/24 08:02, Chanh Nguyen wrote: > > > On 19/09/2024 17:39, Krzysztof Kozlowski wrote: >> [EXTERNAL EMAIL NOTICE: This email originated from an external sender. Please be mindful of safe email handling and proprietary information protection practices.] >> >> >> On 19/09/2024 11:43, Chanh Nguyen wrote: >>>>> +properties: >>>>> + compatible: >>>>> + const: onnn,adt7462 >>>>> + >>>>> + reg: >>>>> + maxItems: 1 >>>>> + >>>>> + resets: >>>>> + maxItems: 1 >>>> >>>> How would this work? 'resets' generally is used for on-chip devices and >>>> a reset controller. That doesn't exist at the board level. A standalone >>>> device typically uses a GPIO lines if there's a s/w controlled reset. >>>> That would be the 'reset-gpios' property. >>>> >>> >>> Thank Rob for your comments! The ADT7462 includes an active low reset >>> pin (Pin #14). >>> >>> I'll change 'resets' into the 'reset-gpios' property. >>> >>> The example in the binding will be >> >> The question how did it work in the first place is still valid... I >> think we might benefit from asking people to post their upstreamed DTS. >> Otherwise we will take broken or half-baked bindings, because we never >> saw the bigger picture. :( >> > > Thank Krzysztof, > > I saw the ADT7462 includes an active low reset pin (Pin #14) to reset device via I/O pin. So, I introduced a reset property follow the device datasheet. > > But the adt7462 driver has not yet implemented this property. My platform also doesn't design this pin on board, so I don't need it at least now. > > Anyway, I hope Rob, Guenter and Krzysztof can give me advice on whether I should add this property to the binding? > Not from my perspective, and I won't let you add it to the driver unless you can actually test it. Really, this is such an old chip that it would make more sense to just leave its driver alone unless there is a problem with it. Why didn't you just add the chip to the list of trivial devices ? Guenter
On 19/09/2024 22:14, Guenter Roeck wrote: > On 9/19/24 08:02, Chanh Nguyen wrote: >> >> >> On 19/09/2024 17:39, Krzysztof Kozlowski wrote: >>> [EXTERNAL EMAIL NOTICE: This email originated from an external >>> sender. Please be mindful of safe email handling and proprietary >>> information protection practices.] >>> >>> >>> On 19/09/2024 11:43, Chanh Nguyen wrote: >>>>>> +properties: >>>>>> + compatible: >>>>>> + const: onnn,adt7462 >>>>>> + >>>>>> + reg: >>>>>> + maxItems: 1 >>>>>> + >>>>>> + resets: >>>>>> + maxItems: 1 >>>>> >>>>> How would this work? 'resets' generally is used for on-chip devices >>>>> and >>>>> a reset controller. That doesn't exist at the board level. A >>>>> standalone >>>>> device typically uses a GPIO lines if there's a s/w controlled reset. >>>>> That would be the 'reset-gpios' property. >>>>> >>>> >>>> Thank Rob for your comments! The ADT7462 includes an active low reset >>>> pin (Pin #14). >>>> >>>> I'll change 'resets' into the 'reset-gpios' property. >>>> >>>> The example in the binding will be >>> >>> The question how did it work in the first place is still valid... I >>> think we might benefit from asking people to post their upstreamed DTS. >>> Otherwise we will take broken or half-baked bindings, because we never >>> saw the bigger picture. :( >>> >> >> Thank Krzysztof, >> >> I saw the ADT7462 includes an active low reset pin (Pin #14) to reset >> device via I/O pin. So, I introduced a reset property follow the >> device datasheet. >> >> But the adt7462 driver has not yet implemented this property. My >> platform also doesn't design this pin on board, so I don't need it at >> least now. >> >> Anyway, I hope Rob, Guenter and Krzysztof can give me advice on >> whether I should add this property to the binding? >> > > Not from my perspective, and I won't let you add it to the driver unless > you can > actually test it. Really, this is such an old chip that it would make > more sense > to just leave its driver alone unless there is a problem with it. Why > didn't you > just add the chip to the list of trivial devices ? > No Guenter, I don't think I'll change or update anythings to the adt7462 driver. The driver is working fine for me. I just wonder about this property in the dt-binding. It seems I shouldn't add the reset property to the dt-binding. "Why didn't you just add the chip to the list of trivial devices ?" => Awesome, I just learned about the trivial-devices.yaml from your suggestion (https://github.com/torvalds/linux/blob/master/Documentation/devicetree/bindings/trivial-devices.yaml), this is thing that I'm finding for the adt7462 binding. Perfect! I'm looking forward to hearing Rob and Krzysztof's idea. If nothing else, I'll add this chip to the list of trivial devices in the patch v2. Thanks, Chanh > Guenter >
diff --git a/Documentation/devicetree/bindings/hwmon/onnn,adt7462.yaml b/Documentation/devicetree/bindings/hwmon/onnn,adt7462.yaml new file mode 100644 index 000000000000..4a980cca419a --- /dev/null +++ b/Documentation/devicetree/bindings/hwmon/onnn,adt7462.yaml @@ -0,0 +1,51 @@ +# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause) +%YAML 1.2 +--- +$id: http://devicetree.org/schemas/hwmon/onnn,adt7462.yaml# +$schema: http://devicetree.org/meta-schemas/core.yaml# + +title: ON Semiconductor ADT7462 Temperature, Voltage Monitor and Fan Controller + +maintainers: + - Chanh Nguyen <chanh@os.amperecomputing.com> + +description: | + The ADT7462 has temperature monitors, voltage monitors and multiple PWN Fan + controllers. + + The ADT7462 supports monitoring and controlling up to four PWM Fan Drive + Outputs and eight TACH Inputs Measures. The ADT7462 supports reading a single + on chip temperature sensor and three remote temperature sensors. There are up + to 13 voltage monitoring inputs, ranging from 12V to 0.9V. + + Datasheets: + https://www.onsemi.com/pub/Collateral/ADT7462-D.PDF + +properties: + compatible: + const: onnn,adt7462 + + reg: + maxItems: 1 + + resets: + maxItems: 1 + +required: + - compatible + - reg + +additionalProperties: false + +examples: + - | + i2c { + #address-cells = <1>; + #size-cells = <0>; + + hwmon@5c { + compatible = "onnn,adt7462"; + reg = <0x5c>; + resets = <&reset 0>; + }; + };
Add device tree binding and example for adt7462 device. Signed-off-by: Chanh Nguyen <chanh@os.amperecomputing.com> --- .../bindings/hwmon/onnn,adt7462.yaml | 51 +++++++++++++++++++ 1 file changed, 51 insertions(+) create mode 100644 Documentation/devicetree/bindings/hwmon/onnn,adt7462.yaml