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

[Andrew Jeffery]

On Thu, 2017-08-10 at 11:13 -0500, Rob Herring wrote:
> On Wed, Aug 02, 2017 at 04:45:38PM +0930, Andrew Jeffery wrote:
> > > > Signed-off-by: Andrew Jeffery <andrew@xxxxxxxx>
> > ---
> > Â.../devicetree/bindings/hwmon/pmbus/max31785.txtÂÂÂ| 126 +++++++++++++++++++++
> > Â1 file changed, 126 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..8ddc4ea1958d
> > --- /dev/null
> > +++ b/Documentation/devicetree/bindings/hwmon/pmbus/max31785.txt
> > @@ -0,0 +1,126 @@
> > +Bindings for the Maxim MAX31785 Intelligent Fan Controller
> > +==========================================================
>Â
> This is the second fan controller binding I've seen recently. The otherÂ
> being hwmon/aspeed-pwm-tacho.txt. I think some of this needs to beÂ
> common. There's only so many types of fans, right?

Heh, you'd think so, I'll take a look. However much of this is driven by the
PMBus specification and the Aspeed PWM/Tacho isn't a PMBus-based device.

I was hesitant to start a generic PMBus bindings document without having more
PMBus devices with bindings, but maybe that's the way I should go? It would
make clear what's from the fan-control parts of the PMBus specification and
what's here for the purpose of supporting the MAX31785.

>Â
> And we have the thermal binding which you don't appear to tie into.

I'll look into that also.

>Â
> > +
> > +Reference:
> > +[1] 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
> > +
> > +Optional properties:
> > +- use-stored-presenceÂÂÂÂ: Do not treat the devicetree description as canon for
>Â
> Is this maxim specific? If so, needs a vendor prefix.

PMBus specifies two 8-bit registers of the same structure: FAN_CONFIG_1_2
and FAN_CONFIG_3_4. It's not intended to be manufacturer-specific.

A bit in each nibble of FAN_CONFIG_* is specified as marking whether or not the
fan is installed. The intent of this property is to define whether the consumer
should consider the devicetree as canonical, or the device. In the absence of
the property consumer of the node should mark fans described in the devicetree
as installed (set the bit, and clear the bit for any fan pages that are not
described in the devicetree). Alternatively, the device may be pre-programmed
with a default register value that is suitable for the system the controller
resides in, in which case the devicetree consumer should itself consume the
installed bit from the device rather than set/clear it.

The two remaining fields in FAN_CONFIG_*, fan mode (RPM/PWM) and
tach-pulses-per-revolution (1-4), are covered by optional properties below.

>Â
> > +ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂfan presence (the 'installed' bit of FAN_CONFIG_*).
> > +ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂInstead, rely on the on the default value store of
> > +ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂthe device to populate it.
> > +
> > +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"
>Â
> What defines a pmbus-fan? Sounds generic, but then you have all theseÂ
> maxim specific properties.

It's driven by the two optional properties, fan-mode and tach-pulses, which
make up the remaining fields of the FAN_CONFIG_* registers from the PMBus
specification. PMBus reserves command ranges for manufacturer-specific use, and
Maxim chose to use one of these reserved commands as MFR_FAN_CONFIG
(Manufacturer-specific Fan Configuration). The vendor-prefixed properties deal
with the properties described in the 16-bit MFR_FAN_CONFIG register.

>Â
> > +- regÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂ: The PMBus page the properties apply to.
> > +- 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.
> > +- 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
> > +
> > +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 fan indexes whose fans are controlled by
> > +ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂthe current temperature sensor. Fans are indexed from
> > +ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂzero. The valid values are 0 - 5 inclusive.
>Â
> Why not use a phandle here.

I think that's a better approach. I'll rework it.

Thanks for the feedback.

Andrew

>Â
> > +
> > +Example:
> > > > > > +	fan-max31785: max31785@52 {
> > > > +		reg = <0x52>;
> > > > +		compatible = "maxim,max31785";
> > +ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂ#address-cells = <1>;
> > +ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂ#size-cells = <0>;
> > +
> > > > +ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂfan@0 {
> > +ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂcompatible = "pmbus-fan";
> > +ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂreg = <0>;
> > +ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂmode = "rpm";
> > +ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂtach-pulses = <1>;
> > +ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂmaxim,fan-rotor-input = "tach";
> > +ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂmaxim,fan-dual-tach;
> > +ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂ};
> > +
> > > > +ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂfan@1 {
> > +ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂcompatible = "pmbus-fan";
> > +ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂreg = <1>;
> > +ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂmode = "rpm";
> > +ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂtach-pulses = <1>;
> > +ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂmaxim,fan-rotor-input = "tach";
> > +ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂmaxim,tmp-hysteresis = <2>;
> > +ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂmaxim,fan-lookup-table = <
> > +ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂ/*ÂÂTemperatureÂÂÂÂRPMÂÂ*/
> > +ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂ0ÂÂÂÂÂÂÂÂ1000
> > +ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂ10ÂÂÂÂÂÂÂÂ2000
> > +ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂ20ÂÂÂÂÂÂÂÂ3000
> > +ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂ30ÂÂÂÂÂÂÂÂ4000
> > +ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂ40ÂÂÂÂÂÂÂÂ5000
> > +ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂ50ÂÂÂÂÂÂÂÂ6000
> > +ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂ60ÂÂÂÂÂÂÂÂ7000
> > +ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂ70ÂÂÂÂÂÂÂÂ8000
> > +ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂ>;
> > +ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂ};
> > > > +	};
> > --Â
> > 2.11.0
> >Â

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