Re: [PATCH 1/3] iio: Add ADM1177 Hot Swap Controller and Digital Power Monitor driver

[Guenter Roeck]

On Wed, Nov 13, 2019 at 08:10:50AM +0000, Hennerich, Michael wrote:
> 
> 
> > -----Original Message-----
> > From: Guenter Roeck <groeck7@xxxxxxxxx> On Behalf Of Guenter Roeck
> > Sent: Dienstag, 12. November 2019 20:18
> > To: Jonathan Cameron <jonathan.cameron@xxxxxxxxxx>
> > Cc: Bia, Beniamin <Beniamin.Bia@xxxxxxxxxx>; jic23@xxxxxxxxxx;
> > lars@xxxxxxxxxx; Hennerich, Michael <Michael.Hennerich@xxxxxxxxxx>;
> > pmeerw@xxxxxxxxxx; gregkh@xxxxxxxxxxxxxxxxxxx; linux-
> > iio@xxxxxxxxxxxxxxx; devel@xxxxxxxxxxxxxxxxxxxx; linux-
> > kernel@xxxxxxxxxxxxxxx; mark.rutland@xxxxxxx; robh+dt@xxxxxxxxxx;
> > devicetree@xxxxxxxxxxxxxxx; paulmck@xxxxxxxxxxxxx;
> > mchehab+samsung@xxxxxxxxxx; linus.walleij@xxxxxxxxxx;
> > nicolas.ferre@xxxxxxxxxxxxx; biabeniamin@xxxxxxxxxxx; Jean Delvare
> > <jdelvare@xxxxxxxx>
> > Subject: Re: [PATCH 1/3] iio: Add ADM1177 Hot Swap Controller and Digital
> > Power Monitor driver
> > 
> > On Tue, Nov 12, 2019 at 05:37:57PM +0000, Jonathan Cameron wrote:
> > > On Tue, 12 Nov 2019 17:35:50 +0200
> > > Beniamin Bia <beniamin.bia@xxxxxxxxxx> wrote:
> > >
> > > > From: Michael Hennerich <michael.hennerich@xxxxxxxxxx>
> > > >
> > > > ADM1177 is a Hot Swap Controller and Digital Power Monitor with Soft
> > > > Start Pin.
> > > >
> > > > Datasheet:
> > > > Link:
> > > > https://www.analog.com/media/en/technical-documentation/data-
> > sheets/
> > > > ADM1177.pdf
> > > >
> > > > Signed-off-by: Michael Hennerich <michael.hennerich@xxxxxxxxxx>
> > > > Co-developed-by: Beniamin Bia <beniamin.bia@xxxxxxxxxx>
> > > > Signed-off-by: Beniamin Bia <beniamin.bia@xxxxxxxxxx>
> > >
> > > Hi Beniamin,
> > >
> > > A couple immediate thoughts.
> > >
> > > 1. That cc list has some rather non obvious people on it.  Unless something
> > >    fairly surprising is going on, probably better to cut it back a bit.
> > >
> > > 2. Why IIO?  Not entirely obvious to me.  From first glance this is definitely
> > >    doing hardware monitoring.  If there is a reason there should be a clear
> > >    statement here on why.
> > >
> > 
> > I don't see why this is implemented as iio driver. I think it should be
> > implemented as hardware monitoring driver.
> 
> Totally agree that this driver could have been implemented as HWMON driver.
> Well we use this device as USB supply monitor on our embedded portably kits, to detect low VBUS or excess current draw. 
> 
> ADALM-PLUTO and ADALM2000:
> https://www.analog.com/en/design-center/evaluation-hardware-and-software/evaluation-boards-kits/adalm-pluto.html
> 
> https://www.analog.com/en/design-center/evaluation-hardware-and-software/evaluation-boards-kits/ADALM2000.html
> 
> The only connectivity to the host PC is via IIO/libiio USB Gadget FS and Ethernet backends.
> 
> We recommend people to read the IIO attributes whenever they report an issue.
> Unless libiio supports directly HWMON or HWMON adds an IIO bridge we would prefer this driver being exposed as IIO device, since HWMON users still can use the IIO/HWMON bridge.
> 

I do not think this is a valid argument.

- This is a hardware monitoring chip.
- Implementing kernel support as IIO driver, keeping it out of tree for years,
  establishing an iio based use case, and then pressuring kernel maintainers
  to accept an iio driver seems inappropriate.
- The argument of "we need libiio support for this chip" could effectively
  be used to re-implement pretty much all hwmon drivers as iio drivers.
- The iio->hwmon bridge would add complexity for the majority of potential
  users of this chip. Focus should be on the majority, not on one use case.
- Userspace may as well use libsensors and/or sensors to do the necessary
  access - or implement it if neded. Or add a libsensors based backend to
  libiio (or to iiod).
- Last but not least, it would be more appropriate to implement a generic
  hwmon->iio bridge for iio use cases for chips supported by hwmon drivers,
  similar to the hwmon->thermal bridge.

Guenter