2020-03-24 12:36:06

by Rajat Jain

[permalink] [raw]
Subject: [PATCH v2 1/5] input/serio/i8042: Attach fwnode to serio i8042 kbd device

Attach the firmware node to the serio i8042 kbd device so that device
properties can be passed from the firmware.

Signed-off-by: Rajat Jain <[email protected]>
---
v2: Remove the Change-Id from the commit log

drivers/input/serio/i8042-x86ia64io.h | 1 +
drivers/input/serio/i8042.c | 3 +++
2 files changed, 4 insertions(+)

diff --git a/drivers/input/serio/i8042-x86ia64io.h b/drivers/input/serio/i8042-x86ia64io.h
index dc974c288e880..ed9ec4310d976 100644
--- a/drivers/input/serio/i8042-x86ia64io.h
+++ b/drivers/input/serio/i8042-x86ia64io.h
@@ -927,6 +927,7 @@ static int i8042_pnp_kbd_probe(struct pnp_dev *dev, const struct pnp_device_id *
}
i8042_pnp_id_to_string(dev->id, i8042_kbd_firmware_id,
sizeof(i8042_kbd_firmware_id));
+ i8042_kbd_fwnode = dev_fwnode(&dev->dev);

/* Keyboard ports are always supposed to be wakeup-enabled */
device_set_wakeup_enable(&dev->dev, true);
diff --git a/drivers/input/serio/i8042.c b/drivers/input/serio/i8042.c
index 20ff2bed3917a..0dddf273afd94 100644
--- a/drivers/input/serio/i8042.c
+++ b/drivers/input/serio/i8042.c
@@ -21,6 +21,7 @@
#include <linux/i8042.h>
#include <linux/slab.h>
#include <linux/suspend.h>
+#include <linux/property.h>

#include <asm/io.h>

@@ -124,6 +125,7 @@ MODULE_PARM_DESC(unmask_kbd_data, "Unconditional enable (may reveal sensitive da
static bool i8042_bypass_aux_irq_test;
static char i8042_kbd_firmware_id[128];
static char i8042_aux_firmware_id[128];
+static struct fwnode_handle *i8042_kbd_fwnode;

#include "i8042.h"

@@ -1335,6 +1337,7 @@ static int __init i8042_create_kbd_port(void)
strlcpy(serio->phys, I8042_KBD_PHYS_DESC, sizeof(serio->phys));
strlcpy(serio->firmware_id, i8042_kbd_firmware_id,
sizeof(serio->firmware_id));
+ set_primary_fwnode(&serio->dev, i8042_kbd_fwnode);

port->serio = serio;
port->irq = I8042_KBD_IRQ;
--
2.25.1.696.g5e7596f4ac-goog


2020-03-24 12:36:18

by Rajat Jain

[permalink] [raw]
Subject: [PATCH v2 2/5] Input: atkbd: Expose function row physical map to userspace

Certain keyboards have their top-row keys intended
for actions such as "Browser back", "Browser Refresh", "Fullscreen"
etc as their primary mode, thus they will send physical codes for those
actions. Further, they don't have a dedicated "Fn" key so don't have
the capability to generate function key codes (e.g. F1, F2 etc..).
However in this case, if userspace still wants to "synthesize" those
function keys using the top row action keys, it needs to know the
physical position of the top row keys. (Essentially a mapping between
usage codes and a physical location in the top row).

This patch enhances the atkbd driver to receive such a mapping from the
firmware / device tree, and expose it to userspace in the form of
a function-row-physmap attribute. The attribute would be a space
separated ordered list of physical codes, for the keys in the function
row, in left-to-right order.

The attribute will only be present if the kernel knows about such
mapping, otherwise the attribute shall not be visible.

Signed-off-by: Rajat Jain <[email protected]>
---
v2: Remove the Change-Id from the commit log

drivers/input/keyboard/atkbd.c | 59 ++++++++++++++++++++++++++++++++++
1 file changed, 59 insertions(+)

diff --git a/drivers/input/keyboard/atkbd.c b/drivers/input/keyboard/atkbd.c
index 7e3eae54c1926..7623eebef2593 100644
--- a/drivers/input/keyboard/atkbd.c
+++ b/drivers/input/keyboard/atkbd.c
@@ -24,6 +24,7 @@
#include <linux/libps2.h>
#include <linux/mutex.h>
#include <linux/dmi.h>
+#include <linux/property.h>

#define DRIVER_DESC "AT and PS/2 keyboard driver"

@@ -63,6 +64,8 @@ static bool atkbd_terminal;
module_param_named(terminal, atkbd_terminal, bool, 0);
MODULE_PARM_DESC(terminal, "Enable break codes on an IBM Terminal keyboard connected via AT/PS2");

+#define MAX_FUNCTION_ROW_KEYS 24
+
/*
* Scancode to keycode tables. These are just the default setting, and
* are loadable via a userland utility.
@@ -230,6 +233,9 @@ struct atkbd {

/* Serializes reconnect(), attr->set() and event work */
struct mutex mutex;
+
+ u16 function_row_physmap[MAX_FUNCTION_ROW_KEYS];
+ int num_function_row_keys;
};

/*
@@ -283,6 +289,7 @@ static struct device_attribute atkbd_attr_##_name = \
__ATTR(_name, S_IRUGO, atkbd_do_show_##_name, NULL);

ATKBD_DEFINE_RO_ATTR(err_count);
+ATKBD_DEFINE_RO_ATTR(function_row_physmap);

static struct attribute *atkbd_attributes[] = {
&atkbd_attr_extra.attr,
@@ -292,11 +299,42 @@ static struct attribute *atkbd_attributes[] = {
&atkbd_attr_softrepeat.attr,
&atkbd_attr_softraw.attr,
&atkbd_attr_err_count.attr,
+ &atkbd_attr_function_row_physmap.attr,
NULL
};

+static ssize_t atkbd_show_function_row_physmap(struct atkbd *atkbd, char *buf)
+{
+ ssize_t size = 0;
+ int i;
+
+ if (!atkbd->num_function_row_keys)
+ return 0;
+
+ for (i = 0; i < atkbd->num_function_row_keys; i++)
+ size += sprintf(buf + size, "%02X ",
+ atkbd->function_row_physmap[i]);
+ size += sprintf(buf + size, "\n");
+ return size;
+}
+
+static umode_t atkbd_attr_is_visible(struct kobject *kobj,
+ struct attribute *attr, int i)
+{
+ struct device *dev = container_of(kobj, struct device, kobj);
+ struct serio *serio = to_serio_port(dev);
+ struct atkbd *atkbd = serio_get_drvdata(serio);
+
+ if (attr == &atkbd_attr_function_row_physmap.attr &&
+ !atkbd->num_function_row_keys)
+ return 0;
+
+ return attr->mode;
+}
+
static struct attribute_group atkbd_attribute_group = {
.attrs = atkbd_attributes,
+ .is_visible = atkbd_attr_is_visible,
};

static const unsigned int xl_table[] = {
@@ -1121,6 +1159,25 @@ static void atkbd_set_device_attrs(struct atkbd *atkbd)
}
}

+static void atkbd_parse_fwnode_data(struct serio *serio)
+{
+ struct atkbd *atkbd = serio_get_drvdata(serio);
+ struct device *dev = &serio->dev;
+ int n;
+
+ if (!dev_fwnode(dev))
+ return;
+
+ /* Parse "function-row-physmap" property */
+ n = device_property_count_u16(dev, "function-row-physmap");
+ if (n > 0 && n <= MAX_FUNCTION_ROW_KEYS &&
+ !device_property_read_u16_array(dev, "function-row-physmap",
+ atkbd->function_row_physmap, n)) {
+ atkbd->num_function_row_keys = n;
+ dev_info(dev, "FW reported %d function-row key locations\n", n);
+ }
+}
+
/*
* atkbd_connect() is called when the serio module finds an interface
* that isn't handled yet by an appropriate device driver. We check if
@@ -1184,6 +1241,8 @@ static int atkbd_connect(struct serio *serio, struct serio_driver *drv)
atkbd->id = 0xab00;
}

+ atkbd_parse_fwnode_data(serio);
+
atkbd_set_keycode_table(atkbd);
atkbd_set_device_attrs(atkbd);

--
2.25.1.696.g5e7596f4ac-goog

2020-03-24 12:36:32

by Rajat Jain

[permalink] [raw]
Subject: [PATCH v2 3/5] dt-bindings: input/atkbd.txt: Add binding for "function-row-physmap"

Create the documentation for the new introduced property, that
describes the function-row keys physical positions.

Signed-off-by: Rajat Jain <[email protected]>
---
v2: Remove the Change-Id from the commit log

.../devicetree/bindings/input/atkbd.txt | 34 +++++++++++++++++++
1 file changed, 34 insertions(+)
create mode 100644 Documentation/devicetree/bindings/input/atkbd.txt

diff --git a/Documentation/devicetree/bindings/input/atkbd.txt b/Documentation/devicetree/bindings/input/atkbd.txt
new file mode 100644
index 0000000000000..816653eb8e98d
--- /dev/null
+++ b/Documentation/devicetree/bindings/input/atkbd.txt
@@ -0,0 +1,34 @@
+Device tree bindings for AT / PS2 keyboard device
+
+Optional properties:
+
+ function-row-physmap:
+ An ordered array of the physical codes for the function
+ row keys. Arranged in order from left to right.
+
+Example:
+
+ This is a sample ACPI _DSD node describing the property:
+
+ Name (_DSD, Package () {
+ ToUUID("daffd814-6eba-4d8c-8a91-bc9bbf4aa301"),
+ Package () {
+ Package () { "function-row-physmap",
+ Package () {
+ 0xEA, /* T1 BACK */
+ 0xE7, /* T2 REFRESH */
+ 0x91, /* T3 FULLSCREEN */
+ 0x92, /* T4 SCALE */
+ 0x93, /* T5 SNIP */
+ 0x94, /* T6 BRIGHTNESS_DOWN */
+ 0x95, /* T7 BRIGHTNESS_UP */
+ 0x96, /* T8 PRIVACY_SCRN_TOGGLE */
+ 0x97, /* T9 KBD_BKLIGHT_DOWN */
+ 0x98, /* T10 KBD_BKLIGHT_UP */
+ 0xA0, /* T11 VOL_MUTE */
+ 0xAE, /* T12 VOL_DOWN */
+ 0xB0, /* T13 VOL_UP */
+ }
+ }
+ }
+ })
--
2.25.1.696.g5e7596f4ac-goog

2020-03-24 12:36:53

by Rajat Jain

[permalink] [raw]
Subject: [PATCH v2 5/5] dt-bindings: input/atkbd.txt: Add binding info for "keymap" property

Add the info for keymap property that allows firmware to specify the
mapping from physical code to linux keycode, that the kernel should use.

Signed-off-by: Rajat Jain <[email protected]>
---
v2: Remove the Change-Id from the commit log

.../devicetree/bindings/input/atkbd.txt | 27 ++++++++++++++++++-
1 file changed, 26 insertions(+), 1 deletion(-)

diff --git a/Documentation/devicetree/bindings/input/atkbd.txt b/Documentation/devicetree/bindings/input/atkbd.txt
index 816653eb8e98d..0a0037d70adc8 100644
--- a/Documentation/devicetree/bindings/input/atkbd.txt
+++ b/Documentation/devicetree/bindings/input/atkbd.txt
@@ -6,9 +6,15 @@ Optional properties:
An ordered array of the physical codes for the function
row keys. Arranged in order from left to right.

+ keymap:
+ An array of the u32 entries to specify mapping from the
+ keyboard physcial codes to linux keycodes. The top 16
+ bits of each entry are the physical code, and bottom
+ 16 bits are the linux keycode.
+
Example:

- This is a sample ACPI _DSD node describing the property:
+ This is a sample ACPI _DSD node describing the properties:

Name (_DSD, Package () {
ToUUID("daffd814-6eba-4d8c-8a91-bc9bbf4aa301"),
@@ -29,6 +35,25 @@ Example:
0xAE, /* T12 VOL_DOWN */
0xB0, /* T13 VOL_UP */
}
+ },
+ Package () { "keymap",
+ Package () {
+ 0xEA009E, /* EA -> KEY_BACK */
+ 0xE700AD, /* E7 -> KEY_REFRESH */
+ 0x910174, /* 91 -> KEY_FULL_SCREEN */
+ 0x920078, /* 92 -> KEY_SCALE */
+ 0x930280, /* 93 -> 0x280 */
+ 0x9400E0, /* 94 -> KEY_BRIGHTNESS_DOWN*/
+ 0x9500E1, /* 95 -> KEY_BRIGHTNESS_UP */
+ 0x960279, /* 96 -> KEY_PRIVACY_SCRN_TOGGLE*/
+ 0x9700E5, /* 97 -> KEY_KBDILLUMDOWN */
+ 0x9800E6, /* 98 -> KEY_KBDILLUMUP */
+ 0xA00071, /* A0 -> KEY_MUTE */
+ 0xAE0072, /* AE -> KEY_VOLUMEDOWN */
+ 0xB00073, /* B0 -> KEY_VOLUMEUP */
+ ...
+ <snip other entries>
+ }
}
}
})
--
2.25.1.696.g5e7596f4ac-goog

2020-03-24 12:37:43

by Rajat Jain

[permalink] [raw]
Subject: [PATCH v2 4/5] Input: atkbd: Receive and use physcode->keycode mapping from FW

Allow the firmware to specify the mapping between the physical
code and the linux keycode. This takes the form of a "keymap"
property which is an array of u32 values, each value specifying
mapping for a key.

Signed-off-by: Rajat Jain <[email protected]>
---
v2: Remove the Change-Id from the commit log

drivers/input/keyboard/atkbd.c | 39 ++++++++++++++++++++++++++++++++--
1 file changed, 37 insertions(+), 2 deletions(-)

diff --git a/drivers/input/keyboard/atkbd.c b/drivers/input/keyboard/atkbd.c
index 7623eebef2593..c8017a5707581 100644
--- a/drivers/input/keyboard/atkbd.c
+++ b/drivers/input/keyboard/atkbd.c
@@ -66,6 +66,9 @@ MODULE_PARM_DESC(terminal, "Enable break codes on an IBM Terminal keyboard conne

#define MAX_FUNCTION_ROW_KEYS 24

+#define PHYSCODE(keymap) ((keymap >> 16) & 0xFFFF)
+#define KEYCODE(keymap) (keymap & 0xFFFF)
+
/*
* Scancode to keycode tables. These are just the default setting, and
* are loadable via a userland utility.
@@ -236,6 +239,9 @@ struct atkbd {

u16 function_row_physmap[MAX_FUNCTION_ROW_KEYS];
int num_function_row_keys;
+
+ unsigned short fw_keymap[ATKBD_KEYMAP_SIZE];
+ bool use_fw_keymap;
};

/*
@@ -1045,7 +1051,10 @@ static void atkbd_set_keycode_table(struct atkbd *atkbd)
memset(atkbd->keycode, 0, sizeof(atkbd->keycode));
bitmap_zero(atkbd->force_release_mask, ATKBD_KEYMAP_SIZE);

- if (atkbd->translated) {
+ if (atkbd->use_fw_keymap) {
+ memcpy(atkbd->keycode, atkbd->fw_keymap,
+ sizeof(atkbd->keycode));
+ } else if (atkbd->translated) {
for (i = 0; i < 128; i++) {
scancode = atkbd_unxlate_table[i];
atkbd->keycode[i] = atkbd_set2_keycode[scancode];
@@ -1163,7 +1172,9 @@ static void atkbd_parse_fwnode_data(struct serio *serio)
{
struct atkbd *atkbd = serio_get_drvdata(serio);
struct device *dev = &serio->dev;
- int n;
+ int i, n;
+ u32 *ptr;
+ u16 physcode, keycode;

if (!dev_fwnode(dev))
return;
@@ -1176,6 +1187,30 @@ static void atkbd_parse_fwnode_data(struct serio *serio)
atkbd->num_function_row_keys = n;
dev_info(dev, "FW reported %d function-row key locations\n", n);
}
+
+ /* Parse "keymap" property */
+ n = device_property_count_u32(dev, "keymap");
+ if (n > 0 && n <= ATKBD_KEYMAP_SIZE) {
+
+ ptr = kcalloc(n, sizeof(u32), GFP_KERNEL);
+ if (!ptr)
+ return;
+
+ if (device_property_read_u32_array(dev, "keymap", ptr, n)) {
+ dev_err(dev, "problem parsing FW keymap property\n");
+ kfree(ptr);
+ return;
+ }
+
+ for (i = 0; i < n; i++) {
+ physcode = PHYSCODE(ptr[i]);
+ keycode = KEYCODE(ptr[i]);
+ atkbd->fw_keymap[physcode] = keycode;
+ }
+ dev_info(dev, "Using FW keymap (%d keys)\n", n);
+ atkbd->use_fw_keymap = true;
+ kfree(ptr);
+ }
}

/*
--
2.25.1.696.g5e7596f4ac-goog

2020-03-26 21:21:15

by Dmitry Torokhov

[permalink] [raw]
Subject: Re: [PATCH v2 4/5] Input: atkbd: Receive and use physcode->keycode mapping from FW

Hi Rajat,

On Tue, Mar 24, 2020 at 05:35:17AM -0700, Rajat Jain wrote:
> Allow the firmware to specify the mapping between the physical
> code and the linux keycode. This takes the form of a "keymap"
> property which is an array of u32 values, each value specifying
> mapping for a key.
>
> Signed-off-by: Rajat Jain <[email protected]>
> ---
> v2: Remove the Change-Id from the commit log
>
> drivers/input/keyboard/atkbd.c | 39 ++++++++++++++++++++++++++++++++--
> 1 file changed, 37 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/input/keyboard/atkbd.c b/drivers/input/keyboard/atkbd.c
> index 7623eebef2593..c8017a5707581 100644
> --- a/drivers/input/keyboard/atkbd.c
> +++ b/drivers/input/keyboard/atkbd.c
> @@ -66,6 +66,9 @@ MODULE_PARM_DESC(terminal, "Enable break codes on an IBM Terminal keyboard conne
>
> #define MAX_FUNCTION_ROW_KEYS 24
>
> +#define PHYSCODE(keymap) ((keymap >> 16) & 0xFFFF)
> +#define KEYCODE(keymap) (keymap & 0xFFFF)
> +
> /*
> * Scancode to keycode tables. These are just the default setting, and
> * are loadable via a userland utility.
> @@ -236,6 +239,9 @@ struct atkbd {
>
> u16 function_row_physmap[MAX_FUNCTION_ROW_KEYS];
> int num_function_row_keys;
> +
> + unsigned short fw_keymap[ATKBD_KEYMAP_SIZE];
> + bool use_fw_keymap;

Why do we need to keep firmware-provided keymap in atkbd instance? It is
not going anywhere and can be accessed via device_property_* API
whenever we decide to refresh the keymap.

> };
>
> /*
> @@ -1045,7 +1051,10 @@ static void atkbd_set_keycode_table(struct atkbd *atkbd)
> memset(atkbd->keycode, 0, sizeof(atkbd->keycode));
> bitmap_zero(atkbd->force_release_mask, ATKBD_KEYMAP_SIZE);
>
> - if (atkbd->translated) {
> + if (atkbd->use_fw_keymap) {
> + memcpy(atkbd->keycode, atkbd->fw_keymap,
> + sizeof(atkbd->keycode));
> + } else if (atkbd->translated) {
> for (i = 0; i < 128; i++) {
> scancode = atkbd_unxlate_table[i];
> atkbd->keycode[i] = atkbd_set2_keycode[scancode];
> @@ -1163,7 +1172,9 @@ static void atkbd_parse_fwnode_data(struct serio *serio)
> {
> struct atkbd *atkbd = serio_get_drvdata(serio);
> struct device *dev = &serio->dev;
> - int n;
> + int i, n;
> + u32 *ptr;
> + u16 physcode, keycode;
>
> if (!dev_fwnode(dev))
> return;
> @@ -1176,6 +1187,30 @@ static void atkbd_parse_fwnode_data(struct serio *serio)
> atkbd->num_function_row_keys = n;
> dev_info(dev, "FW reported %d function-row key locations\n", n);
> }
> +
> + /* Parse "keymap" property */
> + n = device_property_count_u32(dev, "keymap");
> + if (n > 0 && n <= ATKBD_KEYMAP_SIZE) {
> +
> + ptr = kcalloc(n, sizeof(u32), GFP_KERNEL);
> + if (!ptr)
> + return;
> +
> + if (device_property_read_u32_array(dev, "keymap", ptr, n)) {
> + dev_err(dev, "problem parsing FW keymap property\n");
> + kfree(ptr);
> + return;
> + }
> +
> + for (i = 0; i < n; i++) {
> + physcode = PHYSCODE(ptr[i]);
> + keycode = KEYCODE(ptr[i]);
> + atkbd->fw_keymap[physcode] = keycode;
> + }
> + dev_info(dev, "Using FW keymap (%d keys)\n", n);

This should be dev_dbg().

> + atkbd->use_fw_keymap = true;
> + kfree(ptr);
> + }
> }
>
> /*
> --
> 2.25.1.696.g5e7596f4ac-goog
>

Thanks.

--
Dmitry

2020-03-26 21:26:42

by Dmitry Torokhov

[permalink] [raw]
Subject: Re: [PATCH v2 1/5] input/serio/i8042: Attach fwnode to serio i8042 kbd device

On Tue, Mar 24, 2020 at 05:35:14AM -0700, Rajat Jain wrote:
> Attach the firmware node to the serio i8042 kbd device so that device
> properties can be passed from the firmware.
>
> Signed-off-by: Rajat Jain <[email protected]>
> ---
> v2: Remove the Change-Id from the commit log
>
> drivers/input/serio/i8042-x86ia64io.h | 1 +
> drivers/input/serio/i8042.c | 3 +++
> 2 files changed, 4 insertions(+)
>
> diff --git a/drivers/input/serio/i8042-x86ia64io.h b/drivers/input/serio/i8042-x86ia64io.h
> index dc974c288e880..ed9ec4310d976 100644
> --- a/drivers/input/serio/i8042-x86ia64io.h
> +++ b/drivers/input/serio/i8042-x86ia64io.h
> @@ -927,6 +927,7 @@ static int i8042_pnp_kbd_probe(struct pnp_dev *dev, const struct pnp_device_id *
> }
> i8042_pnp_id_to_string(dev->id, i8042_kbd_firmware_id,
> sizeof(i8042_kbd_firmware_id));
> + i8042_kbd_fwnode = dev_fwnode(&dev->dev);
>
> /* Keyboard ports are always supposed to be wakeup-enabled */
> device_set_wakeup_enable(&dev->dev, true);
> diff --git a/drivers/input/serio/i8042.c b/drivers/input/serio/i8042.c
> index 20ff2bed3917a..0dddf273afd94 100644
> --- a/drivers/input/serio/i8042.c
> +++ b/drivers/input/serio/i8042.c
> @@ -21,6 +21,7 @@
> #include <linux/i8042.h>
> #include <linux/slab.h>
> #include <linux/suspend.h>
> +#include <linux/property.h>
>
> #include <asm/io.h>
>
> @@ -124,6 +125,7 @@ MODULE_PARM_DESC(unmask_kbd_data, "Unconditional enable (may reveal sensitive da
> static bool i8042_bypass_aux_irq_test;
> static char i8042_kbd_firmware_id[128];
> static char i8042_aux_firmware_id[128];
> +static struct fwnode_handle *i8042_kbd_fwnode;
>
> #include "i8042.h"
>
> @@ -1335,6 +1337,7 @@ static int __init i8042_create_kbd_port(void)
> strlcpy(serio->phys, I8042_KBD_PHYS_DESC, sizeof(serio->phys));
> strlcpy(serio->firmware_id, i8042_kbd_firmware_id,
> sizeof(serio->firmware_id));
> + set_primary_fwnode(&serio->dev, i8042_kbd_fwnode);

Rafael, do you have any concerns here? We want to be able to access
properties assigned to KBC or similar device in ACPI. Serio port devices
in i8042 essentially mirror the 2 PNP (typically called KBC, PS2K or
PS2M) devices in ACPI.

Thanks.

--
Dmitry

2020-03-26 21:29:07

by Dmitry Torokhov

[permalink] [raw]
Subject: Re: [PATCH v2 2/5] Input: atkbd: Expose function row physical map to userspace

On Tue, Mar 24, 2020 at 05:35:15AM -0700, Rajat Jain wrote:
> Certain keyboards have their top-row keys intended
> for actions such as "Browser back", "Browser Refresh", "Fullscreen"
> etc as their primary mode, thus they will send physical codes for those
> actions. Further, they don't have a dedicated "Fn" key so don't have
> the capability to generate function key codes (e.g. F1, F2 etc..).
> However in this case, if userspace still wants to "synthesize" those
> function keys using the top row action keys, it needs to know the
> physical position of the top row keys. (Essentially a mapping between
> usage codes and a physical location in the top row).
>
> This patch enhances the atkbd driver to receive such a mapping from the
> firmware / device tree, and expose it to userspace in the form of
> a function-row-physmap attribute. The attribute would be a space
> separated ordered list of physical codes, for the keys in the function
> row, in left-to-right order.
>
> The attribute will only be present if the kernel knows about such
> mapping, otherwise the attribute shall not be visible.
>
> Signed-off-by: Rajat Jain <[email protected]>
> ---
> v2: Remove the Change-Id from the commit log
>
> drivers/input/keyboard/atkbd.c | 59 ++++++++++++++++++++++++++++++++++
> 1 file changed, 59 insertions(+)
>
> diff --git a/drivers/input/keyboard/atkbd.c b/drivers/input/keyboard/atkbd.c
> index 7e3eae54c1926..7623eebef2593 100644
> --- a/drivers/input/keyboard/atkbd.c
> +++ b/drivers/input/keyboard/atkbd.c
> @@ -24,6 +24,7 @@
> #include <linux/libps2.h>
> #include <linux/mutex.h>
> #include <linux/dmi.h>
> +#include <linux/property.h>
>
> #define DRIVER_DESC "AT and PS/2 keyboard driver"
>
> @@ -63,6 +64,8 @@ static bool atkbd_terminal;
> module_param_named(terminal, atkbd_terminal, bool, 0);
> MODULE_PARM_DESC(terminal, "Enable break codes on an IBM Terminal keyboard connected via AT/PS2");
>
> +#define MAX_FUNCTION_ROW_KEYS 24
> +
> /*
> * Scancode to keycode tables. These are just the default setting, and
> * are loadable via a userland utility.
> @@ -230,6 +233,9 @@ struct atkbd {
>
> /* Serializes reconnect(), attr->set() and event work */
> struct mutex mutex;
> +
> + u16 function_row_physmap[MAX_FUNCTION_ROW_KEYS];
> + int num_function_row_keys;
> };
>
> /*
> @@ -283,6 +289,7 @@ static struct device_attribute atkbd_attr_##_name = \
> __ATTR(_name, S_IRUGO, atkbd_do_show_##_name, NULL);
>
> ATKBD_DEFINE_RO_ATTR(err_count);
> +ATKBD_DEFINE_RO_ATTR(function_row_physmap);
>
> static struct attribute *atkbd_attributes[] = {
> &atkbd_attr_extra.attr,
> @@ -292,11 +299,42 @@ static struct attribute *atkbd_attributes[] = {
> &atkbd_attr_softrepeat.attr,
> &atkbd_attr_softraw.attr,
> &atkbd_attr_err_count.attr,
> + &atkbd_attr_function_row_physmap.attr,
> NULL
> };
>
> +static ssize_t atkbd_show_function_row_physmap(struct atkbd *atkbd, char *buf)
> +{
> + ssize_t size = 0;
> + int i;
> +
> + if (!atkbd->num_function_row_keys)
> + return 0;
> +
> + for (i = 0; i < atkbd->num_function_row_keys; i++)
> + size += sprintf(buf + size, "%02X ",
> + atkbd->function_row_physmap[i]);
> + size += sprintf(buf + size, "\n");
> + return size;
> +}
> +
> +static umode_t atkbd_attr_is_visible(struct kobject *kobj,
> + struct attribute *attr, int i)
> +{
> + struct device *dev = container_of(kobj, struct device, kobj);
> + struct serio *serio = to_serio_port(dev);
> + struct atkbd *atkbd = serio_get_drvdata(serio);
> +
> + if (attr == &atkbd_attr_function_row_physmap.attr &&
> + !atkbd->num_function_row_keys)
> + return 0;
> +
> + return attr->mode;
> +}
> +
> static struct attribute_group atkbd_attribute_group = {
> .attrs = atkbd_attributes,
> + .is_visible = atkbd_attr_is_visible,
> };
>
> static const unsigned int xl_table[] = {
> @@ -1121,6 +1159,25 @@ static void atkbd_set_device_attrs(struct atkbd *atkbd)
> }
> }
>
> +static void atkbd_parse_fwnode_data(struct serio *serio)
> +{
> + struct atkbd *atkbd = serio_get_drvdata(serio);
> + struct device *dev = &serio->dev;
> + int n;
> +
> + if (!dev_fwnode(dev))
> + return;

I do not think we need this guard.

> +
> + /* Parse "function-row-physmap" property */
> + n = device_property_count_u16(dev, "function-row-physmap");
> + if (n > 0 && n <= MAX_FUNCTION_ROW_KEYS &&
> + !device_property_read_u16_array(dev, "function-row-physmap",
> + atkbd->function_row_physmap, n)) {
> + atkbd->num_function_row_keys = n;
> + dev_info(dev, "FW reported %d function-row key locations\n", n);

dev_dbg().

> + }
> +}
> +
> /*
> * atkbd_connect() is called when the serio module finds an interface
> * that isn't handled yet by an appropriate device driver. We check if
> @@ -1184,6 +1241,8 @@ static int atkbd_connect(struct serio *serio, struct serio_driver *drv)
> atkbd->id = 0xab00;
> }
>
> + atkbd_parse_fwnode_data(serio);
> +
> atkbd_set_keycode_table(atkbd);
> atkbd_set_device_attrs(atkbd);
>
> --
> 2.25.1.696.g5e7596f4ac-goog
>

--
Dmitry

2020-03-26 21:29:59

by Dmitry Torokhov

[permalink] [raw]
Subject: Re: [PATCH v2 5/5] dt-bindings: input/atkbd.txt: Add binding info for "keymap" property

On Tue, Mar 24, 2020 at 05:35:18AM -0700, Rajat Jain wrote:
> Add the info for keymap property that allows firmware to specify the
> mapping from physical code to linux keycode, that the kernel should use.
>
> Signed-off-by: Rajat Jain <[email protected]>
> ---
> v2: Remove the Change-Id from the commit log
>
> .../devicetree/bindings/input/atkbd.txt | 27 ++++++++++++++++++-
> 1 file changed, 26 insertions(+), 1 deletion(-)
>
> diff --git a/Documentation/devicetree/bindings/input/atkbd.txt b/Documentation/devicetree/bindings/input/atkbd.txt
> index 816653eb8e98d..0a0037d70adc8 100644
> --- a/Documentation/devicetree/bindings/input/atkbd.txt
> +++ b/Documentation/devicetree/bindings/input/atkbd.txt
> @@ -6,9 +6,15 @@ Optional properties:
> An ordered array of the physical codes for the function
> row keys. Arranged in order from left to right.
>
> + keymap:

This map will contain Linux specific values, so I would say
"linux,keymap". Rob?

> + An array of the u32 entries to specify mapping from the
> + keyboard physcial codes to linux keycodes. The top 16
> + bits of each entry are the physical code, and bottom
> + 16 bits are the linux keycode.
> +
> Example:
>
> - This is a sample ACPI _DSD node describing the property:
> + This is a sample ACPI _DSD node describing the properties:
>
> Name (_DSD, Package () {
> ToUUID("daffd814-6eba-4d8c-8a91-bc9bbf4aa301"),
> @@ -29,6 +35,25 @@ Example:
> 0xAE, /* T12 VOL_DOWN */
> 0xB0, /* T13 VOL_UP */
> }
> + },
> + Package () { "keymap",
> + Package () {
> + 0xEA009E, /* EA -> KEY_BACK */
> + 0xE700AD, /* E7 -> KEY_REFRESH */
> + 0x910174, /* 91 -> KEY_FULL_SCREEN */
> + 0x920078, /* 92 -> KEY_SCALE */
> + 0x930280, /* 93 -> 0x280 */
> + 0x9400E0, /* 94 -> KEY_BRIGHTNESS_DOWN*/
> + 0x9500E1, /* 95 -> KEY_BRIGHTNESS_UP */
> + 0x960279, /* 96 -> KEY_PRIVACY_SCRN_TOGGLE*/
> + 0x9700E5, /* 97 -> KEY_KBDILLUMDOWN */
> + 0x9800E6, /* 98 -> KEY_KBDILLUMUP */
> + 0xA00071, /* A0 -> KEY_MUTE */
> + 0xAE0072, /* AE -> KEY_VOLUMEDOWN */
> + 0xB00073, /* B0 -> KEY_VOLUMEUP */
> + ...
> + <snip other entries>
> + }
> }
> }
> })
> --
> 2.25.1.696.g5e7596f4ac-goog
>

Thanks.

--
Dmitry

2020-03-26 22:20:02

by Rajat Jain

[permalink] [raw]
Subject: Re: [PATCH v2 2/5] Input: atkbd: Expose function row physical map to userspace

On Thu, Mar 26, 2020 at 2:27 PM Dmitry Torokhov
<[email protected]> wrote:
>
> On Tue, Mar 24, 2020 at 05:35:15AM -0700, Rajat Jain wrote:
> > Certain keyboards have their top-row keys intended
> > for actions such as "Browser back", "Browser Refresh", "Fullscreen"
> > etc as their primary mode, thus they will send physical codes for those
> > actions. Further, they don't have a dedicated "Fn" key so don't have
> > the capability to generate function key codes (e.g. F1, F2 etc..).
> > However in this case, if userspace still wants to "synthesize" those
> > function keys using the top row action keys, it needs to know the
> > physical position of the top row keys. (Essentially a mapping between
> > usage codes and a physical location in the top row).
> >
> > This patch enhances the atkbd driver to receive such a mapping from the
> > firmware / device tree, and expose it to userspace in the form of
> > a function-row-physmap attribute. The attribute would be a space
> > separated ordered list of physical codes, for the keys in the function
> > row, in left-to-right order.
> >
> > The attribute will only be present if the kernel knows about such
> > mapping, otherwise the attribute shall not be visible.
> >
> > Signed-off-by: Rajat Jain <[email protected]>
> > ---
> > v2: Remove the Change-Id from the commit log
> >
> > drivers/input/keyboard/atkbd.c | 59 ++++++++++++++++++++++++++++++++++
> > 1 file changed, 59 insertions(+)
> >
> > diff --git a/drivers/input/keyboard/atkbd.c b/drivers/input/keyboard/atkbd.c
> > index 7e3eae54c1926..7623eebef2593 100644
> > --- a/drivers/input/keyboard/atkbd.c
> > +++ b/drivers/input/keyboard/atkbd.c
> > @@ -24,6 +24,7 @@
> > #include <linux/libps2.h>
> > #include <linux/mutex.h>
> > #include <linux/dmi.h>
> > +#include <linux/property.h>
> >
> > #define DRIVER_DESC "AT and PS/2 keyboard driver"
> >
> > @@ -63,6 +64,8 @@ static bool atkbd_terminal;
> > module_param_named(terminal, atkbd_terminal, bool, 0);
> > MODULE_PARM_DESC(terminal, "Enable break codes on an IBM Terminal keyboard connected via AT/PS2");
> >
> > +#define MAX_FUNCTION_ROW_KEYS 24
> > +
> > /*
> > * Scancode to keycode tables. These are just the default setting, and
> > * are loadable via a userland utility.
> > @@ -230,6 +233,9 @@ struct atkbd {
> >
> > /* Serializes reconnect(), attr->set() and event work */
> > struct mutex mutex;
> > +
> > + u16 function_row_physmap[MAX_FUNCTION_ROW_KEYS];
> > + int num_function_row_keys;
> > };
> >
> > /*
> > @@ -283,6 +289,7 @@ static struct device_attribute atkbd_attr_##_name = \
> > __ATTR(_name, S_IRUGO, atkbd_do_show_##_name, NULL);
> >
> > ATKBD_DEFINE_RO_ATTR(err_count);
> > +ATKBD_DEFINE_RO_ATTR(function_row_physmap);
> >
> > static struct attribute *atkbd_attributes[] = {
> > &atkbd_attr_extra.attr,
> > @@ -292,11 +299,42 @@ static struct attribute *atkbd_attributes[] = {
> > &atkbd_attr_softrepeat.attr,
> > &atkbd_attr_softraw.attr,
> > &atkbd_attr_err_count.attr,
> > + &atkbd_attr_function_row_physmap.attr,
> > NULL
> > };
> >
> > +static ssize_t atkbd_show_function_row_physmap(struct atkbd *atkbd, char *buf)
> > +{
> > + ssize_t size = 0;
> > + int i;
> > +
> > + if (!atkbd->num_function_row_keys)
> > + return 0;
> > +
> > + for (i = 0; i < atkbd->num_function_row_keys; i++)
> > + size += sprintf(buf + size, "%02X ",
> > + atkbd->function_row_physmap[i]);
> > + size += sprintf(buf + size, "\n");
> > + return size;
> > +}
> > +
> > +static umode_t atkbd_attr_is_visible(struct kobject *kobj,
> > + struct attribute *attr, int i)
> > +{
> > + struct device *dev = container_of(kobj, struct device, kobj);
> > + struct serio *serio = to_serio_port(dev);
> > + struct atkbd *atkbd = serio_get_drvdata(serio);
> > +
> > + if (attr == &atkbd_attr_function_row_physmap.attr &&
> > + !atkbd->num_function_row_keys)
> > + return 0;
> > +
> > + return attr->mode;
> > +}
> > +
> > static struct attribute_group atkbd_attribute_group = {
> > .attrs = atkbd_attributes,
> > + .is_visible = atkbd_attr_is_visible,
> > };
> >
> > static const unsigned int xl_table[] = {
> > @@ -1121,6 +1159,25 @@ static void atkbd_set_device_attrs(struct atkbd *atkbd)
> > }
> > }
> >
> > +static void atkbd_parse_fwnode_data(struct serio *serio)
> > +{
> > + struct atkbd *atkbd = serio_get_drvdata(serio);
> > + struct device *dev = &serio->dev;
> > + int n;
> > +
> > + if (!dev_fwnode(dev))
> > + return;
>
> I do not think we need this guard.

Done.

>
> > +
> > + /* Parse "function-row-physmap" property */
> > + n = device_property_count_u16(dev, "function-row-physmap");
> > + if (n > 0 && n <= MAX_FUNCTION_ROW_KEYS &&
> > + !device_property_read_u16_array(dev, "function-row-physmap",
> > + atkbd->function_row_physmap, n)) {
> > + atkbd->num_function_row_keys = n;
> > + dev_info(dev, "FW reported %d function-row key locations\n", n);
>
> dev_dbg().

Done.

>
> > + }
> > +}
> > +
> > /*
> > * atkbd_connect() is called when the serio module finds an interface
> > * that isn't handled yet by an appropriate device driver. We check if
> > @@ -1184,6 +1241,8 @@ static int atkbd_connect(struct serio *serio, struct serio_driver *drv)
> > atkbd->id = 0xab00;
> > }
> >
> > + atkbd_parse_fwnode_data(serio);
> > +
> > atkbd_set_keycode_table(atkbd);
> > atkbd_set_device_attrs(atkbd);
> >
> > --
> > 2.25.1.696.g5e7596f4ac-goog
> >
>
> --
> Dmitry

2020-03-27 01:39:07

by Rajat Jain

[permalink] [raw]
Subject: Re: [PATCH v2 4/5] Input: atkbd: Receive and use physcode->keycode mapping from FW

On Thu, Mar 26, 2020 at 2:20 PM Dmitry Torokhov
<[email protected]> wrote:
>
> Hi Rajat,
>
> On Tue, Mar 24, 2020 at 05:35:17AM -0700, Rajat Jain wrote:
> > Allow the firmware to specify the mapping between the physical
> > code and the linux keycode. This takes the form of a "keymap"
> > property which is an array of u32 values, each value specifying
> > mapping for a key.
> >
> > Signed-off-by: Rajat Jain <[email protected]>
> > ---
> > v2: Remove the Change-Id from the commit log
> >
> > drivers/input/keyboard/atkbd.c | 39 ++++++++++++++++++++++++++++++++--
> > 1 file changed, 37 insertions(+), 2 deletions(-)
> >
> > diff --git a/drivers/input/keyboard/atkbd.c b/drivers/input/keyboard/atkbd.c
> > index 7623eebef2593..c8017a5707581 100644
> > --- a/drivers/input/keyboard/atkbd.c
> > +++ b/drivers/input/keyboard/atkbd.c
> > @@ -66,6 +66,9 @@ MODULE_PARM_DESC(terminal, "Enable break codes on an IBM Terminal keyboard conne
> >
> > #define MAX_FUNCTION_ROW_KEYS 24
> >
> > +#define PHYSCODE(keymap) ((keymap >> 16) & 0xFFFF)
> > +#define KEYCODE(keymap) (keymap & 0xFFFF)
> > +
> > /*
> > * Scancode to keycode tables. These are just the default setting, and
> > * are loadable via a userland utility.
> > @@ -236,6 +239,9 @@ struct atkbd {
> >
> > u16 function_row_physmap[MAX_FUNCTION_ROW_KEYS];
> > int num_function_row_keys;
> > +
> > + unsigned short fw_keymap[ATKBD_KEYMAP_SIZE];
> > + bool use_fw_keymap;
>
> Why do we need to keep firmware-provided keymap in atkbd instance? It is
> not going anywhere and can be accessed via device_property_* API
> whenever we decide to refresh the keymap.

Done. I've sent out a new v3 patchset for review with this change.

>
> > };
> >
> > /*
> > @@ -1045,7 +1051,10 @@ static void atkbd_set_keycode_table(struct atkbd *atkbd)
> > memset(atkbd->keycode, 0, sizeof(atkbd->keycode));
> > bitmap_zero(atkbd->force_release_mask, ATKBD_KEYMAP_SIZE);
> >
> > - if (atkbd->translated) {
> > + if (atkbd->use_fw_keymap) {
> > + memcpy(atkbd->keycode, atkbd->fw_keymap,
> > + sizeof(atkbd->keycode));
> > + } else if (atkbd->translated) {
> > for (i = 0; i < 128; i++) {
> > scancode = atkbd_unxlate_table[i];
> > atkbd->keycode[i] = atkbd_set2_keycode[scancode];
> > @@ -1163,7 +1172,9 @@ static void atkbd_parse_fwnode_data(struct serio *serio)
> > {
> > struct atkbd *atkbd = serio_get_drvdata(serio);
> > struct device *dev = &serio->dev;
> > - int n;
> > + int i, n;
> > + u32 *ptr;
> > + u16 physcode, keycode;
> >
> > if (!dev_fwnode(dev))
> > return;
> > @@ -1176,6 +1187,30 @@ static void atkbd_parse_fwnode_data(struct serio *serio)
> > atkbd->num_function_row_keys = n;
> > dev_info(dev, "FW reported %d function-row key locations\n", n);
> > }
> > +
> > + /* Parse "keymap" property */
> > + n = device_property_count_u32(dev, "keymap");
> > + if (n > 0 && n <= ATKBD_KEYMAP_SIZE) {
> > +
> > + ptr = kcalloc(n, sizeof(u32), GFP_KERNEL);
> > + if (!ptr)
> > + return;
> > +
> > + if (device_property_read_u32_array(dev, "keymap", ptr, n)) {
> > + dev_err(dev, "problem parsing FW keymap property\n");
> > + kfree(ptr);
> > + return;
> > + }
> > +
> > + for (i = 0; i < n; i++) {
> > + physcode = PHYSCODE(ptr[i]);
> > + keycode = KEYCODE(ptr[i]);
> > + atkbd->fw_keymap[physcode] = keycode;
> > + }
> > + dev_info(dev, "Using FW keymap (%d keys)\n", n);
>
> This should be dev_dbg().

Done.

Thanks,

Rajat

>
> > + atkbd->use_fw_keymap = true;
> > + kfree(ptr);
> > + }
> > }
> >
> > /*
> > --
> > 2.25.1.696.g5e7596f4ac-goog
> >
>
> Thanks.
>
> --
> Dmitry