Re: [PATCH 3/5] HID: core: Export some report item parsing functions.

[Benjamin Tissoires]

On Mon, Mar 1, 2021 at 3:18 PM Andy Shevchenko
 wrote:
>
> On Sun, Feb 28, 2021 at 3:30 AM Ronald Tschalär  wrote:
> >
> > These are useful to drivers that need to scan or parse reports
> > themselves.
>
> ...
>
> > -   while ((start = fetch_item(start, end, )) != NULL)
> > +   while ((start = hid_fetch_item(start, end, )) != NULL)
> > dispatch_type[item.type](parser, );
>
> > -   while ((next = fetch_item(start, end, )) != NULL) {
> > +   while ((next = hid_fetch_item(start, end, )) != NULL) {
> > start = next;
>
> I don't see the full picture, but perhaps you may also introduce
> for_each_hid_item() or so.

Same here, I don't see the full picture, but I would suggest to not
export those functions at all.

>From 4/5, I can see that you are using them in
appleib_find_collection(), which is only called by
appleib_add_device(), which in turn is always called with a parsed and
started HID device. Why can you not rely on the core parsing and
iterate over the already parsed hid_field?

Cheers,
Benjamin

Re: [PATCH 3/5] HID: core: Export some report item parsing functions.

[Andy Shevchenko]

On Sun, Feb 28, 2021 at 3:30 AM Ronald Tschalär  wrote:
>
> These are useful to drivers that need to scan or parse reports
> themselves.

...

> -   while ((start = fetch_item(start, end, )) != NULL)
> +   while ((start = hid_fetch_item(start, end, )) != NULL)
> dispatch_type[item.type](parser, );

> -   while ((next = fetch_item(start, end, )) != NULL) {
> +   while ((next = hid_fetch_item(start, end, )) != NULL) {
> start = next;

I don't see the full picture, but perhaps you may also introduce
for_each_hid_item() or so.

-- 
With Best Regards,
Andy Shevchenko

[PATCH 3/5] HID: core: Export some report item parsing functions.

[Ronald Tschalär]

These are useful to drivers that need to scan or parse reports
themselves.

Signed-off-by: Ronald Tschalär 
---
 drivers/hid/hid-core.c | 54 +-
 include/linux/hid.h|  4 
 2 files changed, 36 insertions(+), 22 deletions(-)

diff --git a/drivers/hid/hid-core.c b/drivers/hid/hid-core.c
index a96b252f97366..b4c3d71a0ddda 100644
--- a/drivers/hid/hid-core.c
+++ b/drivers/hid/hid-core.c
@@ -340,7 +340,7 @@ static int hid_add_field(struct hid_parser *parser, 
unsigned report_type, unsign
  * Read data value from item.
  */
 
-static u32 item_udata(struct hid_item *item)
+u32 hid_item_udata(struct hid_item *item)
 {
switch (item->size) {
case 1: return item->data.u8;
@@ -349,8 +349,9 @@ static u32 item_udata(struct hid_item *item)
}
return 0;
 }
+EXPORT_SYMBOL_GPL(hid_item_udata);
 
-static s32 item_sdata(struct hid_item *item)
+s32 hid_item_sdata(struct hid_item *item)
 {
switch (item->size) {
case 1: return item->data.s8;
@@ -359,6 +360,7 @@ static s32 item_sdata(struct hid_item *item)
}
return 0;
 }
+EXPORT_SYMBOL_GPL(hid_item_sdata);
 
 /*
  * Process a global item.
@@ -391,29 +393,29 @@ static int hid_parser_global(struct hid_parser *parser, 
struct hid_item *item)
return 0;
 
case HID_GLOBAL_ITEM_TAG_USAGE_PAGE:
-   parser->global.usage_page = item_udata(item);
+   parser->global.usage_page = hid_item_udata(item);
return 0;
 
case HID_GLOBAL_ITEM_TAG_LOGICAL_MINIMUM:
-   parser->global.logical_minimum = item_sdata(item);
+   parser->global.logical_minimum = hid_item_sdata(item);
return 0;
 
case HID_GLOBAL_ITEM_TAG_LOGICAL_MAXIMUM:
if (parser->global.logical_minimum < 0)
-   parser->global.logical_maximum = item_sdata(item);
+   parser->global.logical_maximum = hid_item_sdata(item);
else
-   parser->global.logical_maximum = item_udata(item);
+   parser->global.logical_maximum = hid_item_udata(item);
return 0;
 
case HID_GLOBAL_ITEM_TAG_PHYSICAL_MINIMUM:
-   parser->global.physical_minimum = item_sdata(item);
+   parser->global.physical_minimum = hid_item_sdata(item);
return 0;
 
case HID_GLOBAL_ITEM_TAG_PHYSICAL_MAXIMUM:
if (parser->global.physical_minimum < 0)
-   parser->global.physical_maximum = item_sdata(item);
+   parser->global.physical_maximum = hid_item_sdata(item);
else
-   parser->global.physical_maximum = item_udata(item);
+   parser->global.physical_maximum = hid_item_udata(item);
return 0;
 
case HID_GLOBAL_ITEM_TAG_UNIT_EXPONENT:
@@ -421,7 +423,7 @@ static int hid_parser_global(struct hid_parser *parser, 
struct hid_item *item)
 * nibble due to the common misunderstanding of HID
 * specification 1.11, 6.2.2.7 Global Items. Attempt to handle
 * both this and the standard encoding. */
-   raw_value = item_sdata(item);
+   raw_value = hid_item_sdata(item);
if (!(raw_value & 0xfff0))
parser->global.unit_exponent = hid_snto32(raw_value, 4);
else
@@ -429,11 +431,11 @@ static int hid_parser_global(struct hid_parser *parser, 
struct hid_item *item)
return 0;
 
case HID_GLOBAL_ITEM_TAG_UNIT:
-   parser->global.unit = item_udata(item);
+   parser->global.unit = hid_item_udata(item);
return 0;
 
case HID_GLOBAL_ITEM_TAG_REPORT_SIZE:
-   parser->global.report_size = item_udata(item);
+   parser->global.report_size = hid_item_udata(item);
if (parser->global.report_size > 256) {
hid_err(parser->device, "invalid report_size %d\n",
parser->global.report_size);
@@ -442,7 +444,7 @@ static int hid_parser_global(struct hid_parser *parser, 
struct hid_item *item)
return 0;
 
case HID_GLOBAL_ITEM_TAG_REPORT_COUNT:
-   parser->global.report_count = item_udata(item);
+   parser->global.report_count = hid_item_udata(item);
if (parser->global.report_count > HID_MAX_USAGES) {
hid_err(parser->device, "invalid report_count %d\n",
parser->global.report_count);
@@ -451,7 +453,7 @@ static int hid_parser_global(struct hid_parser *parser, 
struct hid_item *item)
return 0;
 
case HID_GLOBAL_ITEM_TAG_REPORT_ID:
-   parser->global.report_id = item_udata(item);
+   parser->global.report_id = hid_item_udata(item);