IIO: in kernel pull interfaces.

Quoted 3 lines · Jonathan Cameron

-static struct device_type iio_dev_type = { +struct device_type iio_dev_type = { .name = "iio_device", .release = iio_dev_release, };

Why is this exported?

Quoted 3 lines

(quoted diff) +1 -0

+	mutex_lock(&iio_map_list_lock); +	while ((maps[i].consumer_dev

!= NULL) || + (maps[i].consumer_dev_name != NULL)) {

I'd suggest just dropping the struct device - the reason we support the struct device directly in the regulator API is that we originally had only a struct device and kept the code around as a transition measure (though now it's so old we should be able to kill it off). This would simplify the code and the interface a bit.

Quoted 3 lines · Mark Brown

Quoted 1 line · Jonathan Cameron

+ =A0 =A0mutex_lock(&iio_map_list_lock); + =A0 =A0 =A0 while ((maps[i].=

Quoted 1 line

Quoted 1 line

!=3D NULL) || + =A0 =A0 =A0 =A0 =A0 =A0 =A0 (maps[i].consumer_dev_name =

Quoted 7 lines

I'd suggest just dropping the struct device - the reason we support the struct device directly in the regulator API is that we originally had only a struct device and kept the code around as a transition measure (though now it's so old we should be able to kill it off). =A0This would simplify the code and the interface a bit.

Ah, that explains your comment on the 5th patch. =A0Alright, I'll let thi=

sit for a few days and if no one comes up with a good reason not to we'll go with just the dev_name option.

Quoted 13 lines · Jonathan Cameron

Quoted 11 lines · Mark Brown

Quoted 2 lines · Jonathan Cameron

+ mutex_lock(&iio_map_list_lock); + while ((maps[i].consumer_dev
!= NULL) || + (maps[i].consumer_dev_name != NULL)) {

I'd suggest just dropping the struct device - the reason we support the struct device directly in the regulator API is that we originally had only a struct device and kept the code around as a transition measure (though now it's so old we should be able to kill it off). This would simplify the code and the interface a bit.

Quoted 3 lines

Ah, that explains your comment on the 5th patch. Alright, I'll let this sit for a few days and if no one comes up with a good reason not to we'll go with just the dev_name option.

I've just deleted the yse of struct device * from the pin control subsystem and I bet ark will have it deleted from regulator_consumer before long, so just let it go :-)

From: Jonathan Cameron <[email protected]>

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.

V2: As per Greg KH suggestion, move over to registration by passing the tables into the provider drivers (how regulator does it). This does not prevent us using the original more flexible approach if at a later date there is a usecase that demands it.

Signed-off-by Jonathan Cameron <[email protected]>

---
 drivers/staging/iio/Kconfig             |    7 +-
 drivers/staging/iio/Makefile            |    2 +-
 drivers/staging/iio/consumer.h          |  103 +++++++++++++
 drivers/staging/iio/driver.h            |   34 ++++
 drivers/staging/iio/iio_core.h          |    3 +
 drivers/staging/iio/industrialio-core.c |    2 +-
 drivers/staging/iio/inkern.c            |  256 +++++++++++++++++++++++++++++++
 drivers/staging/iio/machine.h           |   30 ++++
 8 files changed, 434 insertions(+), 3 deletions(-)

drivers/staging/iio/Kconfig +0 -0

index 90162aa..65c2a8e 100644

Quoted 32 lines · Jonathan Cameron

From: Jonathan Cameron <[email protected]>

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.

V2: As per Greg KH suggestion, move over to registration by passing the tables into the provider drivers (how regulator does it). This does not prevent us using the original more flexible approach if at a later date there is a usecase that demands it.

Signed-off-by Jonathan Cameron <[email protected]>

---
 drivers/staging/iio/Kconfig             |    7 +-
 drivers/staging/iio/Makefile            |    2 +-
 drivers/staging/iio/consumer.h          |  103 +++++++++++++
 drivers/staging/iio/driver.h            |   34 ++++
 drivers/staging/iio/iio_core.h          |    3 +
 drivers/staging/iio/industrialio-core.c |    2 +-
 drivers/staging/iio/inkern.c            |  256 +++++++++++++++++++++++++++++++
 drivers/staging/iio/machine.h           |   30 ++++
 8 files changed, 434 insertions(+), 3 deletions(-)

drivers/staging/iio/Kconfig +0 -0

index 90162aa..65c2a8e 100644

drivers/staging/iio/Kconfig menuconfig IIO · line 19

Quoted 10 lines

drivers/staging/iio/Kconfig +5 -0

@@ -10,8 +10,14 @@ menuconfig IIO
 	  drivers for many different types of embedded sensors using a
 	  number of different physical interfaces (i2c, spi, etc). See
 	  drivers/staging/iio/Documentation for more information.
+
 if IIO
 
+config IIO_INKERN
+	bool "In kernel support for IIO"
+	help
+	  Support in kernel users of IIO device drivers.

Of course you want this, all of the code in the kernel.org tree is "in kernel users" :)

Seriously, I still fail to understand what is so special here that makes this a totally different design pattern from all other busses, especially when you aren't even a bus at all, but rather a "interface" to userspace for different device types.

Why can't a driver just depend on this interface type, like all other interface types are, that way the built-in vs. as-a-module issues are all handled "automagically" by the config and build system.

Quoted 47 lines · Greg KH

Quoted 32 lines · Jonathan Cameron

From: Jonathan Cameron <[email protected]>

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.

V2: As per Greg KH suggestion, move over to registration by passing the tables into the provider drivers (how regulator does it). This does not prevent us using the original more flexible approach if at a later date there is a usecase that demands it.

Signed-off-by Jonathan Cameron <[email protected]>

---
 drivers/staging/iio/Kconfig             |    7 +-
 drivers/staging/iio/Makefile            |    2 +-
 drivers/staging/iio/consumer.h          |  103 +++++++++++++
 drivers/staging/iio/driver.h            |   34 ++++
 drivers/staging/iio/iio_core.h          |    3 +
 drivers/staging/iio/industrialio-core.c |    2 +-
 drivers/staging/iio/inkern.c            |  256 +++++++++++++++++++++++++++++++
 drivers/staging/iio/machine.h           |   30 ++++
 8 files changed, 434 insertions(+), 3 deletions(-)

drivers/staging/iio/Kconfig +0 -0

index 90162aa..65c2a8e 100644

drivers/staging/iio/Kconfig menuconfig IIO · line 19

Quoted 10 lines

drivers/staging/iio/Kconfig +5 -0

@@ -10,8 +10,14 @@ menuconfig IIO
 	  drivers for many different types of embedded sensors using a
 	  number of different physical interfaces (i2c, spi, etc). See
 	  drivers/staging/iio/Documentation for more information.
+
 if IIO
 
+config IIO_INKERN
+	bool "In kernel support for IIO"
+	help
+	  Support in kernel users of IIO device drivers.

Of course you want this, all of the code in the kernel.org tree is "in kernel users" :)

Quoted 9 lines

Seriously, I still fail to understand what is so special here that makes this a totally different design pattern from all other busses, especially when you aren't even a bus at all, but rather a "interface" to userspace for different device types.

Why can't a driver just depend on this interface type, like all other interface types are, that way the built-in vs. as-a-module issues are all handled "automagically" by the config and build system.

Best simple comparison is with regulators. They provide voltages to consumers; This iio stuff is about providing readings of voltage (simplifying massively) to other users. All these adc's need their own driver to provide that voltage to those that care. The client drivers just want to ask 'what is the voltage?'.

So to do it as a conventional module dependency we need a separate dependency for every IIO device capable of providing a voltage to every driver that might want to read one.

There are a number of common use cases or which I'll list a few here.
a) Soc adc's. Typical soc might have an 8 channel adc. On a typical board 2 channels are used for input (touchscreen), 2 for monitoring the battery and
4 brought out to an edge connector. Which are used for what and how many of each are used is entirely under control of the board designer. Thus we could have each SoC type adc driver handle the logic necessary to register an input device, a hwmon device (and perhaps an IIO device). This is the usecase that Mark Brown originally brought up. Right now Arnd is blocking some SoC adc drivers that do this internally and sending them in the direction of IIO.

b) High spec devices used for input. This one we originally proposed doing via a bridge in userspace and piping back in via uinput. When I say high spec I meant 300+ dolar imu's etc. People do it for niche systems, but the part will never primarily be used for this so normally sits better in IIO, If we have borderline devices, they will end up in IIO. If this stuff had been in place earlier (as Dmitry pushed for) some of the existing parts in input might never have been accepted.

c) The other side is generating signals. People use arbitary waveform generators to produce inputs for all sort of strange devices.

So the question was whether to sit another subsystem under hwmon, input IIO etc, or to have IIO evolve into that underlying subsystem. Given with the channel mapping stuff we already had all the infrastructure in place to do this, a vague consensus said do it in IIO.

Now in many ways we'd love to have the IIO userspace stuff act as just another client, but that's a way off yet basically due to the huge number of special cases that would need to be handled. I'm personally not yet sure that will ever happen.

So in summary we don't do this as a simple dependency because of the combinatorial explosion in dependencies that would be needed and the connection modules that would be needed to glue it all together vs what is a simple mapping that will sit nicely in device tree etc.

So, would the "keep looping until all dependancies are met" patches from Grant that have been floating around for a while, solve this issue? Modules would properly initialize only when the needed other bits are present, which solves the problems the regulators and other drivers have today.

If so, then I suggest we get that code into the tree soon, and then all of this "wierd" logic would go away, right?

Quoted 8 lines · Greg KH

So, would the "keep looping until all dependancies are met" patches from Grant that have been floating around for a while, solve this issue? Modules would properly initialize only when the needed other bits are present, which solves the problems the regulators and other drivers have today.

If so, then I suggest we get that code into the tree soon, and then all of this "wierd" logic would go away, right?

Could you identify the logic you're talking about here? Looking at the last patch set I can find[1] this all looks very vanilla for this sort of subsystem? You've got a mapping mechanism and a get function but that's about it which is all pretty normal. Grant's changes make module probing nicer in that you don't need to worry about the order in which things get registered but I can't see anything here which is particularly related to that ordering.

All the code is doing is providing a mechanism for drivers that need IIO facilities to locate the thing that's providing their IIO facilities (which is in the general case going to be board specific on a totally unrelated chip in a different part of the system and discovered through reading the schematic) without requiring custom code in every single single IIO using device to do the mapping. Instead the boards use a standard subsystem facility to tell the IIO core what's wired where and then the core matches things up for them.

This is nothing to do with init ordering, it's all about the two devices finding each other at all.

Quoted 2 lines · Mark Brown

This is nothing to do with init ordering, it's all about the two devices finding each other at all.

Ok, if that's all this is for, they why name it "iio in-kernel logic"? That made me believe that this is something that is needed if anyone who happens to want to use the iio interface needs it.

If it's just "iio discovery" then call it that :)

Quoted 9 lines · Greg KH

Quoted 2 lines · Mark Brown

This is nothing to do with init ordering, it's all about the two devices finding each other at all.

Ok, if that's all this is for, they why name it "iio in-kernel logic"? That made me believe that this is something that is needed if anyone who happens to want to use the iio interface needs it.

Quoted 1 line

If it's just "iio discovery" then call it that :)

I think that's all due to the overall goal of the patch series - the original use case for IIO was purely for userspace users and what the patch series is doing is adding the ability to use IIO devices from within the kernel.

[Mark]

Quoted 3 lines

the original use case for IIO was purely for userspace users and what the patch series is doing is adding the ability to use IIO devices from within the kernel.

That was my thinking. =A0Happy to take suggestions for clearer naming!

Quoted 3 lines · Jonathan Cameron

+static struct iio_map max1363_consumer_map[] = { + { +
.adc_channel_label = "AIN1", + .consumer_dev =
&iio_hwmon_test.dev, + .consumer_channel = "testchan1",

I do think it's better to use dev_name here rather than a struct device pointer - for several of the buses it's not actually possible to get a struct device until a device has been instantiated which isn't helpful for setting up the mappings.

Quoted 5 lines · Mark Brown

Quoted 3 lines · Jonathan Cameron

+static struct iio_map max1363_consumer_map[] = { + { +
.adc_channel_label = "AIN1", + .consumer_dev =
&iio_hwmon_test.dev, + .consumer_channel = "testchan1",

Quoted 4 lines

I do think it's better to use dev_name here rather than a struct device pointer - for several of the buses it's not actually possible to get a struct device until a device has been instantiated which isn't helpful for setting up the mappings.

We allow both. In cases like this where the dev pointer is explicitly available what gain do we get from not using it?

Quoted 13 lines · Jonathan Cameron

Quoted 12 lines · Mark Brown

Quoted 3 lines · Jonathan Cameron

+static struct iio_map max1363_consumer_map[] = { + { +
.adc_channel_label = "AIN1", + .consumer_dev =
&iio_hwmon_test.dev, + .consumer_channel = "testchan1",

I do think it's better to use dev_name here rather than a struct device pointer - for several of the buses it's not actually possible to get a struct device until a device has been instantiated which isn't helpful for setting up the mappings.

Quoted 2 lines

We allow both. In cases like this where the dev pointer is explicitly available what gain do we get from not using it?

You avoid user confusion from having two ways of doing the same thing and you save a little complexity in the implementation. The user confusion does happen in practice.

Quoted 4 lines · Jonathan Cameron

Quoted 3 lines · Mark Brown

Quoted 1 line · Jonathan Cameron

+static struct iio_map max1363_consumer_map[] =3D { + =A0 =A0 =A0 =A0=

Quoted 1 line

Quoted 1 line

Quoted 1 line

.adc_channel_label =3D "AIN1", + =A0 =A0 =A0 =A0 =A0 =A0 .consumer_de=

Quoted 1 line

Quoted 1 line

Quoted 1 line

&iio_hwmon_test.dev, + =A0 =A0 =A0 =A0 =A0 =A0 .consumer_channel =3D =

Quoted 5 lines

Quoted 5 lines

I do think it's better to use dev_name here rather than a struct device pointer - for several of the buses it's not actually possible to get a struct device until a device has been instantiated which isn't helpful for setting up the mappings.

Quoted 2 lines

We allow both. =A0In cases like this where the dev pointer is explicitly available what gain do we get from not using it?

You avoid user confusion from having two ways of doing the same thing and you save a little complexity in the implementation. =A0The user confusion does happen in practice.

On Mon, Jan 30, 2012 at 10:22 PM, Mark Brown

Quoted 3 lines

You avoid user confusion from having two ways of doing the same thing and you save a little complexity in the implementation.  The user confusion does happen in practice.

So I guess this pattern comes from the regulator framework struct regulator_consumer_supply design pattern initially.

But we should then try to rid it from the kernel at large, no big deal, I have only one in-kernel user of this ATM I think so I can kill it off from the pin control API if you think you can kill it from regulators.

Firstly this is an RFC rather than a final proposal because I know there =

an issue with the provider being removed before the consumer. =A0It will =

to make this less likely to happen, but I am not entirely sure how to avo=

any possible race (suggestion welcome).