diff mbox series

[v5,6/8] serial: qcom-geni: move clock-rate logic to separate function

Message ID 20250506180232.1299-7-quic_ptalari@quicinc.com
State Superseded
Headers show
Series Enable QUPs and Serial on SA8255p Qualcomm platforms | expand

Commit Message

Praveen Talari May 6, 2025, 6:02 p.m. UTC
Facilitates future modifications within the new function,
leading to better readability and maintainability of the code.

Move the code that handles the actual logic of clock-rate
calculations to a separate function geni_serial_set_rate()
which enhances code readability.

Signed-off-by: Praveen Talari <quic_ptalari@quicinc.com>
---
v3 -> v4
- added version log after ---

v1 -> v2
- resolved build warnings for datatype format specifiers
- removed double spaces in log
---
 drivers/tty/serial/qcom_geni_serial.c | 62 +++++++++++++++++----------
 1 file changed, 39 insertions(+), 23 deletions(-)

Comments

Bryan O'Donoghue June 3, 2025, 2:41 p.m. UTC | #1
On 06/05/2025 19:02, Praveen Talari wrote:
> -			"Couldn't find suitable clock rate for %u\n",
> +			"Couldn't find suitable clock rate for %lu\n",
>   			baud * sampling_rate);
> -		return;
> +		return -EINVAL;
>   	}
> 
> -	dev_dbg(port->se.dev, "desired_rate = %u, clk_rate = %lu, clk_div = %u\n",
> -			baud * sampling_rate, clk_rate, clk_div);
> +	dev_dbg(port->se.dev, "desired_rate = %lu, clk_rate = %lu, clk_div = %u\n",
> +		baud * sampling_rate, clk_rate, clk_div);

Separate this stuff out.

Your code should match the commit log. If you want to convert %u to %lu 
make a patch to do that, even if it seems trivial, it is better to make 
granular submissions.

---
bod
Praveen Talari June 4, 2025, 5:11 p.m. UTC | #2
Hi Bryan,

On 6/3/2025 8:11 PM, Bryan O'Donoghue wrote:
> On 06/05/2025 19:02, Praveen Talari wrote:
>> -            "Couldn't find suitable clock rate for %u\n",
>> +            "Couldn't find suitable clock rate for %lu\n",
>>               baud * sampling_rate);
>> -        return;
>> +        return -EINVAL;
>>       }
>>
>> -    dev_dbg(port->se.dev, "desired_rate = %u, clk_rate = %lu, clk_div 
>> = %u\n",
>> -            baud * sampling_rate, clk_rate, clk_div);
>> +    dev_dbg(port->se.dev, "desired_rate = %lu, clk_rate = %lu, 
>> clk_div = %u\n",
>> +        baud * sampling_rate, clk_rate, clk_div);
> 
> Separate this stuff out.
> 
> Your code should match the commit log. If you want to convert %u to %lu 
> make a patch to do that, even if it seems trivial, it is better to make 
> granular submissions.

It comes under newly added API. Do we still need to make separate patch?

Thanks,
Praveen Talari
> 
> ---
> bod
Bryan O'Donoghue June 4, 2025, 11:49 p.m. UTC | #3
On 04/06/2025 18:11, Praveen Talari wrote:
>> Separate this stuff out.
>>
>> Your code should match the commit log. If you want to convert %u to 
>> %lu make a patch to do that, even if it seems trivial, it is better to 
>> make granular submissions.
> 
> It comes under newly added API. Do we still need to make separate patch?

Best practice is to split this stuff up.

If your commit log says "I'm moving code" then it should _only_ move 
code, don't sneak any other changes in, no matter how seemingly innocuous.

---
bod
Praveen Talari June 5, 2025, 3:34 a.m. UTC | #4
Hi Bryan,

Thank you for your inputs.

On 6/5/2025 5:19 AM, Bryan O'Donoghue wrote:
> On 04/06/2025 18:11, Praveen Talari wrote:
>>> Separate this stuff out.
>>>
>>> Your code should match the commit log. If you want to convert %u to 
>>> %lu make a patch to do that, even if it seems trivial, it is better 
>>> to make granular submissions.
>>
>> It comes under newly added API. Do we still need to make separate patch?
> 
> Best practice is to split this stuff up.
We can avoid this change by using unsigned int instead of unsigned long 
in newly added API function params.

Will fix in next version.

Thanks,
Praveen Talari
> 
> If your commit log says "I'm moving code" then it should _only_ move 
> code, don't sneak any other changes in, no matter how seemingly innocuous.
> 
> ---
> bod
diff mbox series

Patch

diff --git a/drivers/tty/serial/qcom_geni_serial.c b/drivers/tty/serial/qcom_geni_serial.c
index 2cd2085473f3..60afee3884a6 100644
--- a/drivers/tty/serial/qcom_geni_serial.c
+++ b/drivers/tty/serial/qcom_geni_serial.c
@@ -1283,27 +1283,14 @@  static unsigned long get_clk_div_rate(struct clk *clk, unsigned int baud,
 	return ser_clk;
 }
 
-static void qcom_geni_serial_set_termios(struct uart_port *uport,
-					 struct ktermios *termios,
-					 const struct ktermios *old)
+static int geni_serial_set_rate(struct uart_port *uport, unsigned long baud)
 {
-	unsigned int baud;
-	u32 bits_per_char;
-	u32 tx_trans_cfg;
-	u32 tx_parity_cfg;
-	u32 rx_trans_cfg;
-	u32 rx_parity_cfg;
-	u32 stop_bit_len;
-	unsigned int clk_div;
-	u32 ser_clk_cfg;
 	struct qcom_geni_serial_port *port = to_dev_port(uport);
 	unsigned long clk_rate;
-	u32 ver, sampling_rate;
 	unsigned int avg_bw_core;
-	unsigned long timeout;
-
-	/* baud rate */
-	baud = uart_get_baud_rate(uport, termios, old, 300, 4000000);
+	unsigned int clk_div;
+	u32 ver, sampling_rate;
+	u32 ser_clk_cfg;
 
 	sampling_rate = UART_OVERSAMPLING;
 	/* Sampling rate is halved for IP versions >= 2.5 */
@@ -1315,13 +1302,13 @@  static void qcom_geni_serial_set_termios(struct uart_port *uport,
 		sampling_rate, &clk_div);
 	if (!clk_rate) {
 		dev_err(port->se.dev,
-			"Couldn't find suitable clock rate for %u\n",
+			"Couldn't find suitable clock rate for %lu\n",
 			baud * sampling_rate);
-		return;
+		return -EINVAL;
 	}
 
-	dev_dbg(port->se.dev, "desired_rate = %u, clk_rate = %lu, clk_div = %u\n",
-			baud * sampling_rate, clk_rate, clk_div);
+	dev_dbg(port->se.dev, "desired_rate = %lu, clk_rate = %lu, clk_div = %u\n",
+		baud * sampling_rate, clk_rate, clk_div);
 
 	uport->uartclk = clk_rate;
 	port->clk_rate = clk_rate;
@@ -1339,6 +1326,37 @@  static void qcom_geni_serial_set_termios(struct uart_port *uport,
 	port->se.icc_paths[CPU_TO_GENI].avg_bw = Bps_to_icc(baud);
 	geni_icc_set_bw(&port->se);
 
+	writel(ser_clk_cfg, uport->membase + GENI_SER_M_CLK_CFG);
+	writel(ser_clk_cfg, uport->membase + GENI_SER_S_CLK_CFG);
+	return 0;
+}
+
+static void qcom_geni_serial_set_termios(struct uart_port *uport,
+					 struct ktermios *termios,
+					 const struct ktermios *old)
+{
+	struct qcom_geni_serial_port *port = to_dev_port(uport);
+	unsigned int baud;
+	unsigned long timeout;
+	u32 bits_per_char;
+	u32 tx_trans_cfg;
+	u32 tx_parity_cfg;
+	u32 rx_trans_cfg;
+	u32 rx_parity_cfg;
+	u32 stop_bit_len;
+	int ret = 0;
+
+	/* baud rate */
+	baud = uart_get_baud_rate(uport, termios, old, 300, 4000000);
+
+	ret = geni_serial_set_rate(uport, baud);
+	if (ret) {
+		dev_err(port->se.dev,
+			"%s: Failed to set baud:%u ret:%d\n",
+			__func__, baud, ret);
+		return;
+	}
+
 	/* parity */
 	tx_trans_cfg = readl(uport->membase + SE_UART_TX_TRANS_CFG);
 	tx_parity_cfg = readl(uport->membase + SE_UART_TX_PARITY_CFG);
@@ -1406,8 +1424,6 @@  static void qcom_geni_serial_set_termios(struct uart_port *uport,
 	writel(bits_per_char, uport->membase + SE_UART_TX_WORD_LEN);
 	writel(bits_per_char, uport->membase + SE_UART_RX_WORD_LEN);
 	writel(stop_bit_len, uport->membase + SE_UART_TX_STOP_BIT_LEN);
-	writel(ser_clk_cfg, uport->membase + GENI_SER_M_CLK_CFG);
-	writel(ser_clk_cfg, uport->membase + GENI_SER_S_CLK_CFG);
 }
 
 #ifdef CONFIG_SERIAL_QCOM_GENI_CONSOLE