On Sun, Sep 27, 2015 at 9:32 PM, Peter Crosthwaite <crosthwaitepe...@gmail.com> wrote: > On Sun, Sep 27, 2015 at 10:51 AM, Max Filippov <jcmvb...@gmail.com> wrote: >> On Sun, Sep 27, 2015 at 8:25 PM, Peter Crosthwaite >> <crosthwaitepe...@gmail.com> wrote: >>> On Sun, Sep 27, 2015 at 10:16 AM, Max Filippov <jcmvb...@gmail.com> wrote: >>>> pflash_cfi01_register always registers FLASH to the default >>>> system_memory region, which may not always be the right thing. >>>> >>>> Provide pflash_cfi01_init function that only creates FLASH device and >>>> sets its properties, leaving MMIO region registration to its caller. >>>> >>> >>> You should just use QOM to create your device inline without need for >>> a construction helper. See Vexpress's ve_pflash_cfi01_register for an >>> example. >> >> I thought about it and it looks like a lot of boilerplate code for nothing. > > Not really. It self documents the code. This is instantly understandable: > > qdev_prop_set_uint32(dev, "num-blocks", > VEXPRESS_FLASH_SIZE / VEXPRESS_FLASH_SECT_SIZE); > qdev_prop_set_uint64(dev, "sector-length", VEXPRESS_FLASH_SECT_SIZE); > qdev_prop_set_uint8(dev, "width", 4); > qdev_prop_set_uint8(dev, "device-width", 2); > qdev_prop_set_bit(dev, "big-endian", false); > qdev_prop_set_uint16(dev, "id0", 0x89); > qdev_prop_set_uint16(dev, "id1", 0x18); > qdev_prop_set_uint16(dev, "id2", 0x00); > qdev_prop_set_uint16(dev, "id3", 0x00); > qdev_prop_set_string(dev, "name", name); > > Whereas a call to pflash_cfi_register is a jumble of numbers without > any labelling of what is what. In this scheme you don't have to open > headers to figure out what args are. > >> Maybe it'd be better to remove qdev_init_nofail call from that init function >> and let ve_pflash_cfi01_register reuse it too? >> > > There was a conscious decision some time ago to get rid of qdev init > helper functions as provided by devs themselves. The interface between > dev and board should be QOM.
Ok. -- Thanks. -- Max