* [patch 0/4] mfd updates and proposed changes @ 2008-07-09 10:49 Ben Dooks 2008-07-09 10:49 ` [patch 1/4] MFD: Use to_platform_device instead of container_of Ben Dooks ` (3 more replies) 0 siblings, 4 replies; 39+ messages in thread From: Ben Dooks @ 2008-07-09 10:49 UTC (permalink / raw) To: linux-kernel, linux-arm-kernel; +Cc: sameo, dbaryshkov Updates and changes to mfd -- ^ permalink raw reply [flat|nested] 39+ messages in thread
* [patch 1/4] MFD: Use to_platform_device instead of container_of. 2008-07-09 10:49 [patch 0/4] mfd updates and proposed changes Ben Dooks @ 2008-07-09 10:49 ` Ben Dooks 2008-07-09 11:10 ` Dmitry 2008-07-29 0:06 ` Samuel Ortiz 2008-07-09 10:49 ` [patch 2/4] MFD: Coding style fixes Ben Dooks ` (2 subsequent siblings) 3 siblings, 2 replies; 39+ messages in thread From: Ben Dooks @ 2008-07-09 10:49 UTC (permalink / raw) To: linux-kernel, linux-arm-kernel; +Cc: sameo, dbaryshkov, Ben Dooks [-- Attachment #1: mfd-fix-platform-device-translation.patch --] [-- Type: text/plain, Size: 744 bytes --] Convert mfd_remove_devices_fn() to use to_platform_device() instead of doing container_of(). Signed-off-by: Ben Dooks <ben-linux@fluff.org> Index: linux-2.6.26-rc9-next20080709/drivers/mfd/mfd-core.c =================================================================== --- linux-2.6.26-rc9-next20080709.orig/drivers/mfd/mfd-core.c 2008-07-09 10:13:53.000000000 +0100 +++ linux-2.6.26-rc9-next20080709/drivers/mfd/mfd-core.c 2008-07-09 10:14:26.000000000 +0100 @@ -99,8 +99,7 @@ EXPORT_SYMBOL(mfd_add_devices); static int mfd_remove_devices_fn(struct device *dev, void *unused) { - platform_device_unregister( - container_of(dev, struct platform_device, dev)); + platform_device_unregister(to_platform_device(dev)); return 0; } -- ^ permalink raw reply [flat|nested] 39+ messages in thread
* Re: [patch 1/4] MFD: Use to_platform_device instead of container_of. 2008-07-09 10:49 ` [patch 1/4] MFD: Use to_platform_device instead of container_of Ben Dooks @ 2008-07-09 11:10 ` Dmitry 2008-07-10 14:47 ` Samuel Ortiz 2008-07-29 0:06 ` Samuel Ortiz 1 sibling, 1 reply; 39+ messages in thread From: Dmitry @ 2008-07-09 11:10 UTC (permalink / raw) To: Ben Dooks; +Cc: linux-kernel, linux-arm-kernel, sameo 2008/7/9 Ben Dooks <ben-linux@fluff.org>: > Convert mfd_remove_devices_fn() to use to_platform_device() > instead of doing container_of(). > > Signed-off-by: Ben Dooks <ben-linux@fluff.org> Acked-by: Dmitry Baryshkov <dbaryshkov@gmail.com> > > Index: linux-2.6.26-rc9-next20080709/drivers/mfd/mfd-core.c > =================================================================== > --- linux-2.6.26-rc9-next20080709.orig/drivers/mfd/mfd-core.c 2008-07-09 10:13:53.000000000 +0100 > +++ linux-2.6.26-rc9-next20080709/drivers/mfd/mfd-core.c 2008-07-09 10:14:26.000000000 +0100 > @@ -99,8 +99,7 @@ EXPORT_SYMBOL(mfd_add_devices); > > static int mfd_remove_devices_fn(struct device *dev, void *unused) > { > - platform_device_unregister( > - container_of(dev, struct platform_device, dev)); > + platform_device_unregister(to_platform_device(dev)); > return 0; > } > > > -- > -- With best wishes Dmitry ^ permalink raw reply [flat|nested] 39+ messages in thread
* Re: [patch 1/4] MFD: Use to_platform_device instead of container_of. 2008-07-09 11:10 ` Dmitry @ 2008-07-10 14:47 ` Samuel Ortiz 0 siblings, 0 replies; 39+ messages in thread From: Samuel Ortiz @ 2008-07-10 14:47 UTC (permalink / raw) To: Dmitry; +Cc: Ben Dooks, linux-kernel, linux-arm-kernel On Wed, Jul 09, 2008 at 03:10:01PM +0400, Dmitry wrote: > 2008/7/9 Ben Dooks <ben-linux@fluff.org>: > > Convert mfd_remove_devices_fn() to use to_platform_device() > > instead of doing container_of(). > > > > Signed-off-by: Ben Dooks <ben-linux@fluff.org> > > Acked-by: Dmitry Baryshkov <dbaryshkov@gmail.com> I'll apply this one after the merge window. Cheers, Samuel. > > > > Index: linux-2.6.26-rc9-next20080709/drivers/mfd/mfd-core.c > > =================================================================== > > --- linux-2.6.26-rc9-next20080709.orig/drivers/mfd/mfd-core.c 2008-07-09 10:13:53.000000000 +0100 > > +++ linux-2.6.26-rc9-next20080709/drivers/mfd/mfd-core.c 2008-07-09 10:14:26.000000000 +0100 > > @@ -99,8 +99,7 @@ EXPORT_SYMBOL(mfd_add_devices); > > > > static int mfd_remove_devices_fn(struct device *dev, void *unused) > > { > > - platform_device_unregister( > > - container_of(dev, struct platform_device, dev)); > > + platform_device_unregister(to_platform_device(dev)); > > return 0; > > } > > > > > > -- > > > > > > -- > With best wishes > Dmitry ^ permalink raw reply [flat|nested] 39+ messages in thread
* Re: [patch 1/4] MFD: Use to_platform_device instead of container_of. 2008-07-09 10:49 ` [patch 1/4] MFD: Use to_platform_device instead of container_of Ben Dooks 2008-07-09 11:10 ` Dmitry @ 2008-07-29 0:06 ` Samuel Ortiz 1 sibling, 0 replies; 39+ messages in thread From: Samuel Ortiz @ 2008-07-29 0:06 UTC (permalink / raw) To: Ben Dooks; +Cc: linux-kernel, linux-arm-kernel, dbaryshkov On Wed, Jul 09, 2008 at 11:49:17AM +0100, Ben Dooks wrote: > Convert mfd_remove_devices_fn() to use to_platform_device() > instead of doing container_of(). > > Signed-off-by: Ben Dooks <ben-linux@fluff.org> Applied to the MFD tree, thanks. > Index: linux-2.6.26-rc9-next20080709/drivers/mfd/mfd-core.c > =================================================================== > --- linux-2.6.26-rc9-next20080709.orig/drivers/mfd/mfd-core.c 2008-07-09 10:13:53.000000000 +0100 > +++ linux-2.6.26-rc9-next20080709/drivers/mfd/mfd-core.c 2008-07-09 10:14:26.000000000 +0100 > @@ -99,8 +99,7 @@ EXPORT_SYMBOL(mfd_add_devices); > > static int mfd_remove_devices_fn(struct device *dev, void *unused) > { > - platform_device_unregister( > - container_of(dev, struct platform_device, dev)); > + platform_device_unregister(to_platform_device(dev)); > return 0; > } > > > -- ^ permalink raw reply [flat|nested] 39+ messages in thread
* [patch 2/4] MFD: Coding style fixes 2008-07-09 10:49 [patch 0/4] mfd updates and proposed changes Ben Dooks 2008-07-09 10:49 ` [patch 1/4] MFD: Use to_platform_device instead of container_of Ben Dooks @ 2008-07-09 10:49 ` Ben Dooks 2008-07-09 11:11 ` Dmitry 2008-07-29 0:07 ` Samuel Ortiz 2008-07-09 10:49 ` [patch 3/4] MFD: Remove unnecessary fields if mfd_cell structure Ben Dooks 2008-07-09 10:49 ` [patch 4/4] MFD: Change mfd platform device usage to wrapper platform_device Ben Dooks 3 siblings, 2 replies; 39+ messages in thread From: Ben Dooks @ 2008-07-09 10:49 UTC (permalink / raw) To: linux-kernel, linux-arm-kernel; +Cc: sameo, dbaryshkov, Ben Dooks [-- Attachment #1: mfd-fix-codingstyle.patch --] [-- Type: text/plain, Size: 2376 bytes --] Fix some coding style fixes in the mfd core driver. Signed-off-by: Ben Dooks <ben-linux@fluff.org> Index: linux-2.6.26-rc9-next20080709/drivers/mfd/mfd-core.c =================================================================== --- linux-2.6.26-rc9-next20080709.orig/drivers/mfd/mfd-core.c 2008-07-09 10:43:54.000000000 +0100 +++ linux-2.6.26-rc9-next20080709/drivers/mfd/mfd-core.c 2008-07-09 10:44:45.000000000 +0100 @@ -16,9 +16,9 @@ #include <linux/mfd/core.h> static int mfd_add_device(struct platform_device *parent, - const struct mfd_cell *cell, - struct resource *mem_base, - int irq_base) + const struct mfd_cell *cell, + struct resource *mem_base, + int irq_base) { struct resource res[cell->num_resources]; struct platform_device *pdev; @@ -75,11 +75,10 @@ fail_alloc: return ret; } -int mfd_add_devices( - struct platform_device *parent, - const struct mfd_cell *cells, int n_devs, - struct resource *mem_base, - int irq_base) +int mfd_add_devices(struct platform_device *parent, + const struct mfd_cell *cells, int n_devs, + struct resource *mem_base, + int irq_base) { int i; int ret = 0; Index: linux-2.6.26-rc9-next20080709/include/linux/mfd/core.h =================================================================== --- linux-2.6.26-rc9-next20080709.orig/include/linux/mfd/core.h 2008-07-09 10:43:54.000000000 +0100 +++ linux-2.6.26-rc9-next20080709/include/linux/mfd/core.h 2008-07-09 10:44:45.000000000 +0100 @@ -1,5 +1,3 @@ -#ifndef MFD_CORE_H -#define MFD_CORE_H /* * drivers/mfd/mfd-core.h * @@ -13,6 +11,9 @@ * */ +#ifndef MFD_CORE_H +#define MFD_CORE_H + #include <linux/platform_device.h> /* @@ -38,17 +39,15 @@ struct mfd_cell { const struct resource *resources; }; -static inline struct mfd_cell * -mfd_get_cell(struct platform_device *pdev) +static inline struct mfd_cell *mfd_get_cell(struct platform_device *pdev) { return (struct mfd_cell *)pdev->dev.platform_data; } -extern int mfd_add_devices( - struct platform_device *parent, - const struct mfd_cell *cells, int n_devs, - struct resource *mem_base, - int irq_base); +extern int mfd_add_devices(struct platform_device *parent, + const struct mfd_cell *cells, int n_devs, + struct resource *mem_base, + int irq_base); extern void mfd_remove_devices(struct platform_device *parent); -- ^ permalink raw reply [flat|nested] 39+ messages in thread
* Re: [patch 2/4] MFD: Coding style fixes 2008-07-09 10:49 ` [patch 2/4] MFD: Coding style fixes Ben Dooks @ 2008-07-09 11:11 ` Dmitry 2008-07-09 11:12 ` Ben Dooks 2008-07-09 11:46 ` ian 2008-07-29 0:07 ` Samuel Ortiz 1 sibling, 2 replies; 39+ messages in thread From: Dmitry @ 2008-07-09 11:11 UTC (permalink / raw) To: Ben Dooks; +Cc: linux-kernel, linux-arm-kernel, sameo 2008/7/9 Ben Dooks <ben-linux@fluff.org>: > Fix some coding style fixes in the mfd core driver. > > Signed-off-by: Ben Dooks <ben-linux@fluff.org> I don't have a strong feeling about this. As it's pretty much only whitespace changes, my feelings are closer to NAK. Leaving decision to Samuel or Ian. > > Index: linux-2.6.26-rc9-next20080709/drivers/mfd/mfd-core.c > =================================================================== > --- linux-2.6.26-rc9-next20080709.orig/drivers/mfd/mfd-core.c 2008-07-09 10:43:54.000000000 +0100 > +++ linux-2.6.26-rc9-next20080709/drivers/mfd/mfd-core.c 2008-07-09 10:44:45.000000000 +0100 > @@ -16,9 +16,9 @@ > #include <linux/mfd/core.h> > > static int mfd_add_device(struct platform_device *parent, > - const struct mfd_cell *cell, > - struct resource *mem_base, > - int irq_base) > + const struct mfd_cell *cell, > + struct resource *mem_base, > + int irq_base) > { > struct resource res[cell->num_resources]; > struct platform_device *pdev; > @@ -75,11 +75,10 @@ fail_alloc: > return ret; > } > > -int mfd_add_devices( > - struct platform_device *parent, > - const struct mfd_cell *cells, int n_devs, > - struct resource *mem_base, > - int irq_base) > +int mfd_add_devices(struct platform_device *parent, > + const struct mfd_cell *cells, int n_devs, > + struct resource *mem_base, > + int irq_base) > { > int i; > int ret = 0; > Index: linux-2.6.26-rc9-next20080709/include/linux/mfd/core.h > =================================================================== > --- linux-2.6.26-rc9-next20080709.orig/include/linux/mfd/core.h 2008-07-09 10:43:54.000000000 +0100 > +++ linux-2.6.26-rc9-next20080709/include/linux/mfd/core.h 2008-07-09 10:44:45.000000000 +0100 > @@ -1,5 +1,3 @@ > -#ifndef MFD_CORE_H > -#define MFD_CORE_H > /* > * drivers/mfd/mfd-core.h > * > @@ -13,6 +11,9 @@ > * > */ > > +#ifndef MFD_CORE_H > +#define MFD_CORE_H > + > #include <linux/platform_device.h> > > /* > @@ -38,17 +39,15 @@ struct mfd_cell { > const struct resource *resources; > }; > > -static inline struct mfd_cell * > -mfd_get_cell(struct platform_device *pdev) > +static inline struct mfd_cell *mfd_get_cell(struct platform_device *pdev) > { > return (struct mfd_cell *)pdev->dev.platform_data; > } > > -extern int mfd_add_devices( > - struct platform_device *parent, > - const struct mfd_cell *cells, int n_devs, > - struct resource *mem_base, > - int irq_base); > +extern int mfd_add_devices(struct platform_device *parent, > + const struct mfd_cell *cells, int n_devs, > + struct resource *mem_base, > + int irq_base); > > extern void mfd_remove_devices(struct platform_device *parent); > > > -- > -- With best wishes Dmitry ^ permalink raw reply [flat|nested] 39+ messages in thread
* Re: [patch 2/4] MFD: Coding style fixes 2008-07-09 11:11 ` Dmitry @ 2008-07-09 11:12 ` Ben Dooks 2008-07-10 14:48 ` Samuel Ortiz 2008-07-09 11:46 ` ian 1 sibling, 1 reply; 39+ messages in thread From: Ben Dooks @ 2008-07-09 11:12 UTC (permalink / raw) To: Dmitry; +Cc: Ben Dooks, linux-kernel, linux-arm-kernel, sameo On Wed, Jul 09, 2008 at 03:11:21PM +0400, Dmitry wrote: > 2008/7/9 Ben Dooks <ben-linux@fluff.org>: > > Fix some coding style fixes in the mfd core driver. > > > > Signed-off-by: Ben Dooks <ben-linux@fluff.org> > > I don't have a strong feeling about this. As it's pretty much only > whitespace changes, > my feelings are closer to NAK. Leaving decision to Samuel or Ian. The coding style is all over the place, this makes it far more readable. > > > > Index: linux-2.6.26-rc9-next20080709/drivers/mfd/mfd-core.c > > =================================================================== > > --- linux-2.6.26-rc9-next20080709.orig/drivers/mfd/mfd-core.c 2008-07-09 10:43:54.000000000 +0100 > > +++ linux-2.6.26-rc9-next20080709/drivers/mfd/mfd-core.c 2008-07-09 10:44:45.000000000 +0100 > > @@ -16,9 +16,9 @@ > > #include <linux/mfd/core.h> > > > > static int mfd_add_device(struct platform_device *parent, > > - const struct mfd_cell *cell, > > - struct resource *mem_base, > > - int irq_base) > > + const struct mfd_cell *cell, > > + struct resource *mem_base, > > + int irq_base) > > { > > struct resource res[cell->num_resources]; > > struct platform_device *pdev; > > @@ -75,11 +75,10 @@ fail_alloc: > > return ret; > > } > > > > -int mfd_add_devices( > > - struct platform_device *parent, > > - const struct mfd_cell *cells, int n_devs, > > - struct resource *mem_base, > > - int irq_base) > > +int mfd_add_devices(struct platform_device *parent, > > + const struct mfd_cell *cells, int n_devs, > > + struct resource *mem_base, > > + int irq_base) > > { > > int i; > > int ret = 0; > > Index: linux-2.6.26-rc9-next20080709/include/linux/mfd/core.h > > =================================================================== > > --- linux-2.6.26-rc9-next20080709.orig/include/linux/mfd/core.h 2008-07-09 10:43:54.000000000 +0100 > > +++ linux-2.6.26-rc9-next20080709/include/linux/mfd/core.h 2008-07-09 10:44:45.000000000 +0100 > > @@ -1,5 +1,3 @@ > > -#ifndef MFD_CORE_H > > -#define MFD_CORE_H > > /* > > * drivers/mfd/mfd-core.h > > * > > @@ -13,6 +11,9 @@ > > * > > */ > > > > +#ifndef MFD_CORE_H > > +#define MFD_CORE_H > > + > > #include <linux/platform_device.h> > > > > /* > > @@ -38,17 +39,15 @@ struct mfd_cell { > > const struct resource *resources; > > }; > > > > -static inline struct mfd_cell * > > -mfd_get_cell(struct platform_device *pdev) > > +static inline struct mfd_cell *mfd_get_cell(struct platform_device *pdev) > > { > > return (struct mfd_cell *)pdev->dev.platform_data; > > } > > > > -extern int mfd_add_devices( > > - struct platform_device *parent, > > - const struct mfd_cell *cells, int n_devs, > > - struct resource *mem_base, > > - int irq_base); > > +extern int mfd_add_devices(struct platform_device *parent, > > + const struct mfd_cell *cells, int n_devs, > > + struct resource *mem_base, > > + int irq_base); > > > > extern void mfd_remove_devices(struct platform_device *parent); > > > > > > -- > > > > > > -- > With best wishes > Dmitry -- -- Ben Q: What's a light-year? A: One-third less calories than a regular year. ^ permalink raw reply [flat|nested] 39+ messages in thread
* Re: [patch 2/4] MFD: Coding style fixes 2008-07-09 11:12 ` Ben Dooks @ 2008-07-10 14:48 ` Samuel Ortiz 0 siblings, 0 replies; 39+ messages in thread From: Samuel Ortiz @ 2008-07-10 14:48 UTC (permalink / raw) To: Ben Dooks; +Cc: Dmitry, linux-kernel, linux-arm-kernel On Wed, Jul 09, 2008 at 12:12:56PM +0100, Ben Dooks wrote: > On Wed, Jul 09, 2008 at 03:11:21PM +0400, Dmitry wrote: > > 2008/7/9 Ben Dooks <ben-linux@fluff.org>: > > > Fix some coding style fixes in the mfd core driver. > > > > > > Signed-off-by: Ben Dooks <ben-linux@fluff.org> > > > > I don't have a strong feeling about this. As it's pretty much only > > whitespace changes, > > my feelings are closer to NAK. Leaving decision to Samuel or Ian. > > The coding style is all over the place, this makes it far more readable. I agree. I'll apply it after the merge window as well. Cheers, Samuel. > > > > > > Index: linux-2.6.26-rc9-next20080709/drivers/mfd/mfd-core.c > > > =================================================================== > > > --- linux-2.6.26-rc9-next20080709.orig/drivers/mfd/mfd-core.c 2008-07-09 10:43:54.000000000 +0100 > > > +++ linux-2.6.26-rc9-next20080709/drivers/mfd/mfd-core.c 2008-07-09 10:44:45.000000000 +0100 > > > @@ -16,9 +16,9 @@ > > > #include <linux/mfd/core.h> > > > > > > static int mfd_add_device(struct platform_device *parent, > > > - const struct mfd_cell *cell, > > > - struct resource *mem_base, > > > - int irq_base) > > > + const struct mfd_cell *cell, > > > + struct resource *mem_base, > > > + int irq_base) > > > { > > > struct resource res[cell->num_resources]; > > > struct platform_device *pdev; > > > @@ -75,11 +75,10 @@ fail_alloc: > > > return ret; > > > } > > > > > > -int mfd_add_devices( > > > - struct platform_device *parent, > > > - const struct mfd_cell *cells, int n_devs, > > > - struct resource *mem_base, > > > - int irq_base) > > > +int mfd_add_devices(struct platform_device *parent, > > > + const struct mfd_cell *cells, int n_devs, > > > + struct resource *mem_base, > > > + int irq_base) > > > { > > > int i; > > > int ret = 0; > > > Index: linux-2.6.26-rc9-next20080709/include/linux/mfd/core.h > > > =================================================================== > > > --- linux-2.6.26-rc9-next20080709.orig/include/linux/mfd/core.h 2008-07-09 10:43:54.000000000 +0100 > > > +++ linux-2.6.26-rc9-next20080709/include/linux/mfd/core.h 2008-07-09 10:44:45.000000000 +0100 > > > @@ -1,5 +1,3 @@ > > > -#ifndef MFD_CORE_H > > > -#define MFD_CORE_H > > > /* > > > * drivers/mfd/mfd-core.h > > > * > > > @@ -13,6 +11,9 @@ > > > * > > > */ > > > > > > +#ifndef MFD_CORE_H > > > +#define MFD_CORE_H > > > + > > > #include <linux/platform_device.h> > > > > > > /* > > > @@ -38,17 +39,15 @@ struct mfd_cell { > > > const struct resource *resources; > > > }; > > > > > > -static inline struct mfd_cell * > > > -mfd_get_cell(struct platform_device *pdev) > > > +static inline struct mfd_cell *mfd_get_cell(struct platform_device *pdev) > > > { > > > return (struct mfd_cell *)pdev->dev.platform_data; > > > } > > > > > > -extern int mfd_add_devices( > > > - struct platform_device *parent, > > > - const struct mfd_cell *cells, int n_devs, > > > - struct resource *mem_base, > > > - int irq_base); > > > +extern int mfd_add_devices(struct platform_device *parent, > > > + const struct mfd_cell *cells, int n_devs, > > > + struct resource *mem_base, > > > + int irq_base); > > > > > > extern void mfd_remove_devices(struct platform_device *parent); > > > > > > > > > -- > > > > > > > > > > > -- > > With best wishes > > Dmitry > > -- > -- > Ben > > Q: What's a light-year? > A: One-third less calories than a regular year. > ^ permalink raw reply [flat|nested] 39+ messages in thread
* Re: [patch 2/4] MFD: Coding style fixes 2008-07-09 11:11 ` Dmitry 2008-07-09 11:12 ` Ben Dooks @ 2008-07-09 11:46 ` ian 1 sibling, 0 replies; 39+ messages in thread From: ian @ 2008-07-09 11:46 UTC (permalink / raw) To: Dmitry; +Cc: Ben Dooks, linux-kernel, linux-arm-kernel On Wed, 2008-07-09 at 15:11 +0400, Dmitry wrote: > > Signed-off-by: Ben Dooks <ben-linux@fluff.org> > > I don't have a strong feeling about this. As it's pretty much only > whitespace changes, my feelings are closer to NAK. Leaving decision > to Samuel or Ian. I like whitespace-fixing patches... so I'm ok with it. ^ permalink raw reply [flat|nested] 39+ messages in thread
* Re: [patch 2/4] MFD: Coding style fixes 2008-07-09 10:49 ` [patch 2/4] MFD: Coding style fixes Ben Dooks 2008-07-09 11:11 ` Dmitry @ 2008-07-29 0:07 ` Samuel Ortiz 1 sibling, 0 replies; 39+ messages in thread From: Samuel Ortiz @ 2008-07-29 0:07 UTC (permalink / raw) To: Ben Dooks; +Cc: linux-kernel, linux-arm-kernel, dbaryshkov On Wed, Jul 09, 2008 at 11:49:18AM +0100, Ben Dooks wrote: > Fix some coding style fixes in the mfd core driver. > > Signed-off-by: Ben Dooks <ben-linux@fluff.org> Applied as well, thanks. Cheers, Samuel. > Index: linux-2.6.26-rc9-next20080709/drivers/mfd/mfd-core.c > =================================================================== > --- linux-2.6.26-rc9-next20080709.orig/drivers/mfd/mfd-core.c 2008-07-09 10:43:54.000000000 +0100 > +++ linux-2.6.26-rc9-next20080709/drivers/mfd/mfd-core.c 2008-07-09 10:44:45.000000000 +0100 > @@ -16,9 +16,9 @@ > #include <linux/mfd/core.h> > > static int mfd_add_device(struct platform_device *parent, > - const struct mfd_cell *cell, > - struct resource *mem_base, > - int irq_base) > + const struct mfd_cell *cell, > + struct resource *mem_base, > + int irq_base) > { > struct resource res[cell->num_resources]; > struct platform_device *pdev; > @@ -75,11 +75,10 @@ fail_alloc: > return ret; > } > > -int mfd_add_devices( > - struct platform_device *parent, > - const struct mfd_cell *cells, int n_devs, > - struct resource *mem_base, > - int irq_base) > +int mfd_add_devices(struct platform_device *parent, > + const struct mfd_cell *cells, int n_devs, > + struct resource *mem_base, > + int irq_base) > { > int i; > int ret = 0; > Index: linux-2.6.26-rc9-next20080709/include/linux/mfd/core.h > =================================================================== > --- linux-2.6.26-rc9-next20080709.orig/include/linux/mfd/core.h 2008-07-09 10:43:54.000000000 +0100 > +++ linux-2.6.26-rc9-next20080709/include/linux/mfd/core.h 2008-07-09 10:44:45.000000000 +0100 > @@ -1,5 +1,3 @@ > -#ifndef MFD_CORE_H > -#define MFD_CORE_H > /* > * drivers/mfd/mfd-core.h > * > @@ -13,6 +11,9 @@ > * > */ > > +#ifndef MFD_CORE_H > +#define MFD_CORE_H > + > #include <linux/platform_device.h> > > /* > @@ -38,17 +39,15 @@ struct mfd_cell { > const struct resource *resources; > }; > > -static inline struct mfd_cell * > -mfd_get_cell(struct platform_device *pdev) > +static inline struct mfd_cell *mfd_get_cell(struct platform_device *pdev) > { > return (struct mfd_cell *)pdev->dev.platform_data; > } > > -extern int mfd_add_devices( > - struct platform_device *parent, > - const struct mfd_cell *cells, int n_devs, > - struct resource *mem_base, > - int irq_base); > +extern int mfd_add_devices(struct platform_device *parent, > + const struct mfd_cell *cells, int n_devs, > + struct resource *mem_base, > + int irq_base); > > extern void mfd_remove_devices(struct platform_device *parent); > > > -- ^ permalink raw reply [flat|nested] 39+ messages in thread
* [patch 3/4] MFD: Remove unnecessary fields if mfd_cell structure. 2008-07-09 10:49 [patch 0/4] mfd updates and proposed changes Ben Dooks 2008-07-09 10:49 ` [patch 1/4] MFD: Use to_platform_device instead of container_of Ben Dooks 2008-07-09 10:49 ` [patch 2/4] MFD: Coding style fixes Ben Dooks @ 2008-07-09 10:49 ` Ben Dooks 2008-07-09 11:09 ` Dmitry 2008-07-09 10:49 ` [patch 4/4] MFD: Change mfd platform device usage to wrapper platform_device Ben Dooks 3 siblings, 1 reply; 39+ messages in thread From: Ben Dooks @ 2008-07-09 10:49 UTC (permalink / raw) To: linux-kernel, linux-arm-kernel; +Cc: sameo, dbaryshkov, Ben Dooks [-- Attachment #1: mfd-remove-silly-entries.patch --] [-- Type: text/plain, Size: 880 bytes --] The enable,disable,suspend and resume entry in the struct mfd_cell do not seem to be necessary as they should be handled by the platform driver that the device created has bound to. Signed-off-by: Ben Dooks <ben-linux@fluff.org> Index: linux-2.6.26-rc9-next20080709/include/linux/mfd/core.h =================================================================== --- linux-2.6.26-rc9-next20080709.orig/include/linux/mfd/core.h 2008-07-09 10:44:45.000000000 +0100 +++ linux-2.6.26-rc9-next20080709/include/linux/mfd/core.h 2008-07-09 10:45:06.000000000 +0100 @@ -23,12 +26,6 @@ */ struct mfd_cell { const char *name; - - int (*enable)(struct platform_device *dev); - int (*disable)(struct platform_device *dev); - int (*suspend)(struct platform_device *dev); - int (*resume)(struct platform_device *dev); - void *driver_data; /* driver-specific data */ /* -- ^ permalink raw reply [flat|nested] 39+ messages in thread
* Re: [patch 3/4] MFD: Remove unnecessary fields if mfd_cell structure. 2008-07-09 10:49 ` [patch 3/4] MFD: Remove unnecessary fields if mfd_cell structure Ben Dooks @ 2008-07-09 11:09 ` Dmitry 2008-07-09 11:12 ` Ben Dooks 0 siblings, 1 reply; 39+ messages in thread From: Dmitry @ 2008-07-09 11:09 UTC (permalink / raw) To: Ben Dooks; +Cc: linux-kernel, linux-arm-kernel, sameo 2008/7/9 Ben Dooks <ben-linux@fluff.org>: > The enable,disable,suspend and resume entry in the struct mfd_cell > do not seem to be necessary as they should be handled by the > platform driver that the device created has bound to. NAK. It's used e.g. by the tmio-nand driver. These fields provide easy way to support enhanced power management, etc. > > Signed-off-by: Ben Dooks <ben-linux@fluff.org> > > Index: linux-2.6.26-rc9-next20080709/include/linux/mfd/core.h > =================================================================== > --- linux-2.6.26-rc9-next20080709.orig/include/linux/mfd/core.h 2008-07-09 10:44:45.000000000 +0100 > +++ linux-2.6.26-rc9-next20080709/include/linux/mfd/core.h 2008-07-09 10:45:06.000000000 +0100 > @@ -23,12 +26,6 @@ > */ > struct mfd_cell { > const char *name; > - > - int (*enable)(struct platform_device *dev); > - int (*disable)(struct platform_device *dev); > - int (*suspend)(struct platform_device *dev); > - int (*resume)(struct platform_device *dev); > - > void *driver_data; /* driver-specific data */ > > /* > > -- > -- With best wishes Dmitry ^ permalink raw reply [flat|nested] 39+ messages in thread
* Re: [patch 3/4] MFD: Remove unnecessary fields if mfd_cell structure. 2008-07-09 11:09 ` Dmitry @ 2008-07-09 11:12 ` Ben Dooks 2008-07-09 11:16 ` Dmitry 0 siblings, 1 reply; 39+ messages in thread From: Ben Dooks @ 2008-07-09 11:12 UTC (permalink / raw) To: Dmitry; +Cc: Ben Dooks, linux-kernel, linux-arm-kernel, sameo On Wed, Jul 09, 2008 at 03:09:12PM +0400, Dmitry wrote: > 2008/7/9 Ben Dooks <ben-linux@fluff.org>: > > The enable,disable,suspend and resume entry in the struct mfd_cell > > do not seem to be necessary as they should be handled by the > > platform driver that the device created has bound to. > > NAK. It's used e.g. by the tmio-nand driver. These fields provide easy > way to support > enhanced power management, etc. What enhancements over the standard platform device model do you need? > > > > Signed-off-by: Ben Dooks <ben-linux@fluff.org> > > > > Index: linux-2.6.26-rc9-next20080709/include/linux/mfd/core.h > > =================================================================== > > --- linux-2.6.26-rc9-next20080709.orig/include/linux/mfd/core.h 2008-07-09 10:44:45.000000000 +0100 > > +++ linux-2.6.26-rc9-next20080709/include/linux/mfd/core.h 2008-07-09 10:45:06.000000000 +0100 > > @@ -23,12 +26,6 @@ > > */ > > struct mfd_cell { > > const char *name; > > - > > - int (*enable)(struct platform_device *dev); > > - int (*disable)(struct platform_device *dev); > > - int (*suspend)(struct platform_device *dev); > > - int (*resume)(struct platform_device *dev); > > - > > void *driver_data; /* driver-specific data */ > > > > /* > > > > -- > > > > > > -- > With best wishes > Dmitry -- -- Ben Q: What's a light-year? A: One-third less calories than a regular year. ^ permalink raw reply [flat|nested] 39+ messages in thread
* Re: [patch 3/4] MFD: Remove unnecessary fields if mfd_cell structure. 2008-07-09 11:12 ` Ben Dooks @ 2008-07-09 11:16 ` Dmitry 2008-07-09 11:38 ` Ben Dooks 0 siblings, 1 reply; 39+ messages in thread From: Dmitry @ 2008-07-09 11:16 UTC (permalink / raw) To: Ben Dooks; +Cc: linux-kernel, linux-arm-kernel, sameo 2008/7/9 Ben Dooks <ben-linux@fluff.org>: > On Wed, Jul 09, 2008 at 03:09:12PM +0400, Dmitry wrote: >> 2008/7/9 Ben Dooks <ben-linux@fluff.org>: >> > The enable,disable,suspend and resume entry in the struct mfd_cell >> > do not seem to be necessary as they should be handled by the >> > platform driver that the device created has bound to. >> >> NAK. It's used e.g. by the tmio-nand driver. These fields provide easy >> way to support >> enhanced power management, etc. > > What enhancements over the standard platform device model do you need? As an example of what we need, please check the tmio-nand driver. > >> > >> > Signed-off-by: Ben Dooks <ben-linux@fluff.org> >> > >> > Index: linux-2.6.26-rc9-next20080709/include/linux/mfd/core.h >> > =================================================================== >> > --- linux-2.6.26-rc9-next20080709.orig/include/linux/mfd/core.h 2008-07-09 10:44:45.000000000 +0100 >> > +++ linux-2.6.26-rc9-next20080709/include/linux/mfd/core.h 2008-07-09 10:45:06.000000000 +0100 >> > @@ -23,12 +26,6 @@ >> > */ >> > struct mfd_cell { >> > const char *name; >> > - >> > - int (*enable)(struct platform_device *dev); >> > - int (*disable)(struct platform_device *dev); >> > - int (*suspend)(struct platform_device *dev); >> > - int (*resume)(struct platform_device *dev); >> > - >> > void *driver_data; /* driver-specific data */ >> > >> > /* >> > >> > -- >> > >> >> >> >> -- >> With best wishes >> Dmitry > > -- > -- > Ben > > Q: What's a light-year? > A: One-third less calories than a regular year. > > -- With best wishes Dmitry ^ permalink raw reply [flat|nested] 39+ messages in thread
* Re: [patch 3/4] MFD: Remove unnecessary fields if mfd_cell structure. 2008-07-09 11:16 ` Dmitry @ 2008-07-09 11:38 ` Ben Dooks 2008-07-09 11:44 ` Dmitry 0 siblings, 1 reply; 39+ messages in thread From: Ben Dooks @ 2008-07-09 11:38 UTC (permalink / raw) To: Dmitry; +Cc: Ben Dooks, linux-kernel, linux-arm-kernel On Wed, Jul 09, 2008 at 03:16:47PM +0400, Dmitry wrote: > 2008/7/9 Ben Dooks <ben-linux@fluff.org>: > > On Wed, Jul 09, 2008 at 03:09:12PM +0400, Dmitry wrote: > >> 2008/7/9 Ben Dooks <ben-linux@fluff.org>: > >> > The enable,disable,suspend and resume entry in the struct mfd_cell > >> > do not seem to be necessary as they should be handled by the > >> > platform driver that the device created has bound to. > >> > >> NAK. It's used e.g. by the tmio-nand driver. These fields provide easy > >> way to support > >> enhanced power management, etc. > > > > What enhancements over the standard platform device model do you need? > > As an example of what we need, please check the tmio-nand driver. As posted below, the driver on the linux-mtd list does not use this, and further more in [1] the method of getting to the the mfd_cell from a given platform_device has gone which would be used by [2] if it actually touched the mfd_cell information. [1] http://lists.arm.linux.org.uk/lurker/message/20080708.153450.bb33046d.en.html [2] http://lists.infradead.org/pipermail/linux-mtd/2008-June/022137.html > > > >> > > >> > Signed-off-by: Ben Dooks <ben-linux@fluff.org> > >> > > >> > Index: linux-2.6.26-rc9-next20080709/include/linux/mfd/core.h > >> > =================================================================== > >> > --- linux-2.6.26-rc9-next20080709.orig/include/linux/mfd/core.h 2008-07-09 10:44:45.000000000 +0100 > >> > +++ linux-2.6.26-rc9-next20080709/include/linux/mfd/core.h 2008-07-09 10:45:06.000000000 +0100 > >> > @@ -23,12 +26,6 @@ > >> > */ > >> > struct mfd_cell { > >> > const char *name; > >> > - > >> > - int (*enable)(struct platform_device *dev); > >> > - int (*disable)(struct platform_device *dev); > >> > - int (*suspend)(struct platform_device *dev); > >> > - int (*resume)(struct platform_device *dev); > >> > - > >> > void *driver_data; /* driver-specific data */ > >> > > >> > /* > >> > > >> > -- > >> > > >> > >> > >> > >> -- > >> With best wishes > >> Dmitry > > > > -- > > -- > > Ben > > > > Q: What's a light-year? > > A: One-third less calories than a regular year. > > > > > > > > -- > With best wishes > Dmitry > > ------------------------------------------------------------------- > List admin: http://lists.arm.linux.org.uk/mailman/listinfo/linux-arm-kernel > FAQ: http://www.arm.linux.org.uk/mailinglists/faq.php > Etiquette: http://www.arm.linux.org.uk/mailinglists/etiquette.php -- -- Ben Q: What's a light-year? A: One-third less calories than a regular year. ^ permalink raw reply [flat|nested] 39+ messages in thread
* Re: [patch 3/4] MFD: Remove unnecessary fields if mfd_cell structure. 2008-07-09 11:38 ` Ben Dooks @ 2008-07-09 11:44 ` Dmitry 0 siblings, 0 replies; 39+ messages in thread From: Dmitry @ 2008-07-09 11:44 UTC (permalink / raw) To: Ben Dooks; +Cc: linux-kernel, linux-arm-kernel 2008/7/9 Ben Dooks <ben-linux@fluff.org>: > On Wed, Jul 09, 2008 at 03:16:47PM +0400, Dmitry wrote: >> 2008/7/9 Ben Dooks <ben-linux@fluff.org>: >> > On Wed, Jul 09, 2008 at 03:09:12PM +0400, Dmitry wrote: >> >> 2008/7/9 Ben Dooks <ben-linux@fluff.org>: >> >> > The enable,disable,suspend and resume entry in the struct mfd_cell >> >> > do not seem to be necessary as they should be handled by the >> >> > platform driver that the device created has bound to. >> >> >> >> NAK. It's used e.g. by the tmio-nand driver. These fields provide easy >> >> way to support >> >> enhanced power management, etc. >> > >> > What enhancements over the standard platform device model do you need? >> >> As an example of what we need, please check the tmio-nand driver. > > As posted below, the driver on the linux-mtd list does not use this, > and further more in [1] the method of getting to the the mfd_cell from > a given platform_device has gone which would be used by [2] if it > actually touched the mfd_cell information. It is used. Go, search for mfd_get_cell. And yes, it will require a minor change as discussed in the mfd thread yesterday. > > [1] http://lists.arm.linux.org.uk/lurker/message/20080708.153450.bb33046d.en.html > [2] http://lists.infradead.org/pipermail/linux-mtd/2008-June/022137.html > >> > >> >> > >> >> > Signed-off-by: Ben Dooks <ben-linux@fluff.org> >> >> > >> >> > Index: linux-2.6.26-rc9-next20080709/include/linux/mfd/core.h >> >> > =================================================================== >> >> > --- linux-2.6.26-rc9-next20080709.orig/include/linux/mfd/core.h 2008-07-09 10:44:45.000000000 +0100 >> >> > +++ linux-2.6.26-rc9-next20080709/include/linux/mfd/core.h 2008-07-09 10:45:06.000000000 +0100 >> >> > @@ -23,12 +26,6 @@ >> >> > */ >> >> > struct mfd_cell { >> >> > const char *name; >> >> > - >> >> > - int (*enable)(struct platform_device *dev); >> >> > - int (*disable)(struct platform_device *dev); >> >> > - int (*suspend)(struct platform_device *dev); >> >> > - int (*resume)(struct platform_device *dev); >> >> > - >> >> > void *driver_data; /* driver-specific data */ >> >> > >> >> > /* >> >> > >> >> > -- >> >> > >> >> >> >> >> >> >> >> -- >> >> With best wishes >> >> Dmitry >> > >> > -- >> > -- >> > Ben >> > >> > Q: What's a light-year? >> > A: One-third less calories than a regular year. >> > >> > >> >> >> >> -- >> With best wishes >> Dmitry >> >> ------------------------------------------------------------------- >> List admin: http://lists.arm.linux.org.uk/mailman/listinfo/linux-arm-kernel >> FAQ: http://www.arm.linux.org.uk/mailinglists/faq.php >> Etiquette: http://www.arm.linux.org.uk/mailinglists/etiquette.php > > -- > -- > Ben > > Q: What's a light-year? > A: One-third less calories than a regular year. > > -- With best wishes Dmitry ^ permalink raw reply [flat|nested] 39+ messages in thread
* [patch 4/4] MFD: Change mfd platform device usage to wrapper platform_device 2008-07-09 10:49 [patch 0/4] mfd updates and proposed changes Ben Dooks ` (2 preceding siblings ...) 2008-07-09 10:49 ` [patch 3/4] MFD: Remove unnecessary fields if mfd_cell structure Ben Dooks @ 2008-07-09 10:49 ` Ben Dooks 2008-07-09 11:15 ` Dmitry 2008-07-09 20:56 ` Russell King - ARM Linux 3 siblings, 2 replies; 39+ messages in thread From: Ben Dooks @ 2008-07-09 10:49 UTC (permalink / raw) To: linux-kernel, linux-arm-kernel; +Cc: sameo, dbaryshkov, Ben Dooks [-- Attachment #1: mfd-dont-use-platform-device-data-to-store-mfd.patch --] [-- Type: text/plain, Size: 3748 bytes --] This patch changes the mfd core behaviour to wrapper the platform_device it creates in an struct mfd_device which contains the information about the cell that was created. 1) The creation of the resource list and then passing it to the platform_device_add_resources() causes the allocation of a large array on the stack as well as copying the source data twice (it is copied from the mfd_cell to the temporary array and then copied into the newly allocated array) 2) We can wrapper the platform_device into an mfd_device and use that to do the platform_device and resource allocation in one go to reduce the failiure. Note, is there actually any reason to pass the sub devices any information about the cell they are created from? The mfd core already makes the appropriate resource adjustments and anything else like clocks should be exported by the clock drivers? Signed-off-by: Ben Dooks <ben-linux@fluff.org> Index: linux-2.6.26-rc9-next20080709/include/linux/mfd/core.h =================================================================== --- linux-2.6.26-rc9-next20080709.orig/include/linux/mfd/core.h 2008-07-09 10:46:23.000000000 +0100 +++ linux-2.6.26-rc9-next20080709/include/linux/mfd/core.h 2008-07-09 11:14:55.000000000 +0100 @@ -18,8 +18,6 @@ /* * This struct describes the MFD part ("cell"). - * After registration the copy of this structure will become the platform data - * of the resulting platform_device */ struct mfd_cell { const char *name; @@ -33,9 +31,21 @@ struct mfd_cell { const struct resource *resources; }; +struct mfd_device { + struct platform_device pdev; + struct mfd_cell *cell; + struct resource resources[0]; +}; + +static inline struct mfd_device *to_mfd_device(struct device *dev) +{ + struct platform_device *pdev = to_platform_device(dev); + return container_of(pdev, struct mfd_device, pdev); +} + static inline struct mfd_cell *mfd_get_cell(struct platform_device *pdev) { - return (struct mfd_cell *)pdev->dev.platform_data; + return container_of(pdev, struct mfd_device, pdev)->cell; } extern int mfd_add_devices(struct platform_device *parent, Index: linux-2.6.26-rc9-next20080709/drivers/mfd/mfd-core.c =================================================================== --- linux-2.6.26-rc9-next20080709.orig/drivers/mfd/mfd-core.c 2008-07-09 10:59:54.000000000 +0100 +++ linux-2.6.26-rc9-next20080709/drivers/mfd/mfd-core.c 2008-07-09 11:09:59.000000000 +0100 @@ -15,28 +15,41 @@ #include <linux/platform_device.h> #include <linux/mfd/core.h> +static void mfd_dev_release(struct device *dev) +{ + struct mfd_device *mdev = to_mfd_device(dev); + + kfree(mdev); +} + static int mfd_add_device(struct platform_device *parent, const struct mfd_cell *cell, struct resource *mem_base, int irq_base) { - struct resource res[cell->num_resources]; + struct resource *res; + struct mfd_device *mdev; struct platform_device *pdev; int ret = -ENOMEM; int r; - pdev = platform_device_alloc(cell->name, parent->id); - if (!pdev) + mdev = kzalloc(sizeof(struct mfd_device) + + sizeof(struct resource) * cell->num_resources, + GFP_KERNEL); + if (!mdev) goto fail_alloc; - pdev->dev.parent = &parent->dev; + mdev->cell = cell; + mdev->pdev.dev.parent = &parent->dev; - ret = platform_device_add_data(pdev, - cell, sizeof(struct mfd_cell)); - if (ret) - goto fail_device; + pdev = &mdev->pdev; + res = &mdev->resources; + + device_initialise(&pdev->dev); + pdev->id = parent->id; + pdev->name = cell->name; + pdev->release = mfd_device_release; - memzero(res, sizeof(res)); for (r = 0; r < cell->num_resources; r++) { res[r].name = cell->resources[r].name; res[r].flags = cell->resources[r].flags; -- ^ permalink raw reply [flat|nested] 39+ messages in thread
* Re: [patch 4/4] MFD: Change mfd platform device usage to wrapper platform_device 2008-07-09 10:49 ` [patch 4/4] MFD: Change mfd platform device usage to wrapper platform_device Ben Dooks @ 2008-07-09 11:15 ` Dmitry 2008-07-09 11:24 ` Ben Dooks 2008-07-09 11:45 ` ian 2008-07-09 20:56 ` Russell King - ARM Linux 1 sibling, 2 replies; 39+ messages in thread From: Dmitry @ 2008-07-09 11:15 UTC (permalink / raw) To: Ben Dooks; +Cc: linux-kernel, linux-arm-kernel, sameo 2008/7/9 Ben Dooks <ben-linux@fluff.org>: > This patch changes the mfd core behaviour to wrapper the platform_device > it creates in an struct mfd_device which contains the information > about the cell that was created. > > 1) The creation of the resource list and then passing it to the > platform_device_add_resources() causes the allocation of a > large array on the stack as well as copying the source data > twice (it is copied from the mfd_cell to the temporary array > and then copied into the newly allocated array) > > 2) We can wrapper the platform_device into an mfd_device and use > that to do the platform_device and resource allocation in one > go to reduce the failiure. > > Note, is there actually any reason to pass the sub devices any > information about the cell they are created from? The mfd core > already makes the appropriate resource adjustments and anything > else like clocks should be exported by the clock drivers? > > Signed-off-by: Ben Dooks <ben-linux@fluff.org> NAK. 0) It was discussed yesterday on the list and the decision was to go in a different way. I've provided a bit cleaner patch with the same idea, but then we decided to go in a bit different way. 1) I prefer patch by Mike Rapoport which is more clear and goes in a more correct way. 2) Please examine the tmio-nand driver (was here on the list and on linux-mtd). It uses the mfd_cell to call hooks from the "host" driver (tc6393xb, more to be added soon). > > Index: linux-2.6.26-rc9-next20080709/include/linux/mfd/core.h > =================================================================== > --- linux-2.6.26-rc9-next20080709.orig/include/linux/mfd/core.h 2008-07-09 10:46:23.000000000 +0100 > +++ linux-2.6.26-rc9-next20080709/include/linux/mfd/core.h 2008-07-09 11:14:55.000000000 +0100 > @@ -18,8 +18,6 @@ > > /* > * This struct describes the MFD part ("cell"). > - * After registration the copy of this structure will become the platform data > - * of the resulting platform_device > */ > struct mfd_cell { > const char *name; > @@ -33,9 +31,21 @@ struct mfd_cell { > const struct resource *resources; > }; > > +struct mfd_device { > + struct platform_device pdev; > + struct mfd_cell *cell; > + struct resource resources[0]; > +}; > + > +static inline struct mfd_device *to_mfd_device(struct device *dev) > +{ > + struct platform_device *pdev = to_platform_device(dev); > + return container_of(pdev, struct mfd_device, pdev); > +} > + > static inline struct mfd_cell *mfd_get_cell(struct platform_device *pdev) > { > - return (struct mfd_cell *)pdev->dev.platform_data; > + return container_of(pdev, struct mfd_device, pdev)->cell; > } > > extern int mfd_add_devices(struct platform_device *parent, > Index: linux-2.6.26-rc9-next20080709/drivers/mfd/mfd-core.c > =================================================================== > --- linux-2.6.26-rc9-next20080709.orig/drivers/mfd/mfd-core.c 2008-07-09 10:59:54.000000000 +0100 > +++ linux-2.6.26-rc9-next20080709/drivers/mfd/mfd-core.c 2008-07-09 11:09:59.000000000 +0100 > @@ -15,28 +15,41 @@ > #include <linux/platform_device.h> > #include <linux/mfd/core.h> > > +static void mfd_dev_release(struct device *dev) > +{ > + struct mfd_device *mdev = to_mfd_device(dev); > + > + kfree(mdev); > +} > + > static int mfd_add_device(struct platform_device *parent, > const struct mfd_cell *cell, > struct resource *mem_base, > int irq_base) > { > - struct resource res[cell->num_resources]; > + struct resource *res; > + struct mfd_device *mdev; > struct platform_device *pdev; > int ret = -ENOMEM; > int r; > > - pdev = platform_device_alloc(cell->name, parent->id); > - if (!pdev) > + mdev = kzalloc(sizeof(struct mfd_device) + > + sizeof(struct resource) * cell->num_resources, > + GFP_KERNEL); > + if (!mdev) > goto fail_alloc; > > - pdev->dev.parent = &parent->dev; > + mdev->cell = cell; > + mdev->pdev.dev.parent = &parent->dev; > > - ret = platform_device_add_data(pdev, > - cell, sizeof(struct mfd_cell)); > - if (ret) > - goto fail_device; > + pdev = &mdev->pdev; > + res = &mdev->resources; > + > + device_initialise(&pdev->dev); > + pdev->id = parent->id; > + pdev->name = cell->name; > + pdev->release = mfd_device_release; > > - memzero(res, sizeof(res)); > for (r = 0; r < cell->num_resources; r++) { > res[r].name = cell->resources[r].name; > res[r].flags = cell->resources[r].flags; > > -- > -- With best wishes Dmitry ^ permalink raw reply [flat|nested] 39+ messages in thread
* Re: [patch 4/4] MFD: Change mfd platform device usage to wrapper platform_device 2008-07-09 11:15 ` Dmitry @ 2008-07-09 11:24 ` Ben Dooks 2008-07-09 11:31 ` Dmitry 2008-07-09 11:45 ` ian 1 sibling, 1 reply; 39+ messages in thread From: Ben Dooks @ 2008-07-09 11:24 UTC (permalink / raw) To: Dmitry; +Cc: Ben Dooks, linux-kernel, linux-arm-kernel, sameo On Wed, Jul 09, 2008 at 03:15:47PM +0400, Dmitry wrote: > 2008/7/9 Ben Dooks <ben-linux@fluff.org>: > > This patch changes the mfd core behaviour to wrapper the platform_device > > it creates in an struct mfd_device which contains the information > > about the cell that was created. > > > > 1) The creation of the resource list and then passing it to the > > platform_device_add_resources() causes the allocation of a > > large array on the stack as well as copying the source data > > twice (it is copied from the mfd_cell to the temporary array > > and then copied into the newly allocated array) > > > > 2) We can wrapper the platform_device into an mfd_device and use > > that to do the platform_device and resource allocation in one > > go to reduce the failiure. > > > > Note, is there actually any reason to pass the sub devices any > > information about the cell they are created from? The mfd core > > already makes the appropriate resource adjustments and anything > > else like clocks should be exported by the clock drivers? > > > > Signed-off-by: Ben Dooks <ben-linux@fluff.org> > > > NAK. > 0) It was discussed yesterday on the list and the decision was to go > in a different way. > I've provided a bit cleaner patch with the same idea, but then we > decided to go in a bit different way. > 1) I prefer patch by Mike Rapoport which is more clear and goes in a > more correct way. How "more correct", whilst the patch by Mike makes the platform data be passed from the cell, there is no longer any way to get from the platform device to the mfd_cell... The current driver is being inefficent in the way it creates resources on the stack and then calls a routine that does an kalloc/memcpy on the resources. > 2) Please examine the tmio-nand driver (was here on the list and on > linux-mtd). It uses the mfd_cell > to call hooks from the "host" driver (tc6393xb, more to be added soon). The one posted in [1] does not call these hooks at-all, can ou please explain why these hooks are needed in addition to the ones already available in the platform device driver? [1] http://lists.infradead.org/pipermail/linux-mtd/2008-June/022137.html > > > > Index: linux-2.6.26-rc9-next20080709/include/linux/mfd/core.h > > =================================================================== > > --- linux-2.6.26-rc9-next20080709.orig/include/linux/mfd/core.h 2008-07-09 10:46:23.000000000 +0100 > > +++ linux-2.6.26-rc9-next20080709/include/linux/mfd/core.h 2008-07-09 11:14:55.000000000 +0100 > > @@ -18,8 +18,6 @@ > > > > /* > > * This struct describes the MFD part ("cell"). > > - * After registration the copy of this structure will become the platform data > > - * of the resulting platform_device > > */ > > struct mfd_cell { > > const char *name; > > @@ -33,9 +31,21 @@ struct mfd_cell { > > const struct resource *resources; > > }; > > > > +struct mfd_device { > > + struct platform_device pdev; > > + struct mfd_cell *cell; > > + struct resource resources[0]; > > +}; > > + > > +static inline struct mfd_device *to_mfd_device(struct device *dev) > > +{ > > + struct platform_device *pdev = to_platform_device(dev); > > + return container_of(pdev, struct mfd_device, pdev); > > +} > > + > > static inline struct mfd_cell *mfd_get_cell(struct platform_device *pdev) > > { > > - return (struct mfd_cell *)pdev->dev.platform_data; > > + return container_of(pdev, struct mfd_device, pdev)->cell; > > } > > > > extern int mfd_add_devices(struct platform_device *parent, > > Index: linux-2.6.26-rc9-next20080709/drivers/mfd/mfd-core.c > > =================================================================== > > --- linux-2.6.26-rc9-next20080709.orig/drivers/mfd/mfd-core.c 2008-07-09 10:59:54.000000000 +0100 > > +++ linux-2.6.26-rc9-next20080709/drivers/mfd/mfd-core.c 2008-07-09 11:09:59.000000000 +0100 > > @@ -15,28 +15,41 @@ > > #include <linux/platform_device.h> > > #include <linux/mfd/core.h> > > > > +static void mfd_dev_release(struct device *dev) > > +{ > > + struct mfd_device *mdev = to_mfd_device(dev); > > + > > + kfree(mdev); > > +} > > + > > static int mfd_add_device(struct platform_device *parent, > > const struct mfd_cell *cell, > > struct resource *mem_base, > > int irq_base) > > { > > - struct resource res[cell->num_resources]; > > + struct resource *res; > > + struct mfd_device *mdev; > > struct platform_device *pdev; > > int ret = -ENOMEM; > > int r; > > > > - pdev = platform_device_alloc(cell->name, parent->id); > > - if (!pdev) > > + mdev = kzalloc(sizeof(struct mfd_device) + > > + sizeof(struct resource) * cell->num_resources, > > + GFP_KERNEL); > > + if (!mdev) > > goto fail_alloc; > > > > - pdev->dev.parent = &parent->dev; > > + mdev->cell = cell; > > + mdev->pdev.dev.parent = &parent->dev; > > > > - ret = platform_device_add_data(pdev, > > - cell, sizeof(struct mfd_cell)); > > - if (ret) > > - goto fail_device; > > + pdev = &mdev->pdev; > > + res = &mdev->resources; > > + > > + device_initialise(&pdev->dev); > > + pdev->id = parent->id; > > + pdev->name = cell->name; > > + pdev->release = mfd_device_release; > > > > - memzero(res, sizeof(res)); > > for (r = 0; r < cell->num_resources; r++) { > > res[r].name = cell->resources[r].name; > > res[r].flags = cell->resources[r].flags; > > > > -- > > > > > > -- > With best wishes > Dmitry -- -- Ben Q: What's a light-year? A: One-third less calories than a regular year. ^ permalink raw reply [flat|nested] 39+ messages in thread
* Re: [patch 4/4] MFD: Change mfd platform device usage to wrapper platform_device 2008-07-09 11:24 ` Ben Dooks @ 2008-07-09 11:31 ` Dmitry 2008-07-09 11:50 ` Ben Dooks 0 siblings, 1 reply; 39+ messages in thread From: Dmitry @ 2008-07-09 11:31 UTC (permalink / raw) To: Ben Dooks; +Cc: linux-kernel, linux-arm-kernel, sameo 2008/7/9 Ben Dooks <ben-linux@fluff.org>: > On Wed, Jul 09, 2008 at 03:15:47PM +0400, Dmitry wrote: >> 2008/7/9 Ben Dooks <ben-linux@fluff.org>: >> > This patch changes the mfd core behaviour to wrapper the platform_device >> > it creates in an struct mfd_device which contains the information >> > about the cell that was created. >> > >> > 1) The creation of the resource list and then passing it to the >> > platform_device_add_resources() causes the allocation of a >> > large array on the stack as well as copying the source data >> > twice (it is copied from the mfd_cell to the temporary array >> > and then copied into the newly allocated array) >> > >> > 2) We can wrapper the platform_device into an mfd_device and use >> > that to do the platform_device and resource allocation in one >> > go to reduce the failiure. >> > >> > Note, is there actually any reason to pass the sub devices any >> > information about the cell they are created from? The mfd core >> > already makes the appropriate resource adjustments and anything >> > else like clocks should be exported by the clock drivers? >> > >> > Signed-off-by: Ben Dooks <ben-linux@fluff.org> >> >> >> NAK. >> 0) It was discussed yesterday on the list and the decision was to go >> in a different way. >> I've provided a bit cleaner patch with the same idea, but then we >> decided to go in a bit different way. >> 1) I prefer patch by Mike Rapoport which is more clear and goes in a >> more correct way. > > How "more correct", whilst the patch by Mike makes the platform data > be passed from the cell, there is no longer any way to get from the > platform device to the mfd_cell... Basically we have two choises for the subdevice driver: 1) it doesn't know about cells at all (e.g. generic-bl, IIRC). Then we are safe to loose that "cell" information 2) If it does use cell information (to get access to hooks), we pass it via platform_data pointer in the mfd_cell and we are ok with it. > The current driver is being inefficent in the way it creates resources > on the stack and then calls a routine that does an kalloc/memcpy on > the resources. I don't see any inefficiency ATM. >> 2) Please examine the tmio-nand driver (was here on the list and on >> linux-mtd). It uses the mfd_cell >> to call hooks from the "host" driver (tc6393xb, more to be added soon). > > The one posted in [1] does not call these hooks at-all, can ou please > explain why these hooks are needed in addition to the ones already > available in the platform device driver? > > [1] http://lists.infradead.org/pipermail/linux-mtd/2008-June/022137.html + +static int tmio_hw_init(struct platform_device *dev, struct tmio_nand *tmio) +{ + struct mfd_cell *cell = mfd_get_cell(dev); + const struct resource *nfcr = NULL; + unsigned long base; + int i; + + for (i = 0; i < cell->num_resources; i++) + if (!strcmp((cell->resources+i)->name, TMIO_NAND_CONTROL)) + nfcr = &cell->resources[i]; + + if (nfcr == NULL) + return -ENOMEM; + + if (cell->enable) { + int rc = cell->enable(dev); + if (rc) + return rc; + } That cell->enable() is necessary to set up the host (in the tc6393xb case to enable buffers) to enable access to the nand. -- With best wishes Dmitry ^ permalink raw reply [flat|nested] 39+ messages in thread
* Re: [patch 4/4] MFD: Change mfd platform device usage to wrapper platform_device 2008-07-09 11:31 ` Dmitry @ 2008-07-09 11:50 ` Ben Dooks 2008-07-09 11:56 ` Dmitry 2008-07-09 12:13 ` ian 0 siblings, 2 replies; 39+ messages in thread From: Ben Dooks @ 2008-07-09 11:50 UTC (permalink / raw) To: Dmitry; +Cc: Ben Dooks, linux-kernel, linux-arm-kernel On Wed, Jul 09, 2008 at 03:31:04PM +0400, Dmitry wrote: > 2008/7/9 Ben Dooks <ben-linux@fluff.org>: > > On Wed, Jul 09, 2008 at 03:15:47PM +0400, Dmitry wrote: > >> 2008/7/9 Ben Dooks <ben-linux@fluff.org>: > >> > This patch changes the mfd core behaviour to wrapper the platform_device > >> > it creates in an struct mfd_device which contains the information > >> > about the cell that was created. > >> > > >> > 1) The creation of the resource list and then passing it to the > >> > platform_device_add_resources() causes the allocation of a > >> > large array on the stack as well as copying the source data > >> > twice (it is copied from the mfd_cell to the temporary array > >> > and then copied into the newly allocated array) > >> > > >> > 2) We can wrapper the platform_device into an mfd_device and use > >> > that to do the platform_device and resource allocation in one > >> > go to reduce the failiure. > >> > > >> > Note, is there actually any reason to pass the sub devices any > >> > information about the cell they are created from? The mfd core > >> > already makes the appropriate resource adjustments and anything > >> > else like clocks should be exported by the clock drivers? > >> > > >> > Signed-off-by: Ben Dooks <ben-linux@fluff.org> > >> > >> > >> NAK. > >> 0) It was discussed yesterday on the list and the decision was to go > >> in a different way. > >> I've provided a bit cleaner patch with the same idea, but then we > >> decided to go in a bit different way. > >> 1) I prefer patch by Mike Rapoport which is more clear and goes in a > >> more correct way. > > > > How "more correct", whilst the patch by Mike makes the platform data > > be passed from the cell, there is no longer any way to get from the > > platform device to the mfd_cell... > > Basically we have two choises for the subdevice driver: > 1) it doesn't know about cells at all (e.g. generic-bl, IIRC). Then we are safe > to loose that "cell" information > 2) If it does use cell information (to get access to hooks), we pass it > via platform_data pointer in the mfd_cell and we are ok with it. Erm, that is complete non-answer. The driver model and various other parts of the kernel are littered with examples of embedding one structure within another to gain an C++ like object inheritance. I've supplied an reasonable example of doing this to create an mfd_cell device from an platform_device without creating an large amount of code and improving the efficiency and code-lineage in the process. I do not see how this isn't "correct" or in any way breaing the current linux model of doing things. > > > The current driver is being inefficent in the way it creates resources > > on the stack and then calls a routine that does an kalloc/memcpy on > > the resources. > > I don't see any inefficiency ATM. > > >> 2) Please examine the tmio-nand driver (was here on the list and on > >> linux-mtd). It uses the mfd_cell > >> to call hooks from the "host" driver (tc6393xb, more to be added soon). > > > > The one posted in [1] does not call these hooks at-all, can ou please > > explain why these hooks are needed in addition to the ones already > > available in the platform device driver? > > > > [1] http://lists.infradead.org/pipermail/linux-mtd/2008-June/022137.html > > + > +static int tmio_hw_init(struct platform_device *dev, struct tmio_nand *tmio) > +{ > + struct mfd_cell *cell = mfd_get_cell(dev); > + const struct resource *nfcr = NULL; > + unsigned long base; > + int i; > + > + for (i = 0; i < cell->num_resources; i++) > + if (!strcmp((cell->resources+i)->name, TMIO_NAND_CONTROL)) > + nfcr = &cell->resources[i]; > + > + if (nfcr == NULL) > + return -ENOMEM; > + > + if (cell->enable) { > + int rc = cell->enable(dev); > + if (rc) > + return rc; > + } > > That cell->enable() is necessary to set up the host (in the tc6393xb > case to enable buffers) > to enable access to the nand. So, the enable/disable calls might be useful, however is there any reason this could not be handled by the clock framework? The suspend/resume entries where not used, and I belive should not be in here. As noted before, mfd_get_cell() got dropped by [2] [2] http://lists.arm.linux.org.uk/lurker/message/20080708.153450.bb33046d.en.html -- Ben Q: What's a light-year? A: One-third less calories than a regular year. ^ permalink raw reply [flat|nested] 39+ messages in thread
* Re: [patch 4/4] MFD: Change mfd platform device usage to wrapper platform_device 2008-07-09 11:50 ` Ben Dooks @ 2008-07-09 11:56 ` Dmitry 2008-07-09 12:07 ` Ben Dooks 2008-07-11 21:37 ` Samuel Ortiz 2008-07-09 12:13 ` ian 1 sibling, 2 replies; 39+ messages in thread From: Dmitry @ 2008-07-09 11:56 UTC (permalink / raw) To: Ben Dooks; +Cc: linux-kernel, linux-arm-kernel 2008/7/9 Ben Dooks <ben-linux@fluff.org>: > On Wed, Jul 09, 2008 at 03:31:04PM +0400, Dmitry wrote: >> 2008/7/9 Ben Dooks <ben-linux@fluff.org>: >> > On Wed, Jul 09, 2008 at 03:15:47PM +0400, Dmitry wrote: >> >> 2008/7/9 Ben Dooks <ben-linux@fluff.org>: >> >> > This patch changes the mfd core behaviour to wrapper the platform_device >> >> > it creates in an struct mfd_device which contains the information >> >> > about the cell that was created. >> >> > >> >> > 1) The creation of the resource list and then passing it to the >> >> > platform_device_add_resources() causes the allocation of a >> >> > large array on the stack as well as copying the source data >> >> > twice (it is copied from the mfd_cell to the temporary array >> >> > and then copied into the newly allocated array) >> >> > >> >> > 2) We can wrapper the platform_device into an mfd_device and use >> >> > that to do the platform_device and resource allocation in one >> >> > go to reduce the failiure. >> >> > >> >> > Note, is there actually any reason to pass the sub devices any >> >> > information about the cell they are created from? The mfd core >> >> > already makes the appropriate resource adjustments and anything >> >> > else like clocks should be exported by the clock drivers? >> >> > >> >> > Signed-off-by: Ben Dooks <ben-linux@fluff.org> >> >> >> >> >> >> NAK. >> >> 0) It was discussed yesterday on the list and the decision was to go >> >> in a different way. >> >> I've provided a bit cleaner patch with the same idea, but then we >> >> decided to go in a bit different way. >> >> 1) I prefer patch by Mike Rapoport which is more clear and goes in a >> >> more correct way. >> > >> > How "more correct", whilst the patch by Mike makes the platform data >> > be passed from the cell, there is no longer any way to get from the >> > platform device to the mfd_cell... >> >> Basically we have two choises for the subdevice driver: >> 1) it doesn't know about cells at all (e.g. generic-bl, IIRC). Then we are safe >> to loose that "cell" information >> 2) If it does use cell information (to get access to hooks), we pass it >> via platform_data pointer in the mfd_cell and we are ok with it. > > Erm, that is complete non-answer. The driver model and various other > parts of the kernel are littered with examples of embedding one > structure within another to gain an C++ like object inheritance. > > I've supplied an reasonable example of doing this to create an mfd_cell > device from an platform_device without creating an large amount of code > and improving the efficiency and code-lineage in the process. I do not > see how this isn't "correct" or in any way breaing the current linux > model of doing things. It isn't breaking it. OK. I'm leaving the decision to the MFD or ARM maintainers. And BTW, nearly the same patch was sent yesterday by me[1]. Is it an independant work, or did you miss my sign-off? [1]: http://permalink.gmane.org/gmane.linux.ports.arm.kernel/44142 > >> >> > The current driver is being inefficent in the way it creates resources >> > on the stack and then calls a routine that does an kalloc/memcpy on >> > the resources. >> >> I don't see any inefficiency ATM. >> >> >> 2) Please examine the tmio-nand driver (was here on the list and on >> >> linux-mtd). It uses the mfd_cell >> >> to call hooks from the "host" driver (tc6393xb, more to be added soon). >> > >> > The one posted in [1] does not call these hooks at-all, can ou please >> > explain why these hooks are needed in addition to the ones already >> > available in the platform device driver? >> > >> > [1] http://lists.infradead.org/pipermail/linux-mtd/2008-June/022137.html >> >> + >> +static int tmio_hw_init(struct platform_device *dev, struct tmio_nand *tmio) >> +{ >> + struct mfd_cell *cell = mfd_get_cell(dev); >> + const struct resource *nfcr = NULL; >> + unsigned long base; >> + int i; >> + >> + for (i = 0; i < cell->num_resources; i++) >> + if (!strcmp((cell->resources+i)->name, TMIO_NAND_CONTROL)) >> + nfcr = &cell->resources[i]; >> + >> + if (nfcr == NULL) >> + return -ENOMEM; >> + >> + if (cell->enable) { >> + int rc = cell->enable(dev); >> + if (rc) >> + return rc; >> + } >> >> That cell->enable() is necessary to set up the host (in the tc6393xb >> case to enable buffers) >> to enable access to the nand. > > So, the enable/disable calls might be useful, however is there any > reason this could not be handled by the clock framework? The suspend/resume > entries where not used, and I belive should not be in here. They should be here for exactly the same reason. They are used by the drivers that will be submitted later. E.g. OHCI driver needs such suspend/resume handling. > As noted before, mfd_get_cell() got dropped by [2] > > [2] http://lists.arm.linux.org.uk/lurker/message/20080708.153450.bb33046d.en.html Yes, and as I said before it will need some small modifications. -- With best wishes Dmitry ^ permalink raw reply [flat|nested] 39+ messages in thread
* Re: [patch 4/4] MFD: Change mfd platform device usage to wrapper platform_device 2008-07-09 11:56 ` Dmitry @ 2008-07-09 12:07 ` Ben Dooks 2008-07-09 12:31 ` Dmitry 2008-07-09 13:28 ` ian 2008-07-11 21:37 ` Samuel Ortiz 1 sibling, 2 replies; 39+ messages in thread From: Ben Dooks @ 2008-07-09 12:07 UTC (permalink / raw) To: Dmitry; +Cc: Ben Dooks, linux-kernel, linux-arm-kernel On Wed, Jul 09, 2008 at 03:56:54PM +0400, Dmitry wrote: > 2008/7/9 Ben Dooks <ben-linux@fluff.org>: > > On Wed, Jul 09, 2008 at 03:31:04PM +0400, Dmitry wrote: > >> 2008/7/9 Ben Dooks <ben-linux@fluff.org>: > >> > On Wed, Jul 09, 2008 at 03:15:47PM +0400, Dmitry wrote: > >> >> 2008/7/9 Ben Dooks <ben-linux@fluff.org>: > >> >> > This patch changes the mfd core behaviour to wrapper the platform_device > >> >> > it creates in an struct mfd_device which contains the information > >> >> > about the cell that was created. > >> >> > > >> >> > 1) The creation of the resource list and then passing it to the > >> >> > platform_device_add_resources() causes the allocation of a > >> >> > large array on the stack as well as copying the source data > >> >> > twice (it is copied from the mfd_cell to the temporary array > >> >> > and then copied into the newly allocated array) > >> >> > > >> >> > 2) We can wrapper the platform_device into an mfd_device and use > >> >> > that to do the platform_device and resource allocation in one > >> >> > go to reduce the failiure. > >> >> > > >> >> > Note, is there actually any reason to pass the sub devices any > >> >> > information about the cell they are created from? The mfd core > >> >> > already makes the appropriate resource adjustments and anything > >> >> > else like clocks should be exported by the clock drivers? > >> >> > > >> >> > Signed-off-by: Ben Dooks <ben-linux@fluff.org> > >> >> > >> >> > >> >> NAK. > >> >> 0) It was discussed yesterday on the list and the decision was to go > >> >> in a different way. > >> >> I've provided a bit cleaner patch with the same idea, but then we > >> >> decided to go in a bit different way. > >> >> 1) I prefer patch by Mike Rapoport which is more clear and goes in a > >> >> more correct way. > >> > > >> > How "more correct", whilst the patch by Mike makes the platform data > >> > be passed from the cell, there is no longer any way to get from the > >> > platform device to the mfd_cell... > >> > >> Basically we have two choises for the subdevice driver: > >> 1) it doesn't know about cells at all (e.g. generic-bl, IIRC). Then we are safe > >> to loose that "cell" information > >> 2) If it does use cell information (to get access to hooks), we pass it > >> via platform_data pointer in the mfd_cell and we are ok with it. > > > > Erm, that is complete non-answer. The driver model and various other > > parts of the kernel are littered with examples of embedding one > > structure within another to gain an C++ like object inheritance. > > > > I've supplied an reasonable example of doing this to create an mfd_cell > > device from an platform_device without creating an large amount of code > > and improving the efficiency and code-lineage in the process. I do not > > see how this isn't "correct" or in any way breaing the current linux > > model of doing things. > > It isn't breaking it. OK. I'm leaving the decision to the MFD or ARM > maintainers. > And BTW, nearly the same patch was sent yesterday by me[1]. Is it an independant > work, or did you miss my sign-off? > > [1]: http://permalink.gmane.org/gmane.linux.ports.arm.kernel/44142 Hmm, thanks, completely missed because it has a completely un-related looking title. > > > >> > >> > The current driver is being inefficent in the way it creates resources > >> > on the stack and then calls a routine that does an kalloc/memcpy on > >> > the resources. > >> > >> I don't see any inefficiency ATM. > >> > >> >> 2) Please examine the tmio-nand driver (was here on the list and on > >> >> linux-mtd). It uses the mfd_cell > >> >> to call hooks from the "host" driver (tc6393xb, more to be added soon). > >> > > >> > The one posted in [1] does not call these hooks at-all, can ou please > >> > explain why these hooks are needed in addition to the ones already > >> > available in the platform device driver? > >> > > >> > [1] http://lists.infradead.org/pipermail/linux-mtd/2008-June/022137.html > >> > >> + > >> +static int tmio_hw_init(struct platform_device *dev, struct tmio_nand *tmio) > >> +{ > >> + struct mfd_cell *cell = mfd_get_cell(dev); > >> + const struct resource *nfcr = NULL; > >> + unsigned long base; > >> + int i; > >> + > >> + for (i = 0; i < cell->num_resources; i++) > >> + if (!strcmp((cell->resources+i)->name, TMIO_NAND_CONTROL)) > >> + nfcr = &cell->resources[i]; > >> + > >> + if (nfcr == NULL) > >> + return -ENOMEM; > >> + > >> + if (cell->enable) { > >> + int rc = cell->enable(dev); > >> + if (rc) > >> + return rc; > >> + } > >> > >> That cell->enable() is necessary to set up the host (in the tc6393xb > >> case to enable buffers) > >> to enable access to the nand. > > > > So, the enable/disable calls might be useful, however is there any > > reason this could not be handled by the clock framework? The suspend/resume > > entries where not used, and I belive should not be in here. > > They should be here for exactly the same reason. They are used by the drivers > that will be submitted later. E.g. OHCI driver needs such > suspend/resume handling. No, you don't understand. I'll make a rather explicit point about the very clever way the device tree works since the devices are registered with their parent device set. In the suspend, all sub devices are suspended via their platform_driver.suspend method before the parent device's suspend method is called. When resuming, the parent is resumed before calling the children's platform_driver.resume methods. > > As noted before, mfd_get_cell() got dropped by [2] > > > > [2] http://lists.arm.linux.org.uk/lurker/message/20080708.153450.bb33046d.en.html > > Yes, and as I said before it will need some small modifications. > > -- > With best wishes > Dmitry > > ------------------------------------------------------------------- > List admin: http://lists.arm.linux.org.uk/mailman/listinfo/linux-arm-kernel > FAQ: http://www.arm.linux.org.uk/mailinglists/faq.php > Etiquette: http://www.arm.linux.org.uk/mailinglists/etiquette.php -- Ben Q: What's a light-year? A: One-third less calories than a regular year. ^ permalink raw reply [flat|nested] 39+ messages in thread
* Re: [patch 4/4] MFD: Change mfd platform device usage to wrapper platform_device 2008-07-09 12:07 ` Ben Dooks @ 2008-07-09 12:31 ` Dmitry 2008-07-09 13:28 ` ian 1 sibling, 0 replies; 39+ messages in thread From: Dmitry @ 2008-07-09 12:31 UTC (permalink / raw) To: Ben Dooks; +Cc: linux-kernel, linux-arm-kernel 2008/7/9 Ben Dooks <ben-linux@fluff.org>: > On Wed, Jul 09, 2008 at 03:56:54PM +0400, Dmitry wrote: >> 2008/7/9 Ben Dooks <ben-linux@fluff.org>: >> > On Wed, Jul 09, 2008 at 03:31:04PM +0400, Dmitry wrote: >> >> 2008/7/9 Ben Dooks <ben-linux@fluff.org>: >> >> > On Wed, Jul 09, 2008 at 03:15:47PM +0400, Dmitry wrote: >> >> >> 2008/7/9 Ben Dooks <ben-linux@fluff.org>: >> >> >> > This patch changes the mfd core behaviour to wrapper the platform_device >> >> >> > it creates in an struct mfd_device which contains the information >> >> >> > about the cell that was created. >> >> >> > >> >> >> > 1) The creation of the resource list and then passing it to the >> >> >> > platform_device_add_resources() causes the allocation of a >> >> >> > large array on the stack as well as copying the source data >> >> >> > twice (it is copied from the mfd_cell to the temporary array >> >> >> > and then copied into the newly allocated array) >> >> >> > >> >> >> > 2) We can wrapper the platform_device into an mfd_device and use >> >> >> > that to do the platform_device and resource allocation in one >> >> >> > go to reduce the failiure. >> >> >> > >> >> >> > Note, is there actually any reason to pass the sub devices any >> >> >> > information about the cell they are created from? The mfd core >> >> >> > already makes the appropriate resource adjustments and anything >> >> >> > else like clocks should be exported by the clock drivers? >> >> >> > >> >> >> > Signed-off-by: Ben Dooks <ben-linux@fluff.org> >> >> >> >> >> >> >> >> >> NAK. >> >> >> 0) It was discussed yesterday on the list and the decision was to go >> >> >> in a different way. >> >> >> I've provided a bit cleaner patch with the same idea, but then we >> >> >> decided to go in a bit different way. >> >> >> 1) I prefer patch by Mike Rapoport which is more clear and goes in a >> >> >> more correct way. >> >> > >> >> > How "more correct", whilst the patch by Mike makes the platform data >> >> > be passed from the cell, there is no longer any way to get from the >> >> > platform device to the mfd_cell... >> >> >> >> Basically we have two choises for the subdevice driver: >> >> 1) it doesn't know about cells at all (e.g. generic-bl, IIRC). Then we are safe >> >> to loose that "cell" information >> >> 2) If it does use cell information (to get access to hooks), we pass it >> >> via platform_data pointer in the mfd_cell and we are ok with it. >> > >> > Erm, that is complete non-answer. The driver model and various other >> > parts of the kernel are littered with examples of embedding one >> > structure within another to gain an C++ like object inheritance. >> > >> > I've supplied an reasonable example of doing this to create an mfd_cell >> > device from an platform_device without creating an large amount of code >> > and improving the efficiency and code-lineage in the process. I do not >> > see how this isn't "correct" or in any way breaing the current linux >> > model of doing things. >> >> It isn't breaking it. OK. I'm leaving the decision to the MFD or ARM >> maintainers. >> And BTW, nearly the same patch was sent yesterday by me[1]. Is it an independant >> work, or did you miss my sign-off? >> >> [1]: http://permalink.gmane.org/gmane.linux.ports.arm.kernel/44142 > > Hmm, thanks, completely missed because it has a completely un-related > looking title. > >> > >> >> >> >> > The current driver is being inefficent in the way it creates resources >> >> > on the stack and then calls a routine that does an kalloc/memcpy on >> >> > the resources. >> >> >> >> I don't see any inefficiency ATM. >> >> >> >> >> 2) Please examine the tmio-nand driver (was here on the list and on >> >> >> linux-mtd). It uses the mfd_cell >> >> >> to call hooks from the "host" driver (tc6393xb, more to be added soon). >> >> > >> >> > The one posted in [1] does not call these hooks at-all, can ou please >> >> > explain why these hooks are needed in addition to the ones already >> >> > available in the platform device driver? >> >> > >> >> > [1] http://lists.infradead.org/pipermail/linux-mtd/2008-June/022137.html >> >> >> >> + >> >> +static int tmio_hw_init(struct platform_device *dev, struct tmio_nand *tmio) >> >> +{ >> >> + struct mfd_cell *cell = mfd_get_cell(dev); >> >> + const struct resource *nfcr = NULL; >> >> + unsigned long base; >> >> + int i; >> >> + >> >> + for (i = 0; i < cell->num_resources; i++) >> >> + if (!strcmp((cell->resources+i)->name, TMIO_NAND_CONTROL)) >> >> + nfcr = &cell->resources[i]; >> >> + >> >> + if (nfcr == NULL) >> >> + return -ENOMEM; >> >> + >> >> + if (cell->enable) { >> >> + int rc = cell->enable(dev); >> >> + if (rc) >> >> + return rc; >> >> + } >> >> >> >> That cell->enable() is necessary to set up the host (in the tc6393xb >> >> case to enable buffers) >> >> to enable access to the nand. >> > >> > So, the enable/disable calls might be useful, however is there any >> > reason this could not be handled by the clock framework? The suspend/resume >> > entries where not used, and I belive should not be in here. >> >> They should be here for exactly the same reason. They are used by the drivers >> that will be submitted later. E.g. OHCI driver needs such >> suspend/resume handling. > > No, you don't understand. I'll make a rather explicit point about the > very clever way the device tree works since the devices are registered > with their parent device set. > > In the suspend, all sub devices are suspended via their > platform_driver.suspend method before the parent device's suspend method > is called. When resuming, the parent is resumed before calling the > children's platform_driver.resume methods. Suspending of sub-devices is handled in two places: 1) suspend the state of subdevice (e.g. ohci stores some info) and then 2) sub-device host suspends/disables the cell (e.g. clocks, power, etc). These steps depend completely on the MFD-device, not only on the sub-device. These two-stage power management is represented by the suspend/resume hooks in the mfd_cell. However, I think we will be able to drop the suspend/resume when/if generic clocks and voltage regulators frameworks get merged. >> > As noted before, mfd_get_cell() got dropped by [2] >> > >> > [2] http://lists.arm.linux.org.uk/lurker/message/20080708.153450.bb33046d.en.html >> >> Yes, and as I said before it will need some small modifications. >> >> -- >> With best wishes >> Dmitry >> >> ------------------------------------------------------------------- >> List admin: http://lists.arm.linux.org.uk/mailman/listinfo/linux-arm-kernel >> FAQ: http://www.arm.linux.org.uk/mailinglists/faq.php >> Etiquette: http://www.arm.linux.org.uk/mailinglists/etiquette.php > > -- > Ben > > Q: What's a light-year? > A: One-third less calories than a regular year. > > -- With best wishes Dmitry ^ permalink raw reply [flat|nested] 39+ messages in thread
* Re: [patch 4/4] MFD: Change mfd platform device usage to wrapper platform_device 2008-07-09 12:07 ` Ben Dooks 2008-07-09 12:31 ` Dmitry @ 2008-07-09 13:28 ` ian 2008-07-09 13:34 ` pHilipp Zabel 1 sibling, 1 reply; 39+ messages in thread From: ian @ 2008-07-09 13:28 UTC (permalink / raw) To: Ben Dooks; +Cc: Dmitry, linux-kernel, linux-arm-kernel On Wed, 2008-07-09 at 13:07 +0100, Ben Dooks wrote: > > They should be here for exactly the same reason. They are used by > the drivers > > that will be submitted later. E.g. OHCI driver needs such > > suspend/resume handling. > > No, you don't understand. I'll make a rather explicit point about the > very clever way the device tree works since the devices are registered > with their parent device set. Actually I misthought here too. The problem comes when the subdevices arent *quite* truely independant of their core, and thus need to ask the core to turn off power / clokcs / etc. for them they cant just do it themselves because the subdevices may be used on more than one core that does this hanling in different ways (eg. T7L and TC6393XB handle the 32KHz clock completely differently. on tc6393xb, theres a clock gate on the MFD core chip and on t7l66xb the clock has to be handled right back at the platform layer, in board specific code because the core has no gate, and the clock is fed to it from an external pin. Thats one example - there are others, not all clocks. ^ permalink raw reply [flat|nested] 39+ messages in thread
* Re: [patch 4/4] MFD: Change mfd platform device usage to wrapper platform_device 2008-07-09 13:28 ` ian @ 2008-07-09 13:34 ` pHilipp Zabel 2008-07-09 13:37 ` ian 0 siblings, 1 reply; 39+ messages in thread From: pHilipp Zabel @ 2008-07-09 13:34 UTC (permalink / raw) To: ian; +Cc: Ben Dooks, Dmitry, linux-kernel, linux-arm-kernel On Wed, Jul 9, 2008 at 3:28 PM, ian <spyro@f2s.com> wrote: > On Wed, 2008-07-09 at 13:07 +0100, Ben Dooks wrote: >> > They should be here for exactly the same reason. They are used by >> the drivers >> > that will be submitted later. E.g. OHCI driver needs such >> > suspend/resume handling. >> >> No, you don't understand. I'll make a rather explicit point about the >> very clever way the device tree works since the devices are registered >> with their parent device set. > > Actually I misthought here too. > > The problem comes when the subdevices arent *quite* truely independant > of their core, and thus need to ask the core to turn off power / > clokcs / etc. for them I agree "power" and "etc." are issues. Clocks should be handled by the clock API just fine. > they cant just do it themselves because the subdevices may be used on > more than one core that does this hanling in different ways (eg. T7L and > TC6393XB handle the 32KHz clock completely differently. That shouln't matter with generic clocks. If they clk_get(&mfd_cell->pdev.dev, "my_clk_input"), that should be dispatched to the correct MFD clock regardless of the actual chip. > on tc6393xb, > theres a clock gate on the MFD core chip and on t7l66xb the clock has to > be handled right back at the platform layer, in board specific code > because the core has no gate, and the clock is fed to it from an > external pin. Still, that could be done inside the MFD driver's custom clock enable/disable methods. > Thats one example - there are others, not all clocks. > > -- > To unsubscribe from this list: send the line "unsubscribe linux-kernel" in > the body of a message to majordomo@vger.kernel.org > More majordomo info at http://vger.kernel.org/majordomo-info.html > Please read the FAQ at http://www.tux.org/lkml/ > ^ permalink raw reply [flat|nested] 39+ messages in thread
* Re: [patch 4/4] MFD: Change mfd platform device usage to wrapper platform_device 2008-07-09 13:34 ` pHilipp Zabel @ 2008-07-09 13:37 ` ian 0 siblings, 0 replies; 39+ messages in thread From: ian @ 2008-07-09 13:37 UTC (permalink / raw) To: pHilipp Zabel; +Cc: Ben Dooks, Dmitry, linux-kernel, linux-arm-kernel On Wed, 2008-07-09 at 15:34 +0200, pHilipp Zabel wrote: > On Wed, Jul 9, 2008 at 3:28 PM, ian <spyro@f2s.com> wrote: > > On Wed, 2008-07-09 at 13:07 +0100, Ben Dooks wrote: > I agree "power" and "etc." are issues. Clocks should be handled by the > clock API just fine. When its merged. > > they cant just do it themselves because the subdevices may be used on > > more than one core that does this hanling in different ways (eg. T7L and > > TC6393XB handle the 32KHz clock completely differently. > > That shouln't matter with generic clocks. If they > clk_get(&mfd_cell->pdev.dev, "my_clk_input"), that should be > dispatched to the correct MFD clock regardless of the actual chip. I agree but its not merged yet. Until it is, we should stick with the enable / disable methods. Also, suspend / resume for TC* need careful sequencing of the GPIOs attached to #PCLR and #SUSPEND (get it wrong and you reset the chip instead of sleep it) :-) ^ permalink raw reply [flat|nested] 39+ messages in thread
* Re: [patch 4/4] MFD: Change mfd platform device usage to wrapper platform_device 2008-07-09 11:56 ` Dmitry 2008-07-09 12:07 ` Ben Dooks @ 2008-07-11 21:37 ` Samuel Ortiz 1 sibling, 0 replies; 39+ messages in thread From: Samuel Ortiz @ 2008-07-11 21:37 UTC (permalink / raw) To: Dmitry; +Cc: Ben Dooks, linux-kernel, linux-arm-kernel On Wed, Jul 09, 2008 at 03:56:54PM +0400, Dmitry wrote: > >> Basically we have two choises for the subdevice driver: > >> 1) it doesn't know about cells at all (e.g. generic-bl, IIRC). Then we are safe > >> to loose that "cell" information > >> 2) If it does use cell information (to get access to hooks), we pass it > >> via platform_data pointer in the mfd_cell and we are ok with it. > > > > Erm, that is complete non-answer. The driver model and various other > > parts of the kernel are littered with examples of embedding one > > structure within another to gain an C++ like object inheritance. > > > > I've supplied an reasonable example of doing this to create an mfd_cell > > device from an platform_device without creating an large amount of code > > and improving the efficiency and code-lineage in the process. I do not > > see how this isn't "correct" or in any way breaing the current linux > > model of doing things. > > It isn't breaking it. OK. I'm leaving the decision to the MFD or ARM > maintainers. > And BTW, nearly the same patch was sent yesterday by me[1]. Is it an independant > work, or did you miss my sign-off? > > [1]: http://permalink.gmane.org/gmane.linux.ports.arm.kernel/44142 As Russel pointed out, both patches are broken as long as we support a modular mfd-core (and there's no reason why we shouldnt). So, so far, Mike's patch is the best candidate, even though the idea of wrapping the platform device into an mfd_device sounded neater at first. And again, this is definitely post merge window material. Cheers, Samuel. > > > >> > >> > The current driver is being inefficent in the way it creates resources > >> > on the stack and then calls a routine that does an kalloc/memcpy on > >> > the resources. > >> > >> I don't see any inefficiency ATM. > >> > >> >> 2) Please examine the tmio-nand driver (was here on the list and on > >> >> linux-mtd). It uses the mfd_cell > >> >> to call hooks from the "host" driver (tc6393xb, more to be added soon). > >> > > >> > The one posted in [1] does not call these hooks at-all, can ou please > >> > explain why these hooks are needed in addition to the ones already > >> > available in the platform device driver? > >> > > >> > [1] http://lists.infradead.org/pipermail/linux-mtd/2008-June/022137.html > >> > >> + > >> +static int tmio_hw_init(struct platform_device *dev, struct tmio_nand *tmio) > >> +{ > >> + struct mfd_cell *cell = mfd_get_cell(dev); > >> + const struct resource *nfcr = NULL; > >> + unsigned long base; > >> + int i; > >> + > >> + for (i = 0; i < cell->num_resources; i++) > >> + if (!strcmp((cell->resources+i)->name, TMIO_NAND_CONTROL)) > >> + nfcr = &cell->resources[i]; > >> + > >> + if (nfcr == NULL) > >> + return -ENOMEM; > >> + > >> + if (cell->enable) { > >> + int rc = cell->enable(dev); > >> + if (rc) > >> + return rc; > >> + } > >> > >> That cell->enable() is necessary to set up the host (in the tc6393xb > >> case to enable buffers) > >> to enable access to the nand. > > > > So, the enable/disable calls might be useful, however is there any > > reason this could not be handled by the clock framework? The suspend/resume > > entries where not used, and I belive should not be in here. > > They should be here for exactly the same reason. They are used by the drivers > that will be submitted later. E.g. OHCI driver needs such > suspend/resume handling. > > > As noted before, mfd_get_cell() got dropped by [2] > > > > [2] http://lists.arm.linux.org.uk/lurker/message/20080708.153450.bb33046d.en.html > > Yes, and as I said before it will need some small modifications. > > -- > With best wishes > Dmitry > > ------------------------------------------------------------------- > List admin: http://lists.arm.linux.org.uk/mailman/listinfo/linux-arm-kernel > FAQ: http://www.arm.linux.org.uk/mailinglists/faq.php > Etiquette: http://www.arm.linux.org.uk/mailinglists/etiquette.php ^ permalink raw reply [flat|nested] 39+ messages in thread
* Re: [patch 4/4] MFD: Change mfd platform device usage to wrapper platform_device 2008-07-09 11:50 ` Ben Dooks 2008-07-09 11:56 ` Dmitry @ 2008-07-09 12:13 ` ian 2008-07-09 12:29 ` pHilipp Zabel 1 sibling, 1 reply; 39+ messages in thread From: ian @ 2008-07-09 12:13 UTC (permalink / raw) To: Ben Dooks; +Cc: Dmitry, linux-kernel, linux-arm-kernel On Wed, 2008-07-09 at 12:50 +0100, Ben Dooks wrote: > > So, the enable/disable calls might be useful, however is there any > reason this could not be handled by the clock framework? The > suspend/resume entries where not used, and I belive should not be in > here. Hi Ben, I agree with you on wrapping the platform device, however the enable / disable methods must stay. Other cores need them, and they arent only there for clock support. I've just looked at the TMIO MMC driver to refersh my memory and the suspend / resume hooks must stay also. TMIO MMC needs to be handled differently when suspending and when being disabled (loss of state when disabled). As yet, only tc6393 is 'in tree' but Im waiting for the core to settle down before I convert my other TMIO based code (again again) to the new core. So, in summary - ACK to your changes to wrap the device as an 'mfd_device' NAK to deleting the mfd_cell power management. ^ permalink raw reply [flat|nested] 39+ messages in thread
* Re: [patch 4/4] MFD: Change mfd platform device usage to wrapper platform_device 2008-07-09 12:13 ` ian @ 2008-07-09 12:29 ` pHilipp Zabel 0 siblings, 0 replies; 39+ messages in thread From: pHilipp Zabel @ 2008-07-09 12:29 UTC (permalink / raw) To: ian; +Cc: Ben Dooks, Dmitry, linux-kernel, linux-arm-kernel On Wed, Jul 9, 2008 at 2:13 PM, ian <spyro@f2s.com> wrote: > On Wed, 2008-07-09 at 12:50 +0100, Ben Dooks wrote: >> >> So, the enable/disable calls might be useful, however is there any >> reason this could not be handled by the clock framework? The >> suspend/resume entries where not used, and I belive should not be in >> here. > > Hi Ben, > > I agree with you on wrapping the platform device, however the enable / > disable methods must stay. Ack on that. Also look also at the ds1wm driver. Right now it provides its own enable/disable methods in platform data because it predates the MFD core. It should be changed into an MFD cell driver, so enable/disable methods in mfd_cell would be most useful for it. > Other cores need them, and they arent only there for clock support. > > I've just looked at the TMIO MMC driver to refersh my memory and the > suspend / resume hooks must stay also. TMIO MMC needs to be handled > differently when suspending and when being disabled (loss of state when > disabled). > > As yet, only tc6393 is 'in tree' but Im waiting for the core to settle > down before I convert my other TMIO based code (again again) to the new > core. > > So, in summary - > > ACK to your changes to wrap the device as an 'mfd_device' > NAK to deleting the mfd_cell power management. > > -- > To unsubscribe from this list: send the line "unsubscribe linux-kernel" in > the body of a message to majordomo@vger.kernel.org > More majordomo info at http://vger.kernel.org/majordomo-info.html > Please read the FAQ at http://www.tux.org/lkml/ regards Philipp ^ permalink raw reply [flat|nested] 39+ messages in thread
* Re: [patch 4/4] MFD: Change mfd platform device usage to wrapper platform_device 2008-07-09 11:15 ` Dmitry 2008-07-09 11:24 ` Ben Dooks @ 2008-07-09 11:45 ` ian 2008-07-09 11:52 ` Dmitry 1 sibling, 1 reply; 39+ messages in thread From: ian @ 2008-07-09 11:45 UTC (permalink / raw) To: Dmitry; +Cc: Ben Dooks, linux-kernel, linux-arm-kernel On Wed, 2008-07-09 at 15:15 +0400, Dmitry wrote: > NAK. > 0) It was discussed yesterday on the list and the decision was to go > in a different way. It was? I prefer the wrapped way personally... ^ permalink raw reply [flat|nested] 39+ messages in thread
* Re: [patch 4/4] MFD: Change mfd platform device usage to wrapper platform_device 2008-07-09 11:45 ` ian @ 2008-07-09 11:52 ` Dmitry 2008-07-09 21:03 ` Russell King - ARM Linux 0 siblings, 1 reply; 39+ messages in thread From: Dmitry @ 2008-07-09 11:52 UTC (permalink / raw) To: ian; +Cc: Ben Dooks, linux-kernel, linux-arm-kernel 2008/7/9 ian <spyro@f2s.com>: > On Wed, 2008-07-09 at 15:15 +0400, Dmitry wrote: >> NAK. >> 0) It was discussed yesterday on the list and the decision was to go >> in a different way. > > It was? > > I prefer the wrapped way personally... In any case IMO it's better to call platform_device_register() rather than device_initialise()/platform_device_add(). Samuel? Russell? -- With best wishes Dmitry ^ permalink raw reply [flat|nested] 39+ messages in thread
* Re: [patch 4/4] MFD: Change mfd platform device usage to wrapper platform_device 2008-07-09 11:52 ` Dmitry @ 2008-07-09 21:03 ` Russell King - ARM Linux 2008-07-09 21:13 ` Dmitry Baryshkov ` (2 more replies) 0 siblings, 3 replies; 39+ messages in thread From: Russell King - ARM Linux @ 2008-07-09 21:03 UTC (permalink / raw) To: Dmitry; +Cc: ian, linux-kernel, Ben Dooks, linux-arm-kernel On Wed, Jul 09, 2008 at 03:52:08PM +0400, Dmitry wrote: > 2008/7/9 ian <spyro@f2s.com>: > > On Wed, 2008-07-09 at 15:15 +0400, Dmitry wrote: > >> NAK. > >> 0) It was discussed yesterday on the list and the decision was to go > >> in a different way. > > > > It was? > > > > I prefer the wrapped way personally... > > In any case IMO it's better to call platform_device_register() rather than > device_initialise()/platform_device_add(). > > Samuel? Russell? WTF??? That's just completely wrong - assuming the internals of how the platform device alloc API works... What it's clear from my *brief* read of this thread is that the MFD support doesn't seem to be ready for mainline yet - there's clearly issues here that need further work. Given that, and where we are (there's maybe two of *my* days left until the merge window opens) I'm *very* tempted to drop the MFD support out of my tree for this merge window - which basically means removing 5127/1, 5128/1 and 5129/1. ^ permalink raw reply [flat|nested] 39+ messages in thread
* Re: [patch 4/4] MFD: Change mfd platform device usage to wrapper platform_device 2008-07-09 21:03 ` Russell King - ARM Linux @ 2008-07-09 21:13 ` Dmitry Baryshkov 2008-07-09 21:13 ` ian 2008-07-11 21:41 ` Samuel Ortiz 2 siblings, 0 replies; 39+ messages in thread From: Dmitry Baryshkov @ 2008-07-09 21:13 UTC (permalink / raw) To: Russell King - ARM Linux; +Cc: ian, linux-kernel, Ben Dooks, linux-arm-kernel On Wed, Jul 09, 2008 at 10:03:14PM +0100, Russell King - ARM Linux wrote: > On Wed, Jul 09, 2008 at 03:52:08PM +0400, Dmitry wrote: > > 2008/7/9 ian <spyro@f2s.com>: > > > On Wed, 2008-07-09 at 15:15 +0400, Dmitry wrote: > > >> NAK. > > >> 0) It was discussed yesterday on the list and the decision was to go > > >> in a different way. > > > > > > It was? > > > > > > I prefer the wrapped way personally... > > > > In any case IMO it's better to call platform_device_register() rather than > > device_initialise()/platform_device_add(). > > > > Samuel? Russell? > > WTF??? That's just completely wrong - assuming the internals of how the > platform device alloc API works... > > What it's clear from my *brief* read of this thread is that the MFD > support doesn't seem to be ready for mainline yet - there's clearly issues > here that need further work. > > Given that, and where we are (there's maybe two of *my* days left until > the merge window opens) I'm *very* tempted to drop the MFD support out > of my tree for this merge window - which basically means removing > 5127/1, 5128/1 and 5129/1. Why? We were talking about improvements, not the current code found in the referenced patches. They may be not the best ones, but they contain pretty clean code which works. The only issue with current code is that tc6393xb depends on some arm-specific defines, so we should either apply patch submitted today ("tc6393xb irq fixes") or make "depends on ARM". -- With best wishes Dmitry ^ permalink raw reply [flat|nested] 39+ messages in thread
* Re: [patch 4/4] MFD: Change mfd platform device usage to wrapper platform_device 2008-07-09 21:03 ` Russell King - ARM Linux 2008-07-09 21:13 ` Dmitry Baryshkov @ 2008-07-09 21:13 ` ian 2008-07-11 21:41 ` Samuel Ortiz 2 siblings, 0 replies; 39+ messages in thread From: ian @ 2008-07-09 21:13 UTC (permalink / raw) To: Russell King - ARM Linux Cc: Dmitry, linux-kernel, Ben Dooks, linux-arm-kernel On Wed, 2008-07-09 at 22:03 +0100, Russell King - ARM Linux wrote: > > Given that, and where we are (there's maybe two of *my* days left > until the merge window opens) I'm *very* tempted to drop the MFD > support out of my tree for this merge window - which basically means > removing 5127/1, 5128/1 and 5129/1. Please dont. The code thats in already is perfectly ok for what its doing. The discussion now is on ways to improve MFD for new devices, and somewhat tangential. All the improvements can wait until after 2.6.26. ^ permalink raw reply [flat|nested] 39+ messages in thread
* Re: [patch 4/4] MFD: Change mfd platform device usage to wrapper platform_device 2008-07-09 21:03 ` Russell King - ARM Linux 2008-07-09 21:13 ` Dmitry Baryshkov 2008-07-09 21:13 ` ian @ 2008-07-11 21:41 ` Samuel Ortiz 2 siblings, 0 replies; 39+ messages in thread From: Samuel Ortiz @ 2008-07-11 21:41 UTC (permalink / raw) To: Russell King - ARM Linux Cc: Dmitry, linux-kernel, Ben Dooks, linux-arm-kernel On Wed, Jul 09, 2008 at 10:03:14PM +0100, Russell King - ARM Linux wrote: > On Wed, Jul 09, 2008 at 03:52:08PM +0400, Dmitry wrote: > > 2008/7/9 ian <spyro@f2s.com>: > > > On Wed, 2008-07-09 at 15:15 +0400, Dmitry wrote: > > >> NAK. > > >> 0) It was discussed yesterday on the list and the decision was to go > > >> in a different way. > > > > > > It was? > > > > > > I prefer the wrapped way personally... > > > > In any case IMO it's better to call platform_device_register() rather than > > device_initialise()/platform_device_add(). > > > > Samuel? Russell? > > WTF??? That's just completely wrong - assuming the internals of how the > platform device alloc API works... > > What it's clear from my *brief* read of this thread is that the MFD > support doesn't seem to be ready for mainline yet - there's clearly issues > here that need further work. I think the main issue at the moment is that it's not passing generic platform data if we ever want to use an existing driver as a cell driver. I'd say this is more of a lack of feature rather than broken code, so please dont drop the mfd-core patch. Cheers, Samuel. > Given that, and where we are (there's maybe two of *my* days left until > the merge window opens) I'm *very* tempted to drop the MFD support out > of my tree for this merge window - which basically means removing > 5127/1, 5128/1 and 5129/1. > > ------------------------------------------------------------------- > List admin: http://lists.arm.linux.org.uk/mailman/listinfo/linux-arm-kernel > FAQ: http://www.arm.linux.org.uk/mailinglists/faq.php > Etiquette: http://www.arm.linux.org.uk/mailinglists/etiquette.php ^ permalink raw reply [flat|nested] 39+ messages in thread
* Re: [patch 4/4] MFD: Change mfd platform device usage to wrapper platform_device 2008-07-09 10:49 ` [patch 4/4] MFD: Change mfd platform device usage to wrapper platform_device Ben Dooks 2008-07-09 11:15 ` Dmitry @ 2008-07-09 20:56 ` Russell King - ARM Linux 2008-07-09 21:04 ` Ben Dooks 1 sibling, 1 reply; 39+ messages in thread From: Russell King - ARM Linux @ 2008-07-09 20:56 UTC (permalink / raw) To: Ben Dooks; +Cc: linux-kernel, linux-arm-kernel, dbaryshkov On Wed, Jul 09, 2008 at 11:49:20AM +0100, Ben Dooks wrote: > This patch changes the mfd core behaviour to wrapper the platform_device > it creates in an struct mfd_device which contains the information > about the cell that was created. You can't do this. Grab a reference to the platform device (by holding one of its sysfs files open) and then remove all the users of the mfd-core module and the mfd-core module itself. Then, read from that file and close it. Watch your kernel oops. That's why device release methods inside modules are a BAD IDEA and why we have the platform device alloc API. ^ permalink raw reply [flat|nested] 39+ messages in thread
* Re: [patch 4/4] MFD: Change mfd platform device usage to wrapper platform_device 2008-07-09 20:56 ` Russell King - ARM Linux @ 2008-07-09 21:04 ` Ben Dooks 0 siblings, 0 replies; 39+ messages in thread From: Ben Dooks @ 2008-07-09 21:04 UTC (permalink / raw) To: Russell King - ARM Linux Cc: Ben Dooks, dbaryshkov, linux-kernel, linux-arm-kernel On Wed, Jul 09, 2008 at 09:56:25PM +0100, Russell King - ARM Linux wrote: > On Wed, Jul 09, 2008 at 11:49:20AM +0100, Ben Dooks wrote: > > This patch changes the mfd core behaviour to wrapper the platform_device > > it creates in an struct mfd_device which contains the information > > about the cell that was created. > > You can't do this. Grab a reference to the platform device (by holding > one of its sysfs files open) and then remove all the users of the mfd-core > module and the mfd-core module itself. > > Then, read from that file and close it. Watch your kernel oops. > > That's why device release methods inside modules are a BAD IDEA and why > we have the platform device alloc API. Would this be fixed by grabbing a reference to the mfd-core module whilst the mfd device is open? -- Ben Q: What's a light-year? A: One-third less calories than a regular year. ^ permalink raw reply [flat|nested] 39+ messages in thread
end of thread, other threads:[~2008-07-29 0:07 UTC | newest] Thread overview: 39+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2008-07-09 10:49 [patch 0/4] mfd updates and proposed changes Ben Dooks 2008-07-09 10:49 ` [patch 1/4] MFD: Use to_platform_device instead of container_of Ben Dooks 2008-07-09 11:10 ` Dmitry 2008-07-10 14:47 ` Samuel Ortiz 2008-07-29 0:06 ` Samuel Ortiz 2008-07-09 10:49 ` [patch 2/4] MFD: Coding style fixes Ben Dooks 2008-07-09 11:11 ` Dmitry 2008-07-09 11:12 ` Ben Dooks 2008-07-10 14:48 ` Samuel Ortiz 2008-07-09 11:46 ` ian 2008-07-29 0:07 ` Samuel Ortiz 2008-07-09 10:49 ` [patch 3/4] MFD: Remove unnecessary fields if mfd_cell structure Ben Dooks 2008-07-09 11:09 ` Dmitry 2008-07-09 11:12 ` Ben Dooks 2008-07-09 11:16 ` Dmitry 2008-07-09 11:38 ` Ben Dooks 2008-07-09 11:44 ` Dmitry 2008-07-09 10:49 ` [patch 4/4] MFD: Change mfd platform device usage to wrapper platform_device Ben Dooks 2008-07-09 11:15 ` Dmitry 2008-07-09 11:24 ` Ben Dooks 2008-07-09 11:31 ` Dmitry 2008-07-09 11:50 ` Ben Dooks 2008-07-09 11:56 ` Dmitry 2008-07-09 12:07 ` Ben Dooks 2008-07-09 12:31 ` Dmitry 2008-07-09 13:28 ` ian 2008-07-09 13:34 ` pHilipp Zabel 2008-07-09 13:37 ` ian 2008-07-11 21:37 ` Samuel Ortiz 2008-07-09 12:13 ` ian 2008-07-09 12:29 ` pHilipp Zabel 2008-07-09 11:45 ` ian 2008-07-09 11:52 ` Dmitry 2008-07-09 21:03 ` Russell King - ARM Linux 2008-07-09 21:13 ` Dmitry Baryshkov 2008-07-09 21:13 ` ian 2008-07-11 21:41 ` Samuel Ortiz 2008-07-09 20:56 ` Russell King - ARM Linux 2008-07-09 21:04 ` Ben Dooks
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).