[v2,6/7] i2c: pxa: support i2c controller from DT
diff mbox

Message ID 1311835293-18125-7-git-send-email-haojian.zhuang@marvell.com
State New, archived
Headers show

Commit Message

Haojian Zhuang July 28, 2011, 6:41 a.m. UTC
support i2c-pxa controller from DT.

Signed-off-by: Haojian Zhuang <haojian.zhuang@marvell.com>
---
 drivers/i2c/busses/i2c-pxa.c |   51 +++++++++++++++++++++++++++++++----------
 1 files changed, 38 insertions(+), 13 deletions(-)

Comments

Grant Likely July 29, 2011, 4:52 p.m. UTC | #1
On Thu, Jul 28, 2011 at 02:41:32PM +0800, Haojian Zhuang wrote:
> support i2c-pxa controller from DT.
> 
> Signed-off-by: Haojian Zhuang <haojian.zhuang@marvell.com>
> ---
>  drivers/i2c/busses/i2c-pxa.c |   51 +++++++++++++++++++++++++++++++----------
>  1 files changed, 38 insertions(+), 13 deletions(-)
> 
> diff --git a/drivers/i2c/busses/i2c-pxa.c b/drivers/i2c/busses/i2c-pxa.c
> index d603646..8c27733 100644
> --- a/drivers/i2c/busses/i2c-pxa.c
> +++ b/drivers/i2c/busses/i2c-pxa.c
> @@ -29,6 +29,7 @@
>  #include <linux/errno.h>
>  #include <linux/interrupt.h>
>  #include <linux/i2c-pxa.h>
> +#include <linux/of_device.h>
>  #include <linux/of_i2c.h>
>  #include <linux/platform_device.h>
>  #include <linux/err.h>
> @@ -1044,15 +1045,31 @@ static const struct i2c_algorithm i2c_pxa_pio_algorithm = {
>  	.functionality	= i2c_pxa_functionality,
>  };
>  
> +static const struct of_device_id pxa_i2c_of_match[] = {
> +	{ .compatible = "mrvl,pxa255-i2c",    .data = (void *)REGS_PXA2XX, },
> +	{ .compatible = "mrvl,pxa300-pwri2c", .data = (void *)REGS_PXA3XX, },
> +	{},
> +};
> +
>  static int i2c_pxa_probe(struct platform_device *dev)
>  {
> +	struct device_node *np = dev->dev.of_node;
> +	const struct of_device_id *match;
>  	struct pxa_i2c *i2c;
> -	struct resource *res;
>  	struct i2c_pxa_platform_data *plat = dev->dev.platform_data;
>  	const struct platform_device_id *id = platform_get_device_id(dev);
> -	enum pxa_i2c_types i2c_type = id->driver_data;
> -	int ret;
> -	int irq;
> +	enum pxa_i2c_types i2c_type;
> +	struct resource *res;
> +	int irq, ret;
> +	static int idx = 0;
> +
> +	if (np) {
> +		match = of_match_device(pxa_i2c_of_match, &dev->dev);
> +		if (match == NULL)
> +			return -ENODEV;
> +		i2c_type = (int)match->data;
> +	} else
> +		i2c_type = id->driver_data;
>  
>  	res = platform_get_resource(dev, IORESOURCE_MEM, 0);
>  	irq = platform_get_irq(dev, 0);
> @@ -1074,16 +1091,23 @@ static int i2c_pxa_probe(struct platform_device *dev)
>  	spin_lock_init(&i2c->lock);
>  	init_waitqueue_head(&i2c->wait);
>  
> -	/*
> -	 * If "dev->id" is negative we consider it as zero.
> -	 * The reason to do so is to avoid sysfs names that only make
> -	 * sense when there are multiple adapters.
> -	 */
> -	i2c->adap.nr = dev->id;
> -	snprintf(i2c->adap.name, sizeof(i2c->adap.name), "pxa_i2c-i2c.%u",
> -		 i2c->adap.nr);
>  
> -	i2c->clk = clk_get(&dev->dev, NULL);
> +	if (np) {
> +		i2c->adap.nr = idx++;

Use this so that a bus number gets dynamically assigned:
		i2c->adap.nr = -1;

> +		snprintf(i2c->adap.name, sizeof(i2c->adap.name),
> +			"pxa2xx-i2c.%u", i2c->adap.nr);
> +		i2c->clk = clk_get_sys(i2c->adap.name, NULL);

Missing i2c->adap.dev.of_node = dev->dev.of_node;

and after the bus is registered:

of_i2c_register_devices(&i2c->adap);

> +	} else {
> +		/*
> +		 * If "dev->id" is negative we consider it as zero.
> +		 * The reason to do so is to avoid sysfs names that only make
> +		 * sense when there are multiple adapters.
> +		 */
> +		i2c->adap.nr = dev->id;
> +		snprintf(i2c->adap.name, sizeof(i2c->adap.name),
> +			"pxa_i2c-i2c.%u", i2c->adap.nr);
> +		i2c->clk = clk_get(&dev->dev, NULL);
> +	}
>  	if (IS_ERR(i2c->clk)) {
>  		ret = PTR_ERR(i2c->clk);
>  		goto eclk;
> @@ -1234,6 +1258,7 @@ static struct platform_driver i2c_pxa_driver = {
>  		.name	= "pxa2xx-i2c",
>  		.owner	= THIS_MODULE,
>  		.pm	= I2C_PXA_DEV_PM_OPS,
> +		.of_match_table	= pxa_i2c_of_match,
>  	},
>  	.id_table	= i2c_pxa_id_table,
>  };
> -- 
> 1.5.6.5
>
Russell King - ARM Linux July 29, 2011, 4:55 p.m. UTC | #2
On Fri, Jul 29, 2011 at 10:52:22AM -0600, Grant Likely wrote:
> On Thu, Jul 28, 2011 at 02:41:32PM +0800, Haojian Zhuang wrote:
> > -	/*
> > -	 * If "dev->id" is negative we consider it as zero.
> > -	 * The reason to do so is to avoid sysfs names that only make
> > -	 * sense when there are multiple adapters.
> > -	 */
> > -	i2c->adap.nr = dev->id;
> > -	snprintf(i2c->adap.name, sizeof(i2c->adap.name), "pxa_i2c-i2c.%u",
> > -		 i2c->adap.nr);
> >  
> > -	i2c->clk = clk_get(&dev->dev, NULL);
> > +	if (np) {
> > +		i2c->adap.nr = idx++;
> 
> Use this so that a bus number gets dynamically assigned:
> 		i2c->adap.nr = -1;
> 
> > +		snprintf(i2c->adap.name, sizeof(i2c->adap.name),
> > +			"pxa2xx-i2c.%u", i2c->adap.nr);
> > +		i2c->clk = clk_get_sys(i2c->adap.name, NULL);
> 
> Missing i2c->adap.dev.of_node = dev->dev.of_node;

And here we go again.  Is it really the case that this DT stuff doesn't
have stable device names?
Mitch Bradley July 30, 2011, 2:29 p.m. UTC | #3
On 7/29/2011 6:55 AM, Russell King - ARM Linux wrote:
> On Fri, Jul 29, 2011 at 10:52:22AM -0600, Grant Likely wrote:
>> On Thu, Jul 28, 2011 at 02:41:32PM +0800, Haojian Zhuang wrote:
>>> -	/*
>>> -	 * If "dev->id" is negative we consider it as zero.
>>> -	 * The reason to do so is to avoid sysfs names that only make
>>> -	 * sense when there are multiple adapters.
>>> -	 */
>>> -	i2c->adap.nr = dev->id;
>>> -	snprintf(i2c->adap.name, sizeof(i2c->adap.name), "pxa_i2c-i2c.%u",
>>> -		 i2c->adap.nr);
>>>
>>> -	i2c->clk = clk_get(&dev->dev, NULL);
>>> +	if (np) {
>>> +		i2c->adap.nr = idx++;
>>
>> Use this so that a bus number gets dynamically assigned:
>> 		i2c->adap.nr = -1;
>>
>>> +		snprintf(i2c->adap.name, sizeof(i2c->adap.name),
>>> +			"pxa2xx-i2c.%u", i2c->adap.nr);
>>> +		i2c->clk = clk_get_sys(i2c->adap.name, NULL);
>>
>> Missing i2c->adap.dev.of_node = dev->dev.of_node;
>
> And here we go again.  Is it really the case that this DT stuff doesn't
> have stable device names?

Device tree names are completely stable, based on hardware addresses 
that don't change from boot to boot.  Even for buses where access 
addresses are dynamically assigned, the device tree "reg property" 
address is based on a stable addressing form.  For example, PCI devices 
use the (stable) configuration address as the reg property and USB 
devices use the (stable) hub port number.

People's tendency to want to assign sequential small integers in Linux 
has nothing to do with the device tree.  I suspect that it's a carryover 
from the historical Unix major/minor device numbering model, but in any 
case, there's nothing unstable about the device tree naming model. 
Quite the opposite - stable naming was a fundamental criterion when I 
designed Open Firmware.

> _______________________________________________
> devicetree-discuss mailing list
> devicetree-discuss@lists.ozlabs.org
> https://lists.ozlabs.org/listinfo/devicetree-discuss
>
Russell King - ARM Linux July 30, 2011, 3:37 p.m. UTC | #4
On Sat, Jul 30, 2011 at 04:29:50AM -1000, Mitch Bradley wrote:
> On 7/29/2011 6:55 AM, Russell King - ARM Linux wrote:
>> On Fri, Jul 29, 2011 at 10:52:22AM -0600, Grant Likely wrote:
>>> On Thu, Jul 28, 2011 at 02:41:32PM +0800, Haojian Zhuang wrote:
>>>> -	/*
>>>> -	 * If "dev->id" is negative we consider it as zero.
>>>> -	 * The reason to do so is to avoid sysfs names that only make
>>>> -	 * sense when there are multiple adapters.
>>>> -	 */
>>>> -	i2c->adap.nr = dev->id;
>>>> -	snprintf(i2c->adap.name, sizeof(i2c->adap.name), "pxa_i2c-i2c.%u",
>>>> -		 i2c->adap.nr);
>>>>
>>>> -	i2c->clk = clk_get(&dev->dev, NULL);
>>>> +	if (np) {
>>>> +		i2c->adap.nr = idx++;
>>>
>>> Use this so that a bus number gets dynamically assigned:
>>> 		i2c->adap.nr = -1;
>>>
>>>> +		snprintf(i2c->adap.name, sizeof(i2c->adap.name),
>>>> +			"pxa2xx-i2c.%u", i2c->adap.nr);
>>>> +		i2c->clk = clk_get_sys(i2c->adap.name, NULL);
>>>
>>> Missing i2c->adap.dev.of_node = dev->dev.of_node;
>>
>> And here we go again.  Is it really the case that this DT stuff doesn't
>> have stable device names?
>
> Device tree names are completely stable, based on hardware addresses  
> that don't change from boot to boot.  Even for buses where access  
> addresses are dynamically assigned, the device tree "reg property"  
> address is based on a stable addressing form.  For example, PCI devices  
> use the (stable) configuration address as the reg property and USB  
> devices use the (stable) hub port number.
>
> People's tendency to want to assign sequential small integers in Linux  
> has nothing to do with the device tree.  I suspect that it's a carryover  
> from the historical Unix major/minor device numbering model, but in any  
> case, there's nothing unstable about the device tree naming model. Quite 
> the opposite - stable naming was a fundamental criterion when I designed 
> Open Firmware.

Which means - if Grant is accepting the conversion of ARM to DT and
upstreaming it, he needs to keep an eye on this madness and reject
stuff which tries to do as per this patch.
Grant Likely July 31, 2011, 12:38 a.m. UTC | #5
On Sat, Jul 30, 2011 at 04:37:28PM +0100, Russell King - ARM Linux wrote:
> On Sat, Jul 30, 2011 at 04:29:50AM -1000, Mitch Bradley wrote:
> > On 7/29/2011 6:55 AM, Russell King - ARM Linux wrote:
> >> On Fri, Jul 29, 2011 at 10:52:22AM -0600, Grant Likely wrote:
> >>> On Thu, Jul 28, 2011 at 02:41:32PM +0800, Haojian Zhuang wrote:
> >>>> -	/*
> >>>> -	 * If "dev->id" is negative we consider it as zero.
> >>>> -	 * The reason to do so is to avoid sysfs names that only make
> >>>> -	 * sense when there are multiple adapters.
> >>>> -	 */
> >>>> -	i2c->adap.nr = dev->id;
> >>>> -	snprintf(i2c->adap.name, sizeof(i2c->adap.name), "pxa_i2c-i2c.%u",
> >>>> -		 i2c->adap.nr);
> >>>>
> >>>> -	i2c->clk = clk_get(&dev->dev, NULL);
> >>>> +	if (np) {
> >>>> +		i2c->adap.nr = idx++;
> >>>
> >>> Use this so that a bus number gets dynamically assigned:
> >>> 		i2c->adap.nr = -1;
> >>>
> >>>> +		snprintf(i2c->adap.name, sizeof(i2c->adap.name),
> >>>> +			"pxa2xx-i2c.%u", i2c->adap.nr);
> >>>> +		i2c->clk = clk_get_sys(i2c->adap.name, NULL);
> >>>
> >>> Missing i2c->adap.dev.of_node = dev->dev.of_node;
> >>
> >> And here we go again.  Is it really the case that this DT stuff doesn't
> >> have stable device names?

The "full_name" is indeed stable for a device_node, so that isn't
actually the problem, but the full_name is the entire path from the
root of the tree to the device.  The problem is that it is far too
long for to be used as (struct device*)->kobj.name, and it has '/'
characters in it which aren't particularly friendly.  The short node
name is unique among its siblings, but not globally.

Currently the kernel constructs a 'short' name when creating a device
with a heuristic that tries to use the physical address of the device
and the node name.  If that isn't available (ie. non-memory mapped
devices), then it uses a globally incremented integer to assign each
device a unique name so that sysfs doesn't freak out.

See of_device_make_bus_id() in drivers/of/platform.c

The /real/ problem is that I don't much like the heuristic; but I
haven't been able to think of anything better.  There is enough
uncertainty (very tiny, but uncertainty never the less) on device
names, and the fact that I'm hoping to improve the method of creating
device names, that I don't want to rely on them for matching up
resources.

Instead, the dev_resdata array that is passed into
of_platform_populate() has a field to override the heuristic generated
name which neatly solves the problem entirely without tracking down
all the references that need to be added, and has the added
advantage of DT and non-DT platforms using the same names.

The clk_get() changes in this patch are absolutely the wrong solution.

> > Device tree names are completely stable, based on hardware addresses  
> > that don't change from boot to boot.  Even for buses where access  
> > addresses are dynamically assigned, the device tree "reg property"  
> > address is based on a stable addressing form.  For example, PCI devices  
> > use the (stable) configuration address as the reg property and USB  
> > devices use the (stable) hub port number.
> >
> > People's tendency to want to assign sequential small integers in Linux  
> > has nothing to do with the device tree.  I suspect that it's a carryover  
> > from the historical Unix major/minor device numbering model, but in any  
> > case, there's nothing unstable about the device tree naming model. Quite 
> > the opposite - stable naming was a fundamental criterion when I designed 
> > Open Firmware.
> 
> Which means - if Grant is accepting the conversion of ARM to DT and
> upstreaming it, he needs to keep an eye on this madness and reject
> stuff which tries to do as per this patch.

Correct, I will nack/reject any patches messing about with clock
attachment or similar changes.  I was distracted by the other issues,
so I didn't comment on it in this patch.

g.

Patch
diff mbox

diff --git a/drivers/i2c/busses/i2c-pxa.c b/drivers/i2c/busses/i2c-pxa.c
index d603646..8c27733 100644
--- a/drivers/i2c/busses/i2c-pxa.c
+++ b/drivers/i2c/busses/i2c-pxa.c
@@ -29,6 +29,7 @@ 
 #include <linux/errno.h>
 #include <linux/interrupt.h>
 #include <linux/i2c-pxa.h>
+#include <linux/of_device.h>
 #include <linux/of_i2c.h>
 #include <linux/platform_device.h>
 #include <linux/err.h>
@@ -1044,15 +1045,31 @@  static const struct i2c_algorithm i2c_pxa_pio_algorithm = {
 	.functionality	= i2c_pxa_functionality,
 };
 
+static const struct of_device_id pxa_i2c_of_match[] = {
+	{ .compatible = "mrvl,pxa255-i2c",    .data = (void *)REGS_PXA2XX, },
+	{ .compatible = "mrvl,pxa300-pwri2c", .data = (void *)REGS_PXA3XX, },
+	{},
+};
+
 static int i2c_pxa_probe(struct platform_device *dev)
 {
+	struct device_node *np = dev->dev.of_node;
+	const struct of_device_id *match;
 	struct pxa_i2c *i2c;
-	struct resource *res;
 	struct i2c_pxa_platform_data *plat = dev->dev.platform_data;
 	const struct platform_device_id *id = platform_get_device_id(dev);
-	enum pxa_i2c_types i2c_type = id->driver_data;
-	int ret;
-	int irq;
+	enum pxa_i2c_types i2c_type;
+	struct resource *res;
+	int irq, ret;
+	static int idx = 0;
+
+	if (np) {
+		match = of_match_device(pxa_i2c_of_match, &dev->dev);
+		if (match == NULL)
+			return -ENODEV;
+		i2c_type = (int)match->data;
+	} else
+		i2c_type = id->driver_data;
 
 	res = platform_get_resource(dev, IORESOURCE_MEM, 0);
 	irq = platform_get_irq(dev, 0);
@@ -1074,16 +1091,23 @@  static int i2c_pxa_probe(struct platform_device *dev)
 	spin_lock_init(&i2c->lock);
 	init_waitqueue_head(&i2c->wait);
 
-	/*
-	 * If "dev->id" is negative we consider it as zero.
-	 * The reason to do so is to avoid sysfs names that only make
-	 * sense when there are multiple adapters.
-	 */
-	i2c->adap.nr = dev->id;
-	snprintf(i2c->adap.name, sizeof(i2c->adap.name), "pxa_i2c-i2c.%u",
-		 i2c->adap.nr);
 
-	i2c->clk = clk_get(&dev->dev, NULL);
+	if (np) {
+		i2c->adap.nr = idx++;
+		snprintf(i2c->adap.name, sizeof(i2c->adap.name),
+			"pxa2xx-i2c.%u", i2c->adap.nr);
+		i2c->clk = clk_get_sys(i2c->adap.name, NULL);
+	} else {
+		/*
+		 * If "dev->id" is negative we consider it as zero.
+		 * The reason to do so is to avoid sysfs names that only make
+		 * sense when there are multiple adapters.
+		 */
+		i2c->adap.nr = dev->id;
+		snprintf(i2c->adap.name, sizeof(i2c->adap.name),
+			"pxa_i2c-i2c.%u", i2c->adap.nr);
+		i2c->clk = clk_get(&dev->dev, NULL);
+	}
 	if (IS_ERR(i2c->clk)) {
 		ret = PTR_ERR(i2c->clk);
 		goto eclk;
@@ -1234,6 +1258,7 @@  static struct platform_driver i2c_pxa_driver = {
 		.name	= "pxa2xx-i2c",
 		.owner	= THIS_MODULE,
 		.pm	= I2C_PXA_DEV_PM_OPS,
+		.of_match_table	= pxa_i2c_of_match,
 	},
 	.id_table	= i2c_pxa_id_table,
 };