Re: [PATCH v10 3/9] hw/fsi: Introduce IBM's cfam

[Ninad Palsule]

Hello Cedric,

> > [ clg: - moved object FSIScratchPad under FSICFAMState
> >         - moved FSIScratchPad code under cfam.c
> >         - introduced fsi_cfam_instance_init()
> >         - reworked fsi_cfam_realize() ]
>
> Move the list down before my S-o-b.
Done.
> > +
> > +/* Valid, slots, version, type, crc */
> > +#define CFAM_CONFIG_REG_FSI_SLAVE (ENGINE_CONFIG_NEXT       | \
> > + 0x00010000               | \
> > + 0x00005000               | \
> > + ENGINE_CONFIG_TYPE_FSI   | \
> > +                                            0x0000000a)
> > +
> > +/* Valid, slots, version, type, crc */
> > +#define CFAM_CONFIG_REG_SCRATCHPAD (ENGINE_CONFIG_NEXT       | \
> > + 0x00010000               | \
> > + 0x00001000               | \
> > + ENGINE_CONFIG_TYPE_SCRATCHPAD | \
> > +                                            0x00000007)
>
> I was expecting a macro taking argument to build the config reg value
> of each sub engine but that's fine also.

Added single macro.


> > +
> > +    memory_region_add_subregion(&cfam->mr, 0, &cfam->config_iomem);
> > +    memory_region_add_subregion(&cfam->mr, 0x800, &slave->iomem);
> > +    memory_region_add_subregion(&cfam->mr, 0xc00, &cfam->lbus.mr);
> > +
> > +    /* Add scratchpad engine */
> > +    if (!qdev_realize(DEVICE(&cfam->scratchpad), BUS(&cfam->lbus),
> > +                                errp)) {
>
> could be a single line.

Yep, Made it a single line.

Thanks for the review.

Regards,

Ninad