Re: [PATCH v3 1/3] module: add SCMI device table alias support

[Hans de Goede]

Hi Uwe,

On 24-Aug-26 14:22, Uwe Kleine-König wrote:
> Hello,
> 
> I just replied to v2 and only then found out there is a v3 already.
> There is something wrong with my mail filtering, sorry.

Not sure if it is your filtering, v2 is somewhat old
and I only just revived this series.

I'll add you to the Cc list for v4.

> I'm forwarding my review comments to here (the part that still applies).
> 
> On Thu, Aug 20, 2026 at 09:48:48PM +0200, Hans de Goede wrote:
>> diff --git a/include/linux/device-id/scmi.h b/include/linux/device-id/scmi.h
>> new file mode 100644
>> index 000000000000..4150481524d4
>> --- /dev/null
>> +++ b/include/linux/device-id/scmi.h
>> @@ -0,0 +1,19 @@
>> +/* SPDX-License-Identifier: GPL-2.0-only */
>> +#ifndef LINUX_DEVICE_ID_SCMI_H
>> +#define LINUX_DEVICE_ID_SCMI_H
>> +
>> +#ifdef __KERNEL__
>> +#include <linux/types.h>
>> +typedef unsigned long kernel_ulong_t;
>> +#endif
>> +
>> +#define SCMI_NAME_SIZE		32
>> +#define SCMI_MODULE_PREFIX	"scmi:"
>> +
>> +struct scmi_device_id {
>> +	__u8 protocol_id;
>> +	char name[SCMI_NAME_SIZE];
>> +	kernel_ulong_t driver_data;
> 
> Please use
> 
> 	union {
> 		kernel_ulong_t driver_data;
> 		const void *driver_data_ptr;
> 	};
> 
> here instead of the plain driver_data member (or don't add driver_data
> at all if not needed). This yields better type-safety, see e.g.
> https://lore.kernel.org/all/cover.1780048925.git.u.kleine-koenig@xxxxxxxxxxxx
> for an extended explanation.

Ack, I'll check if driver_data is used at all.

> 
>> +};
>> +
>> +#endif /* ifndef LINUX_DEVICE_ID_SCMI_H */
>> diff --git a/include/linux/mod_devicetable.h b/include/linux/mod_devicetable.h
>> index a397213bedac..d241421c36aa 100644
>> --- a/include/linux/mod_devicetable.h
>> +++ b/include/linux/mod_devicetable.h
>> @@ -49,6 +49,7 @@
>>  #include "device-id/pnp.h"
>>  #include "device-id/rio.h"
>>  #include "device-id/rpmsg.h"
>> +#include "device-id/scmi.h"
> 
> Please don't add new includes to mod_devicetable.h. That header is ugly
> and each include makes it worse. Just add <linux/device-id/scmi.h> to
> the modpost source.

Hmm, I did that at first, but that is quite ugly also because
file2alias.c, does:

#include "../../include/linux/mod_devicetable.h"

So I ended up needing to change that to:

#include "../../include/linux/device-id/scmi.h"
#include "../../include/linux/mod_devicetable.h"

my thinking was that the purpose would be to move other
consumers of mod_devicetable.h to directly include
linux/device-id/*.h"

And keep mod_devicetable.h as a file which includes all
of them for use in devicetable-offsets.c + file2alias.c

But I can see how actually getting there will be a long time
in the making and in the mean time this will cause longer
compile times.

So if you're saying the somewhat ugly thing of adding
linux/device-id/scmi.h directly to devicetable-offsets.c +
file2alias.c is the right thing to do, then I'm happy to
do that for v4.

If I'm reading your comment correctly, that is what you
want me to do, right ?

Regards,

Hans