Supports use in PM system/runtime frameworks, helping to
distinguish new resource control mechanisms and facilitate
future modifications within the new API.

The code that handles the actual enable or disable of resources
like clock and ICC paths to a separate function
(geni_serial_resources_on() and geni_serial_resources_off()) which
enhances code readability.

Signed-off-by: Praveen Talari <quic_ptalari@quicinc.com>
---
 drivers/tty/serial/qcom_geni_serial.c | 53 +++++++++++++++++++++------
 1 file changed, 42 insertions(+), 11 deletions(-)

diff --git a/drivers/tty/serial/qcom_geni_serial.c b/drivers/tty/serial/qcom_geni_serial.c
index 889ce8961e0a..e341f5090ecc 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_resources_off(struct uart_port *uport)
+{
+	struct qcom_geni_serial_port *port = to_dev_port(uport);
+	int ret;
+
+	dev_pm_opp_set_rate(uport->dev, 0);
+	ret = geni_se_resources_off(&port->se);
+	if (ret)
+		return ret;
+
+	geni_icc_disable(&port->se);
+
+	return ret;
+}
+
+static int geni_serial_resources_on(struct uart_port *uport)
+{
+	struct qcom_geni_serial_port *port = to_dev_port(uport);
+	int ret;
+
+	ret = geni_icc_enable(&port->se);
+	if (ret)
+		return ret;
+
+	ret = geni_se_resources_on(&port->se);
+	if (ret) {
+		geni_icc_disable(&port->se);
+		return ret;
+	}
+
+	if (port->clk_rate)
+		dev_pm_opp_set_rate(uport->dev, port->clk_rate);
+
+	return ret;
+}
+
 static int geni_serial_resource_init(struct qcom_geni_serial_port *port)
 {
 	int ret;
@@ -1617,17 +1653,12 @@ static void qcom_geni_serial_pm(struct uart_port *uport,
 	if (old_state == UART_PM_STATE_UNDEFINED)
 		old_state = UART_PM_STATE_OFF;
 
-	if (new_state == UART_PM_STATE_ON && old_state == UART_PM_STATE_OFF) {
-		geni_icc_enable(&port->se);
-		if (port->clk_rate)
-			dev_pm_opp_set_rate(uport->dev, port->clk_rate);
-		geni_se_resources_on(&port->se);
-	} else if (new_state == UART_PM_STATE_OFF &&
-			old_state == UART_PM_STATE_ON) {
-		geni_se_resources_off(&port->se);
-		dev_pm_opp_set_rate(uport->dev, 0);
-		geni_icc_disable(&port->se);
-	}
+	if (new_state == UART_PM_STATE_ON && old_state == UART_PM_STATE_OFF)
+		geni_serial_resources_on(uport);
+	else if (new_state == UART_PM_STATE_OFF &&
+			old_state == UART_PM_STATE_ON)
+		geni_serial_resources_off(uport);
+
 }
 
 static const struct uart_ops qcom_geni_console_pops = {
-- 
2.17.1

On 10. 04. 25, 19:40, Praveen Talari wrote:
> Supports use in PM system/runtime frameworks, helping to
> distinguish new resource control mechanisms and facilitate
> future modifications within the new API.
> 
> The code that handles the actual enable or disable of resources
> like clock and ICC paths to a separate function
> (geni_serial_resources_on() and geni_serial_resources_off()) which
> enhances code readability.
> 
> Signed-off-by: Praveen Talari <quic_ptalari@quicinc.com>
> ---
>   drivers/tty/serial/qcom_geni_serial.c | 53 +++++++++++++++++++++------
>   1 file changed, 42 insertions(+), 11 deletions(-)
> 
> diff --git a/drivers/tty/serial/qcom_geni_serial.c b/drivers/tty/serial/qcom_geni_serial.c
> index 889ce8961e0a..e341f5090ecc 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_resources_off(struct uart_port *uport)
> +{
> +	struct qcom_geni_serial_port *port = to_dev_port(uport);
> +	int ret;
> +
> +	dev_pm_opp_set_rate(uport->dev, 0);
> +	ret = geni_se_resources_off(&port->se);
> +	if (ret)
> +		return ret;
> +
> +	geni_icc_disable(&port->se);
> +
> +	return ret;

This is a bit confusing (needs context). return 0 directly.

> +}
> +
> +static int geni_serial_resources_on(struct uart_port *uport)
> +{
> +	struct qcom_geni_serial_port *port = to_dev_port(uport);
> +	int ret;
> +
> +	ret = geni_icc_enable(&port->se);
> +	if (ret)
> +		return ret;
> +
> +	ret = geni_se_resources_on(&port->se);
> +	if (ret) {
> +		geni_icc_disable(&port->se);
> +		return ret;
> +	}
> +
> +	if (port->clk_rate)
> +		dev_pm_opp_set_rate(uport->dev, port->clk_rate);
> +
> +	return ret;

Same here.

thanks,
-- 
js
suse labs

Hi

On 4/14/2025 1:29 PM, Jiri Slaby wrote:
> On 10. 04. 25, 19:40, Praveen Talari wrote:
>> Supports use in PM system/runtime frameworks, helping to
>> distinguish new resource control mechanisms and facilitate
>> future modifications within the new API.
>>
>> The code that handles the actual enable or disable of resources
>> like clock and ICC paths to a separate function
>> (geni_serial_resources_on() and geni_serial_resources_off()) which
>> enhances code readability.
>>
>> Signed-off-by: Praveen Talari <quic_ptalari@quicinc.com>
>> ---
>>   drivers/tty/serial/qcom_geni_serial.c | 53 +++++++++++++++++++++------
>>   1 file changed, 42 insertions(+), 11 deletions(-)
>>
>> diff --git a/drivers/tty/serial/qcom_geni_serial.c 
>> b/drivers/tty/serial/qcom_geni_serial.c
>> index 889ce8961e0a..e341f5090ecc 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_resources_off(struct uart_port *uport)
>> +{
>> +    struct qcom_geni_serial_port *port = to_dev_port(uport);
>> +    int ret;
>> +
>> +    dev_pm_opp_set_rate(uport->dev, 0);
>> +    ret = geni_se_resources_off(&port->se);
>> +    if (ret)
>> +        return ret;
>> +
>> +    geni_icc_disable(&port->se);
>> +
>> +    return ret;
>
> This is a bit confusing (needs context). return 0 directly.
here "ret" is also pointing to 0. Why can't we use "ret" directly 
instead of 0.
>
>> +}
>> +
>> +static int geni_serial_resources_on(struct uart_port *uport)
>> +{
>> +    struct qcom_geni_serial_port *port = to_dev_port(uport);
>> +    int ret;
>> +
>> +    ret = geni_icc_enable(&port->se);
>> +    if (ret)
>> +        return ret;
>> +
>> +    ret = geni_se_resources_on(&port->se);
>> +    if (ret) {
>> +        geni_icc_disable(&port->se);
>> +        return ret;
>> +    }
>> +
>> +    if (port->clk_rate)
>> +        dev_pm_opp_set_rate(uport->dev, port->clk_rate);
>> +
>> +    return ret;
>
> Same here.
>
> thanks,