Hi Wolfram, On Wed, Aug 19, 2015 at 10:43 AM, Wolfram Sang <wsa@xxxxxxxxxxxxx> wrote: > >> > > > @@ -659,20 +662,47 @@ static int i2c_device_probe(struct device *dev) >> > > > if (!device_can_wakeup(&client->dev)) >> > > > device_init_wakeup(&client->dev, >> > > > client->flags & I2C_CLIENT_WAKE); >> > > >> > > I was about to ask if we couldn't combine this and the later if-blocks >> > > with an if-else combination. But now I stumble over the above block in >> > > general: If the device cannot cause wake ups, then we might initialize >> > > it as a wakeup-device depending on client->flags?? >> > >> > I believe it is done so that we do not try to re-add wakeup source after >> > unbinding/rebinding the device. With my patch we clearing wakeup flag on >> > unbind, so it is OK, but there is still error path where we might want >> > to reset the wakeup flag as well. >> >> I was wondering if it wants to achieve that, why does it not >> unconditionally use 0 instead of the WAKE flag. > > When reviewing V2, I wasn't comfortable with just guessing what the old > code means. So, I did some digging and found: > > https://lkml.org/lkml/2008/8/10/204 > > Quoting the interesting paragraph from David Brownell: > > === > > 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. > >> > + device_init_wakeup(&client->dev, client->flags & >> > I2C_CLIENT_WAKE); > > === > > I have to admit that I am not familiar with device wakeup handling and > especially its userspace policies. Can you double check that your V2 > meets the above intention? No it does not; it explicitly resets the wakeup flag. Note that the original code was not quite right in that regard either: it would preserve wakeup flag set by userspace upon driver rebinding; but it would re-arm the wakeup flag if it was disabled by userspace. I believe that resetting the flag upon re-binding the driver is proper behavior as the driver is responsible for setting up and handling wakeups. Thanks. -- Dmitry -- To unsubscribe from this list: send the line "unsubscribe linux-i2c" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
