Re: [PATCH 0/4] hw/block/pflash_cfi01: Remove pflash_cfi01_get_memory()

[Peter Maydell]

On Mon, 15 Mar 2021 at 11:34, Paolo Bonzini <pbonzini@redhat.com> wrote:
>
> On 07/03/21 23:26, Philippe Mathieu-Daudé wrote:
> > TYPE_PFLASH_CFI01 is a TYPE_SYS_BUS_DEVICE which registers its romd
> > MemoryRegion with sysbus_init_mmio(), so we can use the generic
> > sysbus_mmio_get_region() to get the region, no need for a specific
> > pflash_cfi01_get_memory() helper.
> >
> > First replace the few pflash_cfi01_get_memory() uses by
> > sysbus_mmio_get_region(), then remove the now unused helper.
>
> Why is this an improvement?  You're replacing nice and readable code
> with an implementation-dependent function whose second argument is a
> magic number.  The right patch would _add_ more of these helpers, not
> remove them.

I agree that sysbus_mmio_get_region()'s use of arbitrary
integers is unfortunate (we should look at improving that
to use usefully named regions I guess), but I don't see
why pflash_cfi01 should expose its MemoryRegion to users
in a different way to every other sysbus device.

thanks
-- PMM