From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:49044) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1Z0Uc4-0005Nr-OV for qemu-devel@nongnu.org; Thu, 04 Jun 2015 08:51:14 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1Z0Uc0-00081f-IT for qemu-devel@nongnu.org; Thu, 04 Jun 2015 08:51:12 -0400 Received: from mx1.redhat.com ([209.132.183.28]:36300) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1Z0Uc0-00081S-C9 for qemu-devel@nongnu.org; Thu, 04 Jun 2015 08:51:08 -0400 Message-ID: <557049B7.4010801@redhat.com> Date: Thu, 04 Jun 2015 14:51:03 +0200 From: Laszlo Ersek MIME-Version: 1.0 References: <1433351328-23326-1-git-send-email-pbonzini@redhat.com> <1433351328-23326-9-git-send-email-pbonzini@redhat.com> <55700630.7090006@redhat.com> In-Reply-To: <55700630.7090006@redhat.com> Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: 7bit Subject: Re: [Qemu-devel] [PATCH v2 08/23] pflash_cfi01: change to new-style MMIO accessors List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Paolo Bonzini , Peter Crosthwaite Cc: "Michael S. Tsirkin" , "qemu-devel@nongnu.org Developers" , Gerd Hoffmann On 06/04/15 10:02, Paolo Bonzini wrote: > > > On 04/06/2015 08:19, Peter Crosthwaite wrote: >> On Wed, Jun 3, 2015 at 10:08 AM, Paolo Bonzini wrote: >>> This is a required step to implement read_with_attrs and write_with_attrs. >>> >>> Signed-off-by: Paolo Bonzini >>> --- >>> hw/block/pflash_cfi01.c | 96 ++++++------------------------------------------- >> >> Nice stats. >> >>> 1 file changed, 10 insertions(+), 86 deletions(-) >>> >>> diff --git a/hw/block/pflash_cfi01.c b/hw/block/pflash_cfi01.c >>> index 7507a15..0b3667a 100644 >>> --- a/hw/block/pflash_cfi01.c >>> +++ b/hw/block/pflash_cfi01.c >>> @@ -650,101 +650,25 @@ static void pflash_write(pflash_t *pfl, hwaddr offset, >>> } >>> >>> >>> -static uint32_t pflash_readb_be(void *opaque, hwaddr addr) >>> -{ >>> - return pflash_read(opaque, addr, 1, 1); >>> -} >>> - >>> -static uint32_t pflash_readb_le(void *opaque, hwaddr addr) >>> -{ >>> - return pflash_read(opaque, addr, 1, 0); >>> -} >>> - >>> -static uint32_t pflash_readw_be(void *opaque, hwaddr addr) >>> +static uint64_t pflash_mem_read(void *opaque, hwaddr addr, unsigned len) >>> { >>> pflash_t *pfl = opaque; >>> + bool be = !!(pfl->features & (1 << PFLASH_BE)); >> >> !!() not needed. Otherwise > > I don't like magic bool-ification... Is there a coding style item that > forbids this idiom in bool assignments? (Side remark: in edk2, BOOLEAN is actually UINT8. !!(expr) -- or, ((expr) != 0) -- is a necessity there.) Thanks Laszlo > > Paolo > >> Reviewed-by: Peter Crosthwaite >> >>> >>> - return pflash_read(pfl, addr, 2, 1); >>> + return pflash_read(pfl, addr, len, be); >>> } >>> >>> -static uint32_t pflash_readw_le(void *opaque, hwaddr addr) >>> +static void pflash_mem_write(void *opaque, hwaddr addr, uint64_t value, unsigned len) >>> { >>> pflash_t *pfl = opaque; >>> + bool be = !!(pfl->features & (1 << PFLASH_BE)); >>> >>> - return pflash_read(pfl, addr, 2, 0); >>> + pflash_write(pfl, addr, value, len, be); >>> } >>> >>> -static uint32_t pflash_readl_be(void *opaque, hwaddr addr) >>> -{ >>> - pflash_t *pfl = opaque; >>> - >>> - return pflash_read(pfl, addr, 4, 1); >>> -} >>> - >>> -static uint32_t pflash_readl_le(void *opaque, hwaddr addr) >>> -{ >>> - pflash_t *pfl = opaque; >>> - >>> - return pflash_read(pfl, addr, 4, 0); >>> -} >>> - >>> -static void pflash_writeb_be(void *opaque, hwaddr addr, >>> - uint32_t value) >>> -{ >>> - pflash_write(opaque, addr, value, 1, 1); >>> -} >>> - >>> -static void pflash_writeb_le(void *opaque, hwaddr addr, >>> - uint32_t value) >>> -{ >>> - pflash_write(opaque, addr, value, 1, 0); >>> -} >>> - >>> -static void pflash_writew_be(void *opaque, hwaddr addr, >>> - uint32_t value) >>> -{ >>> - pflash_t *pfl = opaque; >>> - >>> - pflash_write(pfl, addr, value, 2, 1); >>> -} >>> - >>> -static void pflash_writew_le(void *opaque, hwaddr addr, >>> - uint32_t value) >>> -{ >>> - pflash_t *pfl = opaque; >>> - >>> - pflash_write(pfl, addr, value, 2, 0); >>> -} >>> - >>> -static void pflash_writel_be(void *opaque, hwaddr addr, >>> - uint32_t value) >>> -{ >>> - pflash_t *pfl = opaque; >>> - >>> - pflash_write(pfl, addr, value, 4, 1); >>> -} >>> - >>> -static void pflash_writel_le(void *opaque, hwaddr addr, >>> - uint32_t value) >>> -{ >>> - pflash_t *pfl = opaque; >>> - >>> - pflash_write(pfl, addr, value, 4, 0); >>> -} >>> - >>> -static const MemoryRegionOps pflash_cfi01_ops_be = { >>> - .old_mmio = { >>> - .read = { pflash_readb_be, pflash_readw_be, pflash_readl_be, }, >>> - .write = { pflash_writeb_be, pflash_writew_be, pflash_writel_be, }, >>> - }, >>> - .endianness = DEVICE_NATIVE_ENDIAN, >>> -}; >>> - >>> -static const MemoryRegionOps pflash_cfi01_ops_le = { >>> - .old_mmio = { >>> - .read = { pflash_readb_le, pflash_readw_le, pflash_readl_le, }, >>> - .write = { pflash_writeb_le, pflash_writew_le, pflash_writel_le, }, >>> - }, >>> +static const MemoryRegionOps pflash_cfi01_ops = { >>> + .read = pflash_mem_read, >>> + .write = pflash_mem_write, >>> .endianness = DEVICE_NATIVE_ENDIAN, >>> }; >>> >>> @@ -775,7 +699,7 @@ static void pflash_cfi01_realize(DeviceState *dev, Error **errp) >>> >>> memory_region_init_rom_device( >>> &pfl->mem, OBJECT(dev), >>> - pfl->features & (1 << PFLASH_BE) ? &pflash_cfi01_ops_be : &pflash_cfi01_ops_le, >>> + &pflash_cfi01_ops, >>> pfl, >>> pfl->name, total_len, &local_err); >>> if (local_err) { >>> -- >>> 2.4.1 >>> >>> >>>