Re: [PATCH] ndfc driver

Josh Boyer
2008-12-04T14:01:23Z

On Wed, 3 Dec 2008 22:28:32 -0500
Sean MacLennan <smaclennan@pikatech.com> wrote:

Hi Sean,

A couple of comments/requests below.

> The current ndfc driver only compiles under arch/ppc. This arch was
> removed from the kernel. I notice the event entry for the ndfc in
> Kconfig has been removed in 2.6.28.
>
> This patch converts the ndfc to a proper OF (OpenFirmware) driver. I
> can give a working example of the DTS if needed.

In addition to an example DTS patch (probably to warp itself), could
you briefly write up a binding and put it in
Documentation/powerpc/dts-bindings/amcc (or similar)? Also please CC
the devicetree-discuss list on that part.

> The patch has been in production use on the PIKA Warp Appliance and is
> in use by others. The Warp basically boots from NAND, so the ndfc driver
> is very important to us.

Looking over the patch it seems pretty straight-forward and I don't see
anything immediately wrong with it. You do have a number of
semi-unrelated changes to the actual port to of_platform though, like
the s/__raw_writel/out_be32 stuff, the addition of partition parsing,
etc. I'm wondering if you could do those fixups separately from the
actual port.

Also, could you document why the data structures changed as they did in
the changelog or perhaps in a summary email.

You also seem to only support a single NAND chip, however the NDFC can
support multiple chips. Have you looked at how the the fsl_elbc_nand
driver does multiple chip support? If not, could you at least document
the limitation in the patch?

> This is a bi-monthly posting ;)

Keep at it Sean. Your work is appreciated, even if the rest of us are
slow to review.

josh