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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-ID: <7689997a-134b-6735-4a08-001ca62c46c8@redhat.com>
Date:   Wed, 4 Oct 2023 11:32:11 +0200
From:   Hans de Goede <hdegoede@...hat.com>
To:     Henry Shi <henryshi2018@...il.com>, hbshi69@...mail.com,
        tglx@...utronix.de, mingo@...hat.com, bp@...en8.de,
        dave.hansen@...ux.intel.com, x86@...nel.org, hpa@...or.com,
        markgross@...nel.org, jdelvare@...e.com, linux@...ck-us.net,
        linux-kernel@...r.kernel.org, platform-driver-x86@...r.kernel.org,
        linux-hwmon@...r.kernel.org
Cc:     hb_shi2003@...oo.com, henrys@...icom-usa.com, wenw@...icom-usa.com
Subject: Re: [PATCH] platform/x86: Add Silicom Platform Driver

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@...il.com>
> ---

<snip>

> change from patch v7 to v8 (current patch)
> ===========================================
> 
> changes suggested by Hans de Goede <hdegoede@...hat.com>:
> .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@...ux.intel.com>:
> .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