Re: [PATCH] platform/x86: Add Silicom Platform Driver

[Hans de Goede]

Hi Henry,

On 10/3/23 22:54, Henry Shi wrote:
> platform/x86: Add Silicom Platform Driver
> 
> Add Silicom platform (silicom-platform) Linux driver for Swisscom
> Business Box (Swisscom BB) as well as Cordoba family products.
> 
> This platform driver provides support for various functions via
> the Linux LED framework, GPIO framework, Hardware Monitoring (HWMON)
> and device attributes.
> 
> Signed-off-by: Henry Shi <henryshi2018@xxxxxxxxx>
> ---

<snip>

> change from patch v7 to v8 (current patch)
> ===========================================
> 
> changes suggested by Hans de Goede <hdegoede@xxxxxxxxxx>:
> .Chnage commit message of this driver.
> .Adjust location of change log and signed-off-by.
> .Change "config SILICOM_PLATFORM" and help contents location,
> and put it to source "drivers/platform/x86/siemens/Kconfig".
> .Set editor tab to 8 and align the start of extra function
> parameters to directly after (. This should be applied for
> all function.
> .Do not manually create a sysfs dir and register sysfs attribute,
> instead define a platform_driver structure.
> .Move MODULE_DEVICE_TABLE(dmi, silicom_dmi_ids) directly after
> table declaration.
> .Using pr_info() instead of dev_info() in function
> silicom_platform_info_init().
> .Made dmi_check_system() check the first thing to do in
> silicom_platform_init().
> .Instead of separate platform_device creation + driver registration,
> switched to using platform_create_bundle().
> .Removed mutex_destroy(&mec_io_mutex).
> 
> 
> Changes suggested by Ilpo Järvinen <ilpo.jarvinen@xxxxxxxxxxxxxxx>:
> .Too many GENMASK() within to code itself, need put them to
> #define. Removed all GENMASK() in c functions.

Thank you for the new version. At a quick look this looks much
better now wrt the probe / init / sysfs attr registration, great work!

2 quick remarks:

1. Please add a Documentation/ABI/testing/sysfs-platform-silicom
   file to document driver specific the sysfs attributes of this driver
   See: Documentation/ABI/testing/sysfs-platform-wilco-ec for an example
   (and other Documentation/ABI/testing/sysfs-platform-* files)

2. :

> diff --git a/drivers/platform/x86/Makefile b/drivers/platform/x86/Makefile
> index b457de5abf7d..07efff8b24f7 100644
> --- a/drivers/platform/x86/Makefile
> +++ b/drivers/platform/x86/Makefile
> @@ -113,6 +113,7 @@ obj-$(CONFIG_SERIAL_MULTI_INSTANTIATE)	+= serial-multi-instantiate.o
>  obj-$(CONFIG_MLX_PLATFORM)		+= mlx-platform.o
>  obj-$(CONFIG_TOUCHSCREEN_DMI)		+= touchscreen_dmi.o
>  obj-$(CONFIG_WIRELESS_HOTKEY)		+= wireless-hotkey.o
> +obj-$(CONFIG_SILICOM_PLATFORM)		+= silicom-platform.o
>  obj-$(CONFIG_X86_ANDROID_TABLETS)	+= x86-android-tablets/
>  
>  # Intel uncore drivers

Like with the Kconfig part, please group this together with
the other industrial PC drivers we have at the end of
the Makefile:

# Siemens Simatic Industrial PCs
obj-$(CONFIG_SIEMENS_SIMATIC_IPC)       += siemens/

# Silicom
obj-$(CONFIG_SILICOM_PLATFORM)		+= silicom-platform.o

# Winmate
obj-$(CONFIG_WINMATE_FM07_KEYS)         += winmate-fm07-keys.o

# SEL
obj-$(CONFIG_SEL3350_PLATFORM)          += sel3350-platform.o


Other then that please keep working with Ilpo to polish the code
a bit more and then hopefully this will be ready for merging soon.

Regards,

Hans