Hi Dmitry/Bjorn/Krzysztof,

Thank you for review.

On 7/1/2025 4:42 PM, Dmitry Baryshkov wrote:

On Mon, Jun 30, 2025 at 10:40:25AM +0530, Praveen Talari wrote:

quoted

Hi Bjorn,

Thank you for review.

On 6/17/2025 9:23 PM, Bjorn Andersson wrote:

quoted

On Fri, Jun 06, 2025 at 10:51:13PM +0530, Praveen Talari wrote:

quoted

Add Power Management (PM) runtime support to Qualcomm GENI
serial driver.

Doesn't this have impact on the behavior outside of your
project? Or is the transition from qcom_geni_serial_pm() to explicit
RPM merely moving code around?

Seems like this deserves to not be hidden in a middle of a patch series.

quoted

Introduce necessary callbacks and updates to ensure seamless
transitions between power states, enhancing overall power
efficiency.

This commit message fails to state why we need runtime PM support in the
driver.

Introduce PM runtime support to the Qualcomm GENI serial driver to enable
better power efficiency and modularity across diverse resource control
mechanisms, including Linux and firmware-managed systems.

As part of this enhancement, the existing qcom_geni_serial_pm() logic to
use standard PM runtime APIs such as pm_runtime_resume_and_get() and
pm_runtime_put_sync(). Power state transitions are now handled through
the geni_serial_resources_on() and geni_serial_resources_off() functions.

Is it fine?
Please guide me/correct me if needed

Please start by stating out the problem that you are trying to solve.
There is no actual issue description in your patch.

I hope this commit describes the actual problem.

The GENI serial driver currently handles power resource management
through calls to the statically defined geni_serial_resources_on() and
geni_serial_resources_off() functions. This approach reduces modularity
and limits support for platforms with diverse power management 
mechanisms, including resource managed by firmware.

Improve modularity and enable better integration with platform-specific
power management, introduce support for runtime PM. Use
pm_runtime_resume_and_get() and pm_runtime_put_sync() within the
qcom_geni_serial_pm() callback to control resource power state 
transitions based on UART power state changes.

Thanks,
Praveen Talari

quoted

Thanks,
Praveen Talari

quoted

Also, start your commit message with a problem description, per
https://docs.kernel.org/process/submitting-patches.html#describe-your-changes

quoted

Reviewed-by: Bryan O'Donoghue <bryan.odonoghue@linaro.org>
Signed-off-by: Praveen Talari <redacted>
---
v5 -> v6
- added reviewed-by tag in commit
- added __maybe_unused to PM callback functions to avoid
    warnings of defined but not used
---
   drivers/tty/serial/qcom_geni_serial.c | 33 +++++++++++++++++++++++----
   1 file changed, 29 insertions(+), 4 deletions(-)

diff --git a/drivers/tty/serial/qcom_geni_serial.c b/drivers/tty/serial/qcom_geni_serial.c
index b6fa7dc9b1fb..3691340ce7e8 100644
--- a/drivers/tty/serial/qcom_geni_serial.c
+++ b/drivers/tty/serial/qcom_geni_serial.c

@@ -1686,10 +1686,10 @@ static void qcom_geni_serial_pm(struct uart_port *uport,
   		old_state = UART_PM_STATE_OFF;
   	if (new_state == UART_PM_STATE_ON && old_state == UART_PM_STATE_OFF)
-		geni_serial_resources_on(uport);
+		pm_runtime_resume_and_get(uport->dev);
   	else if (new_state == UART_PM_STATE_OFF &&
   		 old_state == UART_PM_STATE_ON)
-		geni_serial_resources_off(uport);
+		pm_runtime_put_sync(uport->dev);
   }

@@ -1827,9 +1827,11 @@ static int qcom_geni_serial_probe(struct platform_device *pdev)
   		return ret;
   	}
+	pm_runtime_enable(port->se.dev);

Any reason not to use devm_pm_runtime_enable() and avoid the
two pm_runtime_disable() below?

Regards,
Bjorn

quoted

+
   	ret = uart_add_one_port(drv, uport);
   	if (ret)
-		return ret;
+		goto error;
   	if (port->wakeup_irq > 0) {
   		device_init_wakeup(&pdev->dev, true);

@@ -1839,11 +1841,15 @@ static int qcom_geni_serial_probe(struct platform_device *pdev)
   			device_init_wakeup(&pdev->dev, false);
   			ida_free(&port_ida, uport->line);
   			uart_remove_one_port(drv, uport);
-			return ret;
+			goto error;
   		}
   	}
   	return 0;
+
+error:
+	pm_runtime_disable(port->se.dev);
+	return ret;
   }
   static void qcom_geni_serial_remove(struct platform_device *pdev)

@@ -1855,9 +1861,26 @@ static void qcom_geni_serial_remove(struct platform_device *pdev)
   	dev_pm_clear_wake_irq(&pdev->dev);
   	device_init_wakeup(&pdev->dev, false);
   	ida_free(&port_ida, uport->line);
+	pm_runtime_disable(port->se.dev);
   	uart_remove_one_port(drv, &port->uport);
   }
+static int __maybe_unused qcom_geni_serial_runtime_suspend(struct device *dev)
+{
+	struct qcom_geni_serial_port *port = dev_get_drvdata(dev);
+	struct uart_port *uport = &port->uport;
+
+	return geni_serial_resources_off(uport);
+}
+
+static int __maybe_unused qcom_geni_serial_runtime_resume(struct device *dev)
+{
+	struct qcom_geni_serial_port *port = dev_get_drvdata(dev);
+	struct uart_port *uport = &port->uport;
+
+	return geni_serial_resources_on(uport);
+}
+
   static int qcom_geni_serial_suspend(struct device *dev)
   {
   	struct qcom_geni_serial_port *port = dev_get_drvdata(dev);

@@ -1901,6 +1924,8 @@ static const struct qcom_geni_device_data qcom_geni_uart_data = {
   };
   static const struct dev_pm_ops qcom_geni_serial_pm_ops = {
+	SET_RUNTIME_PM_OPS(qcom_geni_serial_runtime_suspend,
+			   qcom_geni_serial_runtime_resume, NULL)
   	SYSTEM_SLEEP_PM_OPS(qcom_geni_serial_suspend, qcom_geni_serial_resume)
   };

-- 
2.17.1