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

[Guenter Roeck <linux@roeck-us.net>]

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

Guenter