> On 10-Feb-2023, at 9:04 PM, Thomas Weißschuh <thomas@xxxxxxxx> wrote:
>
> 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;
It was a typo on my part
+ /* check and set usb config first */
+ if (hid_is_usb(hdev))
+ udev = hid_to_usb_dev(hdev);
+ else
+ return -EINVAL;
This is what I have in my patch set now.
If there is something wrong with this, then do tell me
Thanks
>
> *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
