* [PATCH 1/6] IIO: core: add datasheet_name to chan_spec
2011-10-20 9:33 [RFC V3 PATCH 0/6] IIO in kernel interfaces Jonathan Cameron
@ 2011-10-20 9:33 ` Jonathan Cameron
2011-10-20 9:33 ` [PATCH 2/6] IIO:ADC:max1363 add datasheet_name entries Jonathan Cameron
` (4 subsequent siblings)
5 siblings, 0 replies; 18+ messages in thread
From: Jonathan Cameron @ 2011-10-20 9:33 UTC (permalink / raw)
To: linux-kernel, linux-iio
Cc: linus.ml.walleij, zdevai, linux, arnd, broonie, gregkh,
lm-sensors, guenter.roeck, khali, thomas.petazzoni, maxime.ripard,
Jonathan Cameron
This allows for matching against the name given
on a datasheet, however silly/inconsistent it might
be.
Useful for in kernel interfaces.
Signed-off-by: Jonathan Cameron <jic23@cam.ac.uk>
---
include/linux/iio/iio.h | 1 +
1 files changed, 1 insertions(+), 0 deletions(-)
diff --git a/include/linux/iio/iio.h b/include/linux/iio/iio.h
index 8066c8a..beedc5c 100644
--- a/include/linux/iio/iio.h
+++ b/include/linux/iio/iio.h
@@ -128,6 +128,7 @@ struct iio_chan_spec {
} scan_type;
long info_mask;
char *extend_name;
+ const char *datasheet_name;
unsigned processed_val:1;
unsigned modified:1;
unsigned indexed:1;
--
1.7.7
^ permalink raw reply related [flat|nested] 18+ messages in thread* [PATCH 2/6] IIO:ADC:max1363 add datasheet_name entries.
2011-10-20 9:33 [RFC V3 PATCH 0/6] IIO in kernel interfaces Jonathan Cameron
2011-10-20 9:33 ` [PATCH 1/6] IIO: core: add datasheet_name to chan_spec Jonathan Cameron
@ 2011-10-20 9:33 ` Jonathan Cameron
2011-10-20 9:33 ` [PATCH 3/6] IIO:CORE: put defs needed by inkern and userspace interfaces into chan_spec.h Jonathan Cameron
` (3 subsequent siblings)
5 siblings, 0 replies; 18+ messages in thread
From: Jonathan Cameron @ 2011-10-20 9:33 UTC (permalink / raw)
To: linux-kernel, linux-iio
Cc: linus.ml.walleij, zdevai, linux, arnd, broonie, gregkh,
lm-sensors, guenter.roeck, khali, thomas.petazzoni, maxime.ripard,
Jonathan Cameron
Kind of obvious for this device but useful
for testing purposes.
Signed-off-by: Jonathan Cameron <jic23@cam.ac.uk>
---
drivers/iio/adc/max1363_core.c | 4 +++-
1 files changed, 3 insertions(+), 1 deletions(-)
diff --git a/drivers/iio/adc/max1363_core.c b/drivers/iio/adc/max1363_core.c
index ee06d48..e035df9 100644
--- a/drivers/iio/adc/max1363_core.c
+++ b/drivers/iio/adc/max1363_core.c
@@ -251,7 +251,8 @@ static const enum max1363_modes max1363_mode_list[] = {
.indexed = 1, \
.channel = num, \
.address = addr, \
- .info_mask = MAX1363_INFO_MASK \
+ .info_mask = MAX1363_INFO_MASK, \
+ .datasheet_name = "AIN"#num \
} \
/* bipolar channel */
@@ -264,6 +265,7 @@ static const enum max1363_modes max1363_mode_list[] = {
.channel2 = num2, \
.address = addr, \
.info_mask = MAX1363_INFO_MASK, \
+ .datasheet_name = "AIN"#num"-AIN"#num2 \
}
#define MAX1363_INFO_MASK (1 << IIO_CHAN_INFO_SCALE_SHARED)
--
1.7.7
^ permalink raw reply related [flat|nested] 18+ messages in thread* [PATCH 3/6] IIO:CORE: put defs needed by inkern and userspace interfaces into chan_spec.h
2011-10-20 9:33 [RFC V3 PATCH 0/6] IIO in kernel interfaces Jonathan Cameron
2011-10-20 9:33 ` [PATCH 1/6] IIO: core: add datasheet_name to chan_spec Jonathan Cameron
2011-10-20 9:33 ` [PATCH 2/6] IIO:ADC:max1363 add datasheet_name entries Jonathan Cameron
@ 2011-10-20 9:33 ` Jonathan Cameron
2011-10-20 9:33 ` [PATCH 4/6] IIO:CORE add in kernel interface mapping and getting IIO channels Jonathan Cameron
` (2 subsequent siblings)
5 siblings, 0 replies; 18+ messages in thread
From: Jonathan Cameron @ 2011-10-20 9:33 UTC (permalink / raw)
To: linux-kernel, linux-iio
Cc: linus.ml.walleij, zdevai, linux, arnd, broonie, gregkh,
lm-sensors, guenter.roeck, khali, thomas.petazzoni, maxime.ripard,
Jonathan Cameron
Signed-off-by: Jonathan Cameron <jic23@cam.ac.uk>
---
include/linux/iio/chan_spec.h | 46 +++++++++++++++++++++++++++++++++++++++++
include/linux/iio/iio.h | 33 +----------------------------
2 files changed, 47 insertions(+), 32 deletions(-)
diff --git a/include/linux/iio/chan_spec.h b/include/linux/iio/chan_spec.h
new file mode 100644
index 0000000..933480b
--- /dev/null
+++ b/include/linux/iio/chan_spec.h
@@ -0,0 +1,46 @@
+/*
+ * The industrial I/O channel descriptions
+ *
+ * Copyright (c) 2008-2011 Jonathan Cameron
+ *
+ * This program is free software; you can redistribute it and/or modify it
+ * under the terms of the GNU General Public License version 2 as published by
+ * the Free Software Foundation.
+ */
+
+#ifndef _IIO_CHAN_SPEC_H_
+#define _IIO_CHAN_SPEC_H_
+
+enum iio_data_type {
+ IIO_RAW,
+ IIO_PROCESSED,
+};
+
+enum iio_direction {
+ IIO_IN,
+ IIO_OUT,
+};
+
+enum iio_chan_type {
+ IIO_VOLTAGE,
+ IIO_CURRENT,
+ IIO_POWER,
+ IIO_CAPACITANCE,
+ IIO_ACCEL,
+ IIO_ANGL_VEL,
+ IIO_MAGN,
+ IIO_LIGHT,
+ IIO_INTENSITY,
+ IIO_PROXIMITY,
+ IIO_TEMP,
+ IIO_INCLI,
+ IIO_ROT,
+ IIO_ANGL,
+ IIO_TIMESTAMP,
+};
+
+#define IIO_VAL_INT 1
+#define IIO_VAL_INT_PLUS_MICRO 2
+#define IIO_VAL_INT_PLUS_NANO 3
+
+#endif
diff --git a/include/linux/iio/iio.h b/include/linux/iio/iio.h
index beedc5c..8b98e92 100644
--- a/include/linux/iio/iio.h
+++ b/include/linux/iio/iio.h
@@ -9,6 +9,7 @@
*/
#include <linux/klist.h>
#include <linux/device.h>
+#include <linux/iio/chan_spec.h>
#ifndef _IIO_H_
#define _IIO_H_
@@ -16,29 +17,6 @@
/* Minimum alignment of priv within iio_dev */
#define IIO_ALIGN L1_CACHE_BYTES
-enum iio_data_type {
- IIO_RAW,
- IIO_PROCESSED,
-};
-
-enum iio_chan_type {
- IIO_VOLTAGE,
- IIO_CURRENT,
- IIO_POWER,
- IIO_CAPACITANCE,
- IIO_ACCEL,
- IIO_ANGL_VEL,
- IIO_MAGN,
- IIO_LIGHT,
- IIO_INTENSITY,
- IIO_PROXIMITY,
- IIO_TEMP,
- IIO_INCLI,
- IIO_ROT,
- IIO_ANGL,
- IIO_TIMESTAMP,
-};
-
enum iio_modifier {
IIO_NO_MOD,
IIO_MOD_X,
@@ -73,15 +51,6 @@ enum iio_chan_info_enum {
IIO_CHAN_INFO_QUADRATURE_CORRECTION_RAW_SEPARATE,
};
-enum iio_direction {
- IIO_IN,
- IIO_OUT,
-};
-
-#define IIO_VAL_INT 1
-#define IIO_VAL_INT_PLUS_MICRO 2
-#define IIO_VAL_INT_PLUS_NANO 3
-
/**
* struct iio_chan_spec - specification of a single channel
* @type: What type of measurement is the channel making.
--
1.7.7
^ permalink raw reply related [flat|nested] 18+ messages in thread* [PATCH 4/6] IIO:CORE add in kernel interface mapping and getting IIO channels.
2011-10-20 9:33 [RFC V3 PATCH 0/6] IIO in kernel interfaces Jonathan Cameron
` (2 preceding siblings ...)
2011-10-20 9:33 ` [PATCH 3/6] IIO:CORE: put defs needed by inkern and userspace interfaces into chan_spec.h Jonathan Cameron
@ 2011-10-20 9:33 ` Jonathan Cameron
2011-10-20 9:33 ` [PATCH 5/6] IIO:hwmon interface client driver Jonathan Cameron
2011-10-20 9:33 ` [PATCH 6/6] stargate2: example of map configuration for iio to hwmon example Jonathan Cameron
5 siblings, 0 replies; 18+ messages in thread
From: Jonathan Cameron @ 2011-10-20 9:33 UTC (permalink / raw)
To: linux-kernel, linux-iio
Cc: linus.ml.walleij, zdevai, linux, arnd, broonie, gregkh,
lm-sensors, guenter.roeck, khali, thomas.petazzoni, maxime.ripard,
Jonathan Cameron
Two elements here:
* Map as defined in include/linux/iio/inkern.h
* Matching code to actually get the iio_dev and channel
that we want from the global list of IIO devices.
Signed-off-by: Jonathan Cameron <jic23@cam.ac.uk>
---
drivers/Makefile | 2 +-
drivers/iio/Makefile | 1 +
drivers/iio/iio.c | 279 +++++++++++++++++++++++++++++++++++++++++++-
drivers/iio/inkern.c | 21 ++++
include/linux/iio/iio.h | 7 +-
include/linux/iio/inkern.h | 87 ++++++++++++++
6 files changed, 390 insertions(+), 7 deletions(-)
diff --git a/drivers/Makefile b/drivers/Makefile
index df39628..2b389c5 100644
--- a/drivers/Makefile
+++ b/drivers/Makefile
@@ -130,5 +130,5 @@ obj-$(CONFIG_VIRT_DRIVERS) += virt/
obj-$(CONFIG_HYPERV) += hv/
-obj-$(CONFIG_IIO) += iio/
+obj-y += iio/
diff --git a/drivers/iio/Makefile b/drivers/iio/Makefile
index db3c426..cfb588a 100644
--- a/drivers/iio/Makefile
+++ b/drivers/iio/Makefile
@@ -1,6 +1,7 @@
#
# Makefile for the Industrial I/O subsystem
#
+obj-y = inkern.o
obj-$(CONFIG_IIO) += iio.o
industrialio-y := core.o
diff --git a/drivers/iio/iio.c b/drivers/iio/iio.c
index 9e6acc1..e094c40 100644
--- a/drivers/iio/iio.c
+++ b/drivers/iio/iio.c
@@ -12,8 +12,10 @@
#include <linux/module.h>
#include <linux/slab.h>
#include <linux/idr.h>
+#include <linux/err.h>
#include <linux/iio/iio.h>
#include <linux/iio/sysfs.h>
+#include <linux/iio/inkern.h>
static DEFINE_IDA(iio_ida);
@@ -68,6 +70,278 @@ static const char * const iio_chan_info_postfix[] = {
= "quadrature_correction_raw",
};
+static void iio_dev_release(struct device *device);
+static struct device_type iio_dev_type = {
+ .name = "iio_device",
+ .release = iio_dev_release,
+};
+
+static int iio_match_dev(struct device *dev, void *data)
+{
+ struct iio_dev *indio_dev;
+ struct device *dev2 = data;
+
+ if (dev->type != &iio_dev_type)
+ return 0;
+
+ indio_dev = container_of(dev, struct iio_dev, dev);
+ if (indio_dev->info->get_hardware_id)
+ return indio_dev->info->get_hardware_id(indio_dev) == dev2;
+ else
+ return indio_dev->dev.parent == dev2;
+}
+
+static int iio_match_dev_name(struct device *dev, void *data)
+{
+ struct iio_dev *indio_dev;
+ const char *name = data;
+
+ if (dev->type != &iio_dev_type)
+ return 0;
+
+ indio_dev = container_of(dev, struct iio_dev, dev);
+ if (indio_dev->info->get_hardware_id)
+ return !strcmp(dev_name(indio_dev->info
+ ->get_hardware_id(indio_dev)),
+ name);
+ else if (indio_dev->dev.parent)
+ return !strcmp(dev_name(indio_dev->dev.parent), name);
+ return 0;
+}
+
+struct iio_channel *iio_channel_get(const struct device *dev,
+ const char *name,
+ const char *channel_name)
+{
+ struct iio_map *c_i = NULL, *c = NULL;
+ struct iio_dev *indio_dev = NULL;
+ const struct iio_chan_spec *chan = NULL;
+ struct device *dev_i;
+ int i;
+ struct iio_channel *channel;
+
+ if (dev == NULL && name == NULL && channel_name == NULL)
+ return ERR_PTR(-ENODEV);
+ /* first find matching entry the channel map */
+ list_for_each_entry(c_i, &iio_map_list, l) {
+ if ((dev && dev != c_i->consumer_dev) ||
+ (name && strcmp(name, c_i->consumer_dev_name) != 0) ||
+ (channel_name &&
+ strcmp(channel_name, c_i->consumer_channel) != 0))
+ continue;
+ c = c_i;
+ break;
+ }
+ if (c == NULL)
+ return ERR_PTR(-ENODEV);
+
+ /* now find the iio device if it has been registered */
+ if (c->adc_dev)
+ dev_i = bus_find_device(&iio_bus_type, NULL, c->adc_dev,
+ &iio_match_dev);
+ else if (c->adc_dev_name)
+ dev_i = bus_find_device(&iio_bus_type, NULL,
+ (void *)c->adc_dev_name,
+ &iio_match_dev_name);
+ else
+ return ERR_PTR(-EINVAL);
+ if (IS_ERR(dev_i))
+ return (void *)dev_i;
+ if (dev_i == NULL)
+ return ERR_PTR(-ENODEV);
+ indio_dev = container_of(dev_i, struct iio_dev, dev);
+
+ /* finally verify the channel exists */
+ if (c->adc_channel_label)
+ for (i = 0; i < indio_dev->num_channels; i++)
+ if (indio_dev->channels[i].datasheet_name &&
+ strcmp(c->adc_channel_label,
+ indio_dev->channels[i].datasheet_name)
+ == 0) {
+ chan = &indio_dev->channels[i];
+ break;
+ }
+ channel = kmalloc(sizeof(*channel), GFP_KERNEL);
+ if (channel == NULL)
+ return ERR_PTR(-ENOMEM);
+ channel->indio_dev = indio_dev;
+ if (chan == NULL)
+ channel->channel = &indio_dev->channels[c->channel_number];
+ else
+ channel->channel = chan;
+ return channel;
+}
+EXPORT_SYMBOL_GPL(iio_channel_get);
+
+static const struct iio_chan_spec
+*iio_chan_spec_from_name(const struct iio_dev *indio_dev,
+ const char *name)
+{
+ int i;
+ const struct iio_chan_spec *chan = NULL;
+ for (i = 0; i < indio_dev->num_channels; i++)
+ if (indio_dev->channels[i].datasheet_name &&
+ strcmp(name, indio_dev->channels[i].datasheet_name) == 0) {
+ chan = &indio_dev->channels[i];
+ break;
+ }
+ return chan;
+}
+
+struct iio_channel **iio_channel_get_all(const struct device *dev,
+ const char *name)
+{
+ struct iio_channel **chans;
+ struct iio_map *c = NULL;
+ struct iio_dev *indio_dev;
+ int nummaps = 0;
+ int mapind = 0;
+ int i, ret;
+ struct device *dev_i;
+
+ if (dev == NULL && name == NULL) {
+ ret = -EINVAL;
+ goto error_ret;
+ }
+
+ /* first count the matching maps */
+ list_for_each_entry(c, &iio_map_list, l)
+ if ((dev && dev != c->consumer_dev) ||
+ (name && strcmp(name, c->consumer_dev_name) != 0))
+ continue;
+ else
+ nummaps++;
+
+ if (nummaps == 0) {
+ ret = -ENODEV;
+ goto error_ret;
+ }
+
+ chans = kzalloc(sizeof(*chans)*(nummaps + 1), GFP_KERNEL);
+ if (chans == NULL) {
+ ret = -ENOMEM;
+ goto error_ret;
+ }
+ for (i = 0; i < nummaps; i++) {
+ chans[i] = kzalloc(sizeof(*chans[0]), GFP_KERNEL);
+ if (chans[i] == NULL) {
+ ret = -ENOMEM;
+ goto error_free_chans;
+ }
+ }
+
+ /* for each map fill in the chans element */
+ list_for_each_entry(c, &iio_map_list, l) {
+ dev_i = NULL;
+ if (dev && dev != c->consumer_dev)
+ continue;
+ if (name && strcmp(name, c->consumer_dev_name) != 0)
+ continue;
+ while (1) {
+ if (c->adc_dev) {
+ dev_i = bus_find_device(&iio_bus_type,
+ dev_i,
+ c->adc_dev,
+ &iio_match_dev);
+ } else if (c->adc_dev_name) {
+ dev_i = bus_find_device(&iio_bus_type,
+ dev_i,
+ (void *)c->adc_dev_name,
+ &iio_match_dev_name);
+ } else {
+ ret = -EINVAL;
+ goto error_free_chans;
+ }
+ if (IS_ERR(dev_i)) {
+ ret = PTR_ERR(dev_i);
+ goto error_free_chans;
+ }
+ if (dev_i == NULL)
+ break;
+
+ indio_dev = container_of(dev_i, struct iio_dev, dev);
+ chans[mapind]->indio_dev = indio_dev;
+ chans[mapind]->channel =
+ iio_chan_spec_from_name(indio_dev,
+ c->adc_channel_label);
+ if (chans[mapind]->channel == NULL) {
+ ret = -EINVAL;
+ put_device(&indio_dev->dev);
+ goto error_free_chans;
+ }
+ mapind++;
+ }
+ }
+ return chans;
+
+error_free_chans:
+ for (i = 0; i < nummaps; i++)
+ if (chans[i]) {
+ put_device(&chans[i]->indio_dev->dev);
+ kfree(chans[i]);
+ }
+error_ret:
+
+ return ERR_PTR(ret);
+}
+EXPORT_SYMBOL_GPL(iio_channel_get_all);
+
+void iio_channel_release(struct iio_channel *channel)
+{
+ put_device(&channel->indio_dev->dev);
+ kfree(channel);
+}
+EXPORT_SYMBOL_GPL(iio_channel_release);
+
+void iio_channel_release_all(struct iio_channel **channels)
+{
+ int i = 0;
+ struct iio_channel *chan = channels[i];
+
+ while (chan) {
+ put_device(&chan->indio_dev->dev);
+ kfree(chan);
+ i++;
+ chan = channels[i];
+ }
+ kfree(channels);
+}
+EXPORT_SYMBOL_GPL(iio_channel_release_all);
+
+int iio_read_channel_raw(struct iio_channel *chan, int *val)
+{
+ int val2;
+ return chan->indio_dev->info->read_raw(chan->indio_dev, chan->channel,
+ val, &val2, 0);
+}
+EXPORT_SYMBOL_GPL(iio_read_channel_raw);
+
+int iio_read_channel_scale(struct iio_channel *chan, int *val, int *val2)
+{
+ /* Does this channel have shared scale? */
+ if (chan->channel->info_mask & (1 << IIO_CHAN_INFO_SCALE_SHARED))
+ return chan->indio_dev
+ ->info->read_raw(chan->indio_dev,
+ chan->channel,
+ val, val2,
+ (1 << IIO_CHAN_INFO_SCALE_SHARED));
+ else if (chan->channel->info_mask & (1 << IIO_CHAN_INFO_SCALE_SEPARATE))
+ return chan->indio_dev
+ ->info->read_raw(chan->indio_dev,
+ chan->channel,
+ val, val2,
+ (1 << IIO_CHAN_INFO_SCALE_SEPARATE));
+ else
+ return -EINVAL;
+}
+EXPORT_SYMBOL_GPL(iio_read_channel_scale);
+
+enum iio_chan_type iio_get_channel_type(struct iio_channel *channel)
+{
+ return channel->channel->type;
+}
+EXPORT_SYMBOL_GPL(iio_get_channel_type);
+
static void iio_device_free_read_attr(struct iio_dev *indio_dev,
struct iio_dev_attr *p)
{
@@ -93,11 +367,6 @@ static void iio_dev_release(struct device *device)
iio_device_unregister_sysfs(indio_dev);
}
-static struct device_type iio_dev_type = {
- .name = "iio_device",
- .release = iio_dev_release,
-};
-
struct iio_dev *iio_device_allocate(int sizeof_priv)
{
struct iio_dev *dev;
diff --git a/drivers/iio/inkern.c b/drivers/iio/inkern.c
new file mode 100644
index 0000000..e2ba5d6
--- /dev/null
+++ b/drivers/iio/inkern.c
@@ -0,0 +1,21 @@
+/* The industrial I/O core in kernel channel mapping
+ *
+ * Copyright (c) 2011 Jonathan Cameron
+ *
+ * This program is free software; you can redistribute it and/or modify it
+ * under the terms of the GNU General Public License version 2 as published by
+ * the Free Software Foundation.
+ */
+#include <linux/iio/inkern.h>
+#include <linux/err.h>
+
+LIST_HEAD(iio_map_list);
+EXPORT_SYMBOL_GPL(iio_map_list);
+void iio_map_array_register(struct iio_map *map, int nummaps)
+{
+ int i;
+ for (i = 0; i < nummaps; i++)
+ list_add(&map[i].l, &iio_map_list);
+}
+EXPORT_SYMBOL(iio_map_array_register);
+
diff --git a/include/linux/iio/iio.h b/include/linux/iio/iio.h
index 8b98e92..472ade9 100644
--- a/include/linux/iio/iio.h
+++ b/include/linux/iio/iio.h
@@ -122,7 +122,10 @@ struct iio_dev;
* @write_raw_get_fmt: callback function to query the expected
* format/precision. If not set by the driver, write_raw
* returns IIO_VAL_INT_PLUS_MICRO.
- **/
+ * @get_hardware_id: obtain device relating to hardware. Typically based on
+ * the parent device (actual hardware). Note that if
+ * not specified then iio_dev.dev->parent is used.
+ */
struct iio_info {
struct module *driver_module;
const struct attribute_group *attrs;
@@ -142,6 +145,7 @@ struct iio_info {
int (*write_raw_get_fmt)(struct iio_dev *indio_dev,
struct iio_chan_spec const *chan,
long mask);
+ struct device *(*get_hardware_id)(struct iio_dev *indio_dev);
};
/**
@@ -176,6 +180,7 @@ struct iio_dev {
#define IIO_MAX_GROUPS 1
const struct attribute_group *groups[IIO_MAX_GROUPS + 1];
int groupcounter;
+ struct list_head dev_list_entry;
};
/**
diff --git a/include/linux/iio/inkern.h b/include/linux/iio/inkern.h
new file mode 100644
index 0000000..6eef0a2
--- /dev/null
+++ b/include/linux/iio/inkern.h
@@ -0,0 +1,87 @@
+#include <linux/device.h>
+#include <linux/list.h>
+#include <linux/iio/chan_spec.h>
+
+#ifndef _IIO_INKERN_H_
+#define _IIO_INKERN_H_
+
+struct iio_dev;
+struct iio_chan_spec;
+
+struct iio_channel {
+ struct iio_dev *indio_dev;
+ const struct iio_chan_spec *channel;
+};
+
+extern struct list_head iio_map_list;
+
+struct iio_map {
+ /* iio device side */
+ struct device *adc_dev;
+ const char *adc_dev_name;
+ const char *adc_channel_label;
+ int channel_number; /*naughty starting point */
+
+ /* consumer side */
+ struct device *consumer_dev;
+ const char *consumer_dev_name;
+ const char *consumer_channel;
+
+ /* management - probably neater ways of doing this */
+ struct list_head l;
+};
+
+void iio_map_array_register(struct iio_map *map, int nummaps);
+/**
+ * iio_channel_get() - get an opaque reference to a specified device.
+ */
+struct iio_channel *iio_channel_get(const struct device *dev,
+ const char *name,
+ const char *consumer_channel);
+void iio_channel_release(struct iio_channel *chan);
+
+/**
+ * iio_channel_get_all() - get all channels associated with a client
+ *
+ * returns a null terminated array of pointers to iio_channel structures.
+ */
+struct iio_channel **iio_channel_get_all(const struct device *dev,
+ const char *name);
+
+void iio_channel_release_all(struct iio_channel **chan);
+
+/**
+ * iio_read_channel_raw() - read from a given channel
+ * @channel: the channel being queried.
+ * @val: value read back.
+ *
+ * Note raw reads from iio channels are in adc counts and hence
+ * scale will need to be applied if standard units required.
+ *
+ * Maybe want to pass the type as a sanity check.
+ */
+int iio_read_channel_raw(struct iio_channel *chan,
+ int *val);
+
+/**
+ * iio_get_channel_type() - get the type of a channel
+ * @channel: the channel being queried.
+ *
+ * returns the enum iio_chan_type of the channel
+ */
+enum iio_chan_type iio_get_channel_type(struct iio_channel *channel);
+
+/**
+ * iio_read_channel_scale() - read the scale value for a channel
+ * @channel: the channel being queried.
+ * @val: first part of value read back.
+ * @val2: second part of value read back.
+ *
+ * Note returns a description of what is in val and val2, such
+ * as IIO_VAL_INT_PLUS_MICRO telling us we have a value of val
+ * + val2/1e6
+ */
+int iio_read_channel_scale(struct iio_channel *chan, int *val,
+ int *val2);
+
+#endif
--
1.7.7
^ permalink raw reply related [flat|nested] 18+ messages in thread* [PATCH 5/6] IIO:hwmon interface client driver.
2011-10-20 9:33 [RFC V3 PATCH 0/6] IIO in kernel interfaces Jonathan Cameron
` (3 preceding siblings ...)
2011-10-20 9:33 ` [PATCH 4/6] IIO:CORE add in kernel interface mapping and getting IIO channels Jonathan Cameron
@ 2011-10-20 9:33 ` Jonathan Cameron
2011-10-20 15:12 ` Guenter Roeck
2011-10-20 9:33 ` [PATCH 6/6] stargate2: example of map configuration for iio to hwmon example Jonathan Cameron
5 siblings, 1 reply; 18+ messages in thread
From: Jonathan Cameron @ 2011-10-20 9:33 UTC (permalink / raw)
To: linux-kernel, linux-iio
Cc: linus.ml.walleij, zdevai, linux, arnd, broonie, gregkh,
lm-sensors, guenter.roeck, khali, thomas.petazzoni, maxime.ripard,
Jonathan Cameron
Should move to drivers/hwmon once people are happy with it.
Minimal support of simple in, curr and temp attributes
so far.
Signed-off-by: Jonathan Cameron <jic23@cam.ac.uk>
---
drivers/iio/Kconfig | 8 ++
drivers/iio/Makefile | 1 +
drivers/iio/iio_hwmon.c | 227 +++++++++++++++++++++++++++++++++++++++++++++++
3 files changed, 236 insertions(+), 0 deletions(-)
diff --git a/drivers/iio/Kconfig b/drivers/iio/Kconfig
index 308bc97..c2f0970 100644
--- a/drivers/iio/Kconfig
+++ b/drivers/iio/Kconfig
@@ -11,6 +11,14 @@ menuconfig IIO
if IIO
+config IIO_HWMON
+ tristate "Hwmon driver that uses channels specified via iio maps"
+ depends on HWMON
+ help
+ This is a platform driver that in combination with a suitable
+ map allows IIO devices to provide basic hwmon functionality
+ for those channels specified in the map.
+
source "drivers/iio/adc/Kconfig"
source "drivers/iio/imu/Kconfig"
source "drivers/iio/light/Kconfig"
diff --git a/drivers/iio/Makefile b/drivers/iio/Makefile
index cfb588a..5f9c01a 100644
--- a/drivers/iio/Makefile
+++ b/drivers/iio/Makefile
@@ -6,6 +6,7 @@ obj-y = inkern.o
obj-$(CONFIG_IIO) += iio.o
industrialio-y := core.o
+obj-$(CONFIG_IIO_HWMON) += iio_hwmon.o
obj-y += adc/
obj-y += imu/
obj-y += light/
diff --git a/drivers/iio/iio_hwmon.c b/drivers/iio/iio_hwmon.c
new file mode 100644
index 0000000..b3348ad
--- /dev/null
+++ b/drivers/iio/iio_hwmon.c
@@ -0,0 +1,227 @@
+/* Hwmon client for industrial I/O devices
+ *
+ * Copyright (c) 2011 Jonathan Cameron
+ *
+ * This program is free software; you can redistribute it and/or modify it
+ * under the terms of the GNU General Public License version 2 as published by
+ * the Free Software Foundation.
+ *
+ * Limited functionality currently supported.
+ */
+
+#include <linux/kernel.h>
+#include <linux/slab.h>
+#include <linux/module.h>
+#include <linux/err.h>
+#include <linux/platform_device.h>
+#include <linux/iio/inkern.h>
+#include <linux/hwmon.h>
+#include <linux/hwmon-sysfs.h>
+
+/**
+ * struct iio_hwmon_state - device instance state
+ * @channels: filled with null terminated array of channels from iio
+ * @num_channels: number of channels in channels (saves counting twice)
+ * @hwmon_dev: associated hwmon device
+ * @attr_group: the group of attributes
+ * @attrs: null terminated array of attribute pointers.
+ */
+struct iio_hwmon_state {
+ struct iio_channel **channels;
+ int num_channels;
+ struct device *hwmon_dev;
+ struct attribute_group attr_group;
+ struct attribute **attrs;
+};
+
+/*
+ * Assumes that IIO and hwmon operate in the same base units.
+ * This is supposed to be true, but needs verification for
+ * new channel types.
+ */
+static ssize_t iio_hwmon_read_val(struct device *dev,
+ struct device_attribute *attr,
+ char *buf)
+{
+ long result;
+ int val, ret, scaleint, scalepart;
+ struct sensor_device_attribute *sattr = to_sensor_dev_attr(attr);
+ struct iio_hwmon_state *state = dev_get_drvdata(dev);
+
+ /*
+ * No locking between this pair, so theoretically possible
+ * the scale has changed.
+ */
+ ret = iio_read_channel_raw(state->channels[sattr->index],
+ &val);
+ if (ret < 0)
+ return ret;
+
+ ret = iio_read_channel_scale(state->channels[sattr->index],
+ &scaleint, &scalepart);
+ if (ret < 0)
+ return ret;
+ switch (ret) {
+ case IIO_VAL_INT:
+ result = val * scaleint;
+ break;
+ case IIO_VAL_INT_PLUS_MICRO:
+ result = (long)val * (long)scaleint +
+ (long)val * (long)scalepart / 1000000L;
+ break;
+ case IIO_VAL_INT_PLUS_NANO:
+ result = (long)val * (long)scaleint +
+ (long)val * (long)scalepart / 1000000000L;
+ break;
+ default:
+ return -EINVAL;
+ }
+ return sprintf(buf, "%ld\n", result);
+}
+
+static void iio_hwmon_free_attrs(struct iio_hwmon_state *st)
+{
+ int i;
+ struct sensor_device_attribute *a;
+ for (i = 0; i < st->num_channels; i++)
+ if (st->attrs[i]) {
+ a = to_sensor_dev_attr(
+ container_of(st->attrs[i],
+ struct device_attribute,
+ attr));
+ kfree(a);
+ }
+}
+
+static int __devinit iio_hwmon_probe(struct platform_device *pdev)
+{
+ struct iio_hwmon_state *st;
+ struct sensor_device_attribute *a;
+ int ret, i;
+ int in_i = 1, temp_i = 1, curr_i = 1;
+
+ st = kzalloc(sizeof(*st), GFP_KERNEL);
+ if (st == NULL) {
+ ret = -ENOMEM;
+ goto error_ret;
+ }
+
+ st->channels = iio_channel_get_all(&pdev->dev, NULL);
+ if (IS_ERR(st->channels)) {
+ ret = PTR_ERR(st->channels);
+ goto error_free_state;
+ }
+
+ /* count how many attributes we have */
+ while (st->channels[st->num_channels])
+ st->num_channels++;
+
+ st->attrs = kzalloc(sizeof(st->attrs) * (st->num_channels + 1),
+ GFP_KERNEL);
+ if (st->attrs == NULL) {
+ ret = -ENOMEM;
+ goto error_release_channels;
+ }
+ for (i = 0; i < st->num_channels; i++) {
+ a = kzalloc(sizeof(*a), GFP_KERNEL);
+ if (a == NULL) {
+ ret = -ENOMEM;
+ goto error_free_attrs;
+ }
+
+ sysfs_attr_init(&a->dev_attr.attr);
+ switch (iio_get_channel_type(st->channels[i])) {
+ case IIO_VOLTAGE:
+ a->dev_attr.attr.name = kasprintf(GFP_KERNEL,
+ "in%d_input",
+ in_i++);
+ break;
+ case IIO_TEMP:
+ a->dev_attr.attr.name = kasprintf(GFP_KERNEL,
+ "temp%d_input",
+ temp_i++);
+ break;
+ case IIO_CURRENT:
+ a->dev_attr.attr.name = kasprintf(GFP_KERNEL,
+ "curr%d_input",
+ curr_i++);
+ break;
+ default:
+ ret = -EINVAL;
+ kfree(a);
+ goto error_free_attrs;
+ }
+ if (a->dev_attr.attr.name == NULL) {
+ kfree(a);
+ ret = -ENOMEM;
+ goto error_free_attrs;
+ }
+ a->dev_attr.show = iio_hwmon_read_val;
+ a->dev_attr.attr.mode = S_IRUGO;
+ a->index = i;
+ st->attrs[i] = &a->dev_attr.attr;
+ }
+
+ st->attr_group.attrs = st->attrs;
+ platform_set_drvdata(pdev, st);
+ ret = sysfs_create_group(&pdev->dev.kobj, &st->attr_group);
+ if (ret < 0)
+ goto error_free_attrs;
+
+ st->hwmon_dev = hwmon_device_register(&pdev->dev);
+ if (IS_ERR(st->hwmon_dev)) {
+ ret = PTR_ERR(st->hwmon_dev);
+ goto error_remove_group;
+ }
+ return 0;
+
+error_remove_group:
+ sysfs_remove_group(&pdev->dev.kobj, &st->attr_group);
+error_free_attrs:
+ iio_hwmon_free_attrs(st);
+ kfree(st->attrs);
+error_release_channels:
+ iio_channel_release_all(st->channels);
+error_free_state:
+ kfree(st);
+error_ret:
+ return ret;
+}
+
+static int __devexit iio_hwmon_remove(struct platform_device *pdev)
+{
+ struct iio_hwmon_state *st = platform_get_drvdata(pdev);
+
+ hwmon_device_unregister(st->hwmon_dev);
+ sysfs_remove_group(&pdev->dev.kobj, &st->attr_group);
+ iio_hwmon_free_attrs(st);
+ kfree(st->attrs);
+ iio_channel_release_all(st->channels);
+
+ return 0;
+}
+
+static struct platform_driver __refdata iio_hwmon_driver = {
+ .driver = {
+ .name = "iio_hwmon",
+ .owner = THIS_MODULE,
+ },
+ .probe = iio_hwmon_probe,
+ .remove = __devexit_p(iio_hwmon_remove),
+};
+
+static int iio_inkern_init(void)
+{
+ return platform_driver_register(&iio_hwmon_driver);
+}
+module_init(iio_inkern_init);
+
+static void iio_inkern_exit(void)
+{
+ platform_driver_unregister(&iio_hwmon_driver);
+}
+module_exit(iio_inkern_exit);
+
+MODULE_AUTHOR("Jonathan Cameron <jic23@cam.ac.uk>");
+MODULE_DESCRIPTION("IIO to hwmon driver");
+MODULE_LICENSE("GPL v2");
--
1.7.7
^ permalink raw reply related [flat|nested] 18+ messages in thread* Re: [PATCH 5/6] IIO:hwmon interface client driver.
2011-10-20 9:33 ` [PATCH 5/6] IIO:hwmon interface client driver Jonathan Cameron
@ 2011-10-20 15:12 ` Guenter Roeck
2011-10-20 15:30 ` Jonathan Cameron
0 siblings, 1 reply; 18+ messages in thread
From: Guenter Roeck @ 2011-10-20 15:12 UTC (permalink / raw)
To: Jonathan Cameron
Cc: linux-kernel@vger.kernel.org, linux-iio@vger.kernel.org,
linus.ml.walleij@gmail.com, zdevai@gmail.com,
linux@arm.linux.org.uk, arnd@arndb.de,
broonie@opensource.wolfsonmicro.com, gregkh@suse.de,
lm-sensors@lm-sensors.org, khali@linux-fr.org,
thomas.petazzoni@free-electrons.com,
maxime.ripard@free-electrons.com
On Thu, 2011-10-20 at 05:33 -0400, Jonathan Cameron wrote:
> Should move to drivers/hwmon once people are happy with it.
>
> Minimal support of simple in, curr and temp attributes
> so far.
>
> Signed-off-by: Jonathan Cameron <jic23@cam.ac.uk>
> ---
> drivers/iio/Kconfig | 8 ++
> drivers/iio/Makefile | 1 +
> drivers/iio/iio_hwmon.c | 227 +++++++++++++++++++++++++++++++++++++++++++++++
> 3 files changed, 236 insertions(+), 0 deletions(-)
>
> diff --git a/drivers/iio/Kconfig b/drivers/iio/Kconfig
> index 308bc97..c2f0970 100644
> --- a/drivers/iio/Kconfig
> +++ b/drivers/iio/Kconfig
> @@ -11,6 +11,14 @@ menuconfig IIO
>
> if IIO
>
> +config IIO_HWMON
> + tristate "Hwmon driver that uses channels specified via iio maps"
> + depends on HWMON
> + help
> + This is a platform driver that in combination with a suitable
> + map allows IIO devices to provide basic hwmon functionality
> + for those channels specified in the map.
> +
> source "drivers/iio/adc/Kconfig"
> source "drivers/iio/imu/Kconfig"
> source "drivers/iio/light/Kconfig"
> diff --git a/drivers/iio/Makefile b/drivers/iio/Makefile
> index cfb588a..5f9c01a 100644
> --- a/drivers/iio/Makefile
> +++ b/drivers/iio/Makefile
> @@ -6,6 +6,7 @@ obj-y = inkern.o
> obj-$(CONFIG_IIO) += iio.o
> industrialio-y := core.o
>
> +obj-$(CONFIG_IIO_HWMON) += iio_hwmon.o
> obj-y += adc/
> obj-y += imu/
> obj-y += light/
> diff --git a/drivers/iio/iio_hwmon.c b/drivers/iio/iio_hwmon.c
> new file mode 100644
> index 0000000..b3348ad
> --- /dev/null
> +++ b/drivers/iio/iio_hwmon.c
> @@ -0,0 +1,227 @@
> +/* Hwmon client for industrial I/O devices
> + *
> + * Copyright (c) 2011 Jonathan Cameron
> + *
> + * This program is free software; you can redistribute it and/or modify it
> + * under the terms of the GNU General Public License version 2 as published by
> + * the Free Software Foundation.
> + *
> + * Limited functionality currently supported.
Just nitpicking ... this comment doesn't provide much value. It doesn't
explain the limits, nor what could be improved.
> + */
> +
> +#include <linux/kernel.h>
> +#include <linux/slab.h>
> +#include <linux/module.h>
> +#include <linux/err.h>
> +#include <linux/platform_device.h>
> +#include <linux/iio/inkern.h>
> +#include <linux/hwmon.h>
> +#include <linux/hwmon-sysfs.h>
> +
> +/**
> + * struct iio_hwmon_state - device instance state
> + * @channels: filled with null terminated array of channels from iio
> + * @num_channels: number of channels in channels (saves counting twice)
> + * @hwmon_dev: associated hwmon device
> + * @attr_group: the group of attributes
> + * @attrs: null terminated array of attribute pointers.
> + */
> +struct iio_hwmon_state {
> + struct iio_channel **channels;
> + int num_channels;
> + struct device *hwmon_dev;
> + struct attribute_group attr_group;
> + struct attribute **attrs;
> +};
> +
> +/*
> + * Assumes that IIO and hwmon operate in the same base units.
> + * This is supposed to be true, but needs verification for
> + * new channel types.
> + */
> +static ssize_t iio_hwmon_read_val(struct device *dev,
> + struct device_attribute *attr,
> + char *buf)
> +{
> + long result;
> + int val, ret, scaleint, scalepart;
> + struct sensor_device_attribute *sattr = to_sensor_dev_attr(attr);
> + struct iio_hwmon_state *state = dev_get_drvdata(dev);
> +
> + /*
> + * No locking between this pair, so theoretically possible
> + * the scale has changed.
> + */
> + ret = iio_read_channel_raw(state->channels[sattr->index],
> + &val);
> + if (ret < 0)
> + return ret;
> +
> + ret = iio_read_channel_scale(state->channels[sattr->index],
> + &scaleint, &scalepart);
> + if (ret < 0)
> + return ret;
> + switch (ret) {
> + case IIO_VAL_INT:
> + result = val * scaleint;
> + break;
> + case IIO_VAL_INT_PLUS_MICRO:
> + result = (long)val * (long)scaleint +
> + (long)val * (long)scalepart / 1000000L;
> + break;
> + case IIO_VAL_INT_PLUS_NANO:
> + result = (long)val * (long)scaleint +
> + (long)val * (long)scalepart / 1000000000L;
> + break;
Still easy to imagine that val * scalepart gets larger than 2147483647L
(on machines where sizeof(long) = 4) ... it will already happen if the
result of (val * scalepart / 1000000000) is larger than 2.
What value range do you expect to see here ?
If (val * scaleint) is already the milli-unit, scalepart would possibly
only address fractions of milli-units. If so, the result of (val *
scalepart / 1000000000L) might always be smaller than 1, ie 0. If so,
for the calculation to have any value, you might be better off using
DIV_ROUND_CLOSEST(val * scalepart, 1000000000L).
I am a bit confused by this anyway. Since hwmon in general reports
milli-units, VAL_INT appears to reflect milli-units, VAL_INT_PLUS_MICRO
really means nano-units, and IIO_VAL_INT_PLUS_NANO really means
pico-units. Is this correct ?
> + default:
> + return -EINVAL;
> + }
> + return sprintf(buf, "%ld\n", result);
> +}
> +
> +static void iio_hwmon_free_attrs(struct iio_hwmon_state *st)
> +{
> + int i;
> + struct sensor_device_attribute *a;
> + for (i = 0; i < st->num_channels; i++)
> + if (st->attrs[i]) {
> + a = to_sensor_dev_attr(
> + container_of(st->attrs[i],
> + struct device_attribute,
> + attr));
> + kfree(a);
> + }
> +}
> +
> +static int __devinit iio_hwmon_probe(struct platform_device *pdev)
> +{
> + struct iio_hwmon_state *st;
> + struct sensor_device_attribute *a;
> + int ret, i;
> + int in_i = 1, temp_i = 1, curr_i = 1;
> +
> + st = kzalloc(sizeof(*st), GFP_KERNEL);
> + if (st == NULL) {
> + ret = -ENOMEM;
> + goto error_ret;
> + }
> +
> + st->channels = iio_channel_get_all(&pdev->dev, NULL);
> + if (IS_ERR(st->channels)) {
> + ret = PTR_ERR(st->channels);
> + goto error_free_state;
> + }
> +
> + /* count how many attributes we have */
> + while (st->channels[st->num_channels])
> + st->num_channels++;
> +
> + st->attrs = kzalloc(sizeof(st->attrs) * (st->num_channels + 1),
> + GFP_KERNEL);
Why "+ 1" ?
Unless I am missing something, you only use st->attrs[0] ..
st->attrs[st->num_channels-1].
Thanks,
Guenter
^ permalink raw reply [flat|nested] 18+ messages in thread* Re: [PATCH 5/6] IIO:hwmon interface client driver.
2011-10-20 15:12 ` Guenter Roeck
@ 2011-10-20 15:30 ` Jonathan Cameron
2011-10-24 10:09 ` Jonathan Cameron
0 siblings, 1 reply; 18+ messages in thread
From: Jonathan Cameron @ 2011-10-20 15:30 UTC (permalink / raw)
To: guenter.roeck
Cc: linux-kernel@vger.kernel.org, linux-iio@vger.kernel.org,
linus.ml.walleij@gmail.com, zdevai@gmail.com,
linux@arm.linux.org.uk, arnd@arndb.de,
broonie@opensource.wolfsonmicro.com, gregkh@suse.de,
lm-sensors@lm-sensors.org, khali@linux-fr.org,
thomas.petazzoni@free-electrons.com,
maxime.ripard@free-electrons.com
On 10/20/11 16:12, Guenter Roeck wrote:
> On Thu, 2011-10-20 at 05:33 -0400, Jonathan Cameron wrote:
>> Should move to drivers/hwmon once people are happy with it.
>>
>> Minimal support of simple in, curr and temp attributes
>> so far.
>>
>> Signed-off-by: Jonathan Cameron <jic23@cam.ac.uk>
>> ---
>> drivers/iio/Kconfig | 8 ++
>> drivers/iio/Makefile | 1 +
>> drivers/iio/iio_hwmon.c | 227 +++++++++++++++++++++++++++++++++++++++++++++++
>> 3 files changed, 236 insertions(+), 0 deletions(-)
>>
>> diff --git a/drivers/iio/Kconfig b/drivers/iio/Kconfig
>> index 308bc97..c2f0970 100644
>> --- a/drivers/iio/Kconfig
>> +++ b/drivers/iio/Kconfig
>> @@ -11,6 +11,14 @@ menuconfig IIO
>>
>> if IIO
>>
>> +config IIO_HWMON
>> + tristate "Hwmon driver that uses channels specified via iio maps"
>> + depends on HWMON
>> + help
>> + This is a platform driver that in combination with a suitable
>> + map allows IIO devices to provide basic hwmon functionality
>> + for those channels specified in the map.
>> +
>> source "drivers/iio/adc/Kconfig"
>> source "drivers/iio/imu/Kconfig"
>> source "drivers/iio/light/Kconfig"
>> diff --git a/drivers/iio/Makefile b/drivers/iio/Makefile
>> index cfb588a..5f9c01a 100644
>> --- a/drivers/iio/Makefile
>> +++ b/drivers/iio/Makefile
>> @@ -6,6 +6,7 @@ obj-y = inkern.o
>> obj-$(CONFIG_IIO) += iio.o
>> industrialio-y := core.o
>>
>> +obj-$(CONFIG_IIO_HWMON) += iio_hwmon.o
>> obj-y += adc/
>> obj-y += imu/
>> obj-y += light/
>> diff --git a/drivers/iio/iio_hwmon.c b/drivers/iio/iio_hwmon.c
>> new file mode 100644
>> index 0000000..b3348ad
>> --- /dev/null
>> +++ b/drivers/iio/iio_hwmon.c
>> @@ -0,0 +1,227 @@
>> +/* Hwmon client for industrial I/O devices
>> + *
>> + * Copyright (c) 2011 Jonathan Cameron
>> + *
>> + * This program is free software; you can redistribute it and/or modify it
>> + * under the terms of the GNU General Public License version 2 as published by
>> + * the Free Software Foundation.
>> + *
>> + * Limited functionality currently supported.
>
> Just nitpicking ... this comment doesn't provide much value. It doesn't
> explain the limits, nor what could be improved.
>
>> + */
>> +
>> +#include <linux/kernel.h>
>> +#include <linux/slab.h>
>> +#include <linux/module.h>
>> +#include <linux/err.h>
>> +#include <linux/platform_device.h>
>> +#include <linux/iio/inkern.h>
>> +#include <linux/hwmon.h>
>> +#include <linux/hwmon-sysfs.h>
>> +
>> +/**
>> + * struct iio_hwmon_state - device instance state
>> + * @channels: filled with null terminated array of channels from iio
>> + * @num_channels: number of channels in channels (saves counting twice)
>> + * @hwmon_dev: associated hwmon device
>> + * @attr_group: the group of attributes
>> + * @attrs: null terminated array of attribute pointers.
>> + */
>> +struct iio_hwmon_state {
>> + struct iio_channel **channels;
>> + int num_channels;
>> + struct device *hwmon_dev;
>> + struct attribute_group attr_group;
>> + struct attribute **attrs;
>> +};
>> +
>> +/*
>> + * Assumes that IIO and hwmon operate in the same base units.
>> + * This is supposed to be true, but needs verification for
>> + * new channel types.
>> + */
>> +static ssize_t iio_hwmon_read_val(struct device *dev,
>> + struct device_attribute *attr,
>> + char *buf)
>> +{
>> + long result;
>> + int val, ret, scaleint, scalepart;
>> + struct sensor_device_attribute *sattr = to_sensor_dev_attr(attr);
>> + struct iio_hwmon_state *state = dev_get_drvdata(dev);
>> +
>> + /*
>> + * No locking between this pair, so theoretically possible
>> + * the scale has changed.
>> + */
>> + ret = iio_read_channel_raw(state->channels[sattr->index],
>> + &val);
>> + if (ret < 0)
>> + return ret;
>> +
>> + ret = iio_read_channel_scale(state->channels[sattr->index],
>> + &scaleint, &scalepart);
>> + if (ret < 0)
>> + return ret;
>> + switch (ret) {
>> + case IIO_VAL_INT:
>> + result = val * scaleint;
>> + break;
>> + case IIO_VAL_INT_PLUS_MICRO:
>> + result = (long)val * (long)scaleint +
>> + (long)val * (long)scalepart / 1000000L;
>> + break;
>> + case IIO_VAL_INT_PLUS_NANO:
>> + result = (long)val * (long)scaleint +
>> + (long)val * (long)scalepart / 1000000000L;
>> + break;
>
> Still easy to imagine that val * scalepart gets larger than 2147483647L
> (on machines where sizeof(long) = 4) ... it will already happen if the
> result of (val * scalepart / 1000000000) is larger than 2.
Good point. I really ought to have done the calcs.
If we have maximum possible value in here things will be ugly.
Worst case is scalepart is 9999999999. (could be done as 1 - 0.000000001
which would be nicer, but we don't specify a preference - from this
discussion I am suspecting we should!)
Looks like 64 bits is going to be a requirement as you say.
>
> What value range do you expect to see here ?
>
> If (val * scaleint) is already the milli-unit, scalepart would possibly
> only address fractions of milli-units. If so, the result of (val *
> scalepart / 1000000000L) might always be smaller than 1, ie 0.
It certainly should be.
> If so, for the calculation to have any value, you might be better off using
> DIV_ROUND_CLOSEST(val * scalepart, 1000000000L).
Good idea.
>
> I am a bit confused by this anyway. Since hwmon in general reports
> milli-units, VAL_INT appears to reflect milli-units, VAL_INT_PLUS_MICRO
> really means nano-units, and IIO_VAL_INT_PLUS_NANO really means
> pico-units. Is this correct ?
Micro units of the scale factor.
Take my test part a max1363...
Scale is actually 0.5 so each adc count (e.g. raw value) is 0.5millivolts.
scale int here is 0,
scale part is 500,000 (so 0.5) and it returns IIO_VAL_INT_PLUS_MICRO.
>
>> + default:
>> + return -EINVAL;
>> + }
>> + return sprintf(buf, "%ld\n", result);
>> +}
>> +
>> +static void iio_hwmon_free_attrs(struct iio_hwmon_state *st)
>> +{
>> + int i;
>> + struct sensor_device_attribute *a;
>> + for (i = 0; i < st->num_channels; i++)
>> + if (st->attrs[i]) {
>> + a = to_sensor_dev_attr(
>> + container_of(st->attrs[i],
>> + struct device_attribute,
>> + attr));
>> + kfree(a);
>> + }
>> +}
>> +
>> +static int __devinit iio_hwmon_probe(struct platform_device *pdev)
>> +{
>> + struct iio_hwmon_state *st;
>> + struct sensor_device_attribute *a;
>> + int ret, i;
>> + int in_i = 1, temp_i = 1, curr_i = 1;
>> +
>> + st = kzalloc(sizeof(*st), GFP_KERNEL);
>> + if (st == NULL) {
>> + ret = -ENOMEM;
>> + goto error_ret;
>> + }
>> +
>> + st->channels = iio_channel_get_all(&pdev->dev, NULL);
>> + if (IS_ERR(st->channels)) {
>> + ret = PTR_ERR(st->channels);
>> + goto error_free_state;
>> + }
>> +
>> + /* count how many attributes we have */
>> + while (st->channels[st->num_channels])
>> + st->num_channels++;
>> +
>> + st->attrs = kzalloc(sizeof(st->attrs) * (st->num_channels + 1),
>> + GFP_KERNEL);
>
> Why "+ 1" ?
Null terminated list for attribute groups. Hence the kzalloc.
>
> Unless I am missing something, you only use st->attrs[0] ..
> st->attrs[st->num_channels-1].
>
> Thanks,
> Guenter
>
>
>
^ permalink raw reply [flat|nested] 18+ messages in thread* Re: [PATCH 5/6] IIO:hwmon interface client driver.
2011-10-20 15:30 ` Jonathan Cameron
@ 2011-10-24 10:09 ` Jonathan Cameron
2011-10-24 15:39 ` Guenter Roeck
0 siblings, 1 reply; 18+ messages in thread
From: Jonathan Cameron @ 2011-10-24 10:09 UTC (permalink / raw)
To: Jonathan Cameron
Cc: guenter.roeck, linux-kernel@vger.kernel.org,
linux-iio@vger.kernel.org, linus.ml.walleij@gmail.com,
zdevai@gmail.com, linux@arm.linux.org.uk, arnd@arndb.de,
broonie@opensource.wolfsonmicro.com, gregkh@suse.de,
lm-sensors@lm-sensors.org, khali@linux-fr.org,
thomas.petazzoni@free-electrons.com,
maxime.ripard@free-electrons.com
On 10/20/11 16:30, Jonathan Cameron wrote:
> On 10/20/11 16:12, Guenter Roeck wrote:
>> On Thu, 2011-10-20 at 05:33 -0400, Jonathan Cameron wrote:
>>> Should move to drivers/hwmon once people are happy with it.
>>>
>>> Minimal support of simple in, curr and temp attributes
>>> so far.
>>>
>>> Signed-off-by: Jonathan Cameron <jic23@cam.ac.uk>
>>> ---
>>> drivers/iio/Kconfig | 8 ++
>>> drivers/iio/Makefile | 1 +
>>> drivers/iio/iio_hwmon.c | 227 +++++++++++++++++++++++++++++++++++++++++++++++
>>> 3 files changed, 236 insertions(+), 0 deletions(-)
>>>
>>> diff --git a/drivers/iio/Kconfig b/drivers/iio/Kconfig
>>> index 308bc97..c2f0970 100644
>>> --- a/drivers/iio/Kconfig
>>> +++ b/drivers/iio/Kconfig
>>> @@ -11,6 +11,14 @@ menuconfig IIO
>>>
>>> if IIO
>>>
>>> +config IIO_HWMON
>>> + tristate "Hwmon driver that uses channels specified via iio maps"
>>> + depends on HWMON
>>> + help
>>> + This is a platform driver that in combination with a suitable
>>> + map allows IIO devices to provide basic hwmon functionality
>>> + for those channels specified in the map.
>>> +
>>> source "drivers/iio/adc/Kconfig"
>>> source "drivers/iio/imu/Kconfig"
>>> source "drivers/iio/light/Kconfig"
>>> diff --git a/drivers/iio/Makefile b/drivers/iio/Makefile
>>> index cfb588a..5f9c01a 100644
>>> --- a/drivers/iio/Makefile
>>> +++ b/drivers/iio/Makefile
>>> @@ -6,6 +6,7 @@ obj-y = inkern.o
>>> obj-$(CONFIG_IIO) += iio.o
>>> industrialio-y := core.o
>>>
>>> +obj-$(CONFIG_IIO_HWMON) += iio_hwmon.o
>>> obj-y += adc/
>>> obj-y += imu/
>>> obj-y += light/
>>> diff --git a/drivers/iio/iio_hwmon.c b/drivers/iio/iio_hwmon.c
>>> new file mode 100644
>>> index 0000000..b3348ad
>>> --- /dev/null
>>> +++ b/drivers/iio/iio_hwmon.c
>>> @@ -0,0 +1,227 @@
>>> +/* Hwmon client for industrial I/O devices
>>> + *
>>> + * Copyright (c) 2011 Jonathan Cameron
>>> + *
>>> + * This program is free software; you can redistribute it and/or modify it
>>> + * under the terms of the GNU General Public License version 2 as published by
>>> + * the Free Software Foundation.
>>> + *
>>> + * Limited functionality currently supported.
>>
>> Just nitpicking ... this comment doesn't provide much value. It doesn't
>> explain the limits, nor what could be improved.
>>
>>> + */
>>> +
>>> +#include <linux/kernel.h>
>>> +#include <linux/slab.h>
>>> +#include <linux/module.h>
>>> +#include <linux/err.h>
>>> +#include <linux/platform_device.h>
>>> +#include <linux/iio/inkern.h>
>>> +#include <linux/hwmon.h>
>>> +#include <linux/hwmon-sysfs.h>
>>> +
>>> +/**
>>> + * struct iio_hwmon_state - device instance state
>>> + * @channels: filled with null terminated array of channels from iio
>>> + * @num_channels: number of channels in channels (saves counting twice)
>>> + * @hwmon_dev: associated hwmon device
>>> + * @attr_group: the group of attributes
>>> + * @attrs: null terminated array of attribute pointers.
>>> + */
>>> +struct iio_hwmon_state {
>>> + struct iio_channel **channels;
>>> + int num_channels;
>>> + struct device *hwmon_dev;
>>> + struct attribute_group attr_group;
>>> + struct attribute **attrs;
>>> +};
>>> +
>>> +/*
>>> + * Assumes that IIO and hwmon operate in the same base units.
>>> + * This is supposed to be true, but needs verification for
>>> + * new channel types.
>>> + */
>>> +static ssize_t iio_hwmon_read_val(struct device *dev,
>>> + struct device_attribute *attr,
>>> + char *buf)
>>> +{
>>> + long result;
>>> + int val, ret, scaleint, scalepart;
>>> + struct sensor_device_attribute *sattr = to_sensor_dev_attr(attr);
>>> + struct iio_hwmon_state *state = dev_get_drvdata(dev);
>>> +
>>> + /*
>>> + * No locking between this pair, so theoretically possible
>>> + * the scale has changed.
>>> + */
>>> + ret = iio_read_channel_raw(state->channels[sattr->index],
>>> + &val);
>>> + if (ret < 0)
>>> + return ret;
>>> +
>>> + ret = iio_read_channel_scale(state->channels[sattr->index],
>>> + &scaleint, &scalepart);
>>> + if (ret < 0)
>>> + return ret;
>>> + switch (ret) {
>>> + case IIO_VAL_INT:
>>> + result = val * scaleint;
>>> + break;
>>> + case IIO_VAL_INT_PLUS_MICRO:
>>> + result = (long)val * (long)scaleint +
>>> + (long)val * (long)scalepart / 1000000L;
>>> + break;
>>> + case IIO_VAL_INT_PLUS_NANO:
>>> + result = (long)val * (long)scaleint +
>>> + (long)val * (long)scalepart / 1000000000L;
>>> + break;
>>
>> Still easy to imagine that val * scalepart gets larger than 2147483647L
>> (on machines where sizeof(long) = 4) ... it will already happen if the
>> result of (val * scalepart / 1000000000) is larger than 2.
> Good point. I really ought to have done the calcs.
> If we have maximum possible value in here things will be ugly.
>
> Worst case is scalepart is 9999999999. (could be done as 1 - 0.000000001
> which would be nicer, but we don't specify a preference - from this
> discussion I am suspecting we should!)
>
> Looks like 64 bits is going to be a requirement as you say.
>>
>> What value range do you expect to see here ?
>>
>> If (val * scaleint) is already the milli-unit, scalepart would possibly
>> only address fractions of milli-units. If so, the result of (val *
>> scalepart / 1000000000L) might always be smaller than 1, ie 0.
> It certainly should be.
>> If so, for the calculation to have any value, you might be better off using
>> DIV_ROUND_CLOSEST(val * scalepart, 1000000000L).
> Good idea.
>>
>> I am a bit confused by this anyway. Since hwmon in general reports
>> milli-units, VAL_INT appears to reflect milli-units, VAL_INT_PLUS_MICRO
>> really means nano-units, and IIO_VAL_INT_PLUS_NANO really means
>> pico-units. Is this correct ?
> Micro units of the scale factor.
>
> Take my test part a max1363...
> Scale is actually 0.5 so each adc count (e.g. raw value) is 0.5millivolts.
>
> scale int here is 0,
> scale part is 500,000 (so 0.5) and it returns IIO_VAL_INT_PLUS_MICRO.
How about the following? It'll be extremely costly, but this isn't exactly
a fast path!
case IIO_VAL_INT_PLUS_MICRO:
result = (s64)val * (s64)scaleint +
div_s64((s64)val * (s64)scalepart, 1000000LL);
break;
case IIO_VAL_INT_PLUS_NANO:
result = (s64)val * (s64)scaleint +
div_s64((s64)val * (s64)scalepart, 1000000000LL);
break;
Everything should fit in there and it should give us pretty good precision.
>
>
>>
>>> + default:
>>> + return -EINVAL;
>>> + }
>>> + return sprintf(buf, "%ld\n", result);
>>> +}
>>> +
>>> +static void iio_hwmon_free_attrs(struct iio_hwmon_state *st)
>>> +{
>>> + int i;
>>> + struct sensor_device_attribute *a;
>>> + for (i = 0; i < st->num_channels; i++)
>>> + if (st->attrs[i]) {
>>> + a = to_sensor_dev_attr(
>>> + container_of(st->attrs[i],
>>> + struct device_attribute,
>>> + attr));
>>> + kfree(a);
>>> + }
>>> +}
>>> +
>>> +static int __devinit iio_hwmon_probe(struct platform_device *pdev)
>>> +{
>>> + struct iio_hwmon_state *st;
>>> + struct sensor_device_attribute *a;
>>> + int ret, i;
>>> + int in_i = 1, temp_i = 1, curr_i = 1;
>>> +
>>> + st = kzalloc(sizeof(*st), GFP_KERNEL);
>>> + if (st == NULL) {
>>> + ret = -ENOMEM;
>>> + goto error_ret;
>>> + }
>>> +
>>> + st->channels = iio_channel_get_all(&pdev->dev, NULL);
>>> + if (IS_ERR(st->channels)) {
>>> + ret = PTR_ERR(st->channels);
>>> + goto error_free_state;
>>> + }
>>> +
>>> + /* count how many attributes we have */
>>> + while (st->channels[st->num_channels])
>>> + st->num_channels++;
>>> +
>>> + st->attrs = kzalloc(sizeof(st->attrs) * (st->num_channels + 1),
>>> + GFP_KERNEL);
>>
>> Why "+ 1" ?
> Null terminated list for attribute groups. Hence the kzalloc.
>>
>> Unless I am missing something, you only use st->attrs[0] ..
>> st->attrs[st->num_channels-1].
>>
>> Thanks,
>> Guenter
>>
>>
>>
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-iio" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at http://vger.kernel.org/majordomo-info.html
>
^ permalink raw reply [flat|nested] 18+ messages in thread* Re: [PATCH 5/6] IIO:hwmon interface client driver.
2011-10-24 10:09 ` Jonathan Cameron
@ 2011-10-24 15:39 ` Guenter Roeck
2011-10-24 15:58 ` Jonathan Cameron
0 siblings, 1 reply; 18+ messages in thread
From: Guenter Roeck @ 2011-10-24 15:39 UTC (permalink / raw)
To: Jonathan Cameron
Cc: linux-kernel@vger.kernel.org, linux-iio@vger.kernel.org,
linus.ml.walleij@gmail.com, zdevai@gmail.com,
linux@arm.linux.org.uk, arnd@arndb.de,
broonie@opensource.wolfsonmicro.com, gregkh@suse.de,
lm-sensors@lm-sensors.org, khali@linux-fr.org,
thomas.petazzoni@free-electrons.com,
maxime.ripard@free-electrons.com
On Mon, 2011-10-24 at 06:09 -0400, Jonathan Cameron wrote:
[ ... ]
> >>> +/*
> >>> + * Assumes that IIO and hwmon operate in the same base units.
> >>> + * This is supposed to be true, but needs verification for
> >>> + * new channel types.
> >>> + */
> >>> +static ssize_t iio_hwmon_read_val(struct device *dev,
> >>> + struct device_attribute *attr,
> >>> + char *buf)
> >>> +{
> >>> + long result;
> >>> + int val, ret, scaleint, scalepart;
> >>> + struct sensor_device_attribute *sattr = to_sensor_dev_attr(attr);
> >>> + struct iio_hwmon_state *state = dev_get_drvdata(dev);
> >>> +
> >>> + /*
> >>> + * No locking between this pair, so theoretically possible
> >>> + * the scale has changed.
> >>> + */
> >>> + ret = iio_read_channel_raw(state->channels[sattr->index],
> >>> + &val);
> >>> + if (ret < 0)
> >>> + return ret;
> >>> +
> >>> + ret = iio_read_channel_scale(state->channels[sattr->index],
> >>> + &scaleint, &scalepart);
> >>> + if (ret < 0)
> >>> + return ret;
> >>> + switch (ret) {
> >>> + case IIO_VAL_INT:
> >>> + result = val * scaleint;
> >>> + break;
> >>> + case IIO_VAL_INT_PLUS_MICRO:
> >>> + result = (long)val * (long)scaleint +
> >>> + (long)val * (long)scalepart / 1000000L;
> >>> + break;
> >>> + case IIO_VAL_INT_PLUS_NANO:
> >>> + result = (long)val * (long)scaleint +
> >>> + (long)val * (long)scalepart / 1000000000L;
> >>> + break;
> >>
> >> Still easy to imagine that val * scalepart gets larger than 2147483647L
> >> (on machines where sizeof(long) = 4) ... it will already happen if the
> >> result of (val * scalepart / 1000000000) is larger than 2.
> > Good point. I really ought to have done the calcs.
> > If we have maximum possible value in here things will be ugly.
> >
> > Worst case is scalepart is 9999999999. (could be done as 1 - 0.000000001
> > which would be nicer, but we don't specify a preference - from this
> > discussion I am suspecting we should!)
> >
> > Looks like 64 bits is going to be a requirement as you say.
> >>
> >> What value range do you expect to see here ?
> >>
> >> If (val * scaleint) is already the milli-unit, scalepart would possibly
> >> only address fractions of milli-units. If so, the result of (val *
> >> scalepart / 1000000000L) might always be smaller than 1, ie 0.
> > It certainly should be.
> >> If so, for the calculation to have any value, you might be better off using
> >> DIV_ROUND_CLOSEST(val * scalepart, 1000000000L).
> > Good idea.
> >>
> >> I am a bit confused by this anyway. Since hwmon in general reports
> >> milli-units, VAL_INT appears to reflect milli-units, VAL_INT_PLUS_MICRO
> >> really means nano-units, and IIO_VAL_INT_PLUS_NANO really means
> >> pico-units. Is this correct ?
> > Micro units of the scale factor.
> >
> > Take my test part a max1363...
> > Scale is actually 0.5 so each adc count (e.g. raw value) is 0.5millivolts.
> >
> > scale int here is 0,
> > scale part is 500,000 (so 0.5) and it returns IIO_VAL_INT_PLUS_MICRO.
>
> How about the following? It'll be extremely costly, but this isn't exactly
> a fast path!
>
> case IIO_VAL_INT_PLUS_MICRO:
> result = (s64)val * (s64)scaleint +
> div_s64((s64)val * (s64)scalepart, 1000000LL);
> break;
> case IIO_VAL_INT_PLUS_NANO:
> result = (s64)val * (s64)scaleint +
> div_s64((s64)val * (s64)scalepart, 1000000000LL);
> break;
Is div_s64 really necessary, or would
result = (long)val * (long)scaleint +
DIV_ROUND_CLOSEST((s64)val * (s64)scalepart,
1000000000LL);
work as well ?
Thanks,
Guenter
^ permalink raw reply [flat|nested] 18+ messages in thread* Re: [PATCH 5/6] IIO:hwmon interface client driver.
2011-10-24 15:39 ` Guenter Roeck
@ 2011-10-24 15:58 ` Jonathan Cameron
2011-10-24 16:10 ` Guenter Roeck
2011-10-24 19:33 ` Russell King - ARM Linux
0 siblings, 2 replies; 18+ messages in thread
From: Jonathan Cameron @ 2011-10-24 15:58 UTC (permalink / raw)
To: guenter.roeck
Cc: linux-kernel@vger.kernel.org, linux-iio@vger.kernel.org,
linus.ml.walleij@gmail.com, zdevai@gmail.com,
linux@arm.linux.org.uk, arnd@arndb.de,
broonie@opensource.wolfsonmicro.com, gregkh@suse.de,
lm-sensors@lm-sensors.org, khali@linux-fr.org,
thomas.petazzoni@free-electrons.com,
maxime.ripard@free-electrons.com
On 10/24/11 16:39, Guenter Roeck wrote:
> On Mon, 2011-10-24 at 06:09 -0400, Jonathan Cameron wrote:
> [ ... ]
>>>>> +/*
>>>>> + * Assumes that IIO and hwmon operate in the same base units.
>>>>> + * This is supposed to be true, but needs verification for
>>>>> + * new channel types.
>>>>> + */
>>>>> +static ssize_t iio_hwmon_read_val(struct device *dev,
>>>>> + struct device_attribute *attr,
>>>>> + char *buf)
>>>>> +{
>>>>> + long result;
>>>>> + int val, ret, scaleint, scalepart;
>>>>> + struct sensor_device_attribute *sattr = to_sensor_dev_attr(attr);
>>>>> + struct iio_hwmon_state *state = dev_get_drvdata(dev);
>>>>> +
>>>>> + /*
>>>>> + * No locking between this pair, so theoretically possible
>>>>> + * the scale has changed.
>>>>> + */
>>>>> + ret = iio_read_channel_raw(state->channels[sattr->index],
>>>>> + &val);
>>>>> + if (ret < 0)
>>>>> + return ret;
>>>>> +
>>>>> + ret = iio_read_channel_scale(state->channels[sattr->index],
>>>>> + &scaleint, &scalepart);
>>>>> + if (ret < 0)
>>>>> + return ret;
>>>>> + switch (ret) {
>>>>> + case IIO_VAL_INT:
>>>>> + result = val * scaleint;
>>>>> + break;
>>>>> + case IIO_VAL_INT_PLUS_MICRO:
>>>>> + result = (long)val * (long)scaleint +
>>>>> + (long)val * (long)scalepart / 1000000L;
>>>>> + break;
>>>>> + case IIO_VAL_INT_PLUS_NANO:
>>>>> + result = (long)val * (long)scaleint +
>>>>> + (long)val * (long)scalepart / 1000000000L;
>>>>> + break;
>>>>
>>>> Still easy to imagine that val * scalepart gets larger than 2147483647L
>>>> (on machines where sizeof(long) = 4) ... it will already happen if the
>>>> result of (val * scalepart / 1000000000) is larger than 2.
>>> Good point. I really ought to have done the calcs.
>>> If we have maximum possible value in here things will be ugly.
>>>
>>> Worst case is scalepart is 9999999999. (could be done as 1 - 0.000000001
>>> which would be nicer, but we don't specify a preference - from this
>>> discussion I am suspecting we should!)
>>>
>>> Looks like 64 bits is going to be a requirement as you say.
>>>>
>>>> What value range do you expect to see here ?
>>>>
>>>> If (val * scaleint) is already the milli-unit, scalepart would possibly
>>>> only address fractions of milli-units. If so, the result of (val *
>>>> scalepart / 1000000000L) might always be smaller than 1, ie 0.
>>> It certainly should be.
>>>> If so, for the calculation to have any value, you might be better off using
>>>> DIV_ROUND_CLOSEST(val * scalepart, 1000000000L).
>>> Good idea.
>>>>
>>>> I am a bit confused by this anyway. Since hwmon in general reports
>>>> milli-units, VAL_INT appears to reflect milli-units, VAL_INT_PLUS_MICRO
>>>> really means nano-units, and IIO_VAL_INT_PLUS_NANO really means
>>>> pico-units. Is this correct ?
>>> Micro units of the scale factor.
>>>
>>> Take my test part a max1363...
>>> Scale is actually 0.5 so each adc count (e.g. raw value) is 0.5millivolts.
>>>
>>> scale int here is 0,
>>> scale part is 500,000 (so 0.5) and it returns IIO_VAL_INT_PLUS_MICRO.
>>
>> How about the following? It'll be extremely costly, but this isn't exactly
>> a fast path!
>>
>> case IIO_VAL_INT_PLUS_MICRO:
>> result = (s64)val * (s64)scaleint +
>> div_s64((s64)val * (s64)scalepart, 1000000LL);
>> break;
>> case IIO_VAL_INT_PLUS_NANO:
>> result = (s64)val * (s64)scaleint +
>> div_s64((s64)val * (s64)scalepart, 1000000000LL);
>> break;
>
> Is div_s64 really necessary, or would
>
> result = (long)val * (long)scaleint +
> DIV_ROUND_CLOSEST((s64)val * (s64)scalepart,
> 1000000000LL);
>
> work as well ?
Not if you want it to compile on arm v5 by the look of it.
ERROR: "__aeabi_ldivmod" [drivers/staging/iio/iio_hwmon.ko] undefined!
^ permalink raw reply [flat|nested] 18+ messages in thread* Re: [PATCH 5/6] IIO:hwmon interface client driver.
2011-10-24 15:58 ` Jonathan Cameron
@ 2011-10-24 16:10 ` Guenter Roeck
2011-10-24 16:15 ` Jonathan Cameron
2011-10-24 19:33 ` Russell King - ARM Linux
1 sibling, 1 reply; 18+ messages in thread
From: Guenter Roeck @ 2011-10-24 16:10 UTC (permalink / raw)
To: Jonathan Cameron
Cc: linux-kernel@vger.kernel.org, linux-iio@vger.kernel.org,
linus.ml.walleij@gmail.com, zdevai@gmail.com,
linux@arm.linux.org.uk, arnd@arndb.de,
broonie@opensource.wolfsonmicro.com, gregkh@suse.de,
lm-sensors@lm-sensors.org, khali@linux-fr.org,
thomas.petazzoni@free-electrons.com,
maxime.ripard@free-electrons.com
On Mon, 2011-10-24 at 11:58 -0400, Jonathan Cameron wrote:
> On 10/24/11 16:39, Guenter Roeck wrote:
> > On Mon, 2011-10-24 at 06:09 -0400, Jonathan Cameron wrote:
> > [ ... ]
> >>>>> +/*
> >>>>> + * Assumes that IIO and hwmon operate in the same base units.
> >>>>> + * This is supposed to be true, but needs verification for
> >>>>> + * new channel types.
> >>>>> + */
> >>>>> +static ssize_t iio_hwmon_read_val(struct device *dev,
> >>>>> + struct device_attribute *attr,
> >>>>> + char *buf)
> >>>>> +{
> >>>>> + long result;
> >>>>> + int val, ret, scaleint, scalepart;
> >>>>> + struct sensor_device_attribute *sattr = to_sensor_dev_attr(attr);
> >>>>> + struct iio_hwmon_state *state = dev_get_drvdata(dev);
> >>>>> +
> >>>>> + /*
> >>>>> + * No locking between this pair, so theoretically possible
> >>>>> + * the scale has changed.
> >>>>> + */
> >>>>> + ret = iio_read_channel_raw(state->channels[sattr->index],
> >>>>> + &val);
> >>>>> + if (ret < 0)
> >>>>> + return ret;
> >>>>> +
> >>>>> + ret = iio_read_channel_scale(state->channels[sattr->index],
> >>>>> + &scaleint, &scalepart);
> >>>>> + if (ret < 0)
> >>>>> + return ret;
> >>>>> + switch (ret) {
> >>>>> + case IIO_VAL_INT:
> >>>>> + result = val * scaleint;
> >>>>> + break;
> >>>>> + case IIO_VAL_INT_PLUS_MICRO:
> >>>>> + result = (long)val * (long)scaleint +
> >>>>> + (long)val * (long)scalepart / 1000000L;
> >>>>> + break;
> >>>>> + case IIO_VAL_INT_PLUS_NANO:
> >>>>> + result = (long)val * (long)scaleint +
> >>>>> + (long)val * (long)scalepart / 1000000000L;
> >>>>> + break;
> >>>>
> >>>> Still easy to imagine that val * scalepart gets larger than 2147483647L
> >>>> (on machines where sizeof(long) = 4) ... it will already happen if the
> >>>> result of (val * scalepart / 1000000000) is larger than 2.
> >>> Good point. I really ought to have done the calcs.
> >>> If we have maximum possible value in here things will be ugly.
> >>>
> >>> Worst case is scalepart is 9999999999. (could be done as 1 - 0.000000001
> >>> which would be nicer, but we don't specify a preference - from this
> >>> discussion I am suspecting we should!)
> >>>
> >>> Looks like 64 bits is going to be a requirement as you say.
> >>>>
> >>>> What value range do you expect to see here ?
> >>>>
> >>>> If (val * scaleint) is already the milli-unit, scalepart would possibly
> >>>> only address fractions of milli-units. If so, the result of (val *
> >>>> scalepart / 1000000000L) might always be smaller than 1, ie 0.
> >>> It certainly should be.
> >>>> If so, for the calculation to have any value, you might be better off using
> >>>> DIV_ROUND_CLOSEST(val * scalepart, 1000000000L).
> >>> Good idea.
> >>>>
> >>>> I am a bit confused by this anyway. Since hwmon in general reports
> >>>> milli-units, VAL_INT appears to reflect milli-units, VAL_INT_PLUS_MICRO
> >>>> really means nano-units, and IIO_VAL_INT_PLUS_NANO really means
> >>>> pico-units. Is this correct ?
> >>> Micro units of the scale factor.
> >>>
> >>> Take my test part a max1363...
> >>> Scale is actually 0.5 so each adc count (e.g. raw value) is 0.5millivolts.
> >>>
> >>> scale int here is 0,
> >>> scale part is 500,000 (so 0.5) and it returns IIO_VAL_INT_PLUS_MICRO.
> >>
> >> How about the following? It'll be extremely costly, but this isn't exactly
> >> a fast path!
> >>
> >> case IIO_VAL_INT_PLUS_MICRO:
> >> result = (s64)val * (s64)scaleint +
> >> div_s64((s64)val * (s64)scalepart, 1000000LL);
> >> break;
> >> case IIO_VAL_INT_PLUS_NANO:
> >> result = (s64)val * (s64)scaleint +
> >> div_s64((s64)val * (s64)scalepart, 1000000000LL);
> >> break;
> >
> > Is div_s64 really necessary, or would
> >
> > result = (long)val * (long)scaleint +
> > DIV_ROUND_CLOSEST((s64)val * (s64)scalepart,
> > 1000000000LL);
> >
> > work as well ?
> Not if you want it to compile on arm v5 by the look of it.
>
> ERROR: "__aeabi_ldivmod" [drivers/staging/iio/iio_hwmon.ko] undefined!
>
Annoying. Ok, I don't have a better idea than using div_s64. You don't
need s64 for the first part of the operation (val * scaleint), though,
since the result is a long.
Thanks,
Guenter
^ permalink raw reply [flat|nested] 18+ messages in thread* Re: [PATCH 5/6] IIO:hwmon interface client driver.
2011-10-24 16:10 ` Guenter Roeck
@ 2011-10-24 16:15 ` Jonathan Cameron
0 siblings, 0 replies; 18+ messages in thread
From: Jonathan Cameron @ 2011-10-24 16:15 UTC (permalink / raw)
To: guenter.roeck
Cc: linux-kernel@vger.kernel.org, linux-iio@vger.kernel.org,
linus.ml.walleij@gmail.com, zdevai@gmail.com,
linux@arm.linux.org.uk, arnd@arndb.de,
broonie@opensource.wolfsonmicro.com, gregkh@suse.de,
lm-sensors@lm-sensors.org, khali@linux-fr.org,
thomas.petazzoni@free-electrons.com,
maxime.ripard@free-electrons.com
On 10/24/11 17:10, Guenter Roeck wrote:
> On Mon, 2011-10-24 at 11:58 -0400, Jonathan Cameron wrote:
>> On 10/24/11 16:39, Guenter Roeck wrote:
>>> On Mon, 2011-10-24 at 06:09 -0400, Jonathan Cameron wrote:
>>> [ ... ]
>>>>>>> +/*
>>>>>>> + * Assumes that IIO and hwmon operate in the same base units.
>>>>>>> + * This is supposed to be true, but needs verification for
>>>>>>> + * new channel types.
>>>>>>> + */
>>>>>>> +static ssize_t iio_hwmon_read_val(struct device *dev,
>>>>>>> + struct device_attribute *attr,
>>>>>>> + char *buf)
>>>>>>> +{
>>>>>>> + long result;
>>>>>>> + int val, ret, scaleint, scalepart;
>>>>>>> + struct sensor_device_attribute *sattr = to_sensor_dev_attr(attr);
>>>>>>> + struct iio_hwmon_state *state = dev_get_drvdata(dev);
>>>>>>> +
>>>>>>> + /*
>>>>>>> + * No locking between this pair, so theoretically possible
>>>>>>> + * the scale has changed.
>>>>>>> + */
>>>>>>> + ret = iio_read_channel_raw(state->channels[sattr->index],
>>>>>>> + &val);
>>>>>>> + if (ret < 0)
>>>>>>> + return ret;
>>>>>>> +
>>>>>>> + ret = iio_read_channel_scale(state->channels[sattr->index],
>>>>>>> + &scaleint, &scalepart);
>>>>>>> + if (ret < 0)
>>>>>>> + return ret;
>>>>>>> + switch (ret) {
>>>>>>> + case IIO_VAL_INT:
>>>>>>> + result = val * scaleint;
>>>>>>> + break;
>>>>>>> + case IIO_VAL_INT_PLUS_MICRO:
>>>>>>> + result = (long)val * (long)scaleint +
>>>>>>> + (long)val * (long)scalepart / 1000000L;
>>>>>>> + break;
>>>>>>> + case IIO_VAL_INT_PLUS_NANO:
>>>>>>> + result = (long)val * (long)scaleint +
>>>>>>> + (long)val * (long)scalepart / 1000000000L;
>>>>>>> + break;
>>>>>>
>>>>>> Still easy to imagine that val * scalepart gets larger than 2147483647L
>>>>>> (on machines where sizeof(long) = 4) ... it will already happen if the
>>>>>> result of (val * scalepart / 1000000000) is larger than 2.
>>>>> Good point. I really ought to have done the calcs.
>>>>> If we have maximum possible value in here things will be ugly.
>>>>>
>>>>> Worst case is scalepart is 9999999999. (could be done as 1 - 0.000000001
>>>>> which would be nicer, but we don't specify a preference - from this
>>>>> discussion I am suspecting we should!)
>>>>>
>>>>> Looks like 64 bits is going to be a requirement as you say.
>>>>>>
>>>>>> What value range do you expect to see here ?
>>>>>>
>>>>>> If (val * scaleint) is already the milli-unit, scalepart would possibly
>>>>>> only address fractions of milli-units. If so, the result of (val *
>>>>>> scalepart / 1000000000L) might always be smaller than 1, ie 0.
>>>>> It certainly should be.
>>>>>> If so, for the calculation to have any value, you might be better off using
>>>>>> DIV_ROUND_CLOSEST(val * scalepart, 1000000000L).
>>>>> Good idea.
>>>>>>
>>>>>> I am a bit confused by this anyway. Since hwmon in general reports
>>>>>> milli-units, VAL_INT appears to reflect milli-units, VAL_INT_PLUS_MICRO
>>>>>> really means nano-units, and IIO_VAL_INT_PLUS_NANO really means
>>>>>> pico-units. Is this correct ?
>>>>> Micro units of the scale factor.
>>>>>
>>>>> Take my test part a max1363...
>>>>> Scale is actually 0.5 so each adc count (e.g. raw value) is 0.5millivolts.
>>>>>
>>>>> scale int here is 0,
>>>>> scale part is 500,000 (so 0.5) and it returns IIO_VAL_INT_PLUS_MICRO.
>>>>
>>>> How about the following? It'll be extremely costly, but this isn't exactly
>>>> a fast path!
>>>>
>>>> case IIO_VAL_INT_PLUS_MICRO:
>>>> result = (s64)val * (s64)scaleint +
>>>> div_s64((s64)val * (s64)scalepart, 1000000LL);
>>>> break;
>>>> case IIO_VAL_INT_PLUS_NANO:
>>>> result = (s64)val * (s64)scaleint +
>>>> div_s64((s64)val * (s64)scalepart, 1000000000LL);
>>>> break;
>>>
>>> Is div_s64 really necessary, or would
>>>
>>> result = (long)val * (long)scaleint +
>>> DIV_ROUND_CLOSEST((s64)val * (s64)scalepart,
>>> 1000000000LL);
>>>
>>> work as well ?
>> Not if you want it to compile on arm v5 by the look of it.
>>
>> ERROR: "__aeabi_ldivmod" [drivers/staging/iio/iio_hwmon.ko] undefined!
>>
> Annoying. Ok, I don't have a better idea than using div_s64. You don't
> need s64 for the first part of the operation (val * scaleint), though,
> since the result is a long.
True enough. Pretty unlikely we are going to have 2 MV hwmon devices any
time soon. I'll pop that back down to int * int I think!
^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH 5/6] IIO:hwmon interface client driver.
2011-10-24 15:58 ` Jonathan Cameron
2011-10-24 16:10 ` Guenter Roeck
@ 2011-10-24 19:33 ` Russell King - ARM Linux
1 sibling, 0 replies; 18+ messages in thread
From: Russell King - ARM Linux @ 2011-10-24 19:33 UTC (permalink / raw)
To: Jonathan Cameron
Cc: guenter.roeck, linux-kernel@vger.kernel.org,
linux-iio@vger.kernel.org, linus.ml.walleij@gmail.com,
zdevai@gmail.com, arnd@arndb.de,
broonie@opensource.wolfsonmicro.com, gregkh@suse.de,
lm-sensors@lm-sensors.org, khali@linux-fr.org,
thomas.petazzoni@free-electrons.com,
maxime.ripard@free-electrons.com
On Mon, Oct 24, 2011 at 04:58:49PM +0100, Jonathan Cameron wrote:
> On 10/24/11 16:39, Guenter Roeck wrote:
> > Is div_s64 really necessary, or would
> >
> > result = (long)val * (long)scaleint +
> > DIV_ROUND_CLOSEST((s64)val * (s64)scalepart,
> > 1000000000LL);
> >
> > work as well ?
> Not if you want it to compile on arm v5 by the look of it.
>
> ERROR: "__aeabi_ldivmod" [drivers/staging/iio/iio_hwmon.ko] undefined!
You know, div64 is there to deal with the case of _sanely_ dividing
a 64-bit number by a 32-bit number - there should be absolutely no
question about _not_ using it if that's the operation you are wanting
to perform.
Expecting gcc to do a better job without using div64 is just asking for
bad code on 32-bit platforms.
^ permalink raw reply [flat|nested] 18+ messages in thread
* [PATCH 6/6] stargate2: example of map configuration for iio to hwmon example.
2011-10-20 9:33 [RFC V3 PATCH 0/6] IIO in kernel interfaces Jonathan Cameron
` (4 preceding siblings ...)
2011-10-20 9:33 ` [PATCH 5/6] IIO:hwmon interface client driver Jonathan Cameron
@ 2011-10-20 9:33 ` Jonathan Cameron
5 siblings, 0 replies; 18+ messages in thread
From: Jonathan Cameron @ 2011-10-20 9:33 UTC (permalink / raw)
To: linux-kernel, linux-iio
Cc: linus.ml.walleij, zdevai, linux, arnd, broonie, gregkh,
lm-sensors, guenter.roeck, khali, thomas.petazzoni, maxime.ripard,
Jonathan Cameron
Do not commit.
---
arch/arm/mach-pxa/stargate2.c | 23 +++++++++++++++++++++++
1 files changed, 23 insertions(+), 0 deletions(-)
diff --git a/arch/arm/mach-pxa/stargate2.c b/arch/arm/mach-pxa/stargate2.c
index 3f8d0af..48dcbb7 100644
--- a/arch/arm/mach-pxa/stargate2.c
+++ b/arch/arm/mach-pxa/stargate2.c
@@ -54,6 +54,8 @@
#include <linux/mfd/da903x.h>
#include <linux/sht15.h>
+#include <linux/iio/inkern.h>
+
#include "devices.h"
#include "generic.h"
@@ -406,6 +408,24 @@ static struct i2c_pxa_platform_data i2c_pdata = {
.fast_mode = 1,
};
+static struct platform_device iio_hwmon_test = {
+ .name = "iio_hwmon",
+};
+
+static struct iio_map adc_map[] = {
+ {
+ .adc_dev_name = "0-0035",
+ .adc_channel_label = "AIN1",
+ .consumer_dev = &iio_hwmon_test.dev,
+ .consumer_channel = "testchan1",
+ }, {
+ .adc_dev_name = "0-0035",
+ .adc_channel_label = "AIN2",
+ .consumer_dev = &iio_hwmon_test.dev,
+ .consumer_channel = "testchan2",
+ },
+};
+
static void __init imote2_stargate2_init(void)
{
@@ -423,6 +443,8 @@ static void __init imote2_stargate2_init(void)
pxa27x_set_i2c_power_info(&i2c_pwr_pdata);
pxa_set_i2c_info(&i2c_pdata);
+
+ iio_map_array_register(adc_map, ARRAY_SIZE(adc_map));
}
#ifdef CONFIG_MACH_INTELMOTE2
@@ -971,6 +993,7 @@ static struct platform_device *stargate2_devices[] = {
&stargate2_sram,
&smc91x_device,
&sht15,
+ &iio_hwmon_test,
};
static void __init stargate2_init(void)
--
1.7.7
^ permalink raw reply related [flat|nested] 18+ messages in thread