| 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
| ||
|
Date: Mon, 16 Nov 2015 17:27:02 +0000
From: Jonathan Cameron <jic23@...23.retrosnub.co.uk>
To: Marc Titinger <mtitinger@...libre.com>,
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,
mturquette@...libre.com, bcousson@...libre.com,
ptitiano@...libre.com
Subject: Re: [RFC v2 1/2] iio: ina2xx: add direct IO support for TI INA2xx Power Monitors
On 16 November 2015 09:31:17 GMT+00:00, Marc Titinger <mtitinger@...libre.com> wrote:
>On 14/11/2015 19:59, Jonathan Cameron wrote:
>> On 12/11/15 12:57, Marc Titinger wrote:
>>> Basic support or direct IO raw read, with averaging attribute.
>>> Values are RAW, INT_PLUS_MICRO (Volt/Ampere/Watt).
>>>
>>> Output of iio_info:
>>>
>>> iio:device0: ina226
>>> 4 channels found:
>>> power3: (input)
>>> 1 channel-specific attributes found:
>>> attr 0: raw value: 1.150000
>>> voltage0: (input)
>>> 1 channel-specific attributes found:
>>> attr 0: raw value: 0.000003
>>> voltage1: (input)
>>> 1 channel-specific attributes found:
>>> attr 0: raw value: 4.277500
>>> current2: (input)
>>> 1 channel-specific attributes found:
>>> attr 0: raw value: 0.268000
>>> 2 device-specific attributes found:
>>> attr 0: in_calibscale value: 10000
>>> attr 1: in_mean_raw value: 4
>>> attr 2: in_sampling_frequency value: 455
>>>
>>> Tested with ina226, on BeagleBoneBlack.
>>>
>>> Datasheet: http://www.ti.com/lit/gpn/ina226
>>>
>>> Signed-off-by: Marc Titinger <mtitinger@...libre.com>
>> Hi Marc
>>
>
>Hi Jonathan,
>
>> To express a personal preference, please don't have series of patches
>as
>> replies to the original thread. It gets really hard to follow after
>> a couple of revisions!
>>
>Ok, sorry for that.
>
>> May seem a random question, but what do you want to gain from an IIO
>> driver over what the hwmon provides?
>
>Good question. In the frame of Baylibre's ACME power and temperature
>monitoring demo based on Sigrock, we wish to add a remote mode for the
>GUI and processing front-end as well as explore other possibilities
>like
>using an on-board accelerator to capture the sensor data and stream it
>back to the front-end. This work is a pathfinder as IIO seems
>appropriate for what we want to do.
Fair enough I guess. Will need to check we aren't stepping on too many toes in
hwmon if we merge a second driver...
>
>>
>> Various bits inline..
>
>Thanks for the review, further questions below!
>
>Marc.
>
>>
>> Mostly looks pretty good though.
>>
>> Jonathan
>>> ---
>>>
>
>...
>
>>> +#define INA2XX_RSHUNT_DEFAULT 10000
>>> +
>>> +/* bit mask for reading the averaging setting in the configuration
>register */
>>> +#define INA226_AVG_RD_MASK 0x0E00
>> genmask is always good for these.
>>
>
>ok.
>
>..
>
>>> +
>>> +static bool ina2xx_is_writeable_reg(struct device *dev, unsigned
>int reg)
>>> +{
>>> + return (reg == INA2XX_CONFIG)
>>> + || (reg == INA2XX_CALIBRATION);
>> On one line I think this is still way less than 80 chars..
>>> +}
>
>ok
>
>...
>
>
>>> +struct ina2xx_chip_info {
>>> + struct iio_dev *indio_dev;
>> Having a pointer back to the indio_dev is usually a sign of
>> something 'unusual' going on... Will be interesting to see what it
>is for ;)
>>
>> Ah, that was easy, you don't use it that I can see.
>>
>
>Ah, forgot to remove it when splitting into two patches, but yeah
>you're
>right, I shall pass the indio_dev ptr as data in the first place.
>
>...
>
>>> +
>>> +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);
>>> + *val = *uval/1000000;
>>> + *uval = *uval - *val*1000000;
>>> + return IIO_VAL_INT_PLUS_MICRO;
>> I'm somewhat unconvinced that this wrapper is adding anything over
>> just doing this where you care about the result.
>> Also, this is a straight forward linear conversion. Do it in
>userspace
>> by providing the relevant 'scale' element.
>
>got it! A practical question: can you specify a divider rather than a
>multiplier as a scale so that userspace does the division?
Sort of see IIO_TYPE_FRACTIONAL
>
>>> +
>>> + case INA2XX_BUS_VOLTAGE:
>>> + *uval = (regval >> chip->config->bus_voltage_shift)
>>> + * chip->config->bus_voltage_lsb;
>>> + *val = *uval/1000000;
>>> + *uval = *uval - *val*1000000;
>>> + return IIO_VAL_INT_PLUS_MICRO;
>...
>
>>> + tmp = config;
>>> +
>>> + switch (mask) {
>>> + case IIO_CHAN_INFO_AVERAGE_RAW:
>>> +
>>> + ret = ina226_set_average(chip, val, &tmp);
>> This isn't what the ABI uses the info_average_raw attribute for -
>it's
>> for outputing the average of a channel rather than setting a mean
>> filter width (which is what you have here). Probably need some new
>ABI
>> for this as our current filter description ABI is rather limited!
>>
>Ah ok, should this go into a sysfs attribute then, until the ABI
>section
>is defined ? How about specifying the ABI section while we are at it ?
Go ahead and have a go at defining a suitable ABI :)
Post it as a separate RFC though as discussion might be rather in depth and
not really driver related.
>
>...
>
>.driver_module = THIS_MODULE,
>>> +};
>>> +
>>> +/*
>> Single line comment, use single line comment syntax.
>
>ok
--
Sent from my Android device with K-9 Mail. Please excuse my brevity.
--
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