| Message ID | 179d3595-dda8-4c50-84e3-5f447ef5e34b@www.fastmail.com (mailing list archive) |
|---|---|
| State | Changes Requested, archived |
| Headers | show |
| Series | [1/2] platform/x86: system76_acpi: Add hwmon driver | expand |
> [...] > +static void input_key(struct system76_data *data, unsigned int code) > +{ > + input_report_key(data->input, code, 1); > + input_sync(data->input); > + > + input_report_key(data->input, code, 0); > + input_sync(data->input); > +} > + > // Handle ACPI notification > static void system76_notify(struct acpi_device *acpi_dev, u32 event) > { > @@ -459,6 +470,9 @@ static void system76_notify(struct acpi_device *acpi_dev, u32 event) > case 0x84: > kb_led_hotkey_color(data); > break; > + case 0x85: > + input_key(data, KEY_SCREENLOCK); > + break; > } > } > > @@ -524,6 +538,21 @@ static int system76_add(struct acpi_device *acpi_dev) > if (IS_ERR(data->therm)) > return PTR_ERR(data->therm); > > + data->input = devm_input_allocate_device(&acpi_dev->dev); > + if (!data->input) > + return -ENOMEM; > + data->input->name = "System76 ACPI Hotkeys"; > + data->input->phys = "system76_acpi/input0"; > + data->input->id.bustype = BUS_HOST; > + data->input->dev.parent = &acpi_dev->dev; > + set_bit(EV_KEY, data->input->evbit); > + set_bit(KEY_SCREENLOCK, data->input->keybit); > + err = input_register_device(data->input); > + if (err) { > + input_free_device(data->input); > + return err; > + } > + > return 0; > } Hi, wouldn't sparse_keymap be a better choice here? Barnabás Pőcze
Hi, On 9/4/20 8:20 PM, Barnabás Pőcze wrote: >> [...] >> +static void input_key(struct system76_data *data, unsigned int code) >> +{ >> + input_report_key(data->input, code, 1); >> + input_sync(data->input); >> + >> + input_report_key(data->input, code, 0); >> + input_sync(data->input); >> +} >> + >> // Handle ACPI notification >> static void system76_notify(struct acpi_device *acpi_dev, u32 event) >> { >> @@ -459,6 +470,9 @@ static void system76_notify(struct acpi_device *acpi_dev, u32 event) >> case 0x84: >> kb_led_hotkey_color(data); >> break; >> + case 0x85: >> + input_key(data, KEY_SCREENLOCK); >> + break; >> } >> } >> >> @@ -524,6 +538,21 @@ static int system76_add(struct acpi_device *acpi_dev) >> if (IS_ERR(data->therm)) >> return PTR_ERR(data->therm); >> >> + data->input = devm_input_allocate_device(&acpi_dev->dev); >> + if (!data->input) >> + return -ENOMEM; >> + data->input->name = "System76 ACPI Hotkeys"; >> + data->input->phys = "system76_acpi/input0"; >> + data->input->id.bustype = BUS_HOST; >> + data->input->dev.parent = &acpi_dev->dev; >> + set_bit(EV_KEY, data->input->evbit); >> + set_bit(KEY_SCREENLOCK, data->input->keybit); >> + err = input_register_device(data->input); >> + if (err) { >> + input_free_device(data->input); >> + return err; >> + } >> + >> return 0; >> } > > Hi, > > wouldn't sparse_keymap be a better choice here? Since none of the notify events are actually keys; and since there is only one keycode involved atm, that seems like a bit of overkill to me. Regards, Hans
Hi, thanks for the feedback. > [...] > >> +static void input_key(struct system76_data *data, unsigned int code) > >> +{ > >> + input_report_key(data->input, code, 1); > >> + input_sync(data->input); > >> + > >> + input_report_key(data->input, code, 0); > >> + input_sync(data->input); > >> +} > >> + > >> // Handle ACPI notification > >> static void system76_notify(struct acpi_device *acpi_dev, u32 event) > >> { > >> @@ -459,6 +470,9 @@ static void system76_notify(struct acpi_device *acpi_dev, u32 event) > >> case 0x84: > >> kb_led_hotkey_color(data); > >> break; > >> + case 0x85: > >> + input_key(data, KEY_SCREENLOCK); > >> + break; > >> } > >> } > >> > >> @@ -524,6 +538,21 @@ static int system76_add(struct acpi_device *acpi_dev) > >> if (IS_ERR(data->therm)) > >> return PTR_ERR(data->therm); > >> > >> + data->input = devm_input_allocate_device(&acpi_dev->dev); > >> + if (!data->input) > >> + return -ENOMEM; > >> + data->input->name = "System76 ACPI Hotkeys"; > >> + data->input->phys = "system76_acpi/input0"; > >> + data->input->id.bustype = BUS_HOST; > >> + data->input->dev.parent = &acpi_dev->dev; > >> + set_bit(EV_KEY, data->input->evbit); > >> + set_bit(KEY_SCREENLOCK, data->input->keybit); > >> + err = input_register_device(data->input); > >> + if (err) { > >> + input_free_device(data->input); > >> + return err; > >> + } > >> + > >> return 0; > >> } > > > > Hi, > > > > wouldn't sparse_keymap be a better choice here? > > Since none of the notify events are actually keys; I'm not sure I understand what you mean, could you please clarify? > and since there is only one keycode involved atm, that > seems like a bit of overkill to me. Indeed, it might be an overkill, but I'd still vote for that since - it is an ε effort investment to convert the current code, and - the number of keys is expected to grow (at least that's my assumption), and - it avoids code duplication, the resulting code is simple and short. > [...] Barnabás Pőcze
Hi, On 9/7/20 5:49 PM, Barnabás Pőcze wrote: > Hi, > > thanks for the feedback. > >> [...] >>>> +static void input_key(struct system76_data *data, unsigned int code) >>>> +{ >>>> + input_report_key(data->input, code, 1); >>>> + input_sync(data->input); >>>> + >>>> + input_report_key(data->input, code, 0); >>>> + input_sync(data->input); >>>> +} >>>> + >>>> // Handle ACPI notification >>>> static void system76_notify(struct acpi_device *acpi_dev, u32 event) >>>> { >>>> @@ -459,6 +470,9 @@ static void system76_notify(struct acpi_device *acpi_dev, u32 event) >>>> case 0x84: >>>> kb_led_hotkey_color(data); >>>> break; >>>> + case 0x85: >>>> + input_key(data, KEY_SCREENLOCK); >>>> + break; >>>> } >>>> } >>>> >>>> @@ -524,6 +538,21 @@ static int system76_add(struct acpi_device *acpi_dev) >>>> if (IS_ERR(data->therm)) >>>> return PTR_ERR(data->therm); >>>> >>>> + data->input = devm_input_allocate_device(&acpi_dev->dev); >>>> + if (!data->input) >>>> + return -ENOMEM; >>>> + data->input->name = "System76 ACPI Hotkeys"; >>>> + data->input->phys = "system76_acpi/input0"; >>>> + data->input->id.bustype = BUS_HOST; >>>> + data->input->dev.parent = &acpi_dev->dev; >>>> + set_bit(EV_KEY, data->input->evbit); >>>> + set_bit(KEY_SCREENLOCK, data->input->keybit); >>>> + err = input_register_device(data->input); >>>> + if (err) { >>>> + input_free_device(data->input); >>>> + return err; >>>> + } >>>> + >>>> return 0; >>>> } >>> >>> Hi, >>> >>> wouldn't sparse_keymap be a better choice here? >> >> Since none of the notify events are actually keys; > > I'm not sure I understand what you mean, could you please clarify? What I meant to say (but didn't) is: "Since none of the notify events are _currently_ actually keys" Currently, as in before this patch: static void system76_notify(struct acpi_device *acpi_dev, u32 event) { struct system76_data *data; data = acpi_driver_data(acpi_dev); switch (event) { case 0x80: kb_led_hotkey_hardware(data); break; case 0x81: kb_led_hotkey_toggle(data); break; case 0x82: kb_led_hotkey_down(data); break; case 0x83: kb_led_hotkey_up(data); break; case 0x84: kb_led_hotkey_color(data); break; } } So we cannot just take the event code and feed it to the sparse_keymap code since events 0x80-0x84 are not key events (they are related to the LEDs on the kbd). >> and since there is only one keycode involved atm, that >> seems like a bit of overkill to me. > > Indeed, it might be an overkill, but I'd still vote for that since > - it is an ε effort investment to convert the current code, and > - the number of keys is expected to grow (at least that's my assumption), and > - it avoids code duplication, the resulting code is simple and short. If Jeremy is ok with adding sparse_keymap support to the next version, that is fine with me. But IMHO it is not really necessary for adding just this single key. Regards, Hans
> Hi, > > On 9/7/20 5:49 PM, Barnabás Pőcze wrote: > > Hi, > > > > thanks for the feedback. > > > >> [...] > >>>> +static void input_key(struct system76_data *data, unsigned int code) > >>>> +{ > >>>> + input_report_key(data->input, code, 1); > >>>> + input_sync(data->input); > >>>> + > >>>> + input_report_key(data->input, code, 0); > >>>> + input_sync(data->input); > >>>> +} > >>>> + > >>>> // Handle ACPI notification > >>>> static void system76_notify(struct acpi_device *acpi_dev, u32 event) > >>>> { > >>>> @@ -459,6 +470,9 @@ static void system76_notify(struct acpi_device *acpi_dev, u32 event) > >>>> case 0x84: > >>>> kb_led_hotkey_color(data); > >>>> break; > >>>> + case 0x85: > >>>> + input_key(data, KEY_SCREENLOCK); > >>>> + break; > >>>> } > >>>> } > >>>> > >>>> @@ -524,6 +538,21 @@ static int system76_add(struct acpi_device *acpi_dev) > >>>> if (IS_ERR(data->therm)) > >>>> return PTR_ERR(data->therm); > >>>> > >>>> + data->input = devm_input_allocate_device(&acpi_dev->dev); > >>>> + if (!data->input) > >>>> + return -ENOMEM; > >>>> + data->input->name = "System76 ACPI Hotkeys"; > >>>> + data->input->phys = "system76_acpi/input0"; > >>>> + data->input->id.bustype = BUS_HOST; > >>>> + data->input->dev.parent = &acpi_dev->dev; > >>>> + set_bit(EV_KEY, data->input->evbit); > >>>> + set_bit(KEY_SCREENLOCK, data->input->keybit); > >>>> + err = input_register_device(data->input); > >>>> + if (err) { > >>>> + input_free_device(data->input); > >>>> + return err; > >>>> + } > >>>> + > >>>> return 0; > >>>> } > >>> > >>> Hi, > >>> > >>> wouldn't sparse_keymap be a better choice here? > >> > >> Since none of the notify events are actually keys; > > > > I'm not sure I understand what you mean, could you please clarify? > > What I meant to say (but didn't) is: > > "Since none of the notify events are _currently_ actually keys" > > Currently, as in before this patch: > > static void system76_notify(struct acpi_device *acpi_dev, u32 event) > { > struct system76_data *data; > > data = acpi_driver_data(acpi_dev); > switch (event) { > case 0x80: > kb_led_hotkey_hardware(data); > break; > case 0x81: > kb_led_hotkey_toggle(data); > break; > case 0x82: > kb_led_hotkey_down(data); > break; > case 0x83: > kb_led_hotkey_up(data); > break; > case 0x84: > kb_led_hotkey_color(data); > break; > } > } > > So we cannot just take the event code and feed it to the > sparse_keymap code since events 0x80-0x84 are not > key events (they are related to the LEDs on the kbd). > > >> and since there is only one keycode involved atm, that > >> seems like a bit of overkill to me. > > > > Indeed, it might be an overkill, but I'd still vote for that since > > - it is an ε effort investment to convert the current code, and > > - the number of keys is expected to grow (at least that's my assumption), and > > - it avoids code duplication, the resulting code is simple and short. > > If Jeremy is ok with adding sparse_keymap support to the next version, that > is fine with me. But IMHO it is not really necessary for adding just this > single key. We don't really have a plan for any more keycodes. The intel-hid driver does most of what we need, but for some reason omits the screenlock key. So we have added it here. If there is a keycode we want to add, I would not mind looking into any alternative, provided it is less code to maintain. > Regards, > > Hans > >
diff --git a/drivers/platform/x86/Kconfig b/drivers/platform/x86/Kconfig index d2f9c3dcf4b9..a8b75c1b96fb 100644 --- a/drivers/platform/x86/Kconfig +++ b/drivers/platform/x86/Kconfig @@ -1159,6 +1159,7 @@ config SYSTEM76_ACPI tristate "System76 ACPI Driver" depends on ACPI depends on HWMON + depends on INPUT select NEW_LEDS select LEDS_CLASS select LEDS_TRIGGERS diff --git a/drivers/platform/x86/system76_acpi.c b/drivers/platform/x86/system76_acpi.c index aef0ea34829d..eaff9adea504 100644 --- a/drivers/platform/x86/system76_acpi.c +++ b/drivers/platform/x86/system76_acpi.c @@ -13,6 +13,7 @@ #include <linux/hwmon.h> #include <linux/hwmon-sysfs.h> #include <linux/init.h> +#include <linux/input.h> #include <linux/kernel.h> #include <linux/leds.h> #include <linux/module.h> @@ -29,6 +30,7 @@ struct system76_data { struct device *therm; union acpi_object *nfan; union acpi_object *ntmp; + struct input_dev *input; }; static const struct acpi_device_id device_ids[] = { @@ -437,6 +439,15 @@ static const struct hwmon_chip_info thermal_chip_info = { .info = thermal_channel_info, }; +static void input_key(struct system76_data *data, unsigned int code) +{ + input_report_key(data->input, code, 1); + input_sync(data->input); + + input_report_key(data->input, code, 0); + input_sync(data->input); +} + // Handle ACPI notification static void system76_notify(struct acpi_device *acpi_dev, u32 event) { @@ -459,6 +470,9 @@ static void system76_notify(struct acpi_device *acpi_dev, u32 event) case 0x84: kb_led_hotkey_color(data); break; + case 0x85: + input_key(data, KEY_SCREENLOCK); + break; } } @@ -524,6 +538,21 @@ static int system76_add(struct acpi_device *acpi_dev) if (IS_ERR(data->therm)) return PTR_ERR(data->therm); + data->input = devm_input_allocate_device(&acpi_dev->dev); + if (!data->input) + return -ENOMEM; + data->input->name = "System76 ACPI Hotkeys"; + data->input->phys = "system76_acpi/input0"; + data->input->id.bustype = BUS_HOST; + data->input->dev.parent = &acpi_dev->dev; + set_bit(EV_KEY, data->input->evbit); + set_bit(KEY_SCREENLOCK, data->input->keybit); + err = input_register_device(data->input); + if (err) { + input_free_device(data->input); + return err; + } + return 0; }
platform/x86: system76_acpi: Add input driver The input driver reports KEY_SCREENLOCK when the firmware notifies the kernel of event 0x85. This is to support hardware with a screen lock hotkey. Signed-off-by: Jeremy Soller <jeremy@system76.com> Cc: platform-driver-x86@vger.kernel.org ---