[U-Boot] [PATCH 12/23] ARM: tegra: Implement XUSB pad controller

[Thierry Reding <thierry.reding@gmail.com>]

On Wed, Aug 20, 2014 at 12:32:45PM -0600, Stephen Warren wrote:
> On 08/18/2014 01:16 AM, Thierry Reding wrote:
> >From: Thierry Reding <treding@nvidia.com>
> >
> >This controller was introduced on Tegra114 to handle XUSB pads. On
> >Tegra124 it is also used for PCIe and SATA pin muxing and PHY control.
> >Only the Tegra124 PCIe and SATA functionality is currently implemented,
> >with weak symbols on Tegra114.
> >
> >Tegra20 and Tegra30 also provide weak symbols for these functions so
> >that drivers can use the same API irrespective of which SoC they're
> >being built for.
> 
> >  arch/arm/include/asm/arch-tegra/xusb-padctl.h    |  14 +
> >  arch/arm/include/asm/arch-tegra114/xusb-padctl.h |   6 +
> >  arch/arm/include/asm/arch-tegra124/xusb-padctl.h |   6 +
> >  arch/arm/include/asm/arch-tegra20/xusb-padctl.h  |   6 +
> >  arch/arm/include/asm/arch-tegra30/xusb-padctl.h  |   6 +
> 
> These per-SoC headers do nothing but include the common header. Why not just
> have the code include the common header directly? IIRC, that's what we do
> now for some other modules that are identical. We only need SoC-specific
> headers if we have SoC-specific information to represent.

Done.

> >diff --git a/arch/arm/cpu/tegra124-common/Makefile b/arch/arm/cpu/tegra124-common/Makefile
> 
> >  obj-y	+= clock.o
> >  obj-y	+= funcmux.o
> >  obj-y	+= pinmux.o
> >+obj-y	+= xusb-padctl.o
> 
> It looks like there's indentation inconsistency there?

I don't have it in my local tree. But perhaps I've fixed it meanwhile
without remembering?

> >diff --git a/arch/arm/cpu/tegra124-common/xusb-padctl.c b/arch/arm/cpu/tegra124-common/xusb-padctl.c
> 
> >+int fdtdec_count_strings(const void *fdt, int node, const char *prop_name)
> 
> >+int fdtdec_get_string_index(const void *fdt, int node, const char *prop_name,
> >+			    int index, const char **output)
> 
> >+int fdtdec_get_string(const void *fdt, int node, const char *prop_name,
> 
> Shouldn't those be in libfdt or similar? I thought I saw a patch to do that.

I'm not sure where exactly to put them. They are slightly different from
the fdt_get_string_index() that's implemented in an earlier patch in
that they return a pointer to the string at a given index whereas the
fdt_get_string_index() function looks up a string in a list and returns
its index. The naming is horrible, I know, but I'm open to suggestions.

> >diff --git a/arch/arm/include/asm/arch-tegra/xusb-padctl.h b/arch/arm/include/asm/arch-tegra/xusb-padctl.h
> 
> >+struct tegra_xusb_phy *tegra_xusb_phy_get(unsigned int type);
> 
> What is "type"? A comment would be useful, or using an enum type instead.

The value should come directly from DT, so an enum won't be very useful.
It's also very much SoC dependent, so it's difficult to give an example
that works for all SoCs. I'll try what I can come up with for a comment.

Thierry
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 819 bytes
Desc: not available
URL: <http://lists.denx.de/pipermail/u-boot/attachments/20140822/6312c0fe/attachment.pgp>