Re: [PATCH v5 2/2] Input: cros-ec-keyb - Expose function row physical map to userspace

Re: [PATCH v5 2/2] Input: cros-ec-keyb - Expose function row physical map to userspace

[kernel test robot]

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

CC: kbuild-all(a)lists.01.org
In-Reply-To: <20210113172450.v5.2.I38e90f114f0311b8aa3bcfff750ba381c09dd3b6@changeid>
References: <20210113172450.v5.2.I38e90f114f0311b8aa3bcfff750ba381c09dd3b6(a)changeid>
TO: Philip Chen <philipchen@chromium.org>
TO: LKML <linux-kernel@vger.kernel.org>
TO: dmitry.torokhov(a)gmail.com
CC: dianders(a)chromium.org
CC: swboyd(a)chromium.org
CC: Philip Chen <philipchen@chromium.org>
CC: Benson Leung <bleung@chromium.org>
CC: Enric Balletbo i Serra <enric.balletbo@collabora.com>
CC: Guenter Roeck <groeck@chromium.org>
CC: Lee Jones <lee.jones@linaro.org>
CC: Rajat Jain <rajatja@google.com>

Hi Philip,

Thank you for the patch! Perhaps something to improve:

[auto build test WARNING on linus/master]
[also build test WARNING on v5.11-rc3 next-20210113]
[cannot apply to input/next linux/master chrome-platform-linux/for-next]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch]

url:    https://github.com/0day-ci/linux/commits/Philip-Chen/dt-bindings-input-cros-ec-keyb-Add-a-new-property/20210114-094548
base:   https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git 65f0d2414b7079556fbbcc070b3d1c9f9587606d
:::::: branch date: 4 hours ago
:::::: commit date: 4 hours ago
config: i386-randconfig-c001-20210114 (attached as .config)
compiler: gcc-9 (Debian 9.3.0-15) 9.3.0

If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot <lkp@intel.com>
Reported-by: Julia Lawall <julia.lawall@lip6.fr>


"coccinelle warnings: (new ones prefixed by >>)"
>> drivers/input/keyboard/cros_ec_keyb.c:644:60-61: WARNING opportunity for kobj_to_dev()

Please review and possibly fold the followup patch.

---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/kbuild-all(a)lists.01.org

[-- Attachment #2: config.gz --]
[-- Type: application/gzip, Size: 33818 bytes --]

[PATCH] Input: fix kobj_to_dev.cocci warnings

[kernel test robot]

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

CC: kbuild-all(a)lists.01.org
In-Reply-To: <20210113172450.v5.2.I38e90f114f0311b8aa3bcfff750ba381c09dd3b6@changeid>
References: <20210113172450.v5.2.I38e90f114f0311b8aa3bcfff750ba381c09dd3b6(a)changeid>
TO: Philip Chen <philipchen@chromium.org>
TO: LKML <linux-kernel@vger.kernel.org>
TO: dmitry.torokhov(a)gmail.com
CC: dianders(a)chromium.org
CC: swboyd(a)chromium.org
CC: Philip Chen <philipchen@chromium.org>
CC: Benson Leung <bleung@chromium.org>
CC: Enric Balletbo i Serra <enric.balletbo@collabora.com>
CC: Guenter Roeck <groeck@chromium.org>
CC: Lee Jones <lee.jones@linaro.org>
CC: Rajat Jain <rajatja@google.com>

From: kernel test robot <lkp@intel.com>

drivers/input/keyboard/cros_ec_keyb.c:644:60-61: WARNING opportunity for kobj_to_dev()


 Use kobj_to_dev() instead of container_of()

Generated by: scripts/coccinelle/api/kobj_to_dev.cocci

CC: Philip Chen <philipchen@chromium.org>
Reported-by: kernel test robot <lkp@intel.com>
Signed-off-by: kernel test robot <lkp@intel.com>
---

url:    https://github.com/0day-ci/linux/commits/Philip-Chen/dt-bindings-input-cros-ec-keyb-Add-a-new-property/20210114-094548
base:   https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git 65f0d2414b7079556fbbcc070b3d1c9f9587606d
:::::: branch date: 4 hours ago
:::::: commit date: 4 hours ago

Please take the patch only if it's a positive warning. Thanks!

 cros_ec_keyb.c |    2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

--- a/drivers/input/keyboard/cros_ec_keyb.c
+++ b/drivers/input/keyboard/cros_ec_keyb.c
@@ -641,7 +641,7 @@ static umode_t cros_ec_keyb_attr_is_visi
 					    struct attribute *attr,
 					    int n)
 {
-	struct device *dev = container_of(kobj, struct device, kobj);
+	struct device *dev = kobj_to_dev(kobj);
 	struct cros_ec_keyb *ckdev = dev_get_drvdata(dev);
 
 	if (attr == &dev_attr_function_row_physmap.attr &&

[PATCH v5 1/2] dt-bindings: input: cros-ec-keyb: Add a new property

[Philip Chen]

This patch adds a new property `function-row-physmap` to the
device tree for the custom keyboard top row design.

The property describes the rows/columns of the top row keys
from left to right.

Signed-off-by: Philip Chen <philipchen@chromium.org>
---

Changes in v5:
- add minItems and maxItems for `function-row-physmap`

Changes in v2:
- add `function-row-physmap` instead of `google,custom-keyb-top-row`

 .../bindings/input/google,cros-ec-keyb.yaml          | 12 ++++++++++++
 1 file changed, 12 insertions(+)

diff --git a/Documentation/devicetree/bindings/input/google,cros-ec-keyb.yaml b/Documentation/devicetree/bindings/input/google,cros-ec-keyb.yaml
index 8e50c14a9d778..e573ef3e58b65 100644
--- a/Documentation/devicetree/bindings/input/google,cros-ec-keyb.yaml
+++ b/Documentation/devicetree/bindings/input/google,cros-ec-keyb.yaml
@@ -31,6 +31,18 @@ properties:
       if the EC does not have its own logic or hardware for this.
     type: boolean
 
+  function-row-physmap:
+    $ref: '/schemas/types.yaml#/definitions/uint32-array'
+    minItems: 1
+    maxItems: 15
+    description: |
+      An ordered u32 array describing the rows/columns (in the scan matrix)
+      of top row keys from physical left (KEY_F1) to right. Each entry
+      encodes the row/column as:
+      (((row) & 0xFF) << 24) | (((column) & 0xFF) << 16)
+      where the lower 16 bits are reserved. This property is specified only
+      when the keyboard has a custom design for the top row keys.
+
 required:
   - compatible
 
-- 
2.26.2

[PATCH v5 2/2] Input: cros-ec-keyb - Expose function row physical map to userspace

[Philip Chen]

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 v5:
- change the data type for a few local variables
- update function_row_physmap_show() and cros_ec_keyb_register_matrix()
  to improve readability/efficiency

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 | 76 +++++++++++++++++++++++++++
 1 file changed, 76 insertions(+)

diff --git a/drivers/input/keyboard/cros_ec_keyb.c b/drivers/input/keyboard/cros_ec_keyb.c
index b379ed7628781..273e3c9ba0b03 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];
+	size_t num_function_row_keys;
 };
 
 /**
@@ -527,6 +535,10 @@ static int cros_ec_keyb_register_matrix(struct cros_ec_keyb *ckdev)
 	struct input_dev *idev;
 	const char *phys;
 	int err;
+	struct property *prop;
+	const __be32 *p;
+	u32 key_pos;
+	int i = 0;
 
 	err = matrix_keypad_parse_properties(dev, &ckdev->rows, &ckdev->cols);
 	if (err)
@@ -578,6 +590,19 @@ static int cros_ec_keyb_register_matrix(struct cros_ec_keyb *ckdev)
 	ckdev->idev = idev;
 	cros_ec_keyb_compute_valid_keys(ckdev);
 
+	of_property_for_each_u32(dev->of_node, "function-row-physmap",
+				 prop, p, key_pos) {
+		if (i >= MAX_NUM_TOP_ROW_KEYS) {
+			dev_err(dev, "Only support up to %d top row keys.\n",
+				MAX_NUM_TOP_ROW_KEYS);
+			break;
+		}
+		ckdev->function_row_physmap[i] = MATRIX_SCAN_CODE(
+			KEY_ROW(key_pos), KEY_COL(key_pos), ckdev->row_shift);
+		i++;
+	}
+	ckdev->num_function_row_keys = i;
+
 	err = input_register_device(ckdev->idev);
 	if (err) {
 		dev_err(dev, "cannot register input device\n");
@@ -587,6 +612,51 @@ 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;
+	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;
+}
+
+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 +687,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);
-- 
2.26.2

Re: [PATCH v5 2/2] Input: cros-ec-keyb - Expose function row physical map to userspace

[Stephen Boyd]

Quoting Philip Chen (2021-01-13 17:25:13)
> diff --git a/drivers/input/keyboard/cros_ec_keyb.c b/drivers/input/keyboard/cros_ec_keyb.c
> index b379ed7628781..273e3c9ba0b03 100644
> --- a/drivers/input/keyboard/cros_ec_keyb.c
> +++ b/drivers/input/keyboard/cros_ec_keyb.c
> @@ -578,6 +590,19 @@ static int cros_ec_keyb_register_matrix(struct cros_ec_keyb *ckdev)
>         ckdev->idev = idev;
>         cros_ec_keyb_compute_valid_keys(ckdev);
>  
> +       of_property_for_each_u32(dev->of_node, "function-row-physmap",
> +                                prop, p, key_pos) {
> +               if (i >= MAX_NUM_TOP_ROW_KEYS) {
> +                       dev_err(dev, "Only support up to %d top row keys.\n",

dev_warn? And drop the period please as we don't add them in the kernel
usually.

> +                               MAX_NUM_TOP_ROW_KEYS);
> +                       break;
> +               }
> +               ckdev->function_row_physmap[i] = MATRIX_SCAN_CODE(

Maybe grow some more local variables, like for function_row_physmap so
this can fit on one line?

		row = KEY_ROW(key_pos);
		col = KEY_COL(key_pos);
		map[i] = MATRIX_SCAN_CODE(row, col, ckdev->row_shift);

> +                       KEY_ROW(key_pos), KEY_COL(key_pos), ckdev->row_shift);
> +               i++;

We could remove this and just increment num_function_row_keys instead.
Then that condition check may be a little longer but probably still ok.


> +       }
> +       ckdev->num_function_row_keys = i;
> +
>         err = input_register_device(ckdev->idev);
>         if (err) {
>                 dev_err(dev, "cannot register input device\n");

Re: [PATCH v5 2/2] Input: cros-ec-keyb - Expose function row physical map to userspace

[Philip Chen]

On Wed, Jan 13, 2021 at 5:36 PM Stephen Boyd <swboyd@chromium.org> wrote:
>
> Quoting Philip Chen (2021-01-13 17:25:13)
> > diff --git a/drivers/input/keyboard/cros_ec_keyb.c b/drivers/input/keyboard/cros_ec_keyb.c
> > index b379ed7628781..273e3c9ba0b03 100644
> > --- a/drivers/input/keyboard/cros_ec_keyb.c
> > +++ b/drivers/input/keyboard/cros_ec_keyb.c
> > @@ -578,6 +590,19 @@ static int cros_ec_keyb_register_matrix(struct cros_ec_keyb *ckdev)
> >         ckdev->idev = idev;
> >         cros_ec_keyb_compute_valid_keys(ckdev);
> >
> > +       of_property_for_each_u32(dev->of_node, "function-row-physmap",
> > +                                prop, p, key_pos) {
> > +               if (i >= MAX_NUM_TOP_ROW_KEYS) {
> > +                       dev_err(dev, "Only support up to %d top row keys.\n",
>
> dev_warn? And drop the period please as we don't add them in the kernel
> usually.
Done.
>
> > +                               MAX_NUM_TOP_ROW_KEYS);
> > +                       break;
> > +               }
> > +               ckdev->function_row_physmap[i] = MATRIX_SCAN_CODE(
>
> Maybe grow some more local variables, like for function_row_physmap so
> this can fit on one line?
Done. Please take a look at v6.
>
>                 row = KEY_ROW(key_pos);
>                 col = KEY_COL(key_pos);
>                 map[i] = MATRIX_SCAN_CODE(row, col, ckdev->row_shift);
>
> > +                       KEY_ROW(key_pos), KEY_COL(key_pos), ckdev->row_shift);
> > +               i++;
>
> We could remove this and just increment num_function_row_keys instead.
> Then that condition check may be a little longer but probably still ok.
Done. Please take a look at v6.
>
>
> > +       }
> > +       ckdev->num_function_row_keys = i;
> > +
> >         err = input_register_device(ckdev->idev);
> >         if (err) {
> >                 dev_err(dev, "cannot register input device\n");