Re: [PATCH 2/6] arm/tegra: prepare pinmux code for multiple tegra variants

[Peter De Schrijver <pdeschrijver-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org>]

On Thu, Oct 27, 2011 at 09:59:49PM +0200, Stephen Warren wrote:
> Peter De Schrijver wrote at Tuesday, October 25, 2011 10:54 AM:
> > This patch modifies the pinmux code to be useable for multiple tegra variants.
> > Some tegra20 specific constants will be replaced by variables which will be
> > initialized to the appropriate value at runtime.
> 
> You should probably also mention:
> * Removal of include of pinmux-t2.h from pinmux.h.
> * The file renames you performed and why.
> * The removal of the suspend/resume code and why.
> 
> > diff --git a/arch/arm/mach-tegra/include/mach/pinmux-t2.h b/arch/arm/mach-tegra/include/mach/pinmux-t2.h
> > deleted file mode 100644
> > index 4c26263..0000000
> > --- a/arch/arm/mach-tegra/include/mach/pinmux-t2.h
> > +++ /dev/null
> 
> So this file shows up as a delete and an add. Isn't there a way to get
> it to show up as a rename; I've certainly seen git diff tell me that, but
> I'm not sure if the same feature is available for format-patch. In
> particular, IIRC from internal review, there weren't any changes to this
> file besides direct fallout from the rename, but there were quite a few
> symbol name changes in pinmux-t2-tables.c, and also the removal of
> suspend/resume, which it'd be nice if the diff highlighted.
> 

Hmm. Maybe if I split it in 2 patches? 2 rename and 1 modification?

> > diff --git a/arch/arm/mach-tegra/include/mach/pinmux.h b/arch/arm/mach-tegra/include/mach/pinmux.h
> > index bb7dfdb..8f6e9f3 100644
> > --- a/arch/arm/mach-tegra/include/mach/pinmux.h
> > +++ b/arch/arm/mach-tegra/include/mach/pinmux.h
> ...
> > @@ -219,13 +213,20 @@ struct tegra_pingroup_desc {
> >  	s8 pupd_bit;	/* offset into the PULL_UPDOWN_REG_* register bit */
> >  };
> > 
> > -extern const struct tegra_pingroup_desc tegra_soc_pingroups[];
> > -extern const struct tegra_drive_pingroup_desc tegra_soc_drive_pingroups[];
> > +struct tegra_pinmux_pm {
> > +	void (*suspend)(void);
> > +	void (*resume)(void);
> > +};
> 
> Since we elected to strip out suspend/resume (it's currently incomplete
> anyway) to simplify the addition of Tegra30, I don't think you need that
> struct type any more.
> 

True :)

> I wonder if removal of the suspend/resume functions should have been a
> separate patch before this?
> 

Good point.

> > diff --git a/arch/arm/mach-tegra/pinmux.c b/arch/arm/mach-tegra/pinmux.c
> > index 1d20165..0789122 100644
> > --- a/arch/arm/mach-tegra/pinmux.c
> > +++ b/arch/arm/mach-tegra/pinmux.c
> > @@ -21,6 +21,9 @@
> >  #include <linux/spinlock.h>
> >  #include <linux/io.h>
> >  #include <linux/platform_device.h>
> > +#ifdef CONFIG_OF
> > +#include <linux/of_platform.h>
> > +#endif
> 
> I think just include that without any ifdef.
> 

Thinking of it, probably indeed.

> > @@ -668,11 +668,24 @@ void tegra_pinmux_config_pullupdown_table(const struct tegra_pingroup_config *co
> >  	}
> >  }
> > 
> > +static struct of_device_id tegra_pinmux_of_match[] __devinitdata = {
> > +	{ .compatible = "nvidia,tegra20-pinmux", tegra20_pinmux_init },
> > +	{ },
> > +};
> > +
> >  static int __devinit tegra_pinmux_probe(struct platform_device *pdev)
> >  {
> >  	struct resource *res;
> >  	int i;
> >  	int config_bad = 0;
> > +	const struct of_device_id *match;
> > +
> > +#ifdef CONFIG_OF
> > +	match = of_match_device(tegra_pinmux_of_match, &pdev->dev);
> 
> What if match==NULL? I suppose that "can't" happen with DT, since the
> device wouldn't have been probed unless there was a matching entry...
> 

Exactly. That was my reasoning as well. We can't get here unless there is a
match.

> Does this work when booting without DT; IIRC the internal patches I saw
> fell back to hard-coding a call to tegra20_pinmux_init() when booting
> a static board file (i.e. not booting device-tree)? Perhaps that's in a

Unfortunately I don't have a tegra20 board which is supported without
devicetree.

Cheers,

Peter.