Hi Guenter, Appreciate your feedback. Please see my comments below. Thanks Henry -----Original Message----- From: Guenter Roeck <groeck7@xxxxxxxxx> On Behalf Of Guenter Roeck Sent: Monday, August 28, 2023 8:41 PM To: Henry Shi <henryshi2018@xxxxxxxxx> Cc: hbshi69@xxxxxxxxxxx; tglx@xxxxxxxxxxxxx; mingo@xxxxxxxxxx; bp@xxxxxxxxx; dave.hansen@xxxxxxxxxxxxxxx; x86@xxxxxxxxxx; hpa@xxxxxxxxx; hdegoede@xxxxxxxxxx; markgross@xxxxxxxxxx; jdelvare@xxxxxxxx; linux-kernel@xxxxxxxxxxxxxxx; platform-driver-x86@xxxxxxxxxxxxxxx; linux-hwmon@xxxxxxxxxxxxxxx; hb_shi2003@xxxxxxxxx; Huibin Shi <henrys@xxxxxxxxxxxxxxx>; Wen Wang <wenw@xxxxxxxxxxxxxxx> Subject: Re: [PATCH v5] Add Silicom Platform Driver Caution: This is an external email. Please take care when clicking links or opening attachments. On Mon, Aug 28, 2023 at 05:26:22PM -0400, Henry Shi wrote: > The Silicom platform (silicom-platform) Linux driver for Swisscom > Business Box (Swisscom BB) as well as Cordoba family products is a > software solution designed to facilitate the efficient management and > control of devices through the integration of various Linux > frameworks. This platform driver provides seamless support for device > management via the Linux LED framework, GPIO framework, Hardware > Monitoring (HWMON), and device attributes. The Silicom platform > driver's compatibility with these Linux frameworks allows applications > to access and control Cordoba family devices using existing software > that is compatible with these frameworks. This compatibility > simplifies the development process, reduces dependencies on > proprietary solutions, and promotes interoperability with other > Linux-based systems and software. > > Signed-off-by: Henry Shi <henryshi2018@xxxxxxxxx> Again, my feedback is only for hwmon code. [ ... ] > + > +static int silicom_fan_control_read(struct device *dev, > + enum hwmon_sensor_types type, > + u32 attr, int channel, > + > +long *val) Excessively long continuation lines. That seeme to be the case for almost all continuation lines, except where it is too short. I'd suggest to run the patch through checkpatch --strict and fix what it reports. total: 0 errors, 9 warnings, 18 checks, 1077 lines checked is really a bit much. Henry: OK, I will fix those warnings. [ ... ] > + > + hwmon_dev = devm_hwmon_device_register_with_info(&device->dev, name, NULL, > + &silicom_chip_info, NULL); Did you try to compile this with CONFIG_HWMON=n or with CONFIG_HWMON=m and SILICOM_PLATFORM=y ? Henry: Great question. I did not try that before. When I force "CONFIG_HWMON=m and SILICOM_PLATFORM=y" and compile kernel, the build failed with message "silicom-platform.c:(.init.text+0x8ff5b): undefined reference to `devm_hwmon_device_register_with_info'". I tried following change in drivers/platform/x86/Kconfig: config SILICOM_PLATFORM tristate "Silicom Edge Networking device support" depends on DMI select LEDS_CLASS_MULTICOLOR select GPIOLIB select HWMON ----> added help This option enables support for the LEDs/GPIO/etc downstream of the embedded controller on Silicom "Cordoba" hardware and derivatives. If you have a Silicom network appliance, say Y or M here. After this change, kernel build can be completed. And "CONFIG_HWMON" was changed to "y" in .config file. Not sure whether there are any other better ways to resolve CONFIG dependence in when adding a new kernel driver. Guenter
