Re: [PATCH v11 1/2] dt-bindings: adc: add AD7173

[Ceclan Dumitru <mitrutzceclan@xxxxxxxxx>]

On 1/16/24 18:30, Jonathan Cameron wrote:
> On Mon, 15 Jan 2024 15:53:39 -0600
> David Lechner <dlechner@xxxxxxxxxxxx> wrote:
> 
>> On Wed, Dec 20, 2023 at 4:48 AM Dumitru Ceclan <mitrutzceclan@xxxxxxxxx> wrote:

...

>> Sorry for the late reply as I see this has been applied already but...
> We have plenty of time.  Rather than dropping the ad7173 from my tree,
> I'd prefer to see additional patches on top to tidy up whatever
> makes sense from David's feedback.
> 
Alright then.

...

>>
>> As discussed in v8 [1] it is not clear what signal this is. Based on
>> that discussion, I'm assuming the RDY signal, but how would bindings
>> consumers know that without a description since it is not the only
>> digital output signal of the chip? And why the ERROR signal was
>> omitted here was never addressed AFAICT.
>>
>> [1]: https://lore.kernel.org/linux-iio/20231217135007.3e5d959a@jic23-huawei/
> 
> I'd forgotten about that.  Adding interrupt-names would be the easiest
> way to resolve this.
> 

I'll add this, but my curiosity for the long run is: How should
differences between what bindings include and what drivers support
should be managed and documented?

...

>>> +
>>> +  refin-supply:
>>> +    description: external reference supply, can be used as reference for conversion.  
>>
>> If I'm understanding correctly, this represents both voltage inputs
>> REF+ and REF-, correct? The datasheet says "Reference Input Negative
>> Terminal. REF− can span from AVSS to AVDD1 − 1 V". It seems like they
>> should be separate supplies in case REF- is non-zero. Otherwise, how
>> can we know what voltage it is? (same comment applies to refin2.)
> 
> Agreed, in this case these are directly used as references (we recently
> had another driver that could take a wide range of negative and positive
> inputs but in that case an internal reference was generated that didn't
> made it not matter exactly what was being supplied.  Not true here though!
> 
Wouldn't it be alright to specify that the voltage specified here should
be the actual difference (REF+)-(REF-)?

...

>>> +required:
>>> +  - compatible
>>> +  - reg
>>> +  - interrupts  
>>
>> Why are interrupts required? What if the pin is not connected?
>>
> Ah. I clearly failed to review this one closely enough.
> 
> Absolutely agree that interrupts should never be required.
> No need for the driver to work if they aren't, but the binding
> shouldn't require them!
> 
> Jonathan
> 

To make sure that I understand, the driver will not probe without
interrupts, but it is alright to make then optional in the bindings?

This is in the case that someone will want to use this binding and
implement reading with polling?