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

[Julien Grall <julien@xxxxxxx>]

Hi,

On 20/08/2024 10:00, Michal Orzel wrote:
> 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:
In general, we allow to exceed the limit only for the message. If there 
are arguments then ...
>          printk("omap-uart: Clock frequency not specified, defaulting to 
> %uHz\n",
>                 UART_OMAP_DEFAULT_CLK_SPEED);
... this is the preferred approach. I can do the update on commit if an 
updated commit message is provided.
Cheers,

--
Julien Grall