| Message ID | CAAWx3aQybzcH8L4rxpNpSQsxwq_9F0KC3Aqhq4g6seKQLxy-jw@mail.gmail.com |
|---|---|
| State | New |
| Headers | show |
On 28 April 2013 19:26, Maksim Ratnikov <m.o.ratnikov@gmail.com> wrote: > Previous realization doesn't consider flags in the status register. > Add DS and INTR bits of HST_STS register set after transaction execution. > Update bits resetting in HST_STS register. Update error processing: if > DEV_ERR bit are set > transaction isn't execution. Hi; thanks for this patch. A minor comment below... > Signed-off-by: Maksim_Ratnikov <m.o.ratnikov@gmail.com> > --- > hw/i2c/pm_smbus.c | 13 ++++++++++++- > 1 files changed, 12 insertions(+), 1 deletions(-) > > diff --git a/hw/i2c/pm_smbus.c b/hw/i2c/pm_smbus.c > index 0b5bb89..d5f5c56 100644 > --- a/hw/i2c/pm_smbus.c > +++ b/hw/i2c/pm_smbus.c > @@ -50,9 +50,16 @@ static void smb_transaction(PMSMBus *s) > i2c_bus *bus = s->smbus; > > SMBUS_DPRINTF("SMBus trans addr=0x%02x prot=0x%02x\n", addr, prot); > + /* Transaction isn't exec if DEV_ERR bit set */ > + if ((s->smb_stat & 0x04) != 0) > + goto error; QEMU coding style requires braces on this if(). (You can run your patch through scripts/codingstyle.pl to check for this kind of thing.) > switch(prot) { > case 0x0: > smbus_quick_command(bus, addr, read); > + /* Set successfully transaction end: > + * ByteDoneStatus = 1 (HST_STS bit #7) and INTR = 1 (HST_STS bit > #1) > + */ > + s->smb_stat |= 0x82; I think it would be nicer to define some constants for the HST_STS bits. Then you could write s->smb_stat |= STS_BYTE_DONE | STS_INTR; and you wouldn't need to have the comment here explaining the magic numbers. (feel free to pick better constant names, ideally ones matching whatever the datasheet uses.) (then you can use the constants for all the smb_stat uses). thanks -- PMM
29.04.2013 00:03, Peter Maydell пишет: > On 28 April 2013 19:26, Maksim Ratnikov <m.o.ratnikov@gmail.com> wrote: >> Previous realization doesn't consider flags in the status register. >> Add DS and INTR bits of HST_STS register set after transaction execution. >> Update bits resetting in HST_STS register. Update error processing: if >> DEV_ERR bit are set >> transaction isn't execution. > Hi; thanks for this patch. A minor comment below... > >> Signed-off-by: Maksim_Ratnikov <m.o.ratnikov@gmail.com> >> --- >> hw/i2c/pm_smbus.c | 13 ++++++++++++- >> 1 files changed, 12 insertions(+), 1 deletions(-) >> >> diff --git a/hw/i2c/pm_smbus.c b/hw/i2c/pm_smbus.c >> index 0b5bb89..d5f5c56 100644 >> --- a/hw/i2c/pm_smbus.c >> +++ b/hw/i2c/pm_smbus.c >> @@ -50,9 +50,16 @@ static void smb_transaction(PMSMBus *s) >> i2c_bus *bus = s->smbus; >> >> SMBUS_DPRINTF("SMBus trans addr=0x%02x prot=0x%02x\n", addr, prot); >> + /* Transaction isn't exec if DEV_ERR bit set */ >> + if ((s->smb_stat & 0x04) != 0) >> + goto error; > QEMU coding style requires braces on this if(). (You can run > your patch through scripts/codingstyle.pl to check for this > kind of thing.) > >> switch(prot) { >> case 0x0: >> smbus_quick_command(bus, addr, read); >> + /* Set successfully transaction end: >> + * ByteDoneStatus = 1 (HST_STS bit #7) and INTR = 1 (HST_STS bit >> #1) >> + */ >> + s->smb_stat |= 0x82; > I think it would be nicer to define some constants for the > HST_STS bits. Then you could write > s->smb_stat |= STS_BYTE_DONE | STS_INTR; > > and you wouldn't need to have the comment here explaining > the magic numbers. (feel free to pick better constant names, > ideally ones matching whatever the datasheet uses.) > (then you can use the constants for all the smb_stat uses). > > thanks > -- PMM Thank you for your answer. I made some correction and sent new version of patch. I can't find script codingstyle.pl, but I use checkpatch.pl . It shows 0 error and 0 warning.
diff --git a/hw/i2c/pm_smbus.c b/hw/i2c/pm_smbus.c index 0b5bb89..d5f5c56 100644 --- a/hw/i2c/pm_smbus.c +++ b/hw/i2c/pm_smbus.c @@ -50,9 +50,16 @@ static void smb_transaction(PMSMBus *s) i2c_bus *bus = s->smbus; SMBUS_DPRINTF("SMBus trans addr=0x%02x prot=0x%02x\n", addr, prot); + /* Transaction isn't exec if DEV_ERR bit set */ + if ((s->smb_stat & 0x04) != 0) + goto error; switch(prot) { case 0x0: smbus_quick_command(bus, addr, read); + /* Set successfully transaction end: + * ByteDoneStatus = 1 (HST_STS bit #7) and INTR = 1 (HST_STS bit #1) + */ + s->smb_stat |= 0x82; break; case 0x1:
Previous realization doesn't consider flags in the status register. Add DS and INTR bits of HST_STS register set after transaction execution. Update bits resetting in HST_STS register. Update error processing: if DEV_ERR bit are set transaction isn't execution. Signed-off-by: Maksim_Ratnikov <m.o.ratnikov@gmail.com> --- hw/i2c/pm_smbus.c | 13 ++++++++++++- 1 files changed, 12 insertions(+), 1 deletions(-) if (read) { @@ -60,6 +67,7 @@ static void smb_transaction(PMSMBus *s) } else { smbus_send_byte(bus, addr, cmd); } + s->smb_stat |= 0x82; break; case 0x2: if (read) { @@ -67,6 +75,7 @@ static void smb_transaction(PMSMBus *s) } else { smbus_write_byte(bus, addr, cmd, s->smb_data0); } + s->smb_stat |= 0x82; break; case 0x3: if (read) { @@ -77,6 +86,7 @@ static void smb_transaction(PMSMBus *s) } else { smbus_write_word(bus, addr, cmd, (s->smb_data1 << 8) | s->smb_data0); } + s->smb_stat |= 0x82; break; case 0x5: if (read) { @@ -84,6 +94,7 @@ static void smb_transaction(PMSMBus *s) } else { smbus_write_block(bus, addr, cmd, s->smb_data, s->smb_data0); } + s->smb_stat |= 0x82; break; default: goto error; @@ -102,7 +113,7 @@ static void smb_ioport_writeb(void *opaque, hwaddr addr, uint64_t val, SMBUS_DPRINTF("SMB writeb port=0x%04x val=0x%02x\n", addr, val); switch(addr) { case SMBHSTSTS: - s->smb_stat = 0; + s->smb_stat = (~(val & 0xff)) & s->smb_stat; s->smb_index = 0; break; case SMBHSTCNT: