[PATCH RFC] IIO: Proof of concept in kernel interface.

[PATCH RFC] IIO: Proof of concept in kernel interface.

[Jonathan Cameron]

Hi All,

Given I keep blithely stating it would be easy to add an in kernel
interface to IIO, I thought I'd better actually prove it.

There are numerous holes / questions raised by this example.

1) We need a foolproof way of getting the right instance of
a given device.  This PoC does it by part name (unique on my
test board but far from it in general).
The snag as ever is the complexity of getting a reliable reference
to an i2c or spi part.  Basically it's the same problem fudged
over in the regulator framework.

2) Callbacks for triggered read.  Not done here and given the flexibility
of IIO triggers it's not obvious how to do it.  Most other subsystems
pretty much assume they will get a stream of data and do not control
what causes it to appear.  With IIO I we'll either need in kernel control
of this (fiddly as it really isn't that generalizable), assume
sane default settings or have a userspace daemon to set it up.

3) How much hand holding to do.  Right now it gives raw access to the device,
equivalent to what the IIO core has.  Is it worth doing error checking utility
functions such as

int iio_consumer_read_data(struct iio_dev *indio_dev, int channel, enum iio_type type);
which would do stuff like verify the channel exists and that it
is the right sort of channel. Can also use this to 'hide' the
struct iio_dev etc from the consumer drivers.

If people are happy with the general form and we nail the
association problem, I'll write a general hwmon interface
to do simple hardware monitoring from an adc channel.
For now I'd keep it completely trivial and avoid events etc.

So comments welcome,

This sits on a couple of patches that haven't gone upstream yet, so
best bet is probably

https://github.com/jic23/linux-iio/tree/inkern

which also has the dummy driver for anyone who wants to play
and doesn't have hardware.


Jonathan Cameron (1):
  staging:iio:proof of concept in kernel interface.

 drivers/staging/iio/Kconfig             |    5 ++++
 drivers/staging/iio/Makefile            |    3 ++
 drivers/staging/iio/iio.h               |    5 ++++
 drivers/staging/iio/industrialio-core.c |   38 ++++++++++++++++++++++++++++
 drivers/staging/iio/inkern.c            |   41 +++++++++++++++++++++++++++++++
 5 files changed, 92 insertions(+), 0 deletions(-)
 create mode 100644 drivers/staging/iio/inkern.c

-- 
1.7.3.4

[PATCH] staging:iio:proof of concept in kernel interface.

[Jonathan Cameron]

Signed-off-by: Jonathan Cameron <jic23@cam.ac.uk>
---
 drivers/staging/iio/Kconfig             |    5 ++++
 drivers/staging/iio/Makefile            |    3 ++
 drivers/staging/iio/iio.h               |    5 ++++
 drivers/staging/iio/industrialio-core.c |   38 ++++++++++++++++++++++++++++
 drivers/staging/iio/inkern.c            |   41 +++++++++++++++++++++++++++++++
 5 files changed, 92 insertions(+), 0 deletions(-)

diff --git a/drivers/staging/iio/Kconfig b/drivers/staging/iio/Kconfig
index 09cf580..b613379 100644
--- a/drivers/staging/iio/Kconfig
+++ b/drivers/staging/iio/Kconfig
@@ -70,6 +70,11 @@ source "drivers/staging/iio/meter/Kconfig"
 source "drivers/staging/iio/resolver/Kconfig"
 source "drivers/staging/iio/trigger/Kconfig"
 
+config IIO_INKERN_TEST
+       tristate "In kernel interface test module"
+       help
+	 A dumb test of the in kernel interfaces
+
 config IIO_DUMMY_EVGEN
        tristate
 
diff --git a/drivers/staging/iio/Makefile b/drivers/staging/iio/Makefile
index eaa07b0..53aff59 100644
--- a/drivers/staging/iio/Makefile
+++ b/drivers/staging/iio/Makefile
@@ -17,6 +17,9 @@ iio_dummy-$(CONFIG_IIO_SIMPLE_DUMMY_BUFFER) += iio_simple_dummy_buffer.o
 
 obj-$(CONFIG_IIO_DUMMY_EVGEN) += iio_dummy_evgen.o
 
+obj-$(CONFIG_IIO_INKERN_TEST) += iio_inkern.o
+iio_inkern-y := inkern.o
+
 obj-y += accel/
 obj-y += adc/
 obj-y += addac/
diff --git a/drivers/staging/iio/iio.h b/drivers/staging/iio/iio.h
index 1eedf2b..2c02e01 100644
--- a/drivers/staging/iio/iio.h
+++ b/drivers/staging/iio/iio.h
@@ -321,6 +321,7 @@ struct iio_dev {
 #define IIO_MAX_GROUPS 6
 	const struct attribute_group	*groups[IIO_MAX_GROUPS + 1];
 	int				groupcounter;
+	struct list_head		dev_list_entry;
 };
 
 /**
@@ -390,4 +391,8 @@ static inline bool iio_buffer_enabled(struct iio_dev *indio_dev)
 		& (INDIO_BUFFER_TRIGGERED | INDIO_BUFFER_HARDWARE);
 };
 
+struct iio_dev *iio_find_dev(const char *name);
+void iio_release_dev(struct iio_dev *indio_dev);
+
+
 #endif /* _INDUSTRIAL_IO_H_ */
diff --git a/drivers/staging/iio/industrialio-core.c b/drivers/staging/iio/industrialio-core.c
index 0589891..565e2c9 100644
--- a/drivers/staging/iio/industrialio-core.c
+++ b/drivers/staging/iio/industrialio-core.c
@@ -22,12 +22,15 @@
 #include <linux/cdev.h>
 #include <linux/slab.h>
 #include <linux/anon_inodes.h>
+#include <linux/list.h>
 #include "iio.h"
 #include "iio_core.h"
 #include "iio_core_trigger.h"
 #include "chrdev.h"
 #include "sysfs.h"
 
+static DEFINE_MUTEX(iio_device_list_lock);
+static LIST_HEAD(iio_device_list);
 /* IDA to assign each registered device a unique id*/
 static DEFINE_IDA(iio_ida);
 
@@ -90,6 +93,31 @@ static const char * const iio_chan_info_postfix[] = {
 	= "filter_low_pass_3db_frequency",
 };
 
+
+/* hacked together in kernel querying  proof of concept */
+struct iio_dev *iio_find_dev(const char *name)
+{
+	struct iio_dev *indio_dev;
+	printk("find dev called\n");
+	mutex_lock(&iio_device_list_lock);
+	list_for_each_entry(indio_dev, &iio_device_list, dev_list_entry)
+		if (strcmp(indio_dev->name, name) == 0) {
+			mutex_unlock(&iio_device_list_lock);
+			__module_get(indio_dev->info->driver_module);
+			return indio_dev;
+		}
+	mutex_unlock(&iio_device_list_lock);
+	return ERR_PTR(ENODEV);
+}
+EXPORT_SYMBOL(iio_find_dev);
+
+void iio_release_dev(struct iio_dev *indio_dev)
+{
+	module_put(indio_dev->info->driver_module);
+}
+EXPORT_SYMBOL(iio_release_dev);
+
+
 /**
  * struct iio_detected_event_list - list element for events that have occurred
  * @list:		linked list header
@@ -998,6 +1026,7 @@ static void iio_device_unregister_eventset(struct iio_dev *indio_dev)
 static void iio_dev_release(struct device *device)
 {
 	struct iio_dev *indio_dev = container_of(device, struct iio_dev, dev);
+
 	cdev_del(&indio_dev->chrdev);
 	if (indio_dev->modes & INDIO_BUFFER_TRIGGERED)
 		iio_device_unregister_trigger_consumer(indio_dev);
@@ -1128,6 +1157,7 @@ int iio_device_register(struct iio_dev *indio_dev)
 	if (indio_dev->modes & INDIO_BUFFER_TRIGGERED)
 		iio_device_register_trigger_consumer(indio_dev);
 
+
 	ret = device_add(&indio_dev->dev);
 	if (ret < 0)
 		goto error_unreg_eventset;
@@ -1136,6 +1166,10 @@ int iio_device_register(struct iio_dev *indio_dev)
 	ret = cdev_add(&indio_dev->chrdev, indio_dev->dev.devt, 1);
 	if (ret < 0)
 		goto error_del_device;
+
+	mutex_lock(&iio_device_list_lock);
+	list_add(&indio_dev->dev_list_entry, &iio_device_list);
+	mutex_unlock(&iio_device_list_lock);
 	return 0;
 
 error_del_device:
@@ -1151,6 +1185,10 @@ EXPORT_SYMBOL(iio_device_register);
 
 void iio_device_unregister(struct iio_dev *indio_dev)
 {
+	mutex_lock(&iio_device_list_lock);
+	list_del(&indio_dev->dev_list_entry);
+	mutex_unlock(&iio_device_list_lock);
+
 	device_unregister(&indio_dev->dev);
 }
 EXPORT_SYMBOL(iio_device_unregister);
diff --git a/drivers/staging/iio/inkern.c b/drivers/staging/iio/inkern.c
new file mode 100644
index 0000000..39c5129
--- /dev/null
+++ b/drivers/staging/iio/inkern.c
@@ -0,0 +1,41 @@
+#include <linux/kernel.h>
+#include <linux/slab.h>
+#include <linux/module.h>
+#include <linux/moduleparam.h>
+#include <linux/err.h>
+
+#include "iio.h"
+#include "sysfs.h"
+#include "buffer_generic.h"
+#include "iio_simple_dummy.h"
+
+struct iio_dev *indio_dev;
+static int iio_inkern_init(void)
+{
+	int ret;
+	int val, val2;
+	indio_dev = iio_find_dev("max1363");
+	if (IS_ERR(indio_dev))
+		return PTR_ERR(indio_dev);
+	/* read from channel 1 and exit */
+	ret = indio_dev->info->read_raw(indio_dev, &indio_dev->channels[0], &val, &val2, 0);
+	if (ret < 0)
+		return ret;
+	printk("%d\n", val);
+
+	return 0;
+}
+module_init(iio_inkern_init);
+
+
+static void iio_inkern_exit(void)
+{
+	iio_release_dev(indio_dev);
+}
+module_exit(iio_inkern_exit);
+
+MODULE_AUTHOR("Jonathan Cameron <jic23@cam.ac.uk>");
+MODULE_DESCRIPTION("IIO inkern interface test driver");
+MODULE_LICENSE("GPL v2");
+
+	    
-- 
1.7.3.4

Re: [PATCH] staging:iio:proof of concept in kernel interface.

[Mark Brown]

On Tue, Oct 11, 2011 at 12:43:03PM +0100, Jonathan Cameron wrote:

> +struct iio_dev *iio_find_dev(const char *name);
> +void iio_release_dev(struct iio_dev *indio_dev);

This looks like it should really be clk style with a struct device and a
back end interface that deals with mapping the provider to consumers at
runtime.

> +	/* read from channel 1 and exit */
> +	ret = indio_dev->info->read_raw(indio_dev, &indio_dev->channels[0], &val, &val2, 0);

I guess an actual implementation would have wrappers for doing the
indirections rather than having users peer into the ops table directly?

Re: [PATCH] staging:iio:proof of concept in kernel interface.

[Jonathan Cameron]

On 10/13/11 15:32, Mark Brown wrote:
> On Tue, Oct 11, 2011 at 12:43:03PM +0100, Jonathan Cameron wrote:
> 
>> +struct iio_dev *iio_find_dev(const char *name);
>> +void iio_release_dev(struct iio_dev *indio_dev);
> 
> This looks like it should really be clk style with a struct device and a
> back end interface that deals with mapping the provider to consumers at
> runtime.
Agreed.  That definitely looks like the best discovery interface
out there at the moment.  Will reroll with the equivalent.

We'll have to play a few games in the drivers to map back to the underlying
bus names so that we have consistent naming.  (simple callback).
> 
>> +	/* read from channel 1 and exit */
>> +	ret = indio_dev->info->read_raw(indio_dev, &indio_dev->channels[0], &val, &val2, 0);
> 
> I guess an actual implementation would have wrappers for doing the
> indirections rather than having users peer into the ops table directly?
> 
Yup, for some reason the cover letter seems to have detached from this.
It suggested exactly that.  There may be weird cases where peering this
deep into the ops makes sense, but not for something like this one.

Re: [PATCH] staging:iio:proof of concept in kernel interface.

[Mark Brown]

On Thu, Oct 13, 2011 at 03:46:04PM +0100, Jonathan Cameron wrote:

> > I guess an actual implementation would have wrappers for doing the
> > indirections rather than having users peer into the ops table directly?

> Yup, for some reason the cover letter seems to have detached from this.
> It suggested exactly that.  There may be weird cases where peering this
> deep into the ops makes sense, but not for something like this one.

Oh, right.  As a general rule I don't read cover letters for single
patches until after I've read the patch, generally they're either
completely content free (if only by virtue of repeating the changelog)
or there's a problem with the changelog in the actual patch not
explaining what's going on.

Re: [PATCH] staging:iio:proof of concept in kernel interface.

[Jonathan Cameron]

On 10/13/11 21:44, Mark Brown wrote:
> On Thu, Oct 13, 2011 at 03:46:04PM +0100, Jonathan Cameron wrote:
> 
>>> I guess an actual implementation would have wrappers for doing the
>>> indirections rather than having users peer into the ops table directly?
> 
>> Yup, for some reason the cover letter seems to have detached from this.
>> It suggested exactly that.  There may be weird cases where peering this
>> deep into the ops makes sense, but not for something like this one.
> 
> Oh, right.  As a general rule I don't read cover letters for single
> patches until after I've read the patch, generally they're either
> completely content free (if only by virtue of repeating the changelog)
> or there's a problem with the changelog in the actual patch not
> explaining what's going on.
Fair enough.

I'm trying to work out what our equivalent of the clk finding api is.

The best match pair to match on I can come up with is:

part name: iio_dev.name

dev_name of underlying hardware if specified.
dev_name(iio_dev->dev.parent)
This matching source can be overridden by an optional callback if we
unique matching is achievable in some other way for the device.

Typical pairs:

max1363, 0-0035
max1238, 0-0034
lis3l02dq spi1.0
adis16400 spi2.1

On soc ADCs can use any combination of the two that makes local
sense.

Does this look sufficient for description / identification?

Precedence order of both, then column 2 (lets call that id) and finally column 1
(part name).

The concept of connections doesn't make sense quite like it does for clks as
we are getting a reference to the whole device, then picking which bits we want
afterwards.


 

Re: [PATCH] staging:iio:proof of concept in kernel interface.

[Mark Brown]

On Fri, Oct 14, 2011 at 04:59:28PM +0100, Jonathan Cameron wrote:

> I'm trying to work out what our equivalent of the clk finding api is.

> The best match pair to match on I can come up with is:

> part name: iio_dev.name

> dev_name of underlying hardware if specified.
> dev_name(iio_dev->dev.parent)
> This matching source can be overridden by an optional callback if we
> unique matching is achievable in some other way for the device.

For the existing APIs doing this the device used for the request is the
client device, not the providing device.  The APIs then have a mapping
table (clkdev for most of the clk APIs) which means that boards can then
assign providers to users without needing either of the drivers to know
what's going on, though of course they can be coded that way.

Re: [PATCH] staging:iio:proof of concept in kernel interface.

[Linus Walleij]

2011/10/14 Jonathan Cameron <jic23@cam.ac.uk>:

> I'm trying to work out what our equivalent of the clk finding api is.

Judging from the ideal use of our in-tree driver
drivers/mfd/ab8500-gpadc.c a map like this would be
great

struct adc_map {
        struct device *adc_dev;
        const char *adc_dev_name;
        const char *channel;
        struct device *dev;
        const char *dev_name;
};

{ adc_dev, adc_dev_name } are alternative-compulsory identifiers for
the ADC

channel: string identifying the channel on the ADC, strings have
been shown to be good for identifying things.

{ dev, dev_name } are alternative-compulsory identifiers for the
ADC client.

struct device * pointers take precedence, fallback to names if device
pointers are unavailable.

adc_dev_name is the name of the ADC and then I mean what is returned
from it's struct device *dev when you do dev_name() on it. dev_name is
well dev_name(dev);

Then this API feels comfortable:

struct adc *adc_get(struct device *dev, const char *channel);
void adc_put(struct adc *adc);
int adc_read_raw(struct adc *adc, int *val);

We can then add adc_read_voltage(), adc_read_temperature(), adc_read_foo() ...

Usage in say a hwmon driver or whatever:

struct adc *mr_adc = adc_get(dev, NULL);
/*
 * Optional channel parameter used only when a single
 * device use more than one channel, as with clock names
 * or regulator names, so pass in NULL for the one channel
 * tied to this device.
 */
ret = adc_read_raw(mr_adc, &val);
(...)
adc_put(mr_adc);

But this is just what looks useful to us.

Yours,
Linus Walleij

Re: [PATCH] staging:iio:proof of concept in kernel interface.

[Mark Brown]

On Sun, Oct 16, 2011 at 08:45:06PM +0200, Linus Walleij wrote:

> Then this API feels comfortable:

> struct adc *adc_get(struct device *dev, const char *channel);
> void adc_put(struct adc *adc);
> int adc_read_raw(struct adc *adc, int *val);

> We can then add adc_read_voltage(), adc_read_temperature(), adc_read_foo() ...

That should work for pretty much all of the AUXADCs.

Re: [PATCH] staging:iio:proof of concept in kernel interface.

[Jonathan Cameron]

On 10/16/11 19:45, Linus Walleij wrote:
> 2011/10/14 Jonathan Cameron <jic23@cam.ac.uk>:
> 
>> I'm trying to work out what our equivalent of the clk finding api is.
> 
> Judging from the ideal use of our in-tree driver
> drivers/mfd/ab8500-gpadc.c a map like this would be
> great
> 
> struct adc_map {
>         struct device *adc_dev;
>         const char *adc_dev_name;
>         const char *channel;
>         struct device *dev;
>         const char *dev_name;
> };
> 
> { adc_dev, adc_dev_name } are alternative-compulsory identifiers for
> the ADC
> 
> channel: string identifying the channel on the ADC, strings have
> been shown to be good for identifying things.
> 
> { dev, dev_name } are alternative-compulsory identifiers for the
> ADC client.
> 
> struct device * pointers take precedence, fallback to names if device
> pointers are unavailable.
> 
> adc_dev_name is the name of the ADC and then I mean what is returned
> from it's struct device *dev when you do dev_name() on it. dev_name is
> well dev_name(dev);
> 
> Then this API feels comfortable:
> 
> struct adc *adc_get(struct device *dev, const char *channel);
> void adc_put(struct adc *adc);
> int adc_read_raw(struct adc *adc, int *val);
> 
> We can then add adc_read_voltage(), adc_read_temperature(), adc_read_foo() ...
> 
> Usage in say a hwmon driver or whatever:
> 
> struct adc *mr_adc = adc_get(dev, NULL);
> /*
>  * Optional channel parameter used only when a single
>  * device use more than one channel, as with clock names
>  * or regulator names, so pass in NULL for the one channel
>  * tied to this device.
>  */
> ret = adc_read_raw(mr_adc, &val);
> (...)
> adc_put(mr_adc);
> 
> But this is just what looks useful to us.
> 
Thanks for this Linus. It is more or less where I was heading with one
exception.  I was going to link whole physical devices whereas I think
you are proposing linking individual channels.

>>From the client side yours make more sense (why should a client care
that the channels are on the same device?).  Saying that I think convention
for hwmon usage all channels should be on the same physical device
(to match against current hwmon conventions).

For the adc channel I'm not particularly keen on insisting every device
have a unique name for every channel.  That breaks the effort we went to
in IIO to enforce consistent naming (for userspace interfaces) in the first
place.  To my mind it should never be anything other than a number.

Hence I propose that your char *channel is for identifying the element in
the map and we have a full structure with an addition of a channel_number
on the adc side.  (Note I'm keeping adc here to match your naming, but will
not do that later as I want to use the same interface for output devices!)

struct adc_map {
/* Input / output side */
        struct device *adc_dev;
        const char *adc_dev_name;
	int channel_number;
/* User side */
        const char *channel;
        struct device *dev;
        const char *dev_name;
};

Anyhow, I'll have a go at implementing this over the next few days.  Before I
do that I am going to push out the proposal to move the core IIO infrastructure
out of staging.  Clearly that interacts with this discussion (and the cover letter
will reflect that!), but I want to get the rest of that under review (particularly
interfaces) whilst we continue this discussion / development in a parallel track.

Thanks,

Jonathan

Re: [PATCH] staging:iio:proof of concept in kernel interface.

[Jonathan Cameron]

On 10/17/11 10:39, Mark Brown wrote:
> On Sun, Oct 16, 2011 at 08:45:06PM +0200, Linus Walleij wrote:
> 
>> Then this API feels comfortable:
> 
>> struct adc *adc_get(struct device *dev, const char *channel);
>> void adc_put(struct adc *adc);
>> int adc_read_raw(struct adc *adc, int *val);
> 
>> We can then add adc_read_voltage(), adc_read_temperature(), adc_read_foo() ...
> 
> That should work for pretty much all of the AUXADCs.
> 
Agreed with extra indirection for channel association as per my
other email.

Re: [PATCH] staging:iio:proof of concept in kernel interface.

[Mark Brown]

On Mon, Oct 17, 2011 at 10:43:53AM +0100, Jonathan Cameron wrote:

> For the adc channel I'm not particularly keen on insisting every device
> have a unique name for every channel.  That breaks the effort we went to
> in IIO to enforce consistent naming (for userspace interfaces) in the first
> place.  To my mind it should never be anything other than a number.

I think you want something similar to what we do with platform devices -
have both a name and a number.  For devices that are just a bunch of
symmetric channels (which seems to be the main case you're thinking of)
the name would always be the same and the number would vary but if the
device has a bunch of different functions then they'd be named.  That
way you don't have to remember that the channels of a given type start
at offset X.

For example, on a PMIC you'll often have ADC channels wired up to
specific supplies plus some extras which are left for board use.  The
fixed channels might get named after the supplies and the extras all
have the same name distinguished by number.

Re: [PATCH] staging:iio:proof of concept in kernel interface.

[Jonathan Cameron]

On 10/17/11 11:19, Mark Brown wrote:
> On Mon, Oct 17, 2011 at 10:43:53AM +0100, Jonathan Cameron wrote:
> 
>> For the adc channel I'm not particularly keen on insisting every device
>> have a unique name for every channel.  That breaks the effort we went to
>> in IIO to enforce consistent naming (for userspace interfaces) in the first
>> place.  To my mind it should never be anything other than a number.
> 
> I think you want something similar to what we do with platform devices -
> have both a name and a number.  For devices that are just a bunch of
> symmetric channels (which seems to be the main case you're thinking of)
> the name would always be the same and the number would vary but if the
> device has a bunch of different functions then they'd be named.  That
> way you don't have to remember that the channels of a given type start
> at offset X.
> 
> For example, on a PMIC you'll often have ADC channels wired up to
> specific supplies plus some extras which are left for board use.  The
> fixed channels might get named after the supplies and the extras all
> have the same name distinguished by number.
> 
We effectively have the equivalent already - I can add it to the query
interface.  It allows channels to have a supplementary name typically
if they are measuring something not of general purpose (such as the
sensor supply voltage).

I'd also allow for name to be NULL - it provides no information at all
for a lot of devices.

Note, if we are talking functions such 'acceleration' then I'd much
rather have explicit enums for type, direction etc in there.  This
means that we are mapping onto the full channel spec rather than the
channel index. Really don't want strings on the adc side (except for
special cases as described above) as I just spent rather a long time
ripping them out precisely to enforce a consistent interface where
there was previously too much flexibility.

We have a system that allows describing of more or less everything we
have ever encountered in a rigorous consistent fashion (I say almost
as there is a slight wrinkle with some dds channels as currently defined).

Initially I'd be tempted to only support numbered channels (on adc side).
Later we can add the named option - iff it has real use cases.

Jonathan

Re: [PATCH] staging:iio:proof of concept in kernel interface.

[Mark Brown]

On Mon, Oct 17, 2011 at 11:32:03AM +0100, Jonathan Cameron wrote:

> Initially I'd be tempted to only support numbered channels (on adc side).
> Later we can add the named option - iff it has real use cases.

I think the PMIC case is a real one, assigning numbers people need to
use to find things is doesn't really reflect actual usage.

Re: [PATCH] staging:iio:proof of concept in kernel interface.

[Jonathan Cameron]

On 10/17/11 11:46, Mark Brown wrote:
> On Mon, Oct 17, 2011 at 11:32:03AM +0100, Jonathan Cameron wrote:
> 
>> Initially I'd be tempted to only support numbered channels (on adc side).
>> Later we can add the named option - iff it has real use cases.
> 
> I think the PMIC case is a real one, assigning numbers people need to
> use to find things is doesn't really reflect actual usage.
> 
Just to make sure I'm understanding you correctly could you give an example
part.  The supplies one we explicitly handle anyway so I'm happy with that
one, I just want to confirm that this is what you are talking about.

Anyhow, should get to code this afternoon and always easier to discuss
actual code!  

Re: [PATCH] staging:iio:proof of concept in kernel interface.

[Mark Brown]

On Mon, Oct 17, 2011 at 12:13:27PM +0100, Jonathan Cameron wrote:
> On 10/17/11 11:46, Mark Brown wrote:

> > I think the PMIC case is a real one, assigning numbers people need to
> > use to find things is doesn't really reflect actual usage.

> Just to make sure I'm understanding you correctly could you give an example
> part.  The supplies one we explicitly handle anyway so I'm happy with that
> one, I just want to confirm that this is what you are talking about.

The wm831x PMICs have an AUXADC with (actual wirings vary depending on
the particular device):

 - Some voltage inputs hard wired to particular system supplies.
 - One or more temperature inputs wired to particular place (eg, chip
   and battery).
 - Some channels that measure voltages on some of the pins with no
   particular function allocated to them.

Re: [PATCH] staging:iio:proof of concept in kernel interface.

[Jonathan Cameron]

On 10/17/11 12:18, Mark Brown wrote:
> On Mon, Oct 17, 2011 at 12:13:27PM +0100, Jonathan Cameron wrote:
>> On 10/17/11 11:46, Mark Brown wrote:
> 
>>> I think the PMIC case is a real one, assigning numbers people need to
>>> use to find things is doesn't really reflect actual usage.
> 
>> Just to make sure I'm understanding you correctly could you give an example
>> part.  The supplies one we explicitly handle anyway so I'm happy with that
>> one, I just want to confirm that this is what you are talking about.
> 
> The wm831x PMICs have an AUXADC with (actual wirings vary depending on
> the particular device):
Just to pin this down completely...
> 
>  - Some voltage inputs hard wired to particular system supplies.
Hard wired in the pmic?  (e.g. do not vary from device to device).
These I'm happy to see have names.  If it were a normal IIO device their
access attributes would be something like:

in_voltage0_supply3V_raw
in_voltage1_supply2.8V_raw

(have insist on indexing even with named channels because it is needed as
events codes don't want to carry a string.).

>  - One or more temperature inputs wired to particular place (eg, chip
>    and battery).
Not hard wired so to my mind these are just general purpose temperature inputs.
Hence naming doesn't make sense (at least not outside of board file or DT).
If there really is something that stops these being switched round then they can
be named (e.g. if one is actually in the pmic package!). IIO raw attribute would be: 

in_temp0_raw
in_temp1_raw etc

>  - Some channels that measure voltages on some of the pins with no
>    particular function allocated to them.
So these are nameless numbered channels.

in_voltage13_raw onwards.

There are some complexities to deal with that make me wonder if direct indexing
into a driver provided table isn't easier.  The other is channel types.

Perhaps something as involved as the following works - we are basically lifting about
half of struct iio_chan_spec into here:

struct adc_map {
/* Input / output side */
        struct device *adc_dev;
        const char *adc_dev_name;
	int channel_num1;
	int channel_num2;
	enum IIO_TYPE type /*adc etc*/
	bool differential.
/* User side */
        const char *channel;
        struct device *dev;
        const char *dev_name;
};

Is it so bad to insist that the dt writer or equivalent actually looks at the
driver in question and picks the underlying channel index directly?

I'm certainly going to implement that first then add the matching logic
afterwards anyway as it will make it easier to review / test.

Re: [PATCH] staging:iio:proof of concept in kernel interface.

[Mark Brown]

On Mon, Oct 17, 2011 at 12:32:25PM +0100, Jonathan Cameron wrote:
> On 10/17/11 12:18, Mark Brown wrote:

> >  - Some voltage inputs hard wired to particular system supplies.

> Hard wired in the pmic?  (e.g. do not vary from device to device).
> These I'm happy to see have names.  If it were a normal IIO device their
> access attributes would be something like:

> in_voltage0_supply3V_raw
> in_voltage1_supply2.8V_raw

Yes, though the names are more symbolic than that (eg, "USB").

> (have insist on indexing even with named channels because it is needed as
> events codes don't want to carry a string.).

This is orthogonal to the request interface, though.

> >  - One or more temperature inputs wired to particular place (eg, chip
> >    and battery).

> Not hard wired so to my mind these are just general purpose temperature inputs.
> Hence naming doesn't make sense (at least not outside of board file or DT).

No, they're hard wired - like I say they're wired to a particular place.

> There are some complexities to deal with that make me wonder if direct indexing
> into a driver provided table isn't easier.  The other is channel types.

It's not great for usability if you've got to apply an additional layer
of translation on top of mapping the schematic onto the hardware.

> Is it so bad to insist that the dt writer or equivalent actually looks at the
> driver in question and picks the underlying channel index directly?

We certainly can't put Linux specific channel numbers into the device
tree.

Re: [PATCH] staging:iio:proof of concept in kernel interface.

[Jonathan Cameron]

On 10/17/11 13:08, Mark Brown wrote:
> On Mon, Oct 17, 2011 at 12:32:25PM +0100, Jonathan Cameron wrote:
>> On 10/17/11 12:18, Mark Brown wrote:
> 
>>>  - Some voltage inputs hard wired to particular system supplies.
> 
>> Hard wired in the pmic?  (e.g. do not vary from device to device).
>> These I'm happy to see have names.  If it were a normal IIO device their
>> access attributes would be something like:
> 
>> in_voltage0_supply3V_raw
>> in_voltage1_supply2.8V_raw
> 
> Yes, though the names are more symbolic than that (eg, "USB").
> 
>> (have insist on indexing even with named channels because it is needed as
>> events codes don't want to carry a string.).
> 
> This is orthogonal to the request interface, though.
No it isn't - because it will effect the numbering of the gneral purpose ones.
> 
>>>  - One or more temperature inputs wired to particular place (eg, chip
>>>    and battery).
> 
>> Not hard wired so to my mind these are just general purpose temperature inputs.
>> Hence naming doesn't make sense (at least not outside of board file or DT).
> 
> No, they're hard wired - like I say they're wired to a particular place.
If it isn't physically in the pmic package then it doesn't belong in the driver.
> 
>> There are some complexities to deal with that make me wonder if direct indexing
>> into a driver provided table isn't easier.  The other is channel types.
> 
> It's not great for usability if you've got to apply an additional layer
> of translation on top of mapping the schematic onto the hardware.
Agreed.  Lets see how ti comes out.
> 
>> Is it so bad to insist that the dt writer or equivalent actually looks at the
>> driver in question and picks the underlying channel index directly?
> 
> We certainly can't put Linux specific channel numbers into the device
> tree.
Fine so numbering is based on Linux specific channel numbers based on the channel
type rather than a single index.  Thus as a minimum we need everything given
in my last proposed structure.

Note the numbering is still going to be Linux specific just within a given type
of channel. ADC channels regularly have completely inconsistent (and/or stupid) names
in device data sheets.

0...4
1...5
AUXA....AUXD
TEMP_EXT1...TEMP_EXT5 (all of which are just normal adc channels that some
designer decided would be used only for connecting analog temperature sensors.)

All of these get mapped to 0...4.  Anything else leads to totally unpredictable
guessing games when trying to find the channel.

Naming should be restricted for the rare case where it really really is physically
mounted in the same chip package and hence can never be used for anything else.
Sure if the adc has special features that mean it really is only useful for one
thing (like a resolver channel) then it is fine to mark it as a more specific type.

Jonathan

Re: [PATCH] staging:iio:proof of concept in kernel interface.

[Mark Brown]

On Mon, Oct 17, 2011 at 01:31:25PM +0100, Jonathan Cameron wrote:
> On 10/17/11 13:08, Mark Brown wrote:
> > On Mon, Oct 17, 2011 at 12:32:25PM +0100, Jonathan Cameron wrote:

Your mails would be much more legible if you'd leave blank lines between
paragraphs and if you could wrap your mails at less than 80 columns.

> >> (have insist on indexing even with named channels because it is needed as
> >> events codes don't want to carry a string.).

> > This is orthogonal to the request interface, though.

> No it isn't - because it will effect the numbering of the gneral purpose ones.

I don't think the numbers reported in events should be forced to be
directly associated with the numbers (consider the case with mixes of
high sensitivity and low sensitivity inputs for example).

> >> Not hard wired so to my mind these are just general purpose temperature inputs.
> >> Hence naming doesn't make sense (at least not outside of board file or DT).

> > No, they're hard wired - like I say they're wired to a particular place.

> If it isn't physically in the pmic package then it doesn't belong in the driver.

In the case of the die temperature measurement things are obviously
entirely within the PMIC.  In the case of battery temperature monitoring
the actual sensor is outside the device but it's fixed function due to
cross connection with the autonomous control logic for the battery
chargers.

> Note the numbering is still going to be Linux specific just within a given type
> of channel. ADC channels regularly have completely inconsistent (and/or stupid) names
> in device data sheets.

> 0...4
> 1...5
> AUXA....AUXD
> TEMP_EXT1...TEMP_EXT5 (all of which are just normal adc channels that some
> designer decided would be used only for connecting analog temperature sensors.)

None of those look at all unreasonable?

> All of these get mapped to 0...4.  Anything else leads to totally unpredictable
> guessing games when trying to find the channel.

I dunno, things like introducing an offset of 1 into the numbers the
datasheet uses can be pretty painful to work with - it's not obvious
when looking at the code that you've got the number wrong.  With the
PMICs it's been really helpful to do things like map the LDOs and DCDCs
separately even though at the level where you're gluing things together
there's no meaningful difference.

Once we're running it's a different story, it's just when expressing the
wiring to the system that I'm worried.

Re: [PATCH] staging:iio:proof of concept in kernel interface.

[Jonathan Cameron]

On 10/17/11 13:48, Mark Brown wrote:
> On Mon, Oct 17, 2011 at 01:31:25PM +0100, Jonathan Cameron wrote:
>> On 10/17/11 13:08, Mark Brown wrote:
>>> On Mon, Oct 17, 2011 at 12:32:25PM +0100, Jonathan Cameron wrote:
> 
> Your mails would be much more legible if you'd leave blank lines between
> paragraphs and if you could wrap your mails at less than 80 columns.

Sorry.  Too used to pretty colour coding ;)

> 
>>>> (have insist on indexing even with named channels because it is needed as
>>>> events codes don't want to carry a string.).
> 
>>> This is orthogonal to the request interface, though.
> 
>> No it isn't - because it will effect the numbering of the gneral purpose ones.
> 
> I don't think the numbers reported in events should be forced to be
> directly associated with the numbers (consider the case with mixes of
> high sensitivity and low sensitivity inputs for example).
> 
>>>> Not hard wired so to my mind these are just general purpose temperature inputs.
>>>> Hence naming doesn't make sense (at least not outside of board file or DT).
> 
>>> No, they're hard wired - like I say they're wired to a particular place.
> 
>> If it isn't physically in the pmic package then it doesn't belong in the driver.
> 
> In the case of the die temperature measurement things are obviously
> entirely within the PMIC.  In the case of battery temperature monitoring
> the actual sensor is outside the device but it's fixed function due to
> cross connection with the autonomous control logic for the battery
> chargers.

In those cases it would be fine.  We will however want to enforce a
consistent naming across different devices.

> 
>> Note the numbering is still going to be Linux specific just within a given type
>> of channel. ADC channels regularly have completely inconsistent (and/or stupid) names
>> in device data sheets.
> 
>> 0...4
>> 1...5
>> AUXA....AUXD
>> TEMP_EXT1...TEMP_EXT5 (all of which are just normal adc channels that some
>> designer decided would be used only for connecting analog temperature sensors.)
> 
> None of those look at all unreasonable

Fair enough though the temp one is at least stupid, but why should
we match them?

> 
>> All of these get mapped to 0...4.  Anything else leads to totally unpredictable
>> guessing games when trying to find the channel.
> 
> I dunno, things like introducing an offset of 1 into the numbers the
> datasheet uses can be pretty painful to work with - it's not obvious
> when looking at the code that you've got the number wrong.  With the
> PMICs it's been really helpful to do things like map the LDOs and DCDCs
> separately even though at the level where you're gluing things together
> there's no meaningful difference.

Ok, so we could add to every channel a magic matching field called
datasheet_name?  To my mind its silly overhead, but if there is a
consensus on it then fine.  Note however that any remotely general
purpose userspace will not read these values even if we make
them available.

> 
> Once we're running it's a different story, it's just when expressing the
> wiring to the system that I'm worried.

I don't like it, but would adding a magic const char *datasheet_name
to each channel and allowing matching on that work for you?
We'd have to dictate that it was the first name found on a
schematic diagram of the chip in the datasheet as these sheets
are often completely inconsistent in uses of abbreviations etc.
This will also mean insanity such as having separate channel
tables for compatible parts if the naming on their particular
datasheet is different...

So hideous mess in drivers to make mapping a little easier. I guess
there is a balance to be struck.

Re: [PATCH] staging:iio:proof of concept in kernel interface.

[Linus Walleij]

On Mon, Oct 17, 2011 at 11:43 AM, Jonathan Cameron <jic23@cam.ac.uk> wrote:

> Thanks for this Linus. It is more or less where I was heading with one
> exception. =A0I was going to link whole physical devices whereas I think
> you are proposing linking individual channels.

That's for the usecases I have at hand, I don't know which is more
common: grab the entire device and pick a few channels or grab
a single channel. I was thinking the latter would be most common
for in-kernel use.

If you need some inspiring code for how to handle registration of
mapping tables etc you can look at the API I'm pushing for the
pin control subsystem:
http://marc.info/?l=3Dlinux-kernel&m=3D131850357826490&w=3D2

> For the adc channel I'm not particularly keen on insisting every device
> have a unique name for every channel. =A0That breaks the effort we went t=
o
> in IIO to enforce consistent naming (for userspace interfaces) in the fir=
st
> place. =A0To my mind it should never be anything other than a number.

Doesn't feel like a big issue, plain numbers are fine for me.

I was more thinking along the line that in an embedded system an
on-system-in-chip GPADC sure has specific set-in-stone uses for
each channel, i.e. the pin it connects to is hardwired somewhere,
like channel 0 is "battery-voltage", pin 1 is "battery-temperature"
etc, which makes string descriptions make sense.

It doesn't make sense for an ADC card with 100 ADC:s for random
measurements, of course.

> struct adc_map {
> /* Input / output side */
> =A0 =A0 =A0 =A0struct device *adc_dev;
> =A0 =A0 =A0 =A0const char *adc_dev_name;
> =A0 =A0 =A0 =A0int channel_number;
> /* User side */
> =A0 =A0 =A0 =A0const char *channel;
> =A0 =A0 =A0 =A0struct device *dev;
> =A0 =A0 =A0 =A0const char *dev_name;
> };

OK that makes sense.

> Anyhow, I'll have a go at implementing this over the next few days. =A0Be=
fore I
> do that I am going to push out the proposal to move the core IIO infrastr=
ucture
> out of staging.

Good, my gut feeling is that right now it hurts the kernel more to
have IIO in staging than it could ever hurt it to have it in drivers/,
it is getting badly needed.

Yours,
Linus Walleij

Re: [PATCH] staging:iio:proof of concept in kernel interface.

[Mark Brown]

On Mon, Oct 17, 2011 at 02:03:53PM +0100, Jonathan Cameron wrote:
> On 10/17/11 13:48, Mark Brown wrote:

> >> 0...4
> >> 1...5
> >> AUXA....AUXD
> >> TEMP_EXT1...TEMP_EXT5 (all of which are just normal adc channels that some
> >> designer decided would be used only for connecting analog temperature sensors.)

> > None of those look at all unreasonable

> Fair enough though the temp one is at least stupid, but why should
> we match them?

So that the engineer sitting there with the schematics can figure out
how to tell software about the board hookup with minimal pain.

> Ok, so we could add to every channel a magic matching field called
> datasheet_name?  To my mind its silly overhead, but if there is a
> consensus on it then fine.  Note however that any remotely general
> purpose userspace will not read these values even if we make
> them available.

That depends, if the userspace is really general purpose I'd expect it'd
be exposing this information to let the user point and click (or
whatever) through the channels.  Otherwise they'll be scratching their
heads wondering what channel 0 on this particular system actually is.

Re: [PATCH] staging:iio:proof of concept in kernel interface.

[Jonathan Cameron]

On 10/17/11 14:55, Linus Walleij wrote:
> On Mon, Oct 17, 2011 at 11:43 AM, Jonathan Cameron <jic23@cam.ac.uk> wrote:
> 
>> Thanks for this Linus. It is more or less where I was heading with one
>> exception.  I was going to link whole physical devices whereas I think
>> you are proposing linking individual channels.
> 
> That's for the usecases I have at hand, I don't know which is more
> common: grab the entire device and pick a few channels or grab
> a single channel. I was thinking the latter would be most common
> for in-kernel use.
Guess most common for a pull type situation, less so for messing
with input devices (accelerometers etc - where they need a push
interface from the adc to the next layer up).
> 
> If you need some inspiring code for how to handle registration of
> mapping tables etc you can look at the API I'm pushing for the
> pin control subsystem:
> http://marc.info/?l=linux-kernel&m=131850357826490&w=2
Thanks, I admit I lost touch with this a while back :(
> 
>> For the adc channel I'm not particularly keen on insisting every device
>> have a unique name for every channel.  That breaks the effort we went to
>> in IIO to enforce consistent naming (for userspace interfaces) in the first
>> place.  To my mind it should never be anything other than a number.
> 
> Doesn't feel like a big issue, plain numbers are fine for me.
> 
> I was more thinking along the line that in an embedded system an
> on-system-in-chip GPADC sure has specific set-in-stone uses for
> each channel, i.e. the pin it connects to is hardwired somewhere,
> like channel 0 is "battery-voltage", pin 1 is "battery-temperature"
> etc, which makes string descriptions make sense.
Yup. Those I'm fine with. Mark is talking me into providing string
mappings for everything. We'll see how it works in practice.
Perhaps there is a happy middle ground.

> 
> It doesn't make sense for an ADC card with 100 ADC:s for random
> measurements, of course.
> 
>> struct adc_map {
>> /* Input / output side */
>>        struct device *adc_dev;
>>        const char *adc_dev_name;
>>        int channel_number;
>> /* User side */
>>        const char *channel;
>>        struct device *dev;
>>        const char *dev_name;
>> };
> 
> OK that makes sense.
> 
>> Anyhow, I'll have a go at implementing this over the next few days.  Before I
>> do that I am going to push out the proposal to move the core IIO infrastructure
>> out of staging.
> 
> Good, my gut feeling is that right now it hurts the kernel more to
> have IIO in staging than it could ever hurt it to have it in drivers/,
> it is getting badly needed.
We're working on it ;)  All reviews welcome.  We only need the first core
bit to build this pull type in kernel interfaces on top. 
(sorry, didn't cc you on the posting a few minutes ago) 
https://lkml.org/lkml/2011/10/17/157
> 
> Yours,
> Linus Walleij
> 

Re: [PATCH] staging:iio:proof of concept in kernel interface.

[Jonathan Cameron]

On 10/17/11 14:55, Mark Brown wrote:
> On Mon, Oct 17, 2011 at 02:03:53PM +0100, Jonathan Cameron wrote:
>> On 10/17/11 13:48, Mark Brown wrote:
> 
>>>> 0...4
>>>> 1...5
>>>> AUXA....AUXD
>>>> TEMP_EXT1...TEMP_EXT5 (all of which are just normal adc channels that some
>>>> designer decided would be used only for connecting analog temperature sensors.)
> 
>>> None of those look at all unreasonable
> 
>> Fair enough though the temp one is at least stupid, but why should
>> we match them?
> 
> So that the engineer sitting there with the schematics can figure out
> how to tell software about the board hookup with minimal pain.
Fine, I'll code it up and we'll see what works for the cases we have.
> 
>> Ok, so we could add to every channel a magic matching field called
>> datasheet_name?  To my mind its silly overhead, but if there is a
>> consensus on it then fine.  Note however that any remotely general
>> purpose userspace will not read these values even if we make
>> them available.
> 
> That depends, if the userspace is really general purpose I'd expect it'd
> be exposing this information to let the user point and click (or
> whatever) through the channels.  Otherwise they'll be scratching their
> heads wondering what channel 0 on this particular system actually is.
> 
True enough, though ultimately it would be nice if it is easy enough to
work out that they can put sticky labels on the wires...  Lets call these
channel_label and keep them optional for now. If nothing else I don't
really fancy retrofitting our 50+ existing drivers with them all in
one go.

Jonathan