Date:	Wed, 9 Jul 2008 16:31:01 +0400
From:	Dmitry <dbaryshkov@...il.com>
To:	"Ben Dooks" <ben-linux@...ff.org>
Cc:	linux-kernel@...r.kernel.org,
	linux-arm-kernel@...ts.arm.linux.org.uk
Subject: Re: [patch 4/4] MFD: Change mfd platform device usage to wrapper platform_device

2008/7/9 Ben Dooks <ben-linux@...ff.org>:
> On Wed, Jul 09, 2008 at 03:56:54PM +0400, Dmitry wrote:
>> 2008/7/9 Ben Dooks <ben-linux@...ff.org>:
>> > On Wed, Jul 09, 2008 at 03:31:04PM +0400, Dmitry wrote:
>> >> 2008/7/9 Ben Dooks <ben-linux@...ff.org>:
>> >> > On Wed, Jul 09, 2008 at 03:15:47PM +0400, Dmitry wrote:
>> >> >> 2008/7/9 Ben Dooks <ben-linux@...ff.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@...ff.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
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Powered by blists - more mailing lists