Hi Cédric > Subject: Re: [PATCH v1 01/11] hw/arm/aspeed_ast2400: Use unimp array and > table for unimplemented devices > > On 10/1/26 13:51, Jamin Lin wrote: > > > > > >> Cédric Le Goater <[email protected]> 於 2026年10月1日 下午3:41 寫道： > >> > >> On 9/30/26 14:16, Philippe Mathieu-Daudé wrote: > >>> On 30/9/26 13:50, Cédric Le Goater wrote: > >>>>> Note to Peter: I really think keeping duplicating the > >>>>> UnimplementedDeviceConfig structure is a basic software > >>>>> development antipattern. Could you reconsider your objection? > >>>> What's the background ? > >>> I think that was discussed on IRC, the only ref on the list is: > >>> > https://lore.kernel.org/qemu-devel/CAFEAcA_ob6mHqfAAM9iAMae5vfxaYhmc > >>> [email protected]/ > >>> create_unimplemented_device() calls qdev_create() and allocate on > >>> the heap, my motivation was to have another helper for in-place init > >>> (when the UnimplementedDeviceState state is embedded in the parent). > >>> Now we ended with such helpers like > aspeed_mmio_map_unimplemented() > >>> which realize in place, make_unimp_dev() and create_unimp() init and > >>> realize in place and Jamin adds yet another pattern. While all are > >>> valids, I'd rather unify. > >> > >> Could an array such as : > >> > >> static const AspeedUnimpDevice aspeed_soc_ast2700_unimp_devs[] = { > >> { "dpmcu", "aspeed.dpmcu", ASPEED_DEV_DPMCU, 0x0004000 > 0 }, > >> { "iomem", "aspeed.io", ASPEED_DEV_IOMEM, 0x00FE0000 > }, > >> { "iomem0", "aspeed.iomem0", ASPEED_DEV_IOMEM0, 0x01000000 }, > >> { "iomem1", "aspeed.iomem1", ASPEED_DEV_IOMEM1, > 0x01000000 }, }; > >> > >> be defined at the class level of the SoC and handled automatically ? > >> > >> I guess would need some kind on common SoC model in that case. > >> > >> C. > >> > > Hi Cédric, > > > > I will resend v2 and rework the design along these lines. > > Please wait a bit for Phil's feedback. > > > Defining the unimplemented devices as a class-level array and I will > > look into introducing a common SoC helper/model for this. > > This could be a start for a more generalized solution. > > > Besides that, we found that patch 11 breaks the AST2700 I/O expander > support. > > Ah. Did the functional catch this regression ? > No, we don't have a functional test covering IO Expander 1 I2C. I will add a functional test to cover it and catch this kind of regression in v2.
Thanks, Jamin > > The device list should include: > > > > ASPEED_DEV_IOEXP0_I2C, > > ASPEED_DEV_IOEXP1_I2C, > > ASPEED_DEV_IOEXP0_I3C, > > ASPEED_DEV_IOEXP1_I3C, > > ASPEED_DEV_IOEXP0_INTCIO, > > ASPEED_DEV_IOEXP1_INTCIO, > > > > I will fix this in v2 as well. > > OK. > > Thanks, > > C. >
