Hi Rafael, many thanks for looking at this.
2011/7/10 Rafael J. Wysocki <rjw@xxxxxxx>:
>> @@ -1602,6 +1611,15 @@ static int input_dev_resume(struct device *dev)
>> {
>> struct input_dev *input_dev = to_input_dev(dev);
>>
>> + /*
>> + * If this device is a child of serio, which is a child of i8042, and
>> + * i8042 wakeup is enabled (i.e. this input device was not suspended),
>> + * do nothing.
>> + */
>> + if (dev->parent && dev->parent->parent
>> + && device_may_wakeup(dev->parent->parent))
>> + return 0;
>> +
>
> You check exactly the same condition in two places with very similar comments.
> I'd put it into a bool function and add a kerneldoc description to it instead.
OK, I agree. I was hoping that you'd be able to point out a better way
of doing this though - I think you'd agree that this code is not very
nice. But maybe explaining it better and hiding it away in its own
function is the best approach we have.
It is a strange case, because the device hierarchy is as follows:
i8042 -> serio -> input
Yet the kernel sees the i8042 and input devices as somewhat
independent devices on different busses, so each one has its own
independent suspend/resume implementation called from the PM layer.
And in this particular case, I need to make the i8042 controller
communicate the fact that its grandchild input device should not be
suspended or reset.
> The fact that you need provide several empty definitions of
> i8042_platform_suspend() is a bit worrisome. Have you considered using a
> different approach?
This seems quite the norm in i8042-land - those .h files already
include a number of empty functions that were presumably added just
because a small selection of the other supported arches needed to do
something. Nevertheless I'd be happy to convert these drivers to a new
"struct i8042_platform_ops" type system if that helps with the merge
of this patch.
Thanks,
Daniel
--
To unsubscribe from this list: send the line "unsubscribe linux-input" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
