[systemd-devel] [PATCH v2 5/6] udev: builtin-keyboard: add support for EV_ABS_OVERRIDE

[David Herrmann]

Hi

On Tue, Apr 7, 2015 at 2:15 AM, Peter Hutterer <peter.hutterer at who-t.net> wrote:
> On Fri, Mar 27, 2015 at 12:54:50PM +0100, Martin Pitt wrote:
>> Hey Peter,
>>
>> thanks for these! Patches 1 to 4 look good to me, but I have some
>> questions/comments on this one.
>>
>> Peter Hutterer [2015-03-23 11:30 +1000]:
>> > --- /dev/null
>> > +++ b/rules/60-evdev.rules
>> > @@ -0,0 +1,14 @@
>> > +# do not edit this file, it will be overwritten on update
>> > +
>> > +ACTION=="remove", GOTO="evdev_end"
>> > +KERNEL!="event*", GOTO="evdev_end"
>> > +
>> > +# skip later rules when we find something for this input device
>> > +IMPORT{builtin}="hwdb --subsystem=input --lookup-prefix=evdev:", \
>> > +  RUN{builtin}+="keyboard", GOTO="evdev_end"
>> > +
>> > +# device matching the input device name and the machine's DMI data
>> > +KERNELS=="input*", IMPORT{builtin}="hwdb 'evdev:name:$attr{name}:$attr{[dmi/id]modalias}'", \
>> > +  RUN{builtin}+="keyboard", GOTO="evdev_end"
>>
>> With this we would then call the hwdb builtin here, and another time
>> in 60-keyboard.rules, except that that one uses a "keyboard:" prefix.
>> I wonder if we could use the generalized "evdev:" for all of them?
>
> works for me if we don't have emotional attachment to the keyboard prefix :)
> patch coming up.

No emotional attachment, but we usually avoid purely technical
acronyms if we have human descriptions for it. So "input" would be the
prefix of choice here, if it weren't already reserved for the kernel
subsystem.

Not sure what else to use.. so I'd be fine with 'evdev' for now. It's
internal, so we can change it at any time.

Thanks
David

>> > +static int open_device(const char *devnode) {
>> > +        int fd = open(devnode, O_RDWR|O_CLOEXEC|O_NONBLOCK|O_NOCTTY);
>>
>> I think calling functions in variable declarations is frowned upon in
>> the systemd code.
>
> fixed locally, thanks.

Yes, just add the newlines:

int fd;

fd = open();
if (fd < 0)
        log_foobar()

return fd < 0 ? -errno : fd;


Thanks
David

> Cheers,
>    Peter
>
>>
>> The rest LGTM, thanks!