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

[Changhuang Liang]

Hi, Conor

> 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>;
> 	};
> };
> 

Okay, I recently came across a similar example:
https://lore.kernel.org/all/20260921-axiado-ax3000-pwm-fan-v3-0-3def8b6ffec9@xxxxxxxxxx/
I'll refer to this for the next version.

Best Regards,
Changhuang