Re: [PATCH 00/20] Move Intel platform drivers to intel directory to improve readability.

[Hans de Goede <hdegoede@redhat.com>]

Hi Kate,

On 8/10/21 11:58 AM, Kate Hsuan wrote:
> All the intel platform specific drivers are moved to intel/.
> It makes more clear file structure to improve the readability.
> 
> Kate Hsuan (20):
>   Move Intel hid of pdx86 to intel directory to improve readability.
>   Move Intel WMI driver of pdx86 to intel/ directory to improve
>     readability.
>   Move Intel bxtwc driver of pdx86 to intel/ directory to improve
>     readability.
>   Move Intel chtdc_ti driver of pdx86 to improve readability.
>   Move MRFLD power button driver of pdx86 to intel directory to improve
>     readability.
>   Move Intel PMC core of pdx86 to intel/ directory to improve
>     readability.
>   Move Intel PMT driver of pdx86 to intel/ to improve readability.
>   Move Intel P-Unit of pdx86 to intel/ directory to improve readability.
>   Move Intel SCU IPC of pdx86 to intel directory to increase
>     readability.
>   Move Intel SoC telemetry driver to intel directory to improve
>     readability.
>   Move Intel IPS driver of pdx86 to improve readability.
>   Move Intel RST driver of pdx86 to intel directory to improve
>     readability.
>   Move Intel smartconnect driver of pdx86 to intel/ directory to improve
>     readability.
>   Move Intel SST driver to intel/ directory to improve readability.
>   Move Intel turbo max 3 driver to intel/ directory to improve
>     readability.
>   Move Intel uncore freq driver to intel/ directory to improve
>     readability.
>   Move Intel int0002 vgpio driver to intel/ directory to inprove
>     readability.
>   Move Intel thermal driver for menlow platform driver to intel/
>     directory to improve readability.
>   Move OakTrail driver to the intel/ directory to improve readability.
>   Move Intel virtual botton driver to intel/ directory to improve
>     readability.

Thank you for doing this. I have a couple of remarks which I would
like to see addressed for version 2 of this series:

1. The commit messages are currently all one line, so basically
only a Subject and they are missing a Body describing the change
in more detail (as already pointed out by Mika).


2. Kernel patch subjects (the first line of the commit message)
should always be prefixed with the subsystem + component which they
are for, so instead of e.g.

"Move Intel hid of pdx86 to intel directory to improve readability."

you would use:

"platform/x86: intel-hid: Move to intel sub-directory to improve readability."

But that is a bit long; and normally the Subject line is just
a summary while the body explains things like the why, so this should
probably be shorted to:

"platform/x86: intel-hid: Move to intel sub-directory"

For the Subject, with the Body explaining what exactly is being changed
and why.


3. You are using new sub-directories for all drivers which you
are moving, but for drivers which are currently just 1 c-file, this 
means going from 1 c-file to 3 files (c-file + Kconfig + Makefile)
this seems a bit too much. I believe that it would be better for
the single file drivers (e.g. intel-hid.c, intel-vbtn.c) to be moved
directly under drivers/platform/x86/intel and for there Kconfig
and Makefile bits to be moved to the already existing Kconfig
and Makefile files there.


4. As Andy already mentioned when moving a file like
"intel_scu_ipc.c" to drivers/platform/x86/intel/scu then the
whole path becomes:

drivers/platform/x86/intel/scu/intel_scu_ipc.c

Which is quite long / quite a lot to type and repeats
intel + scu twice, so it would be better to drop the intel_scu
prefix from the filenames in this case resulting in:

drivers/platform/x86/intel/scu/ipc.c

in this example's case. This requires some extrea work:

4.1 You will need to adjust private includes to the new
filenames

4.2 If you simply adjust say this Makefile line:

obj-$(CONFIG_INTEL_SCU_PCI)             += intel_scu_pcidrv.o

to:

obj-$(CONFIG_INTEL_SCU_PCI)             += pcidrv.o

a pcidrv.ko will get build, but that is a very generic name
and then we end up with module-name clashes which are not
allowed.

So the Makefile needs to become a bit more complicated like this:

intel_scu_pcidrv-y			:= pcidrv.o
obj-$(CONFIG_INTEL_SCU_PCI)             += intel_scu_pcidrv.o

See for example:
https://git.kernel.org/pub/scm/linux/kernel/git/pdx86/platform-drivers-x86.git/tree/drivers/platform/x86/intel/pmt/Makefile?h=for-next


5. Some of the files which you are moving are mentioned in the
MAINTAINERS file. For each file which you are moving please check if
it is listed in the MAINTAINERS file, keeping wildcards in mind,
so search e.g. for intel_scu for the intel_scu_* move.

And if the file is listed then update the MAINTAINERS entry to
point to the new location.

Regards,

Hans