Message-ID: <db78ac20-9b58-49d1-ba38-cc269eaff254@baylibre.com>
Date: Tue, 10 Jun 2025 10:22:36 -0500
From: David Lechner <dlechner@...libre.com>
To: Marius.Cristea@...rochip.com, jic23@...nel.org, nuno.sa@...log.com,
 andy@...nel.org
Cc: robh@...nel.org, krzk+dt@...nel.org, conor+dt@...nel.org,
 broonie@...nel.org, devicetree@...r.kernel.org, linux-iio@...r.kernel.org,
 linux-kernel@...r.kernel.org
Subject: Re: [PATCH v3 1/2] dt-bindings: iio: adc: adding support for PAC194X

On 6/10/25 9:46 AM, Marius.Cristea@...rochip.com wrote:
> Hi David,
> 
>    Thank you for the feedback. Please see my comments below...
> 

...

>>> +  interrupts:
>>> +    maxItems: 2
>>> +
>>> +  interrupt-names:
>>
>> Needs minItems: 1 if we want to allow a single named interrupt.
>>
> the driver as it is right now it doesn't support any interrupt. I was
> thinking to add them here, just in case there will be a request to be
> added later.
> 

Making the bindings complete even if the driver isn't using it
yet is the right thing to do. :-)

I meant allowing just a single interrupt wired up though. So it
doesn't matter how the driver would handle it.

> 
>>> +    description:
>>> +      alert1 indicates a HIGH or LOW limit was exceeded.
>>> +      alert2 indicates a THERM limit was exceeded.
>>> +    items:
>>> +      - const: alert1
>>> +      - const: alert2
>>> +
>>

...

>>> +
>>> +      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.

Powered by blists - more mailing lists