Re: [PATCH 1/3] HID: apple-ibridge: Add Apple iBridge HID driver for T1 chip.

[Thomas Weißschuh]

Responses inline

On Fri, Feb 10, 2023 at 12:05:13PM +0000, Aditya Garg wrote:
> > On 10-Feb-2023, at 10:26 AM, Thomas Weißschuh <thomas@xxxxxxxx> wrote:
> > 
> > Hi,
> > 
> > some comments inline.
> > 
> > On Fri, Feb 10, 2023 at 03:43:24AM +0000, Aditya Garg wrote:
> > 
> >> +
> >> +static struct {
> >> + unsigned int usage;
> >> + struct hid_device_id *dev_id;
> >> +} appleib_usage_map[] = {
> >> + /* Default iBridge configuration, key inputs and mode settings */
> >> + { 0x00010006, &appleib_sub_hid_ids[0] },
> >> + /* OS X iBridge configuration, digitizer inputs */
> >> + { 0x000D0005, &appleib_sub_hid_ids[0] },
> >> + /* All iBridge configurations, display/DFR settings */
> >> + { 0xFF120001, &appleib_sub_hid_ids[0] },
> >> + /* All iBridge configurations, ALS */
> >> + { 0x00200041, &appleib_sub_hid_ids[1] },
> >> +};
> > 
> > const
> > 
> 
> Constantifying this results in compiler giving warnings
> 
> drivers/hid/apple-ibridge.c:78:23: warning: initialization discards 'const' qualifier from pointer target type [-Wdiscarded-qualifiers]
>    78 |         { 0x00200041, &appleib_sub_hid_ids[1] },

For this you also have to constify the hid_device_id *dev_id in
appleib_usage_map. And then propagate this change to some functions and
variables.

>       |                       ^
> drivers/hid/apple-ibridge.c: In function 'appleib_add_sub_dev':
> drivers/hid/apple-ibridge.c:363:29: warning: assignment discards 'const' qualifier from pointer target type [-Wdiscarded-qualifiers]
>   363 |         sub_hdev->ll_driver = &appleib_ll_driver;

As Benjamin said this is because your changes are based on Linus' tree
but they will break as soon as they will be merged into the HID tree.
You should base your changes off of the HID tree:
https://git.kernel.org/pub/scm/linux/kernel/git/hid/hid.git/log/?h=for-6.3/hid-core

This issue is essentially unlucky timing.

>       |                             ^
> drivers/hid/apple-ibridge.c: In function 'appleib_hid_probe':
> drivers/hid/apple-ibridge.c:436:12: error: expected '(' before 'hid_is_usb'
>   436 |         if hid_is_usb(hdev)
>       |            ^~~~~~~~~~
>       |            (

As the error message indicates, this is invalid syntax and missing a
'('.
What you want to do is to check for 

  if (!hid_is_usb(hdev))
    return -ENODEV;

*before* calling hid_to_usb_dev(hdev);

> In file included from drivers/hid/apple-ibridge.c:48:
> drivers/hid/apple-ibridge.c: In function 'appleib_probe':
> drivers/hid/apple-ibridge.c:544:35: warning: passing argument 1 of '__hid_register_driver' discards 'const' qualifier from pointer target type [-Wdiscarded-qualifiers]
>   544 |         ret = hid_register_driver(&appleib_hid_driver);
>       |                                   ^~~~~~~~~~~~~~~~~~~
> ./include/linux/hid.h:898:31: note: in definition of macro 'hid_register_driver'
>   898 |         __hid_register_driver(driver, THIS_MODULE, KBUILD_MODNAME)
>       |                               ^~~~~~
> ./include/linux/hid.h:893:47: note: expected 'struct hid_driver *' but argument is of type 'const struct hid_driver *'
>   893 | extern int __must_check __hid_register_driver(struct hid_driver *,
>       |                                               ^~~~~~~~~~~~~~~~~~~
> drivers/hid/apple-ibridge.c: In function 'appleib_remove':
> drivers/hid/apple-ibridge.c:558:31: warning: passing argument 1 of 'hid_unregister_driver' discards 'const' qualifier from pointer target type [-Wdiscarded-qualifiers]
>   558 |         hid_unregister_driver(&appleib_hid_driver);
>       |                               ^~~~~~~~~~~~~~~~~~~
> ./include/linux/hid.h:900:35: note: expected 'struct hid_driver *' but argument is of type 'const struct hid_driver *'
>   900 | extern void hid_unregister_driver(struct hid_driver *);
>       |                                   ^~~~~~~~~~~~~~~~~~~

These are all because applib_hid_driver can not be const.
Sorry for the wrong advice.

Benjamin:
HID drivers can not be const because they embed a 'struct driver' that
is needed by the driver core to be mutable.
Fixing this is probably a larger enterprise.

> make[6]: *** [scripts/Makefile.build:250: drivers/hid/apple-ibridge.o] Error 1
> make[5]: *** [scripts/Makefile.build:500: drivers/hid] Error 2
> make[5]: *** Waiting for unfinished jobs….
> 
> Some warnings are also due to a typo in if and constantifying `static struct hid_driver`, although they probably can
> be fixed.
> 
> In short, Thomas, do you really want me to constantify the structure I
> am talking about in this email, as well `static struct hid_driver`?

struct hid_driver: Don't constify
all others: Do constify

Thomas