[v5,1/3] dt-bindings: mfd: Add Gateworks System Controller bindings

This patch adds documentation of device-tree bindings for the
Gateworks System Controller (GSC).

Signed-off-by: Tim Harvey <tharvey@gateworks.com>
---
v5:
 - resolve dt_binding_check issues

v4:
 - move to using pwm<n>_auto_point<m>_{pwm,temp} for FAN PWM
 - remove unncessary resolution/scaling properties for ADCs
 - update to yaml
 - remove watchdog

v3:
 - replaced _ with -
 - remove input bindings
 - added full description of hwmon
 - fix unit address of hwmon child nodes
---
 .../devicetree/bindings/mfd/gateworks-gsc.yaml     | 158 +++++++++++++++++++++
 1 file changed, 158 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/mfd/gateworks-gsc.yaml

On Mon, Feb 24, 2020 at 12:54:23PM -0800, Tim Harvey wrote:
> This patch adds documentation of device-tree bindings for the
> Gateworks System Controller (GSC).
> 
> Signed-off-by: Tim Harvey <tharvey@gateworks.com>
> ---
> v5:
>  - resolve dt_binding_check issues
> 
> v4:
>  - move to using pwm<n>_auto_point<m>_{pwm,temp} for FAN PWM
>  - remove unncessary resolution/scaling properties for ADCs
>  - update to yaml
>  - remove watchdog
> 
> v3:
>  - replaced _ with -
>  - remove input bindings
>  - added full description of hwmon
>  - fix unit address of hwmon child nodes
> ---
>  .../devicetree/bindings/mfd/gateworks-gsc.yaml     | 158 +++++++++++++++++++++
>  1 file changed, 158 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/mfd/gateworks-gsc.yaml
> 
> diff --git a/Documentation/devicetree/bindings/mfd/gateworks-gsc.yaml b/Documentation/devicetree/bindings/mfd/gateworks-gsc.yaml
> new file mode 100644
> index 00000000..f7c1a05
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/mfd/gateworks-gsc.yaml
> @@ -0,0 +1,158 @@
> +# SPDX-License-Identifier: GPL-2.0-only OR BSD-2-Clause
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/mfd/gateworks-gsc.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: Gateworks System Controller multi-function device
> +
> +description: |
> +  The GSC is a Multifunction I2C slave device with the following submodules:
> +   - Watchdog Timer
> +   - GPIO
> +   - Pushbutton controller
> +   - Hardware Monitore with ADC's for temperature and voltage rails and

typo

> +     fan controller
> +
> +maintainers:
> +  - Tim Harvey <tharvey@gateworks.com>
> +  - Robert Jones <rjones@gateworks.com>
> +
> +properties:
> +  $nodename:
> +    pattern: "gsc@[0-9a-f]{1,2}"
> +  compatible:
> +    const: gw,gsc

That's not very specific.

> +
> +  reg:
> +    description: I2C device address
> +    maxItems: 1
> +
> +  interrupts:
> +    maxItems: 1
> +
> +  interrupt-controller: true
> +
> +  "#interrupt-cells":
> +    const: 1
> +    description: The IRQ number

description is wrong. You can just drop it.

> +
> +  hwmon:

'hwmon' is a Linux thing. I'm suspicious...

> +    type: object
> +    description: Optional Hardware Monitoring module
> +
> +    properties:
> +      compatible:
> +        const: gw,gsc-hwmon
> +
> +      "#address-cells":
> +        const: 1
> +
> +      "#size-cells":
> +        const: 0
> +
> +      gw,fan-base:
> +        $ref: /schemas/types.yaml#/definitions/uint32
> +        description: The fan controller base address

Shouldn't this be described as a node in the DT or be implied by the 
compatible?

> +
> +    patternProperties:
> +      "^adc@[0-2]$":

There's only one number space at any level. So if you ever need anything 
else at this level, it can't have an address. Just something to 
consider.

> +        type: object
> +        description: |
> +          Properties for a single ADC which can report cooked values
> +          (ie temperature sensor based on thermister), raw values
> +          (ie voltage rail with a pre-scaling resistor divider).
> +
> +        properties:
> +          reg:
> +            description: Register of the ADC
> +            maxItems: 1
> +
> +          label:
> +            description: Name of the ADC input
> +
> +          type:

Very generic property name, but it's not generic. Needs a vendor prefix 
at least.

> +            description: |
> +              temperature in C*10 (temperature),
> +              pre-scaled voltage value (voltage),
> +              or scaled based on an optional resistor divider and optional
> +              offset (voltage-raw)
> +            enum:
> +              - temperature
> +              - voltage
> +              - voltage-raw
> +
> +          gw,voltage-divider:
> +            allOf:
> +              - $ref: /schemas/types.yaml#/definitions/uint32-array
> +            description: values of resistors for divider on raw ADC input
> +            items:
> +              - description: R1
> +              - description: R2

Needs a standard unit suffix. With that, you can drop the type 
reference.

> +
> +          gw,voltage-offset:
> +            $ref: /schemas/types.yaml#/definitions/uint32
> +            description: |
> +              A positive uV voltage offset to apply to a raw ADC
> +              (ie to compensate for a diode drop).

Needs a unit suffix. 

> +
> +        required:
> +          - type
> +          - reg
> +          - label
> +
> +    required:
> +      - compatible
> +      - "#address-cells"
> +      - "#size-cells"
> +
> +required:
> +  - compatible
> +  - reg
> +  - interrupts
> +  - interrupt-controller
> +  - "#interrupt-cells"
> +
> +examples:
> +  - |
> +    #include <dt-bindings/gpio/gpio.h>
> +    i2c {
> +        #address-cells = <1>;
> +        #size-cells = <0>;
> +
> +        gsc@20 {
> +            compatible = "gw,gsc";
> +            reg = <0x20>;
> +            interrupt-parent = <&gpio1>;
> +            interrupts = <4 GPIO_ACTIVE_LOW>;
> +            interrupt-controller;
> +            #interrupt-cells = <1>;
> +
> +            hwmon {
> +                compatible = "gw,gsc-hwmon";
> +                #address-cells = <1>;
> +                #size-cells = <0>;
> +                gw,fan-base = <0x2c>;
> +
> +                adc@0 { /* A0: Board Temperature */
> +                    type = "temperature";
> +                    reg = <0x00>;
> +                    label = "temp";
> +                };
> +
> +                adc@2 { /* A1: Input Voltage (raw ADC) */
> +                    type = "voltage-raw";
> +                    reg = <0x02>;
> +                    label = "vdd_vin";
> +                    gw,voltage-divider = <22100 1000>;
> +                    gw,voltage-offset = <800000>;
> +                };
> +
> +                adc@b { /* A2: Battery voltage */
> +                    type = "voltage";
> +                    reg = <0x0b>;
> +                    label = "vdd_bat";
> +                };
> +            };
> +        };
> +    };
> -- 
> 2.7.4
>

On Mon, Mar 2, 2020 at 12:49 PM Rob Herring <robh@kernel.org> wrote:
>

Rob,

Thanks for the review! Some questions below:

> On Mon, Feb 24, 2020 at 12:54:23PM -0800, Tim Harvey wrote:
> > This patch adds documentation of device-tree bindings for the
> > Gateworks System Controller (GSC).
> >
> > Signed-off-by: Tim Harvey <tharvey@gateworks.com>
> > ---
> > v5:
> >  - resolve dt_binding_check issues
> >
> > v4:
> >  - move to using pwm<n>_auto_point<m>_{pwm,temp} for FAN PWM
> >  - remove unncessary resolution/scaling properties for ADCs
> >  - update to yaml
> >  - remove watchdog
> >
> > v3:
> >  - replaced _ with -
> >  - remove input bindings
> >  - added full description of hwmon
> >  - fix unit address of hwmon child nodes
> > ---
> >  .../devicetree/bindings/mfd/gateworks-gsc.yaml     | 158 +++++++++++++++++++++
> >  1 file changed, 158 insertions(+)
> >  create mode 100644 Documentation/devicetree/bindings/mfd/gateworks-gsc.yaml
> >
> > diff --git a/Documentation/devicetree/bindings/mfd/gateworks-gsc.yaml b/Documentation/devicetree/bindings/mfd/gateworks-gsc.yaml
> > new file mode 100644
> > index 00000000..f7c1a05
> > --- /dev/null
> > +++ b/Documentation/devicetree/bindings/mfd/gateworks-gsc.yaml
> > @@ -0,0 +1,158 @@
> > +# SPDX-License-Identifier: GPL-2.0-only OR BSD-2-Clause
> > +%YAML 1.2
> > +---
> > +$id: http://devicetree.org/schemas/mfd/gateworks-gsc.yaml#
> > +$schema: http://devicetree.org/meta-schemas/core.yaml#
> > +
> > +title: Gateworks System Controller multi-function device
> > +
> > +description: |
> > +  The GSC is a Multifunction I2C slave device with the following submodules:
> > +   - Watchdog Timer
> > +   - GPIO
> > +   - Pushbutton controller
> > +   - Hardware Monitore with ADC's for temperature and voltage rails and
>
> typo

will fix

>
> > +     fan controller
> > +
> > +maintainers:
> > +  - Tim Harvey <tharvey@gateworks.com>
> > +  - Robert Jones <rjones@gateworks.com>
> > +
> > +properties:
> > +  $nodename:
> > +    pattern: "gsc@[0-9a-f]{1,2}"
> > +  compatible:
> > +    const: gw,gsc
>
> That's not very specific.
>

Do you mean something like 'gw,system-controller' would be better
instead of the gsc abbreviation for 'Gateworks System Controller'?

> +
> > +  reg:
> > +    description: I2C device address
> > +    maxItems: 1
> > +
> > +  interrupts:
> > +    maxItems: 1
> > +
> > +  interrupt-controller: true
> > +
> > +  "#interrupt-cells":
> > +    const: 1
> > +    description: The IRQ number
>
> description is wrong. You can just drop it.
>

ok

> > +
> > +  hwmon:
>
> 'hwmon' is a Linux thing. I'm suspicious...
>

Yes, we've discussed this before and I understand that DT shouldn't
use terminology that is Linux specific (which is why I replaced
'hwmon' with 'adc' in the ADC nodes below) but I still see a long of
dt bindings in Documentation/devicetree/bindings with the word 'hwmon'
in them.

Perhaps this makes more sense?

adc {
  compatible = "gw,gsc-adc";
  #address-cells = <1>;
  #size-cells = <0>;

  channel@6 {
    type = "gw,hwmon-temperature";
    reg = <0x06>;
    label = "temp";
  };
  ...
};


> > +    type: object
> > +    description: Optional Hardware Monitoring module
> > +
> > +    properties:
> > +      compatible:
> > +        const: gw,gsc-hwmon
> > +
> > +      "#address-cells":
> > +        const: 1
> > +
> > +      "#size-cells":
> > +        const: 0
> > +
> > +      gw,fan-base:
> > +        $ref: /schemas/types.yaml#/definitions/uint32
> > +        description: The fan controller base address
>
> Shouldn't this be described as a node in the DT or be implied by the
> compatible?

It does look out of place there. Would adding another subnode outside
of the (perhaps misnamed) 'hwmon' node make more sense?:

fan:
 properties:
   compatible: gw,gsc-fancontroller
   reg:
     description: address of the fan controller base register
     maxItems: 1

>
> > +
> > +    patternProperties:
> > +      "^adc@[0-2]$":
>
> There's only one number space at any level. So if you ever need anything
> else at this level, it can't have an address. Just something to
> consider.
>

yes, one number space is ok if I understand what you mean but I meant
this to be "^adc@[0-9]+$" to support the number of ADC pins the part
supports.

> > +        type: object
> > +        description: |
> > +          Properties for a single ADC which can report cooked values
> > +          (ie temperature sensor based on thermister), raw values
> > +          (ie voltage rail with a pre-scaling resistor divider).
> > +
> > +        properties:
> > +          reg:
> > +            description: Register of the ADC
> > +            maxItems: 1
> > +
> > +          label:
> > +            description: Name of the ADC input
> > +
> > +          type:
>
> Very generic property name, but it's not generic. Needs a vendor prefix
> at least.

You mean the property name of 'type' is fine, but the values will need
to be vendor specific like 'gw,temperature', 'gw,voltage',
'gw,voltage-raw' or is it inappropriate to use 'type'?

>
> > +            description: |
> > +              temperature in C*10 (temperature),
> > +              pre-scaled voltage value (voltage),
> > +              or scaled based on an optional resistor divider and optional
> > +              offset (voltage-raw)
> > +            enum:
> > +              - temperature
> > +              - voltage
> > +              - voltage-raw
> > +
> > +          gw,voltage-divider:
> > +            allOf:
> > +              - $ref: /schemas/types.yaml#/definitions/uint32-array
> > +            description: values of resistors for divider on raw ADC input
> > +            items:
> > +              - description: R1
> > +              - description: R2
>
> Needs a standard unit suffix. With that, you can drop the type
> reference.

I understand the unit suffix but not sure what you mean by type
reference. Do you mean:

gw,voltage-divider-milli-ohms:
  description: values of resistors for divider on raw ADC input
    items:
      - description: R1
      - description: R2

>
> > +
> > +          gw,voltage-offset:
> > +            $ref: /schemas/types.yaml#/definitions/uint32
> > +            description: |
> > +              A positive uV voltage offset to apply to a raw ADC
> > +              (ie to compensate for a diode drop).
>
> Needs a unit suffix.

ok

Thanks!

Tim

On Fri, Mar 6, 2020 at 9:58 AM Tim Harvey <tharvey@gateworks.com> wrote:
>
> On Mon, Mar 2, 2020 at 12:49 PM Rob Herring <robh@kernel.org> wrote:
> >
>
> Rob,
>
> Thanks for the review! Some questions below:
>
> > On Mon, Feb 24, 2020 at 12:54:23PM -0800, Tim Harvey wrote:
> > > This patch adds documentation of device-tree bindings for the
> > > Gateworks System Controller (GSC).
> > >
> > > Signed-off-by: Tim Harvey <tharvey@gateworks.com>
> > > ---
> > > v5:
> > >  - resolve dt_binding_check issues
> > >
> > > v4:
> > >  - move to using pwm<n>_auto_point<m>_{pwm,temp} for FAN PWM
> > >  - remove unncessary resolution/scaling properties for ADCs
> > >  - update to yaml
> > >  - remove watchdog
> > >
> > > v3:
> > >  - replaced _ with -
> > >  - remove input bindings
> > >  - added full description of hwmon
> > >  - fix unit address of hwmon child nodes
> > > ---
> > >  .../devicetree/bindings/mfd/gateworks-gsc.yaml     | 158 +++++++++++++++++++++
> > >  1 file changed, 158 insertions(+)
> > >  create mode 100644 Documentation/devicetree/bindings/mfd/gateworks-gsc.yaml
> > >
> > > diff --git a/Documentation/devicetree/bindings/mfd/gateworks-gsc.yaml b/Documentation/devicetree/bindings/mfd/gateworks-gsc.yaml
> > > new file mode 100644
> > > index 00000000..f7c1a05
> > > --- /dev/null
> > > +++ b/Documentation/devicetree/bindings/mfd/gateworks-gsc.yaml
> > > @@ -0,0 +1,158 @@
> > > +# SPDX-License-Identifier: GPL-2.0-only OR BSD-2-Clause
> > > +%YAML 1.2
> > > +---
> > > +$id: http://devicetree.org/schemas/mfd/gateworks-gsc.yaml#
> > > +$schema: http://devicetree.org/meta-schemas/core.yaml#
> > > +
> > > +title: Gateworks System Controller multi-function device
> > > +
> > > +description: |
> > > +  The GSC is a Multifunction I2C slave device with the following submodules:
> > > +   - Watchdog Timer
> > > +   - GPIO
> > > +   - Pushbutton controller
> > > +   - Hardware Monitore with ADC's for temperature and voltage rails and
> >
> > typo
>
> will fix
>
> >
> > > +     fan controller
> > > +
> > > +maintainers:
> > > +  - Tim Harvey <tharvey@gateworks.com>
> > > +  - Robert Jones <rjones@gateworks.com>
> > > +
> > > +properties:
> > > +  $nodename:
> > > +    pattern: "gsc@[0-9a-f]{1,2}"
> > > +  compatible:
> > > +    const: gw,gsc
> >
> > That's not very specific.
> >
>
> Do you mean something like 'gw,system-controller' would be better
> instead of the gsc abbreviation for 'Gateworks System Controller'?

No, I mean is there or will there be only one version of this?

>
> > +
> > > +  reg:
> > > +    description: I2C device address
> > > +    maxItems: 1
> > > +
> > > +  interrupts:
> > > +    maxItems: 1
> > > +
> > > +  interrupt-controller: true
> > > +
> > > +  "#interrupt-cells":
> > > +    const: 1
> > > +    description: The IRQ number
> >
> > description is wrong. You can just drop it.
> >
>
> ok
>
> > > +
> > > +  hwmon:
> >
> > 'hwmon' is a Linux thing. I'm suspicious...
> >
>
> Yes, we've discussed this before and I understand that DT shouldn't
> use terminology that is Linux specific (which is why I replaced
> 'hwmon' with 'adc' in the ADC nodes below) but I still see a long of
> dt bindings in Documentation/devicetree/bindings with the word 'hwmon'
> in them.
>
> Perhaps this makes more sense?

Yes, that's more aligned with IIO ADC bindings. Yes, IIO is again a
Linuxism, but I think the ADC bindings are fairly independent other
than the directory name.

>
> adc {
>   compatible = "gw,gsc-adc";
>   #address-cells = <1>;
>   #size-cells = <0>;
>
>   channel@6 {
>     type = "gw,hwmon-temperature";
>     reg = <0x06>;
>     label = "temp";
>   };
>   ...
> };
>
>
> > > +    type: object
> > > +    description: Optional Hardware Monitoring module
> > > +
> > > +    properties:
> > > +      compatible:
> > > +        const: gw,gsc-hwmon
> > > +
> > > +      "#address-cells":
> > > +        const: 1
> > > +
> > > +      "#size-cells":
> > > +        const: 0
> > > +
> > > +      gw,fan-base:
> > > +        $ref: /schemas/types.yaml#/definitions/uint32
> > > +        description: The fan controller base address
> >
> > Shouldn't this be described as a node in the DT or be implied by the
> > compatible?
>
> It does look out of place there. Would adding another subnode outside
> of the (perhaps misnamed) 'hwmon' node make more sense?:
>
> fan:
>  properties:
>    compatible: gw,gsc-fancontroller
>    reg:
>      description: address of the fan controller base register
>      maxItems: 1

Seems somewhat better location in that the first level is
sub-functions of this chip.

But now you have 'adc' with no address and 'fan' (w/ reg should be
fan@...) with an address, so that's not consistent.

Also, I think fan controllers and fans need to have separate nodes as
there are different types of fans such as with and without tach
signals. I've tried to steer other fan bindings that way. Depends how
complex the fan controller is whether that's necessary.

> > > +    patternProperties:
> > > +      "^adc@[0-2]$":
> >
> > There's only one number space at any level. So if you ever need anything
> > else at this level, it can't have an address. Just something to
> > consider.
> >
>
> yes, one number space is ok if I understand what you mean but I meant
> this to be "^adc@[0-9]+$" to support the number of ADC pins the part
> supports.
>
> > > +        type: object
> > > +        description: |
> > > +          Properties for a single ADC which can report cooked values
> > > +          (ie temperature sensor based on thermister), raw values
> > > +          (ie voltage rail with a pre-scaling resistor divider).
> > > +
> > > +        properties:
> > > +          reg:
> > > +            description: Register of the ADC
> > > +            maxItems: 1
> > > +
> > > +          label:
> > > +            description: Name of the ADC input
> > > +
> > > +          type:
> >
> > Very generic property name, but it's not generic. Needs a vendor prefix
> > at least.
>
> You mean the property name of 'type' is fine, but the values will need
> to be vendor specific like 'gw,temperature', 'gw,voltage',
> 'gw,voltage-raw' or is it inappropriate to use 'type'?

Don't use 'type'.

Is this for 'how to setup/program the adc' or 'what am I measuring'?
For example, configure the adc for temperature readings vs. measure
CPU temperature. Seems like a common thing needed for ADC. 'label'
already covers the latter case.

> > > +            description: |
> > > +              temperature in C*10 (temperature),
> > > +              pre-scaled voltage value (voltage),
> > > +              or scaled based on an optional resistor divider and optional
> > > +              offset (voltage-raw)
> > > +            enum:
> > > +              - temperature
> > > +              - voltage
> > > +              - voltage-raw
> > > +
> > > +          gw,voltage-divider:
> > > +            allOf:
> > > +              - $ref: /schemas/types.yaml#/definitions/uint32-array
> > > +            description: values of resistors for divider on raw ADC input
> > > +            items:
> > > +              - description: R1
> > > +              - description: R2
> >
> > Needs a standard unit suffix. With that, you can drop the type
> > reference.
>
> I understand the unit suffix but not sure what you mean by type
> reference. Do you mean:
>
> gw,voltage-divider-milli-ohms:
>   description: values of resistors for divider on raw ADC input
>     items:
>       - description: R1
>       - description: R2

Yes, drop the '$ref'.

Rob

On Fri, Mar 6, 2020 at 9:50 AM Rob Herring <robh@kernel.org> wrote:
>
<snip>
> > > > +properties:
> > > > +  $nodename:
> > > > +    pattern: "gsc@[0-9a-f]{1,2}"
> > > > +  compatible:
> > > > +    const: gw,gsc
> > >
> > > That's not very specific.
> > >
> >
> > Do you mean something like 'gw,system-controller' would be better
> > instead of the gsc abbreviation for 'Gateworks System Controller'?
>
> No, I mean is there or will there be only one version of this?
>

currently just 1 version is enough

> >
<snip>
> >
> > > > +
> > > > +  hwmon:
> > >
> > > 'hwmon' is a Linux thing. I'm suspicious...
> > >
> >
> > Yes, we've discussed this before and I understand that DT shouldn't
> > use terminology that is Linux specific (which is why I replaced
> > 'hwmon' with 'adc' in the ADC nodes below) but I still see a long of
> > dt bindings in Documentation/devicetree/bindings with the word 'hwmon'
> > in them.
> >
> > Perhaps this makes more sense?
>
> Yes, that's more aligned with IIO ADC bindings. Yes, IIO is again a
> Linuxism, but I think the ADC bindings are fairly independent other
> than the directory name.
>
> >
> > adc {
> >   compatible = "gw,gsc-adc";
> >   #address-cells = <1>;
> >   #size-cells = <0>;
> >
> >   channel@6 {
> >     type = "gw,hwmon-temperature";
> >     reg = <0x06>;
> >     label = "temp";
> >   };
> >   ...
> > };
> >

ok, will use adc/channel instead of hwmon/adc and change compatible to
'gw,gsc-adc'

> >
> > > > +    type: object
> > > > +    description: Optional Hardware Monitoring module
> > > > +
> > > > +    properties:
> > > > +      compatible:
> > > > +        const: gw,gsc-hwmon
> > > > +
> > > > +      "#address-cells":
> > > > +        const: 1
> > > > +
> > > > +      "#size-cells":
> > > > +        const: 0
> > > > +
> > > > +      gw,fan-base:
> > > > +        $ref: /schemas/types.yaml#/definitions/uint32
> > > > +        description: The fan controller base address
> > >
> > > Shouldn't this be described as a node in the DT or be implied by the
> > > compatible?
> >
> > It does look out of place there. Would adding another subnode outside
> > of the (perhaps misnamed) 'hwmon' node make more sense?:
> >
> > fan:
> >  properties:
> >    compatible: gw,gsc-fancontroller
> >    reg:
> >      description: address of the fan controller base register
> >      maxItems: 1
>
> Seems somewhat better location in that the first level is
> sub-functions of this chip.
>
> But now you have 'adc' with no address and 'fan' (w/ reg should be
> fan@...) with an address, so that's not consistent.
>
> Also, I think fan controllers and fans need to have separate nodes as
> there are different types of fans such as with and without tach
> signals. I've tried to steer other fan bindings that way. Depends how
> complex the fan controller is whether that's necessary.
>

The fan controller does now support a tach signal reported via one of
the ADC channels (which I've neglected to cover) so I can represent
that as well in a new first level node such as:

  fan:
    type: object
    description: Optional FAN controller

    properties:
      compatible:
        const: gw,gsc-fan

      reg:
        description: The fan controller base address
        maxItems: 1

      gw,fan-tach-ch:
        description: The fan tach ADC channel
        maxItems: 1

    required:
      - compatible
      - reg

fan {
  compatible = "gw,gsc-pwm-fan";
  reg = <0x2c>;
  gw,fan-tach-ch = <0x16>;
};

<snip>
> >
> > > > +        type: object
> > > > +        description: |
> > > > +          Properties for a single ADC which can report cooked values
> > > > +          (ie temperature sensor based on thermister), raw values
> > > > +          (ie voltage rail with a pre-scaling resistor divider).
> > > > +
> > > > +        properties:
> > > > +          reg:
> > > > +            description: Register of the ADC
> > > > +            maxItems: 1
> > > > +
> > > > +          label:
> > > > +            description: Name of the ADC input
> > > > +
> > > > +          type:
> > >
> > > Very generic property name, but it's not generic. Needs a vendor prefix
> > > at least.
> >
> > You mean the property name of 'type' is fine, but the values will need
> > to be vendor specific like 'gw,temperature', 'gw,voltage',
> > 'gw,voltage-raw' or is it inappropriate to use 'type'?
>
> Don't use 'type'.
>
> Is this for 'how to setup/program the adc' or 'what am I measuring'?
> For example, configure the adc for temperature readings vs. measure
> CPU temperature. Seems like a common thing needed for ADC. 'label'
> already covers the latter case.

This is for translation of the raw ADC to a cooked value. An earlier
version of the GSC reported cooked values (doing the scaling in the
GSC firmware) and later versions report raw values which need to be
scaled depending on optional voltage divider so you can consider that
'setup'. Instead of handling this via a 'version' of the GSC I elected
to describe the difference in ADC channel type as I already had on
that reported millidegree celcius vs millivolts. I could just move
them to properties such as:

gw,temperature
gw,voltage
gw,voltage-raw

Only one of the above is allowed and am not sure how to represent that
in the yaml.

Alternatively I could call this property name 'gw,conversion' and
leave the three type enum?

>
> > > > +            description: |
> > > > +              temperature in C*10 (temperature),
> > > > +              pre-scaled voltage value (voltage),
> > > > +              or scaled based on an optional resistor divider and optional
> > > > +              offset (voltage-raw)
> > > > +            enum:
> > > > +              - temperature
> > > > +              - voltage
> > > > +              - voltage-raw
> > > > +
> > > > +          gw,voltage-divider:
> > > > +            allOf:
> > > > +              - $ref: /schemas/types.yaml#/definitions/uint32-array
> > > > +            description: values of resistors for divider on raw ADC input
> > > > +            items:
> > > > +              - description: R1
> > > > +              - description: R2
> > >
> > > Needs a standard unit suffix. With that, you can drop the type
> > > reference.
> >
> > I understand the unit suffix but not sure what you mean by type
> > reference. Do you mean:
> >
> > gw,voltage-divider-milli-ohms:
> >   description: values of resistors for divider on raw ADC input
> >     items:
> >       - description: R1
> >       - description: R2
>
> Yes, drop the '$ref'.
>

ok,

Thanks,

Tim