| Message ID | 20210107154200.v4.2.Ibe7d7d53c5b4fe72c60de90111ff763b53f38dbb@changeid (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | [v4,1/2] dt-bindings: input: cros-ec-keyb: Add a new property | expand |
Quoting Philip Chen (2021-01-07 15:42:09) > The top-row keys in a keyboard usually have dual functionalities. > E.g. A function key "F1" is also an action key "Browser back". > > Therefore, when an application receives an action key code from > a top-row key press, the application needs to know how to correlate > the action key code with the function key code and do the conversion > whenever necessary. > > Since the userpace already knows the key scanlines (row/column) > associated with a received key code. Essentially, the userspace only > needs a mapping between the key row/column and the matching physical > location in the top row. > > This patch enhances the cros-ec-keyb driver to create such a mapping > and expose it to userspace in the form of a function-row-physmap > attribute. The attribute would be a space separated ordered list of > row/column codes, for the keys in the function row, in a left-to-right > order. > > The attribute will only be present when the device has a custom design > for the top-row keys. Is it documented in Documentation/ABI/? > > Signed-off-by: Philip Chen <philipchen@chromium.org> > --- > > Changes in v4: > - replace sysfs_create_group() with devm_device_add_group() > - remove an unused member in struct cros_ec_keyb > > Changes in v3: > - parse `function-row-physmap` from DT earlier, when we probe > cros_ec_keyb, and then store the extracted info in struct cros_ec_keyb. > > Changes in v2: > - create function-row-physmap file in sysfs by parsing > `function-row-physmap` property from DT > - assume the device already has a correct keymap to reflect the custom > top-row keys (if they exist) > > drivers/input/keyboard/cros_ec_keyb.c | 78 +++++++++++++++++++++++++++ > 1 file changed, 78 insertions(+) > > diff --git a/drivers/input/keyboard/cros_ec_keyb.c b/drivers/input/keyboard/cros_ec_keyb.c > index b379ed7628781..75d1cb29734ce 100644 > --- a/drivers/input/keyboard/cros_ec_keyb.c > +++ b/drivers/input/keyboard/cros_ec_keyb.c > @@ -27,6 +27,8 @@ > > #include <asm/unaligned.h> > > +#define MAX_NUM_TOP_ROW_KEYS 15 > + Ah, the binding could say max is 15 then. > /** > * struct cros_ec_keyb - Structure representing EC keyboard device > * > @@ -42,6 +44,9 @@ > * @idev: The input device for the matrix keys. > * @bs_idev: The input device for non-matrix buttons and switches (or NULL). > * @notifier: interrupt event notifier for transport devices > + * @function_row_physmap: An array of the encoded rows/columns for the top > + * row function keys, in an order from left to right > + * @num_function_row_keys: The number of top row keys in a custom keyboard > */ > struct cros_ec_keyb { > unsigned int rows; > @@ -58,6 +63,9 @@ struct cros_ec_keyb { > struct input_dev *idev; > struct input_dev *bs_idev; > struct notifier_block notifier; > + > + u16 function_row_physmap[MAX_NUM_TOP_ROW_KEYS]; > + u8 num_function_row_keys; Why not size_t? > }; > > /** > @@ -527,6 +535,8 @@ static int cros_ec_keyb_register_matrix(struct cros_ec_keyb *ckdev) > struct input_dev *idev; > const char *phys; > int err; > + u32 top_row_key_pos[MAX_NUM_TOP_ROW_KEYS] = {0}; > + u8 i; > > err = matrix_keypad_parse_properties(dev, &ckdev->rows, &ckdev->cols); > if (err) > @@ -578,6 +588,22 @@ static int cros_ec_keyb_register_matrix(struct cros_ec_keyb *ckdev) > ckdev->idev = idev; > cros_ec_keyb_compute_valid_keys(ckdev); > > + if (of_property_read_variable_u32_array(dev->of_node, > + "function-row-physmap", > + top_row_key_pos, > + 0, > + MAX_NUM_TOP_ROW_KEYS) > 0) { > + for (i = 0; i < MAX_NUM_TOP_ROW_KEYS; i++) { Can we deindent this once with of_property_for_each_u32()? > + if (!top_row_key_pos[i]) > + break; > + ckdev->function_row_physmap[i] = MATRIX_SCAN_CODE( > + KEY_ROW(top_row_key_pos[i]), > + KEY_COL(top_row_key_pos[i]), And then have a local variable for top_row_key_pos[i] so this is shorter. > + ckdev->row_shift); > + } > + ckdev->num_function_row_keys = i; > + } > + > err = input_register_device(ckdev->idev); > if (err) { > dev_err(dev, "cannot register input device\n"); > @@ -587,6 +613,52 @@ static int cros_ec_keyb_register_matrix(struct cros_ec_keyb *ckdev) > return 0; > } > > +static ssize_t function_row_physmap_show(struct device *dev, > + struct device_attribute *attr, > + char *buf) > +{ > + ssize_t size = 0; > + u8 i; int i? Why u8? Surely the size of a local variable isn't important. > + struct cros_ec_keyb *ckdev = dev_get_drvdata(dev); > + > + if (!ckdev->num_function_row_keys) > + return 0; > + > + for (i = 0; i < ckdev->num_function_row_keys; i++) > + size += scnprintf(buf + size, PAGE_SIZE - size, "%02X ", > + ckdev->function_row_physmap[i]); > + size += scnprintf(buf + size, PAGE_SIZE - size, "\n"); > + > + return size; I'd rather see ssize_t size = 0; int i; struct cros_ec_keyb *ckdev = dev_get_drvdata(dev); u16 *physmap = ckdev->function_row_physmap; for (i = 0; i < ckdev->num_function_row_keys; i++) size += scnprintf(buf + size, PAGE_SIZE - size, "%s%02X", size ? " " : "", physmap[i]); if (size) size += scnprintf(buf + size, PAGE_SIZE - size, "\n"); return size; And I wonder if hex_dump_to_buffer() works for this?
On Mon, Jan 11, 2021 at 6:24 PM Stephen Boyd <swboyd@chromium.org> wrote: > > Quoting Philip Chen (2021-01-07 15:42:09) > > The top-row keys in a keyboard usually have dual functionalities. > > E.g. A function key "F1" is also an action key "Browser back". > > > > Therefore, when an application receives an action key code from > > a top-row key press, the application needs to know how to correlate > > the action key code with the function key code and do the conversion > > whenever necessary. > > > > Since the userpace already knows the key scanlines (row/column) > > associated with a received key code. Essentially, the userspace only > > needs a mapping between the key row/column and the matching physical > > location in the top row. > > > > This patch enhances the cros-ec-keyb driver to create such a mapping > > and expose it to userspace in the form of a function-row-physmap > > attribute. The attribute would be a space separated ordered list of > > row/column codes, for the keys in the function row, in a left-to-right > > order. > > > > The attribute will only be present when the device has a custom design > > for the top-row keys. > > Is it documented in Documentation/ABI/? Not yet. Is it proper to add the documentation to `testing/sysfs-driver-input-keyboard`? > > > > > Signed-off-by: Philip Chen <philipchen@chromium.org> > > --- > > > > Changes in v4: > > - replace sysfs_create_group() with devm_device_add_group() > > - remove an unused member in struct cros_ec_keyb > > > > Changes in v3: > > - parse `function-row-physmap` from DT earlier, when we probe > > cros_ec_keyb, and then store the extracted info in struct cros_ec_keyb. > > > > Changes in v2: > > - create function-row-physmap file in sysfs by parsing > > `function-row-physmap` property from DT > > - assume the device already has a correct keymap to reflect the custom > > top-row keys (if they exist) > > > > drivers/input/keyboard/cros_ec_keyb.c | 78 +++++++++++++++++++++++++++ > > 1 file changed, 78 insertions(+) > > > > diff --git a/drivers/input/keyboard/cros_ec_keyb.c b/drivers/input/keyboard/cros_ec_keyb.c > > index b379ed7628781..75d1cb29734ce 100644 > > --- a/drivers/input/keyboard/cros_ec_keyb.c > > +++ b/drivers/input/keyboard/cros_ec_keyb.c > > @@ -27,6 +27,8 @@ > > > > #include <asm/unaligned.h> > > > > +#define MAX_NUM_TOP_ROW_KEYS 15 > > + > > Ah, the binding could say max is 15 then. Yes, I'll add the documentation to PATCH 1/2. > > > /** > > * struct cros_ec_keyb - Structure representing EC keyboard device > > * > > @@ -42,6 +44,9 @@ > > * @idev: The input device for the matrix keys. > > * @bs_idev: The input device for non-matrix buttons and switches (or NULL). > > * @notifier: interrupt event notifier for transport devices > > + * @function_row_physmap: An array of the encoded rows/columns for the top > > + * row function keys, in an order from left to right > > + * @num_function_row_keys: The number of top row keys in a custom keyboard > > */ > > struct cros_ec_keyb { > > unsigned int rows; > > @@ -58,6 +63,9 @@ struct cros_ec_keyb { > > struct input_dev *idev; > > struct input_dev *bs_idev; > > struct notifier_block notifier; > > + > > + u16 function_row_physmap[MAX_NUM_TOP_ROW_KEYS]; > > + u8 num_function_row_keys; > > Why not size_t? I usually try to use the minimal required bytes for variables, even for local ones. In this case, we only need one byte for num_function_row_keys. Are there any reasons why size_t is better? > > > }; > > > > /** > > @@ -527,6 +535,8 @@ static int cros_ec_keyb_register_matrix(struct cros_ec_keyb *ckdev) > > struct input_dev *idev; > > const char *phys; > > int err; > > + u32 top_row_key_pos[MAX_NUM_TOP_ROW_KEYS] = {0}; > > + u8 i; > > > > err = matrix_keypad_parse_properties(dev, &ckdev->rows, &ckdev->cols); > > if (err) > > @@ -578,6 +588,22 @@ static int cros_ec_keyb_register_matrix(struct cros_ec_keyb *ckdev) > > ckdev->idev = idev; > > cros_ec_keyb_compute_valid_keys(ckdev); > > > > + if (of_property_read_variable_u32_array(dev->of_node, > > + "function-row-physmap", > > + top_row_key_pos, > > + 0, > > + MAX_NUM_TOP_ROW_KEYS) > 0) { > > + for (i = 0; i < MAX_NUM_TOP_ROW_KEYS; i++) { > > Can we deindent this once with of_property_for_each_u32()? Sure, will do. > > > + if (!top_row_key_pos[i]) > > + break; > > + ckdev->function_row_physmap[i] = MATRIX_SCAN_CODE( > > + KEY_ROW(top_row_key_pos[i]), > > + KEY_COL(top_row_key_pos[i]), > > And then have a local variable for top_row_key_pos[i] so this is > shorter. Sure, will do. > > > + ckdev->row_shift); > > + } > > + ckdev->num_function_row_keys = i; > > + } > > + > > err = input_register_device(ckdev->idev); > > if (err) { > > dev_err(dev, "cannot register input device\n"); > > @@ -587,6 +613,52 @@ static int cros_ec_keyb_register_matrix(struct cros_ec_keyb *ckdev) > > return 0; > > } > > > > +static ssize_t function_row_physmap_show(struct device *dev, > > + struct device_attribute *attr, > > + char *buf) > > +{ > > + ssize_t size = 0; > > + u8 i; > > int i? Why u8? Surely the size of a local variable isn't important. The same reason as "u8 num_function_row_keys". Is int better in this case? > > > + struct cros_ec_keyb *ckdev = dev_get_drvdata(dev); > > + > > + if (!ckdev->num_function_row_keys) > > + return 0; > > + > > + for (i = 0; i < ckdev->num_function_row_keys; i++) > > + size += scnprintf(buf + size, PAGE_SIZE - size, "%02X ", > > + ckdev->function_row_physmap[i]); > > + size += scnprintf(buf + size, PAGE_SIZE - size, "\n"); > > + > > + return size; > > I'd rather see > > ssize_t size = 0; > int i; > struct cros_ec_keyb *ckdev = dev_get_drvdata(dev); > u16 *physmap = ckdev->function_row_physmap; > > for (i = 0; i < ckdev->num_function_row_keys; i++) > size += scnprintf(buf + size, PAGE_SIZE - size, > "%s%02X", size ? " " : "", physmap[i]); > > if (size) > size += scnprintf(buf + size, PAGE_SIZE - size, "\n"); > > return size; > > And I wonder if hex_dump_to_buffer() works for this? It seems to work? I'll give it a try. If hex_dump_to_buffer() doesn't work, I'll fall back to the implementation you suggested above. Thanks!
Quoting Philip Chen (2021-01-12 15:55:28) > On Mon, Jan 11, 2021 at 6:24 PM Stephen Boyd <swboyd@chromium.org> wrote: > > > > Quoting Philip Chen (2021-01-07 15:42:09) > > > The top-row keys in a keyboard usually have dual functionalities. > > > E.g. A function key "F1" is also an action key "Browser back". > > > > > > Therefore, when an application receives an action key code from > > > a top-row key press, the application needs to know how to correlate > > > the action key code with the function key code and do the conversion > > > whenever necessary. > > > > > > Since the userpace already knows the key scanlines (row/column) > > > associated with a received key code. Essentially, the userspace only > > > needs a mapping between the key row/column and the matching physical > > > location in the top row. > > > > > > This patch enhances the cros-ec-keyb driver to create such a mapping > > > and expose it to userspace in the form of a function-row-physmap > > > attribute. The attribute would be a space separated ordered list of > > > row/column codes, for the keys in the function row, in a left-to-right > > > order. > > > > > > The attribute will only be present when the device has a custom design > > > for the top-row keys. > > > > Is it documented in Documentation/ABI/? > Not yet. > Is it proper to add the documentation to `testing/sysfs-driver-input-keyboard`? Somewhere in testing is fine. I'm not sure if it is a generic proprty for all keyboards though? What's the path in sysfs? > > > > > > > > /** > > > * struct cros_ec_keyb - Structure representing EC keyboard device > > > * > > > @@ -42,6 +44,9 @@ > > > * @idev: The input device for the matrix keys. > > > * @bs_idev: The input device for non-matrix buttons and switches (or NULL). > > > * @notifier: interrupt event notifier for transport devices > > > + * @function_row_physmap: An array of the encoded rows/columns for the top > > > + * row function keys, in an order from left to right > > > + * @num_function_row_keys: The number of top row keys in a custom keyboard > > > */ > > > struct cros_ec_keyb { > > > unsigned int rows; > > > @@ -58,6 +63,9 @@ struct cros_ec_keyb { > > > struct input_dev *idev; > > > struct input_dev *bs_idev; > > > struct notifier_block notifier; > > > + > > > + u16 function_row_physmap[MAX_NUM_TOP_ROW_KEYS]; > > > + u8 num_function_row_keys; > > > > Why not size_t? > I usually try to use the minimal required bytes for variables, even > for local ones. > In this case, we only need one byte for num_function_row_keys. > Are there any reasons why size_t is better? I suppose to indicate that it's an array size. It's not a super strong argument but the usage of u8 looks like we're trying to save space in a single structure instance (or maybe a couple if there are a few keyboards), when for all I know it actually generates worse code because it has to do some masking operation on the load from memory when it could just load the value directly into a register. > > > > > }; > > > > > > /** > > > @@ -587,6 +613,52 @@ static int cros_ec_keyb_register_matrix(struct cros_ec_keyb *ckdev) > > > return 0; > > > } > > > > > > +static ssize_t function_row_physmap_show(struct device *dev, > > > + struct device_attribute *attr, > > > + char *buf) > > > +{ > > > + ssize_t size = 0; > > > + u8 i; > > > > int i? Why u8? Surely the size of a local variable isn't important. > The same reason as "u8 num_function_row_keys". > Is int better in this case? Yeah int is better because it's a local variable and nobody cares about those extra few bytes. > > > > > + struct cros_ec_keyb *ckdev = dev_get_drvdata(dev); > > > + > > > + if (!ckdev->num_function_row_keys) > > > + return 0; > > > + > > > + for (i = 0; i < ckdev->num_function_row_keys; i++) > > > + size += scnprintf(buf + size, PAGE_SIZE - size, "%02X ", > > > + ckdev->function_row_physmap[i]); > > > + size += scnprintf(buf + size, PAGE_SIZE - size, "\n"); > > > + > > > + return size; > > > > I'd rather see > > > > ssize_t size = 0; > > int i; > > struct cros_ec_keyb *ckdev = dev_get_drvdata(dev); > > u16 *physmap = ckdev->function_row_physmap; > > > > for (i = 0; i < ckdev->num_function_row_keys; i++) > > size += scnprintf(buf + size, PAGE_SIZE - size, > > "%s%02X", size ? " " : "", physmap[i]); > > > > if (size) > > size += scnprintf(buf + size, PAGE_SIZE - size, "\n"); > > > > return size; > > > > And I wonder if hex_dump_to_buffer() works for this? > It seems to work? I'll give it a try. > If hex_dump_to_buffer() doesn't work, I'll fall back to the > implementation you suggested above. Ok sounds good.
On Tue, Jan 12, 2021 at 10:49 PM Stephen Boyd <swboyd@chromium.org> wrote: > > Quoting Philip Chen (2021-01-12 15:55:28) > > On Mon, Jan 11, 2021 at 6:24 PM Stephen Boyd <swboyd@chromium.org> wrote: > > > > > > Quoting Philip Chen (2021-01-07 15:42:09) > > > > The top-row keys in a keyboard usually have dual functionalities. > > > > E.g. A function key "F1" is also an action key "Browser back". > > > > > > > > Therefore, when an application receives an action key code from > > > > a top-row key press, the application needs to know how to correlate > > > > the action key code with the function key code and do the conversion > > > > whenever necessary. > > > > > > > > Since the userpace already knows the key scanlines (row/column) > > > > associated with a received key code. Essentially, the userspace only > > > > needs a mapping between the key row/column and the matching physical > > > > location in the top row. > > > > > > > > This patch enhances the cros-ec-keyb driver to create such a mapping > > > > and expose it to userspace in the form of a function-row-physmap > > > > attribute. The attribute would be a space separated ordered list of > > > > row/column codes, for the keys in the function row, in a left-to-right > > > > order. > > > > > > > > The attribute will only be present when the device has a custom design > > > > for the top-row keys. > > > > > > Is it documented in Documentation/ABI/? > > Not yet. > > Is it proper to add the documentation to `testing/sysfs-driver-input-keyboard`? > > Somewhere in testing is fine. I'm not sure if it is a generic proprty > for all keyboards though? What's the path in sysfs? I wouldn't say it's generic. It is available in the keyboard device node only when the board has a custom top-row keyboard design. The path in sysfs is something like: /sys/class/input/input0/device/function_row_physmap, where input0 is cros_ec. > > > > > > > > > > > > /** > > > > * struct cros_ec_keyb - Structure representing EC keyboard device > > > > * > > > > @@ -42,6 +44,9 @@ > > > > * @idev: The input device for the matrix keys. > > > > * @bs_idev: The input device for non-matrix buttons and switches (or NULL). > > > > * @notifier: interrupt event notifier for transport devices > > > > + * @function_row_physmap: An array of the encoded rows/columns for the top > > > > + * row function keys, in an order from left to right > > > > + * @num_function_row_keys: The number of top row keys in a custom keyboard > > > > */ > > > > struct cros_ec_keyb { > > > > unsigned int rows; > > > > @@ -58,6 +63,9 @@ struct cros_ec_keyb { > > > > struct input_dev *idev; > > > > struct input_dev *bs_idev; > > > > struct notifier_block notifier; > > > > + > > > > + u16 function_row_physmap[MAX_NUM_TOP_ROW_KEYS]; > > > > + u8 num_function_row_keys; > > > > > > Why not size_t? > > I usually try to use the minimal required bytes for variables, even > > for local ones. > > In this case, we only need one byte for num_function_row_keys. > > Are there any reasons why size_t is better? > > I suppose to indicate that it's an array size. It's not a super strong > argument but the usage of u8 looks like we're trying to save space in a > single structure instance (or maybe a couple if there are a few > keyboards), when for all I know it actually generates worse code because > it has to do some masking operation on the load from memory when it > could just load the value directly into a register. OK, I'll do size_t. > > > > > > > > }; > > > > > > > > /** > > > > @@ -587,6 +613,52 @@ static int cros_ec_keyb_register_matrix(struct cros_ec_keyb *ckdev) > > > > return 0; > > > > } > > > > > > > > +static ssize_t function_row_physmap_show(struct device *dev, > > > > + struct device_attribute *attr, > > > > + char *buf) > > > > +{ > > > > + ssize_t size = 0; > > > > + u8 i; > > > > > > int i? Why u8? Surely the size of a local variable isn't important. > > The same reason as "u8 num_function_row_keys". > > Is int better in this case? > > Yeah int is better because it's a local variable and nobody cares about > those extra few bytes. OK, I'll do int. > > > > > > > > + struct cros_ec_keyb *ckdev = dev_get_drvdata(dev); > > > > + > > > > + if (!ckdev->num_function_row_keys) > > > > + return 0; > > > > + > > > > + for (i = 0; i < ckdev->num_function_row_keys; i++) > > > > + size += scnprintf(buf + size, PAGE_SIZE - size, "%02X ", > > > > + ckdev->function_row_physmap[i]); > > > > + size += scnprintf(buf + size, PAGE_SIZE - size, "\n"); > > > > + > > > > + return size; > > > > > > I'd rather see > > > > > > ssize_t size = 0; > > > int i; > > > struct cros_ec_keyb *ckdev = dev_get_drvdata(dev); > > > u16 *physmap = ckdev->function_row_physmap; > > > > > > for (i = 0; i < ckdev->num_function_row_keys; i++) > > > size += scnprintf(buf + size, PAGE_SIZE - size, > > > "%s%02X", size ? " " : "", physmap[i]); > > > > > > if (size) > > > size += scnprintf(buf + size, PAGE_SIZE - size, "\n"); > > > > > > return size; > > > > > > And I wonder if hex_dump_to_buffer() works for this? > > It seems to work? I'll give it a try. > > If hex_dump_to_buffer() doesn't work, I'll fall back to the > > implementation you suggested above. > > Ok sounds good.
Quoting Philip Chen (2021-01-13 14:47:18) > On Tue, Jan 12, 2021 at 10:49 PM Stephen Boyd <swboyd@chromium.org> wrote: > > > > Quoting Philip Chen (2021-01-12 15:55:28) > > > On Mon, Jan 11, 2021 at 6:24 PM Stephen Boyd <swboyd@chromium.org> wrote: > > > > > > > > Quoting Philip Chen (2021-01-07 15:42:09) > > > > > The top-row keys in a keyboard usually have dual functionalities. > > > > > E.g. A function key "F1" is also an action key "Browser back". > > > > > > > > > > Therefore, when an application receives an action key code from > > > > > a top-row key press, the application needs to know how to correlate > > > > > the action key code with the function key code and do the conversion > > > > > whenever necessary. > > > > > > > > > > Since the userpace already knows the key scanlines (row/column) > > > > > associated with a received key code. Essentially, the userspace only > > > > > needs a mapping between the key row/column and the matching physical > > > > > location in the top row. > > > > > > > > > > This patch enhances the cros-ec-keyb driver to create such a mapping > > > > > and expose it to userspace in the form of a function-row-physmap > > > > > attribute. The attribute would be a space separated ordered list of > > > > > row/column codes, for the keys in the function row, in a left-to-right > > > > > order. > > > > > > > > > > The attribute will only be present when the device has a custom design > > > > > for the top-row keys. > > > > > > > > Is it documented in Documentation/ABI/? > > > Not yet. > > > Is it proper to add the documentation to `testing/sysfs-driver-input-keyboard`? > > > > Somewhere in testing is fine. I'm not sure if it is a generic proprty > > for all keyboards though? What's the path in sysfs? > I wouldn't say it's generic. > It is available in the keyboard device node only when the board has a > custom top-row keyboard design. > The path in sysfs is something like: > /sys/class/input/input0/device/function_row_physmap, where input0 is > cros_ec. I see that atkbd already has this so at least it would be common to some sort of keyboard device. I'm not sure where to document it though. I see that atkbd has a handful of undocumented sysfs attributes so adding all of those may lead to a common path. At the least it sounds OK to have a sysfs-driver-input-keyboard file if input folks are OK with it.
On Wed, Jan 13, 2021 at 3:14 PM Stephen Boyd <swboyd@chromium.org> wrote: > > Quoting Philip Chen (2021-01-13 14:47:18) > > On Tue, Jan 12, 2021 at 10:49 PM Stephen Boyd <swboyd@chromium.org> wrote: > > > > > > Quoting Philip Chen (2021-01-12 15:55:28) > > > > On Mon, Jan 11, 2021 at 6:24 PM Stephen Boyd <swboyd@chromium.org> wrote: > > > > > > > > > > Quoting Philip Chen (2021-01-07 15:42:09) > > > > > > The top-row keys in a keyboard usually have dual functionalities. > > > > > > E.g. A function key "F1" is also an action key "Browser back". > > > > > > > > > > > > Therefore, when an application receives an action key code from > > > > > > a top-row key press, the application needs to know how to correlate > > > > > > the action key code with the function key code and do the conversion > > > > > > whenever necessary. > > > > > > > > > > > > Since the userpace already knows the key scanlines (row/column) > > > > > > associated with a received key code. Essentially, the userspace only > > > > > > needs a mapping between the key row/column and the matching physical > > > > > > location in the top row. > > > > > > > > > > > > This patch enhances the cros-ec-keyb driver to create such a mapping > > > > > > and expose it to userspace in the form of a function-row-physmap > > > > > > attribute. The attribute would be a space separated ordered list of > > > > > > row/column codes, for the keys in the function row, in a left-to-right > > > > > > order. > > > > > > > > > > > > The attribute will only be present when the device has a custom design > > > > > > for the top-row keys. > > > > > > > > > > Is it documented in Documentation/ABI/? > > > > Not yet. > > > > Is it proper to add the documentation to `testing/sysfs-driver-input-keyboard`? > > > > > > Somewhere in testing is fine. I'm not sure if it is a generic proprty > > > for all keyboards though? What's the path in sysfs? > > I wouldn't say it's generic. > > It is available in the keyboard device node only when the board has a > > custom top-row keyboard design. > > The path in sysfs is something like: > > /sys/class/input/input0/device/function_row_physmap, where input0 is > > cros_ec. > > I see that atkbd already has this so at least it would be common to some > sort of keyboard device. I'm not sure where to document it though. I see > that atkbd has a handful of undocumented sysfs attributes so adding all > of those may lead to a common path. At the least it sounds OK to have a > sysfs-driver-input-keyboard file if input folks are OK with it. Since there are other undocumented sysfs attributes for input/keyboard anyway, we should probably leave the documentation to another patch? For now, let's move to patch v5, where I've addressed all of the comments so far. Thanks.
Quoting Philip Chen (2021-01-13 17:29:05) > On Wed, Jan 13, 2021 at 3:14 PM Stephen Boyd <swboyd@chromium.org> wrote: > > > > Quoting Philip Chen (2021-01-13 14:47:18) > > > On Tue, Jan 12, 2021 at 10:49 PM Stephen Boyd <swboyd@chromium.org> wrote: > > > > > > > > Quoting Philip Chen (2021-01-12 15:55:28) > > > > > On Mon, Jan 11, 2021 at 6:24 PM Stephen Boyd <swboyd@chromium.org> wrote: > > > > > > > > > > > > Is it documented in Documentation/ABI/? > > > > > Not yet. > > > > > Is it proper to add the documentation to `testing/sysfs-driver-input-keyboard`? > > > > > > > > Somewhere in testing is fine. I'm not sure if it is a generic proprty > > > > for all keyboards though? What's the path in sysfs? > > > I wouldn't say it's generic. > > > It is available in the keyboard device node only when the board has a > > > custom top-row keyboard design. > > > The path in sysfs is something like: > > > /sys/class/input/input0/device/function_row_physmap, where input0 is > > > cros_ec. > > > > I see that atkbd already has this so at least it would be common to some > > sort of keyboard device. I'm not sure where to document it though. I see > > that atkbd has a handful of undocumented sysfs attributes so adding all > > of those may lead to a common path. At the least it sounds OK to have a > > sysfs-driver-input-keyboard file if input folks are OK with it. > Since there are other undocumented sysfs attributes for input/keyboard > anyway, we should probably leave the documentation to another patch? > For now, let's move to patch v5, where I've addressed all of the > comments so far. Please document this one that's being introduced. We should document all the sysfs attributes but we don't always do a good job at it.
On Wed, Jan 13, 2021 at 5:39 PM Stephen Boyd <swboyd@chromium.org> wrote: > > Quoting Philip Chen (2021-01-13 17:29:05) > > On Wed, Jan 13, 2021 at 3:14 PM Stephen Boyd <swboyd@chromium.org> wrote: > > > > > > Quoting Philip Chen (2021-01-13 14:47:18) > > > > On Tue, Jan 12, 2021 at 10:49 PM Stephen Boyd <swboyd@chromium.org> wrote: > > > > > > > > > > Quoting Philip Chen (2021-01-12 15:55:28) > > > > > > On Mon, Jan 11, 2021 at 6:24 PM Stephen Boyd <swboyd@chromium.org> wrote: > > > > > > > > > > > > > > Is it documented in Documentation/ABI/? > > > > > > Not yet. > > > > > > Is it proper to add the documentation to `testing/sysfs-driver-input-keyboard`? > > > > > > > > > > Somewhere in testing is fine. I'm not sure if it is a generic proprty > > > > > for all keyboards though? What's the path in sysfs? > > > > I wouldn't say it's generic. > > > > It is available in the keyboard device node only when the board has a > > > > custom top-row keyboard design. > > > > The path in sysfs is something like: > > > > /sys/class/input/input0/device/function_row_physmap, where input0 is > > > > cros_ec. > > > > > > I see that atkbd already has this so at least it would be common to some > > > sort of keyboard device. I'm not sure where to document it though. I see > > > that atkbd has a handful of undocumented sysfs attributes so adding all > > > of those may lead to a common path. At the least it sounds OK to have a > > > sysfs-driver-input-keyboard file if input folks are OK with it. > > Since there are other undocumented sysfs attributes for input/keyboard > > anyway, we should probably leave the documentation to another patch? > > For now, let's move to patch v5, where I've addressed all of the > > comments so far. > > Please document this one that's being introduced. We should document all > the sysfs attributes but we don't always do a good job at it. OK, will do!
diff --git a/drivers/input/keyboard/cros_ec_keyb.c b/drivers/input/keyboard/cros_ec_keyb.c index b379ed7628781..75d1cb29734ce 100644 --- a/drivers/input/keyboard/cros_ec_keyb.c +++ b/drivers/input/keyboard/cros_ec_keyb.c @@ -27,6 +27,8 @@ #include <asm/unaligned.h> +#define MAX_NUM_TOP_ROW_KEYS 15 + /** * struct cros_ec_keyb - Structure representing EC keyboard device * @@ -42,6 +44,9 @@ * @idev: The input device for the matrix keys. * @bs_idev: The input device for non-matrix buttons and switches (or NULL). * @notifier: interrupt event notifier for transport devices + * @function_row_physmap: An array of the encoded rows/columns for the top + * row function keys, in an order from left to right + * @num_function_row_keys: The number of top row keys in a custom keyboard */ struct cros_ec_keyb { unsigned int rows; @@ -58,6 +63,9 @@ struct cros_ec_keyb { struct input_dev *idev; struct input_dev *bs_idev; struct notifier_block notifier; + + u16 function_row_physmap[MAX_NUM_TOP_ROW_KEYS]; + u8 num_function_row_keys; }; /** @@ -527,6 +535,8 @@ static int cros_ec_keyb_register_matrix(struct cros_ec_keyb *ckdev) struct input_dev *idev; const char *phys; int err; + u32 top_row_key_pos[MAX_NUM_TOP_ROW_KEYS] = {0}; + u8 i; err = matrix_keypad_parse_properties(dev, &ckdev->rows, &ckdev->cols); if (err) @@ -578,6 +588,22 @@ static int cros_ec_keyb_register_matrix(struct cros_ec_keyb *ckdev) ckdev->idev = idev; cros_ec_keyb_compute_valid_keys(ckdev); + if (of_property_read_variable_u32_array(dev->of_node, + "function-row-physmap", + top_row_key_pos, + 0, + MAX_NUM_TOP_ROW_KEYS) > 0) { + for (i = 0; i < MAX_NUM_TOP_ROW_KEYS; i++) { + if (!top_row_key_pos[i]) + break; + ckdev->function_row_physmap[i] = MATRIX_SCAN_CODE( + KEY_ROW(top_row_key_pos[i]), + KEY_COL(top_row_key_pos[i]), + ckdev->row_shift); + } + ckdev->num_function_row_keys = i; + } + err = input_register_device(ckdev->idev); if (err) { dev_err(dev, "cannot register input device\n"); @@ -587,6 +613,52 @@ static int cros_ec_keyb_register_matrix(struct cros_ec_keyb *ckdev) return 0; } +static ssize_t function_row_physmap_show(struct device *dev, + struct device_attribute *attr, + char *buf) +{ + ssize_t size = 0; + u8 i; + struct cros_ec_keyb *ckdev = dev_get_drvdata(dev); + + if (!ckdev->num_function_row_keys) + return 0; + + for (i = 0; i < ckdev->num_function_row_keys; i++) + size += scnprintf(buf + size, PAGE_SIZE - size, "%02X ", + ckdev->function_row_physmap[i]); + size += scnprintf(buf + size, PAGE_SIZE - size, "\n"); + + return size; +} + +static DEVICE_ATTR_RO(function_row_physmap); + +static struct attribute *cros_ec_keyb_attrs[] = { + &dev_attr_function_row_physmap.attr, + NULL, +}; + +static umode_t cros_ec_keyb_attr_is_visible(struct kobject *kobj, + struct attribute *attr, + int n) +{ + struct device *dev = container_of(kobj, struct device, kobj); + struct cros_ec_keyb *ckdev = dev_get_drvdata(dev); + + if (attr == &dev_attr_function_row_physmap.attr && + !ckdev->num_function_row_keys) + return 0; + + return attr->mode; +} + +static const struct attribute_group cros_ec_keyb_attr_group = { + .is_visible = cros_ec_keyb_attr_is_visible, + .attrs = cros_ec_keyb_attrs, +}; + + static int cros_ec_keyb_probe(struct platform_device *pdev) { struct cros_ec_device *ec = dev_get_drvdata(pdev->dev.parent); @@ -617,6 +689,12 @@ static int cros_ec_keyb_probe(struct platform_device *pdev) return err; } + err = devm_device_add_group(dev, &cros_ec_keyb_attr_group); + if (err) { + dev_err(dev, "failed to create attributes. err=%d\n", err); + return err; + } + ckdev->notifier.notifier_call = cros_ec_keyb_work; err = blocking_notifier_chain_register(&ckdev->ec->event_notifier, &ckdev->notifier);
The top-row keys in a keyboard usually have dual functionalities. E.g. A function key "F1" is also an action key "Browser back". Therefore, when an application receives an action key code from a top-row key press, the application needs to know how to correlate the action key code with the function key code and do the conversion whenever necessary. Since the userpace already knows the key scanlines (row/column) associated with a received key code. Essentially, the userspace only needs a mapping between the key row/column and the matching physical location in the top row. This patch enhances the cros-ec-keyb driver to create such a mapping and expose it to userspace in the form of a function-row-physmap attribute. The attribute would be a space separated ordered list of row/column codes, for the keys in the function row, in a left-to-right order. The attribute will only be present when the device has a custom design for the top-row keys. Signed-off-by: Philip Chen <philipchen@chromium.org> --- Changes in v4: - replace sysfs_create_group() with devm_device_add_group() - remove an unused member in struct cros_ec_keyb Changes in v3: - parse `function-row-physmap` from DT earlier, when we probe cros_ec_keyb, and then store the extracted info in struct cros_ec_keyb. Changes in v2: - create function-row-physmap file in sysfs by parsing `function-row-physmap` property from DT - assume the device already has a correct keymap to reflect the custom top-row keys (if they exist) drivers/input/keyboard/cros_ec_keyb.c | 78 +++++++++++++++++++++++++++ 1 file changed, 78 insertions(+)