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

[Conor Dooley]

On Thu, Apr 25, 2024 at 05:46:02PM +0200, Krzysztof Kozlowski wrote:
> On 25/04/2024 16:05, Guenter Roeck wrote:
> > On 4/25/24 03:33, Chanh Nguyen wrote:
> >>
> >>
> >> On 24/04/2024 00:02, Conor Dooley wrote:
> >>> [EXTERNAL EMAIL NOTICE: This email originated from an external sender. Please be mindful of safe email handling and proprietary information protection practices.]
> >>>
> >>
> > 
> > The quote doesn't make much sense.

The reply is to me questioning part of their earlier reply:
	> I think a vendor property is suitable for this purpose. It is only available
	> on specific MAX31790 fan controllers and not on other fan controller
	> devices. So I think we can't use the "tach-ch" in fan-common.yaml.

	Can you explain why you think that only MAX31790 fan controllers are
	capable of an arbitrary mapping of PWM outputs to TACH inputs?

> > 
> >> Sorry Conor, there may be confusion here. I mean the mapping of the PWM output to the TACH input, which is on the MAX31790, and it is not sure a common feature on all fan controllers.
> >>
> > 
> > I think the term "mapping" is a bit confusing here.
> > 
> > tach-ch, as I understand it, is supposed to associate a tachometer input
> > with a pwm output, meaning the fan speed measured with the tachometer input
> > is expected to change if the pwm output changes.
> > 
> > On MAX31790, it is possible to configure a pwm output pin as tachometer input pin.
> > That is something completely different. Also, the association is fixed.
> > If the first pwm channel is used as tachometer channel, it would show up as 7th
> > tachometer channel. If the 6th pwm channel is configured to be used as tachometer
> > input, it would show up as 12th tachometer channel.
> > 
> > Overall, the total number of channels on MAX31790 is always 12. 6 of them
> > are always tachometer inputs, the others can be configured to either be a
> > pwm output or a tachometer input.
> >

> > 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.

Are they? Granted, I probably didn't read this as well as you did, but
figure 3, 4 5 & 6 seem to show mappings that are not 1:1, with PWMOUT1
controlling several fans, each of which report back via a different tach
channel. I think the fan controller binding accounts for this though:
the child nodes would reference the same PWM provider but have different
tach-ch values.

> > 
> >> 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.
> 
> Thanks for explanation. So this is basically pin controller function
> choice - kind of output or input, although not in terms of GPIO.
> 

> Shouldn't we have then fan children which will be consumers of PWMs?

In this case, the max31790 has the PWM hardware, so it would be the
provider...

> Having a consumer makes pin PWM output. Then tach-ch says which pins are
> tachometer for given fan? Just like aspeed,g6-pwm-tach.yaml has?

...and it seems to me like there should be several fan child nodes as in
the aspeed binding you mention here that are the consumers. The binding
as written seems to only support a single fan.
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 228 bytes
Desc: not available
URL: <http://lists.ozlabs.org/pipermail/openbmc/attachments/20240425/5bdbf26b/attachment-0001.sig>