RE: [PATCH v2] serial: samsung: Fixed wrong comparison for baudclk_rate
From: Kukjin Kim <hidden>
Date: 2012-05-17 06:55:03
Also in:
linux-arm-kernel, linux-samsung-soc
Russell King - ARM Linux wrote:
On Tue, May 15, 2012 at 09:37:16PM +0900, Kyoungil Kim wrote:quoted
port->baudclk_rate should be compared to the rate of port->baudclk, because port->baudclk_rate was assigned as the rate of port->baudclkpreviously.quoted
So to check that the current baudclk rate is same as previous rate, the target of comparison sholud be the rate of port->baudclk. Signed-off-by: Jun-Ho, Yoon <redacted> Signed-off-by: Kyoungil Kim <redacted> --- drivers/tty/serial/samsung.c | 4 ++-- 1 files changed, 2 insertions(+), 2 deletions(-)diff --git a/drivers/tty/serial/samsung.c b/drivers/tty/serial/samsung.c index d8b0aee..6a6a86a 100644 --- a/drivers/tty/serial/samsung.c +++ b/drivers/tty/serial/samsung.c@@ -1014,10 +1014,10 @@ static ints3c24xx_serial_cpufreq_transition(struct notifier_block *nb,quoted
* a disturbance in the clock-rate over the change. */ - if (IS_ERR(port->clk)) + if (IS_ERR(port->baudclk) || port->baudclk == NULL)Still no. Why are you wanting to detect a NULL baud clock? As I said, drivers have no business interpreting anything but IS_ERR(clk) as being an error. They should not make any other assumptions. Now that I look at this driver, it makes this mistake all over the place. This needs to be fixed. Something like the below should do it. Please check.
Russell, thanks for your kindly pointing out. Kyoungil, could you please re-submit for this as per Russell's suggestion (with Russell's patch)? Thanks. Best regards, Kgene. -- Kukjin Kim [off-list ref], Senior Engineer, SW Solution Development Team, Samsung Electronics Co., Ltd.
quoted hunk ↗ jump to hunk
diff --git a/drivers/tty/serial/samsung.c b/drivers/tty/serial/samsung.c index d8b0aee..6a952b1 100644 --- a/drivers/tty/serial/samsung.c +++ b/drivers/tty/serial/samsung.c@@ -529,7 +529,7 @@ static void s3c24xx_serial_pm(struct uart_port *port,unsigned int level, switch (level) { case 3: - if (!IS_ERR(ourport->baudclk) && ourport->baudclk != NULL) + if (!IS_ERR(ourport->baudclk)) clk_disable(ourport->baudclk); clk_disable(ourport->clk);@@ -538,7 +538,7 @@ static void s3c24xx_serial_pm(struct uart_port *port,unsigned int level, case 0: clk_enable(ourport->clk); - if (!IS_ERR(ourport->baudclk) && ourport->baudclk != NULL) + if (!IS_ERR(ourport->baudclk)) clk_enable(ourport->baudclk); break;@@ -713,9 +713,9 @@ static void s3c24xx_serial_set_termios(structuart_port *port, if (ourport->baudclk != clk) { s3c24xx_serial_setsource(port, clk_sel); - if (ourport->baudclk != NULL && !IS_ERR(ourport->baudclk)) { + if (!IS_ERR(ourport->baudclk)) { clk_disable(ourport->baudclk); - ourport->baudclk = NULL; + ourport->baudclk = ERR_PTR(-EINVAL); } clk_enable(clk);@@ -1160,6 +1160,9 @@ static ssize_t s3c24xx_serial_show_clksrc(structdevice *dev, struct uart_port *port = s3c24xx_dev_to_port(dev); struct s3c24xx_uart_port *ourport = to_ourport(port); + if (IS_ERR(ourport->baudclk)) + return -EINVAL; + return snprintf(buf, PAGE_SIZE, "* %s\n", ourport->baudclk->name); }@@ -1200,6 +1203,7 @@ static int s3c24xx_serial_probe(structplatform_device *pdev) return -ENODEV; } + ourport->baudclk = ERR_PTR(-EINVAL); ourport->info = ourport->drv_data->info; ourport->cfg = (pdev->dev.platform_data) ? (struct s3c2410_uartcfg *)pdev->dev.platform_data :