Re: [RFC PATCH v2 1/2] dt-bindings: hwmon: emc2305: Add fan-shutdown-percent property

[Conor Dooley]

On Fri, Mar 13, 2026 at 11:12:26AM -0700, Guenter Roeck wrote:
> On 3/13/26 10:38, Conor Dooley wrote:
> > On Fri, Mar 13, 2026 at 07:14:33PM +0200, Florin Leotescu wrote:
> > > On Thu, Mar 12, 2026 at 05:37:51PM +0000, Conor Dooley wrote:
> > > > On Thu, Mar 12, 2026 at 02:22:47PM +0200, florin.leotescu@xxxxxxxxxxx wrote:
> > > > > From: Florin Leotescu <florin.leotescu@xxxxxxx>
> > > > > 
> > > > > The EMC2305 fan controller supports multiple independent PWM fan
> > > > > outputs. Some systems require fans to enter a defined safe state
> > > > > during system shutdown or reboot handoff, until firmware or the next
> > > > > boot stage reconfigures the controller.
> > > > > 
> > > > > Add an optional "fan-shutdown-percent" property to fan child nodes
> > > > > allowing the shutdown fan speed to be configured per fan output.
> > > > > 
> > > > > Signed-off-by: Florin Leotescu <florin.leotescu@xxxxxxx>
> > > > > ---
> > > > >   .../devicetree/bindings/hwmon/microchip,emc2305.yaml      | 8 ++++++++
> > > > >   1 file changed, 8 insertions(+)
> > > > > 
> > > > > diff --git a/Documentation/devicetree/bindings/hwmon/microchip,emc2305.yaml b/Documentation/devicetree/bindings/hwmon/microchip,emc2305.yaml
> > > > > index d3f06ebc19fa..7bcadfab9fc4 100644
> > > > > --- a/Documentation/devicetree/bindings/hwmon/microchip,emc2305.yaml
> > > > > +++ b/Documentation/devicetree/bindings/hwmon/microchip,emc2305.yaml
> > > > > @@ -54,6 +54,12 @@ patternProperties:
> > > > >             The fan number used to determine the associated PWM channel.
> > > > >           maxItems: 1
> > > > > +      fan-shutdown-percent:
> > > > > +        description:
> > > > > +          Fan RPM in percent set during shutdown.
> > > > 
> > > > This sounds like something generic, that other devices might need and
> > > > should be in fan-common.yaml?
> > > > 
> > > Thanks for the feedback.
> > > 
> > > The property name was reused from pwm-fan.yaml where it already exists
> > > with the same meaning. I kept it local to minimize the scope of the change.
> > > 
> > > However, if you prefer it to be modelled as a common property, I can adjust it in the next revision.
> > 
> > I see. Makes me wonder why half the properties in pwm-fan aren't also in
> > fan-common.
> > 
> > I suppose there's a reason for it, carry on I guess.. Can be lifted if
> > there are more users other than you and pwm-fan.
> > 
> > > > > +        minimum: 0
> > > > > +        maximum: 100
> > > > > +
> > > > >       required:
> > > > >         - reg
> > > > > @@ -80,12 +86,14 @@ examples:
> > > > >               fan@0 {
> > > > >                   reg = <0x0>;
> > > > >                   pwms = <&fan_controller 26000 PWM_POLARITY_INVERTED 1>;
> > > > > +                fan-shutdown-percent = <100>;
> > 
> > Also leads to the question of why these devices aren't pwm-fans. Too
> > late now to change anything, I just wanna understand why.
> > 
> 
> They are dedicated fan controllers, with an internal tach -> pwm feedback
> loop. I don't mind modeling them as pwm controllers if there is a use case
> (such as someone using a PWM output to control a LED), but I don't even know
> how modeling them as pwm fans using the pwm-fan compatible would even be
> possible since that would exclude the tach -> pwm feedback loop.
> 
> Also, if that were to be done, we might end up with two hwmon drivers
> for one chip: one to report the fan speed, and one to report pwm values.
> On top of that, even if it is not currently supported by the driver,
> controlling the tach -> pwm feedback loop would become awkward.
> 
> Yes, I understand, there is a strong push to model everything with
> a pwm output as pwm controller, but si far no one was able to describe
> to me how to model a fan controller, and especially its feedback loop,
> in such a scenario. That becomes even more complicated if temperatures
> are used to control the feedback loop.

FWIW, my wording choice was poor here. I said that it was too late to
change anything in an attempt to convey that I didn't want it to be
changed, but it came across as implying that it was wrong.
I just wanted to know why it was the way it was, not change it.

Thanks for the info you provided, that's useful.

Attachment: signature.asc
Description: PGP signature