Re: [Qemu-devel] [PATCH 2/5] hw/block/pflash_cfi01: Extract the pflash_reset() code

[Laszlo Ersek]

On 05/05/19 22:05, Philippe Mathieu-Daudé wrote:
> The reset() code is used in various places, refactor it.
> 
> Signed-off-by: Philippe Mathieu-Daudé <address@hidden>
> ---
>  hw/block/pflash_cfi01.c | 21 ++++++++++++---------
>  1 file changed, 12 insertions(+), 9 deletions(-)
> 
> diff --git a/hw/block/pflash_cfi01.c b/hw/block/pflash_cfi01.c
> index 6dc04f156a7..073cd14978f 100644
> --- a/hw/block/pflash_cfi01.c
> +++ b/hw/block/pflash_cfi01.c
> @@ -108,6 +108,15 @@ static const VMStateDescription vmstate_pflash = {
>      }
>  };
>  
> +static void pflash_reset(PFlashCFI01 *pfl)
> +{
> +    trace_pflash_reset();
> +    pfl->wcycle = 0;
> +    pfl->cmd = 0;
> +    pfl->status = 0;
> +    memory_region_rom_device_set_romd(&pfl->mem, true);
> +}
> +
>  /* Perform a CFI query based on the bank width of the flash.
>   * If this code is called we know we have a device_width set for
>   * this flash.
> @@ -275,8 +284,7 @@ static uint32_t pflash_read(PFlashCFI01 *pfl, hwaddr 
> offset,
>      default:
>          /* This should never happen : reset state & treat it as a read */
>          DPRINTF("%s: unknown command state: %x\n", __func__, pfl->cmd);
> -        pfl->wcycle = 0;
> -        pfl->cmd = 0;
> +        pflash_reset(pfl);
>          /* fall through to read code */
>      case 0x00:
>          /* Flash area read */

This change introduces two new actions:
- zeroing "status"
- flipping the chip to romd mode (unless it's already in romd mode)

> @@ -639,10 +647,7 @@ static void pflash_write(PFlashCFI01 *pfl, hwaddr offset,
>                    "\n", __func__, offset, pfl->wcycle, pfl->cmd, value);
>  
>   reset_flash:
> -    trace_pflash_reset();
> -    memory_region_rom_device_set_romd(&pfl->mem, true);
> -    pfl->wcycle = 0;
> -    pfl->cmd = 0;
> +    pflash_reset(pfl);
>  }

This change introduces a new action:
- zeroing "status"

>  
>  
> @@ -757,9 +762,7 @@ static void pflash_cfi01_realize(DeviceState *dev, Error 
> **errp)
>          pfl->max_device_width = pfl->device_width;
>      }
>  
> -    pfl->wcycle = 0;
> -    pfl->cmd = 0;
> -    pfl->status = 0;
> +    pflash_reset(pfl);

This change introduces a new action:
- flipping the chip to romd mode (unless it's already in romd mode)

>      /* Hardcoded CFI table */
>      /* Standard "QRY" string */
>      pfl->cfi_table[0x10] = 'Q';
> 

Therefore, this patch is not *obviously* a refactoring patch. (IOW, it
may be a pure refactoring patch, but it's not very easy to see.) I suggest:

- either documenting the new actions in the commit message (and why they
are justified),

- or prepending a separate patch that first unifies the behavior in all
the separate reset locations, and then this patch could indeed become an
"obvious" refactoring / code exctraction.

(Clearly, I'm asking for this with an eye towards git-bisect.)

Thanks
Laszlo