Re: [Qemu-devel] [PATCH v2 11/14] sm501: Add some more missing registers

[Peter Maydell]

On 10 December 2016 at 02:05, BALATON Zoltan <address@hidden> wrote:
> Values are not used yet, only stored to allow clients to initialise
> these without failing as long as no 2D engine function is called that
> would use the written value.
>
> Signed-off-by: BALATON Zoltan <address@hidden>
> ---
>
> v2: Fixed mask of video_control register for a read only bit
>     Changed IRQ status register to write ignored as IRQ is not implemented
>
>  hw/display/sm501.c | 49 ++++++++++++++++++++++++++++++++++++++++++++++++-
>  1 file changed, 48 insertions(+), 1 deletion(-)
>
> diff --git a/hw/display/sm501.c b/hw/display/sm501.c
> index caa7e5c..af5e4db 100644
> --- a/hw/display/sm501.c
> +++ b/hw/display/sm501.c
> @@ -511,6 +511,8 @@ typedef struct SM501State {
>      uint32_t dc_panel_hwc_color_1_2;
>      uint32_t dc_panel_hwc_color_3;
>
> +    uint32_t dc_video_control;
> +
>      uint32_t dc_crt_control;
>      uint32_t dc_crt_fb_addr;
>      uint32_t dc_crt_fb_offset;
> @@ -530,13 +532,20 @@ typedef struct SM501State {
>      uint32_t twoD_control;
>      uint32_t twoD_pitch;
>      uint32_t twoD_foreground;
> +    uint32_t twoD_background;
>      uint32_t twoD_stretch;
> +    uint32_t twoD_color_compare;
>      uint32_t twoD_color_compare_mask;
>      uint32_t twoD_mask;
> +    uint32_t twoD_clip_tl;
> +    uint32_t twoD_clip_br;
> +    uint32_t twoD_mono_pattern_low;
> +    uint32_t twoD_mono_pattern_high;
>      uint32_t twoD_window_width;
>      uint32_t twoD_source_base;
>      uint32_t twoD_destination_base;
> -
> +    uint32_t twoD_alpha;
> +    uint32_t twoD_wrap;
>  } SM501State;

You're still implementing almost all of these as writes to structure
fields which can never be read. Either of these:

 (1) a write function case which writes to the struct field, plus
     a read function case which returns the value in the struct field
 (2) a write function which does nothing (and no read function case,
     or a read function case which returns 0)
     [you can usually bunch these up into a lot of case FOO: which
     share a /* unimplemented, ignore writes */ body]

are OK, but a write function which writes to the struct field but
nothing ever reading that field doesn't make sense.

(You can also LOG_UNIMP for dummy stuff like this if you like, but
that's not something we insist on.)


thanks
-- PMM