* [PATCH] base: platform: name the device already during allocation @ 2014-04-28 12:09 Heikki Krogerus 2014-05-28 20:58 ` Greg KH 0 siblings, 1 reply; 3+ messages in thread From: Heikki Krogerus @ 2014-04-28 12:09 UTC (permalink / raw) To: Greg KH; +Cc: Russell King, Jean Delvare, Vivek Gautam, linux-kernel This allows resources such as GPIOs and clocks, which can be matched based on the device name when requested, to be assigned even when PLATFORM_DEVID_AUTO is used. Signed-off-by: Heikki Krogerus <heikki.krogerus@linux.intel.com> --- drivers/base/platform.c | 77 ++++++++++++++++++++++++++++++------------------- 1 file changed, 47 insertions(+), 30 deletions(-) diff --git a/drivers/base/platform.c b/drivers/base/platform.c index 5b47210..697896d 100644 --- a/drivers/base/platform.c +++ b/drivers/base/platform.c @@ -174,11 +174,45 @@ struct platform_object { */ void platform_device_put(struct platform_device *pdev) { - if (pdev) - put_device(&pdev->dev); + if (!pdev) + return; + + if (pdev->id_auto) { + ida_simple_remove(&platform_devid_ida, pdev->id); + pdev->id = PLATFORM_DEVID_AUTO; + } + + put_device(&pdev->dev); } EXPORT_SYMBOL_GPL(platform_device_put); +static int pdev_set_name(struct platform_device *pdev) +{ + int ret; + + switch (pdev->id) { + default: + return dev_set_name(&pdev->dev, "%s.%d", pdev->name, pdev->id); + case PLATFORM_DEVID_NONE: + return dev_set_name(&pdev->dev, "%s", pdev->name); + case PLATFORM_DEVID_AUTO: + /* + * Automatically allocated device ID. We mark it as such so + * that we remember it must be freed, and we append a suffix + * to avoid namespace collision with explicit IDs. + */ + ret = ida_simple_get(&platform_devid_ida, 0, 0, GFP_KERNEL); + if (ret < 0) + return ret; + pdev->id = ret; + pdev->id_auto = true; + return dev_set_name(&pdev->dev, "%s.%d.auto", pdev->name, + pdev->id); + } + + return 0; +} + static void platform_device_release(struct device *dev) { struct platform_object *pa = container_of(dev, struct platform_object, @@ -211,6 +245,10 @@ struct platform_device *platform_device_alloc(const char *name, int id) device_initialize(&pa->pdev.dev); pa->pdev.dev.release = platform_device_release; arch_setup_pdev_archdata(&pa->pdev); + if (pdev_set_name(&pa->pdev)) { + kfree(pa); + return NULL; + } } return pa ? &pa->pdev : NULL; @@ -291,28 +329,6 @@ int platform_device_add(struct platform_device *pdev) pdev->dev.bus = &platform_bus_type; - switch (pdev->id) { - default: - dev_set_name(&pdev->dev, "%s.%d", pdev->name, pdev->id); - break; - case PLATFORM_DEVID_NONE: - dev_set_name(&pdev->dev, "%s", pdev->name); - break; - case PLATFORM_DEVID_AUTO: - /* - * Automatically allocated device ID. We mark it as such so - * that we remember it must be freed, and we append a suffix - * to avoid namespace collision with explicit IDs. - */ - ret = ida_simple_get(&platform_devid_ida, 0, 0, GFP_KERNEL); - if (ret < 0) - goto err_out; - pdev->id = ret; - pdev->id_auto = true; - dev_set_name(&pdev->dev, "%s.%d.auto", pdev->name, pdev->id); - break; - } - for (i = 0; i < pdev->num_resources; i++) { struct resource *p, *r = &pdev->resource[i]; @@ -355,7 +371,6 @@ int platform_device_add(struct platform_device *pdev) release_resource(r); } - err_out: return ret; } EXPORT_SYMBOL_GPL(platform_device_add); @@ -375,11 +390,6 @@ void platform_device_del(struct platform_device *pdev) if (pdev) { device_del(&pdev->dev); - if (pdev->id_auto) { - ida_simple_remove(&platform_devid_ida, pdev->id); - pdev->id = PLATFORM_DEVID_AUTO; - } - for (i = 0; i < pdev->num_resources; i++) { struct resource *r = &pdev->resource[i]; unsigned long type = resource_type(r); @@ -397,8 +407,15 @@ EXPORT_SYMBOL_GPL(platform_device_del); */ int platform_device_register(struct platform_device *pdev) { + int ret; + device_initialize(&pdev->dev); arch_setup_pdev_archdata(pdev); + + ret = pdev_set_name(pdev); + if (ret) + return ret; + return platform_device_add(pdev); } EXPORT_SYMBOL_GPL(platform_device_register); -- 2.0.0.rc0 ^ permalink raw reply related [flat|nested] 3+ messages in thread
* Re: [PATCH] base: platform: name the device already during allocation 2014-04-28 12:09 [PATCH] base: platform: name the device already during allocation Heikki Krogerus @ 2014-05-28 20:58 ` Greg KH 2014-05-30 8:35 ` Heikki Krogerus 0 siblings, 1 reply; 3+ messages in thread From: Greg KH @ 2014-05-28 20:58 UTC (permalink / raw) To: Heikki Krogerus; +Cc: Russell King, Jean Delvare, Vivek Gautam, linux-kernel On Mon, Apr 28, 2014 at 03:09:27PM +0300, Heikki Krogerus wrote: > This allows resources such as GPIOs and clocks, which can be > matched based on the device name when requested, to be > assigned even when PLATFORM_DEVID_AUTO is used. Why would anyone want to do that? > > Signed-off-by: Heikki Krogerus <heikki.krogerus@linux.intel.com> > --- > drivers/base/platform.c | 77 ++++++++++++++++++++++++++++++------------------- > 1 file changed, 47 insertions(+), 30 deletions(-) > > diff --git a/drivers/base/platform.c b/drivers/base/platform.c > index 5b47210..697896d 100644 > --- a/drivers/base/platform.c > +++ b/drivers/base/platform.c > @@ -174,11 +174,45 @@ struct platform_object { > */ > void platform_device_put(struct platform_device *pdev) > { > - if (pdev) > - put_device(&pdev->dev); > + if (!pdev) > + return; > + > + if (pdev->id_auto) { > + ida_simple_remove(&platform_devid_ida, pdev->id); > + pdev->id = PLATFORM_DEVID_AUTO; > + } > + > + put_device(&pdev->dev); > } > EXPORT_SYMBOL_GPL(platform_device_put); > > +static int pdev_set_name(struct platform_device *pdev) > +{ > + int ret; > + > + switch (pdev->id) { > + default: > + return dev_set_name(&pdev->dev, "%s.%d", pdev->name, pdev->id); > + case PLATFORM_DEVID_NONE: > + return dev_set_name(&pdev->dev, "%s", pdev->name); > + case PLATFORM_DEVID_AUTO: > + /* > + * Automatically allocated device ID. We mark it as such so > + * that we remember it must be freed, and we append a suffix > + * to avoid namespace collision with explicit IDs. > + */ > + ret = ida_simple_get(&platform_devid_ida, 0, 0, GFP_KERNEL); > + if (ret < 0) > + return ret; > + pdev->id = ret; > + pdev->id_auto = true; > + return dev_set_name(&pdev->dev, "%s.%d.auto", pdev->name, > + pdev->id); > + } > + > + return 0; > +} > + > static void platform_device_release(struct device *dev) > { > struct platform_object *pa = container_of(dev, struct platform_object, > @@ -211,6 +245,10 @@ struct platform_device *platform_device_alloc(const char *name, int id) > device_initialize(&pa->pdev.dev); > pa->pdev.dev.release = platform_device_release; > arch_setup_pdev_archdata(&pa->pdev); > + if (pdev_set_name(&pa->pdev)) { > + kfree(pa); > + return NULL; > + } > } > > return pa ? &pa->pdev : NULL; > @@ -291,28 +329,6 @@ int platform_device_add(struct platform_device *pdev) > > pdev->dev.bus = &platform_bus_type; > > - switch (pdev->id) { > - default: > - dev_set_name(&pdev->dev, "%s.%d", pdev->name, pdev->id); > - break; > - case PLATFORM_DEVID_NONE: > - dev_set_name(&pdev->dev, "%s", pdev->name); > - break; > - case PLATFORM_DEVID_AUTO: > - /* > - * Automatically allocated device ID. We mark it as such so > - * that we remember it must be freed, and we append a suffix > - * to avoid namespace collision with explicit IDs. > - */ > - ret = ida_simple_get(&platform_devid_ida, 0, 0, GFP_KERNEL); > - if (ret < 0) > - goto err_out; > - pdev->id = ret; > - pdev->id_auto = true; > - dev_set_name(&pdev->dev, "%s.%d.auto", pdev->name, pdev->id); > - break; > - } > - > for (i = 0; i < pdev->num_resources; i++) { > struct resource *p, *r = &pdev->resource[i]; > > @@ -355,7 +371,6 @@ int platform_device_add(struct platform_device *pdev) > release_resource(r); > } > > - err_out: > return ret; > } > EXPORT_SYMBOL_GPL(platform_device_add); > @@ -375,11 +390,6 @@ void platform_device_del(struct platform_device *pdev) > if (pdev) { > device_del(&pdev->dev); > > - if (pdev->id_auto) { > - ida_simple_remove(&platform_devid_ida, pdev->id); > - pdev->id = PLATFORM_DEVID_AUTO; > - } > - > for (i = 0; i < pdev->num_resources; i++) { > struct resource *r = &pdev->resource[i]; > unsigned long type = resource_type(r); > @@ -397,8 +407,15 @@ EXPORT_SYMBOL_GPL(platform_device_del); > */ > int platform_device_register(struct platform_device *pdev) > { > + int ret; > + > device_initialize(&pdev->dev); > arch_setup_pdev_archdata(pdev); > + > + ret = pdev_set_name(pdev); > + if (ret) > + return ret; > + > return platform_device_add(pdev); > } > EXPORT_SYMBOL_GPL(platform_device_register); > -- > 2.0.0.rc0 I don't understand the usage of this, what drivers want/need this change? greg k-h ^ permalink raw reply [flat|nested] 3+ messages in thread
* Re: [PATCH] base: platform: name the device already during allocation 2014-05-28 20:58 ` Greg KH @ 2014-05-30 8:35 ` Heikki Krogerus 0 siblings, 0 replies; 3+ messages in thread From: Heikki Krogerus @ 2014-05-30 8:35 UTC (permalink / raw) To: Greg KH; +Cc: Russell King, Jean Delvare, Vivek Gautam, linux-kernel On Wed, May 28, 2014 at 01:58:06PM -0700, Greg KH wrote: > On Mon, Apr 28, 2014 at 03:09:27PM +0300, Heikki Krogerus wrote: > > This allows resources such as GPIOs and clocks, which can be > > matched based on the device name when requested, to be > > assigned even when PLATFORM_DEVID_AUTO is used. > > Why would anyone want to do that? The idea is to be able to share more resources of the parent device with child platform devices that are created on fly without tricks. Many frameworks offer lookup tables that we can use to bind the device to a resource. I want to be able to always use those instead of being forced to deliver the resources with platform_data in this kind of cases.. > > > > Signed-off-by: Heikki Krogerus <heikki.krogerus@linux.intel.com> > > --- > > drivers/base/platform.c | 77 ++++++++++++++++++++++++++++++------------------- > > 1 file changed, 47 insertions(+), 30 deletions(-) > > > > diff --git a/drivers/base/platform.c b/drivers/base/platform.c > > index 5b47210..697896d 100644 > > --- a/drivers/base/platform.c > > +++ b/drivers/base/platform.c > > @@ -174,11 +174,45 @@ struct platform_object { > > */ > > void platform_device_put(struct platform_device *pdev) > > { > > - if (pdev) > > - put_device(&pdev->dev); > > + if (!pdev) > > + return; > > + > > + if (pdev->id_auto) { > > + ida_simple_remove(&platform_devid_ida, pdev->id); > > + pdev->id = PLATFORM_DEVID_AUTO; > > + } > > + > > + put_device(&pdev->dev); > > } > > EXPORT_SYMBOL_GPL(platform_device_put); > > > > +static int pdev_set_name(struct platform_device *pdev) > > +{ > > + int ret; > > + > > + switch (pdev->id) { > > + default: > > + return dev_set_name(&pdev->dev, "%s.%d", pdev->name, pdev->id); > > + case PLATFORM_DEVID_NONE: > > + return dev_set_name(&pdev->dev, "%s", pdev->name); > > + case PLATFORM_DEVID_AUTO: > > + /* > > + * Automatically allocated device ID. We mark it as such so > > + * that we remember it must be freed, and we append a suffix > > + * to avoid namespace collision with explicit IDs. > > + */ > > + ret = ida_simple_get(&platform_devid_ida, 0, 0, GFP_KERNEL); > > + if (ret < 0) > > + return ret; > > + pdev->id = ret; > > + pdev->id_auto = true; > > + return dev_set_name(&pdev->dev, "%s.%d.auto", pdev->name, > > + pdev->id); > > + } > > + > > + return 0; > > +} > > + > > static void platform_device_release(struct device *dev) > > { > > struct platform_object *pa = container_of(dev, struct platform_object, > > @@ -211,6 +245,10 @@ struct platform_device *platform_device_alloc(const char *name, int id) > > device_initialize(&pa->pdev.dev); > > pa->pdev.dev.release = platform_device_release; > > arch_setup_pdev_archdata(&pa->pdev); > > + if (pdev_set_name(&pa->pdev)) { > > + kfree(pa); > > + return NULL; > > + } > > } > > > > return pa ? &pa->pdev : NULL; > > @@ -291,28 +329,6 @@ int platform_device_add(struct platform_device *pdev) > > > > pdev->dev.bus = &platform_bus_type; > > > > - switch (pdev->id) { > > - default: > > - dev_set_name(&pdev->dev, "%s.%d", pdev->name, pdev->id); > > - break; > > - case PLATFORM_DEVID_NONE: > > - dev_set_name(&pdev->dev, "%s", pdev->name); > > - break; > > - case PLATFORM_DEVID_AUTO: > > - /* > > - * Automatically allocated device ID. We mark it as such so > > - * that we remember it must be freed, and we append a suffix > > - * to avoid namespace collision with explicit IDs. > > - */ > > - ret = ida_simple_get(&platform_devid_ida, 0, 0, GFP_KERNEL); > > - if (ret < 0) > > - goto err_out; > > - pdev->id = ret; > > - pdev->id_auto = true; > > - dev_set_name(&pdev->dev, "%s.%d.auto", pdev->name, pdev->id); > > - break; > > - } > > - > > for (i = 0; i < pdev->num_resources; i++) { > > struct resource *p, *r = &pdev->resource[i]; > > > > @@ -355,7 +371,6 @@ int platform_device_add(struct platform_device *pdev) > > release_resource(r); > > } > > > > - err_out: > > return ret; > > } > > EXPORT_SYMBOL_GPL(platform_device_add); > > @@ -375,11 +390,6 @@ void platform_device_del(struct platform_device *pdev) > > if (pdev) { > > device_del(&pdev->dev); > > > > - if (pdev->id_auto) { > > - ida_simple_remove(&platform_devid_ida, pdev->id); > > - pdev->id = PLATFORM_DEVID_AUTO; > > - } > > - > > for (i = 0; i < pdev->num_resources; i++) { > > struct resource *r = &pdev->resource[i]; > > unsigned long type = resource_type(r); > > @@ -397,8 +407,15 @@ EXPORT_SYMBOL_GPL(platform_device_del); > > */ > > int platform_device_register(struct platform_device *pdev) > > { > > + int ret; > > + > > device_initialize(&pdev->dev); > > arch_setup_pdev_archdata(pdev); > > + > > + ret = pdev_set_name(pdev); > > + if (ret) > > + return ret; > > + > > return platform_device_add(pdev); > > } > > EXPORT_SYMBOL_GPL(platform_device_register); > > -- > > 2.0.0.rc0 > > I don't understand the usage of this, what drivers want/need this > change? The most common things where you have the parent sharing stuff like the clocks and GPIOs are the probe drivers, and quite often you see the platform_data being used there just for that. The first place I want to use this is drivers/usb/dwc3/host.c. We need to share at least the PHYs of the core dwc3 with the xHCI platform device that is created there. Now it's not possible to share them by adding a lookup entry because we can not be certain of the device name of the platform device before platform_device_add() is called, and that is of course too late. By the time it returns, xHCI will have already requested things like the PHYs. So I guess this is actually about reducing the need for driver specific platform data (or at least my hate towards them). With things like clocks, GPIOs, DMA channels, PHYs, etc., if the frameworks offer the lookup tables, the drivers really don't need the platform_data to get them. Actually, IMO it should always be possible and also enough for the drivers to simply request that kind of resources unconditionally, without any specific data. The drivers simply should not need to care about any details related to them. -- heikki ^ permalink raw reply [flat|nested] 3+ messages in thread
end of thread, other threads:[~2014-05-30 8:35 UTC | newest] Thread overview: 3+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2014-04-28 12:09 [PATCH] base: platform: name the device already during allocation Heikki Krogerus 2014-05-28 20:58 ` Greg KH 2014-05-30 8:35 ` Heikki Krogerus
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox