[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-trivial] [PATCH] SMBUS module update.
From: |
Stefan Hajnoczi |
Subject: |
Re: [Qemu-trivial] [PATCH] SMBUS module update. |
Date: |
Fri, 26 Apr 2013 12:06:17 +0200 |
User-agent: |
Mutt/1.5.21 (2010-09-15) |
On Thu, Apr 25, 2013 at 02:20:18AM +0400, Maksim Ratnikov wrote:
> Subject: [PATCH] SMBUS module update. Previous realization doesn't consider
Please choose a more specific commit message, "SMBUS module update" does
not say what this patch does.
> flags in the status register. In this patch processing of bits of DS and
> INTR of the register HST_STS is added. Mechanism of clearing bits value in
> the register HST_STS is updated. Error processing is updated: if DEV_ERR
> bit are set transaction isn't evaluated.
> Signed-off-by: Maksim Ratnikov <address@hidden>
> ---
> hw/pm_smbus.c | 26 ++++++++++++++++++--------
> 1 files changed, 18 insertions(+), 8 deletions(-)
>
> diff --git a/hw/pm_smbus.c b/hw/pm_smbus.c
> index ea1380c..3a7b42d 100644
> --- a/hw/pm_smbus.c
> +++ b/hw/pm_smbus.c
> @@ -17,10 +17,10 @@
> * License along with this library; if not, see
> * <http://www.gnu.org/licenses/>.
> */
> -#include "hw.h"
> -#include "pc.h"
> -#include "pm_smbus.h"
> -#include "smbus.h"
> +#include "hw/hw.h"
> +#include "hw/pc.h"
> +#include "hw/pm_smbus.h"
> +#include "hw/smbus.h"
Looks like this patch is against an outdated QEMU source tree. Please
rebase onto qemu.git/master.
>
> /* no save/load? */
>
> @@ -40,7 +40,6 @@
> # define SMBUS_DPRINTF(format, ...) do { } while (0)
> #endif
>
> -
Please do not make random whitespace changes.
> static void smb_transaction(PMSMBus *s)
> {
> uint8_t prot = (s->smb_ctl >> 2) & 0x07;
> @@ -48,11 +47,16 @@ static void smb_transaction(PMSMBus *s)
> uint8_t cmd = s->smb_cmd;
> uint8_t addr = s->smb_addr >> 1;
> i2c_bus *bus = s->smbus;
> -
> +
> + if ( (s->smb_stat & 0x04) != 0) goto error; // DEV_ERR
> evaluate
> +
> +
> SMBUS_DPRINTF("SMBus trans addr=0x%02x prot=0x%02x\n", addr, prot);
> switch(prot) {
> case 0x0:
> - smbus_quick_command(bus, addr, read);
> + smbus_quick_command(bus, addr, read);
> + s->smb_stat |= 0x82; // set ByteDoneStatus = 1 (bit #7)
> and INTR = (bit #1)
> +
Indentation is off here. Please use 4 spaces, see ./CODING_STYLE.
Line wrap is 80 characters, comments therefore usually look like this:
/* set ByteDoneStatus = 1 (bit #7) and INTR = (bit #1) */
s->smb_stat |= 0x82;
This particular comment suggests defining ByteDoneStatus and Intr
constants. that way you don't need comments to explain the 0x82 magic
number.
> @@ -140,6 +149,7 @@ static uint64_t smb_ioport_readb(void *opaque, hwaddr
> addr, unsigned width)
> switch(addr) {
> case SMBHSTSTS:
> val = s->smb_stat;
> + //val = 0x1e;
> break;
> case SMBHSTCNT:
> s->smb_index = 0;
Random commented out code that should be dropped?