On Tue, Aug 29, 2023 at 10:17:47PM +0000, Huibin Shi wrote: > 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 No, that is wrong. It needs to be "depends on HWMON", or the hwmon code needs to be conditional and the dependency must be something like "depends on HWMON || HWMON=n". Guenter
