From: Ben Dooks <ben-linux@fluff.org>
To: Dmitry <dbaryshkov@gmail.com>
Cc: Ben Dooks <ben-linux@fluff.org>,
linux-kernel@vger.kernel.org,
linux-arm-kernel@lists.arm.linux.org.uk
Subject: Re: [patch 4/4] MFD: Change mfd platform device usage to wrapper platform_device
Date: Wed, 9 Jul 2008 12:50:45 +0100 [thread overview]
Message-ID: <20080709115045.GK8517@trinity.fluff.org> (raw)
In-Reply-To: <bc64b4640807090431g5b1960edua2508337973615da@mail.gmail.com>
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.
next prev parent reply other threads:[~2008-07-09 11:50 UTC|newest]
Thread overview: 39+ messages / expand[flat|nested] mbox.gz Atom feed top
2008-07-09 10:49 [patch 0/4] mfd updates and proposed changes Ben Dooks
2008-07-09 10:49 ` [patch 1/4] MFD: Use to_platform_device instead of container_of Ben Dooks
2008-07-09 11:10 ` Dmitry
2008-07-10 14:47 ` Samuel Ortiz
2008-07-29 0:06 ` Samuel Ortiz
2008-07-09 10:49 ` [patch 2/4] MFD: Coding style fixes Ben Dooks
2008-07-09 11:11 ` Dmitry
2008-07-09 11:12 ` Ben Dooks
2008-07-10 14:48 ` Samuel Ortiz
2008-07-09 11:46 ` ian
2008-07-29 0:07 ` Samuel Ortiz
2008-07-09 10:49 ` [patch 3/4] MFD: Remove unnecessary fields if mfd_cell structure Ben Dooks
2008-07-09 11:09 ` Dmitry
2008-07-09 11:12 ` Ben Dooks
2008-07-09 11:16 ` Dmitry
2008-07-09 11:38 ` Ben Dooks
2008-07-09 11:44 ` Dmitry
2008-07-09 10:49 ` [patch 4/4] MFD: Change mfd platform device usage to wrapper platform_device Ben Dooks
2008-07-09 11:15 ` Dmitry
2008-07-09 11:24 ` Ben Dooks
2008-07-09 11:31 ` Dmitry
2008-07-09 11:50 ` Ben Dooks [this message]
2008-07-09 11:56 ` Dmitry
2008-07-09 12:07 ` Ben Dooks
2008-07-09 12:31 ` Dmitry
2008-07-09 13:28 ` ian
2008-07-09 13:34 ` pHilipp Zabel
2008-07-09 13:37 ` ian
2008-07-11 21:37 ` Samuel Ortiz
2008-07-09 12:13 ` ian
2008-07-09 12:29 ` pHilipp Zabel
2008-07-09 11:45 ` ian
2008-07-09 11:52 ` Dmitry
2008-07-09 21:03 ` Russell King - ARM Linux
2008-07-09 21:13 ` Dmitry Baryshkov
2008-07-09 21:13 ` ian
2008-07-11 21:41 ` Samuel Ortiz
2008-07-09 20:56 ` Russell King - ARM Linux
2008-07-09 21:04 ` Ben Dooks
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=20080709115045.GK8517@trinity.fluff.org \
--to=ben-linux@fluff.org \
--cc=dbaryshkov@gmail.com \
--cc=linux-arm-kernel@lists.arm.linux.org.uk \
--cc=linux-kernel@vger.kernel.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).