Re: [Qemu-devel] [PATCH v13 19/19] i.MX: Adding i2C devices to i.MX31 SOC

[Peter Maydell]

On 16 July 2015 at 22:21, Jean-Christophe Dubois <address@hidden> wrote:
> Signed-off-by: Jean-Christophe Dubois <address@hidden>

The usual commit message verb form is "Add foo", not "Adding foo".
Empty commit message bodies are also generally worth avoiding.

> @@ -154,6 +159,31 @@ static void fsl_imx31_realize(DeviceState *dev, Error 
> **errp)
>                                              epit_table[i].irq));
>      }
>
> +    /* Initialize all I2C */
> +    for (i = 0; i < FSL_IMX31_NUM_I2CS; i++) {
> +        static const struct {
> +            hwaddr addr;
> +            unsigned int irq;
> +        } i2c_table[FSL_IMX31_NUM_I2CS] = {
> +            { FSL_IMX31_I2C1_ADDR, FSL_IMX31_I2C1_IRQ  },
> +            { FSL_IMX31_I2C2_ADDR, FSL_IMX31_I2C2_IRQ  },
> +            { FSL_IMX31_I2C3_ADDR, FSL_IMX31_I2C3_IRQ }

Inconsistent spacing.

> +        };
> +
> +        /* Initialize the I2C */
> +        object_property_set_bool(OBJECT(&s->i2c[i]), true, "realized", &err);
> +        if (err) {
> +            error_propagate((errp), (err));

Unnecessary brackets.

> +            return;
> +        }
> +        /* Map I2C memory */
> +        sysbus_mmio_map(SYS_BUS_DEVICE(&s->i2c[i]), 0, i2c_table[i].addr);
> +        /* Connet I2C IRQ to PIC */

"Connect".

> +        sysbus_connect_irq(SYS_BUS_DEVICE(&s->i2c[i]), 0,
> +                           qdev_get_gpio_in(DEVICE(&s->avic),
> +                                            i2c_table[i].irq));
> +    }

Looks OK otherwise, though I haven't checked against hw specs
and don't have any test images for this SoC.

thanks
-- PMM