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

[Helge Deller <deller@gmx.de>]

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....

  
>   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