diff mbox

[1/6] serial: 8250_dw: add support for clk api

Message ID 1363377988-4966-2-git-send-email-maxime.ripard@free-electrons.com
State New
Headers show

Commit Message

Maxime Ripard March 15, 2013, 8:06 p.m. UTC 
From: Emilio López <emilio@elopez.com.ar>

This commit implements support for using the clk api; this lets us use
the "clocks" property with device tree, instead of having to use
clock-frequency.

Signed-off-by: Emilio López <emilio@elopez.com.ar>
Signed-off-by: Maxime Ripard <maxime.ripard@free-electrons.com>
---
 drivers/tty/serial/8250/8250_dw.c |   33 ++++++++++++++++++++++++---------
 1 file changed, 24 insertions(+), 9 deletions(-)

Comments

Russell King - ARM Linux March 15, 2013, 10:39 p.m. UTC | #1 
On Fri, Mar 15, 2013 at 09:06:23PM +0100, Maxime Ripard wrote:
> +	/* clock got configured through clk api, all done */
> +	if (p->uartclk)

	if (IS_ERR(p->uartclk))

> +		return 0;
> +
> +	/* try to find out clock frequency from DT as fallback */
>  	if (of_property_read_u32(np, "clock-frequency", &val)) {
> -		dev_err(p->dev, "no clock-frequency property set\n");
> +		dev_err(p->dev, "clk or clock-frequency not defined\n");
>  		return -EINVAL;
>  	}
>  	p->uartclk = val;
> @@ -294,9 +301,21 @@ static int dw8250_probe(struct platform_device *pdev)
>  	if (!uart.port.membase)
>  		return -ENOMEM;
>  
> +	data = devm_kzalloc(&pdev->dev, sizeof(*data), GFP_KERNEL);
> +	if (!data)
> +		return -ENOMEM;
> +
> +	data->clk = devm_clk_get(&pdev->dev, NULL);
> +	if (IS_ERR(data->clk))
> +		data->clk = NULL;
> +	else
> +		clk_prepare_enable(data->clk);

	if (!IS_ERR(data->clk))
		clk_prepare_enable(data->clk);

> +
>  	uart.port.iotype = UPIO_MEM;
>  	uart.port.serial_in = dw8250_serial_in;
>  	uart.port.serial_out = dw8250_serial_out;
> +	uart.port.private_data = data;
> +	uart.port.uartclk = clk_get_rate(data->clk);

What if data->clk is invalid?

	if (!IS_ERR(data->clk)
		uart.port.uartclk = clk_get_rate(data->clk);

>  
>  	dw8250_setup_port(&uart);
>  
> @@ -312,12 +331,6 @@ static int dw8250_probe(struct platform_device *pdev)
>  		return -ENODEV;
>  	}
>  
> -	data = devm_kzalloc(&pdev->dev, sizeof(*data), GFP_KERNEL);
> -	if (!data)
> -		return -ENOMEM;
> -
> -	uart.port.private_data = data;
> -
>  	data->line = serial8250_register_8250_port(&uart);
>  	if (data->line < 0)
>  		return data->line;
> @@ -333,6 +346,8 @@ static int dw8250_remove(struct platform_device *pdev)
>  
>  	serial8250_unregister_port(data->line);
>  

	if (!IS_ERR(data->clk)

> +	clk_disable_unprepare(data->clk);
> +
>  	return 0;
>  }
>  
> -- 
> 1.7.10.4
> 
> 
> _______________________________________________
> linux-arm-kernel mailing list
> linux-arm-kernel@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
Emilio López March 16, 2013, 12:15 a.m. UTC | #2 
Hello Russell,

El 15/03/13 19:39, Russell King - ARM Linux escribió:
> On Fri, Mar 15, 2013 at 09:06:23PM +0100, Maxime Ripard wrote:
>> +	/* clock got configured through clk api, all done */
>> +	if (p->uartclk)
> 
> 	if (IS_ERR(p->uartclk))
> 

Isn't IS_ERR for pointers? p->uartclk is an unsigned int

>> +		return 0;
>> +
>> +	/* try to find out clock frequency from DT as fallback */
>>  	if (of_property_read_u32(np, "clock-frequency", &val)) {
>> -		dev_err(p->dev, "no clock-frequency property set\n");
>> +		dev_err(p->dev, "clk or clock-frequency not defined\n");
>>  		return -EINVAL;
>>  	}
>>  	p->uartclk = val;
>> @@ -294,9 +301,21 @@ static int dw8250_probe(struct platform_device *pdev)
>>  	if (!uart.port.membase)
>>  		return -ENOMEM;
>>  
>> +	data = devm_kzalloc(&pdev->dev, sizeof(*data), GFP_KERNEL);
>> +	if (!data)
>> +		return -ENOMEM;
>> +
>> +	data->clk = devm_clk_get(&pdev->dev, NULL);
>> +	if (IS_ERR(data->clk))
>> +		data->clk = NULL;
>> +	else
>> +		clk_prepare_enable(data->clk);
> 
> 	if (!IS_ERR(data->clk))
> 		clk_prepare_enable(data->clk);
> 

See below

>> +
>>  	uart.port.iotype = UPIO_MEM;
>>  	uart.port.serial_in = dw8250_serial_in;
>>  	uart.port.serial_out = dw8250_serial_out;
>> +	uart.port.private_data = data;
>> +	uart.port.uartclk = clk_get_rate(data->clk);
> 
> What if data->clk is invalid?
> 
> 	if (!IS_ERR(data->clk)
> 		uart.port.uartclk = clk_get_rate(data->clk);
> 

I'm not sure if it is coincidental or the way it is supposed to be, but
when using the common clock framework, if you pass a NULL to
clk_get_rate, the function explicitly checks for it and returns 0. I
relied on that behaviour when implementing this; see the if..else block
above. Is this not always the case on other clock drivers?

>>  
>>  	dw8250_setup_port(&uart);
>>  
>> @@ -312,12 +331,6 @@ static int dw8250_probe(struct platform_device *pdev)
>>  		return -ENODEV;
>>  	}
>>  
>> -	data = devm_kzalloc(&pdev->dev, sizeof(*data), GFP_KERNEL);
>> -	if (!data)
>> -		return -ENOMEM;
>> -
>> -	uart.port.private_data = data;
>> -
>>  	data->line = serial8250_register_8250_port(&uart);
>>  	if (data->line < 0)
>>  		return data->line;
>> @@ -333,6 +346,8 @@ static int dw8250_remove(struct platform_device *pdev)
>>  
>>  	serial8250_unregister_port(data->line);
>>  
> 
> 	if (!IS_ERR(data->clk)
> 

I just double checked and clk_enable/disable, clk_prepare/unprepare also
ignore NULLs passed to them on the CCF.

>> +	clk_disable_unprepare(data->clk);
>> +
>>  	return 0;
>>  }
>>  
>> -- 
>> 1.7.10.4

Thanks for the review,

Emilio
Russell King - ARM Linux March 16, 2013, 12:29 a.m. UTC | #3 
On Fri, Mar 15, 2013 at 09:15:11PM -0300, Emilio López wrote:
> Hello Russell,
> 
> El 15/03/13 19:39, Russell King - ARM Linux escribió:
> > On Fri, Mar 15, 2013 at 09:06:23PM +0100, Maxime Ripard wrote:
> >> +	/* clock got configured through clk api, all done */
> >> +	if (p->uartclk)
> > 
> > 	if (IS_ERR(p->uartclk))
> > 
> 
> Isn't IS_ERR for pointers? p->uartclk is an unsigned int

Right, sorry, ignore that.

> >> +		return 0;
> >> +
> >> +	/* try to find out clock frequency from DT as fallback */
> >>  	if (of_property_read_u32(np, "clock-frequency", &val)) {
> >> -		dev_err(p->dev, "no clock-frequency property set\n");
> >> +		dev_err(p->dev, "clk or clock-frequency not defined\n");
> >>  		return -EINVAL;
> >>  	}
> >>  	p->uartclk = val;
> >> @@ -294,9 +301,21 @@ static int dw8250_probe(struct platform_device *pdev)
> >>  	if (!uart.port.membase)
> >>  		return -ENOMEM;
> >>  
> >> +	data = devm_kzalloc(&pdev->dev, sizeof(*data), GFP_KERNEL);
> >> +	if (!data)
> >> +		return -ENOMEM;
> >> +
> >> +	data->clk = devm_clk_get(&pdev->dev, NULL);
> >> +	if (IS_ERR(data->clk))
> >> +		data->clk = NULL;
> >> +	else
> >> +		clk_prepare_enable(data->clk);
> > 
> > 	if (!IS_ERR(data->clk))
> > 		clk_prepare_enable(data->clk);
> > 
> 
> See below
> 
> >> +
> >>  	uart.port.iotype = UPIO_MEM;
> >>  	uart.port.serial_in = dw8250_serial_in;
> >>  	uart.port.serial_out = dw8250_serial_out;
> >> +	uart.port.private_data = data;
> >> +	uart.port.uartclk = clk_get_rate(data->clk);
> > 
> > What if data->clk is invalid?
> > 
> > 	if (!IS_ERR(data->clk)
> > 		uart.port.uartclk = clk_get_rate(data->clk);
> > 
> 
> I'm not sure if it is coincidental or the way it is supposed to be, but
> when using the common clock framework, if you pass a NULL to
> clk_get_rate, the function explicitly checks for it and returns 0. I
> relied on that behaviour when implementing this; see the if..else block
> above. Is this not always the case on other clock drivers?

That's something that the common clock framework decided to do.  It's
not a defined part of the API though, so drivers shouldn't rely on
this behaviour meaning anything special.
Emilio López March 16, 2013, 12:40 a.m. UTC | #4 
El 15/03/13 21:29, Russell King - ARM Linux escribió:
> On Fri, Mar 15, 2013 at 09:15:11PM -0300, Emilio López wrote:
>> Hello Russell,
>>
>> El 15/03/13 19:39, Russell King - ARM Linux escribió:
>>> On Fri, Mar 15, 2013 at 09:06:23PM +0100, Maxime Ripard wrote:
>>>> +	/* clock got configured through clk api, all done */
>>>> +	if (p->uartclk)
>>>
>>> 	if (IS_ERR(p->uartclk))
>>>
>>
>> Isn't IS_ERR for pointers? p->uartclk is an unsigned int
> 
> Right, sorry, ignore that.
> 
>>>> +		return 0;
>>>> +
>>>> +	/* try to find out clock frequency from DT as fallback */
>>>>  	if (of_property_read_u32(np, "clock-frequency", &val)) {
>>>> -		dev_err(p->dev, "no clock-frequency property set\n");
>>>> +		dev_err(p->dev, "clk or clock-frequency not defined\n");
>>>>  		return -EINVAL;
>>>>  	}
>>>>  	p->uartclk = val;
>>>> @@ -294,9 +301,21 @@ static int dw8250_probe(struct platform_device *pdev)
>>>>  	if (!uart.port.membase)
>>>>  		return -ENOMEM;
>>>>  
>>>> +	data = devm_kzalloc(&pdev->dev, sizeof(*data), GFP_KERNEL);
>>>> +	if (!data)
>>>> +		return -ENOMEM;
>>>> +
>>>> +	data->clk = devm_clk_get(&pdev->dev, NULL);
>>>> +	if (IS_ERR(data->clk))
>>>> +		data->clk = NULL;
>>>> +	else
>>>> +		clk_prepare_enable(data->clk);
>>>
>>> 	if (!IS_ERR(data->clk))
>>> 		clk_prepare_enable(data->clk);
>>>
>>
>> See below
>>
>>>> +
>>>>  	uart.port.iotype = UPIO_MEM;
>>>>  	uart.port.serial_in = dw8250_serial_in;
>>>>  	uart.port.serial_out = dw8250_serial_out;
>>>> +	uart.port.private_data = data;
>>>> +	uart.port.uartclk = clk_get_rate(data->clk);
>>>
>>> What if data->clk is invalid?
>>>
>>> 	if (!IS_ERR(data->clk)
>>> 		uart.port.uartclk = clk_get_rate(data->clk);
>>>
>>
>> I'm not sure if it is coincidental or the way it is supposed to be, but
>> when using the common clock framework, if you pass a NULL to
>> clk_get_rate, the function explicitly checks for it and returns 0. I
>> relied on that behaviour when implementing this; see the if..else block
>> above. Is this not always the case on other clock drivers?
> 
> That's something that the common clock framework decided to do.  It's
> not a defined part of the API though, so drivers shouldn't rely on
> this behaviour meaning anything special.

Ok then, I'll rework the error checking on the clock calls and get a new
patch sent. Thanks for the clarification.

Emilio
diff mbox

Patch

diff --git a/drivers/tty/serial/8250/8250_dw.c b/drivers/tty/serial/8250/8250_dw.c
index db0e66f..9a834a1 100644
--- a/drivers/tty/serial/8250/8250_dw.c
+++ b/drivers/tty/serial/8250/8250_dw.c
@@ -26,6 +26,7 @@ 
 #include <linux/platform_device.h>
 #include <linux/slab.h>
 #include <linux/acpi.h>
+#include <linux/clk.h>
 
 #include "8250.h"
 
@@ -55,8 +56,9 @@ 
 
 
 struct dw8250_data {
-	int	last_lcr;
-	int	line;
+	int		last_lcr;
+	int		line;
+	struct clk	*clk;
 };
 
 static void dw8250_serial_out(struct uart_port *p, int offset, int value)
@@ -136,8 +138,13 @@  static int dw8250_probe_of(struct uart_port *p)
 	if (!of_property_read_u32(np, "reg-shift", &val))
 		p->regshift = val;
 
+	/* clock got configured through clk api, all done */
+	if (p->uartclk)
+		return 0;
+
+	/* try to find out clock frequency from DT as fallback */
 	if (of_property_read_u32(np, "clock-frequency", &val)) {
-		dev_err(p->dev, "no clock-frequency property set\n");
+		dev_err(p->dev, "clk or clock-frequency not defined\n");
 		return -EINVAL;
 	}
 	p->uartclk = val;
@@ -294,9 +301,21 @@  static int dw8250_probe(struct platform_device *pdev)
 	if (!uart.port.membase)
 		return -ENOMEM;
 
+	data = devm_kzalloc(&pdev->dev, sizeof(*data), GFP_KERNEL);
+	if (!data)
+		return -ENOMEM;
+
+	data->clk = devm_clk_get(&pdev->dev, NULL);
+	if (IS_ERR(data->clk))
+		data->clk = NULL;
+	else
+		clk_prepare_enable(data->clk);
+
 	uart.port.iotype = UPIO_MEM;
 	uart.port.serial_in = dw8250_serial_in;
 	uart.port.serial_out = dw8250_serial_out;
+	uart.port.private_data = data;
+	uart.port.uartclk = clk_get_rate(data->clk);
 
 	dw8250_setup_port(&uart);
 
@@ -312,12 +331,6 @@  static int dw8250_probe(struct platform_device *pdev)
 		return -ENODEV;
 	}
 
-	data = devm_kzalloc(&pdev->dev, sizeof(*data), GFP_KERNEL);
-	if (!data)
-		return -ENOMEM;
-
-	uart.port.private_data = data;
-
 	data->line = serial8250_register_8250_port(&uart);
 	if (data->line < 0)
 		return data->line;
@@ -333,6 +346,8 @@  static int dw8250_remove(struct platform_device *pdev)
 
 	serial8250_unregister_port(data->line);
 
+	clk_disable_unprepare(data->clk);
+
 	return 0;
 }