From: Stephen Warren <swarren@wwwdotorg.org>
To: Laxman Dewangan <ldewangan@nvidia.com>,
linus.walleij@linaro.org, thierry.reding@gmail.com,
Thierry Reding <treding@nvidia.com>
Cc: rob.herring@calxeda.com, pawel.moll@arm.com,
mark.rutland@arm.com, ijc+devicetree@hellion.org.uk,
grant.likely@linaro.org, devicetree@vger.kernel.org,
linux-doc@vger.kernel.org, linux-tegra@vger.kernel.org,
linux-kernel@vger.kernel.org,
linux-arm-kernel@lists.infradead.org,
Ashwini Ghuge <aghuge@nvidia.com>
Subject: Re: [PATCH 4/4] pinctrl: tegra: add pinmux controller driver for Tegra124
Date: Thu, 05 Dec 2013 16:30:03 -0700 [thread overview]
Message-ID: <52A10C7B.8070406@wwwdotorg.org> (raw)
In-Reply-To: <1386241070-4350-5-git-send-email-ldewangan@nvidia.com>
On 12/05/2013 03:57 AM, Laxman Dewangan wrote:
> From: Ashwini Ghuge <aghuge@nvidia.com>
>
> This adds a driver for the Tegra124 pinmux, and required
> parameterization data for Tegra124.
>
> The driver uses the common Tegra pincontrol driver utility
> functions to implement the majority of the driver.
>
> This driver is not compatible with the earlier NVIDIA's SoCs,
> hence add new compatibile as "nvidia,tegra124-pinmux".
>
> Originally written by Ashwini Gguhe.
> ldewangan:
> - cleanup the patches,
> - Fix address issue.
IIRC, Thierry mentioned he had some fixes in his local branch for this
driver. Thierry, can you please confirm/deny this?
I made the following comment on the version Ashwini posted, which hasn't
been addressed yet:
A day or two ago during upstream review:
>> +static const struct tegra_function tegra124_functions[] = {
> ...
>> + FUNCTION(i2c1),
>> + FUNCTION(i2c2),
>> + FUNCTION(i2c3),
>> + FUNCTION(i2c4),
>> + FUNCTION(i2cpwr),
>
> Is that complete? Tegra124 apparently has 6 I2C controllers. Are the
> pins for the new sixth controller (0x7000d100) not affected by the pinmux?
That said, if we find things are missing, I suppose we can add them
later without breaking existing ABI. Breakage would only happen if we
had to change/remove something.
During downstream review quite a while ago I also said:
>> > +static const struct pinctrl_pin_desc tegra124_pins[] = {
>>
>> There are two spaces before "tegra124_pins[]".
>>
>> > +static const char * const gmi_groups[] = {
>> > + "uart2_cts_n_pj5",
>> > + "uart2_rts_n_pj6",
>> > + "uart3_txd_pw6",
>> > + "uart3_rxd_pw7",
>> > + "uart3_cts_n_pa1",
>> > + "uart3_rts_n_pc0",
>> > +
>> > + "pu0",
>>
>> It'd be best not to have blank lines in the middle of arrays. The same comment exists elsewhere in the
>> file, so make sure you search the whole file.
Nits:
- There are some cases of multiple blank lines back-to-back.
- There's a blank line at the end of the file.
Aside from those minor issues, patches 1/4 and 4/4,
Acked-by: Stephen Warren <swarren@nvidia.com>
(BTW, those 2 patches would go through the pinctrl tree, and patches 2/4
and 3/4 would go through the Tegra tree. You generally shouldn't posted
patches that will be applied to different trees in the same series,
since there aren't dependencies).
next prev parent reply other threads:[~2013-12-05 23:30 UTC|newest]
Thread overview: 16+ messages / expand[flat|nested] mbox.gz Atom feed top
2013-12-05 10:57 [PATCH 0/4] pinctrl: tegra: Add pincontrol driver for Tegra124 SoC Laxman Dewangan
2013-12-05 10:57 ` [PATCH 1/4] pinctrl: tegra: Add devicetree binding document for Tegra124 Laxman Dewangan
2013-12-05 23:17 ` Stephen Warren
2013-12-06 6:21 ` Laxman Dewangan
2013-12-05 10:57 ` [PATCH 2/4] ARM: tegra: add pinmux controller to tegra124.dtsi Laxman Dewangan
2013-12-05 23:16 ` Stephen Warren
2013-12-05 23:19 ` Stephen Warren
2013-12-06 6:05 ` Laxman Dewangan
2013-12-05 10:57 ` [PATCH 3/4] ARM: tegra: select PINCTRL_TEGRA124 for Tegra124 SoC Laxman Dewangan
2013-12-05 23:17 ` Stephen Warren
2013-12-12 18:36 ` Stephen Warren
2013-12-05 10:57 ` [PATCH 4/4] pinctrl: tegra: add pinmux controller driver for Tegra124 Laxman Dewangan
2013-12-05 23:30 ` Stephen Warren [this message]
2013-12-06 6:27 ` Laxman Dewangan
2013-12-06 17:08 ` Stephen Warren
2013-12-06 9:58 ` Thierry Reding
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=52A10C7B.8070406@wwwdotorg.org \
--to=swarren@wwwdotorg.org \
--cc=aghuge@nvidia.com \
--cc=devicetree@vger.kernel.org \
--cc=grant.likely@linaro.org \
--cc=ijc+devicetree@hellion.org.uk \
--cc=ldewangan@nvidia.com \
--cc=linus.walleij@linaro.org \
--cc=linux-arm-kernel@lists.infradead.org \
--cc=linux-doc@vger.kernel.org \
--cc=linux-kernel@vger.kernel.org \
--cc=linux-tegra@vger.kernel.org \
--cc=mark.rutland@arm.com \
--cc=pawel.moll@arm.com \
--cc=rob.herring@calxeda.com \
--cc=thierry.reding@gmail.com \
--cc=treding@nvidia.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox