linux-arm-kernel.lists.infradead.org archive mirror
 help / color / mirror / Atom feed
From: sebastian.hesselbarth@gmail.com (Sebastian Hesselbarth)
To: linux-arm-kernel@lists.infradead.org
Subject: [PATCH v2] arm: dts: add initial support for TBS2910 Matrix ARM mini PC
Date: Sat, 25 Oct 2014 20:51:33 +0200	[thread overview]
Message-ID: <544BF135.4030503@gmail.com> (raw)
In-Reply-To: <544BE2E6.4040903@web.de>

On 25.10.2014 19:50, Soeren Moch wrote:
> thanks for your comments.
>>
>> Very neat patch! A couple of minor comments below ...
>>
>> On Tue, Oct 21, 2014 at 10:23:18PM +0200, Soeren Moch wrote:
>>> TBS2910 is a i.MX6Q based board. For additional details refer to
>>> http://www.tbsdtv.com/products/tbs2910-matrix-arm-mini-pc.html
>>>
>>> Reviewed-by: Sebastian Hesselbarth <sebastian.hesselbarth@gmail.com>
>>> Signed-off-by: Soeren Moch <smoch@web.de>
>>> ---
>>> Cc: Sebastian Hesselbarth <sebastian.hesselbarth@gmail.com>
>>> Cc: Shawn Guo <shawn.guo@linaro.org>
>>> Cc: Sascha Hauer <kernel@pengutronix.de>
>>>
>>> Changes for v2:
>>> - add tbs vendor prefix to vendor-prefixes.txt
>>> - use GPIO_ACTIVE_{HIGH,LOW}
>>> - add led label and default-state="keep"
>>> - whitespace cleanup
>>> ---
>>>   .../devicetree/bindings/vendor-prefixes.txt        |   1 +
>>
>> This is not an i.MX change.  It should go through DT tree or we need
>> an ACK from DT maintainers.
>
> This was not part of the original patch and came in due to review
> comments. If it is not required I can remove it.
>
> If you suggest some other way to handle this, was exactly should I do?
> I'm not very experienced in kernel development.

Split the changes for vendor-prefixes.txt into a separate patch.
When you resend, run ./scripts/get_maintainer.pl on each of the
patches. It will give you people and lists to put into Cc (you can
leave out committers, make sure to add Maintainers and the
corresponding lists).

Once read by a DT maintainer, he will either pick it up or give an
Acked-by.

[...]
>>> diff --git a/arch/arm/boot/dts/imx6q-tbs2910.dts b/arch/arm/boot/dts/imx6q-tbs2910.dts
>>> new file mode 100644
>>> index 0000000..60a91ee
>>> --- /dev/null
>>> +++ b/arch/arm/boot/dts/imx6q-tbs2910.dts
>>> @@ -0,0 +1,415 @@
>>> +/*
>>> + * Copyright 2014 Soeren Moch <smoch@web.de>
>>> + * Copyright 2012 Freescale Semiconductor, Inc.
>>> + * Copyright 2011 Linaro Ltd.
>>> + *
>>> + * The code contained herein is licensed under the GNU General Public
>>> + * License. You may obtain a copy of the GNU General Public License
>>> + * Version 2 or later at the following locations:
>>> + *
>>> + * http://www.opensource.org/licenses/gpl-license.html
>>> + * http://www.gnu.org/copyleft/gpl.html
>>> + */
[...]
>>> +	rtc: ds1307 at 68 {
>>> +		compatible = "dallas,ds1307";
>>> +		reg = <0x68>;
>>> +	};
>>> +};
>>> +
>>> +&iomuxc {
>>
>> We do sort nodes alphabetically, but this one is a little special.
>> Moving it to the bottom of the file will slightly improve the
>> readability of the file.
>
> OK, I will move this node to the bottom.
>
> When talking about readability, in my original patch I used spaces to
> align the pin configuration values to preserve human readability while
> obeying the line length limits. Is it really desired to drop human
> readability in favor of avoiding spaces?

IIRC, there has been no strict 80-column rule for dts{i} files in the
past. In general, I'd prefer readability before 80-column rule if it is
just about some few chars. But that is a matter of taste, I guess.

BTW, the comment I made about indentation on your patch was about
<TAB><TAB><SPACES> instead of <TAB><TAB><TAB> in blue led node. I agree
that once you reached the property indent with TABs, use spaces to align
multi-line properties for readability.

Sebastian

  reply	other threads:[~2014-10-25 18:51 UTC|newest]

Thread overview: 12+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2014-10-18 12:27 [PATCH] arm: dts: add initial support for TBS2910 Matrix ARM mini PC Soeren Moch
2014-10-21  7:40 ` Sebastian Hesselbarth
2014-10-21 11:57   ` Sören Moch
2014-10-21 12:03     ` Sebastian Hesselbarth
2014-10-21 14:17       ` Sascha Hauer
2014-10-21 18:25         ` Sebastian Hesselbarth
2014-10-21 20:23 ` [PATCH v2] " Soeren Moch
2014-10-25 15:33   ` Shawn Guo
2014-10-25 17:50     ` Soeren Moch
2014-10-25 18:51       ` Sebastian Hesselbarth [this message]
2014-10-26  0:15       ` Shawn Guo
2014-10-26  0:55   ` Shawn Guo

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=544BF135.4030503@gmail.com \
    --to=sebastian.hesselbarth@gmail.com \
    --cc=linux-arm-kernel@lists.infradead.org \
    /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;
as well as URLs for NNTP newsgroup(s).