Re: [Qemu-devel] [PATCH 02/11] pxa2xx: QOM'ify I2C slave

[Andreas Färber]

Am 09.02.2014 02:35, schrieb Peter Crosthwaite:
> On Sat, Feb 1, 2014 at 12:34 AM, Andreas Färber <address@hidden> wrote:
>> Replace usages of FROM_I2C_SLAVE() with QOM cast macro and rename parent
>> field to assure we caught all.
>>
>> Signed-off-by: Andreas Färber <address@hidden>
>> ---
>>  hw/arm/pxa2xx.c | 38 +++++++++++++++++++++++++-------------
>>  1 file changed, 25 insertions(+), 13 deletions(-)
>>
>> diff --git a/hw/arm/pxa2xx.c b/hw/arm/pxa2xx.c
>> index daf60e8..e5f1e10 100644
>> --- a/hw/arm/pxa2xx.c
>> +++ b/hw/arm/pxa2xx.c
>> @@ -1222,8 +1222,14 @@ static const TypeInfo pxa2xx_rtc_sysbus_info = {
>>  };
>>
>>  /* I2C Interface */
>> -typedef struct {
>> -    I2CSlave i2c;
>> +
>> +#define TYPE_PXA2XX_I2C_SLAVE "pxa2xx-i2c-slave"
>> +#define PXA2XX_I2C_SLAVE(obj) \
>> +    OBJECT_CHECK(PXA2xxI2CSlaveState, (obj), TYPE_PXA2XX_I2C_SLAVE)
>> +
>> +typedef struct PXA2xxI2CSlaveState {
>> +    I2CSlave parent_obj;
>> +
>>      PXA2xxI2CState *host;
>>  } PXA2xxI2CSlaveState;
>>
>> @@ -1268,7 +1274,7 @@ static void pxa2xx_i2c_update(PXA2xxI2CState *s)
>>  /* These are only stubs now.  */
>>  static void pxa2xx_i2c_event(I2CSlave *i2c, enum i2c_event event)
>>  {
>> -    PXA2xxI2CSlaveState *slave = FROM_I2C_SLAVE(PXA2xxI2CSlaveState, i2c);
>> +    PXA2xxI2CSlaveState *slave = PXA2XX_I2C_SLAVE(i2c);
>>      PXA2xxI2CState *s = slave->host;
>>
>>      switch (event) {
>> @@ -1292,10 +1298,12 @@ static void pxa2xx_i2c_event(I2CSlave *i2c, enum 
>> i2c_event event)
>>
>>  static int pxa2xx_i2c_rx(I2CSlave *i2c)
>>  {
>> -    PXA2xxI2CSlaveState *slave = FROM_I2C_SLAVE(PXA2xxI2CSlaveState, i2c);
>> +    PXA2xxI2CSlaveState *slave = PXA2XX_I2C_SLAVE(i2c);
>>      PXA2xxI2CState *s = slave->host;
>> -    if ((s->control & (1 << 14)) || !(s->control & (1 << 6)))
>> +
>> +    if ((s->control & (1 << 14)) || !(s->control & (1 << 6))) {
>>          return 0;
>> +    }
> 
> This will look funny when git-blamed. Should out-of-scope trivials be
> separate patch so anyone git-blame will at least see "Fix coding
> style" rather than then misleading "QOM'ify I2C"?.

We've had the policy of fixing Coding Style on lines touched or adjacent
to those touched, to gradually get rid of them - this one was within 3
lines of context. I'm sure PXA code will have many more Coding Style
faults, so placing 2 out of X in their own patch seems silly. Should I
rather drop them?

> With the multiple
> instances of this cleanup it should at least feature in the commit
> message.

You're right that I should've mentioned it. Same for replacing parent
field accesses. Fixed.

Regards,
Andreas

>>
>>      if (s->status & (1 << 0)) {                        /* RWM */
>>          s->status |= 1 << 6;                   /* set ITE */
>> @@ -1307,10 +1315,12 @@ static int pxa2xx_i2c_rx(I2CSlave *i2c)
>>
>>  static int pxa2xx_i2c_tx(I2CSlave *i2c, uint8_t data)
>>  {
>> -    PXA2xxI2CSlaveState *slave = FROM_I2C_SLAVE(PXA2xxI2CSlaveState, i2c);
>> +    PXA2xxI2CSlaveState *slave = PXA2XX_I2C_SLAVE(i2c);
>>      PXA2xxI2CState *s = slave->host;
>> -    if ((s->control & (1 << 14)) || !(s->control & (1 << 6)))
>> +
>> +    if ((s->control & (1 << 14)) || !(s->control & (1 << 6))) {
>>          return 1;
>> +    }
>>
>>      if (!(s->status & (1 << 0))) {             /* RWM */
>>          s->status |= 1 << 7;                   /* set IRF */
>> @@ -1325,6 +1335,7 @@ static uint64_t pxa2xx_i2c_read(void *opaque, hwaddr 
>> addr,
>>                                  unsigned size)
>>  {
>>      PXA2xxI2CState *s = (PXA2xxI2CState *) opaque;
>> +    I2CSlave *slave;
>>
>>      addr -= s->offset;
>>      switch (addr) {
>> @@ -1333,7 +1344,8 @@ static uint64_t pxa2xx_i2c_read(void *opaque, hwaddr 
>> addr,
>>      case ISR:
>>          return s->status | (i2c_bus_busy(s->bus) << 2);
>>      case ISAR:
>> -        return s->slave->i2c.address;
>> +        slave = I2C_SLAVE(s->slave);
>> +        return slave->address;
>>      case IDBR:
>>          return s->data;
>>      case IBMR:
>> @@ -1408,7 +1420,7 @@ static void pxa2xx_i2c_write(void *opaque, hwaddr addr,
>>          break;
>>
>>      case ISAR:
>> -        i2c_set_slave_address(&s->slave->i2c, value & 0x7f);
>> +        i2c_set_slave_address(I2C_SLAVE(s->slave), value & 0x7f);
>>          break;
>>
>>      case IDBR:
>> @@ -1432,7 +1444,7 @@ static const VMStateDescription 
>> vmstate_pxa2xx_i2c_slave = {
>>      .minimum_version_id = 1,
>>      .minimum_version_id_old = 1,
>>      .fields      = (VMStateField []) {
>> -        VMSTATE_I2C_SLAVE(i2c, PXA2xxI2CSlaveState),
>> +        VMSTATE_I2C_SLAVE(parent_obj, PXA2xxI2CSlaveState),
>>          VMSTATE_END_OF_LIST()
>>      }
>>  };
>> @@ -1470,7 +1482,7 @@ static void pxa2xx_i2c_slave_class_init(ObjectClass 
>> *klass, void *data)
>>  }
>>
>>  static const TypeInfo pxa2xx_i2c_slave_info = {
>> -    .name          = "pxa2xx-i2c-slave",
>> +    .name          = TYPE_PXA2XX_I2C_SLAVE,
>>      .parent        = TYPE_I2C_SLAVE,
>>      .instance_size = sizeof(PXA2xxI2CSlaveState),
>>      .class_init    = pxa2xx_i2c_slave_class_init,
>> @@ -1496,8 +1508,8 @@ PXA2xxI2CState *pxa2xx_i2c_init(hwaddr base,
>>      s = PXA2XX_I2C(i2c_dev);
>>      /* FIXME: Should the slave device really be on a separate bus?  */
>>      i2cbus = i2c_init_bus(dev, "dummy");
>> -    dev = i2c_create_slave(i2cbus, "pxa2xx-i2c-slave", 0);
>> -    s->slave = FROM_I2C_SLAVE(PXA2xxI2CSlaveState, I2C_SLAVE(dev));
>> +    dev = i2c_create_slave(i2cbus, TYPE_PXA2XX_I2C_SLAVE, 0);
>> +    s->slave = PXA2XX_I2C_SLAVE(dev);
>>      s->slave->host = s;
>>
>>      return s;
>> --
>> 1.8.4.5
>>
>>


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