Re: [PATCH v3 1/2] dt-bindings: iio: adc: adding support for PAC194X

[<Marius.Cristea@microchip.com>]

On Tue, 2025-06-10 at 10:22 -0500, David Lechner wrote:
> EXTERNAL EMAIL: Do not click links or open attachments unless you
> know the content is safe
> 
> On 6/10/25 9:46 AM, Marius.Cristea@microchip.com wrote:
> > Hi David,
> > 
> >    Thank you for the feedback. Please see my comments below...
> > 
> 
> ...
> 
...
> 
> > > > +
> > > > +      microchip,vbus-half-range:
> > > > +        $ref: /schemas/types.yaml#/definitions/flag
> > > > +        description: |
> > > > +          In order to increase measurement resolution and
> > > > keeping
> > > > the same
> > > > +          number the of bits the device has a configurable
> > > > VBUS
> > > > full range scale
> > > > +          (FSR). The range should be set by hardware design
> > > > and it
> > > > should not be
> > > > +          changed during runtime. The bipolar capability for
> > > > VBUS
> > > > enables
> > > > +          accurate offset measurement and correction.
> > > > +          The VBUS could be configured into the following full
> > > > scale range:
> > > > +            - VBUS has unipolar 0V to 32V FSR (default) for
> > > > PAC195X or 0V to 9V
> > > > +              (default) for PAC194X.
> > > > +            - VBUS has bipolar -32V to 32V FSR for PAC195X or
> > > > -9V
> > > > to 9V for
> > > > +              PAC194X. The actual range is limited to about -
> > > > 200
> > > > mV due to the
> > > > +              impact of the ESD structures.
> > > > +            - VBUS has bipolar -16V to 16V FSR for PAC195X or
> > > > -
> > > > 4.5V to 4.5V for
> > > > +              PAC194X. The actual range is limited to about -
> > > > 200
> > > > mV due to the
> > > > +              impact of the ESD structures.
> > > > +
> > > > +      microchip,vbus-bipolar:
> > > > +        $ref: /schemas/types.yaml#/definitions/flag
> > > > +        description:
> > > > +          If provided, the channel is to be used in bipolar
> > > > mode.
> > > > The
> > > > +          actual range is limited to about -200 mV due to the
> > > > impact of the ESD
> > > > +          structures.
> > > > +
> > > 
> > > Using Jonathan's suggestion from v2 to just have a single
> > > property
> > > with 3 different
> > > ranges to chose from seems simpler that this. It would only
> > > require
> > > one property
> > > and would be self-documenting. The description could be shortened
> > > to
> > > just a couple
> > > of lines.
> > 
> > I was thinking to add the range for this property, but it looks
> > (for me
> > at least) more complicated from the checking point of view. The
> > driver
> > is supporting two family of devices that has, each, 3 different
> > voltage
> > range as an input.
> > 
> 
> Usually, having a consistent binding for the same thing among similar
> devices is more important than how easy it is to implement in the
> driver.
> 
> Since this seems to be a common pattern, we could probably justify an
> iio_property_match_ranges() helper function that would simplify the
> implementation in drivers that would need to use such a property.
> Then
> in each driver it would just be a matter of making a static const
> array
> lookup table of ranges for each device and calling the helper
> function.

Sorry for not explaining very well. I have implemented the range into
the driver and I was working well, but I had issues defining the range
into the device binding and the checker was failing. That was the
reason that I've dropped the range from the binding. Also I had some
issues enforcing a certain "available" ranges for a particular part
into the binding.