Re: [PATCH v3 1/4] dt-bindings: mfd: Add Gateworks System Controller bindings

[Guenter Roeck]

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.

> +Optional hwmon properties:
> +- gw,reference-voltage: ADC reference voltage (mV) used in scaling raw ADCs

AFAIK devicetree likes to specify voltages in uV.

> +- gw,resolution: ADC resolution (ie 4096) used in scaling raw ADCs
> +

4096 what ?

> +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" ?

> +- 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.

Guenter

> +		};
> +	};
> -- 
> 2.7.4
>