Re: [PATCH 1/6] HID: intel-ish-hid: add support for MODULE_DEVICE_TABLE()

From: Hans de Goede <hdegoede@redhat.com>
To: "Thomas Weißschuh" <linux@weissschuh.net>
Cc: linux-input@vger.kernel.org, linux-kernel@vger.kernel.org,
	linux-kbuild@vger.kernel.org,
	Srinivas Pandruvada <srinivas.pandruvada@linux.intel.com>,
	Jiri Kosina <jkosina@suse.cz>,
	Benjamin Tissoires <benjamin.tissoires@redhat.com>,
	Masahiro Yamada <masahiroy@kernel.org>,
	Michal Marek <michal.lkml@markovi.net>,
	Nick Desaulniers <ndesaulniers@google.com>
Subject: Re: [PATCH 1/6] HID: intel-ish-hid: add support for MODULE_DEVICE_TABLE()
Date: Mon, 1 Nov 2021 11:15:57 +0100	[thread overview]
Message-ID: <aaca72e9-2c69-c2c2-55cb-8c7790178b53@redhat.com> (raw)
In-Reply-To: <091a4bb3-0734-4b58-9417-7ef18fcc385b@t-8ch.de>

Hi,

On 11/1/21 11:09, Thomas Weißschuh wrote:
> On 2021-11-01 10:58+0100, Hans de Goede wrote:
>> On 10/29/21 17:28, Thomas Weißschuh wrote:
>>> This allows to selectively autoload drivers for ISH devices.
>>> Currently all ISH drivers are loaded for all systems having any ISH
>>> device.
>>>
>>> Signed-off-by: Thomas Weißschuh <linux@weissschuh.net>
>>>
>>> ---
>>>
>>> Cc: linux-kbuild@vger.kernel.org
>>> Cc: linux-input@vger.kernel.org
>>> Cc: Srinivas Pandruvada <srinivas.pandruvada@linux.intel.com>
>>> Cc: Jiri Kosina <jkosina@suse.cz>
>>> Cc: Benjamin Tissoires <benjamin.tissoires@redhat.com>
>>> Cc: Hans de Goede <hdegoede@redhat.com>
>>> Cc: Masahiro Yamada <masahiroy@kernel.org>
>>> Cc: Michal Marek <michal.lkml@markovi.net>
>>> Cc: Nick Desaulniers <ndesaulniers@google.com>
>>> ---
>>>  include/linux/mod_devicetable.h   | 13 +++++++++++++
>>>  scripts/mod/devicetable-offsets.c |  3 +++
>>>  scripts/mod/file2alias.c          | 24 ++++++++++++++++++++++++
>>>  3 files changed, 40 insertions(+)
>>>
>>> diff --git a/include/linux/mod_devicetable.h b/include/linux/mod_devicetable.h
>>> index ae2e75d15b21..befbf53c4b7c 100644
>>> --- a/include/linux/mod_devicetable.h
>>> +++ b/include/linux/mod_devicetable.h
>>> @@ -895,4 +895,17 @@ struct dfl_device_id {
>>>  	kernel_ulong_t driver_data;
>>>  };
>>>  
>>> +/* ISHTP (Integrated Sensor Hub Transport Protocol) */
>>> +
>>> +#define ISHTP_MODULE_PREFIX	"ishtp:"
>>> +
>>> +/**
>>> + * struct ishtp_device_id - ISHTP device identifier
>>> + * @guid_string: 36 char string of the form fa50ff2b-f2e8-45de-83fa-65417f2f49ba
>>> + * @context: pointer to driver specific data
>>> + */
>>> +struct ishtp_device_id {
>>> +	guid_t guid;
>>
>> The kdoc comment documents a context pointer, but this is missing from the
>> actual struct. Having some sort of driver_data (1) field here would be good IMHO.
> 
> Fine for me.
> 
> I left it out because nothing would be using it at the moment and
> it would have been easy to add when needed.

IMHO having a device_id without a context/driver_data field would be
weird and is likely asking for needless churn in the future, but see
below.

> Do you want me to send a v2 for that or would you add it when merging?
> (Or remove the spurious comment)

As I indicated in my reply to the cover-letter, I believe this series
should be merged through the HID tree, so this is up to the HID maintainers
to decide.

Regards,

Hans

p.s.

Thank you for doing this series I did not realize that the eclite driver
would end up being loaded on all systems where the ISH is used, thank you
for fixing this.