Re: [Qemu-devel] [PATCH v3 05/16] i2c: Simplify and correct the SMBus state machine

[Corey Minyard]

On 11/30/18 12:13 PM, Peter Maydell wrote:
> 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.


Ok, I've split this up into separate patches, as they should have been.

Thanks,

-corey


> >   #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