| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <51E9A240.3000108@kernel.org>
Date: Fri, 19 Jul 2013 21:32:00 +0100
From: Jonathan Cameron <jic23@...nel.org>
To: Hector Palacios <hector.palacios@...i.com>
CC: Marek Vasut <marex@...x.de>,
"linux-iio@...r.kernel.org" <linux-iio@...r.kernel.org>,
"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
"devicetree-discuss@...ts.ozlabs.org"
<devicetree-discuss@...ts.ozlabs.org>,
"alexandre.belloni@...e-electrons.com"
<alexandre.belloni@...e-electrons.com>,
"lars@...afoo.de" <lars@...afoo.de>,
"fabio.estevam@...escale.com" <fabio.estevam@...escale.com>
Subject: Re: [PATCH v2 5/5] iio: mxs-lradc: add write_raw function to modify
scale
On 07/19/2013 04:31 PM, Hector Palacios wrote:
> 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@...i.com>
>>> ---
>>> 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.
Could even make the int + nano part a structure then you could have
scale_avail[chan->channel][LRADC_DIV_BY_2].integer / .nano
might not be worth the hassel though for the slight gain in readability.
I'm happy either way.
>
> 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
> --
> To unsubscribe from this list: send the line "unsubscribe linux-iio" in
> the body of a message to majordomo@...r.kernel.org
> More majordomo info at http://vger.kernel.org/majordomo-info.html
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Powered by blists - more mailing lists