Re: [PATCH] Staging: fbtft: Fix style problem in header

[Javier Martinez Canillas <javierm@redhat.com>]

On 4/20/22 16:36, Uwe Kleine-König wrote:
> Hello Ian,
> 
> On Wed, Apr 20, 2022 at 09:57:27AM -0400, Ian Cowan wrote:
>> On Wed, Apr 20, 2022 at 08:47:11AM +0200, Uwe Kleine-König wrote:
>>> On Tue, Apr 19, 2022 at 03:21:28PM -0400, Ian Cowan wrote:
>>>> Removed an unnecessary semicolon at the end of a macro call
>>>>
>>>> Signed-off-by: Ian Cowan <ian@linux.cowan.aero>
>>>> ---
>>>>  drivers/staging/fbtft/fbtft.h | 2 +-
>>>>  1 file changed, 1 insertion(+), 1 deletion(-)
>>>>
>>>> diff --git a/drivers/staging/fbtft/fbtft.h b/drivers/staging/fbtft/fbtft.h
>>>> index 2c2b5f1c1df3..aa66760e1a9c 100644
>>>> --- a/drivers/staging/fbtft/fbtft.h
>>>> +++ b/drivers/staging/fbtft/fbtft.h
>>>> @@ -277,7 +277,7 @@ static const struct of_device_id dt_ids[] = {					\
>>>>  	{ .compatible = _compatible },						\
>>>>  	{},									\
>>>>  };										\
>>>> -MODULE_DEVICE_TABLE(of, dt_ids);
>>>> +MODULE_DEVICE_TABLE(of, dt_ids)
>>>
>>> In fact the ; after MODULE_DEVICE_TABLE is necessary. There is only a
>>> single instance in the kernel without a semicolon[1]. That's in
>>> drivers/pci/controller/pcie-microchip-host.c and this only works because
>>> this driver cannot be compiled as a module and so MODULE_DEVICE_TABLE
>>> evaluates to nothing. Will send a patch for that one.
>

Indeed. I was curious about this so I went to look at the driver code.

For this particular driver it may be not necessary, but that's just due
how these fbtft drivers define their MODULE_DEVICE_TABLE(), using a lot
of macro layers.

As an example, drivers/staging/fbtft/fb_agm1264k-fl.c does the following:

FBTFT_REGISTER_DRIVER(DRVNAME, "displaytronic,fb_agm1264k-fl", &display);

which is defined as:

#define FBTFT_REGISTER_DRIVER(_name, _compatible, _display)                \
...									   \
FBTFT_DT_TABLE(_compatible)						   \
...

which in turn is defined as:

#define FBTFT_DT_TABLE(_compatible)						\
static const struct of_device_id dt_ids[] = {					\
	{ .compatible = _compatible },						\
	{},									\
};										\
MODULE_DEVICE_TABLE(of, dt_ids);

so it seems that it builds, just because the semicolon for the expression
is the one that's after the FBTFT_REGISTER_DRIVER(); in the driver.

> FTR: Patch was sent: https://lore.kernel.org/linux-pci/20220420065832.14173-1-u.kleine-koenig@pengutronix.de
> 
>> When I built this, it appeared to succeed. I used the command "make
>> M=/drivers/staging/fbtft modules". Is this incorrect? For reference this
>> is my first patch so it's highly likely I did this incorrectly.
>

You are just changing a header file though, did you also enable one of the
fbtft drivers as a module to see if those build? But as said, by looking at
the code it seems that should build correctly.

I agree with Uwe though that is less confusing to have a semicolon after
the MODULE_DEVICE_TABLE(), but I'm not the driver maintainer to decide.
 
> I don't know for sure, but I'd have said that the M= stuff is for
> out-of-tree modules only.
>

It does work, I use M= to build drivers in mainline that are configured
to build as a module all the time.

-- 
Best regards,

Javier Martinez Canillas
Linux Engineering
Red Hat