Thomas Gleixner wrote:
> On Fri, 26 Oct 2007, Valentine Barshak wrote:
>> The major difference is that the original implements each chip connected
>> NDFC banks as a
>> separate MTD device. Here I try to have one MTD device spread on all chips
>> found.
>> However, the chips should have equal ID's and sizes, but I've never seen
>> several different
>> chips attached to single ndfc.
>
> Bamboo has 2 different nand chips. And I'm aware of another board
> which has a 2k-page onboard NAND and sockets for SmartMedia /
> PictureXd cards, which will simply break with your design.
>
> Restricting stuff for no good reason is never a good idea.
>
> mtdconcat can build you a big one if you want, so why adding
> restrictions to a driver ?
>
>> I'm adding ndfc_of as a separate file, since some other changes
>> have also been made (e.g. all i/o is made with ndfc_readl/writel inline
>> functions).
>
> This should be done in the original ndfc driver and not in a separate
> incarnation.
>
>> The original version didn't handle driver removal well (it never calls
>> del_mtd...),it's
>> corrected here.
>
> Why not fixing the original driver ?
>
>> Any comments are greatly appreciated.
>
> NACK.
>
> Please fix the existing driver and convert it to deal with OF and fix
> the other short comings as well.
>
> Duplicate code is not going anywhere near drivers/mtd/nand
>
> tglx
Thanks all for your comments.
Well, let me explain why I did this.
First of all I should have checked twice, since I was thinking bamboo
had identical chips :) I planned to add different chip support later a
bit, just wanted to get a simple OF driver version working first.
Surely, mtd concat can be used, but it adds a slight overhead for
identical chips. Eventually, I wanted to make it support both separate
mtd devices on each chip or spread across identical ones depending on
the device-tree settings. The other thing is that the original driver
lacked a distinct parent-child relationship between ndfc and chips.
Simply registering chip driver only after ndfc is successfully
registered doesn't guarantee we initialize chip device after ndfc is
properly initialized.
Even if we set ndfc parent for the chip platform device as suggested by
Stefan:
static struct platform_device nand_dev = {
.name = "ndfc-chip",
.id = 0,
.num_resources = 1,
.resource = &r,
.dev = {
.platform_data = &nand_chip,
.parent = &ndfc_dev.dev,
}
};
If for some reason ndfc probe fails the kernel will crash on the chip
probe. Of course it would work most of the time, but I think that both
initialization and clean-up has to be reworked in the original driver.
The original driver has a tight connection to the
platform_device/platform_data structures. Stefan suggested a
OF-to-platform device wrapper to make both versions work:
http://ozlabs.org/pipermail/linuxppc-dev/2007-October/044019.html
It's fine to have this glue code, but right now only 1 chip is
supported. To add support for more chips we need an array of at least 4
ndfc-chip platform devices. And this approach looks to me like inventing
something new (OF) and then adding glue and quirks to make it work with
the old stuff. Why invent new stuff then?
To make the original driver work with both "platform device" and new OF
device descriptions need additional rework of the current ndfc code (add
some #ifdefs, or completely split platform/OF and nand core stuff into
separate files). I know that duplicating code is no good either, but
since the original stuff is going to die and be removed anyway should it
be a big problem?
So, these are just my thoughts.
Thanks,
Valentine.
_______________________________________________
Linuxppc-dev mailing list
Linuxppc-dev@ozlabs.org
https://ozlabs.org/mailman/listinfo/linuxppc-dev
