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

[Conor Dooley <conor@kernel.org>]

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

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.

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