Messages in this thread | | | Date | Fri, 22 Sep 2023 07:42:42 -0700 | From | Guenter Roeck <> | Subject | Re: [PATCH v2 2/2] dt-bindings: hwmon: add MAX31790 |
| |
On 9/22/23 02:53, Conor Dooley wrote: > On Fri, Sep 22, 2023 at 02:33:06AM +0000, Delphine_CC_Chiu/WYHQ/Wiwynn wrote: >>>> On Fri, Sep 15, 2023 at 02:29:24PM +0800, Delphine CC Chiu wrote: > >>>>> + pwm-as-tach: >>>> >>>> I don't see any other users of this in-tree, so you'd need a vendor >>>> prefix. That said, I'm once bitten, twice shy about fan related >>>> properties in hwmon, so I would definitely like Rob to comment on this >>>> whole binding. >>> >>> Please see this[1] and comment on it to ensure it meets your needs. >>> Otherwise, omit any fan related properties for now. >>> >> This property could only be used in max31790 driver. Would it be ok if we add >> vendor prefix like "maxim, pwm-as-tach"? > > I think the answer to this is a pretty straightforward no. The goal is > to create a set of common fan properties that works for multiple > usecases, not create one specifically for each user... >
Another chip with configurable channel configuration is nct7802, where individual channels can be configured as temperature or voltage sensor. We are using sensor-type to select the mode in that driver. Maybe something similar would make sense / be acceptable here.
>>>> +examples: >>>> + - | >>>> + i2c { >>>> + #address-cells = <1>; >>>> + #size-cells = <0>; >>>> + >>>> + pwm@20 { >>>> + compatible = "maxim,max31790"; >>>> + reg = <0x20>; >>>> + pwm-as-tach = <2 5>; >>> >>> This would be <2>, <5>; no? >>> >> I refer to the other binding documents in hwmon and most of them were using >> the format like <2 5> as an array. > > Which also makes this moot, since it'll be going away. > >>>> diff --git a/MAINTAINERS b/MAINTAINERS index >>>> c8fdd0d03907..97e13b6bf51d 100644 >>>> --- a/MAINTAINERS >>>> +++ b/MAINTAINERS >>>> @@ -1371,6 +1371,12 @@ F: >>> Documentation/devicetree/bindings/hwmon/adi,max31760.yaml >>>> F: Documentation/hwmon/max31760.rst >>>> F: drivers/hwmon/max31760.c >>>> >>>> +ANALOG DEVICES INC MAX31790 DRIVER >>>> +M: Delphine CC Chiu <Delphine_CC_Chiu@wiwynn.com> >>>> +S: Odd Fixes >>> >>> This is a pretty odd status for something you're newly adding. >>> How come it's not going to be maintained? >>> >> We are not the authors of this driver but we want to add a feature to >> config PWM as TACH that was descripted in the datasheet of MAX31790. >> Should we set the status to maintained? > > It's really up to you. I just found it curious & wanted to ask why it > was that way. >
It is misleading because it downgrades the driver from "supported" (like all other hwmon drivers) to "odd fixes".
Guenter
| |