Re: [Qemu-devel] [PATCH 1/2] Add i.MX I2C device emulator.

[Peter Crosthwaite <peter.crosthwaite@xilinx.com>]

Hi JC,

On Fri, May 3, 2013 at 4:33 AM, Jean-Christophe DUBOIS
<jcd@tribudubois.net> wrote:
> Peter,
>
> Thanks for you review.
>
> I have a few questions below.
>
> JC
>
>
> On 05/02/2013 02:16 PM, Peter Crosthwaite wrote:
>>
>> Hi Jean-Christophe,
>>
>> Thanks for your contribution. Please run the patch through
>> scripts/checkpatch.pl to check for formatting errors.
>>
>> On Thu, May 2, 2013 at 5:53 AM, Jean-Christophe DUBOIS
>> <jcd@tribudubois.net> wrote:
>> ERROR: return is not a function, parentheses are not required
>> #148: FILE: hw/i2c/imx_i2c.c:114:
>> +    return (s->i2cr & I2CR_IEN);
>>
>> >From checkpatch, here and below.
>
>
> Will do.
>
>
>>> +}
>>> +
>>> +static inline bool imx_i2c_interrupt_is_enabled(imx_i2c_state *s)
>>> +{
>>> +    return (s->i2cr & I2CR_IIEN);
>>> +}
>>> +
>>> +static inline bool imx_i2c_is_master(imx_i2c_state *s)
>>> +{
>>> +    return (s->i2cr & I2CR_MSTA);
>>> +}
>>> +
>>> +static inline bool imx_i2c_direction_is_tx(imx_i2c_state *s)
>>> +{
>>> +    return (s->i2cr & I2CR_MTX);
>>> +}
>>> +
>>> +static void imx_i2c_reset(DeviceState *d)
>>> +{
>>> +    imx_i2c_state *s = FROM_SYSBUS(imx_i2c_state, SYS_BUS_DEVICE(d));
>>> +
>>
>> Please don't use FROM_SYSBUS in new code. Use QOM cast macros.
>>
>> http://wiki.qemu.org/QOMConventions
>>
>> Has useful guidelines for the rules around this for new devices.
>
>
> It is not very clear what you do expect. Could you point me to a driver that
> is up to date.
>

Its actually quite hard to find them but hw/dma/pl330.c was fairly
recently reviewed and accepted sysbus device. There is one QOM
violation in there just with a quick scan for the more common errors,
that is best fixed in new device models. fix below:

--- a/hw/dma/pl330.c
+++ b/hw/dma/pl330.c
@@ -227,7 +227,7 @@ static const VMStateDescription vmstate_pl330_queue = {
 };

 struct PL330State {
-    SysBusDevice busdev;
+    SysBusDevice parent_obj;

Code of interest:

Type cast macro:

#define TYPE_PL330 "pl330"
#define PL330(obj) OBJECT_CHECK(PL330State, (obj), TYPE_PL330)

Realize fn:

static void pl330_realize(DeviceState *dev, Error **errp)
{
    int i;
    PL330State *s = PL330(dev);

(Theres an example usage of QOM cast macro).

and its wiring up:

static void pl330_class_init(ObjectClass *klass, void *data)
{
    DeviceClass *dc = DEVICE_CLASS(klass);

    dc->realize = pl330_realize;
    dc->reset = pl330_reset;
    dc->props = pl330_properties;
    dc->vmsd = &vmstate_pl330;
}

>
>>> +    if (s->address != ADDR_RESET) {
>>> +        i2c_end_transfer(s->bus);
>>> +    }
>>> +
>>> +    s->address    = ADDR_RESET;
>>> +    s->iadr       = IADR_RESET;
>>> +    s->ifdr       = IFDR_RESET;
>>> +    s->i2cr       = I2CR_RESET;
>>> +    s->i2sr       = I2SR_RESET;
>>> +    s->i2dr_read  = I2DR_RESET;
>>> +    s->i2dr_write = I2DR_RESET;
>>> +}
>>> +
>>> +static inline void imx_i2c_raise_interrupt(imx_i2c_state *s)
>>> +{
>>> +    /*
>>> +     * raise an interrupt if the device is enabled and it is configured
>>> +     * to generate some interrupts.
>>> +     */
>>> +    if (imx_i2c_is_enabled(s) && imx_i2c_interrupt_is_enabled(s)) {
>>> +        s->i2sr |= I2SR_IIF;
>>> +        qemu_irq_raise(s->irq);
>>> +    }
>>> +}
>>> +
>>> +static uint64_t imx_i2c_read(void *opaque, hwaddr offset,
>>> +                             unsigned size)
>>> +{
>>> +    imx_i2c_state *s = (imx_i2c_state *)opaque;
>>
>> QOM cast macro.
>
>
> Could you point me to an up to date driver using QOM cast macro?
>

Covered above

>
>>
>> +
>> +static const MemoryRegionOps imx_i2c_ops = {
>> +    .read = imx_i2c_read,
>> +    .write = imx_i2c_write,
>> +    .endianness = DEVICE_NATIVE_ENDIAN,
>> I think you may need a .valid definition here as it looks like you
>> have restrictions on access size and alignment? (looks like 4bytes
>> accesses only).
>
>
> I'll look into this.
>

More example:

static const MemoryRegionOps pl330_ops = {
    .read = pl330_iomem_read,
    .write = pl330_iomem_write,
    .endianness = DEVICE_NATIVE_ENDIAN,
    .impl = {
        .min_access_size = 4,
        .max_access_size = 4,
    }
};

>
>>> +};
>>> +
>>> +static const VMStateDescription imx_i2c_vmstate = {
>>> +    .name = TYPE_IMX_I2C,
>>> +    .version_id = 1,
>>> +    .minimum_version_id = 1,
>>> +    .fields = (VMStateField[]) {
>>> +        VMSTATE_UINT16(address, imx_i2c_state),
>>> +        VMSTATE_UINT16(iadr, imx_i2c_state),
>>> +        VMSTATE_UINT16(ifdr, imx_i2c_state),
>>> +        VMSTATE_UINT16(i2cr, imx_i2c_state),
>>> +        VMSTATE_UINT16(i2sr, imx_i2c_state),
>>> +        VMSTATE_UINT16(i2dr_read, imx_i2c_state),
>>> +        VMSTATE_UINT16(i2dr_write, imx_i2c_state),
>>> +        VMSTATE_END_OF_LIST()
>>> +    }
>>> +};
>>> +
>>> +static int imx_i2c_init(SysBusDevice *dev)
>>> +{
>>
>> Use of the SysBusDeviceClass::init function is deprecated. Please use
>> DeviceClass::realise or Object::init. With no reliance on properties I
>> would suggest this one can be done as just an Object::init fn.
>
> Could you point me to the documentation or an up to date example?
>

Not sure if there's any formal documentation around. But its covered
by the pl330 examples. There are some relevant code comments around
the place (include/hw/qdev-core.h).

 * # Realization #
 * Devices are constructed in two stages,
 * 1) object instantiation via object_initialize() and
 * 2) device realization via #DeviceState:realized property.
 * The former may not fail (it might assert or exit), the latter may return
 * error information to the caller and must be re-entrant.
 * Trivial field initializations should go into #TypeInfo.instance_init.
 * Operations depending on @props static properties should go into @realize.
 * After successful realization, setting static properties will fail.

Regards,
Peter