| 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: <566E96C3.40905@baylibre.com>
Date: Mon, 14 Dec 2015 11:15:31 +0100
From: Marc Titinger <mtitinger@...libre.com>
To: Jonathan Cameron <jic23@...nel.org>, knaack.h@....de,
lars@...afoo.de, pmeerw@...erw.net
Cc: linux-iio@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH 3/3] iio: ina2xx: add support for CHAN_INFO_SCALE
On 12/12/2015 18:14, Jonathan Cameron wrote:
> On 11/12/15 16:49, Marc Titinger wrote:
>> Provide client apps with the scales to apply to the register values
>> read from the software buffer.
>>
>> Follow the ABI documentation so that values are in milli-unit after scales
>> are applied.
> Umm. The below looks like it is doing rather more than this..
>
> There is an endian switch! I'm going to assume that is correct
> for now and merge it into the original patch (rather than as a follow
> up). If this is wrong and should not be here shout quickly and loudly!
>
I know...I think the endianess issue was not spotted in my tests until I
had the scales coded-in: values in direct read mode were processed
before, and hence ok.
The endianness hint was wrong in streamed mode, but I did only
functional check so far. This is now tested with he sigrok-iio draft
from my colleague and values are now fine with scales applied on the
buffer samples.
> Will take the rest of this patch as is though I would much have
> prefered that this one was rolled into the original driver as
> I hadn't taken that when you sent this change...
>
> Actually never mind I'll smash it into the original driver as git
> seems to be happy to handle that bit of reordering and I haven't
> pushed out yet.
Yes thank you for doing that, it's the good option I think.
>
> So applied as a fixup to the original driver patch.
> Hmm. a few mor bits came up build testing this - such as lack of static on the
> the buffer enable / disable functions.
>
> Please check nothing went wrong in my making various minor changes.
> I've done basic build tests but obviously that doesn't catch everything!
Tested ok with iio utils (using iio_monitor for scales) and and the
sigrok-iio draft using 3 instances of the driver.
Many thanks,
Marc.
>
> Jonathan
>
>>
>> Signed-off-by: Marc Titinger <mtitinger@...libre.com>
>> ---
>> drivers/iio/adc/ina2xx-adc.c | 85 +++++++++++++++++++++-----------------------
>> 1 file changed, 41 insertions(+), 44 deletions(-)
>>
>> diff --git a/drivers/iio/adc/ina2xx-adc.c b/drivers/iio/adc/ina2xx-adc.c
>> index 99afa6e..98939ba 100644
>> --- a/drivers/iio/adc/ina2xx-adc.c
>> +++ b/drivers/iio/adc/ina2xx-adc.c
>> @@ -82,6 +82,11 @@ static bool ina2xx_is_volatile_reg(struct device *dev, unsigned int reg)
>> return (reg != INA2XX_CONFIG);
>> }
>>
>> +static inline bool is_signed_reg(unsigned int reg)
>> +{
>> + return (reg == INA2XX_SHUNT_VOLTAGE) || (reg == INA2XX_CURRENT);
>> +}
>> +
>> static const struct regmap_config ina2xx_regmap_config = {
>> .reg_bits = 8,
>> .val_bits = 16,
>> @@ -133,43 +138,6 @@ static const struct ina2xx_config ina2xx_config[] = {
>> },
>> };
>>
>> -static int ina2xx_get_value(struct ina2xx_chip_info *chip, u8 reg,
>> - unsigned int regval, int *val, int *uval)
>> -{
>> - *val = 0;
>> -
>> - switch (reg) {
>> - case INA2XX_SHUNT_VOLTAGE:
>> - /* signed register */
>> - *uval = DIV_ROUND_CLOSEST((s16) regval,
>> - chip->config->shunt_div);
>> - return IIO_VAL_INT_PLUS_MICRO;
>> -
>> - case INA2XX_BUS_VOLTAGE:
>> - *uval = (regval >> chip->config->bus_voltage_shift)
>> - * chip->config->bus_voltage_lsb;
>> - *val = *uval / 1000000;
>> - *uval = *uval % 1000000;
>> - return IIO_VAL_INT_PLUS_MICRO;
>> -
>> - case INA2XX_POWER:
>> - *uval = regval * chip->config->power_lsb;
>> - *val = *uval / 1000000;
>> - *uval = *uval % 1000000;
>> - return IIO_VAL_INT_PLUS_MICRO;
>> -
>> - case INA2XX_CURRENT:
>> - /* signed register, LSB=1mA (selected), in mA */
>> - *uval = (s16) regval * 1000;
>> - return IIO_VAL_INT_PLUS_MICRO;
>> -
>> - default:
>> - /* programmer goofed */
>> - WARN_ON_ONCE(1);
>> - }
>> - return -EINVAL;
>> -}
>> -
>> static int ina2xx_read_raw(struct iio_dev *indio_dev,
>> struct iio_chan_spec const *chan,
>> int *val, int *val2, long mask)
>> @@ -184,7 +152,12 @@ static int ina2xx_read_raw(struct iio_dev *indio_dev,
>> if (ret < 0)
>> return ret;
>>
>> - return ina2xx_get_value(chip, chan->address, regval, val, val2);
>> + if (is_signed_reg(chan->address))
>> + *val = (s16) regval;
>> + else
>> + *val = regval;
>> +
>> + return IIO_VAL_INT;
>>
>> case IIO_CHAN_INFO_OVERSAMPLING_RATIO:
>> *val = chip->avg;
>> @@ -208,11 +181,34 @@ static int ina2xx_read_raw(struct iio_dev *indio_dev,
>>
>> return IIO_VAL_INT;
>>
>> - default:
>> - return -EINVAL;
>> + case IIO_CHAN_INFO_SCALE:
>> + switch (chan->address) {
>> + case INA2XX_SHUNT_VOLTAGE:
>> + /* processed (mV) = raw*1000/shunt_div */
>> + *val2 = chip->config->shunt_div;
>> + *val = 1000;
>> + return IIO_VAL_FRACTIONAL;
>> +
>> + case INA2XX_BUS_VOLTAGE:
>> + /* processed (mV) = raw*lsb (uV) / (1000 << shift) */
>> + *val = chip->config->bus_voltage_lsb;
>> + *val2 = 1000 << chip->config->bus_voltage_shift;
>> + return IIO_VAL_FRACTIONAL;
>> +
>> + case INA2XX_POWER:
>> + /* processed (mW) = raw*lsb (uW) / 1000 */
>> + *val = chip->config->power_lsb;
>> + *val2 = 1000;
>> + return IIO_VAL_FRACTIONAL;
>> +
>> + case INA2XX_CURRENT:
>> + /* processed (mA) = raw (mA) */
>> + *val = 1;
>> + return IIO_VAL_INT;
>> + }
>> }
>>
>> - return 0;
>> + return -EINVAL;
>> }
>>
>> /*
>> @@ -395,7 +391,7 @@ static ssize_t ina2xx_shunt_resistor_store(struct device *dev,
>> .address = (_address), \
>> .indexed = 1, \
>> .channel = (_index), \
>> - .info_mask_separate = BIT(IIO_CHAN_INFO_RAW), \
>> + .info_mask_separate = BIT(IIO_CHAN_INFO_RAW)|BIT(IIO_CHAN_INFO_SCALE), \
> Spacing wrong about the |
>> .info_mask_shared_by_dir = BIT(IIO_CHAN_INFO_SAMP_FREQ) | \
>> BIT(IIO_CHAN_INFO_OVERSAMPLING_RATIO), \
>> .scan_index = (_index), \
>> @@ -403,7 +399,7 @@ static ssize_t ina2xx_shunt_resistor_store(struct device *dev,
>> .sign = 'u', \
>> .realbits = 16, \
>> .storagebits = 16, \
>> - .endianness = IIO_BE, \
>> + .endianness = IIO_LE, \
>> } \
>> }
>>
>> @@ -417,13 +413,14 @@ static ssize_t ina2xx_shunt_resistor_store(struct device *dev,
>> .indexed = 1, \
>> .channel = (_index), \
>> .info_mask_separate = BIT(IIO_CHAN_INFO_RAW) | \
>> + BIT(IIO_CHAN_INFO_SCALE) | \
>> BIT(IIO_CHAN_INFO_INT_TIME), \
>> .scan_index = (_index), \
>> .scan_type = { \
>> .sign = 'u', \
>> .realbits = 16, \
>> .storagebits = 16, \
>> - .endianness = IIO_BE, \
>> + .endianness = IIO_LE, \
>> } \
>> }
>>
>>
>
--
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