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

[Changhuang Liang]

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?

https://github.com/openbmc/phosphor-pid-control/blob/master/README.md

Best Regards,
Changhuang