[RFC,PATCH] i2c: fix device_init_wakeup place - Kernel

This is a discussion on [RFC,PATCH] i2c: fix device_init_wakeup place - Kernel ; Move device_init_wakeup. At it's current place this is a noop (will be reset in device_initialize). Signed-off-by: Marc Pignat --- 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 ...

+ Reply to Thread
Results 1 to 4 of 4

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

  1. [RFC,PATCH] i2c: fix device_init_wakeup place

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

    Signed-off-by: Marc Pignat
    ---

    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;

    --
    To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
    the body of a message to majordomo@vger.kernel.org
    More majordomo info at http://vger.kernel.org/majordomo-info.html
    Please read the FAQ at http://www.tux.org/lkml/

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

    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
    > ---
    >
    > 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
    --
    To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
    the body of a message to majordomo@vger.kernel.org
    More majordomo info at http://vger.kernel.org/majordomo-info.html
    Please read the FAQ at http://www.tux.org/lkml/

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

    > > 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;
    > >

    >

    --
    To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
    the body of a message to majordomo@vger.kernel.org
    More majordomo info at http://vger.kernel.org/majordomo-info.html
    Please read the FAQ at http://www.tux.org/lkml/

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

    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
    --
    To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
    the body of a message to majordomo@vger.kernel.org
    More majordomo info at http://vger.kernel.org/majordomo-info.html
    Please read the FAQ at http://www.tux.org/lkml/

+ Reply to Thread