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

[Lee Jones <lee.jones@linaro.org>]

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?

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.

What are your reasons for submitting this via Staging?  Is it not
ready yet?  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?

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.

> Mauro Carvalho Chehab (41):
>   staging: spmi: hisi-spmi-controller: coding style fixup
>   staging: spmi: hisi-spmi-controller: fix it to probe successfully
>   staging: spmi: hisi-spmi-controller: fix a typo
>   staging: spmi: hisi-spmi-controller: adjust whitespaces at defines
>   staging: spmi: hisi-spmi-controller: use le32 macros where needed
>   staging: spmi: hisi-spmi-controller: add debug when values are
>     read/write
>   staging: spmi: hisi-spmi-controller: fix the dev_foo() logic
>   staging: spmi: hisi-spmi-controller: add it to the building system
>   staging: spmi: hisi-spmi-controller: do some code cleanups
>   staging: mfd: hi6421-spmi-pmic: get rid of unused code
>   staging: mfd: hi6421-spmi-pmic: deal with non-static functions
>   staging: mfd: hi6421-spmi-pmic: get rid of the static vars
>   staging: mfd: hi6421-spmi-pmic: cleanup hi6421-spmi-pmic.h header
>   staging: mfd: hi6421-spmi-pmic: change the binding logic
>   staging: mfd: hi6421-spmi-pmic: get rid of unused OF properties
>   staging: mfd: hi6421-spmi-pmic: cleanup OF properties
>   staging: mfd: hi6421-spmi-pmic: change namespace on its functions
>   staging: mfd: hi6421-spmi-pmic: fix some coding style issues
>   staging: mfd: hi6421-spmi-pmic: add it to the building system
>   staging: mfd: hi6421-spmi-pmic: cleanup the code
>   staging: regulator: hi6421v600-regulator: get rid of unused code
>   staging: regulator: hi6421v600-regulator: port it to upstream
>   staging: regulator: hi6421v600-regulator: coding style fixups
>   staging: regulator: hi6421v600-regulator: change the binding logic
>   staging: regulator: hi6421v600-regulator: cleanup struct
>     hisi_regulator
>   staging: regulator: hi6421v600-regulator: cleanup debug messages
>   staging: regulator: hi6421v600-regulator: use shorter names for OF
>     properties
>   staging: regulator: hi6421v600-regulator: better handle modes
>   staging: regulator: hi6421v600-regulator: change namespace
>   staging: regulator: hi6421v600-regulator: convert to use get/set
>     voltage_sel
>   staging: regulator: hi6421v600-regulator: don't use usleep_range for
>     off_on_delay
>   staging: regulator: hi6421v600-regulator: add a driver-specific debug
>     macro
>   staging: regulator: hi6421v600-regulator: initialize ramp_delay
>   staging: regulator: hi6421v600-regulator: cleanup DT settings
>   staging: regulator: hi6421v600-regulator: fix some coding style issues
>   staging: regulator: hi6421v600-regulator: add it to the building
>     system
>   staging: regulator: hi6421v600-regulator: code cleanup
>   staging: hikey9xx: add a TODO list
>   MAINTAINERS: add an entry for HiSilicon 6421v600 drivers
>   dt: document HiSilicon SPMI controller and mfd/regulator properties
>   dt: hisilicon: add support for the PMIC found on Hikey 970
> 
> Mayulong (3):
>   staging: spmi: add Hikey 970 SPMI controller driver
>   staging: mfd: add a PMIC driver for HiSilicon 6421 SPMI version
>   staging: regulator: add a regulator driver for HiSilicon 6421v600 SPMI
>     PMIC
> 
>  .../mfd/hisilicon,hi6421-spmi-pmic.yaml       | 182 +++++++
>  .../spmi/hisilicon,hisi-spmi-controller.yaml  |  54 ++
>  MAINTAINERS                                   |   6 +
>  .../boot/dts/hisilicon/hi3670-hikey970.dts    |  16 +-
>  .../boot/dts/hisilicon/hikey970-pmic.dtsi     | 200 ++++++++
>  drivers/staging/Kconfig                       |   2 +
>  drivers/staging/Makefile                      |   1 +
>  drivers/staging/hikey9xx/Kconfig              |  35 ++
>  drivers/staging/hikey9xx/Makefile             |   5 +
>  drivers/staging/hikey9xx/TODO                 |   5 +
>  drivers/staging/hikey9xx/hi6421-spmi-pmic.c   | 381 ++++++++++++++
>  .../staging/hikey9xx/hi6421v600-regulator.c   | 479 ++++++++++++++++++
>  .../staging/hikey9xx/hisi-spmi-controller.c   | 351 +++++++++++++
>  include/linux/mfd/hi6421-spmi-pmic.h          |  68 +++
>  14 files changed, 1773 insertions(+), 12 deletions(-)
>  create mode 100644 Documentation/devicetree/bindings/mfd/hisilicon,hi6421-spmi-pmic.yaml
>  create mode 100644 Documentation/devicetree/bindings/spmi/hisilicon,hisi-spmi-controller.yaml
>  create mode 100644 arch/arm64/boot/dts/hisilicon/hikey970-pmic.dtsi
>  create mode 100644 drivers/staging/hikey9xx/Kconfig
>  create mode 100644 drivers/staging/hikey9xx/Makefile
>  create mode 100644 drivers/staging/hikey9xx/TODO
>  create mode 100644 drivers/staging/hikey9xx/hi6421-spmi-pmic.c
>  create mode 100644 drivers/staging/hikey9xx/hi6421v600-regulator.c
>  create mode 100644 drivers/staging/hikey9xx/hisi-spmi-controller.c
>  create mode 100644 include/linux/mfd/hi6421-spmi-pmic.h
> 

-- 
Lee Jones [李琼斯]
Senior Technical Lead - Developer Services
Linaro.org │ Open source software for Arm SoCs
Follow Linaro: Facebook | Twitter | Blog