Re: [Qemu-devel] [PATCH][SPARC] LEON3: Add emulation of AMBA plug&play

[Fabien Chouteau <chouteau@adacore.com>]

On 10/08/2014 05:38 PM, Andreas Färber wrote:
> Hi,
> 

Hi Andreas,

> Am 08.10.2014 um 16:19 schrieb Fabien Chouteau:
>> From: Jiri Gaisler <jiri@gaisler.se>
>>
>> +
>> +#define TYPE_GRLIB_APB_PNP "grlib,apbpnp"
> 
> If you move the two TYPE_* constants to grlib.h, you can reuse them.
> 

Will do.

>> +#define GRLIB_APB_PNP(obj) \
>> +    OBJECT_CHECK(APBPNP, (obj), TYPE_GRLIB_APB_PNP)
>> +
>> +typedef struct APBPNP {
>> +    SysBusDevice parent_obj;
>> +    MemoryRegion iomem;
>> +} APBPNP;
>> +
>> +static uint64_t grlib_apbpnp_read(void *opaque, hwaddr addr,
>> +                                   unsigned size)
> 
> Indentation is off by one for all read/write functions.
> 

Are you sure? The indentation is 4 spaces right? (checkpatch.pl didn't
raise any error).

>> +static int grlib_apbpnp_init(SysBusDevice *dev)
>> +{
>> +    APBPNP *pnp = GRLIB_APB_PNP(dev);
>> +
>> +    memory_region_init_io(&pnp->iomem, OBJECT(pnp), &grlib_apbpnp_ops, pnp,
>> +                          "apbpnp", APBPNP_REG_SIZE);
>> +
>> +    sysbus_init_mmio(dev, &pnp->iomem);
>
> APBPNP_REG_SIZE seems constant, so you could move both lines into an
> instance_init function.
>

Will do. I don't need a .class_init then.

>> +
>> +    k->init = grlib_apbpnp_init;
>> +}
>> +
>> +static const TypeInfo grlib_apbpnp_info = {
>> +    .name          = TYPE_GRLIB_APB_PNP,
>> +    .parent        = TYPE_SYS_BUS_DEVICE,
>> +    .instance_size = sizeof(APBPNP),
>> +    .class_init    = grlib_apbpnp_class_init,
>> +};
>> +
>> +static void grlib_apbpnp_register_types(void)
>> +{
>> +    type_register_static(&grlib_apbpnp_info);
>> +}
>> +
>> +type_init(grlib_apbpnp_register_types)
> 
> Please either split into two .c files here, ...
> 

>> 
> ... or if unavoidable use just one type_init and registration function.
> +

I will create one type init for both memory regions.

>> +static inline
>> +DeviceState *grlib_ahbpnp_create(hwaddr  base)
>> +{
>> +    DeviceState *dev;
>> +
>> +    dev = qdev_create(NULL, "grlib,ahbpnp");
>> +
>> +    if (qdev_init(dev)) {
>> +        return NULL;
>> +    }
>> +
>> +    sysbus_mmio_map(SYS_BUS_DEVICE(dev), 0, base);
>> +
>> +    return dev;
>> +}
>> +
>>  #endif /* ! _GRLIB_H_ */
> 
> Are these functions really needed? Can't you just inline them?
> Also note that the return value is never actually checked.
>

This is what we do for all GRLIB devices, I think it makes a cleaner
machine init.

Thanks for the review.