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

[Ninad Palsule <ninad@linux.ibm.com>]

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