Re: [Qemu-trivial] [Qemu-devel] [PATCH 02/10] sm501: Use defines instead of constants where available

[BALATON Zoltan]

On Fri, 24 Feb 2017, Peter Maydell wrote:
> On 19 February 2017 at 16:35, BALATON Zoltan <address@hidden> wrote:
> > Signed-off-by: BALATON Zoltan <address@hidden>
> > ---
> >  hw/display/sm501.c          | 8 ++++----
> >  hw/display/sm501_template.h | 2 +-
> >  2 files changed, 5 insertions(+), 5 deletions(-)
> >
> > diff --git a/hw/display/sm501.c b/hw/display/sm501.c
> > index 4f40dee..4eb085c 100644
> > --- a/hw/display/sm501.c
> > +++ b/hw/display/sm501.c
> > @@ -555,7 +555,7 @@ static uint32_t get_local_mem_size_index(uint32_t size)
> >  static inline int is_hwc_enabled(SM501State *state, int crt)
> >  {
> >      uint32_t addr = crt ? state->dc_crt_hwc_addr : state->dc_panel_hwc_addr;
> > -    return addr & 0x80000000;
> > +    return addr & SM501_HWC_EN;
> >  }
> >
> >  /**
> > @@ -1411,9 +1411,9 @@ void sm501_init(MemoryRegion *address_space_mem, uint32_t 
> > base,
> >      s->local_mem_size_index = get_local_mem_size_index(local_mem_bytes);
> >      SM501_DPRINTF("local mem size=%x. index=%d\n", get_local_mem_size(s),
> >                    s->local_mem_size_index);
> > -    s->system_control = 0x00100000;
> > -    s->misc_control = 0x00001000; /* assumes SH, active=low */
> > -    s->dc_panel_control = 0x00010000;
> > +    s->system_control = 0x00100000; /* 2D engine FIFO empty */
> > +    s->misc_control = SM501_MISC_IRQ_INVERT; /* assumes SH, active=low */
> > +    s->dc_panel_control = 0x00010000; /* FIFO level 3 */
>
> s->misc_control was set to 0x1000, but is now set to
> SM501_MISC_IRQ_INVERT, which is (1 << 16), which is not the
> same value... 0x1000 would be SM501_MISC_DAC_POWER.

Thanks for reviewing these patches.

The comment suggests that the intended bit was the IRQ invert bit so the 
constant used before may have been a bug. (In my understanding nothing 
really uses this bit so I could set it either way you prefer but I think 
this is the correct one.) What should I do?
1. Leave it as it is
2. Set it to the old constant contradicting the comment
3. Make it a separate patch?