[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [PATCH] hw/nvram: at24 return 0xff if 1 byte address
From: |
Philippe Mathieu-Daudé |
Subject: |
Re: [PATCH] hw/nvram: at24 return 0xff if 1 byte address |
Date: |
Mon, 20 Dec 2021 10:12:32 +0100 |
User-agent: |
Mozilla/5.0 (X11; Linux x86_64; rv:91.0) Gecko/20100101 Thunderbird/91.4.0 |
Hi Patrick,
On 12/20/21 01:32, Patrick Venture wrote:
> The at24 eeproms are 2 byte devices that return 0xff when they are read
> from with a partial (1-byte) address written. This distinction was
> found comparing model behavior to real hardware testing.
>
> Tested: `i2ctransfer -f -y 45 w1@85 0 r1` returns 0xff instead of next
> byte
>
> Signed-off-by: Patrick Venture <venture@google.com>
> ---
> hw/nvram/eeprom_at24c.c | 8 +++++++-
> 1 file changed, 7 insertions(+), 1 deletion(-)
>
> diff --git a/hw/nvram/eeprom_at24c.c b/hw/nvram/eeprom_at24c.c
> index a9e3702b00..184fac9702 100644
> --- a/hw/nvram/eeprom_at24c.c
> +++ b/hw/nvram/eeprom_at24c.c
> @@ -62,7 +62,9 @@ int at24c_eeprom_event(I2CSlave *s, enum i2c_event event)
> case I2C_START_SEND:
> case I2C_START_RECV:
> case I2C_FINISH:
> - ee->haveaddr = 0;
> + if (event != I2C_START_RECV) {
> + ee->haveaddr = 0;
> + }
Alternatively (matter of taste, I find it easier to read):
case I2C_START_SEND:
case I2C_FINISH:
ee->haveaddr = 0;
/* fallthrough */
case I2C_START_RECV:
> DPRINTK("clear\n");
> if (ee->blk && ee->changed) {
> int len = blk_pwrite(ee->blk, 0, ee->mem, ee->rsize, 0);
> @@ -86,6 +88,10 @@ uint8_t at24c_eeprom_recv(I2CSlave *s)
> EEPROMState *ee = AT24C_EE(s);
> uint8_t ret;
>
> + if (ee->haveaddr == 1) {
> + return 0xff;
Don't we need to increase ee->haveaddr?
> + }
> +
> ret = ee->mem[ee->cur];
>
> ee->cur = (ee->cur + 1u) % ee->rsize;
Here for parity with send, what about:
if (ee->haveaddr < 2) {
ret = 0xff;
ee->haveaddr++;
} else {
ret = ee->mem[ee->cur];
ee->cur = (ee->cur + 1u) % ee->rsize;
}
?