On Fri, 29 Jan 2021 at 01:04, Hao Wu <wuhao...@google.com> wrote:
>
> This patch implements the FIFO mode of the SMBus module. In FIFO, the
> user transmits or receives at most 16 bytes at a time. The FIFO mode
> allows the module to transmit large amount of data faster than single
> byte mode.
>
> Reviewed-by: Doug Evans<d...@google.com>
> Reviewed-by: Tyrong Ting<kft...@nuvoton.com>
> Signed-off-by: Hao Wu <wuhao...@google.com>
> Reviewed-by: Corey Minyard <cminy...@mvista.com>
> Ack-by: Corey Minyard <cminy...@mvista.com>
> ---
> hw/i2c/npcm7xx_smbus.c | 342 +++++++++++++++++++++++++++++--
> hw/i2c/trace-events | 1 +
> include/hw/i2c/npcm7xx_smbus.h | 25 +++
> tests/qtest/npcm7xx_smbus-test.c | 149 +++++++++++++-
> 4 files changed, 501 insertions(+), 16 deletions(-)
>
> diff --git a/hw/i2c/npcm7xx_smbus.c b/hw/i2c/npcm7xx_smbus.c
> index c72b6e446f..be3253d251 100644
> --- a/hw/i2c/npcm7xx_smbus.c
> +++ b/hw/i2c/npcm7xx_smbus.c
> @@ -27,7 +27,7 @@
> #include "trace.h"
>
> #define NPCM7XX_SMBUS_VERSION 1
> -#define NPCM7XX_SMBUS_FIFO_EN 0
> +#define NPCM7XX_SMBUS_FIFO_EN 1
Why has this define changed ?
> #define NPCM7XX_SMBUS_ENABLED(s) ((s)->ctl2 & NPCM7XX_SMBCTL2_ENABLE)
> +#define NPCM7XX_SMBUS_FIFO_ENABLED(s) (NPCM7XX_SMBUS_FIFO_EN && \
> + (s)->fif_ctl & NPCM7XX_SMBFIF_CTL_FIFO_EN)
...and why are we testing something that's always 1 ?
Is NPCM7XX_SMBUS_FIFO_EN supposed to be a debug "turn this feature off"
switch for QEMU developers? If, so it would be helpful to give it a name
that doesn't look like it's defining a bit value for the hardware
and adding a comment saying what it does.
> @@ -754,6 +1059,17 @@ static const VMStateDescription vmstate_npcm7xx_smbus =
> {
> VMSTATE_UINT8_ARRAY(addr, NPCM7xxSMBusState, NPCM7XX_SMBUS_NR_ADDRS),
> VMSTATE_UINT8(scllt, NPCM7xxSMBusState),
> VMSTATE_UINT8(sclht, NPCM7xxSMBusState),
> + VMSTATE_UINT8(fif_ctl, NPCM7xxSMBusState),
> + VMSTATE_UINT8(fif_cts, NPCM7xxSMBusState),
> + VMSTATE_UINT8(fair_per, NPCM7xxSMBusState),
> + VMSTATE_UINT8(txf_ctl, NPCM7xxSMBusState),
> + VMSTATE_UINT8(t_out, NPCM7xxSMBusState),
> + VMSTATE_UINT8(txf_sts, NPCM7xxSMBusState),
> + VMSTATE_UINT8(rxf_sts, NPCM7xxSMBusState),
> + VMSTATE_UINT8(rxf_ctl, NPCM7xxSMBusState),
> + VMSTATE_UINT8_ARRAY(rx_fifo, NPCM7xxSMBusState,
> + NPCM7XX_SMBUS_FIFO_SIZE),
> + VMSTATE_UINT8(rx_cur, NPCM7xxSMBusState),
> VMSTATE_END_OF_LIST(),
> },
> };
It's OK to add fields to the vmstate without bumping the version
number in this special case, because we only just added the device
a few commits earlier in the series, but it's worth specifically
saying that in the commit message.
thanks
-- PMM
