Re: [PATCH] fbdev: sh_mobile_lcdcfb: Restore the per-overlay sysfs attributes

[Shixiong Ou <oushixiong@kylinos.cn>]

On 2026/9/11 05:15, Helge Deller wrote:
> On 9/7/26 04:18, Karl Mehltretter wrote:
>> The ovl_* files documented in
>> Documentation/ABI/testing/sysfs-devices-platform-sh_mobile_lcdc_fb no
>> longer exist.
>>
>> Commit a979182a2453 ("fbdev: lcdcfb: Register sysfs groups through 
>> driver
>> core") moved the attributes from each overlay's framebuffer device to
>> the platform driver's dev_groups and renamed them overlay_*. There is
>> now only one set of attributes for the whole LCDC. The callbacks still
>> expect dev_get_drvdata() to return a struct fb_info, but the platform
>> device holds struct sh_mobile_lcdc_priv, so they access the wrong
>> structure.
>>
>> Restore the documented names and register the group on each overlay's
>> framebuffer device with device_add_groups(). Remove it with
>> device_remove_groups() before unregistering the framebuffer. Skip
>> creation when info->dev is NULL, as the old device_create_file() 
>> calls did.
>>
>> Drop the CONFIG_FB_DEVICE guard, since the driver already depends on it.
>>
>> Fixes: a979182a2453 ("fbdev: lcdcfb: Register sysfs groups through 
>> driver core")
>> Cc: stable@vger.kernel.org
>> Assisted-by: LLM
>> Signed-off-by: Karl Mehltretter <kmehltretter@gmail.com>
>> ---
>> Found while reviewing the sysfs ABI documentation.
>>
>> Compile-tested only (allmodconfig, W=1), I have no SH-Mobile hardware.
>
>
> This all seems correct, but since it reverts a few of the last patches
> I'd prefer that some other person could also review it....
>
Looks good to me, and I agree .dev_groups is wrong here.

Shixiong.

>
>>   drivers/video/fbdev/sh_mobile_lcdcfb.c | 37 +++++++++++++++++---------
>>   1 file changed, 24 insertions(+), 13 deletions(-)
>>
>> diff --git a/drivers/video/fbdev/sh_mobile_lcdcfb.c 
>> b/drivers/video/fbdev/sh_mobile_lcdcfb.c
>> index e8324b01700f..18fc4e465283 100644
>> --- a/drivers/video/fbdev/sh_mobile_lcdcfb.c
>> +++ b/drivers/video/fbdev/sh_mobile_lcdcfb.c
>> @@ -1337,22 +1337,24 @@ overlay_rop3_store(struct device *dev, struct 
>> device_attribute *attr,
>>       return count;
>>   }
>>   -static DEVICE_ATTR_RW(overlay_alpha);
>> -static DEVICE_ATTR_RW(overlay_mode);
>> -static DEVICE_ATTR_RW(overlay_position);
>> -static DEVICE_ATTR_RW(overlay_rop3);
>> -
>> -static struct attribute *overlay_sysfs_attrs[] __maybe_unused = {
>> -    &dev_attr_overlay_alpha.attr,
>> -    &dev_attr_overlay_mode.attr,
>> -    &dev_attr_overlay_position.attr,
>> -    &dev_attr_overlay_rop3.attr,
>> +static struct device_attribute dev_attr_ovl_alpha =
>> +    __ATTR(ovl_alpha, 0644, overlay_alpha_show, overlay_alpha_store);
>> +static struct device_attribute dev_attr_ovl_mode =
>> +    __ATTR(ovl_mode, 0644, overlay_mode_show, overlay_mode_store);
>> +static struct device_attribute dev_attr_ovl_position =
>> +    __ATTR(ovl_position, 0644, overlay_position_show, 
>> overlay_position_store);
>> +static struct device_attribute dev_attr_ovl_rop3 =
>> +    __ATTR(ovl_rop3, 0644, overlay_rop3_show, overlay_rop3_store);
>> +
>> +static struct attribute *overlay_sysfs_attrs[] = {
>> +    &dev_attr_ovl_alpha.attr,
>> +    &dev_attr_ovl_mode.attr,
>> +    &dev_attr_ovl_position.attr,
>> +    &dev_attr_ovl_rop3.attr,
>>       NULL,
>>   };
>>   -#ifdef CONFIG_FB_DEVICE
>>   ATTRIBUTE_GROUPS(overlay_sysfs);
>> -#endif
>>     static const struct fb_fix_screeninfo sh_mobile_lcdc_overlay_fix  
>> = {
>>       .id =        "SH Mobile LCDC",
>> @@ -1513,6 +1515,7 @@ sh_mobile_lcdc_overlay_fb_unregister(struct 
>> sh_mobile_lcdc_overlay *ovl)
>>       if (info == NULL || info->dev == NULL)
>>           return;
>>   +    device_remove_groups(info->dev, overlay_sysfs_groups);
>>       unregister_framebuffer(ovl->info);
>>   }
>>   @@ -1530,6 +1533,15 @@ sh_mobile_lcdc_overlay_fb_register(struct 
>> sh_mobile_lcdc_overlay *ovl)
>>       if (ret < 0)
>>           return ret;
>>   +    /* The framebuffer device is optional, see fb_device_create(). */
>> +    if (info->dev) {
>
> I think this needs to be:
>     if (dev_of_fbinfo(info)) {
> to avoid compile error when CONFIG_FB_DEVICE=n.
>
>
>> +        ret = device_add_groups(info->dev, overlay_sysfs_groups);
>
> same here. dev_of_fbinfo(info).
>
>> +        if (ret < 0) {
>> +            unregister_framebuffer(info);
>> +            return ret;
>> +        }
>> +    }
>> +
>>       dev_info(lcdc->dev, "registered %s/overlay %u as %dx%d %dbpp.\n",
>>            dev_name(lcdc->dev), ovl->index, info->var.xres,
>>            info->var.yres, info->var.bits_per_pixel);
>> @@ -2637,7 +2649,6 @@ static int sh_mobile_lcdc_probe(struct 
>> platform_device *pdev)
>>   static struct platform_driver sh_mobile_lcdc_driver = {
>>       .driver        = {
>>           .name        = "sh_mobile_lcdc_fb",
>> -        .dev_groups    = overlay_sysfs_groups,
>>           .pm        = &sh_mobile_lcdc_dev_pm_ops,
>>       },
>>       .probe        = sh_mobile_lcdc_probe,
>>
>> base-commit: 986c24e0fe44f844b44d365b71ce831947f50298