Hello, On Wed, 21 Aug 2019, Gerd Hoffmann wrote:
@@ -672,48 +678,71 @@ static void ati_mm_write(void *opaque, hwaddr addr, case 0xf00 ... 0xfff: /* read-only copy of PCI config space so ignore writes */ break; - case CUR_OFFSET: - if (s->regs.cur_offset != (data & 0x87fffff0)) { - s->regs.cur_offset = data & 0x87fffff0; + case CUR_OFFSET ... CUR_OFFSET + 3: + { + uint32_t t = s->regs.cur_offset; + + ati_reg_write_offs(&t, addr - CUR_OFFSET, data, size); + t &= 0x87fffff0; + if (s->regs.cur_offset != t) { + s->regs.cur_offset = t;Repeated pattern. I'd suggest to add a "wmask" parameter to ati_reg_write_offs. Maybe also make it return true/false depending on whenever the value did change or not.
This is a pattern in these HW cursor related regs but other callers of write_offs don't do that (currently there are one more of the CUR_* regs vs. others 5 to 4 but there may be other uses later as several regs support less than 32 bit access). It would also break symmetry between read_offs and write_offs so I think I'd leave this off write_offs for now unless new callers in the future will also need wmask. (It you insist I could make it a macro for CUR_* regs but not sure that would improve it much.)
Regards, BALATON Zoltan
