[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [PATCH v3 05/16] i2c: Simplify and correct the SMBus st
From: |
Peter Maydell |
Subject: |
Re: [Qemu-devel] [PATCH v3 05/16] i2c: Simplify and correct the SMBus state machine |
Date: |
Fri, 30 Nov 2018 18:13:28 +0000 |
On Mon, 26 Nov 2018 at 20:04, <address@hidden> wrote:
>
> From: Corey Minyard <address@hidden>
>
> The SMBus slave code had an unneeded state, unnecessary function
> pointers and incorrectly handled quick commands. Rewrite it
> to simplify the code and make it work correctly.
>
> smbus_eeprom is the only user, so no other effects and the eeprom
> code also gets a significant simplification.
>
> Signed-off-by: Corey Minyard <address@hidden>
> ---
> hw/i2c/smbus_eeprom.c | 58 ++++++-----------------
> hw/i2c/smbus_slave.c | 91 ++++++++++++++++--------------------
> include/hw/i2c/smbus_slave.h | 45 +++++++++++++-----
> 3 files changed, 86 insertions(+), 108 deletions(-)
I'm finding this patch difficult to understand -- it
feels like it's trying to do too many things at once.
Is it possible to split it? For instance it looks like
we're getting rid of send_byte and just handling
all writes to the device with write_data -- is that
right? That sounds like it should be its own patch.
Whatever the change is that means that we don't pass in
the cmd argument to the various methods is probably its
own patch. And fixing quick commands sounds like something
that goes in its own patch.
Stray whitespace change (there are more of these below too).
If you want to do formatting tidyups please put those in
their own patch.
> #ifdef DEBUG
> printf("eeprom_receive_byte: addr=0x%02x val=0x%02x\n",
> dev->i2c.address, val);
> @@ -65,37 +49,26 @@ static uint8_t eeprom_receive_byte(SMBusDevice *dev)
> return val;
> }
>
> -static void eeprom_write_data(SMBusDevice *dev, uint8_t cmd, uint8_t *buf,
> int len)
> +static int eeprom_write_data(SMBusDevice *dev, uint8_t *buf, uint8_t len)
> {
> SMBusEEPROMDevice *eeprom = (SMBusEEPROMDevice *) dev;
> - int n;
> + uint8_t *data = eeprom->data;
> +
> #ifdef DEBUG
> printf("eeprom_write_byte: addr=0x%02x cmd=0x%02x val=0x%02x\n",
> dev->i2c.address, cmd, buf[0]);
The "cmd" argument has been removed from the prototype
but is still used in this debug printf.
> #endif
thanks
-- PMM
- Re: [Qemu-devel] [PATCH v3 09/16] migration: Add a VMSTATE_BOOL_TEST() macro, (continued)
- [Qemu-devel] [PATCH v3 10/16] i2c:pm_smbus: Fix state transfer, minyard, 2018/11/26
- [Qemu-devel] [PATCH v3 13/16] i2c:smbus_eeprom: Add a size constant for the smbus_eeprom size, minyard, 2018/11/26
- [Qemu-devel] [PATCH v3 04/16] i2c: Don't check return value from i2c_recv(), minyard, 2018/11/26
- [Qemu-devel] [PATCH v3 12/16] i2c:smbus_eeprom: Add normal type name and cast to smbus_eeprom.c, minyard, 2018/11/26
- [Qemu-devel] [PATCH v3 05/16] i2c: Simplify and correct the SMBus state machine, minyard, 2018/11/26
- Re: [Qemu-devel] [PATCH v3 05/16] i2c: Simplify and correct the SMBus state machine,
Peter Maydell <=
- [Qemu-devel] [PATCH v3 01/16] i2c: Split smbus into parts, minyard, 2018/11/26
- [Qemu-devel] [PATCH v3 02/16] i2c: have I2C receive operation return uint8_t, minyard, 2018/11/26
- [Qemu-devel] [PATCH v3 11/16] i2c:smbus_slave: Add an SMBus vmstate structure, minyard, 2018/11/26
- [Qemu-devel] [PATCH v3 08/16] boards.h: Ignore migration for SMBus devices on older machines, minyard, 2018/11/26