Re: [PATCH 2/2] hwmon: Add ads1000/ads1100 voltage ADCs driver

[Guenter Roeck <linux@roeck-us.net>]

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@roeck-us.net> 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.

Thanks,
Guenter