Re: [PATCH 2/2] platform/x86: system76_acpi: Add input driver

[Hans de Goede <hdegoede@xxxxxxxxxx>]

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