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@xxxxxxxxxxxx>
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.
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?
+
+ 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 ?
...
.driver_module = THIS_MODULE,
+};
+
+/*
Single line comment, use single line comment syntax.
ok
--
To unsubscribe from this list: send the line "unsubscribe linux-iio" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
