| Message ID | 20250506180232.1299-7-quic_ptalari@quicinc.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series | Enable QUPs and Serial on SA8255p Qualcomm platforms | expand |
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
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
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
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 --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
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(-)