[patch 0/4] mfd updates and proposed changes

[patch 0/4] mfd updates and proposed changes

[Ben Dooks]

Updates and changes to mfd

-- 

[patch 1/4] MFD: Use to_platform_device instead of container_of.

[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;
 }
 

-- 

[patch 2/4] MFD: Coding style fixes

[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);
 

-- 

[patch 3/4] MFD: Remove unnecessary fields if mfd_cell structure.

[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 */
 
 	/*

-- 

[patch 4/4] MFD: Change mfd platform device usage to wrapper platform_device

[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;

-- 

Re: [patch 3/4] MFD: Remove unnecessary fields if mfd_cell structure.

[Dmitry]

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

Re: [patch 1/4] MFD: Use to_platform_device instead of container_of.

[Dmitry]

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

Re: [patch 2/4] MFD: Coding style fixes

[Dmitry]

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

Re: [patch 3/4] MFD: Remove unnecessary fields if mfd_cell structure.

[Ben Dooks]

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.

Re: [patch 2/4] MFD: Coding style fixes

[Ben Dooks]

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.

Re: [patch 4/4] MFD: Change mfd platform device usage to wrapper platform_device

[Dmitry]

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

Re: [patch 3/4] MFD: Remove unnecessary fields if mfd_cell structure.

[Dmitry]

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

Re: [patch 4/4] MFD: Change mfd platform device usage to wrapper platform_device

[Ben Dooks]

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.

Re: [patch 4/4] MFD: Change mfd platform device usage to wrapper platform_device

[Dmitry]

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

Re: [patch 3/4] MFD: Remove unnecessary fields if mfd_cell structure.

[Ben Dooks]

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.

Re: [patch 3/4] MFD: Remove unnecessary fields if mfd_cell structure.

[Dmitry]

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

Re: [patch 4/4] MFD: Change mfd platform device usage to wrapper platform_device

[ian]

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...

Re: [patch 2/4] MFD: Coding style fixes

[ian]

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.

Re: [patch 4/4] MFD: Change mfd platform device usage to wrapper platform_device

[Ben Dooks]

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.

Re: [patch 4/4] MFD: Change mfd platform device usage to wrapper platform_device

[Dmitry]

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

Re: [patch 4/4] MFD: Change mfd platform device usage to wrapper platform_device

[Dmitry]

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

Re: [patch 4/4] MFD: Change mfd platform device usage to wrapper platform_device

[Ben Dooks]

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.

Re: [patch 4/4] MFD: Change mfd platform device usage to wrapper platform_device

[ian]

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.

Re: [patch 4/4] MFD: Change mfd platform device usage to wrapper platform_device

[pHilipp Zabel]

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

Re: [patch 4/4] MFD: Change mfd platform device usage to wrapper platform_device

[Dmitry]

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

Re: [patch 4/4] MFD: Change mfd platform device usage to wrapper platform_device

[ian]

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.

Re: [patch 4/4] MFD: Change mfd platform device usage to wrapper platform_device

[pHilipp Zabel]

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/
>

Re: [patch 4/4] MFD: Change mfd platform device usage to wrapper platform_device

[ian]

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)

:-)

Re: [patch 4/4] MFD: Change mfd platform device usage to wrapper platform_device

[Russell King - ARM Linux]

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.

Re: [patch 4/4] MFD: Change mfd platform device usage to wrapper platform_device

[Russell King - ARM Linux]

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.

Re: [patch 4/4] MFD: Change mfd platform device usage to wrapper platform_device

[Ben Dooks]

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.

Re: [patch 4/4] MFD: Change mfd platform device usage to wrapper platform_device

[Dmitry Baryshkov]

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

Re: [patch 4/4] MFD: Change mfd platform device usage to wrapper platform_device

[ian]

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.

Re: [patch 1/4] MFD: Use to_platform_device instead of container_of.

[Samuel Ortiz]

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

Re: [patch 2/4] MFD: Coding style fixes

[Samuel Ortiz]

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.
> 

Re: [patch 4/4] MFD: Change mfd platform device usage to wrapper platform_device

[Samuel Ortiz]

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

Re: [patch 4/4] MFD: Change mfd platform device usage to wrapper platform_device

[Samuel Ortiz]

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

Re: [patch 1/4] MFD: Use to_platform_device instead of container_of.

[Samuel Ortiz]

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;
>  }
>  
> 
> -- 

Re: [patch 2/4] MFD: Coding style fixes

[Samuel Ortiz]

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