1. Patchew
  2. linux
  3. [PATCH v3 0/4] fbdev: Make CONFIG_FB_DEVICE optional for drivers
  4. View patch

[PATCH v3 4/4] fbdev: sh_mobile_lcdc: Make FB_DEVICE dependency optional

Chintan Patel posted 4 patches 1 month, 1 week ago
There is a newer version of this series
[PATCH v3 4/4] fbdev: sh_mobile_lcdc: Make FB_DEVICE dependency optional
Posted by Chintan Patel 1 month, 1 week ago 
The sh_mobile_lcdc driver exposes overlay configuration via sysfs, but the
core driver does not require CONFIG_FB_DEVICE.

Make sysfs support optional by defining overlay_sysfs_groups conditionally
using PTR_IF(). The driver always sets .dev_groups, and the kernel
naturally skips NULL attribute groups while the code remains buildable
and type-checked.

Suggested-by: Helge Deller <deller@gmx.de>
Signed-off-by: Chintan Patel <chintanlike@gmail.com>
---
 drivers/video/fbdev/sh_mobile_lcdcfb.c | 12 +++++++++++-
 1 file changed, 11 insertions(+), 1 deletion(-)

diff --git a/drivers/video/fbdev/sh_mobile_lcdcfb.c b/drivers/video/fbdev/sh_mobile_lcdcfb.c
index dd950e4ab5ce..cb7ed1ff9165 100644
--- a/drivers/video/fbdev/sh_mobile_lcdcfb.c
+++ b/drivers/video/fbdev/sh_mobile_lcdcfb.c
@@ -1350,7 +1350,17 @@ static struct attribute *overlay_sysfs_attrs[] = {
 	&dev_attr_overlay_rop3.attr,
 	NULL,
 };
-ATTRIBUTE_GROUPS(overlay_sysfs);
+
+#ifdef CONFIG_FB_DEVICE
+static const struct attribute_group overlay_sysfs_group = {
+	.attrs = overlay_sysfs_attrs,
+};
+#endif
+
+static const struct attribute_group *overlay_sysfs_groups[] = {
+	PTR_IF(IS_ENABLED(CONFIG_FB_DEVICE), &overlay_sysfs_group),
+	NULL,
+};
 
 static const struct fb_fix_screeninfo sh_mobile_lcdc_overlay_fix  = {
 	.id =		"SH Mobile LCDC",
-- 
2.43.0
Re: [PATCH v3 4/4] fbdev: sh_mobile_lcdc: Make FB_DEVICE dependency optional
Posted by Andy Shevchenko 1 month, 1 week ago 
On Mon, Dec 29, 2025 at 09:28:22PM -0800, Chintan Patel wrote:
> The sh_mobile_lcdc driver exposes overlay configuration via sysfs, but the
> core driver does not require CONFIG_FB_DEVICE.
> 
> Make sysfs support optional by defining overlay_sysfs_groups conditionally
> using PTR_IF(). The driver always sets .dev_groups, and the kernel
> naturally skips NULL attribute groups while the code remains buildable
> and type-checked.

...

> +static const struct attribute_group *overlay_sysfs_groups[] = {
> +	PTR_IF(IS_ENABLED(CONFIG_FB_DEVICE), &overlay_sysfs_group),
> +	NULL,

Please, drop comma at the end of the terminator entry.

> +};

-- 
With Best Regards,
Andy Shevchenko
Re: [PATCH v3 4/4] fbdev: sh_mobile_lcdc: Make FB_DEVICE dependency optional
Posted by Helge Deller 1 month, 1 week ago 
* Chintan Patel <chintanlike@gmail.com>:
> The sh_mobile_lcdc driver exposes overlay configuration via sysfs, but the
> core driver does not require CONFIG_FB_DEVICE.
> 
> Make sysfs support optional by defining overlay_sysfs_groups conditionally
> using PTR_IF(). The driver always sets .dev_groups, and the kernel
> naturally skips NULL attribute groups while the code remains buildable
> and type-checked.
> 
> Suggested-by: Helge Deller <deller@gmx.de>
> Signed-off-by: Chintan Patel <chintanlike@gmail.com>
> ---
>  drivers/video/fbdev/sh_mobile_lcdcfb.c | 12 +++++++++++-
>  1 file changed, 11 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/video/fbdev/sh_mobile_lcdcfb.c b/drivers/video/fbdev/sh_mobile_lcdcfb.c
> index dd950e4ab5ce..cb7ed1ff9165 100644
> --- a/drivers/video/fbdev/sh_mobile_lcdcfb.c
> +++ b/drivers/video/fbdev/sh_mobile_lcdcfb.c
> @@ -1350,7 +1350,17 @@ static struct attribute *overlay_sysfs_attrs[] = {
>  	&dev_attr_overlay_rop3.attr,
>  	NULL,
>  };
> -ATTRIBUTE_GROUPS(overlay_sysfs);

Instead of replacing the ^ ATTRIBUTE_GROUPS() by the code below,
isn't it possible to just mark the overlay_sysfs_attrs[] array
_maybe_unused, and just do:
+ #ifdef CONFIG_FB_DEVICE
+ ATTRIBUTE_GROUPS(overlay_sysfs);
+ #endif

?

Helge


> +
> +#ifdef CONFIG_FB_DEVICE
> +static const struct attribute_group overlay_sysfs_group = {
> +	.attrs = overlay_sysfs_attrs,
> +};
> +#endif
> +
> +static const struct attribute_group *overlay_sysfs_groups[] = {
> +	PTR_IF(IS_ENABLED(CONFIG_FB_DEVICE), &overlay_sysfs_group),
> +	NULL,
> +};
>  
>  static const struct fb_fix_screeninfo sh_mobile_lcdc_overlay_fix  = {
>  	.id =		"SH Mobile LCDC",
> -- 
> 2.43.0
> 
>
Re: [PATCH v3 4/4] fbdev: sh_mobile_lcdc: Make FB_DEVICE dependency optional
Posted by Chintan Patel 1 month, 1 week ago 

On 12/30/25 00:13, Helge Deller wrote:
> * Chintan Patel <chintanlike@gmail.com>:
>> The sh_mobile_lcdc driver exposes overlay configuration via sysfs, but the
>> core driver does not require CONFIG_FB_DEVICE.
>>
>> Make sysfs support optional by defining overlay_sysfs_groups conditionally
>> using PTR_IF(). The driver always sets .dev_groups, and the kernel
>> naturally skips NULL attribute groups while the code remains buildable
>> and type-checked.
>>
>> Suggested-by: Helge Deller <deller@gmx.de>
>> Signed-off-by: Chintan Patel <chintanlike@gmail.com>
>> ---
>>   drivers/video/fbdev/sh_mobile_lcdcfb.c | 12 +++++++++++-
>>   1 file changed, 11 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/video/fbdev/sh_mobile_lcdcfb.c b/drivers/video/fbdev/sh_mobile_lcdcfb.c
>> index dd950e4ab5ce..cb7ed1ff9165 100644
>> --- a/drivers/video/fbdev/sh_mobile_lcdcfb.c
>> +++ b/drivers/video/fbdev/sh_mobile_lcdcfb.c
>> @@ -1350,7 +1350,17 @@ static struct attribute *overlay_sysfs_attrs[] = {
>>   	&dev_attr_overlay_rop3.attr,
>>   	NULL,
>>   };
>> -ATTRIBUTE_GROUPS(overlay_sysfs);
> 
> Instead of replacing the ^ ATTRIBUTE_GROUPS() by the code below,
> isn't it possible to just mark the overlay_sysfs_attrs[] array
> _maybe_unused, and just do:
> + #ifdef CONFIG_FB_DEVICE
> + ATTRIBUTE_GROUPS(overlay_sysfs);
> + #endif
> 
> ?
Hi Helge,

Yes, the __maybe_unused + #ifdef ATTRIBUTE_GROUPS() approach would work.

I went with the PTR_IF(IS_ENABLED()) pattern because Andy suggested 
using PTR_IF() to conditionally include overlay_sysfs_group in 
overlay_sysfs_groups, and to keep .dev_groups always populated while 
letting the device core skip NULL groups. This avoids conditional wiring 
via #ifdef and keeps the code type-checked without CONFIG_FB_DEVICE.
If you still prefer the simpler #ifdef ATTRIBUTE_GROUPS() approach for 
this driver, I can switch to that, but I wanted to follow Andy’s 
guidance here.


Thanks!
Re: [PATCH v3 4/4] fbdev: sh_mobile_lcdc: Make FB_DEVICE dependency optional
Posted by Helge Deller 1 month ago 
On 12/30/25 19:25, Chintan Patel wrote:
> 
> 
> On 12/30/25 00:13, Helge Deller wrote:
>> * Chintan Patel <chintanlike@gmail.com>:
>>> The sh_mobile_lcdc driver exposes overlay configuration via sysfs, but the
>>> core driver does not require CONFIG_FB_DEVICE.
>>>
>>> Make sysfs support optional by defining overlay_sysfs_groups conditionally
>>> using PTR_IF(). The driver always sets .dev_groups, and the kernel
>>> naturally skips NULL attribute groups while the code remains buildable
>>> and type-checked.
>>>
>>> Suggested-by: Helge Deller <deller@gmx.de>
>>> Signed-off-by: Chintan Patel <chintanlike@gmail.com>
>>> ---
>>>   drivers/video/fbdev/sh_mobile_lcdcfb.c | 12 +++++++++++-
>>>   1 file changed, 11 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/drivers/video/fbdev/sh_mobile_lcdcfb.c b/drivers/video/fbdev/sh_mobile_lcdcfb.c
>>> index dd950e4ab5ce..cb7ed1ff9165 100644
>>> --- a/drivers/video/fbdev/sh_mobile_lcdcfb.c
>>> +++ b/drivers/video/fbdev/sh_mobile_lcdcfb.c
>>> @@ -1350,7 +1350,17 @@ static struct attribute *overlay_sysfs_attrs[] = {
>>>       &dev_attr_overlay_rop3.attr,
>>>       NULL,
>>>   };
>>> -ATTRIBUTE_GROUPS(overlay_sysfs);
>>
>> Instead of replacing the ^ ATTRIBUTE_GROUPS() by the code below,
>> isn't it possible to just mark the overlay_sysfs_attrs[] array
>> _maybe_unused, and just do:
>> + #ifdef CONFIG_FB_DEVICE
>> + ATTRIBUTE_GROUPS(overlay_sysfs);
>> + #endif
>>
>> ?
> Hi Helge,
> 
> Yes, the __maybe_unused + #ifdef ATTRIBUTE_GROUPS() approach would work.
> 
> I went with the PTR_IF(IS_ENABLED()) pattern because Andy suggested
> using PTR_IF() to conditionally include overlay_sysfs_group in
> overlay_sysfs_groups, and to keep .dev_groups always populated while
> letting the device core skip NULL groups. This avoids conditional
> wiring via #ifdef and keeps the code type-checked without
> CONFIG_FB_DEVICE.
> If you still prefer the simpler #ifdef ATTRIBUTE_GROUPS() approach
> for this driver, I can switch to that, but I wanted to follow Andy’s
> guidance here.

I assume Andy will agree to my suggested approach, as it's cleaner
and avoids code bloat/duplication. Maybe you send out a v4 with my
suggested approach, then it's easier to judge... ?

Helge
Re: [PATCH v3 4/4] fbdev: sh_mobile_lcdc: Make FB_DEVICE dependency optional
Posted by Andy Shevchenko 1 month ago 
On Sat, Jan 03, 2026 at 10:59:44AM +0100, Helge Deller wrote:
> On 12/30/25 19:25, Chintan Patel wrote:
> > On 12/30/25 00:13, Helge Deller wrote:

...

> > > > -ATTRIBUTE_GROUPS(overlay_sysfs);
> > > 
> > > Instead of replacing the ^ ATTRIBUTE_GROUPS() by the code below,
> > > isn't it possible to just mark the overlay_sysfs_attrs[] array
> > > _maybe_unused, and just do:
> > > + #ifdef CONFIG_FB_DEVICE
> > > + ATTRIBUTE_GROUPS(overlay_sysfs);
> > > + #endif
> > > 
> > > ?
> > 
> > Yes, the __maybe_unused + #ifdef ATTRIBUTE_GROUPS() approach would work.
> > 
> > I went with the PTR_IF(IS_ENABLED()) pattern because Andy suggested
> > using PTR_IF() to conditionally include overlay_sysfs_group in
> > overlay_sysfs_groups, and to keep .dev_groups always populated while
> > letting the device core skip NULL groups. This avoids conditional
> > wiring via #ifdef and keeps the code type-checked without
> > CONFIG_FB_DEVICE.
> > If you still prefer the simpler #ifdef ATTRIBUTE_GROUPS() approach
> > for this driver, I can switch to that, but I wanted to follow Andy’s
> > guidance here.
> 
> I assume Andy will agree to my suggested approach, as it's cleaner
> and avoids code bloat/duplication. Maybe you send out a v4 with my
> suggested approach, then it's easier to judge... ?

I'm also fine with original code. But a suggested approach would work as well
(at least like it sounds from the above description). Ideally would be nice to
get rid of ifdeffery completely (that's why we have PTR_IF() for), although
it might be not so readable. TL;DR: the most readable solution is the winner.

-- 
With Best Regards,
Andy Shevchenko
Re: [PATCH v3 4/4] fbdev: sh_mobile_lcdc: Make FB_DEVICE dependency optional
Posted by Chintan Patel 1 month ago 

On 1/3/26 05:15, Andy Shevchenko wrote:
> On Sat, Jan 03, 2026 at 10:59:44AM +0100, Helge Deller wrote:
>> On 12/30/25 19:25, Chintan Patel wrote:
>>> On 12/30/25 00:13, Helge Deller wrote:
> 
> ...
> 
>>>>> -ATTRIBUTE_GROUPS(overlay_sysfs);
>>>>
>>>> Instead of replacing the ^ ATTRIBUTE_GROUPS() by the code below,
>>>> isn't it possible to just mark the overlay_sysfs_attrs[] array
>>>> _maybe_unused, and just do:
>>>> + #ifdef CONFIG_FB_DEVICE
>>>> + ATTRIBUTE_GROUPS(overlay_sysfs);
>>>> + #endif
>>>>
>>>> ?
>>>
>>> Yes, the __maybe_unused + #ifdef ATTRIBUTE_GROUPS() approach would work.
>>>
>>> I went with the PTR_IF(IS_ENABLED()) pattern because Andy suggested
>>> using PTR_IF() to conditionally include overlay_sysfs_group in
>>> overlay_sysfs_groups, and to keep .dev_groups always populated while
>>> letting the device core skip NULL groups. This avoids conditional
>>> wiring via #ifdef and keeps the code type-checked without
>>> CONFIG_FB_DEVICE.
>>> If you still prefer the simpler #ifdef ATTRIBUTE_GROUPS() approach
>>> for this driver, I can switch to that, but I wanted to follow Andy’s
>>> guidance here.
>>
>> I assume Andy will agree to my suggested approach, as it's cleaner
>> and avoids code bloat/duplication. Maybe you send out a v4 with my
>> suggested approach, then it's easier to judge... ?
> 
> I'm also fine with original code. But a suggested approach would work as well
> (at least like it sounds from the above description). Ideally would be nice to
> get rid of ifdeffery completely (that's why we have PTR_IF() for), although
> it might be not so readable. TL;DR: the most readable solution is the winner.
> 
Thank you both! I will send v4 with Helge's suggestion and take it from 
there.

Patchew 2.1-devel-b67e4c2

© 2016 - 2026 Red Hat, Inc.