Re: [PATCH qemu v3 1/2] hw/at24c : modify at24c to support 1 byte address mode

[Peter Delevoryas]

On Fri, Feb 17, 2023 at 10:31:27AM +0700, ~ssinprem wrote:
> From: Sittisak Sinprem <ssinprem@celestica.com>
> 
> Signed-off-by: Sittisak Sinprem <ssinprem@celestica.com>
> ---
>  hw/nvram/eeprom_at24c.c | 28 +++++++++++++++++++++++++---
>  1 file changed, 25 insertions(+), 3 deletions(-)
> 
> diff --git a/hw/nvram/eeprom_at24c.c b/hw/nvram/eeprom_at24c.c
> index 3328c32814..64259cde67 100644
> --- a/hw/nvram/eeprom_at24c.c
> +++ b/hw/nvram/eeprom_at24c.c
> @@ -41,6 +41,12 @@ struct EEPROMState {
>      uint16_t cur;
>      /* total size in bytes */
>      uint32_t rsize;
> +    /* address byte number 
> +     *  for  24c01, 24c02 size <= 256 byte, use only 1 byte
> +     *  otherwise size > 256, use 2 byte
> +     */
> +    uint8_t asize;

I was going to say "why not address_size", but then I see right above there's
"rsize", which is also very obscure lol, so whatever, this is keeping
consistent with the existing code.

> +
>      bool writable;
>      /* cells changed since last START? */
>      bool changed;
> @@ -91,7 +97,10 @@ uint8_t at24c_eeprom_recv(I2CSlave *s)
>      EEPROMState *ee = AT24C_EE(s);
>      uint8_t ret;
>  
> -    if (ee->haveaddr == 1) {
> +    /* If got the byte address but not completely with address size
> +     *  will return the invalid value
> +     */
> +    if (ee->haveaddr > 0 && ee->haveaddr < ee->asize) {
>          return 0xff;
>      }
>  
> @@ -108,11 +117,11 @@ int at24c_eeprom_send(I2CSlave *s, uint8_t data)
>  {
>      EEPROMState *ee = AT24C_EE(s);
>  
> -    if (ee->haveaddr < 2) {
> +    if (ee->haveaddr < ee->asize) {

Yay, love it 🍍

>          ee->cur <<= 8;
>          ee->cur |= data;
>          ee->haveaddr++;
> -        if (ee->haveaddr == 2) {
> +        if (ee->haveaddr == ee->asize) {
>              ee->cur %= ee->rsize;
>              DPRINTK("Set pointer %04x\n", ee->cur);
>          }
> @@ -199,6 +208,18 @@ static void at24c_eeprom_realize(DeviceState *dev, Error 
> **errp)
>          }
>          DPRINTK("Reset read backing file\n");
>      }
> +
> +    /*
> +     * If address size didn't define with property set
> +     *   value is 0 as default, setting it by Rom size detecting.

Helpful comment, good stuff

> +     */
> +    if (ee->asize == 0) {
> +        if (ee->rsize <= 256) {
> +            ee->asize = 1;
> +        } else {
> +            ee->asize = 2;
> +        }
> +    }
>  }
>  
>  static
> @@ -213,6 +234,7 @@ void at24c_eeprom_reset(DeviceState *state)
>  
>  static Property at24c_eeprom_props[] = {
>      DEFINE_PROP_UINT32("rom-size", EEPROMState, rsize, 0),
> +    DEFINE_PROP_UINT8("address-size", EEPROMState, asize, 0),
>      DEFINE_PROP_BOOL("writable", EEPROMState, writable, true),
>      DEFINE_PROP_DRIVE("drive", EEPROMState, blk),
>      DEFINE_PROP_END_OF_LIST()

Reviewed-by: Peter Delevoryas <peter@pjd.dev>

> -- 
> 2.34.6
> 
>