Re: [PATCH 00/44] SPMI patches needed by Hikey 970

[Mauro Carvalho Chehab]

Hi Lee,

Em Thu, 13 Aug 2020 08:58:27 +0100
Lee Jones <lee.jones@xxxxxxxxxx> escreveu:

> On Wed, 12 Aug 2020, Mauro Carvalho Chehab wrote:
> 
> > Hi Greg,
> > 
> > This patch series is part of a work I'm doing in order to be able to support
> > a HiKey 970 board that I recently got on my hands.
> > 
> > I received some freedback from Mark and from Jonathan on a first
> > attempt I made to upstream this.
> > 
> > I'm opting to submit it via staging, because I had to start from the
> > patch that originally added this driver on a 4.9 Kernel tree:
> > 
> > 	https://github.com/96boards-hikey/linux/tree/hikey970-v4.9
> > 
> > In order to preserve the original SOB from the driver's author.
> > 
> > The patches following it are on the standard way: one patch per
> > logical change.
> > 
> > This is part of a bigger work whose goal is to have upstream support
> > for USB and DRM/KMS on such boards. 
> > 
> > I suspect that, maybe except for the DT part, those 3 specific drivers
> > are more or less ready to be moved from staging, but the other
> > drivers that are also part of this attempt aren't ready. Specially the
> > DRM driver has some bugs that came from the OOT version.
> > 
> > So, my current plan is to submit those drivers to staging for 5.9
> > and move the ones that are ok out of staging on Kernel 5.10.  
> 
> What a mess.  This is no way to upstream a new driver.
> 
> Firstly, could you please add versioning to your submissions.  I know
> this at least version 2.  Were there previous submissions?  Is this
> the latest?

Yeah, that's the second attempt. The first one was:

	https://lore.kernel.org/lkml/176043f329dfa9889f014feec04e7e1553077873.1597160086.git.mchehab+huawei@xxxxxxxxxx/T/#u

I was in doubt about adding a v2 in this specific case or not, 
since I ended submitting it to the staging tree.

> Secondly and more importantly, you have submitted what looks like a
> new driver (bearing in mind that I'm only concerning myself with the
> MFD related changes), then in the same submission you are adding and
> removing large chunks.  Please just submit the new driver, on its own
> as a single patch, complete with its associated Makefile and Kconfig
> changes.

I can't do like that because I'm not the author of the original patch that
added the driver.

The original patch came from the 96board's android-kernel based 4.9 tree:

	https://github.com/96boards-hikey/linux/tree/hikey970-v4.9

> What are your reasons for submitting this via Staging? 

The main reason is to preserve both the patch authorship and its
history.

After the original patch, I wrote several incremental changes cleaning
up the original driver and stripping parts of it that aren't needed.

By preserving the history, if someone wants to restore some removed
functionality, it is just a matter of reverting a patch.

For example, the original driver had its own sysfs interface for
debugging the regulator driver. 

This is not needed for it to work. Also, the right interface for such 
things is via configfs. Yet, someone could think on restoring such 
feat and start from the existing code, instead of coming with 
something else from scratch.

> Is it not ready yet? 

From my side, I believe that, after my changes, the code now meets
upstream requirements, maybe except for DT (and the parsing code).
There are a few things at the DT properties on this driver that could 
be named on a different (more standard way). 

Yet, I'm not a regular contributor for mfd/regulator/spmi. So,
I may have missed something.

> Are the resultant components not at a high enough level of
> quality or enablement to go straight into the subsystems, which is
> more typical?  From an MFD perspective, I would be reviewing the
> driver as a whole when (if) it moves from Staging into MFD anyway, so
> why are you jumping through hoops with this additional, seemingly
> superfluous step?

I'm OK if this gets reviewed by MFD people only after moving it out of
staging. Assuming that this would be merged for Kernel 5.10, I'll
likely send a patch moving it out of staging for 5.11. Then,
you can do a comprehensive review.

> Finally, the subject of authorship is often a contentious one, but
> this is a problem you need to work out with the original author, not
> something that should require special handing by upstream.  You have a
> couple of choices, but bear in mind that upstreaming a non-suitable
> driver then bringing it up to standard is not one of them.
> 
> 1. Keep the original author's authorship and SoB, make your changes
>    and get them to review to ensure they are still happy about being
>    associated with the resultant code.  Ensure you mention all of the
>    changes you make in the commit message and follow-up by adding your
>    own SoB.
> 
> 2. This is the contentious bit.  If you've made enough changes, there
>    is an argument for you to adopt authorship.  You should discuss
>    with the original author whether they are happy for you to retain
>    their SoB.  My suggestion is always try to keep the SoB as a bare
>    minimum to preserve patch history and out of pure courtesy.

It is not only the above. Both the original author and anyone
touching the code should comply with applicable internal policies.

From my experience, dealing with such things takes a lot more of time
then coding, as it require talking with legal departments on different
continents, and with developers and with their bosses in order to be
able to do things like that. 

This can also be a very frustrating process. During almost 20 years of
being the media maintainer, I've seen several cases where trying to
enforce a folded initial patch caused devs to receive NACKS, preventing 
them so submit otherwise good stuff.

So, at the media subsystem, I'm perfectly fine if someone starts from 
the original OOT driver, preserving its original authorships. We're
also dealing there with the patches sent to drivers/staging/media.

I'm not saying that other subsystem maintainers should do the same.
Dealing with staging is time consuming, and I completely understand
that most maintainers prefer to stay out of it ;-)

- 

Since when staging tree started, if someone has to start from the
original patch, such things can be merged at staging. Then,
incremental patches are applied at the top until it reaches what's
required to be promoted.

That's said, there's no hush to have those drivers out of staging.
My end goal is to have DRM/KMS and USB support for Hikey 970. 

The patchsets I have so far are at:

	https://github.com/mchehab/linux/commits/hikey970/to_upstream-2.0-v1.1

(this branch has the v1 of my patchset)

Porting this driver is part of such effort. While this driver is
on a good situation, the other ones may require some time to
mature.

The DRM/KMS driver for example, is not ready to be merged outside 
staging, as it carries several bugs that came from the original
driver and are present at the official tree at 96boards. For example,
there is a a very dirty hack that enforces the HDMI chipset to
only work with a limited set of resolutions that are known to work:

	https://github.com/96boards-hikey/linux/blob/hikey970-v4.9/drivers/gpu/drm/hisilicon/kirin9xx/hdmi/adv7535.c#L869

It also has problems reading the frequencies via EDID interface.
Due to that, the driver fakes an EDID table:

	https://github.com/96boards-hikey/linux/blob/hikey970-v4.9/drivers/gpu/drm/hisilicon/kirin9xx/hdmi/adv7535.c#L463

It sounds to me that some clocks are not properly set for a random
resolution, but fixing it is not trivial and requires deep knowledge
about how the display registers should be tuned to better support
resolutions. The current settings cause underflows with 1080p,
which in turn makes the display driver to (silently) stop working.

So, in summary, I believe that some drivers from my port will
require being at staging for a while. While I was planning to
do that on my next patch submission, placing the PM drivers
there won't make much difference from my side, as I'll need to
be submitting patches anyway moving drivers out of staging as
they become ready.

Thanks,
Mauro