Message-ID: <f3716c3c-695e-de7e-0419-698c621e76e5@kernel.org>
Date:	Fri, 3 Jun 2016 11:26:05 +0100
From:	Jonathan Cameron <jic23@...nel.org>
To:	Laxman Dewangan <ldewangan@...dia.com>, robh+dt@...nel.org,
	corbet@....net, lars@...afoo.de
Cc:	devicetree@...r.kernel.org, linux-kernel@...r.kernel.org,
	linux-doc@...r.kernel.org, linux-iio@...r.kernel.org
Subject: Re: [PATCH 3/3] iio: adc: ina3221: Add sysfs details for TI INA3221

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@...dia.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.
> +
> 

Powered by blists - more mailing lists