Clients normally only write to these registers, nothing is known to
ever read them but they are documented as read/write so allow clients
to also read the values.

Signed-off-by: BALATON Zoltan <balaton@eik.bme.hu>
---
 hw/display/sm501.c | 57 ++++++++++++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 57 insertions(+)

diff --git a/hw/display/sm501.c b/hw/display/sm501.c
index af5e4db..32223f6 100644
--- a/hw/display/sm501.c
+++ b/hw/display/sm501.c
@@ -1149,9 +1149,66 @@ static uint64_t sm501_2d_engine_read(void *opaque, hwaddr addr,
     SM501_DPRINTF("sm501 2d engine regs : read addr=%x\n", (int)addr);
 
     switch (addr) {
+    case SM501_2D_SOURCE:
+        ret = s->twoD_source;
+        break;
+    case SM501_2D_DESTINATION:
+        ret = s->twoD_destination;
+        break;
+    case SM501_2D_DIMENSION:
+        ret = s->twoD_dimension;
+        break;
+    case SM501_2D_CONTROL:
+        ret = s->twoD_control;
+        break;
+    case SM501_2D_PITCH:
+        ret = s->twoD_pitch;
+        break;
+    case SM501_2D_FOREGROUND:
+        ret = s->twoD_foreground;
+        break;
+    case SM501_2D_BACKGROUND:
+        ret = s->twoD_background;
+        break;
+    case SM501_2D_STRETCH:
+        ret = s->twoD_stretch;
+        break;
+    case SM501_2D_COLOR_COMPARE:
+        ret = s->twoD_color_compare;
+        break;
+    case SM501_2D_COLOR_COMPARE_MASK:
+        ret = s->twoD_color_compare_mask;
+        break;
+    case SM501_2D_MASK:
+        ret = s->twoD_mask;
+        break;
+    case SM501_2D_CLIP_TL:
+        ret = s->twoD_clip_tl;
+        break;
+    case SM501_2D_CLIP_BR:
+        ret = s->twoD_clip_br;
+        break;
+    case SM501_2D_MONO_PATTERN_LOW:
+        ret = s->twoD_mono_pattern_low;
+        break;
+    case SM501_2D_MONO_PATTERN_HIGH:
+        ret = s->twoD_mono_pattern_high;
+        break;
+    case SM501_2D_WINDOW_WIDTH:
+        ret = s->twoD_window_width;
+        break;
     case SM501_2D_SOURCE_BASE:
         ret = s->twoD_source_base;
         break;
+    case SM501_2D_DESTINATION_BASE:
+        ret = s->twoD_destination_base;
+        break;
+    case SM501_2D_ALPHA:
+        ret = s->twoD_alpha;
+        break;
+    case SM501_2D_WRAP:
+        ret = s->twoD_wrap;
+        break;
     case SM501_2D_STATUS:
         ret = 0; /* Should return interrupt status */
         break;
-- 
2.7.4

On 25 February 2017 at 21:47, BALATON Zoltan <balaton@eik.bme.hu> wrote:
> Clients normally only write to these registers, nothing is known to
> ever read them but they are documented as read/write so allow clients
> to also read the values.
>
> Signed-off-by: BALATON Zoltan <balaton@eik.bme.hu>
> ---
>  hw/display/sm501.c | 57 ++++++++++++++++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 57 insertions(+)

Oh, the read code is in a separate patch? Please put all the handling
for a particular register in the same patch. If you think the
resulting patch is too long you can split it into several patches
each of which handles a group of registers if you like (but it
probably isn't necessary).

thanks
-- PMM

On Thu, 2 Mar 2017, Peter Maydell wrote:
> On 25 February 2017 at 21:47, BALATON Zoltan <balaton@eik.bme.hu> wrote:
>> Clients normally only write to these registers, nothing is known to
>> ever read them but they are documented as read/write so allow clients
>> to also read the values.
>>
>> Signed-off-by: BALATON Zoltan <balaton@eik.bme.hu>
>> ---
>>  hw/display/sm501.c | 57 ++++++++++++++++++++++++++++++++++++++++++++++++++++++
>>  1 file changed, 57 insertions(+)
>
> Oh, the read code is in a separate patch? Please put all the handling
> for a particular register in the same patch. If you think the
> resulting patch is too long you can split it into several patches
> each of which handles a group of registers if you like (but it
> probably isn't necessary).

So is it OK to squash this in the previous patch adding write and make the 
two a single patch?

>
> thanks
> -- PMM
>
>

On 2 March 2017 at 20:22, BALATON Zoltan <balaton@eik.bme.hu> wrote:
> On Thu, 2 Mar 2017, Peter Maydell wrote:
>>
>> On 25 February 2017 at 21:47, BALATON Zoltan <balaton@eik.bme.hu> wrote:
>>>
>>> Clients normally only write to these registers, nothing is known to
>>> ever read them but they are documented as read/write so allow clients
>>> to also read the values.
>>>
>>> Signed-off-by: BALATON Zoltan <balaton@eik.bme.hu>
>>> ---
>>>  hw/display/sm501.c | 57
>>> ++++++++++++++++++++++++++++++++++++++++++++++++++++++
>>>  1 file changed, 57 insertions(+)
>>
>>
>> Oh, the read code is in a separate patch? Please put all the handling
>> for a particular register in the same patch. If you think the
>> resulting patch is too long you can split it into several patches
>> each of which handles a group of registers if you like (but it
>> probably isn't necessary).
>
>
> So is it OK to squash this in the previous patch adding write and make the
> two a single patch?

Yes please.

thanks
-- PMM