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>
---
v5 -> v6
- used "unsigned int" instead of "unsigned long" in newly
  added API function params to avoid the format specifier
  warnings.

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 | 56 +++++++++++++++++----------
 1 file changed, 36 insertions(+), 20 deletions(-)

diff --git a/drivers/tty/serial/qcom_geni_serial.c b/drivers/tty/serial/qcom_geni_serial.c
index 715db35bab2f..b6fa7dc9b1fb 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 int 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 */
@@ -1317,7 +1304,7 @@ static void qcom_geni_serial_set_termios(struct uart_port *uport,
 		dev_err(port->se.dev,
 			"Couldn't find suitable clock rate for %u\n",
 			baud * sampling_rate);
-		return;
+		return -EINVAL;
 	}
 
 	dev_dbg(port->se.dev, "desired_rate = %u, clk_rate = %lu, clk_div = %u\n",
@@ -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
-- 
2.17.1

Hi Bryan,

Gentle reminder!!

Thanks,
Praveen Talari

On 6/6/2025 10:51 PM, Praveen Talari wrote:
> 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>
> ---
> v5 -> v6
> - used "unsigned int" instead of "unsigned long" in newly
>    added API function params to avoid the format specifier
>    warnings.
> 
> 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 | 56 +++++++++++++++++----------
>   1 file changed, 36 insertions(+), 20 deletions(-)
> 
> diff --git a/drivers/tty/serial/qcom_geni_serial.c b/drivers/tty/serial/qcom_geni_serial.c
> index 715db35bab2f..b6fa7dc9b1fb 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 int 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 */
> @@ -1317,7 +1304,7 @@ static void qcom_geni_serial_set_termios(struct uart_port *uport,
>   		dev_err(port->se.dev,
>   			"Couldn't find suitable clock rate for %u\n",
>   			baud * sampling_rate);
> -		return;
> +		return -EINVAL;
>   	}
>   
>   	dev_dbg(port->se.dev, "desired_rate = %u, clk_rate = %lu, clk_div = %u\n",
> @@ -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

On Mon, Jun 16, 2025 at 09:34:27PM +0530, Praveen Talari wrote:
> Hi Bryan,
> 
> Gentle reminder!!
> 

As I've told you all countless times, if you want attention to your
patchset review each others patches! For some reason you're the only one
showing interest in getting this series merged.

> Thanks,
> Praveen Talari
> 
> On 6/6/2025 10:51 PM, Praveen Talari wrote:
> > 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>
> > ---
> > v5 -> v6
> > - used "unsigned int" instead of "unsigned long" in newly
> >    added API function params to avoid the format specifier
> >    warnings.
> > 
> > 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 | 56 +++++++++++++++++----------
> >   1 file changed, 36 insertions(+), 20 deletions(-)
> > 
> > diff --git a/drivers/tty/serial/qcom_geni_serial.c b/drivers/tty/serial/qcom_geni_serial.c
> > index 715db35bab2f..b6fa7dc9b1fb 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 int 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 */
> > @@ -1317,7 +1304,7 @@ static void qcom_geni_serial_set_termios(struct uart_port *uport,
> >   		dev_err(port->se.dev,
> >   			"Couldn't find suitable clock rate for %u\n",
> >   			baud * sampling_rate);
> > -		return;
> > +		return -EINVAL;
> >   	}
> >   	dev_dbg(port->se.dev, "desired_rate = %u, clk_rate = %lu, clk_div = %u\n",
> > @@ -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);

As far as I can tell there's one error path in geni_serial_set_rate()
and there you already printed a more specific error message, as such
this doesn't add any value.

PS. In general, please don't use __func__, write helpful error messages
instead.

Regards,
Bjorn

> > +		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

HI Bjorn,

Thank you for review.

On 6/17/2025 9:37 PM, Bjorn Andersson wrote:
> On Mon, Jun 16, 2025 at 09:34:27PM +0530, Praveen Talari wrote:
>> Hi Bryan,
>>
>> Gentle reminder!!
>>
> 
> As I've told you all countless times, if you want attention to your
> patchset review each others patches! For some reason you're the only one
> showing interest in getting this series merged.

My intention is to CC Bryan with a polite reminder, one week after the 
initial post.

> 
>> Thanks,
>> Praveen Talari
>>
>> On 6/6/2025 10:51 PM, Praveen Talari wrote:
>>> 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>
>>> ---
>>> v5 -> v6
>>> - used "unsigned int" instead of "unsigned long" in newly
>>>     added API function params to avoid the format specifier
>>>     warnings.
>>>
>>> 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 | 56 +++++++++++++++++----------
>>>    1 file changed, 36 insertions(+), 20 deletions(-)
>>>
>>> diff --git a/drivers/tty/serial/qcom_geni_serial.c b/drivers/tty/serial/qcom_geni_serial.c
>>> index 715db35bab2f..b6fa7dc9b1fb 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 int 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 */
>>> @@ -1317,7 +1304,7 @@ static void qcom_geni_serial_set_termios(struct uart_port *uport,
>>>    		dev_err(port->se.dev,
>>>    			"Couldn't find suitable clock rate for %u\n",
>>>    			baud * sampling_rate);
>>> -		return;
>>> +		return -EINVAL;
>>>    	}
>>>    	dev_dbg(port->se.dev, "desired_rate = %u, clk_rate = %lu, clk_div = %u\n",
>>> @@ -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);
> 
> As far as I can tell there's one error path in geni_serial_set_rate()
> and there you already printed a more specific error message, as such
> this doesn't add any value.
Sure, will review and update in next patch-set.

Thanks,
Praveen Talari
> 
> PS. In general, please don't use __func__, write helpful error messages
> instead.
> 
> Regards,
> Bjorn
> 
>>> +		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

On 16/06/2025 18:04, Praveen Talari wrote:
> Hi Bryan,
> 
> Gentle reminder!!

That's neither gentle nor needed to ping multiple times, every 8 hours.

Best regards,
Krzysztof