| Message ID | 916d5a5b0df5dbe14283ddb31f1128efefbfd1e2.1467875143.git.lv.zheng@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Thu, Jul 7, 2016 at 9:10 AM, Lv Zheng <lv.zheng@intel.com> wrote: > There are many AML tables reporting wrong initial lid state, and some of > them never reports lid state. As a proxy layer acting between, ACPI button > driver is not able to handle all such cases, but need to re-define the > usage model of the ACPI lid. That is: > 1. It's initial state is not reliable; > 2. There may not be open event; > 3. Userspace should only take action against the close event which is > reliable, always sent after a real lid close. > This patch adds a new input key event so that new userspace programs can > use it to handle this usage model correctly. And in the meanwhile, no old > programs will be broken by the userspace changes. > > Link: https://lkml.org/2016/3/7/460 > Link: https://github.com/systemd/systemd/issues/2087 > Signed-off-by: Lv Zheng <lv.zheng@intel.com> > Cc: Bastien Nocera: <hadess@hadess.net> > Cc: Benjamin Tissoires <benjamin.tissoires@gmail.com> > Cc: linux-input@vger.kernel.org > --- > drivers/acpi/button.c | 20 ++++++++++++++------ > include/linux/mod_devicetable.h | 2 +- > include/uapi/linux/input-event-codes.h | 3 ++- > 3 files changed, 17 insertions(+), 8 deletions(-) > > diff --git a/drivers/acpi/button.c b/drivers/acpi/button.c > index 148f4e5..4ef94d2 100644 > --- a/drivers/acpi/button.c > +++ b/drivers/acpi/button.c > @@ -130,7 +130,8 @@ static int acpi_lid_evaluate_state(struct acpi_device *device) > return lid_state ? 1 : 0; > } > > -static int acpi_lid_notify_state(struct acpi_device *device, int state) > +static int acpi_lid_notify_state(struct acpi_device *device, > + int state, bool notify_acpi) > { > struct acpi_button *button = acpi_driver_data(device); > int ret; > @@ -138,6 +139,11 @@ static int acpi_lid_notify_state(struct acpi_device *device, int state) > /* input layer checks if event is redundant */ > input_report_switch(button->input, SW_LID, !state); > input_sync(button->input); > + if (notify_acpi) { > + input_report_switch(button->input, > + SW_ACPI_LID, !state); > + input_sync(button->input); If you use a switch, you'll never send subsequent open state if you doesn't close it yourself. See my comments in 5/5 and please use a KEY event instead. > + } > > if (state) > pm_wakeup_event(&device->dev, 0); > @@ -279,7 +285,8 @@ int acpi_lid_open(void) > } > EXPORT_SYMBOL(acpi_lid_open); > > -static int acpi_lid_update_state(struct acpi_device *device) > +static int acpi_lid_update_state(struct acpi_device *device, > + bool notify_acpi) > { > int state; > > @@ -287,17 +294,17 @@ static int acpi_lid_update_state(struct acpi_device *device) > if (state < 0) > return state; > > - return acpi_lid_notify_state(device, state); > + return acpi_lid_notify_state(device, state, notify_acpi); > } > > static void acpi_lid_initialize_state(struct acpi_device *device) > { > switch (lid_init_state) { > case ACPI_BUTTON_LID_INIT_OPEN: > - (void)acpi_lid_notify_state(device, 1); > + (void)acpi_lid_notify_state(device, 1, false); > break; > case ACPI_BUTTON_LID_INIT_METHOD: > - (void)acpi_lid_update_state(device); > + (void)acpi_lid_update_state(device, false); > break; > case ACPI_BUTTON_LID_INIT_IGNORE: > default: > @@ -317,7 +324,7 @@ static void acpi_button_notify(struct acpi_device *device, u32 event) > case ACPI_BUTTON_NOTIFY_STATUS: > input = button->input; > if (button->type == ACPI_BUTTON_TYPE_LID) { > - acpi_lid_update_state(device); > + acpi_lid_update_state(device, true); > } else { > int keycode; > > @@ -436,6 +443,7 @@ static int acpi_button_add(struct acpi_device *device) > > case ACPI_BUTTON_TYPE_LID: > input_set_capability(input, EV_SW, SW_LID); > + input_set_capability(input, EV_SW, SW_ACPI_LID); Can't we export this new event only if the _LID function is not reliable? This could check for the module parameter lid_init_state and only enable it for ACPI_BUTTON_LID_INIT_OPEN. I really hope we will be able to find a reliable way to determine whether or not the platform support reliable LID state. If not, there might be a need to have a db of reliable switch platforms. This can be set in the kernel or with a hwdb entry in userspace. Cheers, Benjamin > break; > } > > diff --git a/include/linux/mod_devicetable.h b/include/linux/mod_devicetable.h > index 6e4c645..1014968 100644 > --- a/include/linux/mod_devicetable.h > +++ b/include/linux/mod_devicetable.h > @@ -291,7 +291,7 @@ struct pcmcia_device_id { > #define INPUT_DEVICE_ID_LED_MAX 0x0f > #define INPUT_DEVICE_ID_SND_MAX 0x07 > #define INPUT_DEVICE_ID_FF_MAX 0x7f > -#define INPUT_DEVICE_ID_SW_MAX 0x0f > +#define INPUT_DEVICE_ID_SW_MAX 0x10 > > #define INPUT_DEVICE_ID_MATCH_BUS 1 > #define INPUT_DEVICE_ID_MATCH_VENDOR 2 > diff --git a/include/uapi/linux/input-event-codes.h b/include/uapi/linux/input-event-codes.h > index 737fa32..81c344c 100644 > --- a/include/uapi/linux/input-event-codes.h > +++ b/include/uapi/linux/input-event-codes.h > @@ -780,7 +780,8 @@ > #define SW_ROTATE_LOCK 0x0c /* set = rotate locked/disabled */ > #define SW_LINEIN_INSERT 0x0d /* set = inserted */ > #define SW_MUTE_DEVICE 0x0e /* set = device disabled */ > -#define SW_MAX 0x0f > +#define SW_ACPI_LID 0x0f /* set = lid shut */ > +#define SW_MAX 0x10 > #define SW_CNT (SW_MAX+1) > > /* > -- > 1.7.10 > -- To unsubscribe from this list: send the line "unsubscribe linux-input" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Fri, Jul 08, 2016 at 11:27:23AM +0200, Benjamin Tissoires wrote: > On Thu, Jul 7, 2016 at 9:10 AM, Lv Zheng <lv.zheng@intel.com> wrote: > > There are many AML tables reporting wrong initial lid state, and some of > > them never reports lid state. As a proxy layer acting between, ACPI button > > driver is not able to handle all such cases, but need to re-define the > > usage model of the ACPI lid. That is: > > 1. It's initial state is not reliable; > > 2. There may not be open event; > > 3. Userspace should only take action against the close event which is > > reliable, always sent after a real lid close. > > This patch adds a new input key event so that new userspace programs can > > use it to handle this usage model correctly. And in the meanwhile, no old > > programs will be broken by the userspace changes. > > > > Link: https://lkml.org/2016/3/7/460 Does not work for me. > > Link: https://github.com/systemd/systemd/issues/2087 Gives me info about pull "Basic DNSSEC support, and unrelated fixes" > > Signed-off-by: Lv Zheng <lv.zheng@intel.com> > > Cc: Bastien Nocera: <hadess@hadess.net> > > Cc: Benjamin Tissoires <benjamin.tissoires@gmail.com> > > Cc: linux-input@vger.kernel.org > > --- > > drivers/acpi/button.c | 20 ++++++++++++++------ > > include/linux/mod_devicetable.h | 2 +- > > include/uapi/linux/input-event-codes.h | 3 ++- > > 3 files changed, 17 insertions(+), 8 deletions(-) > > > > diff --git a/drivers/acpi/button.c b/drivers/acpi/button.c > > index 148f4e5..4ef94d2 100644 > > --- a/drivers/acpi/button.c > > +++ b/drivers/acpi/button.c > > @@ -130,7 +130,8 @@ static int acpi_lid_evaluate_state(struct acpi_device *device) > > return lid_state ? 1 : 0; > > } > > > > -static int acpi_lid_notify_state(struct acpi_device *device, int state) > > +static int acpi_lid_notify_state(struct acpi_device *device, > > + int state, bool notify_acpi) > > { > > struct acpi_button *button = acpi_driver_data(device); > > int ret; > > @@ -138,6 +139,11 @@ static int acpi_lid_notify_state(struct acpi_device *device, int state) > > /* input layer checks if event is redundant */ > > input_report_switch(button->input, SW_LID, !state); > > input_sync(button->input); > > + if (notify_acpi) { > > + input_report_switch(button->input, > > + SW_ACPI_LID, !state); > > + input_sync(button->input); > > If you use a switch, you'll never send subsequent open state if you > doesn't close it yourself. > See my comments in 5/5 and please use a KEY event instead. > > > + } > > > > if (state) > > pm_wakeup_event(&device->dev, 0); > > @@ -279,7 +285,8 @@ int acpi_lid_open(void) > > } > > EXPORT_SYMBOL(acpi_lid_open); > > > > -static int acpi_lid_update_state(struct acpi_device *device) > > +static int acpi_lid_update_state(struct acpi_device *device, > > + bool notify_acpi) > > { > > int state; > > > > @@ -287,17 +294,17 @@ static int acpi_lid_update_state(struct acpi_device *device) > > if (state < 0) > > return state; > > > > - return acpi_lid_notify_state(device, state); > > + return acpi_lid_notify_state(device, state, notify_acpi); > > } > > > > static void acpi_lid_initialize_state(struct acpi_device *device) > > { > > switch (lid_init_state) { > > case ACPI_BUTTON_LID_INIT_OPEN: > > - (void)acpi_lid_notify_state(device, 1); > > + (void)acpi_lid_notify_state(device, 1, false); > > break; > > case ACPI_BUTTON_LID_INIT_METHOD: > > - (void)acpi_lid_update_state(device); > > + (void)acpi_lid_update_state(device, false); > > break; > > case ACPI_BUTTON_LID_INIT_IGNORE: > > default: > > @@ -317,7 +324,7 @@ static void acpi_button_notify(struct acpi_device *device, u32 event) > > case ACPI_BUTTON_NOTIFY_STATUS: > > input = button->input; > > if (button->type == ACPI_BUTTON_TYPE_LID) { > > - acpi_lid_update_state(device); > > + acpi_lid_update_state(device, true); > > } else { > > int keycode; > > > > @@ -436,6 +443,7 @@ static int acpi_button_add(struct acpi_device *device) > > > > case ACPI_BUTTON_TYPE_LID: > > input_set_capability(input, EV_SW, SW_LID); > > + input_set_capability(input, EV_SW, SW_ACPI_LID); > > Can't we export this new event only if the _LID function is not > reliable? This could check for the module parameter lid_init_state and > only enable it for ACPI_BUTTON_LID_INIT_OPEN. > > I really hope we will be able to find a reliable way to determine > whether or not the platform support reliable LID state. If not, there > might be a need to have a db of reliable switch platforms. This can be > set in the kernel or with a hwdb entry in userspace. > > Cheers, > Benjamin > > > break; > > } > > > > diff --git a/include/linux/mod_devicetable.h b/include/linux/mod_devicetable.h > > index 6e4c645..1014968 100644 > > --- a/include/linux/mod_devicetable.h > > +++ b/include/linux/mod_devicetable.h > > @@ -291,7 +291,7 @@ struct pcmcia_device_id { > > #define INPUT_DEVICE_ID_LED_MAX 0x0f > > #define INPUT_DEVICE_ID_SND_MAX 0x07 > > #define INPUT_DEVICE_ID_FF_MAX 0x7f > > -#define INPUT_DEVICE_ID_SW_MAX 0x0f > > +#define INPUT_DEVICE_ID_SW_MAX 0x10 > > > > #define INPUT_DEVICE_ID_MATCH_BUS 1 > > #define INPUT_DEVICE_ID_MATCH_VENDOR 2 > > diff --git a/include/uapi/linux/input-event-codes.h b/include/uapi/linux/input-event-codes.h > > index 737fa32..81c344c 100644 > > --- a/include/uapi/linux/input-event-codes.h > > +++ b/include/uapi/linux/input-event-codes.h > > @@ -780,7 +780,8 @@ > > #define SW_ROTATE_LOCK 0x0c /* set = rotate locked/disabled */ > > #define SW_LINEIN_INSERT 0x0d /* set = inserted */ > > #define SW_MUTE_DEVICE 0x0e /* set = device disabled */ > > -#define SW_MAX 0x0f > > +#define SW_ACPI_LID 0x0f /* set = lid shut */ 0x0f is busy already. > > +#define SW_MAX 0x10 > > #define SW_CNT (SW_MAX+1) > > > > /* > > -- > > 1.7.10 > >
diff --git a/drivers/acpi/button.c b/drivers/acpi/button.c index 148f4e5..4ef94d2 100644 --- a/drivers/acpi/button.c +++ b/drivers/acpi/button.c @@ -130,7 +130,8 @@ static int acpi_lid_evaluate_state(struct acpi_device *device) return lid_state ? 1 : 0; } -static int acpi_lid_notify_state(struct acpi_device *device, int state) +static int acpi_lid_notify_state(struct acpi_device *device, + int state, bool notify_acpi) { struct acpi_button *button = acpi_driver_data(device); int ret; @@ -138,6 +139,11 @@ static int acpi_lid_notify_state(struct acpi_device *device, int state) /* input layer checks if event is redundant */ input_report_switch(button->input, SW_LID, !state); input_sync(button->input); + if (notify_acpi) { + input_report_switch(button->input, + SW_ACPI_LID, !state); + input_sync(button->input); + } if (state) pm_wakeup_event(&device->dev, 0); @@ -279,7 +285,8 @@ int acpi_lid_open(void) } EXPORT_SYMBOL(acpi_lid_open); -static int acpi_lid_update_state(struct acpi_device *device) +static int acpi_lid_update_state(struct acpi_device *device, + bool notify_acpi) { int state; @@ -287,17 +294,17 @@ static int acpi_lid_update_state(struct acpi_device *device) if (state < 0) return state; - return acpi_lid_notify_state(device, state); + return acpi_lid_notify_state(device, state, notify_acpi); } static void acpi_lid_initialize_state(struct acpi_device *device) { switch (lid_init_state) { case ACPI_BUTTON_LID_INIT_OPEN: - (void)acpi_lid_notify_state(device, 1); + (void)acpi_lid_notify_state(device, 1, false); break; case ACPI_BUTTON_LID_INIT_METHOD: - (void)acpi_lid_update_state(device); + (void)acpi_lid_update_state(device, false); break; case ACPI_BUTTON_LID_INIT_IGNORE: default: @@ -317,7 +324,7 @@ static void acpi_button_notify(struct acpi_device *device, u32 event) case ACPI_BUTTON_NOTIFY_STATUS: input = button->input; if (button->type == ACPI_BUTTON_TYPE_LID) { - acpi_lid_update_state(device); + acpi_lid_update_state(device, true); } else { int keycode; @@ -436,6 +443,7 @@ static int acpi_button_add(struct acpi_device *device) case ACPI_BUTTON_TYPE_LID: input_set_capability(input, EV_SW, SW_LID); + input_set_capability(input, EV_SW, SW_ACPI_LID); break; } diff --git a/include/linux/mod_devicetable.h b/include/linux/mod_devicetable.h index 6e4c645..1014968 100644 --- a/include/linux/mod_devicetable.h +++ b/include/linux/mod_devicetable.h @@ -291,7 +291,7 @@ struct pcmcia_device_id { #define INPUT_DEVICE_ID_LED_MAX 0x0f #define INPUT_DEVICE_ID_SND_MAX 0x07 #define INPUT_DEVICE_ID_FF_MAX 0x7f -#define INPUT_DEVICE_ID_SW_MAX 0x0f +#define INPUT_DEVICE_ID_SW_MAX 0x10 #define INPUT_DEVICE_ID_MATCH_BUS 1 #define INPUT_DEVICE_ID_MATCH_VENDOR 2 diff --git a/include/uapi/linux/input-event-codes.h b/include/uapi/linux/input-event-codes.h index 737fa32..81c344c 100644 --- a/include/uapi/linux/input-event-codes.h +++ b/include/uapi/linux/input-event-codes.h @@ -780,7 +780,8 @@ #define SW_ROTATE_LOCK 0x0c /* set = rotate locked/disabled */ #define SW_LINEIN_INSERT 0x0d /* set = inserted */ #define SW_MUTE_DEVICE 0x0e /* set = device disabled */ -#define SW_MAX 0x0f +#define SW_ACPI_LID 0x0f /* set = lid shut */ +#define SW_MAX 0x10 #define SW_CNT (SW_MAX+1) /*
There are many AML tables reporting wrong initial lid state, and some of them never reports lid state. As a proxy layer acting between, ACPI button driver is not able to handle all such cases, but need to re-define the usage model of the ACPI lid. That is: 1. It's initial state is not reliable; 2. There may not be open event; 3. Userspace should only take action against the close event which is reliable, always sent after a real lid close. This patch adds a new input key event so that new userspace programs can use it to handle this usage model correctly. And in the meanwhile, no old programs will be broken by the userspace changes. Link: https://lkml.org/2016/3/7/460 Link: https://github.com/systemd/systemd/issues/2087 Signed-off-by: Lv Zheng <lv.zheng@intel.com> Cc: Bastien Nocera: <hadess@hadess.net> Cc: Benjamin Tissoires <benjamin.tissoires@gmail.com> Cc: linux-input@vger.kernel.org --- drivers/acpi/button.c | 20 ++++++++++++++------ include/linux/mod_devicetable.h | 2 +- include/uapi/linux/input-event-codes.h | 3 ++- 3 files changed, 17 insertions(+), 8 deletions(-)