Re: [PATCH v2 3/3] dt-bindings: hwmon: max31790: Add maxim,pwmout-pin-as-tach-input property

[Guenter Roeck]

On 6/7/24 09:47, Chanh Nguyen wrote:
> On 08/05/2024 23:47, Conor Dooley wrote:
> > On Wed, May 08, 2024 at 10:44:34AM +0700, Chanh Nguyen wrote:
> > > On 05/05/2024 22:40, Guenter Roeck wrote:
> > > > On 5/5/24 03:08, Chanh Nguyen wrote:
> > > > > On 25/04/2024 21:05, Guenter Roeck wrote:
> > > > > > On 4/25/24 03:33, Chanh Nguyen wrote:
> > > > > >
> > > > > > pwm outputs on MAX31790 are always tied to the matching
> > > > > > tachometer inputs
> > > > > > (pwm1 <--> tach1 etc) and can not be reconfigured, meaning tach-ch for
> > > > > > channel X would always be X.
> > > > > >
> > > > > > > I would like to open a discussion about whether we should
> > > > > > > use the tach-ch property on the fan-common.yaml
> > > > > > >
> > > > > > > I'm looking forward to hearing comments from everyone. For
> > > > > > > me, both tach-ch and vendor property are good.
> > > > > >
> > > > > > I am not even sure how to define tach-ch to mean "use the pwm output pin
> > > > > > associated with this tachometer input channel not as pwm output
> > > > > > but as tachometer input". That would be a boolean, not a number.
> > > > >
> > > > > Thank Guenter,
> > > > >
> > > > > I reviewed again the "tach-ch" property, which is used in the https://elixir.bootlin.com/linux/v6.9-rc6/source/Documentation/devicetree/bindings/hwmon/aspeed,g6-pwm-tach.yaml#L68
> > > > > and https://elixir.bootlin.com/linux/v6.9-rc6/source/drivers/hwmon/aspeed-g6-pwm-tach.c#L434
> > > > >
> > > > > That is something completely different from my purpose.
> > > >
> > > > Based on its definition, tach-ch is associated with fans, and it looks
> > > > like the .yaml file groups multiple sets of fans into a single
> > > > fan node.
> > > >
> > > > In the simple case that would be
> > > >       tach-ch = <1>
> > > > ...
> > > >       tach-ch = <12>
> > > >
> > > > or, if all fans are controlled by a single pwm
> > > >       tach-ch = <1 2 3 4 5 6 8 9 10 11 12>
> > > >
> > > > The existence of tachometer channel 7..12 implies that pwm channel
> > > > (tachometer
> > > > channel - 6) is used as tachometer channel. That should be sufficient to
> > > > program
> > > > the chip for that channel. All you'd have to do is to ensure that pwm
> > > > channel
> > > > "X" is not listed as tachometer channel "X + 6", and program pwm channel
> > > > "X - 6"
> > > > for tachometer channels 7..12 as tachometer channels.
> > >
> > > Hi Guenter,
> > >
> > > I applied the patch [2/3] in my patch series (https://lore.kernel.org/lkml/20240414042246.8681-3-chanh@xxxxxxxxxxxxxxxxxxxxxx/)
> > >
> > > My device tree is configured as below, I would like to configure PWMOUT pins
> > > 5 and 6 to become the tachometer input pins.
> > >
> > >         fan-controller@20 {
> > >           compatible = "maxim,max31790";
> > >           reg = <0x20>;
> > >           maxim,pwmout-pin-as-tach-input = /bits/ 8 <0 0 0 0 1 1>;
> > >         };
> >
> > Why are you still operating off a binding that looks like this? I
> > thought that both I and Krzysztof told you to go and take a look at how
> > the aspeed,g6-pwm-tach.yaml binding looped and do something similar
> > here. You'd end up with something like:
> >
> >          fan-controller@20 {
> >            compatible = "maxim,max31790";
> >            reg = <0x20>;
> >
> >            fan-0 {
> >              pwms = <&pwm-provider ...>;
> >              tach-ch = 6;
> >          };
> >
> >            fan-1 {
> >              pwms = <&pwm-provider ...>;
> >              tach-ch = 7;
> >          };
> > };
> >
> > You can, as tach-ch or pwms do not need to be unique, set multiple
> > channels up as using the same tachs and/or pwms.
> > In the case of this particular fan controller, I think that the max31790
> > is actually the pwm provider so you'd modify it something like:
> >
> >          pwm-provider: fan-controller@20 {
> >            compatible = "maxim,max31790";
> >            reg = <0x20>;
> >       #pwm-cells = <N>;
> >
> >            fan-0 {
> >              pwms = <&pwm-provider ...>;
> >              tach-ch = <6>;
> >          };
> >
> >            fan-1 {
> >              pwms = <&pwm-provider ...>;
> >              tach-ch = <7>;
> >          };
> > };
> >
> > I just wrote this in my mail client's editor, so it may not be not
> > valid, but it is how the fan bindings expect you to represent this kind
> > of scenario.
>
> My apologies for the late reply.
>
> Thank you, Conor, for your recommendation!
>
> I spend more time checking the aspeed,g6-pwm-tach.yaml . Finally, I'll support the child nodes by having different tach-ch values. My system is designed similar to Figure 6 (8 Tach Monitors, 4PMWs).
>
> I'm going to push the patch series v3 soon.
>
> This is a brief binding.
> ....
> properties:
>    compatible:
>      const: maxim,max31790
>
>    reg:
>      maxItems: 1
>
>    clocks:
>      maxItems: 1
>
>    resets:
>      maxItems: 1
>
> patternProperties:
>    "^fan-[0-9]+$":
>      $ref: fan-common.yaml#
>      unevaluatedProperties: false
>
> required:
>    - compatible
>    - reg
>
> additionalProperties: false
>
> examples:
>    - |
>      i2c {
>        #address-cells = <1>;
>        #size-cells = <0>;
>
>        pwm_provider: fan-controller@20 {
>          compatible = "maxim,max31790";
>          reg = <0x20>;
>          clocks = <&sys_clk>;
>          resets = <&reset 0>;
>
>          fan-0 {
>            pwms = <&pwm_provider 1>;
>            tach-ch = <1 2>;
>          };
>
>          fan-1 {
>            pwms = <&pwm_provider 2>;
>            tach-ch = <7 8>;
>          };
>        };
>      };
>
>
> As your example, I saw the #pwm-cells = <N> . Please let me know, what's the purpose of this property?

It is the number of fields in "pwms" after the provider reference.
In your case it would be 1 (the index). If the pwm has additional
configuration parameters such as the frequency or polarity there
would be additional entries.

Guenter