Message-ID: <04df1bb8-8409-4ece-a21c-4c71592eb852@infradead.org>
Date: Fri, 10 Oct 2025 15:51:57 -0700
From: Randy Dunlap <rdunlap@...radead.org>
To: Jonathan Denose <jdenose@...gle.com>
Cc: Thorsten Leemhuis <linux@...mhuis.info>, Jiri Kosina <jikos@...nel.org>,
 Benjamin Tissoires <bentiss@...nel.org>,
 Dmitry Torokhov <dmitry.torokhov@...il.com>, Jonathan Corbet
 <corbet@....net>, Henrik Rydberg <rydberg@...math.org>,
 linux-input@...r.kernel.org, linux-kernel@...r.kernel.org,
 linux-doc@...r.kernel.org, Angela Czubak <aczubak@...gle.com>,
 Sean O'Brien <seobrien@...gle.com>, Lucas GISSOT <lucas.gissot.pro@...il.com>
Subject: Re: [PATCH v3 04/11] HID: haptic: introduce hid_haptic_device



On 10/10/25 1:30 PM, Jonathan Denose wrote:
> Hi all,
> 
> Thanks for looking into this.
> 
> On Fri, Oct 10, 2025 at 1:55 PM Randy Dunlap <rdunlap@...radead.org> wrote:
>>
>> Hi,
>>
>> I think I found it... see below.
>>
>>
>> On 10/9/25 11:48 PM, Thorsten Leemhuis wrote:
>>> [Top-Posting for easier consumption]
>>>
>>> Mainly writing this mail to bring Lucas GISSOT in here, who reported the
>>> same error yesterday here:
>>> https://lore.kernel.org/all/aOgvA8Jiofcnk2xb@ARSENIURE.localdomain/
>>>
>>> Lucas there suggested:
>>> """but it seems to me that the #if IS_ENABLED(CONFIG_HID_HAPTIC) in
>>> hid-haptic.h should be replaced by IS_BUILTIN(CONFIG_HID_HAPTIC) and
>>> Kconfig updated."""
>>>
>>> And Randy: Thx for the closer investigation! It explains some of the
>>> "that feels odd, am I holding this wrong" feeling I had when reporting this.
>>>
>>> Ciao, Thorsten
>>>
>>> On 10/10/25 06:50, Randy Dunlap wrote:
>>>> On 10/9/25 7:43 AM, Thorsten Leemhuis wrote:
>>>>> On 8/19/25 01:08, Jonathan Denose wrote:
>>>>>> From: Angela Czubak <aczubak@...gle.com>
>>>>>>
>>>>>> Define a new structure that contains simple haptic device configuration
>>>>>> as well as current state.
>>>>>> Add functions that recognize auto trigger and manual trigger reports
>>>>>> as well as save their addresses.Hi,
>>>>>> Verify that the pressure unit is either grams or newtons.
>>>>>> Mark the input device as a haptic touchpad if the unit is correct and
>>>>>> the reports are found.
>>>>>>  [...]
>>>>>> +config HID_HAPTIC
>>>>>> +  tristate "Haptic touchpad support"
>>>>>> +  default n
>>>>>> +  help
>>>>>> +  Support for touchpads with force sensors and haptic actuators instead of a
>>>>>> +  traditional button.
>>>>>> +  Adds extra parsing and FF device for the hid multitouch driver.
>>>>>> +  It can be used for Elan 2703 haptic touchpad.
>>>>>> +
>>>>>> +  If unsure, say N.
>>>>>> +
>>>>>>  menu "Special HID drivers"
>>>>>
>>>>> I suspect this change is related to a build error I ran into today:
>>>>>
>>>>>   MODPOST Module.symvers
>>>>> ERROR: modpost: "hid_haptic_init" [drivers/hid/hid-multitouch.ko] undefined!
>>>>> ERROR: modpost: "hid_haptic_pressure_increase" [drivers/hid/hid-multitouch.ko] undefined!
>>>>> ERROR: modpost: "hid_haptic_check_pressure_unit" [drivers/hid/hid-multitouch.ko] undefined!
>>>>> ERROR: modpost: "hid_haptic_input_configured" [drivers/hid/hid-multitouch.ko] undefined!
>>>>> ERROR: modpost: "hid_haptic_input_mapping" [drivers/hid/hid-multitouch.ko] undefined!
>>>>> ERROR: modpost: "hid_haptic_feature_mapping" [drivers/hid/hid-multitouch.ko] undefined!
>>>>> ERROR: modpost: "hid_haptic_pressure_reset" [drivers/hid/hid-multitouch.ko] undefined!
>>>>> make[3]: *** [/home/thl/var/linux.dev/scripts/Makefile.modpost:147: Module.symvers] Error 1
>>>>>
>>>>> The config where this occurred had this:
>>>>>
>>>>> CONFIG_HID=y
>>>>> CONFIG_HID_MULTITOUCH=m
>>>>> CONFIG_HID_HAPTIC=m
>>>>>
>>>>> Changing the latter to "CONFIG_HID_HAPTIC=y" fixed the problem for me.
>>>>
>>>> Sure, but that's just covering up the problem.
>>>>> First, I get this build error:
>>>>
>>>> ERROR: modpost: missing MODULE_LICENSE() in drivers/hid/hid-haptic.o
>>>> WARNING: modpost: missing MODULE_DESCRIPTION() in drivers/hid/hid-haptic.o
>>>>
>>
>> ISTM that tristate is incompatible with this newly added Makefile
>> line:
>>
>> +hid-$(CONFIG_HID_HAPTIC)       += hid-haptic.o
>>
>> hid-* lists files that should be builtin, not loadable modules.
>> These should all be hid-y.  AFAIK, hid-m is not useful.
>> (A maintainer can correct me as needed.)

More correctly, any .o that is listed as being built as part of
hid.o should is going to be controlled by CONFIG_HID and should
not its own have a Kconfig symbol at all.

E.g. here:
 hid-y                  := hid-core.o hid-input.o hid-quirks.o
hid-core, hid-input, and hid-quirks don't have their own
Kconfig symbols.



>> So adding a MODULE_LICENSE() and MODULE_DESCRIPTION() to
>> hid-haptic.c and changing drivers/hid/Makefile to use
>> obj-$(CONFIG_HID_HAPTIC_        += hid-haptic.o
>>
>> fixes the build errors for me.
>>
>> Angela, Jonathan D., is there any reason that
>> hid-haptic needs to be builtin instead of a loadable
>> module?  It's no problem for hid-multitouch.ko to call
>> into hid-haptic.ko (both as loadable modules) as long as
>> hid-haptic.ko is loaded first.
>>
> As long as hid-multitouch.ko is able to call into hid-haptic.ko I
> don't see any issues, but is there a way to enforce that when
> CONFIG_HID_HAPTIC is enabled, hid-haptic.ko will be loaded before
> hid-multitouch.ko?

I only know of two possibilities though there may be more.

a. use request_module(true, "hid-haptic");

This would probably be used in the hid core somewhere, afterthe device matching is done.

b. use udev. When a device that needs hid-multitouch is
discovered, have udev load both hid-haptic and hid-multitouch.


I see that hid-haptic.h is written so that it has stubs for
when CONFIG_HID_HAPTIC is not enabled. Can hid-multitouch
operate (in a reduced capacity) when HID_HAPTIC is not enabled?
So that they are separate modules and hid-multitouch is not
dependent on hid-haptic?

There is probably a use case for hid-multitouch without having
hid-haptic loaded since hid-multitouch existed without having
hid-haptic around at all.

It seems that you want both of them loaded. And then Lucas
has reported a build error when HID_HAPTIC=m and
HID_MULTITOUCH=y, so either (like Lucas said) HID_HAPTIC
should be bool, not tristate; or in Kconfig,
HID_MULTITOUCH should depend on HID_HAPTIC, which would not
allow the problem config that Lucas reported.
But that forces devices that want HID_MULTITOUCH to also
have HID_HAPTIC loaded, even though they may not need it.


-- 
~Randy

Powered by blists - more mailing lists