On Wed, 17 Jan 2024 14:43:21 +0200 Ceclan Dumitru <mitrutzceclan@xxxxxxxxx> wrote: > 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? Drivers almost always support a subset of functionality of the device. This isn't much different. The driver 'should' use interrupt-names but it doesn't need to support all the things that the binding says should be in there. Sometimes we document things in a driver, but there isn't any obligation to do so and those docs are often out of date. > > ... > > >>> + > >>> + 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-)? How do you establish the offset to apply to single ended channels if you don't know the value of REF- (relative to local ground)? So no - as the device supports single ended channels the difference isn't enough information. It would probably be fine to do as you say if it were a device with only differential channels where all that matters is the scaling. > > ... > > >>> +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? Yes - it is fine for a driver to only support a subset of functionality and fail to probe if that subset isn't what the hardware enables. > > This is in the case that someone will want to use this binding and > implement reading with polling? Yes. J