diff mbox series

[RFC,v2,4/6] hw/i2c: add asynchronous send

Message ID 20220601210831.67259-5-its@irrelevant.dk
State New
Headers show
Series hw/i2c: i2c slave mode support | expand

Commit Message

Klaus Jensen June 1, 2022, 9:08 p.m. UTC
From: Klaus Jensen <k.jensen@samsung.com>

Add an asynchronous version of i2c_send() that requires the slave to
explicitly acknowledge on the bus with i2c_ack().

The current master must use the new i2c_start_send_async() to indicate
that it wants to do an asynchronous transfer. This allows the i2c core
to check if the target slave supports this or not. This approach relies
on adding a new enum i2c_event member, which is why a bunch of other
devices needs changes in their event handling switches.

Signed-off-by: Klaus Jensen <k.jensen@samsung.com>
---
 hw/arm/pxa2xx.c            |  2 ++
 hw/display/sii9022.c       |  2 ++
 hw/display/ssd0303.c       |  2 ++
 hw/i2c/core.c              | 36 +++++++++++++++++++++++++++++++++++-
 hw/i2c/smbus_slave.c       |  4 ++++
 hw/i2c/trace-events        |  2 ++
 hw/misc/ibm-cffps.c        |  2 ++
 hw/misc/ir35221.c          |  2 ++
 hw/nvram/eeprom_at24c.c    |  2 ++
 hw/sensor/lsm303dlhc_mag.c |  2 ++
 include/hw/i2c/i2c.h       | 16 ++++++++++++++++
 11 files changed, 71 insertions(+), 1 deletion(-)

Comments

Corey Minyard June 1, 2022, 10:05 p.m. UTC | #1
On Wed, Jun 01, 2022 at 11:08:29PM +0200, Klaus Jensen wrote:
> From: Klaus Jensen <k.jensen@samsung.com>
> 
> Add an asynchronous version of i2c_send() that requires the slave to
> explicitly acknowledge on the bus with i2c_ack().
> 
> The current master must use the new i2c_start_send_async() to indicate
> that it wants to do an asynchronous transfer. This allows the i2c core
> to check if the target slave supports this or not. This approach relies
> on adding a new enum i2c_event member, which is why a bunch of other
> devices needs changes in their event handling switches.

This would be easier to read if you split out the default return of -1
in all the devices to a separate patch.

You've already pointed out the lack of nack support.

I think this is ok outside of that.

-corey

> 
> Signed-off-by: Klaus Jensen <k.jensen@samsung.com>
> ---
>  hw/arm/pxa2xx.c            |  2 ++
>  hw/display/sii9022.c       |  2 ++
>  hw/display/ssd0303.c       |  2 ++
>  hw/i2c/core.c              | 36 +++++++++++++++++++++++++++++++++++-
>  hw/i2c/smbus_slave.c       |  4 ++++
>  hw/i2c/trace-events        |  2 ++
>  hw/misc/ibm-cffps.c        |  2 ++
>  hw/misc/ir35221.c          |  2 ++
>  hw/nvram/eeprom_at24c.c    |  2 ++
>  hw/sensor/lsm303dlhc_mag.c |  2 ++
>  include/hw/i2c/i2c.h       | 16 ++++++++++++++++
>  11 files changed, 71 insertions(+), 1 deletion(-)
> 
> diff --git a/hw/arm/pxa2xx.c b/hw/arm/pxa2xx.c
> index f4f687df68ef..93dda83d7aa9 100644
> --- a/hw/arm/pxa2xx.c
> +++ b/hw/arm/pxa2xx.c
> @@ -1305,6 +1305,8 @@ static int pxa2xx_i2c_event(I2CSlave *i2c, enum i2c_event event)
>      case I2C_NACK:
>          s->status |= 1 << 1;				/* set ACKNAK */
>          break;
> +    default:
> +        return -1;
>      }
>      pxa2xx_i2c_update(s);
>  
> diff --git a/hw/display/sii9022.c b/hw/display/sii9022.c
> index b591a5878901..664fd4046d82 100644
> --- a/hw/display/sii9022.c
> +++ b/hw/display/sii9022.c
> @@ -76,6 +76,8 @@ static int sii9022_event(I2CSlave *i2c, enum i2c_event event)
>          break;
>      case I2C_NACK:
>          break;
> +    default:
> +        return -1;
>      }
>  
>      return 0;
> diff --git a/hw/display/ssd0303.c b/hw/display/ssd0303.c
> index aeae22da9c29..d67b0ad7b529 100644
> --- a/hw/display/ssd0303.c
> +++ b/hw/display/ssd0303.c
> @@ -196,6 +196,8 @@ static int ssd0303_event(I2CSlave *i2c, enum i2c_event event)
>      case I2C_NACK:
>          /* Nothing to do.  */
>          break;
> +    default:
> +        return -1;
>      }
>  
>      return 0;
> diff --git a/hw/i2c/core.c b/hw/i2c/core.c
> index 145dce60782a..d4ba8146bffb 100644
> --- a/hw/i2c/core.c
> +++ b/hw/i2c/core.c
> @@ -161,7 +161,8 @@ static int i2c_do_start_transfer(I2CBus *bus, uint8_t address,
>             start condition.  */
>  
>          if (sc->event) {
> -            trace_i2c_event("start", s->address);
> +            trace_i2c_event(event == I2C_START_SEND ? "start" : "start_async",
> +                            s->address);
>              rv = sc->event(s, event);
>              if (rv && !bus->broadcast) {
>                  if (bus_scanned) {
> @@ -212,6 +213,11 @@ int i2c_start_send(I2CBus *bus, uint8_t address)
>      return i2c_do_start_transfer(bus, address, I2C_START_SEND);
>  }
>  
> +int i2c_start_send_async(I2CBus *bus, uint8_t address)
> +{
> +    return i2c_do_start_transfer(bus, address, I2C_START_SEND_ASYNC);
> +}
> +
>  void i2c_end_transfer(I2CBus *bus)
>  {
>      I2CSlaveClass *sc;
> @@ -261,6 +267,23 @@ int i2c_send(I2CBus *bus, uint8_t data)
>      return ret ? -1 : 0;
>  }
>  
> +int i2c_send_async(I2CBus *bus, uint8_t data)
> +{
> +    I2CNode *node = QLIST_FIRST(&bus->current_devs);
> +    I2CSlave *slave = node->elt;
> +    I2CSlaveClass *sc = I2C_SLAVE_GET_CLASS(slave);
> +
> +    if (!sc->send_async) {
> +        return -1;
> +    }
> +
> +    trace_i2c_send_async(slave->address, data);
> +
> +    sc->send_async(slave, data);
> +
> +    return 0;
> +}
> +
>  uint8_t i2c_recv(I2CBus *bus)
>  {
>      uint8_t data = 0xff;
> @@ -297,6 +320,17 @@ void i2c_nack(I2CBus *bus)
>      }
>  }
>  
> +void i2c_ack(I2CBus *bus)
> +{
> +    if (!bus->bh) {
> +        return;
> +    }
> +
> +    trace_i2c_ack();
> +
> +    qemu_bh_schedule(bus->bh);
> +}
> +
>  static int i2c_slave_post_load(void *opaque, int version_id)
>  {
>      I2CSlave *dev = opaque;
> diff --git a/hw/i2c/smbus_slave.c b/hw/i2c/smbus_slave.c
> index 5d10e27664db..feb3ec633350 100644
> --- a/hw/i2c/smbus_slave.c
> +++ b/hw/i2c/smbus_slave.c
> @@ -143,6 +143,10 @@ static int smbus_i2c_event(I2CSlave *s, enum i2c_event event)
>              dev->mode = SMBUS_CONFUSED;
>              break;
>          }
> +        break;
> +
> +    default:
> +        return -1;
>      }
>  
>      return 0;
> diff --git a/hw/i2c/trace-events b/hw/i2c/trace-events
> index 209275ed2dc8..af181d43ee64 100644
> --- a/hw/i2c/trace-events
> +++ b/hw/i2c/trace-events
> @@ -4,7 +4,9 @@
>  
>  i2c_event(const char *event, uint8_t address) "%s(addr:0x%02x)"
>  i2c_send(uint8_t address, uint8_t data) "send(addr:0x%02x) data:0x%02x"
> +i2c_send_async(uint8_t address, uint8_t data) "send_async(addr:0x%02x) data:0x%02x"
>  i2c_recv(uint8_t address, uint8_t data) "recv(addr:0x%02x) data:0x%02x"
> +i2c_ack(void) ""
>  
>  # aspeed_i2c.c
>  
> diff --git a/hw/misc/ibm-cffps.c b/hw/misc/ibm-cffps.c
> index 042155bb7838..d69a727fd5f9 100644
> --- a/hw/misc/ibm-cffps.c
> +++ b/hw/misc/ibm-cffps.c
> @@ -152,6 +152,8 @@ static int ibm_cffps_event(I2CSlave *i2c, enum i2c_event event)
>      case I2C_FINISH:
>           s->pointer = 0xFF;
>          break;
> +    default:
> +        return -1;
>      }
>  
>      s->len = 0;
> diff --git a/hw/misc/ir35221.c b/hw/misc/ir35221.c
> index 5e01d3655059..c46b9ee1c3bf 100644
> --- a/hw/misc/ir35221.c
> +++ b/hw/misc/ir35221.c
> @@ -117,6 +117,8 @@ static int ir35221_event(I2CSlave *i2c, enum i2c_event event)
>      case I2C_FINISH:
>           s->pointer = 0xFF;
>          break;
> +    default:
> +        return -1;
>      }
>  
>      s->len = 0;
> diff --git a/hw/nvram/eeprom_at24c.c b/hw/nvram/eeprom_at24c.c
> index 01a3093600fa..d695f6ae894a 100644
> --- a/hw/nvram/eeprom_at24c.c
> +++ b/hw/nvram/eeprom_at24c.c
> @@ -75,6 +75,8 @@ int at24c_eeprom_event(I2CSlave *s, enum i2c_event event)
>          break;
>      case I2C_NACK:
>          break;
> +    default:
> +        return -1;
>      }
>      return 0;
>  }
> diff --git a/hw/sensor/lsm303dlhc_mag.c b/hw/sensor/lsm303dlhc_mag.c
> index 4c98ddbf207c..bb8d48b2fdb0 100644
> --- a/hw/sensor/lsm303dlhc_mag.c
> +++ b/hw/sensor/lsm303dlhc_mag.c
> @@ -427,6 +427,8 @@ static int lsm303dlhc_mag_event(I2CSlave *i2c, enum i2c_event event)
>          break;
>      case I2C_NACK:
>          break;
> +    default:
> +        return -1;
>      }
>  
>      s->len = 0;
> diff --git a/include/hw/i2c/i2c.h b/include/hw/i2c/i2c.h
> index be8bb8b78a60..9b9581d23097 100644
> --- a/include/hw/i2c/i2c.h
> +++ b/include/hw/i2c/i2c.h
> @@ -12,6 +12,7 @@
>  enum i2c_event {
>      I2C_START_RECV,
>      I2C_START_SEND,
> +    I2C_START_SEND_ASYNC,
>      I2C_FINISH,
>      I2C_NACK /* Masker NACKed a receive byte.  */
>  };
> @@ -28,6 +29,9 @@ struct I2CSlaveClass {
>      /* Master to slave. Returns non-zero for a NAK, 0 for success. */
>      int (*send)(I2CSlave *s, uint8_t data);
>  
> +    /* Master to slave (asynchronous). Receiving slave must call i2c_ack(). */
> +    void (*send_async)(I2CSlave *s, uint8_t data);
> +
>      /*
>       * Slave to master.  This cannot fail, the device should always
>       * return something here.
> @@ -127,11 +131,23 @@ int i2c_start_recv(I2CBus *bus, uint8_t address);
>   */
>  int i2c_start_send(I2CBus *bus, uint8_t address);
>  
> +/**
> + * i2c_start_send_async: start an asynchronous 'send' transfer on an I2C bus.
> + *
> + * @bus: #I2CBus to be used
> + * @address: address of the slave
> + *
> + * Return: 0 on success, -1 on error
> + */
> +int i2c_start_send_async(I2CBus *bus, uint8_t address);
> +
>  void i2c_end_transfer(I2CBus *bus);
>  void i2c_nack(I2CBus *bus);
> +void i2c_ack(I2CBus *bus);
>  void i2c_bus_master(I2CBus *bus, QEMUBH *bh);
>  void i2c_bus_release(I2CBus *bus);
>  int i2c_send(I2CBus *bus, uint8_t data);
> +int i2c_send_async(I2CBus *bus, uint8_t data);
>  uint8_t i2c_recv(I2CBus *bus);
>  bool i2c_scan_bus(I2CBus *bus, uint8_t address, bool broadcast,
>                    I2CNodeList *current_devs);
> -- 
> 2.36.1
>
Cédric Le Goater June 2, 2022, 7:32 a.m. UTC | #2
On 6/2/22 00:05, Corey Minyard wrote:
> On Wed, Jun 01, 2022 at 11:08:29PM +0200, Klaus Jensen wrote:
>> From: Klaus Jensen <k.jensen@samsung.com>
>>
>> Add an asynchronous version of i2c_send() that requires the slave to
>> explicitly acknowledge on the bus with i2c_ack().
>>
>> The current master must use the new i2c_start_send_async() to indicate
>> that it wants to do an asynchronous transfer. This allows the i2c core
>> to check if the target slave supports this or not. This approach relies
>> on adding a new enum i2c_event member, which is why a bunch of other
>> devices needs changes in their event handling switches.
> 
> This would be easier to read if you split out the default return of -1
> in all the devices to a separate patch.

yes and please drop ibm-cffps.c and ir35221.c which are not in mainline.
I will address them myself.

Thanks,

C.


> 
> You've already pointed out the lack of nack support.
> 
> I think this is ok outside of that.
> 
> -corey
> 
>>
>> Signed-off-by: Klaus Jensen <k.jensen@samsung.com>
>> ---
>>   hw/arm/pxa2xx.c            |  2 ++
>>   hw/display/sii9022.c       |  2 ++
>>   hw/display/ssd0303.c       |  2 ++
>>   hw/i2c/core.c              | 36 +++++++++++++++++++++++++++++++++++-
>>   hw/i2c/smbus_slave.c       |  4 ++++
>>   hw/i2c/trace-events        |  2 ++
>>   hw/misc/ibm-cffps.c        |  2 ++
>>   hw/misc/ir35221.c          |  2 ++
>>   hw/nvram/eeprom_at24c.c    |  2 ++
>>   hw/sensor/lsm303dlhc_mag.c |  2 ++
>>   include/hw/i2c/i2c.h       | 16 ++++++++++++++++
>>   11 files changed, 71 insertions(+), 1 deletion(-)
>>
>> diff --git a/hw/arm/pxa2xx.c b/hw/arm/pxa2xx.c
>> index f4f687df68ef..93dda83d7aa9 100644
>> --- a/hw/arm/pxa2xx.c
>> +++ b/hw/arm/pxa2xx.c
>> @@ -1305,6 +1305,8 @@ static int pxa2xx_i2c_event(I2CSlave *i2c, enum i2c_event event)
>>       case I2C_NACK:
>>           s->status |= 1 << 1;				/* set ACKNAK */
>>           break;
>> +    default:
>> +        return -1;
>>       }
>>       pxa2xx_i2c_update(s);
>>   
>> diff --git a/hw/display/sii9022.c b/hw/display/sii9022.c
>> index b591a5878901..664fd4046d82 100644
>> --- a/hw/display/sii9022.c
>> +++ b/hw/display/sii9022.c
>> @@ -76,6 +76,8 @@ static int sii9022_event(I2CSlave *i2c, enum i2c_event event)
>>           break;
>>       case I2C_NACK:
>>           break;
>> +    default:
>> +        return -1;
>>       }
>>   
>>       return 0;
>> diff --git a/hw/display/ssd0303.c b/hw/display/ssd0303.c
>> index aeae22da9c29..d67b0ad7b529 100644
>> --- a/hw/display/ssd0303.c
>> +++ b/hw/display/ssd0303.c
>> @@ -196,6 +196,8 @@ static int ssd0303_event(I2CSlave *i2c, enum i2c_event event)
>>       case I2C_NACK:
>>           /* Nothing to do.  */
>>           break;
>> +    default:
>> +        return -1;
>>       }
>>   
>>       return 0;
>> diff --git a/hw/i2c/core.c b/hw/i2c/core.c
>> index 145dce60782a..d4ba8146bffb 100644
>> --- a/hw/i2c/core.c
>> +++ b/hw/i2c/core.c
>> @@ -161,7 +161,8 @@ static int i2c_do_start_transfer(I2CBus *bus, uint8_t address,
>>              start condition.  */
>>   
>>           if (sc->event) {
>> -            trace_i2c_event("start", s->address);
>> +            trace_i2c_event(event == I2C_START_SEND ? "start" : "start_async",
>> +                            s->address);
>>               rv = sc->event(s, event);
>>               if (rv && !bus->broadcast) {
>>                   if (bus_scanned) {
>> @@ -212,6 +213,11 @@ int i2c_start_send(I2CBus *bus, uint8_t address)
>>       return i2c_do_start_transfer(bus, address, I2C_START_SEND);
>>   }
>>   
>> +int i2c_start_send_async(I2CBus *bus, uint8_t address)
>> +{
>> +    return i2c_do_start_transfer(bus, address, I2C_START_SEND_ASYNC);
>> +}
>> +
>>   void i2c_end_transfer(I2CBus *bus)
>>   {
>>       I2CSlaveClass *sc;
>> @@ -261,6 +267,23 @@ int i2c_send(I2CBus *bus, uint8_t data)
>>       return ret ? -1 : 0;
>>   }
>>   
>> +int i2c_send_async(I2CBus *bus, uint8_t data)
>> +{
>> +    I2CNode *node = QLIST_FIRST(&bus->current_devs);
>> +    I2CSlave *slave = node->elt;
>> +    I2CSlaveClass *sc = I2C_SLAVE_GET_CLASS(slave);
>> +
>> +    if (!sc->send_async) {
>> +        return -1;
>> +    }
>> +
>> +    trace_i2c_send_async(slave->address, data);
>> +
>> +    sc->send_async(slave, data);
>> +
>> +    return 0;
>> +}
>> +
>>   uint8_t i2c_recv(I2CBus *bus)
>>   {
>>       uint8_t data = 0xff;
>> @@ -297,6 +320,17 @@ void i2c_nack(I2CBus *bus)
>>       }
>>   }
>>   
>> +void i2c_ack(I2CBus *bus)
>> +{
>> +    if (!bus->bh) {
>> +        return;
>> +    }
>> +
>> +    trace_i2c_ack();
>> +
>> +    qemu_bh_schedule(bus->bh);
>> +}
>> +
>>   static int i2c_slave_post_load(void *opaque, int version_id)
>>   {
>>       I2CSlave *dev = opaque;
>> diff --git a/hw/i2c/smbus_slave.c b/hw/i2c/smbus_slave.c
>> index 5d10e27664db..feb3ec633350 100644
>> --- a/hw/i2c/smbus_slave.c
>> +++ b/hw/i2c/smbus_slave.c
>> @@ -143,6 +143,10 @@ static int smbus_i2c_event(I2CSlave *s, enum i2c_event event)
>>               dev->mode = SMBUS_CONFUSED;
>>               break;
>>           }
>> +        break;
>> +
>> +    default:
>> +        return -1;
>>       }
>>   
>>       return 0;
>> diff --git a/hw/i2c/trace-events b/hw/i2c/trace-events
>> index 209275ed2dc8..af181d43ee64 100644
>> --- a/hw/i2c/trace-events
>> +++ b/hw/i2c/trace-events
>> @@ -4,7 +4,9 @@
>>   
>>   i2c_event(const char *event, uint8_t address) "%s(addr:0x%02x)"
>>   i2c_send(uint8_t address, uint8_t data) "send(addr:0x%02x) data:0x%02x"
>> +i2c_send_async(uint8_t address, uint8_t data) "send_async(addr:0x%02x) data:0x%02x"
>>   i2c_recv(uint8_t address, uint8_t data) "recv(addr:0x%02x) data:0x%02x"
>> +i2c_ack(void) ""
>>   
>>   # aspeed_i2c.c
>>   
>> diff --git a/hw/misc/ibm-cffps.c b/hw/misc/ibm-cffps.c
>> index 042155bb7838..d69a727fd5f9 100644
>> --- a/hw/misc/ibm-cffps.c
>> +++ b/hw/misc/ibm-cffps.c
>> @@ -152,6 +152,8 @@ static int ibm_cffps_event(I2CSlave *i2c, enum i2c_event event)
>>       case I2C_FINISH:
>>            s->pointer = 0xFF;
>>           break;
>> +    default:
>> +        return -1;
>>       }
>>   
>>       s->len = 0;
>> diff --git a/hw/misc/ir35221.c b/hw/misc/ir35221.c
>> index 5e01d3655059..c46b9ee1c3bf 100644
>> --- a/hw/misc/ir35221.c
>> +++ b/hw/misc/ir35221.c
>> @@ -117,6 +117,8 @@ static int ir35221_event(I2CSlave *i2c, enum i2c_event event)
>>       case I2C_FINISH:
>>            s->pointer = 0xFF;
>>           break;
>> +    default:
>> +        return -1;
>>       }
>>   
>>       s->len = 0;
>> diff --git a/hw/nvram/eeprom_at24c.c b/hw/nvram/eeprom_at24c.c
>> index 01a3093600fa..d695f6ae894a 100644
>> --- a/hw/nvram/eeprom_at24c.c
>> +++ b/hw/nvram/eeprom_at24c.c
>> @@ -75,6 +75,8 @@ int at24c_eeprom_event(I2CSlave *s, enum i2c_event event)
>>           break;
>>       case I2C_NACK:
>>           break;
>> +    default:
>> +        return -1;
>>       }
>>       return 0;
>>   }
>> diff --git a/hw/sensor/lsm303dlhc_mag.c b/hw/sensor/lsm303dlhc_mag.c
>> index 4c98ddbf207c..bb8d48b2fdb0 100644
>> --- a/hw/sensor/lsm303dlhc_mag.c
>> +++ b/hw/sensor/lsm303dlhc_mag.c
>> @@ -427,6 +427,8 @@ static int lsm303dlhc_mag_event(I2CSlave *i2c, enum i2c_event event)
>>           break;
>>       case I2C_NACK:
>>           break;
>> +    default:
>> +        return -1;
>>       }
>>   
>>       s->len = 0;
>> diff --git a/include/hw/i2c/i2c.h b/include/hw/i2c/i2c.h
>> index be8bb8b78a60..9b9581d23097 100644
>> --- a/include/hw/i2c/i2c.h
>> +++ b/include/hw/i2c/i2c.h
>> @@ -12,6 +12,7 @@
>>   enum i2c_event {
>>       I2C_START_RECV,
>>       I2C_START_SEND,
>> +    I2C_START_SEND_ASYNC,
>>       I2C_FINISH,
>>       I2C_NACK /* Masker NACKed a receive byte.  */
>>   };
>> @@ -28,6 +29,9 @@ struct I2CSlaveClass {
>>       /* Master to slave. Returns non-zero for a NAK, 0 for success. */
>>       int (*send)(I2CSlave *s, uint8_t data);
>>   
>> +    /* Master to slave (asynchronous). Receiving slave must call i2c_ack(). */
>> +    void (*send_async)(I2CSlave *s, uint8_t data);
>> +
>>       /*
>>        * Slave to master.  This cannot fail, the device should always
>>        * return something here.
>> @@ -127,11 +131,23 @@ int i2c_start_recv(I2CBus *bus, uint8_t address);
>>    */
>>   int i2c_start_send(I2CBus *bus, uint8_t address);
>>   
>> +/**
>> + * i2c_start_send_async: start an asynchronous 'send' transfer on an I2C bus.
>> + *
>> + * @bus: #I2CBus to be used
>> + * @address: address of the slave
>> + *
>> + * Return: 0 on success, -1 on error
>> + */
>> +int i2c_start_send_async(I2CBus *bus, uint8_t address);
>> +
>>   void i2c_end_transfer(I2CBus *bus);
>>   void i2c_nack(I2CBus *bus);
>> +void i2c_ack(I2CBus *bus);
>>   void i2c_bus_master(I2CBus *bus, QEMUBH *bh);
>>   void i2c_bus_release(I2CBus *bus);
>>   int i2c_send(I2CBus *bus, uint8_t data);
>> +int i2c_send_async(I2CBus *bus, uint8_t data);
>>   uint8_t i2c_recv(I2CBus *bus);
>>   bool i2c_scan_bus(I2CBus *bus, uint8_t address, bool broadcast,
>>                     I2CNodeList *current_devs);
>> -- 
>> 2.36.1
>>
Klaus Jensen June 2, 2022, 7:35 a.m. UTC | #3
On Jun  2 09:32, Cédric Le Goater wrote:
> On 6/2/22 00:05, Corey Minyard wrote:
> > On Wed, Jun 01, 2022 at 11:08:29PM +0200, Klaus Jensen wrote:
> > > From: Klaus Jensen <k.jensen@samsung.com>
> > > 
> > > Add an asynchronous version of i2c_send() that requires the slave to
> > > explicitly acknowledge on the bus with i2c_ack().
> > > 
> > > The current master must use the new i2c_start_send_async() to indicate
> > > that it wants to do an asynchronous transfer. This allows the i2c core
> > > to check if the target slave supports this or not. This approach relies
> > > on adding a new enum i2c_event member, which is why a bunch of other
> > > devices needs changes in their event handling switches.
> > 
> > This would be easier to read if you split out the default return of -1
> > in all the devices to a separate patch.
> 
> yes and please drop ibm-cffps.c and ir35221.c which are not in mainline.
> I will address them myself.
> 

Roger.
diff mbox series

Patch

diff --git a/hw/arm/pxa2xx.c b/hw/arm/pxa2xx.c
index f4f687df68ef..93dda83d7aa9 100644
--- a/hw/arm/pxa2xx.c
+++ b/hw/arm/pxa2xx.c
@@ -1305,6 +1305,8 @@  static int pxa2xx_i2c_event(I2CSlave *i2c, enum i2c_event event)
     case I2C_NACK:
         s->status |= 1 << 1;				/* set ACKNAK */
         break;
+    default:
+        return -1;
     }
     pxa2xx_i2c_update(s);
 
diff --git a/hw/display/sii9022.c b/hw/display/sii9022.c
index b591a5878901..664fd4046d82 100644
--- a/hw/display/sii9022.c
+++ b/hw/display/sii9022.c
@@ -76,6 +76,8 @@  static int sii9022_event(I2CSlave *i2c, enum i2c_event event)
         break;
     case I2C_NACK:
         break;
+    default:
+        return -1;
     }
 
     return 0;
diff --git a/hw/display/ssd0303.c b/hw/display/ssd0303.c
index aeae22da9c29..d67b0ad7b529 100644
--- a/hw/display/ssd0303.c
+++ b/hw/display/ssd0303.c
@@ -196,6 +196,8 @@  static int ssd0303_event(I2CSlave *i2c, enum i2c_event event)
     case I2C_NACK:
         /* Nothing to do.  */
         break;
+    default:
+        return -1;
     }
 
     return 0;
diff --git a/hw/i2c/core.c b/hw/i2c/core.c
index 145dce60782a..d4ba8146bffb 100644
--- a/hw/i2c/core.c
+++ b/hw/i2c/core.c
@@ -161,7 +161,8 @@  static int i2c_do_start_transfer(I2CBus *bus, uint8_t address,
            start condition.  */
 
         if (sc->event) {
-            trace_i2c_event("start", s->address);
+            trace_i2c_event(event == I2C_START_SEND ? "start" : "start_async",
+                            s->address);
             rv = sc->event(s, event);
             if (rv && !bus->broadcast) {
                 if (bus_scanned) {
@@ -212,6 +213,11 @@  int i2c_start_send(I2CBus *bus, uint8_t address)
     return i2c_do_start_transfer(bus, address, I2C_START_SEND);
 }
 
+int i2c_start_send_async(I2CBus *bus, uint8_t address)
+{
+    return i2c_do_start_transfer(bus, address, I2C_START_SEND_ASYNC);
+}
+
 void i2c_end_transfer(I2CBus *bus)
 {
     I2CSlaveClass *sc;
@@ -261,6 +267,23 @@  int i2c_send(I2CBus *bus, uint8_t data)
     return ret ? -1 : 0;
 }
 
+int i2c_send_async(I2CBus *bus, uint8_t data)
+{
+    I2CNode *node = QLIST_FIRST(&bus->current_devs);
+    I2CSlave *slave = node->elt;
+    I2CSlaveClass *sc = I2C_SLAVE_GET_CLASS(slave);
+
+    if (!sc->send_async) {
+        return -1;
+    }
+
+    trace_i2c_send_async(slave->address, data);
+
+    sc->send_async(slave, data);
+
+    return 0;
+}
+
 uint8_t i2c_recv(I2CBus *bus)
 {
     uint8_t data = 0xff;
@@ -297,6 +320,17 @@  void i2c_nack(I2CBus *bus)
     }
 }
 
+void i2c_ack(I2CBus *bus)
+{
+    if (!bus->bh) {
+        return;
+    }
+
+    trace_i2c_ack();
+
+    qemu_bh_schedule(bus->bh);
+}
+
 static int i2c_slave_post_load(void *opaque, int version_id)
 {
     I2CSlave *dev = opaque;
diff --git a/hw/i2c/smbus_slave.c b/hw/i2c/smbus_slave.c
index 5d10e27664db..feb3ec633350 100644
--- a/hw/i2c/smbus_slave.c
+++ b/hw/i2c/smbus_slave.c
@@ -143,6 +143,10 @@  static int smbus_i2c_event(I2CSlave *s, enum i2c_event event)
             dev->mode = SMBUS_CONFUSED;
             break;
         }
+        break;
+
+    default:
+        return -1;
     }
 
     return 0;
diff --git a/hw/i2c/trace-events b/hw/i2c/trace-events
index 209275ed2dc8..af181d43ee64 100644
--- a/hw/i2c/trace-events
+++ b/hw/i2c/trace-events
@@ -4,7 +4,9 @@ 
 
 i2c_event(const char *event, uint8_t address) "%s(addr:0x%02x)"
 i2c_send(uint8_t address, uint8_t data) "send(addr:0x%02x) data:0x%02x"
+i2c_send_async(uint8_t address, uint8_t data) "send_async(addr:0x%02x) data:0x%02x"
 i2c_recv(uint8_t address, uint8_t data) "recv(addr:0x%02x) data:0x%02x"
+i2c_ack(void) ""
 
 # aspeed_i2c.c
 
diff --git a/hw/misc/ibm-cffps.c b/hw/misc/ibm-cffps.c
index 042155bb7838..d69a727fd5f9 100644
--- a/hw/misc/ibm-cffps.c
+++ b/hw/misc/ibm-cffps.c
@@ -152,6 +152,8 @@  static int ibm_cffps_event(I2CSlave *i2c, enum i2c_event event)
     case I2C_FINISH:
          s->pointer = 0xFF;
         break;
+    default:
+        return -1;
     }
 
     s->len = 0;
diff --git a/hw/misc/ir35221.c b/hw/misc/ir35221.c
index 5e01d3655059..c46b9ee1c3bf 100644
--- a/hw/misc/ir35221.c
+++ b/hw/misc/ir35221.c
@@ -117,6 +117,8 @@  static int ir35221_event(I2CSlave *i2c, enum i2c_event event)
     case I2C_FINISH:
          s->pointer = 0xFF;
         break;
+    default:
+        return -1;
     }
 
     s->len = 0;
diff --git a/hw/nvram/eeprom_at24c.c b/hw/nvram/eeprom_at24c.c
index 01a3093600fa..d695f6ae894a 100644
--- a/hw/nvram/eeprom_at24c.c
+++ b/hw/nvram/eeprom_at24c.c
@@ -75,6 +75,8 @@  int at24c_eeprom_event(I2CSlave *s, enum i2c_event event)
         break;
     case I2C_NACK:
         break;
+    default:
+        return -1;
     }
     return 0;
 }
diff --git a/hw/sensor/lsm303dlhc_mag.c b/hw/sensor/lsm303dlhc_mag.c
index 4c98ddbf207c..bb8d48b2fdb0 100644
--- a/hw/sensor/lsm303dlhc_mag.c
+++ b/hw/sensor/lsm303dlhc_mag.c
@@ -427,6 +427,8 @@  static int lsm303dlhc_mag_event(I2CSlave *i2c, enum i2c_event event)
         break;
     case I2C_NACK:
         break;
+    default:
+        return -1;
     }
 
     s->len = 0;
diff --git a/include/hw/i2c/i2c.h b/include/hw/i2c/i2c.h
index be8bb8b78a60..9b9581d23097 100644
--- a/include/hw/i2c/i2c.h
+++ b/include/hw/i2c/i2c.h
@@ -12,6 +12,7 @@ 
 enum i2c_event {
     I2C_START_RECV,
     I2C_START_SEND,
+    I2C_START_SEND_ASYNC,
     I2C_FINISH,
     I2C_NACK /* Masker NACKed a receive byte.  */
 };
@@ -28,6 +29,9 @@  struct I2CSlaveClass {
     /* Master to slave. Returns non-zero for a NAK, 0 for success. */
     int (*send)(I2CSlave *s, uint8_t data);
 
+    /* Master to slave (asynchronous). Receiving slave must call i2c_ack(). */
+    void (*send_async)(I2CSlave *s, uint8_t data);
+
     /*
      * Slave to master.  This cannot fail, the device should always
      * return something here.
@@ -127,11 +131,23 @@  int i2c_start_recv(I2CBus *bus, uint8_t address);
  */
 int i2c_start_send(I2CBus *bus, uint8_t address);
 
+/**
+ * i2c_start_send_async: start an asynchronous 'send' transfer on an I2C bus.
+ *
+ * @bus: #I2CBus to be used
+ * @address: address of the slave
+ *
+ * Return: 0 on success, -1 on error
+ */
+int i2c_start_send_async(I2CBus *bus, uint8_t address);
+
 void i2c_end_transfer(I2CBus *bus);
 void i2c_nack(I2CBus *bus);
+void i2c_ack(I2CBus *bus);
 void i2c_bus_master(I2CBus *bus, QEMUBH *bh);
 void i2c_bus_release(I2CBus *bus);
 int i2c_send(I2CBus *bus, uint8_t data);
+int i2c_send_async(I2CBus *bus, uint8_t data);
 uint8_t i2c_recv(I2CBus *bus);
 bool i2c_scan_bus(I2CBus *bus, uint8_t address, bool broadcast,
                   I2CNodeList *current_devs);