Re: [PATCH 2/2] hwmon: Add ads1000/ads1100 voltage ADCs driver
From: Serge Semin
Date: Mon Jun 10 2019 - 11:01:33 EST
On Fri, Jun 07, 2019 at 06:02:48PM -0700, Guenter Roeck wrote:
> On 6/7/19 4:01 PM, Serge Semin wrote:
> > Hello folks
> > On Wed, Jun 05, 2019 at 01:55:56PM -0700, Guenter Roeck wrote:
> > > On Mon, Jun 03, 2019 at 12:11:17PM +0100, Jonathan Cameron wrote:
> > > > On Thu, 30 May 2019 05:55:10 -0700
> > > > Guenter Roeck <linux@xxxxxxxxxxxx> wrote:
> > > >
> > > > > Hi,
> > > > >
> > > > > On Wed, May 15, 2019 at 01:58:09AM +0300, Serge Semin wrote:
> > > > > > These are simple Texas Instruments ADC working over i2c-interface with
> > > > > > just one differential input and with configurable 12-16 bits resolution.
> > > > > > Sample rate is fixed to 128 for ads1000 and can vary from 8 to 128 for
> > > > > > ads1100. Vdd value reference value must be supplied so to properly
> > > > > > translate the sampled code to the real voltage. All of these configs are
> > > > > > implemented in the device drivers for hwmon subsystem. The next dts
> > > > > > properties should be specified to comply the device platform setup:
> > > > > > - vdd-supply - voltage regulator connected to the Vdd pin of the device
> > > > > > - ti,gain - programmable gain amplifier
> > > > > > - ti,datarate - converter data rate
> > > > > > - ti,voltage-divider - possible resistors-base external divider
> > > > > > See bindings documentation file for details.
> > > > > >
> > > > > > Even though these devices seem more like ads1015 series, they
> > > > > > in fact pretty much different. First of all ads1000/ads1100 got less
> > > > > > capabilities: just one port, no configurations of digital comparator, no
> > > > > > input multi-channel multiplexer, smaller PGA and data-rate ranges.
> > > > > > In addition they haven't got internal voltage reference, but instead
> > > > > > are created to use Vdd pin voltage. Finally the output code value is
> > > > > > provided in different format. As a result it was much easier for
> > > > > > development and for future support to create a separate driver.
> > > > >
> > > > > This chicp doesn't have any real hardware monitoring characteristics
> > > > > (no limit registers). It seems to be better suited to be implemented
> > > > > as iio driver. If it is used as hardware monitor, the iio-hwmon bridge
> > > > > should work just fine.
> > > > >
> > > > > Jonathan, what do you think ?
> > > > Sorry for slow response, was on vacation.
> > > >
> > > > Agreed, this looks like a standard multipurpose ADC so probably more suited
> > > > to IIO. Whether you bother with a buffered /chardev interface or not given it
> > > > is a fairly slow device is a separate question (can always be added later
> > > > when someone wants it).
> > > >
> > > > Note the voltage-divider in the DT properties is something that should
> > > > have a generic representation. In IIO we have drivers/iio/afe/iio-rescale.c
> > > > for that, in this case using the voltage divider binding.
> > > >
> > > > gain and datarate are both characteristics that should be controlled from
> > > > userspace rather than via a binding.
> > > >
> > >
> > > In summary: Serge, please re-implement the driver as iio adc driver.
> > >
> > Thanks for the comments. I see your point, but since you are asking of a pretty
> > much serious code redevelopment, I want to make sure it is fully justified.
> > I made my decision of creating the hwmon driver following the next logic.
> > Before I started this driver development, I searched the kernel for either a
> > ready-to-use code or for a similar device driver to add the ads1000 ADC support.
> > I found the ads1015 driver, which is created for TI ADC1015 ADCs. These devices
> > are similar to the ads1000 series, but are more complex. Due to the complexity
> > I decided to create a separate driver for ads1000s, and of course since the similar
> > device driver lived in hwmon, I chose it to be home of my new driver.
> > But now you are asking me to move it to IIO, while the driver of more complex
> > ads1015 device exists in the hwmon subsystem of the kernel. Moreover the ads1000
> A driver for ADS1015 also exists in drivers/iio/adc/ti-ads1015.c, meaning there
> are already two drivers for that chip. Accepting the driver for ads1000 into
> hwmon would ultimately mean that we would end up with another duplicate driver,
> as soon as someone needs iio support for this chip. From hwmon perspective,
> that driver would have zero additional functionality.
> Users would then have to choose between the hwmon ads1000 driver and the iio
> ads1000 driver plus iio->hwmon bridge. The kernel maintainers would have to
> maintain two drivers instead of one, for no good reason. We would therefore
> at that time remove hwmon driver from the kernel because it doesn't make sense
> to keep two drivers for the same chip if both drivers provide exactly the same
> functionality. This just doesn't make sense.
> On top of that, the ads1000 has zero characteristics of a typical hardware
> monitoring chip. It doesn't have any limit or alarm status registers.
> > device is utilized on our board to monitor system itself (voltage on the input
> > DC-DC). Could you please tell me why the driver should really be in IIO instead
> > of hwmon and how do you select which subsystem one or another driver is supposed
> > to live in?
> If a chip has typical hardware monitoring characteristics such as slow but accurate
> conversion rates and limit/alarm registers, we are happy to accept it into the
> hardware monitoring subsystem. If the chip has no such characteristics,
> it should be implemented as iio driver.
> Actually, we should remove the ads1015 driver from the hwmon subsystem.
> I'll start a separate thread to discuss that.
Thank you very much for the detailed explanation. I can't believe I missed that
iio-version of ads1015. I'll take a look at that. If it's possible to add
ads1000 support right into the ads1015 drive, I'll do this. Otherwise I'll
port the ads1000 to the IIO subsystem as you suggested.