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 <j...@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.
