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

[Conor Dooley <conor@kernel.org>]

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

On Mon, Aug 31, 2026 at 10:54:32AM -0700, Guenter Roeck wrote:
> On 8/31/26 09:32, Conor Dooley wrote:
> > On Sat, Aug 29, 2026 at 06:19:40PM -0700, Changhuang Liang wrote:
> > > Add compatible "starfive,jhb100-fan-tach" for StarFive JHB100 Fan-Tach
> > > controller.
> > > 
> > > Signed-off-by: Changhuang Liang <changhuang.liang@starfivetech.com>
> > > ---
> > >   .../hwmon/starfive,jhb100-fan-tach.yaml       | 93 +++++++++++++++++++
> > >   1 file changed, 93 insertions(+)
> > >   create mode 100644 Documentation/devicetree/bindings/hwmon/starfive,jhb100-fan-tach.yaml
> > > 
> > > diff --git a/Documentation/devicetree/bindings/hwmon/starfive,jhb100-fan-tach.yaml b/Documentation/devicetree/bindings/hwmon/starfive,jhb100-fan-tach.yaml
> > > new file mode 100644
> > > index 000000000000..1dcd4932768f
> > > --- /dev/null
> > > +++ b/Documentation/devicetree/bindings/hwmon/starfive,jhb100-fan-tach.yaml
> > > @@ -0,0 +1,93 @@
> > > +# SPDX-License-Identifier: GPL-2.0-only OR BSD-2-Clause
> > > +%YAML 1.2
> > > +---
> > > +$id: http://devicetree.org/schemas/hwmon/starfive,jhb100-fan-tach.yaml#
> > > +$schema: http://devicetree.org/meta-schemas/core.yaml#
> > > +
> > > +title: StarFive JHB100 Fan-Tach controller
> > > +
> > > +maintainers:
> > > +  - Changhuang Liang <changhuang.liang@starfivetech.com>
> > > +
> > > +description:
> > > +  The StarFive Fan-Tach controller can support up to 16 fan tach input.
> > > +
> > > +properties:
> > > +  compatible:
> > > +    enum:
> > > +      - starfive,jhb100-fan-tach
> > > +
> > > +  reg:
> > > +    maxItems: 1
> > > +
> > > +  clocks:
> > > +    maxItems: 1
> > > +
> > > +  resets:
> > > +    maxItems: 1
> > > +
> > > +  interrupts:
> > > +    maxItems: 1
> > > +
> > > +  "#address-cells":
> > > +    const: 1
> > > +
> > > +  "#size-cells":
> > > +    const: 0
> > > +
> > > +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?
> > 
> 
> Depends on how the controller works. Normally fan controllers have a feedback
> mechanism which can be either temperature -> pwm output or fan speed -> pwm output.
> I don't know how this controller works, specifically if the fan speed inputs are
> internally associated with pwm outputs or if they are completely independent
> of each other. If it is the latter, aspeed,g6-pwm-tach.yaml would probably
> be a good baseline.

Judging from the other reply, it probably isn't a good baseline because
this is not a pwm provider.

> > 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
> > 
> 
> Sorry, when it comes to devicetree properties, I am completely clueless.
> Usually the devicetree and pwm maintainers insist that all fan controllers
> are modeled as pwm controllers because they have pwm outputs. My approach
> is "use whatever bindings are acceptable by DT maintainers".

Would you mind taking a look at my question in the other reply about how
the feedback path works (or doesn't) in that case? My lack of
familiarity with the subsystem has me scratching my head about how the
tach would be used to adjust the pwm duty cycle with the dt setup
described there!

Thanks,
Conor.

> 
> Guenter
> 
> > Cheers,
> > Conor.
> > 
> > > +        maximum: 15
> > > +
> > > +      tach-ch:
> > > +        description:
> > > +          The tachometer channel(s) wired to the fan(s) driven by this PWM
> > > +          channel. Integer values in the range 0-15.
> > > +        minItems: 1
> > > +        maxItems: 2
> > > +        items:
> > > +          maximum: 15
> > > +
> > > +    required:
> > > +      - reg
> > > +      - tach-ch
> > > +
> > > +required:
> > > +  - compatible
> > > +  - reg
> > > +  - clocks
> > > +  - resets
> > > +  - interrupts
> > > +
> > > +additionalProperties: false
> > > +
> > > +examples:
> > > +  - |
> > > +    fan-controller@11be0000 {
> > > +        compatible = "starfive,jhb100-fan-tach";
> > > +        reg = <0x11be0000 0x400>;
> > > +        interrupts = <128>;
> > > +        clocks = <&per2crg 33>;
> > > +        resets = <&per2crg 5>;
> > > +        #address-cells = <1>;
> > > +        #size-cells = <0>;
> > > +
> > > +        fan@0 {
> > > +            reg = <0x0>;
> > > +            tach-ch = /bits/ 8 <0x0 0x8>;
> > > +            pulses-per-revolution = <2>;
> > > +        };
> > > +        fan@1 {
> > > +            reg = <0x1>;
> > > +            tach-ch = /bits/ 8 <0x1>;
> > > +            pulses-per-revolution = <2>;
> > > +        };
> > > +    };
> > > -- 
> > > 2.25.1
> > > 
> 

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