Re: [PATCH V2] serial: sprd: Use devm_clk_get_optional() to get the input clock

[wenhua lin]

On Wed, Dec 10, 2025 at 3:12 PM Chunyan Zhang <zhang.lyra@xxxxxxxxx> wrote:
>
> On Wed, 10 Dec 2025 at 14:36, Wenhua Lin <Wenhua.Lin@xxxxxxxxxx> wrote:
> >
> > Simplify the code which fetches the input clock by using
> > devm_clk_get_optional(). If no input clock is present
> > devm_clk_get_optional() will return NULL instead of an error
> > which matches the behavior of the old code.
>
> This commit message is not describing the key point of its changes.
>
> >
> > Signed-off-by: Wenhua Lin <Wenhua.Lin@xxxxxxxxxx>
> > ---
> > Change in V2:
> > -Change title.
> > -Change commit message.
> > -Replace devm_clk_get() by devm_clk_get_optional() and drop NULL assignment.
> > -Delete the sprd_uart_is_console function, after using the devm_clk_get_optional()
> >  interface, this conditional check is redundant.
> > ---
> >  drivers/tty/serial/sprd_serial.c | 40 +++++++-------------------------
> >  1 file changed, 9 insertions(+), 31 deletions(-)
> >
> > diff --git a/drivers/tty/serial/sprd_serial.c b/drivers/tty/serial/sprd_serial.c
> > index 8c9366321f8e..83ce77b435ee 100644
> > --- a/drivers/tty/serial/sprd_serial.c
> > +++ b/drivers/tty/serial/sprd_serial.c
> > @@ -1115,34 +1115,21 @@ static void sprd_remove(struct platform_device *dev)
> >                 uart_unregister_driver(&sprd_uart_driver);
> >  }
> >
> > -static bool sprd_uart_is_console(struct uart_port *uport)
> > -{
> > -       struct console *cons = sprd_uart_driver.cons;
> > -
> > -       if ((cons && cons->index >= 0 && cons->index == uport->line) ||
> > -           of_console_check(uport->dev->of_node, SPRD_TTY_NAME, uport->line))
> > -               return true;
> > -
> > -       return false;
> > -}
> > -
> >  static int sprd_clk_init(struct uart_port *uport)
> >  {
> >         struct clk *clk_uart, *clk_parent;
> >         struct sprd_uart_port *u = container_of(uport, struct sprd_uart_port, port);
> >
> > -       clk_uart = devm_clk_get(uport->dev, "uart");
> > +       clk_uart = devm_clk_get_optional(uport->dev, "uart");
> >         if (IS_ERR(clk_uart)) {
> > -               dev_warn(uport->dev, "uart%d can't get uart clock\n",
> > -                        uport->line);
> > -               clk_uart = NULL;
> > +               return dev_err_probe(uport->dev, PTR_ERR(clk_uart),
> > +                       "uart%d can't get uart clock\n", uport->line);
>
> No, as I said before, you cannot do like this, this patch is making
> the clocks mandatory, sprd_serial driver could work as serial ports
> for logs output without this "uart" clock.
>
> It is very useful for early debugging when the clock driver is not even ready.
>
> If other SPRD serials' default clock is 24M rather than 26M, like I
> said in the last version patch, you can set default clock according to
> the compatible string, that's saying make SPRD_DEFAULT_SOURCE_CLK to
> be an element of "of_device_id.data".
>
> So NAK for this change, sorry!
>
> Thanks,
> Chunyan

Hi chunyan:
  When the clock driver is not yet ready, the earlyconsole remains functional.
  At this stage, even if the UART driver probe fails, it will not
affect the debugging process.
Thanks

>
> >         }
> >
> > -       clk_parent = devm_clk_get(uport->dev, "source");
> > +       clk_parent = devm_clk_get_optional(uport->dev, "source");
> >         if (IS_ERR(clk_parent)) {
> > -               dev_warn(uport->dev, "uart%d can't get source clock\n",
> > -                        uport->line);
> > -               clk_parent = NULL;
> > +               return dev_err_probe(uport->dev, PTR_ERR(clk_parent),
> > +                       "uart%d can't get source clock\n", uport->line);
> >         }
> >
> >         if (!clk_uart || clk_set_parent(clk_uart, clk_parent))
> > @@ -1150,19 +1137,10 @@ static int sprd_clk_init(struct uart_port *uport)
> >         else
> >                 uport->uartclk = clk_get_rate(clk_uart);
> >
> > -       u->clk = devm_clk_get(uport->dev, "enable");
> > +       u->clk = devm_clk_get_optional(uport->dev, "enable");
> >         if (IS_ERR(u->clk)) {
> > -               if (PTR_ERR(u->clk) == -EPROBE_DEFER)
> > -                       return -EPROBE_DEFER;
> > -
> > -               dev_warn(uport->dev, "uart%d can't get enable clock\n",
> > -                       uport->line);
> > -
> > -               /* To keep console alive even if the error occurred */
> > -               if (!sprd_uart_is_console(uport))
> > -                       return PTR_ERR(u->clk);
> > -
> > -               u->clk = NULL;
> > +               return dev_err_probe(uport->dev, PTR_ERR(u->clk),
> > +                       "uart%d can't get enable clock\n", uport->line);
> >         }
> >
> >         return 0;
> > --
> > 2.34.1
> >