Date:   Fri, 6 Mar 2020 12:02:31 -0800
From:   Tim Harvey <tharvey@...eworks.com>
To:     Rob Herring <robh@...nel.org>
Cc:     Lee Jones <lee.jones@...aro.org>, Jean Delvare <jdelvare@...e.com>,
        Guenter Roeck <linux@...ck-us.net>,
        Linux HWMON List <linux-hwmon@...r.kernel.org>,
        Frank Rowand <frowand.list@...il.com>,
        devicetree@...r.kernel.org,
        open list <linux-kernel@...r.kernel.org>,
        Robert Jones <rjones@...eworks.com>
Subject: Re: [PATCH v5 1/3] dt-bindings: mfd: Add Gateworks System Controller bindings

On Fri, Mar 6, 2020 at 9:50 AM Rob Herring <robh@...nel.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

Powered by blists - more mailing lists