Message ID | 1277129511-2732-1-git-send-email-u.kleine-koenig@pengutronix.de |
---|---|
State | Not Applicable, archived |
Delegated to: | David Miller |
Headers | show |
On Mon, Jun 21, 2010 at 04:11:44PM +0200, Uwe Kleine-König wrote: > This makes the two similar functions platform_device_register_simple > and platform_device_register_data one line inline functions using a new > generic function platform_device_register_resndata. > > Signed-off-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de> > --- > Hello, > > still unsolved is the naming issue, what do you think about > platform_device_register? We already have a platform_device_register() function :) > I marked the new function as __init_or_module in a separate patch to > make reverting it a bit easier, still I think it should be possible to > fix the caller if a problem occurs. > > I changed the semantic slightly to only call > platform_device_add_resources if data != NULL instead of size != 0. The > idea is to support wrappers like: > > #define add_blablub(id, pdata) \ > platform_device_register_resndata(NULL, "blablub", id, \ > NULL, 0, pdata, sizeof(struct blablub_platform_data)) > > that don't fail if pdata=NULL. Ditto for res. That's fine, but why would you want to have a #define for something like this? Is it really needed? Anyway, this version looks fine to me, I'll go apply it. thanks, greg k-h -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hi Greg, > > I changed the semantic slightly to only call > > platform_device_add_resources if data != NULL instead of size != 0. The > > idea is to support wrappers like: > > > > #define add_blablub(id, pdata) \ > > platform_device_register_resndata(NULL, "blablub", id, \ > > NULL, 0, pdata, sizeof(struct blablub_platform_data)) > > > > that don't fail if pdata=NULL. Ditto for res. > > That's fine, but why would you want to have a #define for something like > this? Is it really needed? Well, what is really needed? I intend to use it on arm/imx. I have several different machines using similar SoCs and so I want to have a function à la: struct platform_device *__init imx_add_imx_i2c(int id, resource_size_t iobase, resource_size_t iosize, int irq, const struct imxi2c_platform_data *pdata) that builds a struct resource[] and then calls platform_device_register_resndata(). And then I have a set of macros like: #define imx21_add_i2c_imx(pdata) \ imx_add_imx_i2c(0, MX2x_I2C_BASE_ADDR, SZ_4K, MX2x_INT_I2C, pdata) #define imx25_add_imx_i2c0(pdata) \ imx_add_imx_i2c(0, MX25_I2C1_BASE_ADDR, SZ_16K, MX25_INT_I2C1, pdata) ##define imx25_add_imx_i2c1(pdata) \ imx_add_imx_i2c(1, MX25_I2C2_BASE_ADDR, SZ_16K, MX25_INT_I2C2, pdata) etc. The final goal is to get rid of files like arch/arm/mach-mx3/devices.c. > Anyway, this version looks fine to me, I'll go apply it. \o/ Best regards and thanks Uwe
2010/6/22 Uwe Kleine-König <u.kleine-koenig@pengutronix.de>: > Hi Greg, > >> > I changed the semantic slightly to only call >> > platform_device_add_resources if data != NULL instead of size != 0. The >> > idea is to support wrappers like: >> > >> > #define add_blablub(id, pdata) \ >> > platform_device_register_resndata(NULL, "blablub", id, \ >> > NULL, 0, pdata, sizeof(struct blablub_platform_data)) >> > >> > that don't fail if pdata=NULL. Ditto for res. >> >> That's fine, but why would you want to have a #define for something like >> this? Is it really needed? > Well, what is really needed? I intend to use it on arm/imx. I have > several different machines using similar SoCs and so I want to have a > function à la: > > struct platform_device *__init imx_add_imx_i2c(int id, > resource_size_t iobase, resource_size_t iosize, int irq, > const struct imxi2c_platform_data *pdata) > > that builds a struct resource[] and then calls > platform_device_register_resndata(). And then I have a set of macros > like: > > #define imx21_add_i2c_imx(pdata) \ > imx_add_imx_i2c(0, MX2x_I2C_BASE_ADDR, SZ_4K, MX2x_INT_I2C, pdata) > #define imx25_add_imx_i2c0(pdata) \ > imx_add_imx_i2c(0, MX25_I2C1_BASE_ADDR, SZ_16K, MX25_INT_I2C1, pdata) > ##define imx25_add_imx_i2c1(pdata) \ > imx_add_imx_i2c(1, MX25_I2C2_BASE_ADDR, SZ_16K, MX25_INT_I2C2, pdata) > > etc. The final goal is to get rid of files like > arch/arm/mach-mx3/devices.c. > Hi Uwe, I suggest you to have a look into arch/arm/mach-mmp/devices.c and arch/arm/mach-mmp/pxa{168,910}.c as well as arch/arm/mach-mmp/include/mach/pxa{168,910}.h, maybe we can find some common practice. >> Anyway, this version looks fine to me, I'll go apply it. > \o/ > > Best regards and thanks > Uwe > > -- > Pengutronix e.K. | Uwe Kleine-König | > Industrial Linux Solutions | http://www.pengutronix.de/ | > -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hi Eric, On Mon, Jun 28, 2010 at 12:55:45PM +0800, Eric Miao wrote: > I suggest you to have a look into arch/arm/mach-mmp/devices.c and > arch/arm/mach-mmp/pxa{168,910}.c as well as > arch/arm/mach-mmp/include/mach/pxa{168,910}.h, maybe we can find > some common practice. I think I like this approach in general, I already thought about not passing all parameters as function/macro arguments, too. But maybe this becomes too excessive for imx as I would need too many of these device desc for the different imx variants?! Anyhow a few things I thought when looking in the files you suggested: - Why not use an array for all uart devdescs, maybe the code for pxa168_add_uart could become a bit smaller then?: extern struct pxa_device_desc pxa168_device_uart[2]; ... static inline int pxa168_add_uart(int id) { struct pxa_device_desc *d = pxa168_device_uart + id; if (id < 0 || id > 2) return -EINVAL; return pxa_register_device(d, NULL, 0); } (Ditto for the other types obviously.) - shouldn't all these pxa_device_descs and pxa168_add_$device functions be __initdata and __init? - pxa_register_device is better than my add_resndata function in (at least) one aspect as it sets coherent_dma_mask, too. This is something I missed when trying to add mxc-mmc (IIRC) devices. Thanks Uwe
2010/6/28 Uwe Kleine-König <u.kleine-koenig@pengutronix.de>: > Hi Eric, > > On Mon, Jun 28, 2010 at 12:55:45PM +0800, Eric Miao wrote: >> I suggest you to have a look into arch/arm/mach-mmp/devices.c and >> arch/arm/mach-mmp/pxa{168,910}.c as well as >> arch/arm/mach-mmp/include/mach/pxa{168,910}.h, maybe we can find >> some common practice. > I think I like this approach in general, I already thought about not > passing all parameters as function/macro arguments, too. But maybe this > becomes too excessive for imx as I would need too many of these device > desc for the different imx variants?! > > Anyhow a few things I thought when looking in the files you suggested: > > - Why not use an array for all uart devdescs, maybe the code for > pxa168_add_uart could become a bit smaller then?: > > extern struct pxa_device_desc pxa168_device_uart[2]; > ... > static inline int pxa168_add_uart(int id) > { > struct pxa_device_desc *d = pxa168_device_uart + id; > > if (id < 0 || id > 2) > return -EINVAL; > > return pxa_register_device(d, NULL, 0); > } > > (Ditto for the other types obviously.) That's a good suggestion, yet it came that way for two reasons: 1. the initial naming mess, uart0 was later renamed to uart1, e.g. 2. and the restrictions of PXA{168,910}_DEVICE() macros, these macros are handy to simplify the definition, but may require fancy tricks to make it support array > > - shouldn't all these pxa_device_descs and pxa168_add_$device functions > be __initdata and __init? > pxa{168,910}_add_device() are actually 'static inline' so my assumption is they will be inlined when referenced, otherwise won't occupy any code space. The *_descs, however, they are __initdata if you look into the definitions of PXA{168,910}_DEVICES > - pxa_register_device is better than my add_resndata function in (at > least) one aspect as it sets coherent_dma_mask, too. This is > something I missed when trying to add mxc-mmc (IIRC) devices. > > Thanks > Uwe > > -- > Pengutronix e.K. | Uwe Kleine-König | > Industrial Linux Solutions | http://www.pengutronix.de/ | > -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/Documentation/DocBook/device-drivers.tmpl b/Documentation/DocBook/device-drivers.tmpl index 1b2dd4f..ecd35e9 100644 --- a/Documentation/DocBook/device-drivers.tmpl +++ b/Documentation/DocBook/device-drivers.tmpl @@ -111,6 +111,7 @@ X!Edrivers/base/attribute_container.c <!-- X!Edrivers/base/interface.c --> +!Iinclude/linux/platform_device.h !Edrivers/base/platform.c !Edrivers/base/bus.c </sect1> diff --git a/drivers/base/platform.c b/drivers/base/platform.c index 26eb69d..ffcfd73 100644 --- a/drivers/base/platform.c +++ b/drivers/base/platform.c @@ -344,108 +344,56 @@ void platform_device_unregister(struct platform_device *pdev) EXPORT_SYMBOL_GPL(platform_device_unregister); /** - * platform_device_register_simple - add a platform-level device and its resources - * @name: base name of the device we're adding - * @id: instance id - * @res: set of resources that needs to be allocated for the device - * @num: number of resources - * - * This function creates a simple platform device that requires minimal - * resource and memory management. Canned release function freeing memory - * allocated for the device allows drivers using such devices to be - * unloaded without waiting for the last reference to the device to be - * dropped. + * platform_device_register_resndata - add a platform-level device with + * resources and platform-specific data * - * This interface is primarily intended for use with legacy drivers which - * probe hardware directly. Because such drivers create sysfs device nodes - * themselves, rather than letting system infrastructure handle such device - * enumeration tasks, they don't fully conform to the Linux driver model. - * In particular, when such drivers are built as modules, they can't be - * "hotplugged". - * - * Returns &struct platform_device pointer on success, or ERR_PTR() on error. - */ -struct platform_device *platform_device_register_simple(const char *name, - int id, - const struct resource *res, - unsigned int num) -{ - struct platform_device *pdev; - int retval; - - pdev = platform_device_alloc(name, id); - if (!pdev) { - retval = -ENOMEM; - goto error; - } - - if (num) { - retval = platform_device_add_resources(pdev, res, num); - if (retval) - goto error; - } - - retval = platform_device_add(pdev); - if (retval) - goto error; - - return pdev; - -error: - platform_device_put(pdev); - return ERR_PTR(retval); -} -EXPORT_SYMBOL_GPL(platform_device_register_simple); - -/** - * platform_device_register_data - add a platform-level device with platform-specific data * @parent: parent device for the device we're adding * @name: base name of the device we're adding * @id: instance id + * @res: set of resources that needs to be allocated for the device + * @num: number of resources * @data: platform specific data for this platform device * @size: size of platform specific data * - * This function creates a simple platform device that requires minimal - * resource and memory management. Canned release function freeing memory - * allocated for the device allows drivers using such devices to be - * unloaded without waiting for the last reference to the device to be - * dropped. - * * Returns &struct platform_device pointer on success, or ERR_PTR() on error. */ -struct platform_device *platform_device_register_data( +struct platform_device *platform_device_register_resndata( struct device *parent, const char *name, int id, + const struct resource *res, unsigned int num, const void *data, size_t size) { + int ret = -ENOMEM; struct platform_device *pdev; - int retval; pdev = platform_device_alloc(name, id); - if (!pdev) { - retval = -ENOMEM; - goto error; - } + if (!pdev) + goto err; pdev->dev.parent = parent; - if (size) { - retval = platform_device_add_data(pdev, data, size); - if (retval) - goto error; + if (res) { + ret = platform_device_add_resources(pdev, res, num); + if (ret) + goto err; } - retval = platform_device_add(pdev); - if (retval) - goto error; + if (data) { + ret = platform_device_add_data(pdev, data, size); + if (ret) + goto err; + } - return pdev; + ret = platform_device_add(pdev); + if (ret) { +err: + platform_device_put(pdev); + return ERR_PTR(ret); + } -error: - platform_device_put(pdev); - return ERR_PTR(retval); + return pdev; } -EXPORT_SYMBOL_GPL(platform_device_register_data); +EXPORT_SYMBOL_GPL(platform_device_register_resndata); static int platform_drv_probe(struct device *_dev) { diff --git a/include/linux/platform_device.h b/include/linux/platform_device.h index 5417944..d7ecad0 100644 --- a/include/linux/platform_device.h +++ b/include/linux/platform_device.h @@ -43,10 +43,64 @@ extern struct resource *platform_get_resource_byname(struct platform_device *, u extern int platform_get_irq_byname(struct platform_device *, const char *); extern int platform_add_devices(struct platform_device **, int); -extern struct platform_device *platform_device_register_simple(const char *, int id, - const struct resource *, unsigned int); -extern struct platform_device *platform_device_register_data(struct device *, - const char *, int, const void *, size_t); +extern struct platform_device *platform_device_register_resndata( + struct device *parent, const char *name, int id, + const struct resource *res, unsigned int num, + const void *data, size_t size); + +/** + * platform_device_register_simple - add a platform-level device and its resources + * @name: base name of the device we're adding + * @id: instance id + * @res: set of resources that needs to be allocated for the device + * @num: number of resources + * + * This function creates a simple platform device that requires minimal + * resource and memory management. Canned release function freeing memory + * allocated for the device allows drivers using such devices to be + * unloaded without waiting for the last reference to the device to be + * dropped. + * + * This interface is primarily intended for use with legacy drivers which + * probe hardware directly. Because such drivers create sysfs device nodes + * themselves, rather than letting system infrastructure handle such device + * enumeration tasks, they don't fully conform to the Linux driver model. + * In particular, when such drivers are built as modules, they can't be + * "hotplugged". + * + * Returns &struct platform_device pointer on success, or ERR_PTR() on error. + */ +static inline struct platform_device *platform_device_register_simple( + const char *name, int id, + const struct resource *res, unsigned int num) +{ + return platform_device_register_resndata(NULL, name, id, + res, num, NULL, 0); +} + +/** + * platform_device_register_data - add a platform-level device with platform-specific data + * @parent: parent device for the device we're adding + * @name: base name of the device we're adding + * @id: instance id + * @data: platform specific data for this platform device + * @size: size of platform specific data + * + * This function creates a simple platform device that requires minimal + * resource and memory management. Canned release function freeing memory + * allocated for the device allows drivers using such devices to be + * unloaded without waiting for the last reference to the device to be + * dropped. + * + * Returns &struct platform_device pointer on success, or ERR_PTR() on error. + */ +static inline struct platform_device *platform_device_register_data( + struct device *parent, const char *name, int id, + const void *data, size_t size) +{ + return platform_device_register_resndata(parent, name, id, + NULL, 0, data, size); +} extern struct platform_device *platform_device_alloc(const char *name, int id); extern int platform_device_add_resources(struct platform_device *pdev,
This makes the two similar functions platform_device_register_simple and platform_device_register_data one line inline functions using a new generic function platform_device_register_resndata. Signed-off-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de> --- Hello, still unsolved is the naming issue, what do you think about platform_device_register? I marked the new function as __init_or_module in a separate patch to make reverting it a bit easier, still I think it should be possible to fix the caller if a problem occurs. I changed the semantic slightly to only call platform_device_add_resources if data != NULL instead of size != 0. The idea is to support wrappers like: #define add_blablub(id, pdata) \ platform_device_register_resndata(NULL, "blablub", id, \ NULL, 0, pdata, sizeof(struct blablub_platform_data)) that don't fail if pdata=NULL. Ditto for res. Best regards Uwe changed since v1: - fix docbook to pick up platform_device_register_simple and platform_device_register_data after moving them to <linux/platform_device.h> - only add_resources and add_data if res and data are non-NULL resp. Documentation/DocBook/device-drivers.tmpl | 1 + drivers/base/platform.c | 104 +++++++--------------------- include/linux/platform_device.h | 62 ++++++++++++++++- 3 files changed, 85 insertions(+), 82 deletions(-)