Re: [Qemu-devel] [PATCH] SM501 emulation for R2D-SH4

[Shin-ichiro KAWASAKI]

Thank you for your review!

> > I'm not sure about following two points.
> >
> >  - Register definitions were copied from Linux : include/linux/sm501-regs.h
>
> I'd try to suck it into the .c file because the definitions are not
> going to be used anywhere else in qemu.

OK.  I'll include it in sm501.c.


> > +#include "vga_int.h"
>
> I think <vga_int.h> isn't needed?

Right.  Will be deleted.


> > +static void sm501_draw_crt(SM501State * s)
> > +{
> > +    int x, y;
> > +    uint32_t crt_width = (s->dc_crt_h_total & 0x00000FFF) + 1;
> > +    uint32_t crt_height = (s->dc_crt_v_total & 0x00000FFF) + 1;
> > +    uint8_t  * buf = s->local_mem;
> > +    uint32_t * palette = (uint32_t *)s->dc_crt_palette;
> > +
> > +    /* adjust console size */
> > +    if (s->last_width != crt_width || s->last_height != crt_height) {
> > +       qemu_console_resize(s->console, crt_width, crt_height);
> > +       s->last_width = crt_width;
> > +       s->last_height = crt_height;
> > +    }
> > +
> > +    switch (s->dc_crt_control & 3) {
> > +    case SM501_DC_CRT_CONTROL_8BPP:
> > +       for (y = 0; y < crt_height; y++) {
> > +           for (x = 0; x < crt_width; x++) {
> > +               int i = (y * crt_width + x) * 4;
> > +               *(uint32_t *)&s->ds->data[i] = palette[*buf];
> > +               buf++;
> > +           }
> > +       }
> > +       break;
> > +    case SM501_DC_CRT_CONTROL_16BPP:
> > +       for (y = 0; y < crt_height; y++) {
> > +           for (x = 0; x < crt_width; x++) {
> > +               int i = (y * crt_width + x) * 4;
> > +               uint32_t rgb565 = *(uint16_t*)buf;
> > +               int r = ((rgb565 >> 11) & 0x1f) << 3;
> > +               int g = ((rgb565 >>  5) & 0x3f) << 2;
> > +               int b = ((rgb565 >>  0) & 0x1f) << 3;
> > +               s->ds->data[i + 0] = b;
> > +               s->ds->data[i + 1] = g;
> > +               s->ds->data[i + 2] = r;
> > +               s->ds->data[i + 3] = 0;
> > +               buf += 2;
> > +           }
> > +       }
> > +       break;
> > +    case SM501_DC_CRT_CONTROL_32BPP:
> > +       for (y = 0; y < crt_height; y++) {
> > +           for (x = 0; x < crt_width; x++) {
> > +               int i = (y * crt_width + x) * 4;
> > +               *(uint32_t *)&s->ds->data[i] = *(uint32_t*)buf;
> > +               buf += 4;
> > +           }
> > +       }
> > +       break;
>
> All the cases assume the host is using 32 bpp colours, which is rare I
> think.  Because s->ds->depth is not checked, it will likely segfault.

Right.  I will introduce templates like hw/vga_template.h to fit
s->ds->depth variety.

Thanks!

Shin-ichiro KAWASAKI