Re: [PATCH v3 5/6] mfd: ahc1ec0-hwmon: Add sub-device hwmon for Advantech embedded controller

[Lee Jones]

On Tue, 03 Nov 2020, AceLan Kao wrote:

> Hi Lee,
> 
> One question for you.
> 
> Lee Jones <lee.jones@xxxxxxxxxx> 於 2020年10月29日 週四 下午9:14寫道：
> >
> > On Thu, 29 Oct 2020, Shihlun Lin wrote:
> >
> > > This is one of sub-device driver for Advantech embedded controller
> > > AHC1EC0. This driver provides sysfs ABI for Advantech related
> > > applications to monitor the system status.
> > >
> > > Signed-off-by: Shihlun Lin <shihlun.lin@xxxxxxxxxxxxxxxx>
> > > Reported-by: kernel test robot <lkp@xxxxxxxxx>
> >
> > LKP reported that your driver needed upstreaming?
> >
> > I'm confused!
> >
> > > ---
> > >  drivers/mfd/Kconfig         |    8 +
> > >  drivers/mfd/Makefile        |    1 +
> > >  drivers/mfd/ahc1ec0-hwmon.c | 1514 +++++++++++++++++++++++++++++++++++
> >
> > This obviously belongs in drivers/hwmon.
> >
> > >  3 files changed, 1523 insertions(+)
> > >  create mode 100644 drivers/mfd/ahc1ec0-hwmon.c
> > >
> > > diff --git a/drivers/mfd/Kconfig b/drivers/mfd/Kconfig
> > > index 965bcafbe5b2..52ca49b211fc 100644
> > > --- a/drivers/mfd/Kconfig
> > > +++ b/drivers/mfd/Kconfig
> > > @@ -2175,5 +2175,13 @@ config MFD_AHC1EC0
> > >         provides expose functions for sub-devices to read/write the value
> > >         to embedded controller.
> > >
> > > +config MFD_AHC1EC0_HWMON
> > > +     tristate "Advantech EC Hareware Monitor Function"
> > > +     depends on MFD_AHC1EC0
> > > +     help
> > > +       This is sub-device for Advantech embedded controller AHC1EC0. This
> > > +       driver provides the sysfs attribues for applications to monitor
> > > +       the system status.
> > > +
> > >  endmenu
> > >  endif
> > > diff --git a/drivers/mfd/Makefile b/drivers/mfd/Makefile
> > > index 80a9a2bdc3ba..eb645db817b5 100644
> > > --- a/drivers/mfd/Makefile
> > > +++ b/drivers/mfd/Makefile
> > > @@ -269,3 +269,4 @@ obj-$(CONFIG_MFD_SIMPLE_MFD_I2C)  += simple-mfd-i2c.o
> > >  obj-$(CONFIG_MFD_INTEL_M10_BMC)   += intel-m10-bmc.o
> > >
> > >  obj-$(CONFIG_MFD_AHC1EC0)    += ahc1ec0.o
> > > +obj-$(CONFIG_MFD_AHC1EC0_HWMON)      += ahc1ec0-hwmon.o
> > > diff --git a/drivers/mfd/ahc1ec0-hwmon.c b/drivers/mfd/ahc1ec0-hwmon.c
> > > new file mode 100644
> > > index 000000000000..3e493b040b4a
> > > --- /dev/null
> > > +++ b/drivers/mfd/ahc1ec0-hwmon.c
> > > @@ -0,0 +1,1514 @@
> > > +// SPDX-License-Identifier: GPL-2.0-only
> > > +/*****************************************************************************
> > > + * Copyright (c) 2018, Advantech Automation Corp.
> >
> > You can't just lift a whole driver from downstream code and send it to
> > the mailing list as-is.
> Could you elaborate more about which part should be fixed, and how it
> should look like?

Frankly, no.

I would expect a contributor of a several thousand line patch-set to
at least read the documentation and adhere to it prior to anyone
conducting a full review.  Simply plucking a very old driver set from
a BSP kernel and dumping it on the upstream mailing list is not the
way to gain the right kind of attention.

At the very least (this is not an exhaustive list);

- all of the copyrights need to be up-to-date
- the drivers need to be located in their correct subsystems
- any internal references/comment headers need to be removed
- licences should be compatible with upstreaming
- hacks removed
- builds without errors or warnings
- checkpatch.pl passed
- submission should tick all of the boxes in SubmittingPatches
- removal of changelogs/versions
  - this is the first version as far as the kernel is concerned

> > > + * THIS IS AN UNPUBLISHED WORK CONTAINING CONFIDENTIAL AND PROPRIETARY
> > > + * INFORMATION WHICH IS THE PROPERTY OF ADVANTECH AUTOMATION CORP.
> > > + *
> > > + * ANY DISCLOSURE, USE, OR REPRODUCTION, WITHOUT WRITTEN AUTHORIZATION FROM
> > > + * ADVANTECH AUTOMATION CORP., IS STRICTLY PROHIBITED.
> > > + *****************************************************************************
> >
> > This warning is in contradiction to the licence you are proposing.
> >

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