Shouldn't DT preserve pdev name and id to allow platform_match to work?
diff mbox

Message ID 4E396361.3060307@ti.com
State New, archived
Headers show

Commit Message

Benoit Cousson Aug. 3, 2011, 3:04 p.m. UTC
Hi Grant,

Going further with the usage of OF_DEV_AUXDATA_ID, I realized that this is is not doing what I was expecting. My expectation might be silly, but in order to make platform_match to work without DT matching mechanism, you need to have the driver name in the pdev->name field:

	/* fall-back to driver name match */
	return (strcmp(pdev->name, drv->name) == 0);

Except that the of_device_add function is doing that:
	pdev->name = dev_name(&ofdev->dev);
	pdev->id = -1;

Thus overwriting the original pdev->name with the dev_name / bus_id in DT case.

Whereas the regular naming convention with pdev is to have dev_name = <name>.<id>".

Because of that, we cannot match the proper driver name with this entry: 

  OF_DEV_AUXDATA_ID("ti,omap-i2c", 0x48000100, "omap-i2c.1", 1, &i2c_pdata)

pdev->id will be properly overwritten after device creation, but not the pdev->name.
pdev->name will be "omap-i2c.1" instead of "omap-i2c".

By changing a little bit the format like that (removing the id from bus_id that is BTW redundant in that case):

  OF_DEV_AUXDATA_ID("ti,omap-i2c", 0x48000100, "omap-i2c", 1, &i2c_pdata)

We will be able to maintain the original driver name inside pdev->name and thus match correctly during probe with the legacy method.

Since DT is building a real platform_device, it seems to be reasonable to maintain the legacy name.

Moreover that will allow a basic driver to still probe correctly its devices.

Please find after a quick and dirty proof of concept.

Does that make sense to you?

Regards,
Benoit 


---

Comments

manjugk manjugk Aug. 3, 2011, 4:17 p.m. UTC | #1
Hi Benoit,

On Wed, Aug 3, 2011 at 8:34 PM, Cousson, Benoit <b-cousson@ti.com> wrote:
> Hi Grant,
>
> Going further with the usage of OF_DEV_AUXDATA_ID, I realized that this is is not doing what I was expecting. My expectation might be silly, but in order to make platform_match to work without DT matching mechanism, you need to have the driver name in the pdev->name field:
>
>        /* fall-back to driver name match */
>        return (strcmp(pdev->name, drv->name) == 0);
>
> Except that the of_device_add function is doing that:
>        pdev->name = dev_name(&ofdev->dev);
>        pdev->id = -1;

This is taken here:
http://git.secretlab.ca/?p=linux-2.6.git;a=commitdiff;h=04f51955dc690666467252015a931d71d5c56696

and it is pending for upstream.

With the above patch, both pdev->name and pdev->id can be set as per
hwmod requirement through aux_data
lookup table. we might have to append pdev->id to pdev->name so that
it can used for device look up in the hwmod
device list.

-Manjunath
Benoit Cousson Aug. 3, 2011, 4:27 p.m. UTC | #2
Hi Manju,

On 8/3/2011 6:17 PM, G, Manjunath Kondaiah wrote:
> Hi Benoit,
>
> On Wed, Aug 3, 2011 at 8:34 PM, Cousson, Benoit<b-cousson@ti.com>  wrote:
>> Hi Grant,
>>
>> Going further with the usage of OF_DEV_AUXDATA_ID, I realized that this is is not doing what I was expecting. My expectation might be silly, but in order to make platform_match to work without DT matching mechanism, you need to have the driver name in the pdev->name field:
>>
>>         /* fall-back to driver name match */
>>         return (strcmp(pdev->name, drv->name) == 0);
>>
>> Except that the of_device_add function is doing that:
>>         pdev->name = dev_name(&ofdev->dev);
>>         pdev->id = -1;
>
> This is taken here:
> http://git.secretlab.ca/?p=linux-2.6.git;a=commitdiff;h=04f51955dc690666467252015a931d71d5c56696
>
> and it is pending for upstream.
>
> With the above patch, both pdev->name and pdev->id can be set as per
> hwmod requirement through aux_data lookup table.

Mmm, but I'm already using that patch, and it is only taking care of the 
pdev->id, hence my email. Did I miss something?

> we might have to append pdev->id to pdev->name so that
> it can used for device look up in the hwmod
> device list.

I'm not sure to get your point here.

Benoit
Grant Likely Aug. 3, 2011, 4:43 p.m. UTC | #3
On Wed, Aug 3, 2011 at 4:04 PM, Cousson, Benoit <b-cousson@ti.com> wrote:
> Hi Grant,
>
> Going further with the usage of OF_DEV_AUXDATA_ID, I realized that this is is not doing what I was expecting. My expectation might be silly, but in order to make platform_match to work without DT matching mechanism, you need to have the driver name in the pdev->name field:
>
>        /* fall-back to driver name match */
>        return (strcmp(pdev->name, drv->name) == 0);
>
> Except that the of_device_add function is doing that:
>        pdev->name = dev_name(&ofdev->dev);
>        pdev->id = -1;

Yes, I know.  It is written that way on purpose so that it doesn't get
used to avoid adding a match table to the device driver because
auxdata is a stop-gap solution.  It is *not* intended to be used long
term (with one exception for passing platform callbacks when there is
no other solution).

auxdata passes platform_data and overrides the device name when there
is no way easy way to make the driver work without it.  It handles the
the current implementation of clocks and regulators which aren't yet
populated from the device tree.  It will go away when clock &
regulator bindings are implemented.

By implementing it the way it is, it ensures that no drivers
inadvertently depends on auxdata, and will thus break when the auxdata
entries are removed.  Manjunath mentioned that there is a patch that
allows the id to be set, but it still doesn't set the driver name in a
way that will allow existing drivers to bind using the old method, and
that is by design.

I understand what you are asking for, but the DT matching mechanism is
absolutely required.  Adding it to drivers is certainly not any more
onerous that maintaining a bunch of auxdata tables for boards.
Besides, part of the point of the move to DT is to eliminate device
and device data tables at the board file level, which is another
reason why auxdata is a temporary solution.

g.
Barry Song Aug. 5, 2011, 10:02 a.m. UTC | #4
> auxdata passes platform_data and overrides the device name when there
> is no way easy way to make the driver work without it.  It handles the
> the current implementation of clocks and regulators which aren't yet
> populated from the device tree.  It will go away when clock &
> regulator bindings are implemented.
Yes. As OF_DEV_AUXDATA_ID still requires hardware information like
0x48000100 as below, it seems it is not
consistent with the origin purpose of ARM DT.
OF_DEV_AUXDATA_ID("ti,omap-i2c", 0x48000100, "omap-i2c.1", 1, &i2c_pdata)
And the information 0x48000100 is something that doesn't want to be in
kernel codes.it should be only in dts.

-barry
Benoit Cousson Aug. 5, 2011, 12:19 p.m. UTC | #5
On 8/3/2011 6:43 PM, Grant Likely wrote:
> On Wed, Aug 3, 2011 at 4:04 PM, Cousson, Benoit<b-cousson@ti.com>  wrote:
>> Hi Grant,
>>
>> Going further with the usage of OF_DEV_AUXDATA_ID, I realized that this is is not doing what I was expecting. My expectation might be silly, but in order to make platform_match to work without DT matching mechanism, you need to have the driver name in the pdev->name field:
>>
>>         /* fall-back to driver name match */
>>         return (strcmp(pdev->name, drv->name) == 0);
>>
>> Except that the of_device_add function is doing that:
>>         pdev->name = dev_name(&ofdev->dev);
>>         pdev->id = -1;
>
> Yes, I know.  It is written that way on purpose so that it doesn't get
> used to avoid adding a match table to the device driver because
> auxdata is a stop-gap solution.  It is *not* intended to be used long
> term (with one exception for passing platform callbacks when there is
> no other solution).

Outch... You guys are doing your best to make our life harder :-)

> auxdata passes platform_data and overrides the device name when there
> is no way easy way to make the driver work without it.  It handles the
> the current implementation of clocks and regulators which aren't yet
> populated from the device tree.  It will go away when clock&
> regulator bindings are implemented.
>
> By implementing it the way it is, it ensures that no drivers
> inadvertently depends on auxdata, and will thus break when the auxdata
> entries are removed.  Manjunath mentioned that there is a patch that
> allows the id to be set, but it still doesn't set the driver name in a
> way that will allow existing drivers to bind using the old method, and
> that is by design.
>
> I understand what you are asking for, but the DT matching mechanism is
> absolutely required.  Adding it to drivers is certainly not any more
> onerous that maintaining a bunch of auxdata tables for boards.

Fair enough.
Bottom line is that DT is a all inclusive pack, you cannot skip one part 
even if it is not necessarily needed right now :-(

I understand very well the final goal, I was just trying to spread the 
huge effort that DT migration will require to focus first on the stuff 
located in mach-omap, meaning all the board and devices init part.

I was hoping to let the drivers for the moment until the first phase is 
done. Since auxdata was already there to help the migration, I was just 
expecting it to be even more helpful.

> Besides, part of the point of the move to DT is to eliminate device
> and device data tables at the board file level, which is another
> reason why auxdata is a temporary solution.

Again, I got that point, it was not our intent to use that as a final 
solution.

Let's start hacking the OMAP drivers then...

Regards,
Benoit
Benoit Cousson Aug. 5, 2011, 12:31 p.m. UTC | #6
On 8/5/2011 12:02 PM, Barry Song wrote:
>> auxdata passes platform_data and overrides the device name when there
>> is no way easy way to make the driver work without it.  It handles the
>> the current implementation of clocks and regulators which aren't yet
>> populated from the device tree.  It will go away when clock&
>> regulator bindings are implemented.
> Yes. As OF_DEV_AUXDATA_ID still requires hardware information like
> 0x48000100 as below, it seems it is not
> consistent with the origin purpose of ARM DT.
> OF_DEV_AUXDATA_ID("ti,omap-i2c", 0x48000100, "omap-i2c.1", 1,&i2c_pdata)
> And the information 0x48000100 is something that doesn't want to be in
> kernel codes.it should be only in dts.

FWIW, I do not care about the physical address at all. This is just used 
by the of_dev_lookup function to get the proper instance for a device 
compatible type.
That's the only way for the OF_DEV_AUXDATA to work, but in theory you 
could do the same as soon as you provide the id. So OF_DEV_AUXDATA_ID 
could avoid it.

Since OF_DEV_AUXDATA_ID seems to be already dead before it even reaches 
the mainline, let's forget about that.

Regards,
Benoit
Grant Likely Aug. 7, 2011, 4:13 a.m. UTC | #7
On Fri, Aug 05, 2011 at 02:31:16PM +0200, Cousson, Benoit wrote:
> On 8/5/2011 12:02 PM, Barry Song wrote:
> >>auxdata passes platform_data and overrides the device name when there
> >>is no way easy way to make the driver work without it.  It handles the
> >>the current implementation of clocks and regulators which aren't yet
> >>populated from the device tree.  It will go away when clock&
> >>regulator bindings are implemented.
> >Yes. As OF_DEV_AUXDATA_ID still requires hardware information like
> >0x48000100 as below, it seems it is not
> >consistent with the origin purpose of ARM DT.
> >OF_DEV_AUXDATA_ID("ti,omap-i2c", 0x48000100, "omap-i2c.1", 1,&i2c_pdata)
> >And the information 0x48000100 is something that doesn't want to be in
> >kernel codes.it should be only in dts.
> 
> FWIW, I do not care about the physical address at all. This is just
> used by the of_dev_lookup function to get the proper instance for a
> device compatible type.
> That's the only way for the OF_DEV_AUXDATA to work, but in theory
> you could do the same as soon as you provide the id. So
> OF_DEV_AUXDATA_ID could avoid it.
> 
> Since OF_DEV_AUXDATA_ID seems to be already dead before it even
> reaches the mainline, let's forget about that.

Actually, I'm planning to merge OF_DEV_AUXDATA_ID().  I was hoping to
avoid it but there was enough hard-to-solve-quickly problems that I've
relented.

Adding the match table is not hard-to-solve though, so I'm not
relenting on that point.  :-)

g.

Patch
diff mbox

diff --git a/drivers/of/device.c b/drivers/of/device.c
index 62b4b32..04727e4 100644
--- a/drivers/of/device.c
+++ b/drivers/of/device.c
@@ -51,11 +51,6 @@  int of_device_add(struct platform_device *ofdev)
 {
 	BUG_ON(ofdev->dev.of_node == NULL);
 
-	/* name and id have to be set so that the platform bus doesn't get
-	 * confused on matching */
-	ofdev->name = dev_name(&ofdev->dev);
-	ofdev->id = -1;
-
 	/* device_add will assume that this device is on the same node as
 	 * the parent. If there is no parent defined, set the node
 	 * explicitly */
diff --git a/drivers/of/platform.c b/drivers/of/platform.c
index ebbbf42..f7b6ec1 100644
--- a/drivers/of/platform.c
+++ b/drivers/of/platform.c
@@ -130,13 +130,14 @@  void of_device_make_bus_id(struct device *dev)
  */
 struct platform_device *of_device_alloc(struct device_node *np,
 				  const char *bus_id,
-				  struct device *parent)
+				  struct device *parent,
+				  int id)
 {
 	struct platform_device *dev;
 	int rc, i, num_reg = 0, num_irq;
 	struct resource *res, temp_res;
 
-	dev = platform_device_alloc("", -1);
+	dev = platform_device_alloc(bus_id ? bus_id : "", id);
 	if (!dev)
 		return NULL;
 
@@ -169,7 +170,10 @@  struct platform_device *of_device_alloc(struct device_node *np,
 	dev->dev.parent = parent;
 
 	if (bus_id)
-		dev_set_name(&dev->dev, "%s", bus_id);
+		if (id == -1)
+			dev_set_name(&dev->dev, "%s", bus_id);
+		else
+			dev_set_name(&dev->dev, "%s.%d", bus_id, id);
 	else
 		of_device_make_bus_id(&dev->dev);
 
@@ -191,14 +195,15 @@  struct platform_device *of_platform_device_create_pdata(
 					struct device_node *np,
 					const char *bus_id,
 					void *platform_data,
-					struct device *parent)
+					struct device *parent,
+					int id)
 {
 	struct platform_device *dev;
 
 	if (!of_device_is_available(np))
 		return NULL;
 
-	dev = of_device_alloc(np, bus_id, parent);
+	dev = of_device_alloc(np, bus_id, parent, id);
 	if (!dev)
 		return NULL;
 
@@ -235,7 +240,7 @@  struct platform_device *of_platform_device_create(struct device_node *np,
 					    const char *bus_id,
 					    struct device *parent)
 {
-	return of_platform_device_create_pdata(np, bus_id, NULL, parent);
+	return of_platform_device_create_pdata(np, bus_id, NULL, parent, -1);
 }
 EXPORT_SYMBOL(of_platform_device_create);
 
@@ -595,11 +600,7 @@  static int of_platform_bus_create(struct device_node *bus,
 		return 0;
 	}
 
-	dev = of_platform_device_create_pdata(bus, bus_id, platform_data, parent);
-
-	/* override the id if auxdata gives an id */
-	if (id != -1)
-		dev->id = id;
+	dev = of_platform_device_create_pdata(bus, bus_id, platform_data, parent, id);
 
 	if (!dev || !of_match_node(matches, bus))
 		return 0;
diff --git a/include/linux/of_platform.h b/include/linux/of_platform.h
index 252246c..9d3cbbe 100644
--- a/include/linux/of_platform.h
+++ b/include/linux/of_platform.h
@@ -84,7 +84,7 @@  extern const struct of_device_id of_default_bus_match_table[];
 /* Platform drivers register/unregister */
 extern struct platform_device *of_device_alloc(struct device_node *np,
 					 const char *bus_id,
-					 struct device *parent);
+					 struct device *parent, int id);
 extern struct platform_device *of_find_device_by_node(struct device_node *np);
 
 #if !defined(CONFIG_SPARC) /* SPARC has its own device registration method */