Message-ID: <20230627163344.GG10378@google.com>
Date:   Tue, 27 Jun 2023 17:33:44 +0100
From:   Lee Jones <lee@...nel.org>
To:     William Breathitt Gray <william.gray@...aro.org>
Cc:     Rob Herring <robh@...nel.org>,
        "Sahin, Okan" <Okan.Sahin@...log.com>,
        Mark Brown <broonie@...nel.org>,
        Krzysztof Kozlowski <krzysztof.kozlowski+dt@...aro.org>,
        Liam Girdwood <lgirdwood@...il.com>,
        Jonathan Cameron <jic23@...nel.org>,
        Lars-Peter Clausen <lars@...afoo.de>,
        Andy Shevchenko <andriy.shevchenko@...ux.intel.com>,
        Cosmin Tanislav <demonsingur@...il.com>,
        Stephen Boyd <sboyd@...nel.org>,
        Ulf Hansson <ulf.hansson@...aro.org>,
        Caleb Connolly <caleb.connolly@...aro.org>,
        Marcus Folkesson <marcus.folkesson@...il.com>,
        "Bolboaca, Ramona" <Ramona.Bolboaca@...log.com>,
        ChiYuan Huang <cy_huang@...htek.com>,
        "Tilki, Ibrahim" <Ibrahim.Tilki@...log.com>,
        Arnd Bergmann <arnd@...db.de>,
        Hugo Villeneuve <hvilleneuve@...onoff.com>,
        ChiaEn Wu <chiaen_wu@...htek.com>,
        Haibo Chen <haibo.chen@....com>,
        "devicetree@...r.kernel.org" <devicetree@...r.kernel.org>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
        "linux-iio@...r.kernel.org" <linux-iio@...r.kernel.org>
Subject: Re: [PATCH v7 5/5] mfd: max77541: Add ADI MAX77541/MAX77540 PMIC
 Support

On Tue, 27 Jun 2023, William Breathitt Gray wrote:

> On Tue, Jun 27, 2023 at 08:10:59AM -0600, Rob Herring wrote:
> > On Tue, Jun 27, 2023 at 7:56 AM Lee Jones <lee@...nel.org> wrote:
> > >
> > > On Mon, 26 Jun 2023, Rob Herring wrote:
> > >
> > > > On Wed, Jun 21, 2023 at 06:13:15PM +0100, Lee Jones wrote:
> > > > > On Tue, 13 Jun 2023, Sahin, Okan wrote:
> > > > >
> > > > > > >On Fri, Apr 21, 2023 at 08:39:38AM +0100, Lee Jones wrote:
> > > > > > >
> > > > > > >> I'll try anything once!
> > > > > > >
> > > > > > >> Fair warning, I think this is going to massively complicate things.
> > > > > > >
> > > > > > >> Either we're going to be left with a situation where child-driver
> > > > > > >> maintainers are scrabbling around looking for previous versions for the
> > > > > > >> MFD pull-request or contributors being forced to wait a full cycle for
> > > > > > >> their dependencies to arrive in the maintainer's base.
> > > > > > >
> > > > > > >If people are resending after the MFD has gone in they really ought to
> > > > > > >be including the pull request in the cover letter, with some combination
> > > > > > >of either referencing the mail or just saying "this depends on the
> > > > > > >signed tag at url+tag", the same way they would for any other dependency.
> > > > > > >
> > > > > > >I can't see how you applying stuff when you can slow things down TBH,
> > > > > > >the MFD bits will be applied faster and either people can pull in a
> > > > > > >shared tag or you can apply more commits on top of the existing core
> > > > > > >driver.
> > > > > > >
> > > > > > >> I'm not sure why simply providing your Ack when you're happy with the
> > > > > > >> driver and forgetting about the set until the pull-request arrives, like
> > > > > > >> we've been doing for nearly a decade now, isn't working for you anymore
> > > > > > >> but I'm mostly sure this method will be a regression.
> > > > > > >
> > > > > > >Like I said I've not been doing that, I've mostly been just applying the
> > > > > > >driver when it's ready.  This might not have been so visible to you
> > > > > > >since it means that the regulator driver doesn't appear in the series by
> > > > > > >the time the MFD settles down.  The whole "Acked-for-MFD" has always
> > > > > > >been a bit confusing TBH, it's not a normal ack ("go ahead and apply
> > > > > > >this, I'm fine with it") so it was never clear what the intention was.
> > > > > > >
> > > > > > >Before I started just applying the drivers there used to be constant
> > > > > > >problems with things like tags going missing (which some of the time is
> > > > > > >the submitter just not carrying them but can also be the result of some
> > > > > > >churn causing them to be deliberately dropped due to changes) or
> > > > > > >forgetting the series as you suggest and then not looking at some other
> > > > > > >very similarly named series that was also getting lots of versions after
> > > > > > >thinking it was one that had been reviewed already.  It was all very
> > > > > > >frustrating.  Not doing the tags until the dependencies have settled
> > > > > > >down means that if it's in my inbox it at least consistently needs some
> > > > > > >kind of attention and that the submitter didn't drop tags or anything so
> > > > > > >I know why there's no tag on it even though the version number is high,
> > > > > > >though it's not ideal either.
> > > > > >
> > > > > > Hi Mark and Lee,
> > > > > >
> > > > > > Is there anything that I need to do for this patch set. I have received reviewed
> > > > > > by tag for all of them so far.
> > > > >
> > > > > Since we are so late in the day, I'm going to just apply this for v6.5.
> > > > >
> > > > > The remainder can then be applied, friction free, for v6.6.
> > > >
> > > > Now we have undocmented bindings in use by the driver (as pointed out by
> > > > 'make dt_compatible_check').
> > > >
> > > > The whole series has all the acks/reviews needed for you to apply the
> > > > whole thing, so why not take the whole thing? Plus this series has been
> > > > sitting for 2 months. Not a great experience for submitters...
> > >
> > > Patches are missing Acked-by tags.
> > >
> > >   Reviewed-by != Acked-by
> > 
> > Reviewed-by > Acked-by
> > 
> > >
> > > I cannot merge other subsystem's patches without and Acked-by.
> > 
> > I (and Krzysztof) give one or the other. If I'm taking a patch, then
> > it's neither. I'm pretty sure Mark only gives Reviewed-by when he is
> > not taking something.
> > 
> > Rob
> 
> It does seem a bit ambiguous whether an "Acked-by" indicates a
> "Reviewed-by + acceptance of the changes" or just a brief look-over with
> acceptance of the changes. FWIW the documentation does use the word
> "reviewed" when describing Acked-by. [^1]
> 
> However, I would argue that a Reviewed-by has a implicit acceptance of
> the changes: why else provide a Reviewed-by line for the commit message
> if you fundamentally disagree with the changes being merged? So a

Where MFD is concerned the complexities are seldom 'whether' a patch
should be merged, but rather 'how' it should be merged.

In order to solve some of these issues in the past, I created a bespoke
tag for scenarios where I'd like to indicate that a submission had been
reviewed, but I also intended to take the patch via the MFD tree once
all of the other pieces were ready.  Despite using this tag for around a
decade, it did cause occasional confusion, even amongst maintainers I'd
been working with for the longest time, so I recently stopped using it
and replaced it with a standard Reviewed-by, to mean that it's reviewed
but permission was *not* given for someone else to merge it - since my
understanding, according to the documentation, is that an Acked-by is
required for that.

Recent discussions with other maintainers culminated in an agreement
that I would start only taking the MFD pieces and follow-up with a
pull-request for an immutable branch for them to pull from.  Since there
is no more time to create, test and submit a maintainer-maintainer
pull-request, I decided to merge this patch anyway, so the leaf drivers
can be applied in a couple of weeks, after the merge-window is closed.

Which brings us to where we are now!

Without different tag which doesn't exist today, I'm not entirely sure
how to solve this issue.  Ideas welcome.

> Reviewed-by given by a maintainer should be seen as approval for those
> changes to be merged.
> 
> William Breathitt Gray
> 
> [^1]: https://www.kernel.org/doc/html/latest/process/submitting-patches.html#when-to-use-acked-by-cc-and-co-developed-by

-- 
Lee Jones [李琼斯]

Powered by blists - more mailing lists