| Message ID | 379a9021607724194b457bf562df9900993788c0.1488068248.git.balaton@eik.bme.hu |
|---|---|
| State | New |
| Headers | show |
On 10 December 2016 at 02:05, BALATON Zoltan <balaton@eik.bme.hu> 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 <balaton@eik.bme.hu> > --- > > 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
On Thu, 2 Mar 2017, Peter Maydell wrote: > On 10 December 2016 at 02:05, BALATON Zoltan <balaton@eik.bme.hu> 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 <balaton@eik.bme.hu> >> --- >> >> 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.) I've stored these in the state because these will be needed if we ever implement more of the 2D engine even if they are not used yet. (Also as you've noticed read is also imlemented.) > > thanks > -- PMM > >
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; static uint32_t get_local_mem_size_index(uint32_t size) @@ -945,6 +954,10 @@ static uint64_t sm501_disp_ctrl_read(void *opaque, hwaddr addr, ret = s->dc_panel_v_sync; break; + case SM501_DC_VIDEO_CONTROL: + ret = s->dc_video_control; + break; + case SM501_DC_CRT_CONTROL: ret = s->dc_crt_control; break; @@ -1060,6 +1073,10 @@ static void sm501_disp_ctrl_write(void *opaque, hwaddr addr, s->dc_panel_hwc_color_3 = value & 0x0000FFFF; break; + case SM501_DC_VIDEO_CONTROL: + s->dc_video_control = value & 0x00037FFF; + break; + case SM501_DC_CRT_CONTROL: s->dc_crt_control = value & 0x0003FFFF; break; @@ -1135,6 +1152,9 @@ static uint64_t sm501_2d_engine_read(void *opaque, hwaddr addr, case SM501_2D_SOURCE_BASE: ret = s->twoD_source_base; break; + case SM501_2D_STATUS: + ret = 0; /* Should return interrupt status */ + break; default: printf("sm501 disp ctrl : not implemented register read." " addr=%x\n", (int)addr); @@ -1177,15 +1197,33 @@ static void sm501_2d_engine_write(void *opaque, hwaddr addr, case SM501_2D_FOREGROUND: s->twoD_foreground = value; break; + case SM501_2D_BACKGROUND: + s->twoD_background = value; + break; case SM501_2D_STRETCH: s->twoD_stretch = value; break; + case SM501_2D_COLOR_COMPARE: + s->twoD_color_compare = value; + break; case SM501_2D_COLOR_COMPARE_MASK: s->twoD_color_compare_mask = value; break; case SM501_2D_MASK: s->twoD_mask = value; break; + case SM501_2D_CLIP_TL: + s->twoD_clip_tl = value; + break; + case SM501_2D_CLIP_BR: + s->twoD_clip_br = value; + break; + case SM501_2D_MONO_PATTERN_LOW: + s->twoD_mono_pattern_low = value; + break; + case SM501_2D_MONO_PATTERN_HIGH: + s->twoD_mono_pattern_high = value; + break; case SM501_2D_WINDOW_WIDTH: s->twoD_window_width = value; break; @@ -1195,6 +1233,15 @@ static void sm501_2d_engine_write(void *opaque, hwaddr addr, case SM501_2D_DESTINATION_BASE: s->twoD_destination_base = value; break; + case SM501_2D_ALPHA: + s->twoD_alpha = value; + break; + case SM501_2D_WRAP: + s->twoD_wrap = value; + break; + case SM501_2D_STATUS: + /* ignored, writing 0 should clear interrupt status */ + break; default: printf("sm501 2d engine : not implemented register write." " addr=%x, val=%x\n", (int)addr, (unsigned)value);
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 <balaton@eik.bme.hu> --- 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(-)