On Wed, Mar 28, 2018 at 12:17:34PM -0700, Tim Harvey wrote: > On Wed, Mar 28, 2018 at 9:24 AM, Guenter Roeck <linux@xxxxxxxxxxxx> wrote: > > On Wed, Mar 28, 2018 at 08:14:00AM -0700, Tim Harvey wrote: > >> This patch adds documentation of device-tree bindings for the > >> Gateworks System Controller (GSC). > >> > >> Signed-off-by: Tim Harvey <tharvey@xxxxxxxxxxxxx> > >> --- > >> v3: > >> - replaced _ with - > >> - remove input bindings > >> - added full description of hwmon > >> - fix unit address of hwmon child nodes > >> > >> --- > >> .../devicetree/bindings/mfd/gateworks-gsc.txt | 135 +++++++++++++++++++++ > >> 1 file changed, 135 insertions(+) > >> create mode 100644 Documentation/devicetree/bindings/mfd/gateworks-gsc.txt > >> > >> diff --git a/Documentation/devicetree/bindings/mfd/gateworks-gsc.txt b/Documentation/devicetree/bindings/mfd/gateworks-gsc.txt > >> new file mode 100644 > >> index 0000000..8f530ed > >> --- /dev/null > >> +++ b/Documentation/devicetree/bindings/mfd/gateworks-gsc.txt > >> @@ -0,0 +1,135 @@ > >> +Gateworks System Controller multi-function device > >> + > >> +The GSC is a Multifunction I2C slave device with the following submodules: > >> +- WDT > >> +- GPIO > >> +- Pushbutton controller > >> +- HWMON > >> + > >> +Required properties: > >> +- compatible : Must be "gw,gsc" > >> +- reg: I2C address of the device > >> +- interrupts: interrupt triggered by GSC_IRQ# signal > >> +- interrupt-parent: Interrupt controller GSC is connected to > >> +- #interrupt-cells: should be <1>, index of the interrupt within the > >> + controller, in accordance with the "one cell" variant of > >> + <devicetree/bindings/interrupt-controller/interrupt.txt> > >> + > >> +Optional nodes: > >> +* watchdog: > >> +The GSC provides a Watchdog monitor which can power cycle the board's > >> +primary power supply on most board models when tripped. > >> + > >> +Required watchdog properties: > >> +- compatible: must be "gw,gsc-watchdog" > >> + > >> +* hwmon: > >> +The GSC provides a set of Analog to Digitcal Converter (ADC) pins used for > >> +temperature and/or voltage monitoring. > >> + > >> +Required hwmon properties: > >> +- compatible: must be "gw,gsc-hwmon" > >> + > > > > "hwmon" is a very Linux specific term. It might make sense to find a more > > generic term. > > The 'hwmon' driver supports child nodes that fall into the following category: > - temperature sensor (GSC internal temperature sensor - i2c registers > returns value in C*10) > - voltage rails (two types here; cooked: i2c registers return > pre-scaled value in mV), raw: i2c registers return a raw ADC value > that must be scaled based on ADC internal ref voltage and resolution > and adjusted for a voltage divider to convert to mV > - fan setpoints (I'll explain these below) > > I called the node 'gw,gsc-hwmon' because the driver fits into the > 'hwmon' API. Isn't that appropriate here for the driver compatible > string? > Devicetree properties are supposed to be OS independent. > > > >> +Optional hwmon properties: > >> +- gw,reference-voltage: ADC reference voltage (mV) used in scaling raw ADCs > > > > AFAIK devicetree likes to specify voltages in uV. > > There are currently plenty of dt props specified in mV (grep -r mV > Documentation/devicetree/bindings/). > "But so many others are speeding, why do I get a ticket ?" Please discuss with Rob. > > > >> +- gw,resolution: ADC resolution (ie 4096) used in scaling raw ADCs > >> + > > > > 4096 what ? > > reference-voltage and resolution are used to scale the values from the > nodes that report a raw ADC value: > > V = Vadc * (reference-voltage / resolution) > > I can provide that in bits if it makes more sense? I can also hard Yes, I think that would make more sense, and please describe what it means. > code both the resolution and the vref in the hwmon driver and remove > it from dt as currently the only GSC that uses raw ADC values is 12bit > with 2.5V ref. > That would be even better. > > > >> +Each hwmon child node defines an ADC input on the chip which the GSC may > >> +report cooked values (ie temperature sensor based on thermister), raw values, > >> +(ie voltage rail with a pre-scaling resistor divider), or a fan controller > >> +setpoint. > >> + > >> +Required hwmon child properties: > >> +- type: one of the following ADC types: > >> + "gw,hwmon-temperature" - reports temperature in C*10 > >> + "gw,hwmon-voltage" - reports a pre-scaled voltage value > >> + "gw,hwmon-voltage-raw" - reports a raw ADC that is scaled with > >> + vreference, resolution, and optional resistor divider > >> + "gw,hwmon-fan" - a fan temperature setpoint in C*10 > > > > What is a "fan temperature setpoint" ? > > > > The GSC supports a fan controller which drives a PWM signal to vary > the speed of a fan based on the GSC internal temperature sensor. The > FAN controller has 6 setpoints each having a fixed PWM duty-cycle but > the temperature at which those setpoints kick in can be varies via > registers at the 0x29 slave address (same slave address as the > temperature sensor and voltage inputs which is why I have it in the > hwmon driver): > > fan0_point - 50% PWM (default 300) > fan1_point - 60% PWM (default 330) > fan2_point - 70% PWM (default 360) > fan3_point - 80% PWM (default 390) > fan4_point - 90% PWM (default 420) > fan5_point - 100% PWM (default 450) > > The values are C/10 thus if the internal GSC temp sensor is below 30C > the fan output will be 0% duty cycle and if it hits 30C it will go to > 50% until it hits 60% at 33C etc. > Please do not define your own scaling factors. pwm values are 0..255, and temperatures are in milli-degrees C. > That is the hardware implementation that I'm trying to abstract and > define here. You pointed out the fact that the fan*_input ABI is > read-only fan PWM and I see that now. What do you suggest I use for No, it isn't. It is the fan speed in RPM. > this feature I'm trying to implement driver support for? > pwm[1-*]_auto_point[1-*]_pwm pwm[1-*]_auto_point[1-*]_temp pwm[1-*]_auto_point[1-*]_temp_hyst may be relevant. From the context, something like pwm1_auto_point1_pwm read-only, set to 128 pwm1_auto_point1_temp 30000 pwm1_auto_point2_pwm read-only, set to 153 pwm1_auto_point2_temp 33000 pwm1_auto_point3_pwm read-only, set to 179 pwm1_auto_point3_temp 36000 pwm1_auto_point4_pwm read-only, set to 204 pwm1_auto_point4_temp 39000 pwm1_auto_point5_pwm read-only, set to 230 pwm1_auto_point5_temp 42000 pwm1_auto_point6_pwm read-only, set to 255 pwm1_auto_point6_temp 45000 might make sense. > I did notice that nouveau_hwmon.c has a single temperature setpoint > similar to this that they support with SENSOR_DEVICE_ATTR: > https://elixir.bootlin.com/linux/latest/source/drivers/gpu/drm/nouveau/nouveau_hwmon.c#L63 > Whatever nouveau_hwmon.c does is, for all practical purposes, absolutely irrelevant. This code has never been reviewed by a hwmon maintainer. It may (or may not) be complete junk. Please do not use anything outside drivers/hwmon as example. > >> +- reg: offset of the ADC register > >> +- label: name of the ADC input or FAN setpoint > >> + > >> +Optional hwmon child properties: > >> +- gw,voltage-divider: An array of two integers containing the resistor > >> + values R1 and R2 of the optinal resistor divider on a raw ADC > >> +- gw,voltage-offset: a mV voltage offset to apply to a raw ADC (ie to > >> + compensate for a diode drop) > >> + > >> +Example: > >> + > >> + gsc: gsc@20 { > >> + compatible = "gw,gsc"; > >> + reg = <0x20>; > >> + interrupt-parent = <&gpio1>; > >> + interrupts = <4 GPIO_ACTIVE_LOW>; > >> + interrupt-controller; > >> + #interrupt-cells = <1>; > >> + > >> + watchdog { > >> + compatible = "gw,gsc-watchdog"; > >> + }; > >> + > >> + hwmon { > >> + compatible = "gw,gsc-hwmon"; > >> + #address-cells = <1>; > >> + #size-cells = <0>; > >> + gw,reference-voltage = <2500>; > >> + gw,resolution = <4096>; > >> + > >> + hwmon@0 { /* A0: Board Temperature */ > >> + type = "gw,hwmon-temperature"; > >> + reg = <0x00>; > >> + label = "temp"; > >> + }; > >> + > >> + hwmon@2 { /* A1: Input Voltage (raw ADC) */ > >> + type = "gw,hwmon-voltage-raw"; > >> + reg = <0x02>; > >> + label = "vdd_vin"; > >> + gw,voltage-divider = <22100 1000>; > >> + gw,voltage-offset = <800>; > >> + }; > >> + > >> + hwmon@b { /* A2: Battery voltage */ > >> + type = "gw,hwmon-voltage"; > >> + reg = <0x0b>; > >> + label = "vdd_bat"; > >> + }; > >> + > >> + hwmon@2c { /* fan temperature setpoint for 50% duty */ > >> + type = "gw,hwmon-fan"; > >> + reg = <0x2c>; > >> + label = "fan_50p"; > >> + }; > >> + > >> + hwmon@2e { /* fan1 */ > >> + type = "gw,hwmon-fan"; > >> + reg = <0x2e>; > >> + label = "fan_60p"; > >> + }; > >> + > >> + hwmon@30 { /* fan2 */ > >> + type = "gw,hwmon-fan"; > >> + reg = <0x30>; > >> + label = "fan_70p"; > >> + }; > >> + > >> + hwmon@32 { /* fan3 */ > >> + type = "gw,hwmon-fan"; > >> + reg = <0x32>; > >> + label = "fan_80p"; > >> + }; > >> + > >> + hwmon@34 { /* fan4 */ > >> + type = "gw,hwmon-fan"; > >> + reg = <0x34>; > >> + label = "fan_90p"; > >> + }; > >> + > >> + hwmon@36 { /* fan5 */ > >> + type = "gw,hwmon-fan"; > >> + reg = <0x36>; > >> + label = "fan_100p"; > >> + }; > > > > No idea what this is supposed to be doing, but whatever it is, > > it appears to be wrong. I'll comment more on it in the hwmon driver. > > > > ok - I'll respond to that thread. > > Thanks for the review! > > Tim -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
