Re: [PATCH v4 1/2] dt-bindings: hwmon: Add starfive,jhb100-fan-tach

[Conor Dooley <conor@kernel.org>]

[-- Attachment #1: Type: text/plain, Size: 4550 bytes --]

On Mon, Sep 07, 2026 at 02:51:44AM +0000, Changhuang Liang wrote:
> Hi, Guenter, Conor
> 
> Thanks for the review.
> 
> > On 9/1/26 03:14, Conor Dooley wrote:
> > > On Tue, Sep 01, 2026 at 01:24:39AM +0000, Changhuang Liang wrote:
> > >>> On Sat, Aug 29, 2026 at 06:19:40PM -0700, Changhuang Liang wrote:
> > >>>> +patternProperties:
> > >>>> +  "^fan@[0-9a-f]+$":
> > >>>> +    $ref: fan-common.yaml#
> > >>>> +    unevaluatedProperties: false
> > >>>> +
> > >>>> +    properties:
> > >>>> +      reg:
> > >>>> +        description:
> > >>>> +          PWM channel index. The driver allows two fans to share
> > >>>> + the
> > >>> same
> > >>>> +          PWM channel, or each fan to use a dedicated channel.
> > >>>
> > >>> Doesn't matter what the driver can do, the description should
> > >>> describe what the hardware supports.
> > >>> pw-bot: changes-requested
> > >>> Does this fan-tach controller provide the PWMs?
> > >>> If so (although Guenter may correct me), I think the fan-tach
> > >>> controller needs to be.
> > >>>
> > >>> If you don't do that, I think you're going to run into problems with
> > >>> having multiple nodes with the same unit address when two fans share a
> > pwm?
> > >>>
> > >>> I think what you're supposed to do is drop "reg" and replace it with
> > >>> "pwms", but once again Guenter may correct me there.
> > >>> e.g. aspeed,g6-pwm-tach.yaml
> > >>>
> > >>
> > >> Perhaps I can refer to aspeed,g6-pwm-tach.yaml and change
> > >> "^fan@[0-9a-f]+$" to "^fan-[0-9]+$", which would remove the reg
> > >> property. In fact, the driver does not use reg either.
> > >>
> > >> Our fan-tach controller does not include PWM. The JHB100 SoC will
> > >> have a separate PWM controller. (This controller uses the same IP as
> > >> the JH7110 SoC, but there are some differences in driver
> > >> implementation.)
> > >>
> > >> The JHB100 has 8 PWM channels and 16 fan tach channels.
> > >>
> > >> So currently we expect the Device Tree to be configured like this:
> > >>
> > >> 	pwm0: pwm {
> > >> 		compatible = "starfive,jhb100-pwm";
> > >> 	};
> > >>
> > >
> > >>
> > >> 	fan0: pwm-fan0 {
> > >> 		compatible = "pwm-fan";
> > >> 		pwms = <&pwm0 0 40000 0>;
> > >> 	};
> > >
> > >>
> > >> fan-controller {
> > >> 	compatible = "starfive,jhb100-fan-tach";
> > >>
> > >> 	fan@0 {
> > >> 		tach-ch = <0x0>, <0x8>;
> > >> 	};
> > >
> > > Truncating this for readability, but it looks wrong to me. How does
> > > the feedback loop work here when there's no way to determine which fan
> > > is connected to a tach channel? The unit address of the child nodes
> > > has no dt enforced guarantee to line up with node names of the fans or
> > > pwm indices.
> > 
> > Normally (for other fan controllers) the fan would have a target speed.
> > The controller measures the speed and adjusts pwm output values until the
> > fan speed matches the expected value. The controller needs to know the
> > association between tachometer input and pwm output for this to work.
> > Typically (for classic fan controllers) that association is static.
> > In the Aspeed G6 fan controller it is dynamic/configurable.
> > 
> > I thought this is the case here as well, but I have no idea if that is correct (or if
> > there is a chip-internal feedback loop to start with).
> 
> Here is some information I found about fan tach and PWM control. It looks like these 
> two devices are managed through an application.
> 
> Do you have any new suggestions for modifications?

I, at least, have no new suggestions. I thought the most recent reply
from Guenter was pretty clear? If you've got a dedicated fan controller
with static association then you don't need to provide the association,
but if there's dynamic/configurable pwm-tach then you need to provide
the association so that the fan controller can work correctly?
To me, that read as if the pwms property would be required here, because
what's proposed above represents the same fan twice, as pwm-fan0 and
fan0 without an association between the two because you have an
different IP entirely providing the pwm.

For this to work, don't you actually need to do:
pwm0: pwm {
	compatible = "starfive,jhb100-pwm";
};

fan-controller {
	compatible = "starfive,jhb100-fan-tach";
	
	fan@0 {
		pwms = <&pwm0 0 40000 0>;
		tach-ch = <0x0>, <0x8>;
	};
};

Cheers,
Conor.

> 
> https://github.com/openbmc/phosphor-pid-control/blob/master/README.md
> 
> Best Regards,
> Changhuang
> 

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]