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

[Tim Harvey]

On Fri, Mar 9, 2018 at 3:14 PM, Rob Herring <robh@xxxxxxxxxx> wrote:
> On Mon, Mar 05, 2018 at 02:02:38PM -0800, Tim Harvey wrote:
>> This patch adds documentation of device-tree bindings for the
>> Gateworks System Controller (GSC).
>>
>> Signed-off-by: Tim Harvey <tharvey@xxxxxxxxxxxxx>
>> ---
>>  Documentation/devicetree/bindings/mfd/gsc.txt | 159 ++++++++++++++++++++++++++
>>  1 file changed, 159 insertions(+)
>>  create mode 100644 Documentation/devicetree/bindings/mfd/gsc.txt
>>
<snip>
>> +
>> +* hwmon:
>> +The GSC provides a set of Analog to Digitcal Converter (ADC) pins used for
>> +temperature and/or voltage monitoring.
>> +
>> +Required properties:
>> +- compatible: must be "gw,gsc-hwmon"
>> +
<snip>
>> +
>> +             gsc_hwmon {
>
> hwmon {
>
>> +                     compatible = "gw,gsc-hwmon";
>> +                     #address-cells = <1>;
>> +                     #size-cells = <0>;
>> +
>> +                     hwmon@0 { /* A0: Board Temperature */
>> +                             type = <0>;
>
> Not documented.
>
>> +                             reg = <0x00>;
>> +                             label = "temp";
>> +                     };
>> +
>> +                     hwmon@1 { /* A1: Input Voltage */
>> +                             type = <1>;
>> +                             reg = <0x02>;
>> +                             label = "Vin";
>> +                     };
>> +
<snip>
>> +
>> +                     hwmon@15 { /* fan0 */
>> +                             type = <2>;
>> +                             reg = <0x2c>;
>> +                             label = "fan_50p";
>> +                     };
>> +
<snip>
>> +             };
>> +     };

Hi Rob,

Thanks for the review. I will roll in your changes.

I'm looking for suggestions on how to better define the child nodes of
the hwmon sub-device:

I have 4 types of hwmon child nodes supported by this device:
1. temperature in C/10 (is that called deci-celcuis?): this is a 2byte value
2. input voltage in mV: this is a 3 byte value pre-scaled by the GSC
firmware from a 10bit ADC where the
ref/bit-resolution/resistor-divider info is baked into the per-board
firmware
3. fan setpoint in C/10
4. input voltage in mV: raw 2-byte value of a 12bit ADC that needs to
be scaled based on a voltage-devider (pre-scaler), a ref voltage, and
resolution (or bit width)

The example I posted above shows use of the first three types but not
the more complicated fourth which requires the driver know more
details in order to present a cooked milivolt value. Note that the two
input types above would not both be present in any single board dts as
they represent a change in the way ADC's are reported in the overall
GSC version. A GSC typically has up to 16 different input ADC's and
the voltage rails monitored vary per board.

You mention that I don't document 'type' and your correct. I'm
thinking this is best done with a string using compatible within the
child node.

Something like:

Required properties:
...
- hwmon: This is the list of child nodes that specify the hwmon nodes.
...

hwmon required properties:
- hwmon-compatible: Must be "fan-setpoint", "temperature",
"input-raw", "input-cooked"
- reg: register offset of the node
- label: name of the input node

hwmon optional properties:
- gw,voltage-divider: an array of two integers containing the resistor
values R1 and R2 of the voltage divider in ohms
- gw,reference-voltage: the internal reference voltage of the ADC
input in milivolts
- gw,resolution: the resolution of the ADC input (ie 4096 for 12bit resolution)
...

Example:
...
hwmon {
  compatible = "gw,gsc-hwmon";
  #address-cells = <1>;
  #size-cells = <0>;

  hwmon@0 {
    compatible = "temperature";
    reg <0x00>;
    label = "temp";
  };

  hwmon@2 {
    compatible = "fan-setpoint";
    reg <0x02>;
    label = "fan_setpoint0";
  };

  /* this one represents the 3-byte ADC value that has been
'pre-scaled' by the GSC from a raw 10bit ADC (older GSC
version/firmware) */
  hwmon@4 {
    compatible = "input-cooked";
    reg <0x02>;
    label = "voltage1";
  };

  /* this one represents the 2-byte 12-bit ADC value that is 'raw'
from the GSC and needs to be scaled by driver (newer GSC
version/firmware) */
  hwmon@4 {
    compatible = "input-raw";
    reg <0x04>;
    label = "voltage2";
    gw,voltage-divider = <10000 10000>; /* R1=10k R2=10k div-by-2 pre-scaler */
    gw,referance-voltage = <2500>; /* 2.5V reference */
    gw,resolution = <4096>; /* 12bit resolution */
  };
};

If I add a byte-size (2|3) or bit-width (16|24) I could essentially
combine the two input types.

Suggestions?

Regards,

Tim