Le 14/07/2020 à 00:09, Pavel Pisa a écrit :
> Hello Laurent and others,
>
> On Monday 06 of July 2020 18:35:50 Laurent Vivier wrote:
>> Le 30/06/2020 à 09:55, Thomas Huth a écrit :
>>> Add fallthrough annotations to be able to compile the code without
>>> warnings when using -Wimplicit-fallthrough in our CFLAGS. Looking
>>> at the code, it seems like the fallthrough is indeed intended here,
>>> so the comments should be appropriate.
>>>
>>> Signed-off-by: Thomas Huth <th...@redhat.com>
>>> ---
>>> hw/net/can/can_sja1000.c | 2 ++
>>> 1 file changed, 2 insertions(+)
>>>
>>> diff --git a/hw/net/can/can_sja1000.c b/hw/net/can/can_sja1000.c
>>> index ea915a023a..299932998a 100644
>>> --- a/hw/net/can/can_sja1000.c
>>> +++ b/hw/net/can/can_sja1000.c
>>> @@ -523,6 +523,7 @@ void can_sja_mem_write(CanSJA1000State *s, hwaddr
>>> addr, uint64_t val, break;
>>> case 16: /* RX frame information addr16-28. */
>>> s->status_pel |= (1 << 5); /* Set transmit status. */
>>> + /* fallthrough */
>>> case 17 ... 28:
>>> if (s->mode & 0x01) { /* Reset mode */
>>> if (addr < 24) {
>>> @@ -620,6 +621,7 @@ void can_sja_mem_write(CanSJA1000State *s, hwaddr
>>> addr, uint64_t val, break;
>>> case 10:
>>> s->status_bas |= (1 << 5); /* Set transmit status. */
>>> + /* fallthrough */
>>> case 11 ... 19:
>>> if ((s->control & 0x01) == 0) { /* Operation mode */
>>> s->tx_buff[addr - 10] = val; /* Store to TX buffer
>>> directly. */
>>
>> cc: Pavel Pisa <p...@cmp.felk.cvut.cz>
>>
>> Reviewed-by: Laurent Vivier <laur...@vivier.eu>
>
> The fallthrough is intentional for sure but I have gone
> through datasheet and checked why the status bit is set
> there and my conclusion is that to mimic real HW the status
> bit should not be set there. In the fact, it should be set
> and immediately (in a future delayed) reset after SJA_CMR
> transmit request write. This would mimic real hardware
> more closely. May it be I send patch in future when more
> of our developed CAN support is added to QEMU. The status
> bit behavior has no influence on actual Linux SJA1000 driver
> anyway.
>
> So for now, I confirm that adding /* fallthrough */ is correct
> step forward.
>
> Reviewed-by: Pavel Pisa <p...@cmp.felk.cvut.cz>
>
Applied to my trivial-patches branch.
Thanks,
Laurent
