Re: [PATCH 1/4] mtd: nand: add NVIDIA Tegra NAND Flash controller driver

[Thierry Reding <thierry.reding-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>]

[-- Attachment #1: Type: text/plain, Size: 3330 bytes --]

On Wed, Jan 07, 2015 at 01:24:38AM +0100, Lucas Stach wrote:
> Am Dienstag, den 06.01.2015, 15:27 -0300 schrieb Ezequiel Garcia:
> > On 01/04/2015 05:39 PM, Lucas Stach wrote:
[...]
> > > diff --git a/drivers/mtd/nand/Kconfig b/drivers/mtd/nand/Kconfig
> > > index 7d0150d..1eafd4e 100644
> > > --- a/drivers/mtd/nand/Kconfig
> > > +++ b/drivers/mtd/nand/Kconfig
> > > @@ -524,4 +524,10 @@ config MTD_NAND_SUNXI
> > >  	help
> > >  	  Enables support for NAND Flash chips on Allwinner SoCs.
> > >  
> > > +config MTD_NAND_TEGRA
> > > +	tristate "Support for NAND on NVIDIA Tegra"
> > > +	depends on ARCH_TEGRA || COMPILE_TEST

I think you're going to need a bunch more dependencies if you use
COMPILE_TEST. Otherwise we're going to get all kinds of build failure
reports.

> > > diff --git a/drivers/mtd/nand/tegra_nand.c b/drivers/mtd/nand/tegra_nand.c
[...]
> > > +struct tegra_nand {
> > > +	void __iomem *regs;
> > > +	int irq;
> > 
> > Seems like you don't need to store irq.
> > 
> > > +	struct clk *clk;
> > > +	struct reset_control *rst;
> > > +	int wp_gpio;
> > > +	int buswidth;
> > 
> > And also you don't seem to need either wp_gpio or buswidth stored
> > in the struct. You only use them at probe time.
> > 
> 
> I'll keep the wp_gpio, as I still hope to use this to WP the NAND when
> no write is pending. I'll fix the others.

Maybe use the gpiod_*() API since the old one is new deprecated?

> > > +static const struct of_device_id tegra_nand_of_match[] = {
> > > +	{ .compatible = "nvidia,tegra20-nand" },
> > > +	{ .compatible = "nvidia,tegra30-nand" },
> > 
> > AFAIK, having two compatible strings, but making no distinction between
> > them is typically frowned upon by devicetree maintainers.
> > 
> > Is the controller any different in tegra20 and tegra30?
> > 
> > If you are not sure about the controllers being different, you can
> > try the following approach. The devicetree is written like this:
> > 
> > nand@foo {
> >    compatible = "nvidia,tegra20-nand", "nvidia,tegra-nand";
> > };
> > 
> > So you only deal with "nvidia,tegra-nand" in the driver, yet the
> > devicetree files are prepared to deal with a difference.

I think it's been more common to have something like this:

	tegra20.dtsi:

		nand-controller@70008000 {
			compatible = "nvidia,tegra20-nand";
			...
		};

	tegra30.dtsi:

		nand-controller@70008000 {
			compatible = "nvidia,tegra30-nand", "nvidia,tegra20-nand";
			...
		};

The idea being that if the Tegra30 variant is indeed compatible with the
Tegra20 variant, the driver can match on "nvidia,tegra20-nand". But at
the same time the DTB has the more specific compatible in case the
driver ever needs to handle generation-specific quirks, or implement any
additional functionality added in Tegra30 that wasn't available in early
generations.

> I believe that tegra30-nand is actually a bit different from tegra20 (at
> least on more clock I know about), but obviously this driver doesn't
> handle those differences and I don't know if I ever get to see Tegra30
> hardware with NAND. Given that I think it's best to just remove the
> tegra30-nand compatible for now and add it back if someone has hardware
> to test with.

Yes, that sounds like the best option for now.

Thierry

[-- Attachment #2: Type: application/pgp-signature, Size: 819 bytes --]