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

[Hector Palacios <hector.palacios-i7dp0qKlBMg@public.gmane.org>]

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;

>
>>   	/*
>>   	 * 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)

# 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.

>> +			writel(1 << LRADC_CTRL2_DIVIDE_BY_TWO_OFFSET,
>> +			       lradc->base + LRADC_CTRL2 + STMP_OFFSET_REG_CLR);
>> +			lradc->is_divided[chan->channel] = 0;
>> +			ret = 0;
>> +		} else if (val == lradc->scale_avail[chan->channel][1][0] &&
>> +			   val2 == lradc->scale_avail[chan->channel][1][1]) {
>> +			/* [1] -> divider by two enabled */
>> +			writel(1 << LRADC_CTRL2_DIVIDE_BY_TWO_OFFSET,
>> +			       lradc->base + LRADC_CTRL2 + STMP_OFFSET_REG_SET);
>> +			lradc->is_divided[chan->channel] = 1;
>> +			ret = 0;
>> +		}
>> +
>> +		break;
>> +	default:
>> +		ret = -EINVAL;
>> +		break;
>> +	}
>> +
>> +	mutex_unlock(&lradc->lock);
>> +
>> +	return ret;
>> +}
>> +
>> +static int mxs_lradc_write_raw_get_fmt(struct iio_dev *iio_dev,
>> +				       const struct iio_chan_spec *chan,
>> +				       long m)
>> +{
>> +	return IIO_VAL_INT_PLUS_NANO;
>> +}
>> +
>>   static ssize_t mxs_lradc_show_scale_available_ch(struct device *dev,
>>   		struct device_attribute *attr,
>>   		char *buf,
>> @@ -400,6 +451,8 @@ static const struct attribute_group
>> mxs_lradc_attribute_group = { static const struct iio_info
>> mxs_lradc_iio_info = {
>>   	.driver_module		= THIS_MODULE,
>>   	.read_raw		= mxs_lradc_read_raw,
>> +	.write_raw		= mxs_lradc_write_raw,
>> +	.write_raw_get_fmt	= &mxs_lradc_write_raw_get_fmt,
>
> Is this & needed here ?

No it isn't.
Thanks.

Best regards,
--
Hector Palacios