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  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 
>> ---
>>  Documentation/devicetree/bindings/mfd/gsc.txt | 159 
>> ++
>>  1 file changed, 159 insertions(+)
>>  create mode 100644 Documentation/devicetree/bindings/mfd/gsc.txt
>>

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

>> +
>> + 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";
>> + };
>> +

>> +
>> + hwmon@15 { /* fan0 */
>> + type = <2>;
>> + reg = <0x2c>;
>> + label = "fan_50p";
>> + };
>> +

>> + };
>> + };

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 = <1 1>; /* 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

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  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 
>> ---
>>  Documentation/devicetree/bindings/mfd/gsc.txt | 159 
>> ++
>>  1 file changed, 159 insertions(+)
>>  create mode 100644 Documentation/devicetree/bindings/mfd/gsc.txt
>>

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

>> +
>> + 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";
>> + };
>> +

>> +
>> + hwmon@15 { /* fan0 */
>> + type = <2>;
>> + reg = <0x2c>;
>> + label = "fan_50p";
>> + };
>> +

>> + };
>> + };

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 = <1 1>; /* 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

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

[Fabio Estevam]

Hi Tim,

On Mon, Mar 5, 2018 at 7:02 PM, Tim Harvey  wrote:

> +
> +   hwmon@1 { /* A1: Input Voltage */
> +   type = <1>;
> +   reg = <0x02>;

Unit address (@1) does not match the 'reg' value.

If someone build this example with W=1 a warning will be shown.

Same problem happens in other places of this example.

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

[Fabio Estevam]

Hi Tim,

On Mon, Mar 5, 2018 at 7:02 PM, Tim Harvey  wrote:

> +
> +   hwmon@1 { /* A1: Input Voltage */
> +   type = <1>;
> +   reg = <0x02>;

Unit address (@1) does not match the 'reg' value.

If someone build this example with W=1 a warning will be shown.

Same problem happens in other places of this example.

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

[Rob Herring]

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 
> ---
>  Documentation/devicetree/bindings/mfd/gsc.txt | 159 
> ++
>  1 file changed, 159 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/mfd/gsc.txt
> 
> diff --git a/Documentation/devicetree/bindings/mfd/gsc.txt 
> b/Documentation/devicetree/bindings/mfd/gsc.txt
> new file mode 100644
> index 000..fe5d114
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/mfd/gsc.txt
> @@ -0,0 +1,159 @@
> +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_v1", "gw,gsc_v2", "gw,gsc_v3"

s/_/-/

> +- 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
> +  
> +
> +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 properties:
> +- compatible: must be "gw,gsc-watchdog"
> +
> +* input:
> +The GSC provides an input device capable of dispatching Linux Input events

Linux details that are not relevant to the binding.

> +for user pushbutton events, tamper switch events, etc.
> +
> +Required properties:
> +- compatible: must be "gw,gsc-input"
> +
> +* 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"
> +
> +Example:
> +
> + gsc: gsc@20 {
> + compatible = "gw,gsc_v2";
> + reg = <0x20>;
> + interrupt-parent = <>;
> + interrupts = <4 GPIO_ACTIVE_LOW>;
> + interrupt-controller;
> + #interrupt-cells = <1>;
> +
> + gsc_input {

input {

> + compatible = "gw,gsc-input";
> + };
> +
> + gsc_watchdog {

watchdog {

> + compatible = "gw,gsc-watchdog";
> + };
> +
> + 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";
> + };
> +
> + hwmon@2 { /* A2: 5P0 */
> + type = <1>;
> + reg = <0x0b>;
> + label = "5P0";
> + };
> +
> + hwmon@4 { /* A4: 0-5V input */
> + type = <1>;
> + reg = <0x14>;
> + label = "ANL0";
> + };
> +
> + hwmon@5 { /* A5: 2P5 PCIe/GigE */
> + type = <1>;
> + reg = <0x23>;
> + label = "2P5";
> + };
> +
> + hwmon@6 { /* A6: 1P8 Aud/Vid */
> + type = <1>;
> + reg = <0x1d>;
> + label = "1P8";
> + };
> +
> + hwmon@7 { /* A7: GPS */
> + type = <1>;
> + reg = <0x26>;
> + label = "GPS";
> + };
> +
> + hwmon@12 { /* A12: VDD_CORE */
> + type = <1>;
> + reg = <0x3>;
> + label = "VDD_CORE";
> + };
> +
> + hwmon@13 { /* A13: VDD_SOC */
> + type = <1>;
> + reg = <0x11>;
> + label = "VDD_SOC";
> + };
> +
> + hwmon@14 { /* A14: 1P0 PCIe SW */
> + type = <1>;
> + reg = <0x20>;
> + label = "1P0";
> + };
> +
> + hwmon@15 { /* fan0 */
> +   

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

[Rob Herring]

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 
> ---
>  Documentation/devicetree/bindings/mfd/gsc.txt | 159 
> ++
>  1 file changed, 159 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/mfd/gsc.txt
> 
> diff --git a/Documentation/devicetree/bindings/mfd/gsc.txt 
> b/Documentation/devicetree/bindings/mfd/gsc.txt
> new file mode 100644
> index 000..fe5d114
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/mfd/gsc.txt
> @@ -0,0 +1,159 @@
> +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_v1", "gw,gsc_v2", "gw,gsc_v3"

s/_/-/

> +- 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
> +  
> +
> +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 properties:
> +- compatible: must be "gw,gsc-watchdog"
> +
> +* input:
> +The GSC provides an input device capable of dispatching Linux Input events

Linux details that are not relevant to the binding.

> +for user pushbutton events, tamper switch events, etc.
> +
> +Required properties:
> +- compatible: must be "gw,gsc-input"
> +
> +* 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"
> +
> +Example:
> +
> + gsc: gsc@20 {
> + compatible = "gw,gsc_v2";
> + reg = <0x20>;
> + interrupt-parent = <>;
> + interrupts = <4 GPIO_ACTIVE_LOW>;
> + interrupt-controller;
> + #interrupt-cells = <1>;
> +
> + gsc_input {

input {

> + compatible = "gw,gsc-input";
> + };
> +
> + gsc_watchdog {

watchdog {

> + compatible = "gw,gsc-watchdog";
> + };
> +
> + 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";
> + };
> +
> + hwmon@2 { /* A2: 5P0 */
> + type = <1>;
> + reg = <0x0b>;
> + label = "5P0";
> + };
> +
> + hwmon@4 { /* A4: 0-5V input */
> + type = <1>;
> + reg = <0x14>;
> + label = "ANL0";
> + };
> +
> + hwmon@5 { /* A5: 2P5 PCIe/GigE */
> + type = <1>;
> + reg = <0x23>;
> + label = "2P5";
> + };
> +
> + hwmon@6 { /* A6: 1P8 Aud/Vid */
> + type = <1>;
> + reg = <0x1d>;
> + label = "1P8";
> + };
> +
> + hwmon@7 { /* A7: GPS */
> + type = <1>;
> + reg = <0x26>;
> + label = "GPS";
> + };
> +
> + hwmon@12 { /* A12: VDD_CORE */
> + type = <1>;
> + reg = <0x3>;
> + label = "VDD_CORE";
> + };
> +
> + hwmon@13 { /* A13: VDD_SOC */
> + type = <1>;
> + reg = <0x11>;
> + label = "VDD_SOC";
> + };
> +
> + hwmon@14 { /* A14: 1P0 PCIe SW */
> + type = <1>;
> + reg = <0x20>;
> + label = "1P0";
> + };
> +
> + hwmon@15 { /* fan0 */
> + type