Re: [PATCH v1 4/4] serial: qcom-geni: Enable Serial on SA8255p Qualcomm platforms

[Andy Shevchenko <andriy.shevchenko@intel.com>]

On Mon, Nov 10, 2025 at 03:40:43PM +0530, Praveen Talari wrote:
> The Qualcomm automotive SA8255p SoC relies on firmware to configure
> platform resources, including clocks, interconnects and TLMM.
> The driver requests resources operations over SCMI using power
> and performance protocols.
> 
> The SCMI power protocol enables or disables resources like clocks,
> interconnect paths, and TLMM (GPIOs) using runtime PM framework APIs,
> such as resume/suspend, to control power states(on/off).
> 
> The SCMI performance protocol manages UART baud rates, with each baud
> rate represented by a performance level. The driver uses the
> dev_pm_opp_set_level() API to request the desired baud rate by
> specifying the performance level.

...

> +static int geni_serial_pwr_init(struct uart_port *uport)
> +{
> +	struct qcom_geni_serial_port *port = to_dev_port(uport);
> +	int ret;
> +
> +	ret = dev_pm_domain_attach_list(port->se.dev,
> +					&port->dev_data->pd_data, &port->pd_list);
> +	if (ret <= 0)
> +		return -EINVAL;
> +
> +	return 0;

Why shadowing an error code?

This should be

	ret = dev_pm_domain_attach_list(port->se.dev,
					&port->dev_data->pd_data, &port->pd_list);
	if (ret < 0)
		return ret; // assuming it returns a Linux err code.
	/* Some comment, perhaps? */
	if (ret == 0)
		return -EINVAL;

	return 0;

> +}

...

>  		port->rx_buf = devm_kzalloc(uport->dev,
>  					    DMA_RX_BUF_SIZE, GFP_KERNEL);
> -		if (!port->rx_buf)
> -			return -ENOMEM;
> +		if (!port->rx_buf) {
> +			ret = -ENOMEM;
> +			goto error;

This is wrong. After devm_*() calls should not be goto:s. It should be very
exceptional cases otherwise.

> +		}

...

>  	port->name = devm_kasprintf(uport->dev, GFP_KERNEL,
>  			"qcom_geni_serial_%s%d",
>  			uart_console(uport) ? "console" : "uart", uport->line);
> -	if (!port->name)
> -		return -ENOMEM;
> +	if (!port->name) {
> +		ret = -ENOMEM;
> +		goto error;
> +	}

Ditto.

...

> +	if (irq < 0) {
> +		ret = irq;
> +		goto error;

Ditto. And so on...

> +	}

...

Hint: use devm_add_action_or_reset() above and drop most of the changes in this
patch.

-- 
With Best Regards,
Andy Shevchenko