On Mon, 2017-08-14 at 11:25 +0930, Andrew Jeffery wrote: > 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. Following up on this, there's not much that conflicts between the two fan descriptions. But there's not much in common either, just because there's really not that much to it all. In both cases the interesting bits are in the manufacturer specific properties. reg is applicable in either case. I require a compatible here to separate fan support from temperature sensing. What would you be looking for in a common fan description? However, as an aside, I think there's a fundamental problem with the Aspeed PWM/Tacho bindings. For reference, a vendor submitted this devicetree patch to OpenBMC: http://patchwork.ozlabs.org/patch/804862/ Particularly, I'd draw your attention to this part of the linked patch: + fan@0 { + reg = <0x00>; + cooling-levels = /bits/ 8 <125 151 177 203 229 255>; + aspeed,fan-tach-ch = /bits/ 8 <0x00>; + }; + + fan@1 { + reg = <0x00>; + aspeed,fan-tach-ch = /bits/ 8 <0x01>; + }; + + fan@2 { + reg = <0x00>; + aspeed,fan-tach-ch = /bits/ 8 <0x02>; + }; As outlined in my comments on the patch in the patchwork link above, the bindings are backwards for what is being described: Eight fans driven by one PWM, so there is a mismatch between the unit and reg. reg is being used to index the one PWM, and tach channels are being associated to the PWM. I rather think fans should be represented by their tach input (the tach ID assigned to reg), and a PWM channel be associated with the node via e.g. an aspeed,pwm-ch property. There is the issue of dual-tach fans, but in the case of the Aspeed PWM/Tach block the dual-tach lines would need to be wired to separate tach inputs, still leaving us free to select the appropriate tach input to drive the fan loop. There are other issues such as the incorrect suggestion for the value of #size-cells in the Aspeed PWM/Tach bindings. > > 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. I've done what I think I need to in order to resolve this. > > > > > > + > > > +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. I've addressed this. Cheers, Andrew
Attachment:
signature.asc
Description: This is a digitally signed message part
