[PATCH] i2c: add irq_flags to board info

[PATCH] i2c: add irq_flags to board info

[Mike Frysinger]

From: Michael Hennerich <michael.hennerich-OyLXuOCK7orQT0dZR+AlfA@public.gmane.org>

These flags can be optionally defined - slave drivers may use them as
flags argument for request_irq().  In case they are left uninitialized
they will default to zero, and therefore shouldn't cause problems.

This allows us to avoid having to add dedicated platform init code just
to call set_irq_type() -- which doesn't work very well when coupled with
module drivers.  It also matches behavior of some other frameworks like
IDE and UIO.

Signed-off-by: Michael Hennerich <michael.hennerich-OyLXuOCK7orQT0dZR+AlfA@public.gmane.org>
Signed-off-by: Mike Frysinger <vapier-aBrp7R+bbdUdnm+yROfE0A@public.gmane.org>
---
 drivers/i2c/i2c-core.c |    1 +
 include/linux/i2c.h    |    4 ++++
 2 files changed, 5 insertions(+), 0 deletions(-)

diff --git a/drivers/i2c/i2c-core.c b/drivers/i2c/i2c-core.c
index bea4c50..830528f 100644
--- a/drivers/i2c/i2c-core.c
+++ b/drivers/i2c/i2c-core.c
@@ -540,6 +540,7 @@ i2c_new_device(struct i2c_adapter *adap, struct i2c_board_info const *info)
 	client->flags = info->flags;
 	client->addr = info->addr;
 	client->irq = info->irq;
+	client->irq_flags = info->irq_flags;
 
 	strlcpy(client->name, info->type, sizeof(client->name));
 
diff --git a/include/linux/i2c.h b/include/linux/i2c.h
index 4bae0b7..e6248c1 100644
--- a/include/linux/i2c.h
+++ b/include/linux/i2c.h
@@ -189,6 +189,7 @@ struct i2c_driver {
  * @driver: device's driver, hence pointer to access routines
  * @dev: Driver model device node for the slave.
  * @irq: indicates the IRQ generated by this device (if any)
+ * @irq_flags: The flags passed to request_irq()
  * @detected: member of an i2c_driver.clients list or i2c-core's
  *	userspace_devices list
  *
@@ -206,6 +207,7 @@ struct i2c_client {
 	struct i2c_driver *driver;	/* and our access routines	*/
 	struct device dev;		/* the device structure		*/
 	int irq;			/* irq issued by device		*/
+	unsigned long irq_flags;	/* flags used by the irq 	*/
 	struct list_head detected;
 };
 #define to_i2c_client(d) container_of(d, struct i2c_client, dev)
@@ -237,6 +239,7 @@ static inline void i2c_set_clientdata(struct i2c_client *dev, void *data)
  * @archdata: copied into i2c_client.dev.archdata
  * @of_node: pointer to OpenFirmware device node
  * @irq: stored in i2c_client.irq
+ * @irq_flags: The flags passed to request_irq() for i2c_client.irq
  *
  * I2C doesn't actually support hardware probing, although controllers and
  * devices may be able to use I2C_SMBUS_QUICK to tell whether or not there's
@@ -259,6 +262,7 @@ struct i2c_board_info {
 	struct device_node *of_node;
 #endif
 	int		irq;
+	unsigned long	irq_flags;
 };
 
 /**
-- 
1.7.3.1

Re: [PATCH] i2c: add irq_flags to board info

[Jean Delvare]

Hi Mike,

On Sun, 17 Oct 2010 19:43:39 -0400, Mike Frysinger wrote:
> From: Michael Hennerich <michael.hennerich-OyLXuOCK7orQT0dZR+AlfA@public.gmane.org>
> 
> These flags can be optionally defined - slave drivers may use them as
> flags argument for request_irq().  In case they are left uninitialized
> they will default to zero, and therefore shouldn't cause problems.
> 
> This allows us to avoid having to add dedicated platform init code just
> to call set_irq_type()

Do you have examples of this? Can we see a preview of the amount of
cleanups this patch would allow?

> -- which doesn't work very well when coupled with
> module drivers.

I don't quite get what you mean here.

> It also matches behavior of some other frameworks like
> IDE and UIO.

This is certainly a good point.

> 
> Signed-off-by: Michael Hennerich <michael.hennerich-OyLXuOCK7orQT0dZR+AlfA@public.gmane.org>
> Signed-off-by: Mike Frysinger <vapier-aBrp7R+bbdUdnm+yROfE0A@public.gmane.org>
> ---
>  drivers/i2c/i2c-core.c |    1 +
>  include/linux/i2c.h    |    4 ++++
>  2 files changed, 5 insertions(+), 0 deletions(-)
> 
> diff --git a/drivers/i2c/i2c-core.c b/drivers/i2c/i2c-core.c
> index bea4c50..830528f 100644
> --- a/drivers/i2c/i2c-core.c
> +++ b/drivers/i2c/i2c-core.c
> @@ -540,6 +540,7 @@ i2c_new_device(struct i2c_adapter *adap, struct i2c_board_info const *info)
>  	client->flags = info->flags;
>  	client->addr = info->addr;
>  	client->irq = info->irq;
> +	client->irq_flags = info->irq_flags;
>  
>  	strlcpy(client->name, info->type, sizeof(client->name));
>  
> diff --git a/include/linux/i2c.h b/include/linux/i2c.h
> index 4bae0b7..e6248c1 100644
> --- a/include/linux/i2c.h
> +++ b/include/linux/i2c.h
> @@ -189,6 +189,7 @@ struct i2c_driver {
>   * @driver: device's driver, hence pointer to access routines
>   * @dev: Driver model device node for the slave.
>   * @irq: indicates the IRQ generated by this device (if any)
> + * @irq_flags: The flags passed to request_irq()
>   * @detected: member of an i2c_driver.clients list or i2c-core's
>   *	userspace_devices list
>   *
> @@ -206,6 +207,7 @@ struct i2c_client {
>  	struct i2c_driver *driver;	/* and our access routines	*/
>  	struct device dev;		/* the device structure		*/
>  	int irq;			/* irq issued by device		*/
> +	unsigned long irq_flags;	/* flags used by the irq 	*/
>  	struct list_head detected;
>  };
>  #define to_i2c_client(d) container_of(d, struct i2c_client, dev)
> @@ -237,6 +239,7 @@ static inline void i2c_set_clientdata(struct i2c_client *dev, void *data)
>   * @archdata: copied into i2c_client.dev.archdata
>   * @of_node: pointer to OpenFirmware device node
>   * @irq: stored in i2c_client.irq
> + * @irq_flags: The flags passed to request_irq() for i2c_client.irq
>   *
>   * I2C doesn't actually support hardware probing, although controllers and
>   * devices may be able to use I2C_SMBUS_QUICK to tell whether or not there's
> @@ -259,6 +262,7 @@ struct i2c_board_info {
>  	struct device_node *of_node;
>  #endif
>  	int		irq;
> +	unsigned long	irq_flags;
>  };
>  
>  /**


-- 
Jean Delvare

RE: [PATCH] i2c: add irq_flags to board info

[Hennerich, Michael]

Jean Delvare wrote on 2010-10-18:
> Hi Mike,
>
> On Sun, 17 Oct 2010 19:43:39 -0400, Mike Frysinger wrote:
>> From: Michael Hennerich <michael.hennerich-OyLXuOCK7orQT0dZR+AlfA@public.gmane.org>
>>
>> These flags can be optionally defined - slave drivers may use them as
>> flags argument for request_irq().  In case they are left uninitialized
>> they will default to zero, and therefore shouldn't cause problems.
>>
>> This allows us to avoid having to add dedicated platform init code
>> just to call set_irq_type()
>
> Do you have examples of this? Can we see a preview of the amount of
> cleanups this patch would allow?

Examples can be found in various board setup files:

One example arch/sh/boards/mach-ecovec24/setup.c:

static struct i2c_board_info ts_i2c_clients = {
        I2C_BOARD_INFO("tsc2007", 0x48),
        .type           = "tsc2007",
        .platform_data  = &tsc2007_info,
        .irq            = IRQ0,
};

[--snip--]

                /* enable TouchScreen */
                i2c_register_board_info(0, &ts_i2c_clients, 1);
                set_irq_type(IRQ0, IRQ_TYPE_LEVEL_LOW);

>> -- which doesn't work very well when coupled with module drivers.
>
> I don't quite get what you mean here.

Since the driver doesn't setup the irq_type itself you need to do it
manually in advance using set_irq_type().
This happens most likely from your paltform setup/configuration file.

Assuming the driver is built as a module, this code gets executed unconditionally,
no matter if the driver gets finally loaded or not.

Assuming you have several drivers build as modules, using the same irq but with
different irq types, you run into problems here.

There are some development boards featuring extension options, which all plug on the same
socket but required different drivers/irq types.

The ideal way is therefore to pass the irq_flags together with the SPI/I2C board info.

>> It also matches behavior of some other frameworks like IDE and UIO.
>
> This is certainly a good point.
>
>>
>> Signed-off-by: Michael Hennerich <michael.hennerich-OyLXuOCK7orQT0dZR+AlfA@public.gmane.org>
>> Signed-off-by: Mike Frysinger <vapier-aBrp7R+bbdUdnm+yROfE0A@public.gmane.org>
>> ---
>>  drivers/i2c/i2c-core.c |    1 +
>>  include/linux/i2c.h    |    4 ++++
>>  2 files changed, 5 insertions(+), 0 deletions(-)
>> diff --git a/drivers/i2c/i2c-core.c b/drivers/i2c/i2c-core.c index
>> bea4c50..830528f 100644
>> --- a/drivers/i2c/i2c-core.c
>> +++ b/drivers/i2c/i2c-core.c
>> @@ -540,6 +540,7 @@ i2c_new_device(struct i2c_adapter *adap, struct
> i2c_board_info const *info)
>>      client->flags = info->flags;
>>      client->addr = info->addr;
>>      client->irq = info->irq;
>> +    client->irq_flags = info->irq_flags;
>>
>>      strlcpy(client->name, info->type, sizeof(client->name));
>> diff --git a/include/linux/i2c.h b/include/linux/i2c.h index
>> 4bae0b7..e6248c1 100644
>> --- a/include/linux/i2c.h
>> +++ b/include/linux/i2c.h
>> @@ -189,6 +189,7 @@ struct i2c_driver {
>>   * @driver: device's driver, hence pointer to access routines * @dev:
>>   Driver model device node for the slave. * @irq: indicates the IRQ
>>   generated by this device (if any) + * @irq_flags: The flags passed to
>>   request_irq() * @detected: member of an i2c_driver.clients list or
>>   i2c-core's *       userspace_devices list *
>> @@ -206,6 +207,7 @@ struct i2c_client {
>>      struct i2c_driver *driver;      /* and our access routines      */      struct
>>  device dev;         /* the device structure         */      int irq;                        /* irq issued by
>>  device              */ +    unsigned long irq_flags;        /* flags used by the irq        */
>>      struct list_head detected; }; #define to_i2c_client(d)
>>  container_of(d, struct i2c_client, dev) @@
>> -237,6 +239,7 @@ static inline void i2c_set_clientdata(struct
> i2c_client *dev, void *data)
>>   * @archdata: copied into i2c_client.dev.archdata * @of_node: pointer
>>   to OpenFirmware device node * @irq: stored in i2c_client.irq + *
>>   @irq_flags: The flags passed to request_irq() for i2c_client.irq * *
>>   I2C doesn't actually support hardware probing, although controllers
>>   and * devices may be able to use I2C_SMBUS_QUICK to tell whether or
>> not there's @@ -259,6 +262,7 @@ struct i2c_board_info {
>>      struct device_node *of_node; #endif     int             irq; +  unsigned
>>  long        irq_flags; };
>>
>>  /**
>
>

Greetings,
Michael

Analog Devices GmbH      Wilhelm-Wagenfeld-Str. 6      80807 Muenchen
Sitz der Gesellschaft Muenchen, Registergericht Muenchen HRB 4036 Geschaeftsfuehrer Thomas Wessel, William A. Martin, Margaret Seif

Re: [PATCH] i2c: add irq_flags to board info

[Jean Delvare]

Hi Michael,

On Mon, 18 Oct 2010 10:55:57 +0100, Hennerich, Michael wrote:
> Jean Delvare wrote on 2010-10-18:
> > Hi Mike,
> >
> > On Sun, 17 Oct 2010 19:43:39 -0400, Mike Frysinger wrote:
> >> From: Michael Hennerich <michael.hennerich@analog.com>
> >>
> >> These flags can be optionally defined - slave drivers may use them as
> >> flags argument for request_irq().  In case they are left uninitialized
> >> they will default to zero, and therefore shouldn't cause problems.
> >>
> >> This allows us to avoid having to add dedicated platform init code
> >> just to call set_irq_type()
> >
> > Do you have examples of this? Can we see a preview of the amount of
> > cleanups this patch would allow?
> 
> Examples can be found in various board setup files:
> 
> One example arch/sh/boards/mach-ecovec24/setup.c:
> 
> static struct i2c_board_info ts_i2c_clients = {
>         I2C_BOARD_INFO("tsc2007", 0x48),
>         .type           = "tsc2007",
>         .platform_data  = &tsc2007_info,
>         .irq            = IRQ0,
> };
> 
> [--snip--]
> 
>                 /* enable TouchScreen */
>                 i2c_register_board_info(0, &ts_i2c_clients, 1);
>                 set_irq_type(IRQ0, IRQ_TYPE_LEVEL_LOW);

This example doesn't quite match the patch description. There's no
"dedicated platform init code just to call set_irq_type()", as you
already have platform code to call i2c_register_board_info(). It's
really only calling set_irq_type() from platform code vs. from driver
code.

> >> -- which doesn't work very well when coupled with module drivers.
> >
> > I don't quite get what you mean here.
> 
> Since the driver doesn't setup the irq_type itself you need to do it
> manually in advance using set_irq_type().
> This happens most likely from your paltform setup/configuration file.
> 
> Assuming the driver is built as a module, this code gets executed unconditionally,
> no matter if the driver gets finally loaded or not.
> 
> Assuming you have several drivers build as modules, using the same irq but with
> different irq types, you run into problems here.

I do not get why.

You're not going to register several I2C devices using the same IRQ but
requiring different IRQ flags anyway, as it wouldn't work. So you'll
have to only register the I2C devices which are actually present on the
system. Setting the IRQ type at the same time or deferring this action
to the driver(s) doesn't make any difference then, does it?

> There are some development boards featuring extension options, which all plug on the same
> socket but required different drivers/irq types.

How do you figure out which extension option is plugged? You can't
instantiate an I2C device which isn't present, so you must have a way
to know what extension option is used, to instantiate the right I2C
device at the right address.

> The ideal way is therefore to pass the irq_flags together with the SPI/I2C board info.

All I see so far is two data structures made slightly larger, for no
actual benefit.

-- 
Jean Delvare

RE: [PATCH] i2c: add irq_flags to board info

[Hennerich, Michael]

Jean Delvare wrote on 2010-10-18:
> Hi Michael,
>
> On Mon, 18 Oct 2010 10:55:57 +0100, Hennerich, Michael wrote:
>> Jean Delvare wrote on 2010-10-18:
>>> Hi Mike,
>>>
>>> On Sun, 17 Oct 2010 19:43:39 -0400, Mike Frysinger wrote:
>>>> From: Michael Hennerich <michael.hennerich-OyLXuOCK7orQT0dZR+AlfA@public.gmane.org>
>>>>
>>>> These flags can be optionally defined - slave drivers may use them as
>>>> flags argument for request_irq().  In case they are left
>>>> uninitialized they will default to zero, and therefore shouldn't
>>>> cause problems.
>>>>
>>>> This allows us to avoid having to add dedicated platform init
>>>> code just to call set_irq_type()
>>>
>>> Do you have examples of this? Can we see a preview of the amount
>>> of cleanups this patch would allow?
>>
>> Examples can be found in various board setup files:
>>
>> One example arch/sh/boards/mach-ecovec24/setup.c:
>>
>> static struct i2c_board_info ts_i2c_clients = {
>>         I2C_BOARD_INFO("tsc2007", 0x48),
>>         .type           = "tsc2007",
>>         .platform_data  = &tsc2007_info,
>>         .irq            = IRQ0,
>> };
>>
>> [--snip--]
>>
>>                 /* enable TouchScreen */
>>                 i2c_register_board_info(0, &ts_i2c_clients, 1);
>>                 set_irq_type(IRQ0, IRQ_TYPE_LEVEL_LOW);
>
> This example doesn't quite match the patch description. There's no
> "dedicated platform init code just to call set_irq_type()", as you
> already have platform code to call i2c_register_board_info(). It's
> really only calling set_irq_type() from platform code vs. from driver
> code.

In the past I also saw initcalls just doing set_irq_type(), to make drivers happy.
That doesn't pass the right irq_flags along with request_irq().

>>>> -- which doesn't work very well when coupled with module drivers.
>>>
>>> I don't quite get what you mean here.
>>
>> Since the driver doesn't setup the irq_type itself you need to do it
>> manually in advance using set_irq_type().
>> This happens most likely from your paltform setup/configuration file.
>>
>> Assuming the driver is built as a module, this code gets executed
>> unconditionally, no matter if the driver gets finally loaded or not.
>>
>> Assuming you have several drivers build as modules, using the same irq
>> but with different irq types, you run into problems here.
>
> I do not get why.
>
> You're not going to register several I2C devices using the same IRQ
> but requiring different IRQ flags anyway, as it wouldn't work. So
> you'll have to only register the I2C devices which are actually
> present on the system. Setting the IRQ type at the same time or
> deferring this action to the driver(s) doesn't make any difference then, does it?

If I remember correctly i2c as well as spi doesn't check for irq conflicts in irqs passed with
struct i2c/spi_board_info. So you can have multiple drivers build as modules all using the same
irq number but with different flags.

The user decides which add-on module is plugged onto the processor board, by loading the appropriate
driver module.

>> There are some development boards featuring extension options, which
>> all plug on the same socket but required different drivers/irq types.
>
> How do you figure out which extension option is plugged? You can't
> instantiate an I2C device which isn't present, so you must have a way
> to know what extension option is used, to instantiate the right I2C
> device at the right address.

The user decides.
The platform code provides resources for all potential boards being used.
And here is exactly the conflict.

>> The ideal way is therefore to pass the irq_flags together with the
> SPI/I2C board info.
>
> All I see so far is two data structures made slightly larger, for no
> actual benefit.

I don't see a reason why i2c/spi bus drivers should be different from other bus drivers.
The platform_device bus driver allows you to pass IORESOURCE_IRQ | IORESOURCE_IRQ_LOWLEVEL with flags
in struct resource.

There are drivers that work around this deficiency, by adding irq_flags to the bus clients dev.platform_data
See include/linux/spi/ads7846.h for one example.

Greetings,
Michael

Analog Devices GmbH      Wilhelm-Wagenfeld-Str. 6      80807 Muenchen
Sitz der Gesellschaft Muenchen, Registergericht Muenchen HRB 4036 Geschaeftsfuehrer Thomas Wessel, William A. Martin, Margaret Seif

Re: [PATCH] i2c: add irq_flags to board info

[Jean Delvare]

Hi Michael, David,

David, question for you below.

On Mon, 18 Oct 2010 13:31:21 +0100, Hennerich, Michael wrote:
> Jean Delvare wrote on 2010-10-18:
> > Hi Michael,
> >
> > On Mon, 18 Oct 2010 10:55:57 +0100, Hennerich, Michael wrote:
> >> Jean Delvare wrote on 2010-10-18:
> >>> Hi Mike,
> >>>
> >>> On Sun, 17 Oct 2010 19:43:39 -0400, Mike Frysinger wrote:
> >>>> From: Michael Hennerich <michael.hennerich-OyLXuOCK7orQT0dZR+AlfA@public.gmane.org>
> >>>>
> >>>> These flags can be optionally defined - slave drivers may use them as
> >>>> flags argument for request_irq().  In case they are left
> >>>> uninitialized they will default to zero, and therefore shouldn't
> >>>> cause problems.
> >>>>
> >>>> This allows us to avoid having to add dedicated platform init
> >>>> code just to call set_irq_type()
> >>>
> >>> Do you have examples of this? Can we see a preview of the amount
> >>> of cleanups this patch would allow?
> >>
> >> Examples can be found in various board setup files:
> >>
> >> One example arch/sh/boards/mach-ecovec24/setup.c:
> >>
> >> static struct i2c_board_info ts_i2c_clients = {
> >>         I2C_BOARD_INFO("tsc2007", 0x48),
> >>         .type           = "tsc2007",
> >>         .platform_data  = &tsc2007_info,
> >>         .irq            = IRQ0,
> >> };
> >>
> >> [--snip--]
> >>
> >>                 /* enable TouchScreen */
> >>                 i2c_register_board_info(0, &ts_i2c_clients, 1);
> >>                 set_irq_type(IRQ0, IRQ_TYPE_LEVEL_LOW);
> >
> > This example doesn't quite match the patch description. There's no
> > "dedicated platform init code just to call set_irq_type()", as you
> > already have platform code to call i2c_register_board_info(). It's
> > really only calling set_irq_type() from platform code vs. from driver
> > code.
> 
> In the past I also saw initcalls just doing set_irq_type(), to make drivers happy.
> That doesn't pass the right irq_flags along with request_irq().

But it's OK to call request_irq() without these flags, as long as
set_irq_type() is called additionally, isn't it?

Why do we have set_irq_type() if we're not supposed to call it? I am
not claiming to be an expert in the area, but it seems totally
reasonable to me that the same piece of code instantiating an I2C
device is also responsible for setting its IRQ type.

> 
> >>>> -- which doesn't work very well when coupled with module drivers.
> >>>
> >>> I don't quite get what you mean here.
> >>
> >> Since the driver doesn't setup the irq_type itself you need to do it
> >> manually in advance using set_irq_type().
> >> This happens most likely from your paltform setup/configuration file.
> >>
> >> Assuming the driver is built as a module, this code gets executed
> >> unconditionally, no matter if the driver gets finally loaded or not.
> >>
> >> Assuming you have several drivers build as modules, using the same irq
> >> but with different irq types, you run into problems here.
> >
> > I do not get why.
> >
> > You're not going to register several I2C devices using the same IRQ
> > but requiring different IRQ flags anyway, as it wouldn't work. So
> > you'll have to only register the I2C devices which are actually
> > present on the system. Setting the IRQ type at the same time or
> > deferring this action to the driver(s) doesn't make any difference then, does it?
> 
> If I remember correctly i2c as well as spi doesn't check for irq conflicts in irqs passed with
> struct i2c/spi_board_info. So you can have multiple drivers build as modules all using the same
> irq number but with different flags.

Indeed i2c doesn't check the IRQs at all, it passes them transparently
from platform data to device driver.

But you're mixing devices and drivers. Devices are bound to an IRQ, not
drivers, and so do IRQ flags.

> 
> The user decides which add-on module is plugged onto the processor board, by loading the appropriate
> driver module.

This can't work, sorry. Unless you are using I2C device auto-detection,
which I seem to understand is NOT used in the embedded world at all,
loading a driver module doesn't do anything if a device it supports
hasn't been instantiated first. So the user can't just load a driver
and see the required I2C devices magically appear for the driver to
bind to them.

So the first problem you have to solve is how you instantiate the right
I2C devices depending on which extension is plugged in. As long as this
problem isn't solved, there's no point in discussing where the IRQ
flags should be set.

> >> There are some development boards featuring extension options, which
> >> all plug on the same socket but required different drivers/irq types.
> >
> > How do you figure out which extension option is plugged? You can't
> > instantiate an I2C device which isn't present, so you must have a way
> > to know what extension option is used, to instantiate the right I2C
> > device at the right address.
> 
> The user decides.
> The platform code provides resources for all potential boards being used.
> And here is exactly the conflict.

"Provides", how? It can't possibly instantiate them all. Please
describe the mechanism.

> >> The ideal way is therefore to pass the irq_flags together with the
> > SPI/I2C board info.
> >
> > All I see so far is two data structures made slightly larger, for no
> > actual benefit.
> 
> I don't see a reason why i2c/spi bus drivers should be different from other bus drivers.
> The platform_device bus driver allows you to pass IORESOURCE_IRQ | IORESOURCE_IRQ_LOWLEVEL with flags
> in struct resource.

This is an interesting point. David, you're the one who added irq to
struct i2c_client, you didn't add irq_flags then, did you have a good
reason not to?

> There are drivers that work around this deficiency, by adding irq_flags to the bus clients dev.platform_data
> See include/linux/spi/ads7846.h for one example.

Interesting example, better matching the initial comment that came
along with the patch. But it also seems to be an isolated case, as I
can't find any other irq_flags in include/linux/{i2c,spi}/.

-- 
Jean Delvare

Re: [PATCH] i2c: add irq_flags to board info

[David Brownell]

On Mon, 2010-10-18 at 16:33 +0200, Jean Delvare wrote:


> 
> This is an interesting point. David, you're the one who added irq to
> struct i2c_client, you didn't add irq_flags then, did you have a good
> reason not to?

There was no evident need to do so.  The problem wasn't characterizing
the IRQ, but knowing what IRQ was associated with a given I2C chip.  The
IRQs would, as a rule, only have one behavior, known in advance by the
I2C chip's driver ... if it only had a way to know client->irq ...

In one model of IRQ setup (like PC BIOS), it's handled before drivers
do more than request the IRQ, and drivers just cope. irq_flags not
needed ... it's often a hardware-to-driver convention, likely matching
I2C chip defaults.  (Or more awkwardly, coping with whatever trigger
mode the platform uses).

In another model, drivers configure their chips according to desired
mode (level low, for the most common example, or edge triggered, etc)
and again, irq_flags not needed, but in this case the platform code
would have had *nothing* do do with such mode setup.



> 
> > There are drivers that work around this deficiency, by adding irq_flags to the bus clients dev.platform_data
> > See include/linux/spi/ads7846.h for one example.

Not a good example; that was changed earlier this year and, from a quick
glance, may not have tried much to avoid that change.  The driver had
worked for years with that alleged "deficiency".


- Dave

Re: [PATCH] i2c: add irq_flags to board info

[Hennerich, Michael]

Jean Delvare wrote on 2010-10-18:
> Hi Michael, David,
>
> David, question for you below.
>
> On Mon, 18 Oct 2010 13:31:21 +0100, Hennerich, Michael wrote:
>> Jean Delvare wrote on 2010-10-18:
>>> Hi Michael,
>>>
>>> On Mon, 18 Oct 2010 10:55:57 +0100, Hennerich, Michael wrote:
>>>> Jean Delvare wrote on 2010-10-18:
>>>>> Hi Mike,
>>>>>
>>>>> On Sun, 17 Oct 2010 19:43:39 -0400, Mike Frysinger wrote:
>>>>>> From: Michael Hennerich <michael.hennerich-OyLXuOCK7orQT0dZR+AlfA@public.gmane.org>
>>>>>>
>>>>>> These flags can be optionally defined - slave drivers may use
>>>>>> them as flags argument for request_irq().  In case they are
>>>>>> left uninitialized they will default to zero, and therefore
>>>>>> shouldn't cause problems.
>>>>>>
>>>>>> This allows us to avoid having to add dedicated platform init
>>>>>> code just to call set_irq_type()
>>>>>
>>>>> Do you have examples of this? Can we see a preview of the amount
>>>>> of cleanups this patch would allow?
>>>>
>>>> Examples can be found in various board setup files:
>>>>
>>>> One example arch/sh/boards/mach-ecovec24/setup.c:
>>>>
>>>> static struct i2c_board_info ts_i2c_clients = {
>>>>         I2C_BOARD_INFO("tsc2007", 0x48),
>>>>         .type           = "tsc2007",
>>>>         .platform_data  = &tsc2007_info,
>>>>         .irq            = IRQ0,
>>>> };
>>>>
>>>> [--snip--]
>>>>
>>>>                 /* enable TouchScreen */
>>>>                 i2c_register_board_info(0, &ts_i2c_clients, 1);
>>>>                 set_irq_type(IRQ0, IRQ_TYPE_LEVEL_LOW);
>>>
>>> This example doesn't quite match the patch description. There's no
>>> "dedicated platform init code just to call set_irq_type()", as you
>>> already have platform code to call i2c_register_board_info(). It's
>>> really only calling set_irq_type() from platform code vs. from
>>> driver code.
>>
>> In the past I also saw initcalls just doing set_irq_type(), to make
>> drivers happy. That doesn't pass the right irq_flags along with
>> request_irq().
>
> But it's OK to call request_irq() without these flags, as long as
> set_irq_type() is called additionally, isn't it?

Yeah - it is. But fact is, set_irq_type() is typically called in advance.
So if the driver that requires the irq_type setting isn't loaded at all,
nothing calls request_irq().

> Why do we have set_irq_type() if we're not supposed to call it? I am
> not claiming to be an expert in the area, but it seems totally
> reasonable to me that the same piece of code instantiating an I2C
> device is also responsible for setting its IRQ type.

I would leave that up to the driver. The driver author knows which irq types the
device may support and which can be handled by the driver.

If someone passes an IRQF_TRIGGER_FALLING, but the hardware device only supports rising edge
interrupts the driver can error verbose.

Drivers that doesn't set irq_type flags with request_irq() typically avoid it, since
some architectures doesn't support edge or level sensitive interrupts.
So historically seen, it was a way to keep this decision out of the driver file.

>>
>>>>>> -- which doesn't work very well when coupled with module
> drivers.
>>>>>
>>>>> I don't quite get what you mean here.
>>>>
>>>> Since the driver doesn't setup the irq_type itself you need to do it
>>>> manually in advance using set_irq_type(). This happens most likely
>>>> from your paltform setup/configuration file.
>>>>
>>>> Assuming the driver is built as a module, this code gets executed
>>>> unconditionally, no matter if the driver gets finally loaded or not.
>>>>
>>>> Assuming you have several drivers build as modules, using the
>>>> same irq but with different irq types, you run into problems here.
>>>
>>> I do not get why.
>>>
>>> You're not going to register several I2C devices using the same
>>> IRQ but requiring different IRQ flags anyway, as it wouldn't work.
>>> So you'll have to only register the I2C devices which are actually
>>> present on the system. Setting the IRQ type at the same time or
>>> deferring this action to the driver(s) doesn't make any difference
> then, does it?
>>
>> If I remember correctly i2c as well as spi doesn't check for irq
>> conflicts in irqs passed with struct i2c/spi_board_info. So you can
>> have multiple drivers build as modules all using the same irq number
> but with different flags.
>
> Indeed i2c doesn't check the IRQs at all, it passes them transparently
> from platform data to device driver.
>
> But you're mixing devices and drivers. Devices are bound to an IRQ,
> not drivers, and so do IRQ flags.

I know the difference. But I'm talking here about different drivers.
Think about a system where you have different hardware options, all handled by the same
kernel image. Option a) modprobe driver_a, Option b) modprobe driver_b.
Both devices bound to driver_a and driver_b, both use IRQ_XY. However a) requires IRQF_TRIGGER_FALLING,
while b) needs IRQF_LEVEL_LOW.

To me is sounds reasonable to set irq_type only if the request_irq() succeeds.

>>
>> The user decides which add-on module is plugged onto the processor
>> board, by loading the appropriate driver module.
>
> This can't work, sorry. Unless you are using I2C device
> auto-detection, which I seem to understand is NOT used in the embedded
> world at all, loading a driver module doesn't do anything if a device
> it supports hasn't been instantiated first. So the user can't just
> load a driver and see the required I2C devices magically appear for
> the driver to bind to them.

That's not what I'm saying here.

> So the first problem you have to solve is how you instantiate the
> right I2C devices depending on which extension is plugged in. As long
> as this problem isn't solved, there's no point in discussing where the
> IRQ flags should be set.

i2c_register_board_info()/spi_register_board_info() may provide resources for all
devices potentially used with your system.
They doesn't necessarily need to be physically present,
nor does the driver they belong to need to be loaded.

However when loading module a) or b) I would expect that the system uses the right irq_type for
The device bound to a/b).

>>>> There are some development boards featuring extension options,
>>>> which all plug on the same socket but required different
> drivers/irq types.
>>>
>>> How do you figure out which extension option is plugged? You can't
>>> instantiate an I2C device which isn't present, so you must have a
>>> way to know what extension option is used, to instantiate the
>>> right I2C device at the right address.
>>
>> The user decides. The platform code provides resources for all
>> potential boards being used. And here is exactly the conflict.
>
> "Provides", how? It can't possibly instantiate them all. Please
> describe the mechanism.
>
>>>> The ideal way is therefore to pass the irq_flags together with
>>>> the
>>> SPI/I2C board info.
>>>
>>> All I see so far is two data structures made slightly larger, for no
>>> actual benefit.
>>
>> I don't see a reason why i2c/spi bus drivers should be different from
>> other bus drivers. The platform_device bus driver allows you to pass
>> IORESOURCE_IRQ | IORESOURCE_IRQ_LOWLEVEL with flags in struct resource.
>
> This is an interesting point. David, you're the one who added irq to
> struct i2c_client, you didn't add irq_flags then, did you have a good
> reason not to?
>
>> There are drivers that work around this deficiency, by adding
>> irq_flags to the bus clients dev.platform_data See
> include/linux/spi/ads7846.h for one example.
>
> Interesting example, better matching the initial comment that came
> along with the patch. But it also seems to be an isolated case, as I
> can't find any other irq_flags in include/linux/{i2c,spi}/.

I think David agrees, that this is an old, and from time to time
reoccurring discussion. And if I remember correctly David is a advocate
of the unconditional set_irq_type() methodology.

Adding these four irq_type bytes, may ultimately stop it, now and forever.

Greetings,
Michael

Analog Devices GmbH      Wilhelm-Wagenfeld-Str. 6      80807 Muenchen
Sitz der Gesellschaft Muenchen, Registergericht Muenchen HRB 4036 Geschaeftsfuehrer Thomas Wessel, William A. Martin, Margaret Seif

Re: [Device-drivers-devel] [PATCH] i2c: add irq_flags to board info

[Mike Frysinger]

On Mon, Oct 18, 2010 at 10:33, Jean Delvare wrote:
> Why do we have set_irq_type() if we're not supposed to call it? I am
> not claiming to be an expert in the area, but it seems totally
> reasonable to me that the same piece of code instantiating an I2C
> device is also responsible for setting its IRQ type.

but we're back to the same issue mentioned earlier -- you cant have a
single kernel build with modules supporting multiple drivers
simultaneously.  we like to ship development boards with a single
kernel build on it with many modules.  then people can pick the addon
boards they wish to prototype with at runtime by plugging in the card
and loading the module.

if the only way to change flags on a pin is via set_irq_type(), that
kernel build is instantly tied to whatever device you've decided to
compile statically into the board's init code.  so now to prototype
multiple boards, we have to tell people to boot different kernels !?
and we have to store multiple kernels and sets of kernel modules in
the rootfs !?

>> The user decides which add-on module is plugged onto the processor board,
>> by loading the appropriate driver module.
>
> This can't work, sorry. Unless you are using I2C device auto-detection,
> which I seem to understand is NOT used in the embedded world at all,
> loading a driver module doesn't do anything if a device it supports
> hasn't been instantiated first. So the user can't just load a driver
> and see the required I2C devices magically appear for the driver to
> bind to them.

why cant user selection work ?  the user picks `modprobe foo` because
they have a 'foo' device plugged in.

i'm not sure what "I2C device auto-detection" means.  are you talking
about a userspace app/daemon that scans the I2C bus, looks up the
slave ids in the module db, and then automatically loads the modules
that it finds bindings for ?

>> There are drivers that work around this deficiency, by adding irq_flags to the bus
>> clients dev.platform_data.  See include/linux/spi/ads7846.h for one example.
>
> Interesting example, better matching the initial comment that came
> along with the patch. But it also seems to be an isolated case, as I
> can't find any other irq_flags in include/linux/{i2c,spi}/.

we've been holding off posting things because we need the core extended first
-mike

Re: [PATCH] i2c: add irq_flags to board info

[Mark Brown]

On Mon, Oct 18, 2010 at 10:36:10AM +0200, Jean Delvare wrote:
> On Sun, 17 Oct 2010 19:43:39 -0400, Mike Frysinger wrote:

> > This allows us to avoid having to add dedicated platform init code just
> > to call set_irq_type()

> Do you have examples of this? Can we see a preview of the amount of
> cleanups this patch would allow?

wm831x has platform data for this.  It doesn't actually call
set_irq_type(), it parses the platform data before calling
request_threaded_irq(), but the principle is the same.

Re: [Device-drivers-devel] [PATCH] i2c: add irq_flags to board info

[Ben Dooks]

On Mon, Oct 18, 2010 at 03:51:49PM -0400, Mike Frysinger wrote:
> On Mon, Oct 18, 2010 at 10:33, Jean Delvare wrote:
> > Why do we have set_irq_type() if we're not supposed to call it? I am
> > not claiming to be an expert in the area, but it seems totally
> > reasonable to me that the same piece of code instantiating an I2C
> > device is also responsible for setting its IRQ type.
> 
> but we're back to the same issue mentioned earlier -- you cant have a
> single kernel build with modules supporting multiple drivers
> simultaneously.  we like to ship development boards with a single
> kernel build on it with many modules.  then people can pick the addon
> boards they wish to prototype with at runtime by plugging in the card
> and loading the module.

I also dislike set_irq_type() as it doesn't check whether there is anyone
registered with the interrupt, which means that you could set the irq
type of someone else's irq.

I wonder if we should pass a struct resource instead, in case there
are multiple interrupt sources, as well as having it registered with
the right resource systems.

-- 
Ben

Q:      What's a light-year?
A:      One-third less calories than a regular year.

Re: [Device-drivers-devel] [PATCH] i2c: add irq_flags to board info

[Jonathan Cameron]

On 10/25/10 01:45, Ben Dooks wrote:
> On Mon, Oct 18, 2010 at 03:51:49PM -0400, Mike Frysinger wrote:
>> On Mon, Oct 18, 2010 at 10:33, Jean Delvare wrote:
>>> Why do we have set_irq_type() if we're not supposed to call it? I am
>>> not claiming to be an expert in the area, but it seems totally
>>> reasonable to me that the same piece of code instantiating an I2C
>>> device is also responsible for setting its IRQ type.
>>
>> but we're back to the same issue mentioned earlier -- you cant have a
>> single kernel build with modules supporting multiple drivers
>> simultaneously.  we like to ship development boards with a single
>> kernel build on it with many modules.  then people can pick the addon
>> boards they wish to prototype with at runtime by plugging in the card
>> and loading the module.
> 
> I also dislike set_irq_type() as it doesn't check whether there is anyone
> registered with the interrupt, which means that you could set the irq
> type of someone else's irq.
> 
> I wonder if we should pass a struct resource instead, in case there
> are multiple interrupt sources, as well as having it registered with
> the right resource systems.
> 
Either works as far as I am concerned. Having seen a large set of drivers
using the flags option (posted to linux-iio yesterday) I'm definitely convinced
some means of allowing devices to match what the board config asks for is useful.

I personally prefer the struct resource option as I have multiple drivers in IIO
which have two interrupts and this is the only reason some of them use platform
data.

Thanks,

Jonathan

Re: [Device-drivers-devel] [PATCH] i2c: add irq_flags to board info

[Mike Frysinger]

On Sun, Oct 24, 2010 at 20:45, Ben Dooks wrote:
> On Mon, Oct 18, 2010 at 03:51:49PM -0400, Mike Frysinger wrote:
>> On Mon, Oct 18, 2010 at 10:33, Jean Delvare wrote:
>> > Why do we have set_irq_type() if we're not supposed to call it? I am
>> > not claiming to be an expert in the area, but it seems totally
>> > reasonable to me that the same piece of code instantiating an I2C
>> > device is also responsible for setting its IRQ type.
>>
>> but we're back to the same issue mentioned earlier -- you cant have a
>> single kernel build with modules supporting multiple drivers
>> simultaneously.  we like to ship development boards with a single
>> kernel build on it with many modules.  then people can pick the addon
>> boards they wish to prototype with at runtime by plugging in the card
>> and loading the module.
>
> I also dislike set_irq_type() as it doesn't check whether there is anyone
> registered with the interrupt, which means that you could set the irq
> type of someone else's irq.
>
> I wonder if we should pass a struct resource instead, in case there
> are multiple interrupt sources, as well as having it registered with
> the right resource systems.

a resource struct would still only have a single interrupt info in it,
so i'm not sure how that would be better
-mike