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é <phi...@redhat.com>
> ---
> 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
