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
