Re: [PATCH v3 2/2] dt-bindings: hwmon: isl68137: add bindings to support voltage dividers

[Conor Dooley <conor@kernel.org>]

[-- Attachment #1: Type: text/plain, Size: 4701 bytes --]

On Thu, Oct 24, 2024 at 01:32:09PM -0500, Grant Peltier wrote:
> On Thu, Oct 24, 2024 at 06:01:11PM +0100, Conor Dooley wrote:
> > On Wed, Oct 23, 2024 at 03:53:51PM -0500, Grant Peltier wrote:
> > > + [...]
> > > +
> > > +properties:
> > > +  compatible:
> > > +    enum:
> > > +      - renesas,isl68220
> > > +      - renesas,isl68221
> > > +      - renesas,isl68222
> > > +      - renesas,isl68223
> > > +      - renesas,isl68224
> > > +      - renesas,isl68225
> > > +      - renesas,isl68226
> > > +      - renesas,isl68227
> > > +      - renesas,isl68229
> > > +      - renesas,isl68233
> > > +      - renesas,isl68239
> > > +      - renesas,isl69222
> > > +      - renesas,isl69223
> > > +      - renesas,isl69224
> > > +      - renesas,isl69225
> > > +      - renesas,isl69227
> > > +      - renesas,isl69228
> > > +      - renesas,isl69234
> > > +      - renesas,isl69236
> > > +      - renesas,isl69239
> > > +      - renesas,isl69242
> > > +      - renesas,isl69243
> > > +      - renesas,isl69247
> > > +      - renesas,isl69248
> > > +      - renesas,isl69254
> > > +      - renesas,isl69255
> > > +      - renesas,isl69256
> > > +      - renesas,isl69259
> > > +      - renesas,isl69260
> > > +      - renesas,isl69268
> > > +      - renesas,isl69269
> > > +      - renesas,isl69298
> > > +      - renesas,raa228000
> > > +      - renesas,raa228004
> > > +      - renesas,raa228006
> > > +      - renesas,raa228228
> > > +      - renesas,raa229001
> > > +      - renesas,raa229004
> > 
> > Damn, that;s a list and a half, innit! Looking briefly at the driver
> > change, the match data implies that quite a few of these actually would
> > be suitable for fallback compatibles.
> 
> Yes, there are quite a few part numbers (and likely to be more in the
> future). My intention was to make the driver more user friendly since the
> variants listed in the driver do not map to something in any of the
> datasheets. So using those instead would require users to inspect the
> source of the driver instead of simply referencing their part number(s).

I don't understand. How would a fallback materially change anything in
that regard? You still put the compatible corresponding to the device
you have in your dts. A fallback means having multiple compatible
strings in the property, not a single one corresponding to another
device.

> > > +  reg:
> > > +    maxItems: 1
> > > +
> > > +  '#address-cells':
> > > +    const: 1
> > > +
> > > +  '#size-cells':
> > > +    const: 0
> > > +
> > > +patternProperties:
> > > +  "^channel@([0-3])$":
> > > +    type: object
> > > +    description:
> > > +      Container for properties specific to a particular channel (rail).
> > > +
> > > +    properties:
> > > +      reg:
> > > +        description: The channel (rail) index.
> > > +        items:
> > > +          minimum: 0
> > > +          maximum: 3
> > > +
> > > +      renesas,vout-voltage-divider:
> > 
> > There's already a binding for voltage dividers: voltage-divider.yaml
> > That said, I have no idea how that would work with an extant driver for
> > the hardware like we have here. I'd imagine it would really have to be
> > used with iio-hwmon? + Peter and Jonathan, since I don't know how the
> > driver side of using the voltage divider works.
> 
> In his recent revier, Guenter requested using a standard voltage divider
> schema as well. I see there is an implementation in maxim,maxim20730.yaml
> but that differs from the one in voltage-divider.yaml. Should I opt to
> match maxim,maxim20730.yaml?

I would rather the standard binding was used, but it would probably
involve having to hook up iio-rescale to hwmon? I don't know enough
about that, which is why I Cced Peter and Jonathan.

> > > +examples:
> > > +  - |
> > > +    i2c {
> > > +      #address-cells = <1>;
> > > +      #size-cells = <0>;
> > > +
> > > +      isl68239@60 {
> > > +        compatible = "renesas,isl68239";
> > > +        reg = <0x60>;
> > > +      };
> > > +    };
> > 
> > Without any channels, what does this actually do? If you've got no
> > channels you cannot measure anything making this example invalid?
> 
> The channel structures are optional to allow users to arbitrarily define
> voltage dividers for any particular rail. Omitting the channel definitions
> still allow the device to be instantiated and probed as an I2C device
> along with all related hwmon PMBus telemetry dictated by the part variant.

I dunno, either the channels are hooked up to something or they are not.
If they are, the channels should be populated in the devicetree.

Cheers,
Conor.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]