Re: [PATCH] keymap: support for force_release quirk

[Martin Pitt <martin.pitt@ubuntu.com>]

[-- Attachment #1: Type: text/plain, Size: 2682 bytes --]

Hello Johannes,

thanks for working on this, and sorry for the late response (had a
busy week..)

Some remarks:

> --- /dev/null
> +++ b/extras/keymap/95-keyboard-force-release.rules
> @@ -0,0 +1,24 @@
> +# Set model specific atkbd force_release quirk
> +#
> +# Serveral laptops have hotkeys which don't generate release events,

Typo → "Several"

> +ACTION!="add", GOTO="force_release_end"

Can we please also do this on "change", so that rule changes can be
picked up on package upgrades? Packages which ship udev rules should
usually do something like

  udevadm trigger --action=change --subsystem-match=...

> +ENV{DMI_VENDOR}=="[sS][aA][mM][sS][uU][nN][gG]*", ATTR{[dmi/id]product_name}=="*N130*", RUN+="keyboard-force-release.sh $sys$devpath samsung-other"

For consistency, can we just use $devpath here, and care for
prepending /sys in the callout?

> --- /dev/null
> +++ b/extras/keymap/keyboard-force-release.sh
> @@ -0,0 +1,22 @@
> +#!/bin/sh -e
> +# read list of scancodes, convert hex to decimal and
> +# append to the atkbd force_release sysfs attribute
> +# $1 sysfs devpath for serioX
> +# $2 file with scancode list (hex or dec)
> +
> +case "$2" in
> +	/*) scf="$2" ;;
> +	*)  scf="${0%keyboard-force-release.sh}/keymaps/force-release/$2" ;;

I don't think we should really tie the callout path to the map
directory. Just hardcoding /lib/udev/keymaps/force-release/ seems
cleaner to me.

> +attr=`cat "$1/force_release"`

Please use "read" to avoid calling an external program and a subshell.
The script is -e already (as it should be), so I think the error
message if the file doesn't exist (older kernels) will be okay.

> +while read scancode dummy; do
> +	case "$scancode" in
> +		\#*) ;;
> +		*)
> +			scancode=$(($scancode))
> +			[ -n "$attr" ] && attr="$attr,$scancode" || attr="$scancode"
> +			;;
> +	esac
> +done <"$scf"
> +echo "$attr" >"$1/force_release"

Corresponding to comment above, please use /sys/$1 here.

A gotcha that I see here is that the force_release only ever gets
appended to. I. e. whenever we run udevtrigger, we'd append the same
keys again. POSIX shell doesn't have elaborate substring matching
capabilities like bash's ${x/pattern/string}, so it might not be too
easy to check if we already have a key in POSIX sh. (But please don't
call grep in a loop; let's rather rewrite this bit in C). There might
be some trickery with splitting by IFS=, into an array or so, if you
want to keep using sh?

Thanks!

Martin

-- 
Martin Pitt                        | http://www.piware.de
Ubuntu Developer (www.ubuntu.com)  | Debian Developer  (www.debian.org)

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 197 bytes --]