[RFC,PATCH] i2c: fix device_init_wakeup place

[RFC,PATCH] i2c: fix device_init_wakeup place

[Marc Pignat]

Move device_init_wakeup. At it's current place this is a noop (will be reset in
device_initialize).

Signed-off-by: Marc Pignat <marc.pignat@hevs.ch>
---

Hi all!

The current code calls device_init_wakeup() before device_register, but
device_register will disable device wakeup.

This patch (against 2.6.27-rc1) move the device_init_wakeup() call in the bus
probe function,just before the probe call.

This will fix 3bbb835d4c53faf0bca62f0e39835926bef40b1f ('New style devices can
support driver modle wakeup flags')which in fact has no effect. I think there
is no need to fix -stable, because there is no in-tree users of the
I2C_CLIENT_WAKE flag.

This patch also include a small functionnal change: the I2C_CLIENT_WAKE is no
more removed from the client flags, but this should't hurt.


Best regards

Marc



diff --git a/drivers/i2c/i2c-core.c b/drivers/i2c/i2c-core.c
index 7bf38c4..99c6a13 100644
--- a/drivers/i2c/i2c-core.c
+++ b/drivers/i2c/i2c-core.c
@@ -108,6 +108,7 @@ static int i2c_device_probe(struct device *dev)
 	if (!driver->probe || !driver->id_table)
 		return -ENODEV;
 	client->driver = driver;
+	device_init_wakeup(&client->dev, client->flags & I2C_CLIENT_WAKE);
 	dev_dbg(dev, "probe\n");
 
 	status = driver->probe(client, i2c_match_id(driver->id_table, client));
@@ -262,9 +263,8 @@ i2c_new_device(struct i2c_adapter *adap, struct i2c_board_info const *info)
 	client->adapter = adap;
 
 	client->dev.platform_data = info->platform_data;
-	device_init_wakeup(&client->dev, info->flags & I2C_CLIENT_WAKE);
 
-	client->flags = info->flags & ~I2C_CLIENT_WAKE;
+	client->flags = info->flags;
 	client->addr = info->addr;
 	client->irq = info->irq;
 

Re: [RFC,PATCH] i2c: fix device_init_wakeup place

[Jean Delvare]

Hi Marc,

On Tue, 29 Jul 2008 15:35:44 +0200, Marc Pignat wrote:
> Move device_init_wakeup. At it's current place this is a noop (will be reset in
> device_initialize).
> 
> Signed-off-by: Marc Pignat <marc.pignat@hevs.ch>
> ---
> 
> Hi all!
> 
> The current code calls device_init_wakeup() before device_register, but
> device_register will disable device wakeup.
> 
> This patch (against 2.6.27-rc1) move the device_init_wakeup() call in the bus
> probe function,just before the probe call.

David, can you please help? This part of the code is yours and I have
no idea how it works. I am surprised that according to Marc the code
simply doesn't work at the moment. Didn't you test it?

> This will fix 3bbb835d4c53faf0bca62f0e39835926bef40b1f ('New style devices can
> support driver modle wakeup flags')which in fact has no effect. I think there
> is no need to fix -stable, because there is no in-tree users of the
> I2C_CLIENT_WAKE flag.

Interesting point... David, what are you using this code for then? Same
question for you Marc.

> This patch also include a small functionnal change: the I2C_CLIENT_WAKE is no
> more removed from the client flags, but this should't hurt.

Why do you want to change this?

> 
> 
> Best regards
> 
> Marc
> 
> 
> 
> diff --git a/drivers/i2c/i2c-core.c b/drivers/i2c/i2c-core.c
> index 7bf38c4..99c6a13 100644
> --- a/drivers/i2c/i2c-core.c
> +++ b/drivers/i2c/i2c-core.c
> @@ -108,6 +108,7 @@ static int i2c_device_probe(struct device *dev)
>  	if (!driver->probe || !driver->id_table)
>  		return -ENODEV;
>  	client->driver = driver;
> +	device_init_wakeup(&client->dev, client->flags & I2C_CLIENT_WAKE);
>  	dev_dbg(dev, "probe\n");
>  
>  	status = driver->probe(client, i2c_match_id(driver->id_table, client));
> @@ -262,9 +263,8 @@ i2c_new_device(struct i2c_adapter *adap, struct i2c_board_info const *info)
>  	client->adapter = adap;
>  
>  	client->dev.platform_data = info->platform_data;
> -	device_init_wakeup(&client->dev, info->flags & I2C_CLIENT_WAKE);
>  
> -	client->flags = info->flags & ~I2C_CLIENT_WAKE;
> +	client->flags = info->flags;
>  	client->addr = info->addr;
>  	client->irq = info->irq;
>  


-- 
Jean Delvare

Re: [RFC,PATCH] i2c: fix device_init_wakeup place

[David Brownell]

> > The current code calls device_init_wakeup() before device_register, but
> > device_register will disable device wakeup.
> > 
> > This patch (against 2.6.27-rc1) move the device_init_wakeup() call in
> > the bus probe function,just before the probe call.
>
> David, can you please help? This part of the code is yours and I have
> no idea how it works.

Patch looked OK to me, except for the one comment below.

The root cause of the bug could be blamed on the new-style code
recycling the legacy i2c_attach_client() code instead of taking
a more driver-model-ish approach.

That forced use of device_register(), instead of device_add() ...
and register() always zero-initializes those wakeup flags.  On the
other hand, add() just takes their original value.  This patch
seems like the most concise fix possible.


>	I am surprised that according to Marc the code
> simply doesn't work at the moment. Didn't you test it?

I thought I had.  The one board I had with a wake-capable I2C
device (i2c/chips/menelaus.c) seems to have an ugly workaround
for the problem this addresses ... I probably just forgot to
remove that workaround, so of course it (still) worked.

(Well, actually I have another now, but its Linux support is
still incomplete, notably power managment.  www.beagleboard.org
can't yet run with mainline kernels.)


> > This will fix 3bbb835d4c53faf0bca62f0e39835926bef40b1f ('New style
> > devices can support driver modle wakeup flags') which in fact has
> > no effect. I think there is no need to fix -stable, because there
> > is no in-tree users of the I2C_CLIENT_WAKE flag.
>
> Interesting point... David, what are you using this code for then? Same
> question for you Marc.

The goal was to make sure that wakeup flags could be initialized
sanely for I2C devices, with RTCs being the most common example.

It would be wrong to have the drivers setting such flags, since they
can't actually know when their IRQs (or whatever) are wakeup-capable.
That kind of information is specific to how any given board is wired.


> > This patch also include a small functionnal change: the I2C_CLIENT_WAKE
> > is no more removed from the client flags, but this should't hurt.
>
> Why do you want to change this?

It's kind of essential to making this patch work!  Else the flag
won't be available when the i2c core gets control of that device
node again, after device_initialize() code zeroes those flags.


> > --- a/drivers/i2c/i2c-core.c
> > +++ b/drivers/i2c/i2c-core.c
> > @@ -108,6 +108,7 @@ static int i2c_device_probe(struct device *dev)
> >  	if (!driver->probe || !driver->id_table)
> >  		return -ENODEV;
> >  	client->driver = driver;

Better would be to preserve any existing settings:

	if (!device_can_wakeup(&client->dev))
		device_init_wakeup(...)

That way the userspace policy setting is preserved unless the
device itself gets removed ... instead of being clobbered by
the simple act of (re)probing a driver.

- Dave


> > +	device_init_wakeup(&client->dev, client->flags & I2C_CLIENT_WAKE);
> >  	dev_dbg(dev, "probe\n");
> >  
> >  	status = driver->probe(client, i2c_match_id(driver->id_table, client));
> > @@ -262,9 +263,8 @@ i2c_new_device(struct i2c_adapter *adap, struct i2c_board_info const *info)
> >  	client->adapter = adap;
> >  
> >  	client->dev.platform_data = info->platform_data;
> > -	device_init_wakeup(&client->dev, info->flags & I2C_CLIENT_WAKE);
> >  
> > -	client->flags = info->flags & ~I2C_CLIENT_WAKE;
> > +	client->flags = info->flags;
> >  	client->addr = info->addr;
> >  	client->irq = info->irq;
> >  
>

Re: [RFC,PATCH] i2c: fix device_init_wakeup place

[Marc Pignat]

Hi all!

First, I would like to ask David to exuse me for saying something like "your
patch did nothing", it was crude.

David had a real good idea to add the flag I2C_CLIENT_WAKE.


On Monday 11 August 2008, David Brownell wrote:
> > > The current code calls device_init_wakeup() before device_register, but
...
> > Interesting point... David, what are you using this code for then? Same
> > question for you Marc.
...

I use it for an rtc chip (ds1374), but the board is not in tree (should be
soon).

> 
> 
> > > This patch also include a small functionnal change: the I2C_CLIENT_WAKE
> > > is no more removed from the client flags, but this should't hurt.
> >
> > Why do you want to change this?
> 
> It's kind of essential to making this patch work!  Else the flag
> won't be available when the i2c core gets control of that device
> node again, after device_initialize() code zeroes those flags.

Now the flag is used in the i2c_device_probe function and must be preserved
for future use (probe of another device or re-probe).

The downside is that it make this flag visible to the i2c_client.

To preserve the original behavior (hide the flag to the driver), the flag
should be saved, for instance in the i2c_device structure.
I think adding more code and adding a field in the i2c_device structure costs
more than making this flag visible (but if you think differently, I can fix the
patch).



> 
> 
...
> Better would be to preserve any existing settings:
> 
> 	if (!device_can_wakeup(&client->dev))
> 		device_init_wakeup(...)
> 
> That way the userspace policy setting is preserved unless the
> device itself gets removed ... instead of being clobbered by
> the simple act of (re)probing a driver.
Ok, will be fixed in version 2


Best regards

Marc