LinuxLists.cc - [patch 4/4] MFD: Change mfd platform device usage to wrapper platform

[permalink] [raw]

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

2008/7/9 Ben Dooks <[email protected]>:
> 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 <[email protected]>

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

2008-07-09 11:24:39

[permalink] [raw]

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

On Wed, Jul 09, 2008 at 03:15:47PM +0400, Dmitry wrote:
> 2008/7/9 Ben Dooks <[email protected]>:
> > 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 <[email protected]>
>
>
> 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.

2008-07-09 11:31:18

[permalink] [raw]

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

2008/7/9 Ben Dooks <[email protected]>:
> On Wed, Jul 09, 2008 at 03:15:47PM +0400, Dmitry wrote:
>> 2008/7/9 Ben Dooks <[email protected]>:
>> > 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 <[email protected]>
>>
>>
>> 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

2008-07-09 11:45:32

[permalink] [raw]

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

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

2008-07-09 11:50:57

[permalink] [raw]

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

On Wed, Jul 09, 2008 at 03:31:04PM +0400, Dmitry wrote:
> 2008/7/9 Ben Dooks <[email protected]>:
> > On Wed, Jul 09, 2008 at 03:15:47PM +0400, Dmitry wrote:
> >> 2008/7/9 Ben Dooks <[email protected]>:
> >> > 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 <[email protected]>
> >>
> >>
> >> 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.

2008-07-09 11:52:23

[permalink] [raw]

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

2008/7/9 ian <[email protected]>:
> 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

2008-07-09 11:57:07

[permalink] [raw]

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

2008/7/9 Ben Dooks <[email protected]>:
> On Wed, Jul 09, 2008 at 03:31:04PM +0400, Dmitry wrote:
>> 2008/7/9 Ben Dooks <[email protected]>:
>> > On Wed, Jul 09, 2008 at 03:15:47PM +0400, Dmitry wrote:
>> >> 2008/7/9 Ben Dooks <[email protected]>:
>> >> > 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 <[email protected]>
>> >>
>> >>
>> >> 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

2008-07-09 12:07:30

[permalink] [raw]

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

On Wed, Jul 09, 2008 at 03:56:54PM +0400, Dmitry wrote:
> 2008/7/9 Ben Dooks <[email protected]>:
> > On Wed, Jul 09, 2008 at 03:31:04PM +0400, Dmitry wrote:
> >> 2008/7/9 Ben Dooks <[email protected]>:
> >> > On Wed, Jul 09, 2008 at 03:15:47PM +0400, Dmitry wrote:
> >> >> 2008/7/9 Ben Dooks <[email protected]>:
> >> >> > 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 <[email protected]>
> >> >>
> >> >>
> >> >> 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.

2008-07-09 12:15:50

[permalink] [raw]

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

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.

2008-07-09 12:29:46

by Philipp Zabel

[permalink] [raw]

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

On Wed, Jul 9, 2008 at 2:13 PM, ian <[email protected]> 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 [email protected]
> More majordomo info at http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at http://www.tux.org/lkml/

regards
Philipp

2008-07-09 12:31:19

[permalink] [raw]

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

2008/7/9 Ben Dooks <[email protected]>:
> On Wed, Jul 09, 2008 at 03:56:54PM +0400, Dmitry wrote:
>> 2008/7/9 Ben Dooks <[email protected]>:
>> > On Wed, Jul 09, 2008 at 03:31:04PM +0400, Dmitry wrote:
>> >> 2008/7/9 Ben Dooks <[email protected]>:
>> >> > On Wed, Jul 09, 2008 at 03:15:47PM +0400, Dmitry wrote:
>> >> >> 2008/7/9 Ben Dooks <[email protected]>:
>> >> >> > 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 <[email protected]>
>> >> >>
>> >> >>
>> >> >> 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

2008-07-09 13:28:49

[permalink] [raw]

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

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.

2008-07-09 13:34:48

by Philipp Zabel

[permalink] [raw]

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

On Wed, Jul 9, 2008 at 3:28 PM, ian <[email protected]> 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 [email protected]
> More majordomo info at http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at http://www.tux.org/lkml/
>

2008-07-09 13:37:40

[permalink] [raw]

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

On Wed, 2008-07-09 at 15:34 +0200, pHilipp Zabel wrote:
> On Wed, Jul 9, 2008 at 3:28 PM, ian <[email protected]> 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)

:-)

2008-07-09 20:57:56

by Russell King - ARM Linux

[permalink] [raw]

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

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.

2008-07-09 21:03:36

by Russell King - ARM Linux

[permalink] [raw]

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

On Wed, Jul 09, 2008 at 03:52:08PM +0400, Dmitry wrote:
> 2008/7/9 ian <[email protected]>:
> > 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.

2008-07-09 21:05:01

[permalink] [raw]

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

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.

2008-07-09 21:13:44

[permalink] [raw]

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

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 <[email protected]>:
> > > 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

2008-07-09 21:14:41