Thomas Zimmermann <tzimmermann@xxxxxxx> writes:
Hello Thomas,
> The plain values as stored in struct screen_info need to be decoded
> before being used. Add helpers that decode the type of video output
> and the framebuffer I/O aperture.
>
> Old or non-x86 systems may not set the type of video directly, but
> only indicate the presence by storing 0x01 in orig_video_isVGA. The
> decoding logic in screen_info_video_type() takes this into account.
I always disliked how the orig_video_isVGA variable lost its meaning.
> It then follows similar code in vgacon's vgacon_startup() to detect
> the video type from the given values.
>
> A call to screen_info_resources() returns all known resources of the
> given screen_info. The resources' values have been taken from existing
> code in vgacon and vga16fb. These drivers can later be converted to
> use the new interfaces.
>
> Signed-off-by: Thomas Zimmermann <tzimmermann@xxxxxxx>
> ---
Thanks for doing this! It's quite useful to have these helpers, since as
you mention the screen_info data decoding is complex and the variables
used to store the video type and modes are confusing / misleading.
Reviewed-by: Javier Martinez Canillas <javierm@xxxxxxxxxx>
I just have a few comments below:
> +static inline bool __screen_info_has_ega_gfx(unsigned int mode)
> +{
> + switch (mode) {
> + case 0x0d: /* 320x200-4 */
> + case 0x0e: /* 640x200-4 */
> + case 0x0f: /* 640x350-1 */
> + case 0x10: /* 640x350-4 */
I wonder if makes sense to define some constant macros for these modes? I
know that check_mode_supported() in drivers/video/fbdev/vga16fb.c also use
magic numbers but I believe that it could ease readability.
> + return true;
> + default:
> + return false;
> + }
> +}
> +
> +static inline bool __screen_info_has_vga_gfx(unsigned int mode)
> +{
> + switch (mode) {
> + case 0x10: /* 640x480-1 */
> + case 0x12: /* 640x480-4 */
> + case 0x13: /* 320-200-8 */
> + case 0x6a: /* 800x600-4 (VESA) */
> + return true;
And same for these.
It can be a follow-up patch though.
[...]
> +int screen_info_resources(const struct screen_info *si, struct resource *r, size_t num)
> +{
> + struct resource *pos = r;
> + unsigned int type = screen_info_video_type(si);
> + u64 base, size;
> +
> + switch (type) {
> + case VIDEO_TYPE_MDA:
> + if (num > 0)
> + resource_init_io_named(pos++, 0x3b0, 12, "mda");
I see that drivers/video/fbdev/i740_reg.h has a #define MDA_BASE
0x3B0. Maybe move to a header in include/video along with the other
constants mentioned above ?
> + if (num > 1)
> + resource_init_io_named(pos++, 0x3bf, 0x01, "mda");
> + if (num > 2)
> + resource_init_mem_named(pos++, 0xb0000, 0x2000, "mda");
Same for these start addresses. I see that are also used by mdacon_startup()
in drivers/video/console/mdacon.c, so some constants defined somewhere might
be useful for that console driver too.
The comment also applies to all the other start addresses, since I see
that those are used by other drivers (i810, vgacon, etc).
--
Best regards,
Javier Martinez Canillas
Core Platforms
Red Hat
