From mboxrd@z Thu Jan  1 00:00:00 1970
Received: from eggs.gnu.org ([2001:4830:134:3::10]:42044)
	by lists.gnu.org with esmtp (Exim 4.71)
	(envelope-from <chouteau@adacore.com>) id 1Xc9KR-00030E-As
	for qemu-devel@nongnu.org; Thu, 09 Oct 2014 04:44:11 -0400
Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71)
	(envelope-from <chouteau@adacore.com>) id 1Xc9KM-0004s9-QG
	for qemu-devel@nongnu.org; Thu, 09 Oct 2014 04:44:07 -0400
Received: from mel.act-europe.fr ([194.98.77.210]:50672
	helo=smtp.eu.adacore.com) by eggs.gnu.org with esmtp (Exim 4.71)
	(envelope-from <chouteau@adacore.com>) id 1Xc9KM-0004s1-L0
	for qemu-devel@nongnu.org; Thu, 09 Oct 2014 04:44:02 -0400
Message-ID: <54364AB9.1090500@adacore.com>
Date: Thu, 09 Oct 2014 10:43:37 +0200
From: Fabien Chouteau <chouteau@adacore.com>
MIME-Version: 1.0
References: <1412777966-28286-1-git-send-email-chouteau@adacore.com>
	<54355A7C.8060007@suse.de>
In-Reply-To: <54355A7C.8060007@suse.de>
Content-Type: text/plain; charset=iso-8859-15
Content-Transfer-Encoding: quoted-printable
Subject: Re: [Qemu-devel] [PATCH][SPARC] LEON3: Add emulation of AMBA
	plug&play
List-Id: <qemu-devel.nongnu.org>
List-Unsubscribe: <https://lists.nongnu.org/mailman/options/qemu-devel>,
	<mailto:qemu-devel-request@nongnu.org?subject=unsubscribe>
List-Archive: <http://lists.nongnu.org/archive/html/qemu-devel>
List-Post: <mailto:qemu-devel@nongnu.org>
List-Help: <mailto:qemu-devel-request@nongnu.org?subject=help>
List-Subscribe: <https://lists.nongnu.org/mailman/listinfo/qemu-devel>,
	<mailto:qemu-devel-request@nongnu.org?subject=subscribe>
To: =?ISO-8859-15?Q?Andreas_F=E4rber?= <afaerber@suse.de>, qemu-devel@nongnu.org
Cc: peter.maydell@linaro.org, mark.cave-ayland@ilande.co.uk, Jiri Gaisler <jiri@gaisler.se>

On 10/08/2014 05:38 PM, Andreas F=E4rber wrote:
> Hi,
>=20

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"
>=20
> If you move the two TYPE_* constants to grlib.h, you can reuse them.
>=20

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)
>=20
> Indentation is off by one for all read/write functions.
>=20

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 =3D 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 =3D grlib_apbpnp_init;
>> +}
>> +
>> +static const TypeInfo grlib_apbpnp_info =3D {
>> +    .name          =3D TYPE_GRLIB_APB_PNP,
>> +    .parent        =3D TYPE_SYS_BUS_DEVICE,
>> +    .instance_size =3D sizeof(APBPNP),
>> +    .class_init    =3D grlib_apbpnp_class_init,
>> +};
>> +
>> +static void grlib_apbpnp_register_types(void)
>> +{
>> +    type_register_static(&grlib_apbpnp_info);
>> +}
>> +
>> +type_init(grlib_apbpnp_register_types)
>=20
> Please either split into two .c files here, ...
>=20

>>=20
> ... 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 =3D 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_ */
>=20
> 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.