LKML Archive on lore.kernel.org
help / color / mirror / Atom feed
From: "Håvard Skinnemoen" <hskinnemoen@gmail.com>
To: Grant Likely <grant.likely@secretlab.ca>
Cc: Thomas Chou <thomas@wytron.com.tw>,
	Ben Dooks <ben-linux@fluff.org>,
	linux-kernel@vger.kernel.org, nios2-dev@sopc.et.ntust.edu.tw,
	devicetree-discuss@lists.ozlabs.org, linux-i2c@vger.kernel.org,
	Jean Delvare <khali@linux-fr.org>,
	Albert Herranz <albert_herranz@yahoo.es>
Subject: Re: [PATCH] i2c-gpio: add devicetree support
Date: Mon, 31 Jan 2011 14:35:31 -0800	[thread overview]
Message-ID: <AANLkTini7Hx6dbTAKJKJy=NnC+LFcQuHEsayxwJJ78-+@mail.gmail.com> (raw)
In-Reply-To: <AANLkTimS70uHqmv3jhr+7mPz4QFKXRXuFnWj8s3EpKsO@mail.gmail.com>

Hi,

On Mon, Jan 31, 2011 at 12:09 AM, Grant Likely
<grant.likely@secretlab.ca> wrote:
> On Sun, Jan 30, 2011 at 8:26 PM, Håvard Skinnemoen
> <hskinnemoen@gmail.com> wrote:
>> Hi,
>>
>> On Sun, Jan 30, 2011 at 7:56 AM, Thomas Chou <thomas@wytron.com.tw> wrote:
>>> From: Albert Herranz <albert_herranz@yahoo.es>
>>>
>>> This patch is based on an earlier patch from Albert Herranz,
>>> http://git.infradead.org/users/herraa1/gc-linux-2.6.git/commit/
>>> 9854eb78607c641ab5ae85bcbe3c9d14ac113733
>>
>> That commit has a single-line description of which I don't understand
>> a single word (unless "wii" is what I think it is, which seems
>> likely). Could you please explain how that commit relates to this
>> patch?
>
> The URL got wrapped.  Try this one (assuming my mailer doesn't wrap it):
>
> http://git.infradead.org/users/herraa1/gc-linux-2.6.git/commit/9854eb78607c641ab5ae85bcbe3c9d14ac113733

Ok, that seems to be a _bit_ more related, but not that much. I'd
really prefer a patch description which can stand on its own.

>>> The dts binding is modified as Grant suggested. The of probing
>>> is merged inline instead of a separate file. It uses the newer
>>> of gpio probe.
>>
>> It seems like a terrible idea to merge firmware-specific code into the
>> driver. Is there are reason why of-based platforms can't just pass the
>> data they need in pdata like everyone else?
>
> Overall Thomas is doing the right thing here.  The driver data has to
> be decoded *somewhere*, but since that code is definitely
> driver-specific (as opposed to platform, subsystem, or arch specific)
> putting it into the driver is absolutely the right thing to do.  Quite
> a few drivers now do exactly this.

Ok, I'm convinced that this is the right thing to do.

> It is however generally a wise practice to limit the of-support code
> to a hook in the drivers probe hook, and as you point out, care must
> be taken to make sure both CONFIG_OF and !CONFIG_OF kernel builds
> work.
>
>>
>> Not saying that it necessarily _is_ a terrible idea, but I think the
>> reasoning behind it needs to be included in the patch description.
>
> Nah, he doesn't really need to defend this since it is a well
> established pattern.  device tree support is in core code now (see
> of_node an of_match_table in include/linux/device.h), and other
> drivers do exactly this.

Well, perhaps you're right, but I still think the patch description is
a bit on the thin side. In particular, terms like "as Grant suggested"
isn't very helpful for people like me who don't know what you
suggested (even though I'm sure it was a good suggestion).

I think the patch lacks a good description of what's being changed and
why. The references may be nice to have as a supplement to that, but
describing things entirely in terms of some unknown e-mail discussion
that happened earlier is not very helpful for people who want to
figure out what's going on a couple of months or years from now.

Havard

  parent reply	other threads:[~2011-01-31 22:35 UTC|newest]

Thread overview: 25+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2011-01-30 15:56 Thomas Chou
2011-01-31  3:26 ` Håvard Skinnemoen
2011-01-31  8:09   ` Grant Likely
2011-01-31 13:55     ` Jon Loeliger
2011-01-31 15:25     ` [PATCH 1/3] of: define dummy of_get_property if not CONFIG_OF Thomas Chou
2011-01-31 22:10       ` Grant Likely
2011-01-31 15:25     ` [PATCH 2/3] of: of_gpiochip_add is needed only for gpiolib Thomas Chou
2011-01-31 22:16       ` Grant Likely
2011-01-31 15:25     ` [PATCH 3/3 v2] i2c-gpio: add devicetree support Thomas Chou
2011-01-31 21:14       ` [3/3,v2] " Milton Miller
2011-01-31 21:29         ` Grant Likely
2011-02-03  2:26           ` [PATCH] " Thomas Chou
2011-02-03  4:24             ` Håvard Skinnemoen
2011-02-03 18:07             ` Grant Likely
2011-02-10  2:29               ` [PATCH v4] " Thomas Chou
2011-02-14  2:30                 ` [PATCH v5] " Thomas Chou
2011-02-16  5:49                   ` Grant Likely
2011-02-16  6:13                     ` Thomas Chou
2011-02-23  1:12                   ` Ben Dooks
2011-02-23 13:18                     ` Thomas Chou
2011-02-24  4:00                     ` [PATCH v6] " Thomas Chou
2011-02-24 16:22                       ` Grant Likely
2011-02-25  2:04                         ` Thomas Chou
2011-01-31 22:35     ` Håvard Skinnemoen [this message]
2011-01-31 23:01       ` [PATCH] " Grant Likely

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='AANLkTini7Hx6dbTAKJKJy=NnC+LFcQuHEsayxwJJ78-+@mail.gmail.com' \
    --to=hskinnemoen@gmail.com \
    --cc=albert_herranz@yahoo.es \
    --cc=ben-linux@fluff.org \
    --cc=devicetree-discuss@lists.ozlabs.org \
    --cc=grant.likely@secretlab.ca \
    --cc=khali@linux-fr.org \
    --cc=linux-i2c@vger.kernel.org \
    --cc=linux-kernel@vger.kernel.org \
    --cc=nios2-dev@sopc.et.ntust.edu.tw \
    --cc=thomas@wytron.com.tw \
    --subject='Re: [PATCH] i2c-gpio: add devicetree support' \
    /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

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).