[PATCH 0/5] IIO/staging inkernel pull interface

[PATCH 0/5] IIO/staging inkernel pull interface

[Jonathan Cameron]

This is a direct lift across from the out of staging tree
of the proposed in kernel pull interface. We need this here
to allow work on having both push and pull interfaces.

This may get tweaked in response to futher feedback on the other
patches.

Thanks,

Jonathan

Jonathan Cameron (5):
  staging:iio: core: add datasheet_name to chan_spec
  staging:iio:adc:max1363 add datasheet_name entries.
  staging:iio:core add in kernel interface mapping and getting IIO
    channels.
  staging;iio: move iio data return types into types.h for use by
    inkern
  staging:iio::hwmon interface client driver.

 drivers/staging/Makefile                |    2 +-
 drivers/staging/iio/Kconfig             |    8 +
 drivers/staging/iio/Makefile            |    4 +-
 drivers/staging/iio/adc/max1363_core.c  |    2 +
 drivers/staging/iio/iio.h               |   17 ++-
 drivers/staging/iio/iio_hwmon.c         |  225 ++++++++++++++++++++++++++
 drivers/staging/iio/industrialio-core.c |  268 ++++++++++++++++++++++++++++++-
 drivers/staging/iio/inkern.c            |   21 +++
 drivers/staging/iio/inkern.h            |   86 ++++++++++
 drivers/staging/iio/types.h             |    4 +
 10 files changed, 623 insertions(+), 14 deletions(-)
 create mode 100644 drivers/staging/iio/iio_hwmon.c
 create mode 100644 drivers/staging/iio/inkern.c
 create mode 100644 drivers/staging/iio/inkern.h

-- 
1.7.7.3

[PATCH 1/5] staging:iio: core: add datasheet_name to chan_spec

[Jonathan Cameron]

From: Jonathan Cameron <jic23@cam.ac.uk>

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>
---
 drivers/staging/iio/iio.h |    5 +++++
 1 files changed, 5 insertions(+), 0 deletions(-)

diff --git a/drivers/staging/iio/iio.h b/drivers/staging/iio/iio.h
index c225542..11c2f25 100644
--- a/drivers/staging/iio/iio.h
+++ b/drivers/staging/iio/iio.h
@@ -110,6 +110,10 @@ enum iio_endian {
  * @extend_name:	Allows labeling of channel attributes with an
  *			informative name. Note this has no effect codes etc,
  *			unlike modifiers.
+ * @datasheet_name:	A name used in in kernel mapping of channels. It should
+ *			corrspond to the first name that the channel is referred
+ *			to by in the datasheet (e.g. IND), or the nearest
+ *			possible compound name (e.g. IND-INC).
  * @processed_val:	Flag to specify the data access attribute should be
  *			*_input rather than *_raw.
  * @modified:		Does a modifier apply to this channel. What these are
@@ -138,6 +142,7 @@ struct iio_chan_spec {
 	long			info_mask;
 	long			event_mask;
 	char			*extend_name;
+	const char		*datasheet_name;
 	unsigned		processed_val:1;
 	unsigned		modified:1;
 	unsigned		indexed:1;
-- 
1.7.7.3

[PATCH 2/5] staging:iio:adc:max1363 add datasheet_name entries.

[Jonathan Cameron]

From: Jonathan Cameron <jic23@cam.ac.uk>

Kind of obvious for this device but useful
for testing purposes.

Signed-off-by: Jonathan Cameron <jic23@cam.ac.uk>
---
 drivers/staging/iio/adc/max1363_core.c |    2 ++
 1 files changed, 2 insertions(+), 0 deletions(-)

diff --git a/drivers/staging/iio/adc/max1363_core.c b/drivers/staging/iio/adc/max1363_core.c
index 9febd1b..0b97123 100644
--- a/drivers/staging/iio/adc/max1363_core.c
+++ b/drivers/staging/iio/adc/max1363_core.c
@@ -298,6 +298,7 @@ static const enum max1363_modes max1363_mode_list[] = {
 		.channel = num,						\
 		.address = addr,					\
 		.info_mask = MAX1363_INFO_MASK,				\
+		.datasheet_name = "AIN"#num,				\
 		.scan_type = {						\
 			.sign = 'u',					\
 			.realbits = bits,				\
@@ -318,6 +319,7 @@ static const enum max1363_modes max1363_mode_list[] = {
 		.channel2 = num2,					\
 		.address = addr,					\
 		.info_mask = MAX1363_INFO_MASK,				\
+		.datasheet_name = "AIN"#num"-AIN"#num2,			\
 		.scan_type = {						\
 			.sign = 's',					\
 			.realbits = bits,				\
-- 
1.7.7.3

[PATCH 3/5] staging:iio:core add in kernel interface mapping and getting IIO channels.

[Jonathan Cameron]

From: Jonathan Cameron <jic23@cam.ac.uk>

Lifted from proposal for in kernel interface built on the out of staging
branch.

Two elements here:
* Map as defined in "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/staging/Makefile                |    2 +-
 drivers/staging/iio/Makefile            |    2 +-
 drivers/staging/iio/iio.h               |    6 +-
 drivers/staging/iio/industrialio-core.c |  268 ++++++++++++++++++++++++++++++-
 drivers/staging/iio/inkern.c            |   21 +++
 drivers/staging/iio/inkern.h            |   86 ++++++++++
 6 files changed, 377 insertions(+), 8 deletions(-)

diff --git a/drivers/staging/Makefile b/drivers/staging/Makefile
index ba3fb8d..46e24b4 100644
--- a/drivers/staging/Makefile
+++ b/drivers/staging/Makefile
@@ -33,7 +33,7 @@ obj-$(CONFIG_VT6656)		+= vt6656/
 obj-$(CONFIG_HYPERV)		+= hv/
 obj-$(CONFIG_VME_BUS)		+= vme/
 obj-$(CONFIG_DX_SEP)            += sep/
-obj-$(CONFIG_IIO)		+= iio/
+obj-y				+= iio/
 obj-$(CONFIG_ZRAM)		+= zram/
 obj-$(CONFIG_XVMALLOC)		+= zram/
 obj-$(CONFIG_ZCACHE)		+= zcache/
diff --git a/drivers/staging/iio/Makefile b/drivers/staging/iio/Makefile
index 1340aea..04d6ad2 100644
--- a/drivers/staging/iio/Makefile
+++ b/drivers/staging/iio/Makefile
@@ -1,7 +1,7 @@
 #
 # Makefile for the industrial I/O core.
 #
-
+obj-y = inkern.o
 obj-$(CONFIG_IIO) += industrialio.o
 industrialio-y := industrialio-core.o
 industrialio-$(CONFIG_IIO_BUFFER) += industrialio-buffer.o
diff --git a/drivers/staging/iio/iio.h b/drivers/staging/iio/iio.h
index 11c2f25..4650a2b 100644
--- a/drivers/staging/iio/iio.h
+++ b/drivers/staging/iio/iio.h
@@ -230,7 +230,9 @@ struct iio_dev;
  *			Meaning is event dependent.
  * @validate_trigger:	function to validate the trigger when the
  *			current trigger gets changed.
- **/
+ * @			the parent device (actual hardware).  Note that if
+ *			not specified then iio_dev.dev->parent is used.
+ */
 struct iio_info {
 	struct module			*driver_module;
 	struct attribute_group		*event_attrs;
@@ -267,8 +269,10 @@ struct iio_info {
 				 int val);
 	int (*validate_trigger)(struct iio_dev *indio_dev,
 				struct iio_trigger *trig);
+
 	int (*update_scan_mode)(struct iio_dev *indio_dev,
 				const unsigned long *scan_mask);
+	struct device *(*get_hardware_id)(struct iio_dev *indio_dev);
 };
 
 /**
diff --git a/drivers/staging/iio/industrialio-core.c b/drivers/staging/iio/industrialio-core.c
index 3754b85..a46d7aa 100644
--- a/drivers/staging/iio/industrialio-core.c
+++ b/drivers/staging/iio/industrialio-core.c
@@ -22,11 +22,13 @@
 #include <linux/cdev.h>
 #include <linux/slab.h>
 #include <linux/anon_inodes.h>
+#include <linux/err.h>
 #include "iio.h"
 #include "iio_core.h"
 #include "iio_core_trigger.h"
 #include "sysfs.h"
 #include "events.h"
+#include "inkern.h"
 
 /* IDA to assign each registered device a unique id*/
 static DEFINE_IDA(iio_ida);
@@ -89,6 +91,267 @@ static const char * const iio_chan_info_postfix[] = {
 	= "filter_low_pass_3db_frequency",
 };
 
+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;
+}
+
+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_st_channel_get(const struct device *dev,
+				       const char *name,
+				       const char *channel_name)
+{
+	struct iio_map *c_i = NULL, *c = NULL;
+	struct device *dev_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);
+
+	channel = kmalloc(sizeof(*channel), GFP_KERNEL);
+	if (channel == NULL)
+		return ERR_PTR(-ENOMEM);
+
+	channel->indio_dev = container_of(dev_i, struct iio_dev, dev);
+
+	if (c->adc_channel_label)
+		channel->channel =
+			iio_chan_spec_from_name(channel->indio_dev,
+						c->adc_channel_label);
+	if (channel->channel == NULL)
+		channel->channel = &channel->indio_dev->
+			channels[c->channel_number];
+
+	return channel;
+}
+EXPORT_SYMBOL_GPL(iio_st_channel_get);
+
+void iio_st_channel_release(struct iio_channel *channel)
+{
+	put_device(&channel->indio_dev->dev);
+	kfree(channel);
+}
+EXPORT_SYMBOL_GPL(iio_st_channel_release);
+
+struct iio_channel **iio_st_channel_get_all(const struct device *dev,
+					    const char *name)
+{
+	struct iio_channel **chans;
+	struct iio_map *c = NULL;
+	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;
+	}
+
+	/* NULL terminated array to save passing size */
+	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;
+
+			chans[mapind]->indio_dev =
+				container_of(dev_i, struct iio_dev, dev);
+			chans[mapind]->channel =
+				iio_chan_spec_from_name(chans[mapind]->
+							indio_dev,
+							c->adc_channel_label);
+			if (chans[mapind]->channel == NULL) {
+				ret = -EINVAL;
+				put_device(&chans[mapind]->indio_dev->dev);
+				goto error_free_chans;
+			}
+			mapind++;
+		}
+	}
+	if (mapind == 0) {
+		ret = -ENODEV;
+		goto error_free_chans;
+	}
+	return chans;
+
+error_free_chans:
+	for (i = 0; i < nummaps; i++)
+		if (chans[i]) {
+			if (chans[i]->indio_dev)
+				put_device(&chans[i]->indio_dev->dev);
+			kfree(chans[i]);
+		}
+	kfree(chans);
+error_ret:
+
+	return ERR_PTR(ret);
+}
+EXPORT_SYMBOL_GPL(iio_st_channel_get_all);
+
+void iio_st_channel_release_all(struct iio_channel **channels)
+{
+	int i = 0;
+	struct iio_channel *chan = channels[i];
+
+	while (chan) {
+		if (chan->indio_dev)
+			put_device(&chan->indio_dev->dev);
+		kfree(chan);
+		i++;
+		chan = channels[i];
+	}
+	kfree(channels);
+}
+EXPORT_SYMBOL_GPL(iio_st_channel_release_all);
+
+int iio_st_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_st_read_channel_raw);
+
+int iio_st_read_channel_scale(struct iio_channel *chan, int *val, int *val2)
+{
+	return chan->indio_dev->info->read_raw(chan->indio_dev,
+					       chan->channel,
+					       val, val2,
+					       IIO_CHAN_INFO_SCALE);
+}
+EXPORT_SYMBOL_GPL(iio_st_read_channel_scale);
+
+enum iio_chan_type iio_st_get_channel_type(struct iio_channel *channel)
+{
+	return channel->channel->type;
+}
+EXPORT_SYMBOL_GPL(iio_st_get_channel_type);
+
 const struct iio_chan_spec
 *iio_find_channel_from_si(struct iio_dev *indio_dev, int si)
 {
@@ -1028,11 +1291,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_allocate_device(int sizeof_priv)
 {
 	struct iio_dev *dev;
diff --git a/drivers/staging/iio/inkern.c b/drivers/staging/iio/inkern.c
new file mode 100644
index 0000000..3e86093
--- /dev/null
+++ b/drivers/staging/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 "inkern.h"
+#include <linux/err.h>
+#include <linux/export.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/drivers/staging/iio/inkern.h b/drivers/staging/iio/inkern.h
new file mode 100644
index 0000000..fc32896
--- /dev/null
+++ b/drivers/staging/iio/inkern.h
@@ -0,0 +1,86 @@
+#include <linux/device.h>
+#include <linux/list.h>
+#include "types.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_st_channel_get(const struct device *dev,
+				    const char *name,
+				    const char *consumer_channel);
+void iio_st_channel_release(struct iio_channel *chan);
+
+/**
+ * iio_st_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_st_channel_get_all(const struct device *dev,
+					const char *name);
+
+void iio_st_channel_release_all(struct iio_channel **chan);
+
+/**
+ * iio_st_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_st_read_channel_raw(struct iio_channel *chan,
+			    int *val);
+
+/**
+ * iio_st_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_st_get_channel_type(struct iio_channel *channel);
+
+/**
+ * iio_st_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_st_read_channel_scale(struct iio_channel *chan, int *val,
+			      int *val2);
+
+#endif
-- 
1.7.7.3

[PATCH 4/5] staging;iio: move iio data return types into types.h for use by inkern

[Jonathan Cameron]

From: Jonathan Cameron <jic23@cam.ac.uk>

In kernel interfaces need these, so make them available.

Signed-off-by: Jonathan Cameron <jic23@cam.ac.uk>
---
 drivers/staging/iio/iio.h |    6 ------
 1 files changed, 0 insertions(+), 6 deletions(-)

diff --git a/drivers/staging/iio/iio.h b/drivers/staging/iio/iio.h
index 4650a2b..e30d33d 100644
--- a/drivers/staging/iio/iio.h
+++ b/drivers/staging/iio/iio.h
@@ -197,12 +197,6 @@ static inline s64 iio_get_time_ns(void)
 #define INDIO_ALL_BUFFER_MODES					\
 	(INDIO_BUFFER_TRIGGERED | INDIO_BUFFER_HARDWARE)
 
-/* Vast majority of this is set by the industrialio subsystem on a
- * call to iio_device_register. */
-#define IIO_VAL_INT 1
-#define IIO_VAL_INT_PLUS_MICRO 2
-#define IIO_VAL_INT_PLUS_NANO 3
-
 struct iio_trigger; /* forward declaration */
 struct iio_dev;
 
-- 
1.7.7.3

Re: [PATCH 4/5] staging;iio: move iio data return types into types.h for use by inkern

[Lars-Peter Clausen]

On 11/27/2011 02:14 PM, Jonathan Cameron wrote:
> From: Jonathan Cameron <jic23@cam.ac.uk>
> 
> In kernel interfaces need these, so make them available.
> 
> Signed-off-by: Jonathan Cameron <jic23@cam.ac.uk>
> ---
>  drivers/staging/iio/iio.h |    6 ------
>  1 files changed, 0 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/staging/iio/iio.h b/drivers/staging/iio/iio.h
> index 4650a2b..e30d33d 100644
> --- a/drivers/staging/iio/iio.h
> +++ b/drivers/staging/iio/iio.h
> @@ -197,12 +197,6 @@ static inline s64 iio_get_time_ns(void)
>  #define INDIO_ALL_BUFFER_MODES					\
>  	(INDIO_BUFFER_TRIGGERED | INDIO_BUFFER_HARDWARE)
>  
> -/* Vast majority of this is set by the industrialio subsystem on a
> - * call to iio_device_register. */
> -#define IIO_VAL_INT 1
> -#define IIO_VAL_INT_PLUS_MICRO 2
> -#define IIO_VAL_INT_PLUS_NANO 3
> -
>  struct iio_trigger; /* forward declaration */
>  struct iio_dev;
>  

The second part of this patch which adds the here removed defines to types.h
seems to have slipped into patch 5.

Re: [PATCH 4/5] staging;iio: move iio data return types into types.h for use by inkern

[Jonathan Cameron]

On 12/05/2011 10:07 AM, Lars-Peter Clausen wrote:
> On 11/27/2011 02:14 PM, Jonathan Cameron wrote:
>> From: Jonathan Cameron <jic23@cam.ac.uk>
>>
>> In kernel interfaces need these, so make them available.
>>
>> Signed-off-by: Jonathan Cameron <jic23@cam.ac.uk>
>> ---
>>  drivers/staging/iio/iio.h |    6 ------
>>  1 files changed, 0 insertions(+), 6 deletions(-)
>>
>> diff --git a/drivers/staging/iio/iio.h b/drivers/staging/iio/iio.h
>> index 4650a2b..e30d33d 100644
>> --- a/drivers/staging/iio/iio.h
>> +++ b/drivers/staging/iio/iio.h
>> @@ -197,12 +197,6 @@ static inline s64 iio_get_time_ns(void)
>>  #define INDIO_ALL_BUFFER_MODES					\
>>  	(INDIO_BUFFER_TRIGGERED | INDIO_BUFFER_HARDWARE)
>>  
>> -/* Vast majority of this is set by the industrialio subsystem on a
>> - * call to iio_device_register. */
>> -#define IIO_VAL_INT 1
>> -#define IIO_VAL_INT_PLUS_MICRO 2
>> -#define IIO_VAL_INT_PLUS_NANO 3
>> -
>>  struct iio_trigger; /* forward declaration */
>>  struct iio_dev;
>>  
> 
> The second part of this patch which adds the here removed defines to types.h
> seems to have slipped into patch 5.
Excellent catch of an idiot mistake!  Thanks. They are now in this patch.

Thanks for looking at all of these.

Jonathan

[PATCH 5/5] staging:iio::hwmon interface client driver.

[Jonathan Cameron]

From: Jonathan Cameron <jic23@cam.ac.uk>

Direct copy of version proposed for the non staging branch.
Needed here to allow testing of more advanced inkernel
interface code.

Minimal support of simple in, curr and temp attributes
so far.

Signed-off-by: Jonathan Cameron <jic23@cam.ac.uk>
---
 drivers/staging/iio/Kconfig     |    8 ++
 drivers/staging/iio/Makefile    |    2 +
 drivers/staging/iio/iio_hwmon.c |  225 +++++++++++++++++++++++++++++++++++++++
 drivers/staging/iio/types.h     |    4 +
 4 files changed, 239 insertions(+), 0 deletions(-)

diff --git a/drivers/staging/iio/Kconfig b/drivers/staging/iio/Kconfig
index 90162aa..f2c5db6 100644
--- a/drivers/staging/iio/Kconfig
+++ b/drivers/staging/iio/Kconfig
@@ -12,6 +12,14 @@ menuconfig IIO
 	  drivers/staging/iio/Documentation for more information.
 if IIO
 
+config IIO_ST_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.
+
 config IIO_BUFFER
 	bool "Enable buffer support within IIO"
 	help
diff --git a/drivers/staging/iio/Makefile b/drivers/staging/iio/Makefile
index 04d6ad2..d5ca5cf 100644
--- a/drivers/staging/iio/Makefile
+++ b/drivers/staging/iio/Makefile
@@ -17,6 +17,8 @@ iio_dummy-$(CONFIG_IIO_SIMPLE_DUMMY_BUFFER) += iio_simple_dummy_buffer.o
 
 obj-$(CONFIG_IIO_DUMMY_EVGEN) += iio_dummy_evgen.o
 
+obj-$(CONFIG_IIO_ST_HWMON) += iio_hwmon.o
+
 obj-y += accel/
 obj-y += adc/
 obj-y += addac/
diff --git a/drivers/staging/iio/iio_hwmon.c b/drivers/staging/iio/iio_hwmon.c
new file mode 100644
index 0000000..de610c0
--- /dev/null
+++ b/drivers/staging/iio/iio_hwmon.c
@@ -0,0 +1,225 @@
+/* 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.
+ */
+
+#include <linux/kernel.h>
+#include <linux/slab.h>
+#include <linux/module.h>
+#include <linux/err.h>
+#include <linux/platform_device.h>
+#include <linux/hwmon.h>
+#include <linux/hwmon-sysfs.h>
+#include "inkern.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_st_read_channel_raw(state->channels[sattr->index],
+				      &val);
+	if (ret < 0)
+		return ret;
+
+	ret = iio_st_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 = (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;
+	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_st_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_st_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_st_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_st_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");
diff --git a/drivers/staging/iio/types.h b/drivers/staging/iio/types.h
index f1f5ca2..b2ab217 100644
--- a/drivers/staging/iio/types.h
+++ b/drivers/staging/iio/types.h
@@ -46,4 +46,8 @@ enum iio_modifier {
 	IIO_MOD_LIGHT_IR,
 };
 
+#define IIO_VAL_INT 1
+#define IIO_VAL_INT_PLUS_MICRO 2
+#define IIO_VAL_INT_PLUS_NANO 3
+
 #endif /* _IIO_TYPES_H_ */
-- 
1.7.7.3

[PATCH 0/5] staging:iio: inkern pull interfaces for staging tree.

[Jonathan Cameron]

Hi Greg,

This set is a little bit of a novelty.  It's actually a direct lift
of a series that has been proposed and reviewed against the
set of patches for the core of IIO outside of the staging
tree.  We need it here in staging for two reasons.

1) To allow for drivers using this to exist in the staging tree
with all the stuff that is there but not yet ready to go out
of staging.

2) To allow development of the push in kernel interface stuff (triggered
capture) to procede.  We have no where near enough of the core
ready to go out of staging yet to be able to do that work there.

Anyhow, very little review here because it was all in the other thread.

For reference:
https://lkml.org/lkml/2011/11/7/258

I haven't lifted the acks across because they were obviously not
able to consider any issues of interaction with the rest of IIO
given they were only looking at a subset.  (Not that I think
there are any!).

Lars-Peter pointed out a silly mess up to do with two patches
getting weirdly split up so that is fixed here wrt to the version
I posted to linux-iio.

Thanks,

Jonathan

Jonathan Cameron (5):
  staging:iio: core: add datasheet_name to chan_spec
  staging:iio:adc:max1363 add datasheet_name entries.
  staging:iio:core add in kernel interface mapping and getting IIO
    channels.
  staging;iio: move iio data return types into types.h for use by
    inkern
  staging:iio::hwmon interface client driver.

 drivers/staging/Makefile                |    2 +-
 drivers/staging/iio/Kconfig             |    8 +
 drivers/staging/iio/Makefile            |    4 +-
 drivers/staging/iio/adc/max1363_core.c  |    2 +
 drivers/staging/iio/iio.h               |   17 ++-
 drivers/staging/iio/iio_hwmon.c         |  225 ++++++++++++++++++++++++++
 drivers/staging/iio/industrialio-core.c |  268 ++++++++++++++++++++++++++++++-
 drivers/staging/iio/inkern.c            |   21 +++
 drivers/staging/iio/inkern.h            |   86 ++++++++++
 drivers/staging/iio/types.h             |    4 +
 10 files changed, 623 insertions(+), 14 deletions(-)
 create mode 100644 drivers/staging/iio/iio_hwmon.c
 create mode 100644 drivers/staging/iio/inkern.c
 create mode 100644 drivers/staging/iio/inkern.h

-- 
1.7.7.4

[PATCH 3/5] staging:iio:core add in kernel interface mapping and getting IIO channels.

[Jonathan Cameron]

From: Jonathan Cameron <jic23@cam.ac.uk>

Lifted from proposal for in kernel interface built on the out of staging
branch.

Two elements here:
* Map as defined in "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/staging/Makefile                |    2 +-
 drivers/staging/iio/Makefile            |    2 +-
 drivers/staging/iio/iio.h               |    6 +-
 drivers/staging/iio/industrialio-core.c |  268 ++++++++++++++++++++++++++++++-
 drivers/staging/iio/inkern.c            |   21 +++
 drivers/staging/iio/inkern.h            |   86 ++++++++++
 6 files changed, 377 insertions(+), 8 deletions(-)

diff --git a/drivers/staging/Makefile b/drivers/staging/Makefile
index 6e615b6..f59ab89 100644
--- a/drivers/staging/Makefile
+++ b/drivers/staging/Makefile
@@ -34,7 +34,7 @@ obj-$(CONFIG_VT6656)		+= vt6656/
 obj-$(CONFIG_HYPERV)		+= hv/
 obj-$(CONFIG_VME_BUS)		+= vme/
 obj-$(CONFIG_DX_SEP)            += sep/
-obj-$(CONFIG_IIO)		+= iio/
+obj-y				+= iio/
 obj-$(CONFIG_ZRAM)		+= zram/
 obj-$(CONFIG_XVMALLOC)		+= zram/
 obj-$(CONFIG_ZCACHE)		+= zcache/
diff --git a/drivers/staging/iio/Makefile b/drivers/staging/iio/Makefile
index 1340aea..04d6ad2 100644
--- a/drivers/staging/iio/Makefile
+++ b/drivers/staging/iio/Makefile
@@ -1,7 +1,7 @@
 #
 # Makefile for the industrial I/O core.
 #
-
+obj-y = inkern.o
 obj-$(CONFIG_IIO) += industrialio.o
 industrialio-y := industrialio-core.o
 industrialio-$(CONFIG_IIO_BUFFER) += industrialio-buffer.o
diff --git a/drivers/staging/iio/iio.h b/drivers/staging/iio/iio.h
index 11c2f25..4650a2b 100644
--- a/drivers/staging/iio/iio.h
+++ b/drivers/staging/iio/iio.h
@@ -230,7 +230,9 @@ struct iio_dev;
  *			Meaning is event dependent.
  * @validate_trigger:	function to validate the trigger when the
  *			current trigger gets changed.
- **/
+ * @			the parent device (actual hardware).  Note that if
+ *			not specified then iio_dev.dev->parent is used.
+ */
 struct iio_info {
 	struct module			*driver_module;
 	struct attribute_group		*event_attrs;
@@ -267,8 +269,10 @@ struct iio_info {
 				 int val);
 	int (*validate_trigger)(struct iio_dev *indio_dev,
 				struct iio_trigger *trig);
+
 	int (*update_scan_mode)(struct iio_dev *indio_dev,
 				const unsigned long *scan_mask);
+	struct device *(*get_hardware_id)(struct iio_dev *indio_dev);
 };
 
 /**
diff --git a/drivers/staging/iio/industrialio-core.c b/drivers/staging/iio/industrialio-core.c
index dbd1ce1..c108fc9 100644
--- a/drivers/staging/iio/industrialio-core.c
+++ b/drivers/staging/iio/industrialio-core.c
@@ -22,11 +22,13 @@
 #include <linux/cdev.h>
 #include <linux/slab.h>
 #include <linux/anon_inodes.h>
+#include <linux/err.h>
 #include "iio.h"
 #include "iio_core.h"
 #include "iio_core_trigger.h"
 #include "sysfs.h"
 #include "events.h"
+#include "inkern.h"
 
 /* IDA to assign each registered device a unique id*/
 static DEFINE_IDA(iio_ida);
@@ -89,6 +91,267 @@ static const char * const iio_chan_info_postfix[] = {
 	= "filter_low_pass_3db_frequency",
 };
 
+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;
+}
+
+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_st_channel_get(const struct device *dev,
+				       const char *name,
+				       const char *channel_name)
+{
+	struct iio_map *c_i = NULL, *c = NULL;
+	struct device *dev_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);
+
+	channel = kmalloc(sizeof(*channel), GFP_KERNEL);
+	if (channel == NULL)
+		return ERR_PTR(-ENOMEM);
+
+	channel->indio_dev = container_of(dev_i, struct iio_dev, dev);
+
+	if (c->adc_channel_label)
+		channel->channel =
+			iio_chan_spec_from_name(channel->indio_dev,
+						c->adc_channel_label);
+	if (channel->channel == NULL)
+		channel->channel = &channel->indio_dev->
+			channels[c->channel_number];
+
+	return channel;
+}
+EXPORT_SYMBOL_GPL(iio_st_channel_get);
+
+void iio_st_channel_release(struct iio_channel *channel)
+{
+	put_device(&channel->indio_dev->dev);
+	kfree(channel);
+}
+EXPORT_SYMBOL_GPL(iio_st_channel_release);
+
+struct iio_channel **iio_st_channel_get_all(const struct device *dev,
+					    const char *name)
+{
+	struct iio_channel **chans;
+	struct iio_map *c = NULL;
+	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;
+	}
+
+	/* NULL terminated array to save passing size */
+	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;
+
+			chans[mapind]->indio_dev =
+				container_of(dev_i, struct iio_dev, dev);
+			chans[mapind]->channel =
+				iio_chan_spec_from_name(chans[mapind]->
+							indio_dev,
+							c->adc_channel_label);
+			if (chans[mapind]->channel == NULL) {
+				ret = -EINVAL;
+				put_device(&chans[mapind]->indio_dev->dev);
+				goto error_free_chans;
+			}
+			mapind++;
+		}
+	}
+	if (mapind == 0) {
+		ret = -ENODEV;
+		goto error_free_chans;
+	}
+	return chans;
+
+error_free_chans:
+	for (i = 0; i < nummaps; i++)
+		if (chans[i]) {
+			if (chans[i]->indio_dev)
+				put_device(&chans[i]->indio_dev->dev);
+			kfree(chans[i]);
+		}
+	kfree(chans);
+error_ret:
+
+	return ERR_PTR(ret);
+}
+EXPORT_SYMBOL_GPL(iio_st_channel_get_all);
+
+void iio_st_channel_release_all(struct iio_channel **channels)
+{
+	int i = 0;
+	struct iio_channel *chan = channels[i];
+
+	while (chan) {
+		if (chan->indio_dev)
+			put_device(&chan->indio_dev->dev);
+		kfree(chan);
+		i++;
+		chan = channels[i];
+	}
+	kfree(channels);
+}
+EXPORT_SYMBOL_GPL(iio_st_channel_release_all);
+
+int iio_st_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_st_read_channel_raw);
+
+int iio_st_read_channel_scale(struct iio_channel *chan, int *val, int *val2)
+{
+	return chan->indio_dev->info->read_raw(chan->indio_dev,
+					       chan->channel,
+					       val, val2,
+					       IIO_CHAN_INFO_SCALE);
+}
+EXPORT_SYMBOL_GPL(iio_st_read_channel_scale);
+
+enum iio_chan_type iio_st_get_channel_type(struct iio_channel *channel)
+{
+	return channel->channel->type;
+}
+EXPORT_SYMBOL_GPL(iio_st_get_channel_type);
+
 const struct iio_chan_spec
 *iio_find_channel_from_si(struct iio_dev *indio_dev, int si)
 {
@@ -1026,11 +1289,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_allocate_device(int sizeof_priv)
 {
 	struct iio_dev *dev;
diff --git a/drivers/staging/iio/inkern.c b/drivers/staging/iio/inkern.c
new file mode 100644
index 0000000..3e86093
--- /dev/null
+++ b/drivers/staging/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 "inkern.h"
+#include <linux/err.h>
+#include <linux/export.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/drivers/staging/iio/inkern.h b/drivers/staging/iio/inkern.h
new file mode 100644
index 0000000..fc32896
--- /dev/null
+++ b/drivers/staging/iio/inkern.h
@@ -0,0 +1,86 @@
+#include <linux/device.h>
+#include <linux/list.h>
+#include "types.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_st_channel_get(const struct device *dev,
+				    const char *name,
+				    const char *consumer_channel);
+void iio_st_channel_release(struct iio_channel *chan);
+
+/**
+ * iio_st_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_st_channel_get_all(const struct device *dev,
+					const char *name);
+
+void iio_st_channel_release_all(struct iio_channel **chan);
+
+/**
+ * iio_st_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_st_read_channel_raw(struct iio_channel *chan,
+			    int *val);
+
+/**
+ * iio_st_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_st_get_channel_type(struct iio_channel *channel);
+
+/**
+ * iio_st_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_st_read_channel_scale(struct iio_channel *chan, int *val,
+			      int *val2);
+
+#endif
-- 
1.7.7.4

Re: [PATCH 3/5] staging:iio:core add in kernel interface mapping and getting IIO channels.

[Greg KH]

On Mon, Dec 05, 2011 at 09:56:02PM +0000, Jonathan Cameron wrote:
> From: Jonathan Cameron <jic23@cam.ac.uk>
> 
> Lifted from proposal for in kernel interface built on the out of staging
> branch.
> 
> Two elements here:
> * Map as defined in "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/staging/Makefile                |    2 +-
>  drivers/staging/iio/Makefile            |    2 +-
>  drivers/staging/iio/iio.h               |    6 +-
>  drivers/staging/iio/industrialio-core.c |  268 ++++++++++++++++++++++++++++++-
>  drivers/staging/iio/inkern.c            |   21 +++
>  drivers/staging/iio/inkern.h            |   86 ++++++++++
>  6 files changed, 377 insertions(+), 8 deletions(-)
> 
> diff --git a/drivers/staging/Makefile b/drivers/staging/Makefile
> index 6e615b6..f59ab89 100644
> --- a/drivers/staging/Makefile
> +++ b/drivers/staging/Makefile
> @@ -34,7 +34,7 @@ obj-$(CONFIG_VT6656)		+= vt6656/
>  obj-$(CONFIG_HYPERV)		+= hv/
>  obj-$(CONFIG_VME_BUS)		+= vme/
>  obj-$(CONFIG_DX_SEP)            += sep/
> -obj-$(CONFIG_IIO)		+= iio/
> +obj-y				+= iio/

Hm, not good.

>  obj-$(CONFIG_ZRAM)		+= zram/
>  obj-$(CONFIG_XVMALLOC)		+= zram/
>  obj-$(CONFIG_ZCACHE)		+= zcache/
> diff --git a/drivers/staging/iio/Makefile b/drivers/staging/iio/Makefile
> index 1340aea..04d6ad2 100644
> --- a/drivers/staging/iio/Makefile
> +++ b/drivers/staging/iio/Makefile
> @@ -1,7 +1,7 @@
>  #
>  # Makefile for the industrial I/O core.
>  #
> -
> +obj-y = inkern.o

Doubly not good.  You just always build this now, even if someone
doesn't want any staging drivers, or iio, right?

I can't accept this, sorry.

> --- /dev/null
> +++ b/drivers/staging/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 "inkern.h"
> +#include <linux/err.h>
> +#include <linux/export.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);

No _GPL here?

If you have a register function, why do you need to export the list
symbol as well?  Shouldn't you have accessor functions for the whole
list?  And what is this list for?

What exactly are you trying to do here with the "inkern.c" file?

> --- /dev/null
> +++ b/drivers/staging/iio/inkern.h
> @@ -0,0 +1,86 @@
> +#include <linux/device.h>
> +#include <linux/list.h>
> +#include "types.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_st_channel_get(const struct device *dev,
> +				    const char *name,
> +				    const char *consumer_channel);
> +void iio_st_channel_release(struct iio_channel *chan);
> +
> +/**
> + * iio_st_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_st_channel_get_all(const struct device *dev,
> +					const char *name);
> +
> +void iio_st_channel_release_all(struct iio_channel **chan);
> +
> +/**
> + * iio_st_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_st_read_channel_raw(struct iio_channel *chan,
> +			    int *val);
> +
> +/**
> + * iio_st_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_st_get_channel_type(struct iio_channel *channel);
> +
> +/**
> + * iio_st_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_st_read_channel_scale(struct iio_channel *chan, int *val,
> +			      int *val2);
> +
> +#endif

You have a bunch of functions and structures here that are not used in
the .c file, which doesn't seem to match up.

Sorry, I can't accept this.

greg k-h

Re: [PATCH 3/5] staging:iio:core add in kernel interface mapping and getting IIO channels.

[Jonathan Cameron]

On 12/08/2011 07:40 PM, Greg KH wrote:
> On Mon, Dec 05, 2011 at 09:56:02PM +0000, Jonathan Cameron wrote:
>> From: Jonathan Cameron <jic23@cam.ac.uk>
>>
>> Lifted from proposal for in kernel interface built on the out of staging
>> branch.
>>
>> Two elements here:
>> * Map as defined in "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/staging/Makefile                |    2 +-
>>  drivers/staging/iio/Makefile            |    2 +-
>>  drivers/staging/iio/iio.h               |    6 +-
>>  drivers/staging/iio/industrialio-core.c |  268 ++++++++++++++++++++++++++++++-
>>  drivers/staging/iio/inkern.c            |   21 +++
>>  drivers/staging/iio/inkern.h            |   86 ++++++++++
>>  6 files changed, 377 insertions(+), 8 deletions(-)
>>
>> diff --git a/drivers/staging/Makefile b/drivers/staging/Makefile
>> index 6e615b6..f59ab89 100644
>> --- a/drivers/staging/Makefile
>> +++ b/drivers/staging/Makefile
>> @@ -34,7 +34,7 @@ obj-$(CONFIG_VT6656)		+= vt6656/
>>  obj-$(CONFIG_HYPERV)		+= hv/
>>  obj-$(CONFIG_VME_BUS)		+= vme/
>>  obj-$(CONFIG_DX_SEP)            += sep/
>> -obj-$(CONFIG_IIO)		+= iio/
>> +obj-y				+= iio/
> 
> Hm, not good.

> 
>>  obj-$(CONFIG_ZRAM)		+= zram/
>>  obj-$(CONFIG_XVMALLOC)		+= zram/
>>  obj-$(CONFIG_ZCACHE)		+= zcache/
>> diff --git a/drivers/staging/iio/Makefile b/drivers/staging/iio/Makefile
>> index 1340aea..04d6ad2 100644
>> --- a/drivers/staging/iio/Makefile
>> +++ b/drivers/staging/iio/Makefile
>> @@ -1,7 +1,7 @@
>>  #
>>  # Makefile for the industrial I/O core.
>>  #
>> -
>> +obj-y = inkern.o
> 
> Doubly not good.  You just always build this now, even if someone
> doesn't want any staging drivers, or iio, right?
Agreed. That one is definitely wrong and needs a selectable symbol.
> 
> I can't accept this, sorry.
> 
>> --- /dev/null
>> +++ b/drivers/staging/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 "inkern.h"
>> +#include <linux/err.h>
>> +#include <linux/export.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);
> 
> No _GPL here?
> 
> If you have a register function, why do you need to export the list
> symbol as well?  Shouldn't you have accessor functions for the whole
> list?  And what is this list for?
> 
> What exactly are you trying to do here with the "inkern.c" file?
> 
>> --- /dev/null
>> +++ b/drivers/staging/iio/inkern.h
>> @@ -0,0 +1,86 @@
>> +#include <linux/device.h>
>> +#include <linux/list.h>
>> +#include "types.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_st_channel_get(const struct device *dev,
>> +				    const char *name,
>> +				    const char *consumer_channel);
>> +void iio_st_channel_release(struct iio_channel *chan);
>> +
>> +/**
>> + * iio_st_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_st_channel_get_all(const struct device *dev,
>> +					const char *name);
>> +
>> +void iio_st_channel_release_all(struct iio_channel **chan);
>> +
>> +/**
>> + * iio_st_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_st_read_channel_raw(struct iio_channel *chan,
>> +			    int *val);
>> +
>> +/**
>> + * iio_st_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_st_get_channel_type(struct iio_channel *channel);
>> +
>> +/**
>> + * iio_st_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_st_read_channel_scale(struct iio_channel *chan, int *val,
>> +			      int *val2);
>> +
>> +#endif
> 
> You have a bunch of functions and structures here that are not used in
> the .c file, which doesn't seem to match up.
Will rework this and get back to you.

Sorry for the waste of your time.  I clearly dropped the ball here.

Re: [PATCH 3/5] staging:iio:core add in kernel interface mapping and getting IIO channels.

[Jonathan Cameron]

...
> 
>> --- /dev/null
>> +++ b/drivers/staging/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 "inkern.h"
>> +#include <linux/err.h>
>> +#include <linux/export.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);
> 
> No _GPL here?
oops
> 
> If you have a register function, why do you need to export the list
> symbol as well?  Shouldn't you have accessor functions for the whole
> list?  And what is this list for?

The issue here is where the access functions reside.

The list registration stuff has to be built in (now optionally!) so that
it can be filled from board files if appropriate.

The list is then used by the iio-core in response to requests from
client drivers that want to get data from iio devices.  Short of
supplying access functions that are effectively the equivalent of
the standard list access functions I can't really see how else to
avoid exporting this.

I'd welcome any suggestions on this!

As for the other issues. I've added another kernel symbol (boolean)
as a top level for iio with the core underneath.  That means that
this bit is no longer always built in.   An additional header and
source file now contain the elements that were defined in inkern.h
and implemented in industrialio-core.c (inkern-consumer.h/.c).
Will repost for review once we've addressed this exported list question!

Jonathan

Re: [PATCH 3/5] staging:iio:core add in kernel interface mapping and getting IIO channels.

[Greg KH]

On Sat, Dec 10, 2011 at 05:08:21PM +0000, Jonathan Cameron wrote:
> ...
> > 
> >> --- /dev/null
> >> +++ b/drivers/staging/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 "inkern.h"
> >> +#include <linux/err.h>
> >> +#include <linux/export.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);
> > 
> > No _GPL here?
> oops
> > 
> > If you have a register function, why do you need to export the list
> > symbol as well?  Shouldn't you have accessor functions for the whole
> > list?  And what is this list for?
> 
> The issue here is where the access functions reside.
> 
> The list registration stuff has to be built in (now optionally!) so that
> it can be filled from board files if appropriate.
> 
> The list is then used by the iio-core in response to requests from
> client drivers that want to get data from iio devices.  Short of
> supplying access functions that are effectively the equivalent of
> the standard list access functions I can't really see how else to
> avoid exporting this.

So why is this separate from the iio-core code?

And how are you guaranteing that anyone walking the list is doing so in
a "safe" manner?  What's to keep an entry from being removed while some
random kernel code is walking it?

Hint, don't allow direct access to this list, if you do so, you are in
for a world of hurt...

> I'd welcome any suggestions on this!
> 
> As for the other issues. I've added another kernel symbol (boolean)
> as a top level for iio with the core underneath.  That means that
> this bit is no longer always built in.   An additional header and
> source file now contain the elements that were defined in inkern.h
> and implemented in industrialio-core.c (inkern-consumer.h/.c).
> Will repost for review once we've addressed this exported list question!

Thanks for changing this.

greg k-h

Re: [PATCH 3/5] staging:iio:core add in kernel interface mapping and getting IIO channels.

[archive]

Greg KH writes: 

> On Sat, Dec 10, 2011 at 05:08:21PM +0000, Jonathan Cameron wrote:
>> ...
>> > 
>> >> --- /dev/null
>> >> +++ b/drivers/staging/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 "inkern.h"
>> >> +#include <linux/err.h>
>> >> +#include <linux/export.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);
>> > 
>> > No _GPL here?
>> oops
>> > 
>> > If you have a register function, why do you need to export the list
>> > symbol as well?  Shouldn't you have accessor functions for the whole
>> > list?  And what is this list for? 
>> 
>> The issue here is where the access functions reside. 
>> 
>> The list registration stuff has to be built in (now optionally!) so that
>> it can be filled from board files if appropriate. 
>> 
>> The list is then used by the iio-core in response to requests from
>> client drivers that want to get data from iio devices.  Short of
>> supplying access functions that are effectively the equivalent of
>> the standard list access functions I can't really see how else to
>> avoid exporting this.
> 
> So why is this separate from the iio-core code? 
> 
> And how are you guaranteing that anyone walking the list is doing so in
> a "safe" manner?  What's to keep an entry from being removed while some
> random kernel code is walking it? 
> 
> Hint, don't allow direct access to this list, if you do so, you are in
> for a world of hurt...
The entire purpose of this list is to allow a small built in chunk of
code to fill it up (typically from board files) whilst the rest of IIO can
still be built as a module.  All but one function that touches this are in
the IIO core modules.  I can wrap this up but only at considerable cost
(the wrappers will basically be the various list functions with a lock taken
 - I'll add one of those).  I can write extreme warnings around it saying
that it strictly for use by the IIO core.  Would that be acceptable?
Clearly this element should stay in the IIO directory once the rest of
the header has gone into include/linux/iio/.  I can do that division now
so the separation is more apparent. 

So in summary two options exist. 

1) Leave as is with a suitable warning and maybe rename to make it appear
less friendly.
2) Wrap it so we end up with accessors that are pretty much the standard
list accessors just with one parameter fixed and trivial locking. This isn't
too painful but seems conceptually wrong to me.
3) Move everything that uses it into the bit that gets built in even if IIO 
is a module. 

This last one will lead to an explosion in what is exported from IIO as we
will still need to have the divide between the bit that can be modular and
that which cannot somewhere! (which is why I didn't suggest it before). 

Conceptually this list is part of the core - I only have it separate to
avoid having to have the whole IIO core built-in if this functionality is
enabled. 

If list wrappers are the only acceptable option then I guess it will only
be a little bit unpleasant...
> 
>> I'd welcome any suggestions on this! 
>> 
>> As for the other issues. I've added another kernel symbol (boolean)
>> as a top level for iio with the core underneath.  That means that
>> this bit is no longer always built in.   An additional header and
>> source file now contain the elements that were defined in inkern.h
>> and implemented in industrialio-core.c (inkern-consumer.h/.c).
>> Will repost for review once we've addressed this exported list question!
> 
> Thanks for changing this. 
> 
> greg k-h

Re: [PATCH 3/5] staging:iio:core add in kernel interface mapping and getting IIO channels.

[Greg KH]

On Fri, Dec 16, 2011 at 08:50:57AM +0000, archive@jic23.retrosnub.co.uk wrote:
> >And how are you guaranteing that anyone walking the list is doing so in
> >a "safe" manner?  What's to keep an entry from being removed while some
> >random kernel code is walking it?
> >
> >Hint, don't allow direct access to this list, if you do so, you are in
> >for a world of hurt...
> The entire purpose of this list is to allow a small built in chunk of
> code to fill it up (typically from board files) whilst the rest of IIO can
> still be built as a module.

Why are you trying to do that?

> All but one function that touches this are in
> the IIO core modules.  I can wrap this up but only at considerable cost
> (the wrappers will basically be the various list functions with a lock taken
> - I'll add one of those).  I can write extreme warnings around it saying
> that it strictly for use by the IIO core.  Would that be acceptable?

I still don't understand the point of the list.  Why is this somehow
different from any other bus in the kernel in that the only way to
"register" a device is to actually register the device.

> Clearly this element should stay in the IIO directory once the rest of
> the header has gone into include/linux/iio/.  I can do that division now
> so the separation is more apparent.
> 
> So in summary two options exist.
> 
> 1) Leave as is with a suitable warning and maybe rename to make it appear
> less friendly.
> 2) Wrap it so we end up with accessors that are pretty much the standard
> list accessors just with one parameter fixed and trivial locking. This isn't
> too painful but seems conceptually wrong to me.
> 3) Move everything that uses it into the bit that gets built in even
> if IIO is a module.

There should never be a "bit that is always built in if IIO is a
module", sorry.

> This last one will lead to an explosion in what is exported from IIO as we
> will still need to have the divide between the bit that can be modular and
> that which cannot somewhere! (which is why I didn't suggest it
> before).

Why are you trying to make such a strange distinction here?  If a device
uses IIO, then you need to have the IIO core.  It can be built as a
module or not, again, just like any other bus in the kernel.

greg k-h

Re: [PATCH 3/5] staging:iio:core add in kernel interface mapping and getting IIO channels.

[Jonathan Cameron]

Hi Greg,

I've cc'd a few more people who have an interest it this support
and have mostly been involved in the discussion for the non
staging patch set.

For those I've added a quick summary.

Greg is unhappy with the fact that the inkernel support in IIO
relies on a small chunck of code that must be built in.
That code just provides a list and a function to fill it. Everything
else is in the IIO core modules.  There were some other issues to
do with which files code was in etc all of which have been resolved.
(Thanks to Greg for looking at this code as clearly there are still
controversial bits in here!)  For reference it's a direct
copy of what was proposed for the out of staging tree).

Greg KH <greg@kroah.com> wrote:

>On Fri, Dec 16, 2011 at 08:50:57AM +0000, archive@jic23.retrosnub.co.uk
>wrote:
>> >And how are you guaranteing that anyone walking the list is doing so
>in
>> >a "safe" manner?  What's to keep an entry from being removed while
>some
>> >random kernel code is walking it?
>> >
>> >Hint, don't allow direct access to this list, if you do so, you are
>in
>> >for a world of hurt...
>> The entire purpose of this list is to allow a small built in chunk of
>> code to fill it up (typically from board files) whilst the rest of
>IIO can
>> still be built as a module.
>
>Why are you trying to do that?
Because I don't see any other remotely elegant way of doing it.
We need a mapping from this output channel on this device to this
input for this device that is using it as a service on a channel
by channel basis with a single output able to be used by many
inputs.  This look up table (which is just a list to allow
the more complex boards to fill it from various places) serves
that purpose.
>
>> All but one function that touches this are in
>> the IIO core modules.  I can wrap this up but only at considerable
>cost
>> (the wrappers will basically be the various list functions with a
>lock taken
>> - I'll add one of those).  I can write extreme warnings around it
>saying
>> that it strictly for use by the IIO core.  Would that be acceptable?
>
>I still don't understand the point of the list.  Why is this somehow
>different from any other bus in the kernel in that the only way to
>"register" a device is to actually register the device.
It is not for registering a device. It is for associations between
devices. Similar to the regulator API. That provides a means to request
a voltage from another device, this provides a means of reading from a
device.

Ultimately it's a convenient way of passing platform data.  We could
pass it into every single supplier device (in a similar way to
regulators) as platform data.  Thus every iio device would have to
handle platform data and pass it on to the core, or, as here, we can
separate it out.
>
>> Clearly this element should stay in the IIO directory once the rest
>of
>> the header has gone into include/linux/iio/.  I can do that division
>now
>> so the separation is more apparent.
>> 
>> So in summary two options exist.
>> 
>> 1) Leave as is with a suitable warning and maybe rename to make it
>appear
>> less friendly.
>> 2) Wrap it so we end up with accessors that are pretty much the
>standard
>> list accessors just with one parameter fixed and trivial locking.
>This isn't
>> too painful but seems conceptually wrong to me.
>> 3) Move everything that uses it into the bit that gets built in even
>> if IIO is a module.
>
>There should never be a "bit that is always built in if IIO is a
>module", sorry.
The only alternative is to make ever single IIO driver take platform
data to specify it's consumers and immediately pass it into the core.
It is only the core that cares.
>
>> This last one will lead to an explosion in what is exported from IIO
>as we
>> will still need to have the divide between the bit that can be
>modular and
>> that which cannot somewhere! (which is why I didn't suggest it
>> before).
>
>Why are you trying to make such a strange distinction here?  If a
>device
>uses IIO, then you need to have the IIO core.
When that driver is loaded, not when the board file sets up it's
association with IIO.  It will not be dependent on the driver
supplying the channel anyway so we will need the backoff and
try again stuff to make this work.

Why have I done it like this?

Because this way we get a fairly clean addition to the subsystem
without having to rewrite every single driver's probe routine
which is the only other way making the data available that I can
see.

> It can be built as a
>module or not, again, just like any other bus in the kernel.
Bus maybe, but other subsystems providing a 'service' like we
have here for iio such as regulators, pinmux etc cannot be built
as modules.  There the arguement was they were core to the
board; that is not typically true of IIO.

Note that if this feels like a bolt on to IIO that is because
inkernel support is exactly that.  We are happy to have it
only if enhances what IIO already offers.

Jonathan

Re: [PATCH 3/5] staging:iio:core add in kernel interface mapping and getting IIO channels.

[Greg KH]

On Fri, Dec 16, 2011 at 07:41:12PM +0000, Jonathan Cameron wrote:
> >> >for a world of hurt...
> >> The entire purpose of this list is to allow a small built in chunk of
> >> code to fill it up (typically from board files) whilst the rest of
> >IIO can
> >> still be built as a module.
> >
> >Why are you trying to do that?
> Because I don't see any other remotely elegant way of doing it.
> We need a mapping from this output channel on this device to this
> input for this device that is using it as a service on a channel
> by channel basis with a single output able to be used by many
> inputs.

Who needs that mapping?

> This look up table (which is just a list to allow the more complex
> boards to fill it from various places) serves that purpose.

And how is that shown by just a "bare" list being exported?  If you need
to provide such a mapping, then by all means do so, but do so in a
documented way with the proper locking for any data structures (which
again, was the most obvious bug you had with this patch.)

> >> All but one function that touches this are in
> >> the IIO core modules.  I can wrap this up but only at considerable
> >cost
> >> (the wrappers will basically be the various list functions with a
> >lock taken
> >> - I'll add one of those).  I can write extreme warnings around it
> >saying
> >> that it strictly for use by the IIO core.  Would that be acceptable?
> >
> >I still don't understand the point of the list.  Why is this somehow
> >different from any other bus in the kernel in that the only way to
> >"register" a device is to actually register the device.
> It is not for registering a device. It is for associations between
> devices. Similar to the regulator API. That provides a means to request
> a voltage from another device, this provides a means of reading from a
> device.

Ok, then show how that works somehow?  Again, a list doesn't show that
to me.

> Ultimately it's a convenient way of passing platform data.  We could
> pass it into every single supplier device (in a similar way to
> regulators) as platform data.  Thus every iio device would have to
> handle platform data and pass it on to the core, or, as here, we can
> separate it out.

Having a function for all of the drivers to call to provide this
information seems like the most obvious way to do this in a proper way,
why not do that?

> >> Clearly this element should stay in the IIO directory once the rest
> >of
> >> the header has gone into include/linux/iio/.  I can do that division
> >now
> >> so the separation is more apparent.
> >> 
> >> So in summary two options exist.
> >> 
> >> 1) Leave as is with a suitable warning and maybe rename to make it
> >appear
> >> less friendly.
> >> 2) Wrap it so we end up with accessors that are pretty much the
> >standard
> >> list accessors just with one parameter fixed and trivial locking.
> >This isn't
> >> too painful but seems conceptually wrong to me.
> >> 3) Move everything that uses it into the bit that gets built in even
> >> if IIO is a module.
> >
> >There should never be a "bit that is always built in if IIO is a
> >module", sorry.
> The only alternative is to make ever single IIO driver take platform
> data to specify it's consumers and immediately pass it into the core.
> It is only the core that cares.

What's wrong with a function call to the IIO core by the drivers?

> >> This last one will lead to an explosion in what is exported from IIO
> >as we
> >> will still need to have the divide between the bit that can be
> >modular and
> >> that which cannot somewhere! (which is why I didn't suggest it
> >> before).
> >
> >Why are you trying to make such a strange distinction here?  If a
> >device
> >uses IIO, then you need to have the IIO core.
> When that driver is loaded, not when the board file sets up it's
> association with IIO.  It will not be dependent on the driver
> supplying the channel anyway so we will need the backoff and
> try again stuff to make this work.
> 
> Why have I done it like this?
> 
> Because this way we get a fairly clean addition to the subsystem
> without having to rewrite every single driver's probe routine
> which is the only other way making the data available that I can
> see.

Rewriting all probe functions really shouldn't be hard as you have to
just change from manipulating this list by-hand, to making a function
call.  That sounds like an easy thing to do, and something that should
be done, right?

> > It can be built as a
> >module or not, again, just like any other bus in the kernel.
> Bus maybe, but other subsystems providing a 'service' like we
> have here for iio such as regulators, pinmux etc cannot be built
> as modules.  There the arguement was they were core to the
> board; that is not typically true of IIO.

Then keep the IIO core from being built as a module if that is the
requirement here.

Or, just be sure to initialize it early enough in the boot process that
it happens before the drivers are, and then, it can be a module, as long
as any driver that uses it is also a module, right?

Again, just like all other busses :)

thanks,

greg k-h

Re: [PATCH 3/5] staging:iio:core add in kernel interface mapping and getting IIO channels.

[Jonathan Cameron]

On 12/16/2011 10:23 PM, Greg KH wrote:
> On Fri, Dec 16, 2011 at 07:41:12PM +0000, Jonathan Cameron wrote:
>>>>> for a world of hurt...
>>>> The entire purpose of this list is to allow a small built in chunk of
>>>> code to fill it up (typically from board files) whilst the rest of
>>> IIO can
>>>> still be built as a module.
>>>
>>> Why are you trying to do that?
>> Because I don't see any other remotely elegant way of doing it.
>> We need a mapping from this output channel on this device to this
>> input for this device that is using it as a service on a channel
>> by channel basis with a single output able to be used by many
>> inputs.
> 
> Who needs that mapping?
Any driver that needs to read/write (via a dac rather than
a regulator - and yes there is clearly a blurred line there) a voltage
(or other channel type) and doesn't have the support on chip.  Currently
we have hwmon and input bridge drivers for iio
supported parts.  Some channels are mapped to hwmon and some to input
on the same underlying adc.  The main interest is from the SoC side
of thing where this sort of sharing is common.  That's why Mark, Linus
are Arnd are cc'd on this. I've also added Maxime given the at91 adc
driver is a clasic example.
> 
>> This look up table (which is just a list to allow the more complex
>> boards to fill it from various places) serves that purpose.
> 
> And how is that shown by just a "bare" list being exported?
A bare is list only exported purely to allow to core parts of
IIO to comunicate.  Nothing else should ever touch that list.
Yes it should have had some locking (I fully except that).
It was a means of allowing IIO to remain buildable as a module.
To my mind that is a good thing, you evidently disagree.
Some input from the SoC guys here would be good.
  If you need
> to provide such a mapping, then by all means do so, but do so in a
> documented way with the proper locking for any data structures (which
> again, was the most obvious bug you had with this patch.)
I accept the locking and documentation points and will add those.
> 
>>>> All but one function that touches this are in
>>>> the IIO core modules.  I can wrap this up but only at considerable
>>> cost
>>>> (the wrappers will basically be the various list functions with a
>>> lock taken
>>>> - I'll add one of those).  I can write extreme warnings around it
>>> saying
>>>> that it strictly for use by the IIO core.  Would that be acceptable?
>>>
>>> I still don't understand the point of the list.  Why is this somehow
>>> different from any other bus in the kernel in that the only way to
>>> "register" a device is to actually register the device.
>> It is not for registering a device. It is for associations between
>> devices. Similar to the regulator API. That provides a means to request
>> a voltage from another device, this provides a means of reading from a
>> device.
> 
> Ok, then show how that works somehow?  Again, a list doesn't show that
> to me.
Read the functions using the list and you'll have your answer.  You are
focusing on the list when that really isn't the interface at all.
I will happily write extremely dire warnings around that list if that
helps.
> 
>> Ultimately it's a convenient way of passing platform data.  We could
>> pass it into every single supplier device (in a similar way to
>> regulators) as platform data.  Thus every iio device would have to
>> handle platform data and pass it on to the core, or, as here, we can
>> separate it out.
> 
> Having a function for all of the drivers to call to provide this
> information seems like the most obvious way to do this in a proper way,
> why not do that?
Because that means replacing the current platform data of ever single
driver. We can do this, but its messy and painful and breaks every
current user (yes they are out of tree given we are in staging and hence
can't have platform dat in tree - still I dislike pissing our users
off.)  The driver it self has no interest whatsoever in this data.

> 
>>>> Clearly this element should stay in the IIO directory once the rest
>>> of
>>>> the header has gone into include/linux/iio/.  I can do that division
>>> now
>>>> so the separation is more apparent.
>>>>
>>>> So in summary two options exist.
>>>>
>>>> 1) Leave as is with a suitable warning and maybe rename to make it
>>> appear
>>>> less friendly.
>>>> 2) Wrap it so we end up with accessors that are pretty much the
>>> standard
>>>> list accessors just with one parameter fixed and trivial locking.
>>> This isn't
>>>> too painful but seems conceptually wrong to me.
>>>> 3) Move everything that uses it into the bit that gets built in even
>>>> if IIO is a module.
>>>
>>> There should never be a "bit that is always built in if IIO is a
>>> module", sorry.
>> The only alternative is to make ever single IIO driver take platform
>> data to specify it's consumers and immediately pass it into the core.
>> It is only the core that cares.
> 
> What's wrong with a function call to the IIO core by the drivers?
Why pass data through them that they don't care about and won't touch?
> 
>>>> This last one will lead to an explosion in what is exported from IIO
>>> as we
>>>> will still need to have the divide between the bit that can be
>>> modular and
>>>> that which cannot somewhere! (which is why I didn't suggest it
>>>> before).
>>>
>>> Why are you trying to make such a strange distinction here?  If a
>>> device
>>> uses IIO, then you need to have the IIO core.
>> When that driver is loaded, not when the board file sets up it's
>> association with IIO.  It will not be dependent on the driver
>> supplying the channel anyway so we will need the backoff and
>> try again stuff to make this work.
>>
>> Why have I done it like this?
>>
>> Because this way we get a fairly clean addition to the subsystem
>> without having to rewrite every single driver's probe routine
>> which is the only other way making the data available that I can
>> see.
> 
> Rewriting all probe functions really shouldn't be hard as you have to
> just change from manipulating this list by-hand, to making a function
> call.  That sounds like an easy thing to do, and something that should
> be done, right?
That plus add a level of indirection to every driver that currently
has any platform data (which covers the vast majority).  Either
the array of consumers has to be added to their existing pd which is
clunky and driver specific or we have another structure with optional
consumer data and another level of platform data to pass in what the
driver actually cares about.  Note throughout that we are passing data
that is completely irrelevant to the driver.  We are doing all this
to support a usecase that isn't even relevant to many IIO drivers.

> 
>>> It can be built as a
>>> module or not, again, just like any other bus in the kernel.
>> Bus maybe, but other subsystems providing a 'service' like we
>> have here for iio such as regulators, pinmux etc cannot be built
>> as modules.  There the arguement was they were core to the
>> board; that is not typically true of IIO.
> 
> Then keep the IIO core from being built as a module if that is the
> requirement here.
A painful cost just to avoid having this one list (with locking added).
Not huge, but I really don't understand why this trivial list export
such a problem. (though there were clearly elements that could be
improved around it).
Note in the multiboard kernels that people are working for on for arm
etc we would just have added this to all configurations if only one
actually needs it.
> 
> Or, just be sure to initialize it early enough in the boot process that
> it happens before the drivers are, and then, it can be a module, as long
> as any driver that uses it is also a module, right?
There is a whole back off and try again infrastructure for module
loading.  Why not use that? (once/if it merges).  There is no
reason to get into the whole game of messing around with load
orders.
> 
> Again, just like all other busses :)
> 
> thanks,
Glad this came up before I tried to push the non staging version
as clearly there are questions to be addressed.

Jonathan

Re: [PATCH 3/5] staging:iio:core add in kernel interface mapping and getting IIO channels.

[Mark Brown]

On Sat, Dec 17, 2011 at 03:54:17PM +0000, Jonathan Cameron wrote:
> On 12/16/2011 10:23 PM, Greg KH wrote:

> > Rewriting all probe functions really shouldn't be hard as you have to
> > just change from manipulating this list by-hand, to making a function
> > call.  That sounds like an easy thing to do, and something that should
> > be done, right?

> That plus add a level of indirection to every driver that currently
> has any platform data (which covers the vast majority).  Either
> the array of consumers has to be added to their existing pd which is
> clunky and driver specific or we have another structure with optional
> consumer data and another level of platform data to pass in what the
> driver actually cares about.  Note throughout that we are passing data
> that is completely irrelevant to the driver.  We are doing all this
> to support a usecase that isn't even relevant to many IIO drivers.

FWIW the generic platform data embed thing is what the regulator API is
currently doing, though of course we've got a bunch more data going in
there in the constraints.  This is actually a bit limiting as it means
we need to know how things are glued together at registeration time,
it'd be nice if we were able to add links in separately.