Re: [PATCH v2] drivers: char: omap-uart: provide a default clock frequency

[Michal Orzel <michal.orzel@xxxxxxx>]

On 20/08/2024 10:22, Amneesh Singh wrote:
> 
> 
> Quite a few TI K3 devices do not have clock-frequency specified in their
> respective UART nodes. However hard-coding the frequency is not a
> solution as the function clock input can differ between SoCs. So, use a
> default frequency of 48MHz if the device tree does not specify one.
I'd mention that this is same as Linux

> 
> Signed-off-by: Amneesh Singh <a-singh21@xxxxxx>
> ---
>  xen/drivers/char/omap-uart.c | 8 ++++++--
>  1 file changed, 6 insertions(+), 2 deletions(-)
> ---
> v1: https://lore.kernel.org/all/20240719113313.145587-1-a-singh21@xxxxxx/T/
> 
> v1 -> v2
> - Ditch adding a dtuart option
> - Use a default value instead
> 
> This default is the same one as found in the 8250_omap driver of the
> linux tree. Already tested with Xen.
> 
> diff --git a/xen/drivers/char/omap-uart.c b/xen/drivers/char/omap-uart.c
> index 1079198..9d3d39c 100644
> --- a/xen/drivers/char/omap-uart.c
> +++ b/xen/drivers/char/omap-uart.c
> @@ -48,6 +48,9 @@
>  /* System configuration register */
>  #define UART_OMAP_SYSC_DEF_CONF   0x0d   /* autoidle mode, wakeup is enabled 
> */
> 
> +/* default clock frequency in hz */
> +#define UART_OMAP_DEFAULT_CLK_SPEED 48000000
I think this should have U suffix to please MISRA 7.2

> +
>  #define omap_read(uart, off)       readl((uart)->regs + ((off) << REG_SHIFT))
>  #define omap_write(uart, off, val) writel(val, \
>                                            (uart)->regs + ((off) << 
> REG_SHIFT))
> @@ -322,8 +325,9 @@ static int __init omap_uart_init(struct dt_device_node 
> *dev,
>      res = dt_property_read_u32(dev, "clock-frequency", &clkspec);
>      if ( !res )
>      {
> -        printk("omap-uart: Unable to retrieve the clock frequency\n");
> -        return -EINVAL;
> +        printk("omap-uart: Unable to retrieve the clock frequency, "
> +               "defaulting to %uHz\n", UART_OMAP_DEFAULT_CLK_SPEED);
Even though there is a comma, printk messages should not really be split. In 
such cases it's fine
to exceed 80 chars limit. Or do:
        printk("omap-uart: Clock frequency not specified, defaulting to %uHz\n",
               UART_OMAP_DEFAULT_CLK_SPEED);

Other than that:
Reviewed-by: Michal Orzel <michal.orzel@xxxxxxx>

~Michal