Re: [Qemu-devel] [PATCH] Extend support of SMBUS(module pm_smbus.c) HST_STS register.

[Peter Maydell]

On 28 April 2013 19:26, Maksim Ratnikov <address@hidden> 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 <address@hidden>
> ---
>  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