Re: [PATCH 3/3] iio: adc: ina3221: Add sysfs details for TI INA3221

Jonathan Cameron
2016-06-03T10:26:10Z

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.
>
> The device export the SW interface with IIO framework. Detail out the
> all sysfs exposed by device for reference.
>
> Signed-off-by: Laxman Dewangan <ldewangan@nvidia.com>
> ---
> Documentation/iio/adc/ina3221.txt | 81 +++++++++++++++++++++++++++++++++++++++
> 1 file changed, 81 insertions(+)
> create mode 100644 Documentation/iio/adc/ina3221.txt
>
> diff --git a/Documentation/iio/adc/ina3221.txt b/Documentation/iio/adc/ina3221.txt
> new file mode 100644
> index 0000000..30cbd6d
> --- /dev/null
> +++ b/Documentation/iio/adc/ina3221.txt
> @@ -0,0 +1,81 @@
> +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.
> +
> +Driver exposes multiple sysfs whose details are as follows:
> +
> +/sys/bus/iio/devices/iio:device0/operating_mode:
> + Operating mode of device whether it is in oneshot mode or
> + continuous conversion mode.
> + To change mode to continuous mode say:
> + echo "c" > operating_mode
> + or
> + echo "C" > operating_mode
> +
> + To change mode to one shot mode, say
> + echo "o" > operating_mode
> + or
> + echo "O" > operating_mode
As I expressed when reviewing the driver I need significant convincing
on this. To my mind, should be in oneshot unless there is a reason
not to be.
> +
> +/sys/bus/iio/devices/iio:device0/continuous_oversampling_ratio:
> + Oversampling ratio (number of sample used for average) when device
> + is in continuous mode.
> + The possible values for number of average samples are:
> + 1, 4, 16, 64, 128, 256, 512, 1024
Not sure why this is separate for continuous and oneshot. If not
then it would be a standard interface.
> +
> +/sys/bus/iio/devices/iio:device0/continuous_vbus_conv_time:
> + ADC conversion time for voltage bus in continuous mode.
> + This is in microseconds.
> + The possible value for conversion times are:
> + 140, 204, 332, 588, 1100, 2116, 4156, 8244
Probably integration_time so standard ABI without the vbus
and continuous bits (which I'd argue don't make sense).
> +
> +/sys/bus/iio/devices/iio:device0/continuous_vshunt_conv_time:
> + ADC conversion time for shunt voltage in continuous mode.
> + This is in microseconds.
> + The possible value for conversion times are:
> + 140, 204, 332, 588, 1100, 2116, 4156, 8244
You feed in the shunt resistance via device tree so why not
just have this as in_current0_integration_time
> +
> +/sys/bus/iio/devices/iio:device0/oneshot_oversampling_ratio:
> + Oversampling ratio (number of sample used for average) when device
> + is in one-shot mode.
> + The possible values for number of average samples are:
> + 1, 4, 16, 64, 128, 256, 512, 1024
> +
> +/sys/bus/iio/devices/iio:device0/oneshot_vbus_conv_time:
> + ADC conversion time for voltage bus in one-shot mode.
> + This is in microseconds.
> + The possible value for conversion times are:
> + 140, 204, 332, 588, 1100, 2116, 4156, 8244
> +
> +/sys/bus/iio/devices/iio:device0/oneshot_vshunt_conv_time
> + ADC conversion time for shunt voltage in one-shot mode.
> + This is in microseconds.
> + The possible value for conversion times are:
> + 140, 204, 332, 588, 1100, 2116, 4156, 8244
> +
> +/sys/bus/iio/devices/iio:device0/rail_name_<x>
> + The rail name of each channels.
This needs more description to my mind to justify it's existence.
I have no problem with a channel label, but I think it should be
more generic ABI than this which is very power monitoring specific.
> +
> +/sys/bus/iio/devices/iio:device0/in_voltage<x>_input:
> + The voltage bus voltage measured by device. This is processed
> + data in microvolts.
Standard ABI covered in the generic docs so don't bother listing here.
> +
> +/sys/bus/iio/devices/iio:device0/in_current<x>_input:
> + The current flow in the bus in microamps.
> +
> +/sys/bus/iio/devices/iio:device0/in_power<x>_input:
> + The input power in the bus for each channel in micro-watt.
Raised this earlier. Device doesn't actually compute this so why
provide it to userspace? If you are doing it to make sure you get
a current / voltage pair (though wouldn't have thought that was
terrible critical here) then support a scan (buffered) interface
to provide time synced data.
> +
> +/sys/bus/iio/devices/iio:device0/in_current<x>_critical_input:
> + The critical current threshold on bus to generate critical
> + alert signal for each channel. This is provide in micro-amps.
> + The value can be override from sysfs for new critical current.
These are events - not channels.
> +
> +/sys/bus/iio/devices/iio:device0/in_current<x>_warning_input:
> + The warning current threshold on bus to generate warning
> + alert signal for each channel. This is provide in micro-amps.
> + The value can be override from sysfs for new warning threshold.
Use the events interface rather than an custom sysfs channel.
> +
>