mbox series

[v5,0/5] Input/HID: Consolidate ChromeOS Vivaldi keyboard logic

Message ID 20220228075446.466016-1-dmitry.torokhov@gmail.com
Headers show
Series Input/HID: Consolidate ChromeOS Vivaldi keyboard logic | expand

Message

Dmitry Torokhov Feb. 28, 2022, 7:54 a.m. UTC
This is a follow-on to this thread[1] where we discussed the need to
support the vivaldi keyboard function row keys in the google hammer
driver. I've extracted the common code into a new vivaldi-fmap.c file
that can be used by the various keyboard drivers used on ChromeOS
devices to expose the function_row_physmap sysfs attribute. Then we make
another file to keep the HID parsing logic common for the vivaldi and
hammer keyboards. Finally, we add support for the function row physmap
attribute to the hammer driver.

NOTE: I dropped Tested-by and Acked-by as patches have been reworked,
please give them another spin.

Changed from v4 (dtor):
(https://lore.kernel.org/r/20220216195901.1326924-1-swboyd@chromium.org):
 * The series is on top of [PATCH] HID: vivaldi: fix sysfs attributes
   leak (https://lore.kernel.org/r/YhmAAjNeTjiNoLlJ@google.com)
 * Added patch to used devm for keyboard backlight LED in hammer driver
 * Avoid putting HID-specific stuff in input header, instead introduce
   new private hid-vivaldi-common.h
 * More code sharing between hid-google-hammer.c and hid-vivaldi.c by
   mandating that vivaldi data instance should be the very first or the
   only driver-private data.

Changes from v3
(https://lore.kernel.org/r/20220211012510.1198155-1-swboyd@chromium.org):
 * Changed vivaldi-keymap to vivaldi-fmap

Changes from v2
(https://lore.kernel.org/r/20220209225556.3992827-1-swboyd@chromium.org):
 * Drop first patch to change to u16
 * Change array type to u32 in vivaldi_data

Changes from v1
(https://lore.kernel.org/r/20220204202021.895426-1-swboyd@chromium.org):
 * Yet another new file for HID part to fix compilation problems

Dmitry Torokhov (1):
  HID: google: switch to devm when registering keyboard backlight LED

Stephen Boyd (3):
  Input: extract ChromeOS vivaldi physmap show function
  HID: google: extract Vivaldi hid feature mapping for use in hid-hammer
  HID: google: Add support for vivaldi to hid-hammer

Zhengqiao Xia (1):
  HID: google: modify HID device groups of eel

 drivers/hid/Kconfig                   |  11 ++
 drivers/hid/Makefile                  |   1 +
 drivers/hid/hid-google-hammer.c       |  51 +++++-----
 drivers/hid/hid-vivaldi-common.c      | 140 ++++++++++++++++++++++++++
 drivers/hid/hid-vivaldi-common.h      |  16 +++
 drivers/hid/hid-vivaldi.c             | 121 +---------------------
 drivers/input/Kconfig                 |   7 ++
 drivers/input/Makefile                |   1 +
 drivers/input/keyboard/Kconfig        |   2 +
 drivers/input/keyboard/atkbd.c        |  27 ++---
 drivers/input/keyboard/cros_ec_keyb.c |  43 +++-----
 drivers/input/vivaldi-fmap.c          |  39 +++++++
 include/linux/input/vivaldi-fmap.h    |  27 +++++
 13 files changed, 296 insertions(+), 190 deletions(-)
 create mode 100644 drivers/hid/hid-vivaldi-common.c
 create mode 100644 drivers/hid/hid-vivaldi-common.h
 create mode 100644 drivers/input/vivaldi-fmap.c
 create mode 100644 include/linux/input/vivaldi-fmap.h

Comments

Stephen Boyd Feb. 28, 2022, 8:56 p.m. UTC | #1
Quoting Dmitry Torokhov (2022-02-27 23:54:44)
> diff --git a/drivers/hid/hid-vivaldi-common.c b/drivers/hid/hid-vivaldi-common.c
> new file mode 100644
> index 000000000000..c88b26f4c78b
> --- /dev/null
> +++ b/drivers/hid/hid-vivaldi-common.c
> @@ -0,0 +1,140 @@
> +// SPDX-License-Identifier: GPL-2.0
[...]
> +/**
> + * vivaldi_feature_mapping - Fill out vivaldi keymap data exposed via HID
> + * @hdev: HID device to parse
> + * @field: HID field to parse
> + * @usage: HID usage to parse
> + *
> + * This function assumes that driver data attached to @hdev contains an

Maybe add

Note: This function assumes ...

> + * instance of &struct vivaldi_data in the very beginning.

It makes me nervous that this can't be checked statically but OK.

> + */
> +void vivaldi_feature_mapping(struct hid_device *hdev,
> +                            struct hid_field *field, struct hid_usage *usage)
> +{
> +       struct vivaldi_data *data = hid_get_drvdata(hdev);
> +       struct hid_report *report = field->report;
> +       u8 *report_data, *buf;
> +       u32 report_len;
> +       unsigned int fn_key;
> +       int ret;
[...]
> +
> +static DEVICE_ATTR_RO(function_row_physmap);
> +static struct attribute *vivaldi_sysfs_attrs[] = {
> +       &dev_attr_function_row_physmap.attr,
> +       NULL
> +};
> +
> +static const struct attribute_group vivaldi_attribute_group = {
> +       .attrs = vivaldi_sysfs_attrs,
> +};
> +
> +/**
> + * vivaldi_feature_mapping - Complete initialization of device using vivaldi map

vivaldi_input_configured

> + * @hdev: HID device to witch vivaldi attributes should be attached

s/witch/which/

> + * @hidinput: HID input device (unused).

Drop the period? It's not on the hdev argument description above.

> + */
> +int vivaldi_input_configured(struct hid_device *hdev,
> +                            struct hid_input *hidinput)
Dmitry Torokhov March 1, 2022, 6:55 a.m. UTC | #2
On Mon, Feb 28, 2022 at 12:57:56PM -0800, Stephen Boyd wrote:
> Quoting Dmitry Torokhov (2022-02-27 23:54:46)
> > From: Zhengqiao Xia <xiazhengqiao@huaqin.corp-partner.google.com>
> >
> > If HID_GROUP of eel is set to HID_GROUP_GENERIC, Whiskers Tablet Mode
> > Switch of eel hammer will not be detected by system because the
> > hid-vivaldi driver probes the device. When it is set to
> > HID_GROUP_VIVALDI, system will detect Whiskers Tablet Mode Switch
> > successfully and also support the vivaldi keyboard layout.
> >
> > Tested-by: "Sean O'Brien" <seobrien@chromium.org>
> > Acked-by: Jiri Kosina <jkosina@suse.cz>
> > Signed-off-by: Zhengqiao Xia <xiazhengqiao@huaqin.corp-partner.google.com>
> > [swboyd@chromium.org: Expand on commit text]
> > Signed-off-by: Stephen Boyd <swboyd@chromium.org>
> > Link: https://lore.kernel.org/r/20220216195901.1326924-5-swboyd@chromium.org
> > Patchwork-Id: 12748989
> 
> Should this patchwork id be removed?

Yeah, this is leftover from my scripts, they will drop it when applying.

Thanks.
Stephen Boyd March 2, 2022, 1:52 a.m. UTC | #3
Quoting Dmitry Torokhov (2022-02-27 23:54:41)
> This is a follow-on to this thread[1] where we discussed the need to
> support the vivaldi keyboard function row keys in the google hammer
> driver. I've extracted the common code into a new vivaldi-fmap.c file
> that can be used by the various keyboard drivers used on ChromeOS
> devices to expose the function_row_physmap sysfs attribute. Then we make
> another file to keep the HID parsing logic common for the vivaldi and
> hammer keyboards. Finally, we add support for the function row physmap
> attribute to the hammer driver.
>
> NOTE: I dropped Tested-by and Acked-by as patches have been reworked,
> please give them another spin.

I tested it on a device with hid-vivaldi (coachz) and a device with
hid-google-hammer (wormdingler) and it works on both. Feel free to add

Tested-by: Stephen Boyd <swboyd@chromium.org> # coachz, wormdingler

to the patches.

>
> Changed from v4 (dtor):
> (https://lore.kernel.org/r/20220216195901.1326924-1-swboyd@chromium.org):
>  * The series is on top of [PATCH] HID: vivaldi: fix sysfs attributes
>    leak (https://lore.kernel.org/r/YhmAAjNeTjiNoLlJ@google.com)
>  * Added patch to used devm for keyboard backlight LED in hammer driver
>  * Avoid putting HID-specific stuff in input header, instead introduce
>    new private hid-vivaldi-common.h
>  * More code sharing between hid-google-hammer.c and hid-vivaldi.c by
>    mandating that vivaldi data instance should be the very first or the
>    only driver-private data.
>