LKML Archive on lore.kernel.org
help / color / mirror / Atom feed
From: Johan Hovold <johan@kernel.org>
To: Alex Elder <elder@linaro.org>
Cc: Matthew Wilcox <willy@infradead.org>,
	Alex Elder <elder@kernel.org>,
	linux-staging@lists.linux.dev, linux-kernel@vger.kernel.org,
	greybus-dev@lists.linaro.org,
	"Fabio M. De Francesco" <fmdefrancesco@gmail.com>
Subject: Re: [greybus-dev] [PATCH v4] staging: greybus: Convert uart.c from IDR to XArray
Date: Tue, 31 Aug 2021 13:51:14 +0200	[thread overview]
Message-ID: <YS4Xsn9YNOzruq2b@hovoldconsulting.com> (raw)
In-Reply-To: <51c57831-71bf-92f8-2b20-3e542160a8bf@linaro.org>

On Tue, Aug 31, 2021 at 05:42:20AM -0500, Alex Elder wrote:
> On 8/31/21 3:07 AM, Johan Hovold wrote:
> > On Mon, Aug 30, 2021 at 08:20:25AM -0500, Alex Elder wrote:
> > 
> >> I have been offering review feedback on this patch for three reasons:
> >>
> >> - First, because I think the intended change does no real harm to the
> >>    Greybus code, and in a small way actually simplifies it.
> > 
> > You leave out that we've already seen three versions of the patch that
> > broke things in various ways and that there was still work to be done
> > with respect to the commit message and verifying the locking. That's all
> > real costs that someone needs to bear.
> 
> This is true.  But it's separate from my reason for doing it,
> and unrelated to the suggested change.

I was perhaps reading the "no harm" bit too literally, but I'd say it
very much applies to the suggested change (which was the example I
used).

> >> - Because I wanted to encourage Fabio's efforts to be part of the
> >>    Linux contributor community.
> > 
> > Helping new contributers that for example have run into a bug or need
> > some assistance adding a new feature that they themselves have use for
> > is one thing.
> > 
> > I'm not so sure we're helping either newcomers or Linux long term by
> > inventing work for an already strained community however.
> > 
> > [ This is more of a general comment and of course in no way a critique
> >    against Fabio or a claim that the XArray conversions are pointless. ]
> 
> Yes, yours is a general comment.  But I would characterize
> this as Fabio "scratching an itch" rather than "inventing
> new work."

Just to clarify again, my comment was in no way directed at Fabio or
not necessarily even at the XArray conversions if it indeed means that
IDR/IDA can be removed.

> The strained community needs more helpers, and
> they don't suddenly appear fully-formed; they need to be
> cultivated.  There's a balance to strike between "I see
> you need a little guidance here" and "go away and come
> back when you know how to do it right."

And here's where I think the invented work bit really comes in. I have
no problem helping someone fix a real problem or add a feature they
need, but spending hours on reviewing changes that in the end no one
needs I find a bit frustrating. My guess is that the former is also more
likely to generate long-term contributors than teaching people C on
made-up tasks or asking them to silence checkpatch.pl indentation
warnings.

> In any case, I don't disagree with your general point, but
> we seem to view this particular instance differently.

Perhaps I shouldn't have brought up the general issue in this case. If
there was a general consensus that IDR was going away and some
precedence outside of staging that could be used as a model, then this
change would be fine.

Johan

  reply	other threads:[~2021-08-31 11:51 UTC|newest]

Thread overview: 19+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2021-08-29  9:22 Fabio M. De Francesco
2021-08-30  9:12 ` Johan Hovold
2021-08-30 11:10   ` Fabio M. De Francesco
2021-08-30 11:52     ` Johan Hovold
2021-08-30 12:16       ` Matthew Wilcox
2021-08-30 12:33         ` Johan Hovold
2021-08-30 13:16           ` Fabio M. De Francesco
2021-08-30 13:20           ` [greybus-dev] " Alex Elder
2021-08-31  8:07             ` Johan Hovold
2021-08-31 10:42               ` Alex Elder
2021-08-31 11:51                 ` Johan Hovold [this message]
2021-08-31 11:50               ` Fabio M. De Francesco
2021-08-31 12:18                 ` Johan Hovold
2021-09-01 12:09                 ` Alex Elder
2021-09-01 13:56                   ` Fabio M. De Francesco
2021-09-01 14:29                     ` Matthew Wilcox
2021-09-01 15:39                       ` Fabio M. De Francesco
2021-08-30 13:31           ` Matthew Wilcox
2021-08-31  8:16             ` Johan Hovold

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=YS4Xsn9YNOzruq2b@hovoldconsulting.com \
    --to=johan@kernel.org \
    --cc=elder@kernel.org \
    --cc=elder@linaro.org \
    --cc=fmdefrancesco@gmail.com \
    --cc=greybus-dev@lists.linaro.org \
    --cc=linux-kernel@vger.kernel.org \
    --cc=linux-staging@lists.linux.dev \
    --cc=willy@infradead.org \
    --subject='Re: [greybus-dev] [PATCH v4] staging: greybus: Convert uart.c from IDR to XArray' \
    /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).