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

[Jean-Christophe DUBOIS]

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
> <address@hidden> 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.

> > +    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?

> +
> +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.

> > +};
> > +
> > +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?