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