lkml.org 
[lkml]   [2011]   [Jul]   [10]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
Patch in this message
/
Date
From
SubjectRe: [RFC PATCH v2 1/4] dt: expose device resource allocator
On Fri, Jul 08, 2011 at 09:54:07AM +0100, Marc Zyngier wrote:
> of_device_alloc() takes care of allocating both a platform
> device and its associated resource, as part of the runtime
> device-tree to platform_device conversion.
>
> The resource allocation itself is useful on its own, and
> would be used by the core driver subsystem. Create the
> of_device_alloc_resources() function, and let of_device_alloc()
> use it.
>
> Signed-off-by: Marc Zyngier <marc.zyngier@arm.com>

Hi Marc,

Comments below.

> ---
> drivers/of/platform.c | 60 +++++++++++++++++++++++++++++-------------
> include/linux/of_platform.h | 2 +
> 2 files changed, 43 insertions(+), 19 deletions(-)
>
> diff --git a/drivers/of/platform.c b/drivers/of/platform.c
> index e75af39..f839f79 100644
> --- a/drivers/of/platform.c
> +++ b/drivers/of/platform.c
> @@ -122,22 +122,16 @@ void of_device_make_bus_id(struct device *dev)
> }
>
> /**
> - * of_device_alloc - Allocate and initialize an of_device
> - * @np: device node to assign to device
> - * @bus_id: Name to assign to the device. May be null to use default name.
> - * @parent: Parent device.
> + * of_device_alloc_resources - Allocate and initialize resources for an
> + * of_device
> + * @np: device node to fetch resources from
> + * @num_res: number of allocated resources
> */
> -struct platform_device *of_device_alloc(struct device_node *np,
> - const char *bus_id,
> - struct device *parent)
> +struct resource *of_device_alloc_resources(struct device_node *np,
> + int *num_res)
> {
> - struct platform_device *dev;
> int rc, i, num_reg = 0, num_irq;
> - struct resource *res, temp_res;
> -
> - dev = platform_device_alloc("", -1);
> - if (!dev)
> - return NULL;
> + struct resource *res = NULL, temp_res;
>
> /* count the io and irq resources */
> while (of_address_to_resource(np, num_reg, &temp_res) == 0)
> @@ -148,19 +142,47 @@ struct platform_device *of_device_alloc(struct device_node *np,
> if (num_irq || num_reg) {
> res = kzalloc(sizeof(*res) * (num_irq + num_reg), GFP_KERNEL);
> if (!res) {
> - platform_device_put(dev);
> + *num_res = -1;

I would argue that it is not a good idea to modify num_res in the
failure case. Though I man not be fond of the pattern, the common
convention in the kernel is to use ERR_PTR when a function needs to
differentiate between a failure case and simply "no data".

> return NULL;
> }
>
> - dev->num_resources = num_reg + num_irq;
> - dev->resource = res;
> - for (i = 0; i < num_reg; i++, res++) {
> - rc = of_address_to_resource(np, i, res);
> + for (i = 0; i < num_reg; i++) {
> + rc = of_address_to_resource(np, i, &res[i]);

Nit: the patch would be less invasive if the 'res' pointer handling
was left alone. Just keep a copy of the starting position to use for
returning.

> WARN_ON(rc);
> }
> - WARN_ON(of_irq_to_resource_table(np, res, num_irq) != num_irq);
> + WARN_ON(of_irq_to_resource_table(np, &res[i], num_irq) != num_irq);
> + }
> +
> + *num_res = num_irq + num_reg;
> + return res;
> +}
> +
> +/**
> + * of_device_alloc - Allocate and initialize an of_device
> + * @np: device node to assign to device
> + * @bus_id: Name to assign to the device. May be null to use default name.
> + * @parent: Parent device.
> + */
> +struct platform_device *of_device_alloc(struct device_node *np,
> + const char *bus_id,
> + struct device *parent)
> +{
> + struct platform_device *dev;
> + int num_res;
> + struct resource *res;
> +
> + dev = platform_device_alloc("", -1);
> + if (!dev)
> + return NULL;
> +
> + res = of_device_alloc_resources(np, &num_res);
> + if (num_res == -1) {
> + platform_device_put(dev);
> + return NULL;
> }
>
> + dev->num_resources = num_res;
> + dev->resource = res;
> dev->dev.of_node = of_node_get(np);
> #if defined(CONFIG_PPC) || defined(CONFIG_MICROBLAZE)
> dev->dev.dma_mask = &dev->archdata.dma_mask;
> diff --git a/include/linux/of_platform.h b/include/linux/of_platform.h
> index 5a6f458..773a880 100644
> --- a/include/linux/of_platform.h
> +++ b/include/linux/of_platform.h
> @@ -77,6 +77,8 @@ struct of_platform_driver
> extern const struct of_device_id of_default_bus_match_table[];
>
> /* Platform drivers register/unregister */
> +extern struct resource *of_device_alloc_resources(struct device_node *np,
> + int *num_res);
> extern struct platform_device *of_device_alloc(struct device_node *np,
> const char *bus_id,
> struct device *parent);
> --
> 1.7.0.4
>
>


\
 
 \ /
  Last update: 2011-07-10 11:01    [W:0.107 / U:5.940 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site