Re: [Qemu-devel] [PATCH v2 2/4] Add i.MX I2C controller driver.

["Andreas Färber" <afaerber@suse.de>]

Am 05.05.2013 05:14, schrieb Peter Crosthwaite:
> On Sun, May 5, 2013 at 12:09 AM, Jean-Christophe DUBOIS
> <jcd@tribudubois.net> wrote:
>> diff --git a/hw/i2c/imx_i2c.c b/hw/i2c/imx_i2c.c
>> new file mode 100644
>> index 0000000..5b0d046
>> --- /dev/null
>> +++ b/hw/i2c/imx_i2c.c
[...]
>> +typedef struct imx_i2c_state {
> 
> types should be in CamelCase IMXI2CState
> 
>> +    SysBusDevice parent_obj;

While at it, please add a white line here. Background is that this
parent field will pretty likely go away once we switch to a better
object-orientation framework - if it were in a header we would annotate
it as private and thus hidden from documentation.

>> +    MemoryRegion iomem;
>> +    i2c_bus *bus;

Please rather use i2c_bus bus; and qbus_create_inline() in instance_init.

>> +    qemu_irq irq;
>> +
>> +    uint16_t  address;
>> +
>> +    uint16_t iadr;
>> +    uint16_t ifdr;
>> +    uint16_t i2cr;
>> +    uint16_t i2sr;
>> +    uint16_t i2dr_read;
>> +    uint16_t i2dr_write;
>> +} imx_i2c_state;
[...]
>> +static uint64_t imx_i2c_read(void *opaque, hwaddr offset,
>> +                             unsigned size)
>> +{
>> +    uint16_t value;
>> +    imx_i2c_state *s = IMX_I2C(opaque);
>> +
>> +    switch (offset) {
>> +    case IADR_ADDR:
>> +        value = s->iadr;
>> +        break;
>> +    case IFDR_ADDR:
>> +        value = s->ifdr;
>> +        break;
>> +    case I2CR_ADDR:
>> +        value = s->i2cr;
>> +        break;
>> +    case I2SR_ADDR:
>> +        value = s->i2sr;
>> +        break;
>> +    case I2DR_ADDR:
>> +        value = s->i2dr_read;
>> +
>> +        if (imx_i2c_is_master(s)) { /* master mode */
>> +            int ret = 0xff;
>> +
>> +            if (s->address == ADDR_RESET) {
>> +                /* something is wrong as the address is not set */
>> +                DPRINT("Trying to read without specifying the slave address\n");
>> +            } else if (s->i2cr & I2CR_MTX) {
>> +                DPRINT("Trying to read but MTX is set\n");
>> +            } else {
>> +                /* get the next byte */
>> +                ret = i2c_recv(s->bus);
>> +
>> +                if (ret >= 0) {
>> +                    imx_i2c_raise_interrupt(s);
>> +                } else {
>> +                    DPRINT("read failed for device 0x%02x\n" s->address);
>> +                    ret = 0xff;
>> +                }
>> +            }
>> +
>> +            s->i2dr_read = ret;
>> +        }
>> +        break;
>> +    default:
>> +        hw_error("%s: Bad address 0x%x\n", __func__, (int)offset);
>> +        break;
>> +    }
>> +
>> +    DPRINT("read %s [0x%02x] -> 0x%02x\n", imx_i2c_get_regname(offset)
> 
> General question for the list, are we encouraging the use or PRIx in
> new code to remove the needs for casts from uint_XXt to unsigned in
> printfery?
> ,
>> +           (unsigned int)offset, value);

I believe so, I've been asked to use HWADDR_PRIx macro in ppc patches.

>> +
>> +    return (uint64_t)value;
>> +}
[snip]

Cheers,
Andreas

-- 
SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany
GF: Jeff Hawn, Jennifer Guild, Felix Imendörffer; HRB 16746 AG Nürnberg