Re: [PATCH 02/11] fbdev/vga16fb: Create EGA/VGA devices in sysfb code

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

Hello Thomas,

On 7/7/22 17:39, Thomas Zimmermann wrote:
> Move the device-creation from vga16fb to sysfb code. Move the few
> extra videomode checks into vga16fb's probe function.
> 
> The vga16fb driver requires a screen_info for type VIDEO_TYPE_VGAC
> or VIDEO_TYPE_EGAC. Such code is nowhere present in the kernel, except
> for some MIPS systems. It's not clear if the vga16fb driver actually
> works in practice.
> 
> Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
> ---
>  drivers/firmware/sysfb.c      |  4 +++
>  drivers/video/fbdev/vga16fb.c | 59 +++++++++++++++++------------------
>  2 files changed, 32 insertions(+), 31 deletions(-)
> 
> diff --git a/drivers/firmware/sysfb.c b/drivers/firmware/sysfb.c
> index 1f276f108cc9..3fd3563d962b 100644
> --- a/drivers/firmware/sysfb.c
> +++ b/drivers/firmware/sysfb.c
> @@ -94,6 +94,10 @@ static __init int sysfb_init(void)
>  		name = "efi-framebuffer";
>  	else if (si->orig_video_isVGA == VIDEO_TYPE_VLFB)
>  		name = "vesa-framebuffer";
> +	else if (si->orig_video_isVGA == VIDEO_TYPE_VGAC)
> +		name = "vga-framebuffer";
> +	else if (si->orig_video_isVGA == VIDEO_TYPE_EGAC)
> +

I wonder if we really need to do this distinction or could just have a single
platform device for both VIDEO_TYPE_VGAC and VIDEO_TYPE_EGAC. After all, the
same fbdev driver is bound against both platform devices.

[...]

>  static int vga16fb_probe(struct platform_device *dev)
>  {
> +	struct screen_info *si;
>  	struct fb_info *info;
>  	struct vga16fb_par *par;
>  	int i;
>  	int ret = 0;
>  
> +	si = dev_get_platdata(&dev->dev);
> +	if (!si)
> +		return -ENODEV;
> +
> +	ret = check_mode_supported(si);
> +	if (ret)
> +		return ret;
> +

What do you see as the advantage of moving the check to the driver's probe?

Because after this change the platform driver will be registered but for no
reason, since can't even probe if orig_video_isVGA is neither VGAC nor EGAC.

[...]

> +static const struct platform_device_id vga16fb_driver_id_table[] = {
> +	{"ega-framebuffer", 0},
> +	{"vga-framebuffer", 0},
> +	{ }
> +};
> +

The fact that the two entries don't have a platform data is an indication for
me that we could just consolidate in a single "vga16-framebuffer" or smt. I
know that this won't be consistent with efi, vesa, etc but I don't think is
that important and also quite likely we will get rid of this driver and the
platform device registration soon. Since as you said, it's unclear that is
even used.

>  static struct platform_driver vga16fb_driver = {
>  	.probe = vga16fb_probe,
>  	.remove = vga16fb_remove,
>  	.driver = {
> -		.name = "vga16fb",
> +		.name = "vga-framebuffer",
>  	},

Maybe "vga16-framebuffer" instead? Since for example VESA is also VGA AFAIK.

-- 
Best regards,

Javier Martinez Canillas
Linux Engineering
Red Hat