Re: [PATCH v1 5/9] serial: qcom-geni: move resource initialization to separate functions

Jiri Slaby
2025-04-14T07:58:27Z

On 10. 04. 25, 19:40, Praveen Talari wrote:
> Enhances code readability and future modifications within the new API.
>
> Move the code that handles the actual initialization of resources
> like clock and ICC paths to a separate function, making the
> probe function cleaner.

The $SUBJ is misleading. There is only one function here.

>
> Signed-off-by: Praveen Talari <quic_ptalari@quicinc.com>
> ---
> drivers/tty/serial/qcom_geni_serial.c | 65 ++++++++++++++++-----------
> 1 file changed, 39 insertions(+), 26 deletions(-)
>
> diff --git a/drivers/tty/serial/qcom_geni_serial.c b/drivers/tty/serial/qcom_geni_serial.c
> index a80ce7aaf309..889ce8961e0a 100644
> --- a/drivers/tty/serial/qcom_geni_serial.c
> +++ b/drivers/tty/serial/qcom_geni_serial.c
> @@ -1572,6 +1572,42 @@ static struct uart_driver qcom_geni_uart_driver = {
> .nr = GENI_UART_PORTS,
> };
>
> +static int geni_serial_resource_init(struct qcom_geni_serial_port *port)
> +{
> + int ret;
> +
> + port->se.clk = devm_clk_get(port->se.dev, "se");
> + if (IS_ERR(port->se.clk)) {
> + ret = PTR_ERR(port->se.clk);

You can return this directly, without assigning it to ret, right?

> + dev_err(port->se.dev, "Err getting SE Core clk %d\n", ret);
> + return ret;
> + }
> +
> + ret = geni_icc_get(&port->se, NULL);
> + if (ret)
> + return ret;
> +
> + port->se.icc_paths[GENI_TO_CORE].avg_bw = GENI_DEFAULT_BW;
> + port->se.icc_paths[CPU_TO_GENI].avg_bw = GENI_DEFAULT_BW;
> +
> + /* Set BW for register access */
> + ret = geni_icc_set_bw(&port->se);
> + if (ret)
> + return ret;
> +
> + ret = devm_pm_opp_set_clkname(port->se.dev, "se");
> + if (ret)
> + return ret;
> +
> + /* OPP table is optional */
> + ret = devm_pm_opp_of_add_table(port->se.dev);
> + if (ret && ret != -ENODEV) {
> + dev_err(port->se.dev, "invalid OPP table in device tree\n");
> + return ret;
> + }
> +
> + return 0;
> +}

A \n missing here.

> static void qcom_geni_serial_pm(struct uart_port *uport,
> unsigned int new_state, unsigned int old_state)
> {

thanks,
--
js
suse labs