Re: [PATCH v3 1/3] dt-bindings: hwmon: pmbus: Add Maxim MAX31785 documentation

[Andrew Jeffery]

On Mon, 2017-09-18 at 14:26 -0500, Rob Herring wrote:
> On Fri, Sep 08, 2017 at 02:39:17PM +1000, Andrew Jeffery wrote:
> > > > Signed-off-by: Andrew Jeffery <andrew@xxxxxxxx>
> > ---
> > Â.../devicetree/bindings/hwmon/pmbus/max31785.txtÂÂÂ| 158 +++++++++++++++++++++
> 
> I think this needs to be located by what it does (fan control), not whatÂ
> interface it has (pmbus).
> 
> I'm not all that happy with hwmon either because things here seem toÂ
> just be based on being Linux hwmon devices which is sometimes arbitrary.

I'll follow-up on this on Guenter's reply if necessary.

> Â
> 
> > Â1 file changed, 158 insertions(+)
> > Âcreate mode 100644 Documentation/devicetree/bindings/hwmon/pmbus/max31785.txt
> > 
> > diff --git a/Documentation/devicetree/bindings/hwmon/pmbus/max31785.txt b/Documentation/devicetree/bindings/hwmon/pmbus/max31785.txt
> > new file mode 100644
> > index 000000000000..af9578e7742c
> > --- /dev/null
> > +++ b/Documentation/devicetree/bindings/hwmon/pmbus/max31785.txt
> > @@ -0,0 +1,158 @@
> > +Bindings for the Maxim MAX31785 Intelligent Fan Controller
> > +==========================================================
> > +
> > +Reference:
> > +
> > +https://datasheets.maximintegrated.com/en/ds/MAX31785.pdf
> > +
> > +Required properties:
> > +- compatibleÂÂÂÂÂ: One of "maxim,max31785" or "maxim,max31785a"
> > +- regÂÂÂÂÂÂÂÂÂÂÂÂ: I2C address, one of 0x52, 0x53, 0x54, 0x55.
> > +- #address-cells : Must be 1
> > +- #size-cellsÂÂÂÂ: Must be 0
> > +- #thermal-sensor-cellsÂÂ: Should be 1. The device supports:
> > +ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂ- One internal sensor
> > +ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂ- Four external I2C digital sensors
> > +ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂ- Six external thermal diodes
> 
> You should define the IDs here, not just how many of each type.

Okay.

> 
> > +
> > +Optional properties:
> > +- use-stored-presenceÂÂÂÂ: Do not treat the devicetree description as canon for
> > +ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂfan presence (the 'installed' bit of FAN_CONFIG_*).
> > +ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂInstead, rely on the on the default value store of
> > +ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂthe device to populate it.
> 
> Boolean? Please be explicit.

Okay.

> 
> Needs a vendor prefix.

We've discussed this previously. It's describing how to interpret part
of the PMBus spec, not something Maxim have done, so I don't think it
should have a vendor prefix.

But maybe I'm representing it wrong?

> 
> > +
> > +Capabilities are configured through subnodes of the controller's node.
> > +
> > +Fans
> > +----
> > +
> > +Only fans with subnodes present will be considered as installed. If
> > +use-stored-presence is present in the parent node, then only fans that are both
> > +defined in the devicetree and have their installed bit set are considered
> > +installed.
> > +
> > +Required subnode properties:
> > +- compatibleÂÂÂÂÂÂÂÂÂÂÂÂÂ: Must be "pmbus-fan"
> 
> "pmbus" is a property of the controller, not the fan, right? We shouldÂ
> have compatibles along the lines of the type of fan like 4-wire, 3-wire,Â
> etc.Â

Yes, PMBus is a property of the controller. But a controller that
implements PMBus fan control and monitoring abstracts away most of the
details of the fan hardware, so I don't see it fit to describe e.g. 4-
wire or 3-wire properties here.

Perhaps this is resolved by having a "pmbus-fan-control" compatible
node, and nesting a fan node under that? My concern would be that the
only element of the fan subnode would be the "tach-pulses" property
(though maybe also dual-tach if we generalise the maxim,fan-dual-tach
property below).

Regardless, what I'm trying to describe here with the non-vendor-
prefixed properties are configurable elements of the PMBus interface
for fan control. Adherence to PMBus by a device dictates the command
interface, and is therefore a property of the controller hardware at
the end of the day. In this instance (PMBus) I don't see how we can
draw the distinction between "what it does" and "what interface it has"
- adherence to relevant parts of the PMBus spec defines the minimum of
what the controller does (PMBus allows for vendor extensions).
Unfortunately the PMBus spec is fairly loose in terms of what it
*requires* you to implement, but I don't think that's relevant here.

> 
> > +- regÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂ: The PMBus page the properties apply to.
> > +- #cooling-cellsÂÂÂÂÂÂÂÂÂ: Should be 2. See the thermal bindings at [1].
> > +- maxim,fan-rotor-inputÂÂ: The type of rotor measurement provided to the
> > +ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂcontroller. Must be either "tach" for tachometer
> > +ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂpulses or "lock" for a locked-rotor signal.
> > +- maxim,fan-lock-polarity: Required iff maxim,fan-rotor-input is "lock". Valid
> > +ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂvalues are "low" for active low, "high" for active
> > +ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂhigh.
> > +
> > +Optional subnode properties:
> > +- fan-modeÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂ: "rpm" or "pwm". Default value is "pwm".
> > +- tach-pulsesÂÂÂÂÂÂÂÂÂÂÂÂ: Tachometer pulses per revolution. Valid values are
> > +ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂ1, 2, 3 or 4. The default is 1.
> > +- cooling-min-levelÂÂÂÂÂÂ: Smallest cooling state accepted. See [1].
> > +- cooling-max-levelÂÂÂÂÂÂ: Largest cooling state accepted. See [1].
> > +- maxim,fan-no-fault-ramp: Do not ramp the fan to 100% PWM duty on detecting a
> > +ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂfan fault
> > +- maxim,fan-startupÂÂÂÂÂÂ: The number of rotations required before taking
> > +ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂemergency action for an unresponsive fan and driving
> > +ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂit with 100% or 0% PWM duty, depending on the state
> > +ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂof maxim,fan-no-fault-ramp. Valid values are 0
> > +ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂ(automatic spin-up disabled), 2, 4, or 8. Default
> > +ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂvalue is 0.
> > +- maxim,fan-healthÂÂÂÂÂÂÂ: Enable automated fan health check
> > +- maxim,fan-rampÂÂÂÂÂÂÂÂÂ: Configures how fast the device ramps the PWM duty
> > +ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂcycle from one value to another. Valid values are 0
> > +ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂto 7 inclusive, with values 0 - 2 configuring a
> > +ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂ1000ms update rate and 1 - 3% duty respective duty
> > +ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂincrease, and 3 - 7 a 200ms update rate with a 1 -
> > +ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂ5% respective duty increase. Default value is 0.
> > +- maxim,fan-no-watchdogÂÂ: Do not ramp fan to 100% PWM duty on failure to
> > +ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂupdate desired fan rate inside 10s. This implies
> > +ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂmaxim,tmp-no-fault-ramp
> > +- maxim,tmp-no-fault-ramp: Do not ramp fan to 100% PWM duty on temperature
> > +ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂsensor fault detection. This implies
> > +ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂmaxim,fan-no-watchdog
> > +- maxim,tmp-hysteresisÂÂÂ: The temperature hysteresis used to determine
> > +ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂtransitions to lower fan speed bands in the
> > +ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂtemperature/fan rate lookup table. Valid values are
> > +ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂ2, 4, 6 or 8 (degrees celcius). Default value is 2.
> > +- maxim,fan-dual-tachÂÂÂÂ: Enable dual tachometer functionality
> > +- maxim,fan-pwm-freqÂÂÂÂÂ: The PWM frequency. Valid values are 30, 50, 100, 150
> > +ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂand 25000 (Hz). Default value is 30Hz.
> > +- maxim,fan-lookup-table : A 16-element cell array of alternating temperature
> > +ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂand rate values representing the look up table. The
> > +ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂrate units are set through the fan-mode property.
> > +- maxim,fan-fault-pin-mon: Ramp fans to 100% PWM duty when the FAULT pin is
> > +ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂasserted
> 
> In general, I think a good portion of these should be either implied byÂ
> a fan compatible string or be part of a common fan binding.

Above you made the distinction between fan and fan controller, but I
feel like following your suggestion here is mixing things up in the
opposite direction to what you desired above. In my mind most of these
properties describe a fan controller's capability rather than a
property of a fan, save say "tach-pulses", which is a property of the
fan's tacho, and maybe "maxim,fan-dual-tach" which applies to any dual-
rotor fan and should probably be generalised.

> 
> > +
> > +Temperature
> > +-----------
> > +
> > +Required subnode properties:
> > +- compatibleÂÂÂÂ: Must be "pmbus-temperature"
> > +- regÂÂÂÂÂÂÂÂÂÂÂ: The PMBus page the properties apply to.
> > +
> > +Optional subnode properties:
> > +- maxim,tmp-offsetÂÂÂÂÂÂ: Valid values are 0 - 30 (degrees celcius) inclusive.
> > +ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂDefault value is 0.
> > +- maxim,tmp-fansÂÂÂÂÂÂÂÂ: An array of phandles to fans controlled by the
> > +ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂcurrent temperature sensor.
> 
> Is this a feature of the Maxim chip or just a mapping of temperatureÂ
> sensors to fans. The latter should be a common property.

It's a manufacturer-specific feature of the Maxim chip, which can do
the full closed-loop fan control using a lookup table mapping
temperatures to fan speeds. The lookup table can be described with the
maxim,fan-lookup-table property specified above, and maxim,tmp-fans
maps the temperature sensor to a set of fans to control using its
temperature readings with respect to each fan's lookup table.

Cheers,

Andrew

> 
> Rob

Attachment: signature.asc
Description: This is a digitally signed message part