Re: [PATCH] hw/char: stm32f2xx_usart: replace print with trace

[Philippe Mathieu-Daudé]

Hi Nikita,

On 20/12/24 09:46, Nikita Shubin wrote:
> From: Nikita Shubin <nshubin@yadro.com>
>
> Drop debug printing macros and replace them with according trace
> functions.
>
> Signed-off-by: Nikita Shubin <n.shubin@yadro.com>
> ---
>   hw/char/stm32f2xx_usart.c | 47 ++++++++++++++++++---------------------
>   hw/char/trace-events      |  6 +++++
>   2 files changed, 28 insertions(+), 25 deletions(-)


>   static int stm32f2xx_usart_can_receive(void *opaque)
>   {
> @@ -67,10 +57,11 @@ static void stm32f2xx_update_irq(STM32F2XXUsartState *s)
>   static void stm32f2xx_usart_receive(void *opaque, const uint8_t *buf, int 
> size)
>   {
>       STM32F2XXUsartState *s = opaque;
> +    DeviceState *d = DEVICE(s);
>   
>       if (!(s->usart_cr1 & USART_CR1_UE && s->usart_cr1 & USART_CR1_RE)) {
>           /* USART not enabled - drop the chars */
> -        DB_PRINT("Dropping the chars\n");
> +        trace_stm32f2xx_usart_drop(d->id);
>           return;
>       }
>   
> @@ -79,7 +70,7 @@ static void stm32f2xx_usart_receive(void *opaque, const uint8_t *buf, int size)
>   
>       stm32f2xx_update_irq(s);
>   
> -    DB_PRINT("Receiving: %c\n", s->usart_dr);
> +    trace_stm32f2xx_usart_recieve(d->id, *buf);

Typo "receive".

>   }
>   
>   static void stm32f2xx_usart_reset(DeviceState *dev)
> @@ -101,49 +92,55 @@ static uint64_t stm32f2xx_usart_read(void *opaque, hwaddr 
> addr,
>                                          unsigned int size)
>   {
>       STM32F2XXUsartState *s = opaque;
> +    DeviceState *d = DEVICE(s);
>       uint64_t retvalue;

'retvalue' should be initialized ...

>   
> -    DB_PRINT("Read 0x%"HWADDR_PRIx"\n", addr);
> -
>       switch (addr) {
>       case USART_SR:
>           retvalue = s->usart_sr;
>           qemu_chr_fe_accept_input(&s->chr);
> -        return retvalue;
> +        break;
>       case USART_DR:
> -        DB_PRINT("Value: 0x%" PRIx32 ", %c\n", s->usart_dr, (char) 
> s->usart_dr);
>           retvalue = s->usart_dr & 0x3FF;
>           s->usart_sr &= ~USART_SR_RXNE;
>           qemu_chr_fe_accept_input(&s->chr);
>           stm32f2xx_update_irq(s);
> -        return retvalue;
> +        break;
>       case USART_BRR:
> -        return s->usart_brr;
> +        retvalue = s->usart_brr;
> +        break;
>       case USART_CR1:
> -        return s->usart_cr1;
> +        retvalue = s->usart_cr1;
> +        break;
>       case USART_CR2:
> -        return s->usart_cr2;
> +        retvalue = s->usart_cr2;
> +        break;
>       case USART_CR3:
> -        return s->usart_cr3;
> +        retvalue = s->usart_cr3;
> +        break;
>       case USART_GTPR:
> -        return s->usart_gtpr;
> +        retvalue = s->usart_gtpr;
> +        break;
>       default:
>           qemu_log_mask(LOG_GUEST_ERROR,
>                         "%s: Bad offset 0x%"HWADDR_PRIx"\n", __func__, addr);
>           return 0;
>       }
>   
> -    return 0;
> +    trace_stm32f2xx_usart_read(d->id, size, addr, retvalue);
> +
> +    return retvalue;

... since you return here.

Otherwise, the patch looks good!

Regards,

Phil.

>   }