Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1417619

Re: [PATCH 2/3] iio: adc: ina3221: Add support for IIO ADC driver for TI INA3221

Show all headers | View raw

On Wednesday 08 June 2016 08:34 PM, Andrew F. Davis wrote:
> On 06/07/2016 05:30 PM, Guenter Roeck wrote:
>> On Fri, Jun 03, 2016 at 10:17:55AM -0500, Andrew F. Davis wrote:
>>> On 06/03/2016 09:14 AM, Laxman Dewangan wrote:
>>>> On Friday 03 June 2016 06:59 PM, Guenter Roeck wrote:
>>>>> On 06/03/2016 03:06 AM, Jonathan Cameron wrote:
>>>>>> On 01/06/16 13:34, Laxman Dewangan wrote:
>>>>>>> The INA3221 is a three-channel, high-side current and bus voltage
>>>>>>> monitor
>>>>>>> with an I2C interface from Texas Instruments. The INA3221 monitors both
>>>>>>> shunt voltage drops and bus supply voltages in addition to having
>>>>>>> programmable conversion times and averaging modes for these signals.
>>>>>>> The INA3221 offers both critical and warning alerts to detect multiple
>>>>>>> programmable out-of-range conditions for each channel.
>>>>>>>
>>>>>>> Add support for INA3221 SW driver via IIO ADC interface. The device is
>>>>>>> register as iio-device and provides interface for voltage/current
>>>>>>> and power
>>>>>>> monitor. Also provide interface for setting oneshot/continuous mode and
>>>>>>> critical/warning threshold for the shunt voltage drop.
>>>>>>>
>>>>>>> Signed-off-by: Laxman Dewangan <ldewangan@nvidia.com>
>>>>>> Hi Laxman,
>>>>>>
>>>>>> As ever with any driver lying on the border of IIO and hwmon, please
>>>>>> include
>>>>>> a short justification of why you need an IIO driver and also cc the
>>>>>> hwmon list + maintainers. (cc'd on this reply).
>>>>>>
>>>>>> I simply won't take a driver where the hwmon maintainers aren't happy.
>>>>>> As it stands I'm not seeing obvious reasons in the code for why this
>>>>>> should be an IIO device.
>>>>>>
>>>>> Me not either.
>>>>>
>>>>> I have a hwmon driver for the same chip pending from Andrew Davis (TI)
>>>>> which I am just about to accept. We had directed Andrew back in April
>>>>> to write a hwmon driver for the chip, which he did.
>>>>>
>>>> Thanks Guenter,  I found the series
>>>>
>>>> [PATCH v2 0/2] Add support for INA3221 Triple Current/Voltage Monitors
>>>> Looks fine to me. I can use the hwmon.
>>>>
>>> If you search even further back you can see I originally went with an
>>> IIO driver as well, comparing the IIO and hwmon version for the simple
>>> use cases I needed the hwmod version turned out much simpler, so it was
>>> probably the right framework for now.
>>>
>>>> However, some of the stuff from my patch are not there which I will add
>>>> later once original patch applied:
>>>> - Dynamic mode changes for continuous and one-shot from sysfs.
>>>> - In one shot, when try to read voltage data, do conversion and then read.
>>>>
>>> My attempt is rather minimal, to be honest I like your stab at this
>>> driver better in some ways, especially relating to DT putting each
>>> channel in its own node with labels, I think this is a bit more clean
>>> and will be more extendable if/when new multi-channel monitor chips are
>>> made.
>>>
>>>> Not sure whether exporting the following will help or not. Can you
>>>> please confirm?
>>>> - Oversampling time i.e. average sample
>>>> - conversion time for bus voltage and shunt voltage if default is not
>>>> suited for system.
>>>>
>>>>
>>> These can always be added as needed, but the DT changes are not as easy.
>>> I would like it if this got in this cycle but if you think something
>>> will be needed to help your improvements, that can not be added
>>> incrementally, it would probably be best to get them into the initial DT
>>> binding doc now.
>>>
>> Andrew,
>>
>> with this, the hwmon driver is pretty much on hold. What do you want me to do ?
>> Should I wait for DT updates ?
>>
> If Laxman would like to commit to making these changes I do not problem
> waiting a bit to get this right the first time.
>
> If we don't hear back soon then I'd just take it as is and anything
> needing to be fixed can be later.
>
>

I will be able to post the patch tomorrow.
I think this series can be applied and then on top of it, I will post 
the dt changes and other my changes.

As this is new driver and no one using now, it will be fine to have dt 
changes if everything complete in one cycle.

I will work from the tree on which this applied to post my patches.

Back to linux.kernel | Previous | Next — Previous in thread | Next in thread | Find similar | Unroll thread

Thread

[PATCH 1/3] iio: adc: ina3221: Add DT binding details Laxman Dewangan <ldewangan@nvidia.com> - 2016-06-01 14:50 +0200
  [PATCH 2/3] iio: adc: ina3221: Add support for IIO ADC driver for TI INA3221 Laxman Dewangan <ldewangan@nvidia.com> - 2016-06-01 14:50 +0200
    Re: [PATCH 2/3] iio: adc: ina3221: Add support for IIO ADC driver for  TI INA3221 Jonathan Cameron <jic23@kernel.org> - 2016-06-03 12:10 +0200
      Re: [PATCH 2/3] iio: adc: ina3221: Add support for IIO ADC driver  for TI INA3221 Laxman Dewangan <ldewangan@nvidia.com> - 2016-06-03 13:40 +0200
        Re: [PATCH 2/3] iio: adc: ina3221: Add support for IIO ADC driver for  TI INA3221 Jonathan Cameron <jic23@kernel.org> - 2016-06-03 14:10 +0200
          Re: [PATCH 2/3] iio: adc: ina3221: Add support for IIO ADC driver  for TI INA3221 Laxman Dewangan <ldewangan@nvidia.com> - 2016-06-03 14:30 +0200
      Re: [PATCH 2/3] iio: adc: ina3221: Add support for IIO ADC driver  for TI INA3221 Laxman Dewangan <ldewangan@nvidia.com> - 2016-06-03 13:50 +0200
        Re: [PATCH 2/3] iio: adc: ina3221: Add support for IIO ADC driver for  TI INA3221 Jonathan Cameron <jic23@kernel.org> - 2016-06-03 14:10 +0200
          Re: [PATCH 2/3] iio: adc: ina3221: Add support for IIO ADC driver  for TI INA3221 Laxman Dewangan <ldewangan@nvidia.com> - 2016-06-03 14:20 +0200
      Re: [PATCH 2/3] iio: adc: ina3221: Add support for IIO ADC driver for  TI INA3221 Guenter Roeck <linux@roeck-us.net> - 2016-06-03 15:30 +0200
        Re: [PATCH 2/3] iio: adc: ina3221: Add support for IIO ADC driver  for TI INA3221 Laxman Dewangan <ldewangan@nvidia.com> - 2016-06-03 16:30 +0200
          Re: [PATCH 2/3] iio: adc: ina3221: Add support for IIO ADC driver for  TI INA3221 "Andrew F. Davis" <afd@ti.com> - 2016-06-03 17:20 +0200
            Re: [PATCH 2/3] iio: adc: ina3221: Add support for IIO ADC driver  for TI INA3221 Guenter Roeck <linux@roeck-us.net> - 2016-06-08 00:40 +0200
              Re: [PATCH 2/3] iio: adc: ina3221: Add support for IIO ADC driver for  TI INA3221 "Andrew F. Davis" <afd@ti.com> - 2016-06-08 17:10 +0200
                Re: [PATCH 2/3] iio: adc: ina3221: Add support for IIO ADC driver  for TI INA3221 Laxman Dewangan <ldewangan@nvidia.com> - 2016-06-08 18:00 +0200
  [PATCH 3/3] iio: adc: ina3221: Add sysfs details for TI INA3221 Laxman Dewangan <ldewangan@nvidia.com> - 2016-06-01 14:50 +0200
    Re: [PATCH 3/3] iio: adc: ina3221: Add sysfs details for TI INA3221 Jonathan Cameron <jic23@kernel.org> - 2016-06-03 12:30 +0200
  Re: [PATCH 1/3] iio: adc: ina3221: Add DT binding details Rob Herring <robh@kernel.org> - 2016-06-03 04:10 +0200
    Re: [PATCH 1/3] iio: adc: ina3221: Add DT binding details Laxman Dewangan <ldewangan@nvidia.com> - 2016-06-03 11:20 +0200
    Re: [PATCH 1/3] iio: adc: ina3221: Add DT binding details Jonathan Cameron <jic23@kernel.org> - 2016-06-03 12:20 +0200
      Re: [PATCH 1/3] iio: adc: ina3221: Add DT binding details Laxman Dewangan <ldewangan@nvidia.com> - 2016-06-03 14:10 +0200
        Re: [PATCH 1/3] iio: adc: ina3221: Add DT binding details Jonathan Cameron <jic23@kernel.org> - 2016-06-03 14:20 +0200
          Re: [PATCH 1/3] iio: adc: ina3221: Add DT binding details Laxman Dewangan <ldewangan@nvidia.com> - 2016-06-03 14:40 +0200

csiph-web