Skip to contentSkip to conversation

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

PATCH Cousson, Benoit 4 participants 8 messages Aug 3, 2011 – Aug 7, 2011 Device tree

lore
Share & tools
lore

Cousson, Benoit

author
#0 lore raw

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

---

drivers/of/device.c +0 -5
index 62b4b32..04727e4 100644
@@ -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 */
drivers/of/platform.c +12 -11
index ebbbf42..f7b6ec1 100644
@@ -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;
include/linux/of_platform.h +1 -1
index 252246c..9d3cbbe 100644
@@ -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 */
#1 lore raw

Hi Benoit,

Quoted 10 lines · Benoit

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

author
#2 lore raw

Hi Manju,

Quoted 14 lines · Manjunath Kondaiah

Hi Benoit,

Quoted 10 lines · Benoit

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;
Quoted 7 lines

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?

Quoted 3 lines

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

#3 lore raw
In reply to Cousson, Benoit
Quoted 10 lines · Benoit

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.

#4 lore raw
Quoted 5 lines

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

author
#5 lore raw
Quoted 11 lines · Barry Song
Quoted 5 lines

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

#6 lore raw
Quoted 20 lines · Benoit
Quoted 11 lines · Barry Song
Quoted 5 lines

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.

Quoted 2 lines

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.

author
#7 lore raw
In reply to Grant Likely
Quoted 12 lines · Grant Likely
Quoted 10 lines · Benoit

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;
Quoted 5 lines

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 :-)

Quoted 13 lines

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.

Quoted 3 lines

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.

Quoted 3 lines

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