Re: [RFC PATCH v6 04/11] hw/ssi: imx_spi: Reduce 'change_mask' variable scope

[Juan Quintela]

Juan Quintela <quintela@redhat.com> wrote:
> Philippe Mathieu-Daudé <f4bug@amsat.org> wrote:
>> Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
>
> I think this one is wrong.

Wrong is a strong word.  I mean that it changes behaviour and the commit
message don't talk about changing behaviour.

Later, Juan.

>
>
>> ---
>>  hw/ssi/imx_spi.c | 3 +--
>>  1 file changed, 1 insertion(+), 2 deletions(-)
>>
>> diff --git a/hw/ssi/imx_spi.c b/hw/ssi/imx_spi.c
>> index 35ab33c0511..bcc535f2893 100644
>> --- a/hw/ssi/imx_spi.c
>> +++ b/hw/ssi/imx_spi.c
>> @@ -303,7 +303,6 @@ static void imx_spi_write(void *opaque, hwaddr offset, 
>> uint64_t value,
>>  {
>>      IMXSPIState *s = opaque;
>>      uint32_t index = offset >> 2;
>> -    uint32_t change_mask;
>>  
>>      if (index >=  ECSPI_MAX) {
>>          qemu_log_mask(LOG_GUEST_ERROR, "[%s]%s: Bad register at offset 0x%"
>> @@ -313,7 +312,6 @@ static void imx_spi_write(void *opaque, hwaddr offset, 
>> uint64_t value,
>>  
>>      trace_imx_spi_write(index, imx_spi_reg_name(index), value);
>>  
>> -    change_mask = s->regs[index] ^ value;
>>  
>>      switch (index) {
>>      case ECSPI_RXDATA:
>> @@ -357,6 +355,7 @@ static void imx_spi_write(void *opaque, hwaddr offset, 
>> uint64_t value,
>>          }
>>  
>>          if (imx_spi_channel_is_master(s)) {
>> +            uint32_t change_mask = s->regs[index] ^ value;
>>              int i;
>>  
>>              /* We are in master mode */
>
> The code does:
>
>     change_mask = s->regs[index] ^ value;
>
>     switch (index) {
>
>     ...
>
>     case ECSPI_CONREG:
>         s->regs[ECSPI_CONREG] = value;  <<---- here
>
>         if (!imx_spi_is_enabled(s)) {
>             /* device is disabled, so this is a reset */
>             imx_spi_reset(DEVICE(s));
>             return;
>         }
>
>         if (imx_spi_channel_is_master(s)) {
>             int i;
>        >>>>>  You are setting change_mask here.
>
> At this point, s->regs[index] has a new value in "here".
>
> Later, Juan.