| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <mtdi5hpkrthohdvhtbojhhp6saip2uohv5343vpqwz2jssvskp@lpebfyxhqcjt>
Date: Tue, 17 Jun 2025 11:07:20 -0500
From: Bjorn Andersson <andersson@...nel.org>
To: Praveen Talari <quic_ptalari@...cinc.com>
Cc: Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
Jiri Slaby <jirislaby@...nel.org>, Rob Herring <robh@...nel.org>,
Krzysztof Kozlowski <krzk+dt@...nel.org>, Conor Dooley <conor+dt@...nel.org>,
Konrad Dybcio <konradybcio@...nel.org>, linux-arm-msm@...r.kernel.org, linux-kernel@...r.kernel.org,
linux-serial@...r.kernel.org, devicetree@...r.kernel.org,
Bryan O'Donoghue <bryan.odonoghue@...aro.org>, psodagud@...cinc.com, djaggi@...cinc.com,
quic_msavaliy@...cinc.com, quic_vtanuku@...cinc.com, quic_arandive@...cinc.com,
quic_mnaresh@...cinc.com, quic_shazhuss@...cinc.com
Subject: Re: [PATCH v6 6/8] serial: qcom-geni: move clock-rate logic to
separate function
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@...cinc.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
Powered by blists - more mailing lists