Re: [PATCH v2 5/5] iio: mxs-lradc: add write_raw function to modify scale

[Marek Vasut <marex-ynQEQJNshbs@public.gmane.org>]

Dear Hector Palacios,

> Dear Marek,
> 
> On 07/19/2013 04:39 PM, Marek Vasut wrote:
> > Dear Hector Palacios,
> > 
> >> Added write_raw function to manipulate the optional divider_by_two
> >> through the scaling attribute out of the available scales.
> >> 
> >> Signed-off-by: Hector Palacios <hector.palacios-i7dp0qKlBMg@public.gmane.org>
> >> ---
> >> 
> >>   drivers/staging/iio/adc/mxs-lradc.c | 55
> >> 
> >> ++++++++++++++++++++++++++++++++++++- 1 file changed, 54 insertions(+),
> >> 1 deletion(-)
> >> 
> >> diff --git a/drivers/staging/iio/adc/mxs-lradc.c
> >> b/drivers/staging/iio/adc/mxs-lradc.c index c929733..286cde2 100644
> >> --- a/drivers/staging/iio/adc/mxs-lradc.c
> >> +++ b/drivers/staging/iio/adc/mxs-lradc.c
> >> @@ -144,6 +144,7 @@ struct mxs_lradc {
> >> 
> >>   	uint32_t		vref_mv[LRADC_MAX_TOTAL_CHANS];
> >>   	unsigned int		scale_avail[LRADC_MAX_TOTAL_CHANS][2][2];
> >> 
> >> +	unsigned int		is_divided[LRADC_MAX_TOTAL_CHANS];
> > 
> > Why not use bitfield ? ;-)
> 
> This is used in some math below and an unsigned int looked more
> appropriate:
> 
> 	case IIO_CHAN_INFO_SCALE:
> 		*val = lradc->vref_mv[chan->channel];
> 		*val2 = chan->scan_type.realbits -
> 			lradc->is_divided[chan->channel];
> 		ret = IIO_VAL_FRACTIONAL_LOG2;
> 		break;

Oh ok.

> >>   	/*
> >>   	
> >>   	 * Touchscreen LRADC channels receives a private slot in the CTRL4
> >> 
> >> @@ -202,6 +203,7 @@ struct mxs_lradc {
> >> 
> >>   #define	LRADC_CTRL1_LRADC_IRQ_OFFSET		0
> >>   
> >>   #define	LRADC_CTRL2				0x20
> >> 
> >> +#define	LRADC_CTRL2_DIVIDE_BY_TWO_OFFSET	24
> >> 
> >>   #define	LRADC_CTRL2_TEMPSENSE_PWD		(1 << 15)
> >>   
> >>   #define	LRADC_STATUS				0x40
> >> 
> >> @@ -310,7 +312,8 @@ static int mxs_lradc_read_raw(struct iio_dev
> >> *iio_dev,
> >> 
> >>   		break;
> >>   	
> >>   	case IIO_CHAN_INFO_SCALE:
> >>   		*val = lradc->vref_mv[chan->channel];
> >> 
> >> -		*val2 = chan->scan_type.realbits;
> >> +		*val2 = chan->scan_type.realbits -
> >> +			lradc->is_divided[chan->channel];
> >> 
> >>   		ret = IIO_VAL_FRACTIONAL_LOG2;
> >>   		break;
> >>   	
> >>   	default:
> >> @@ -323,6 +326,54 @@ static int mxs_lradc_read_raw(struct iio_dev
> >> *iio_dev,
> >> 
> >>   	return ret;
> >>   
> >>   }
> >> 
> >> +static int mxs_lradc_write_raw(struct iio_dev *iio_dev,
> >> +			       const struct iio_chan_spec *chan,
> >> +			       int val, int val2, long m)
> >> +{
> >> +	struct mxs_lradc *lradc = iio_priv(iio_dev);
> >> +	int ret;
> >> +
> >> +	ret = mutex_trylock(&lradc->lock);
> >> +	if (!ret)
> >> +		return -EBUSY;
> >> +
> >> +	switch (m) {
> >> +	case IIO_CHAN_INFO_SCALE:
> >> +		ret = -EINVAL;
> >> +		if (val == lradc->scale_avail[chan->channel][0][0] &&
> >> +		    val2 == lradc->scale_avail[chan->channel][0][1]) {
> >> +			/* [0] -> divider by two disabled */
> > 
> > This comment is confusing, you use [0] in different dimensions of the
> > array. So is the stuff below.
> > 
> > Still, how does this even work, can you show me and example how to set
> > the divider from userland ? Sorry, I'm a bit confused with this
> > 3D-business here, even if the comment in previous patch made it a bit
> > clearer. Actually you can use some #define to specify if you're
> > accessing div/2 or div/1 portion of the array to make it more readable.
> > 
> > Like ... scale_avail[chan->channel][LRADC_DIV_BY_2][LRADC_DECIMAL_PART]
> > ... would by nice.
> 
> Agreed.
> 
> How it works:
> # cd /sys/devices/80000000.apb/80040000.apbx/80050000.lradc/iio:device0
> 
> Here you have three entries per channel:
> in_voltageX_raw		-> the sample raw value
> in_voltageX_scale	-> the scale to multiply the raw value to get the 
voltage
> in mV in_voltageX_scale_available -> lists the available scales of the
> channel
> 
> For example for channel 0:
> 
> # cat in_voltage0_scale_available
> 0.451660156 0.903320312	(two scales available, first with divider by two
> disabled, second with divider by two enabled)
> 
> # cat in_voltage0_scale
> 0.451660156			(divider by two is currently disabled)
> 
> # cat in_voltage0_raw		(shows measured value)
> 1000
> 
> If you multiply the value by the scale you get: 1000 * 0.451660156 = 451.6
> mV
> 
> # echo 0.903320312 > in_voltage0_scale	(enables the divider by two)

Ok, so I have to remember this value : '0.903320312' in case I want to enable 
divide-by-two functionality? Hmmmm ... why would this interface not work:

echo 2 > in_voltage0_scale

or 

echo 1 > in_voltage0_scale

?

> # cat in_voltage0_raw		(shows measured value)
> 500
> 
> Voltage at channel is the same but now measured value is applying the scale
> so it shows half the value than before. Now if you multiply: 500 *
> 0.903320312 = 451.6 mV (the same voltage but you now have a bigger scale
> and can measure up to 3.7V).
> 
> Other channels (like 10 on the MX28) will show different scales because of
> fixed predividers.
> The multi-dimension array is needed to store the big decimal number.

Yes, understood. Thanks for the explanation, it really helped!