On 7/28/23 09:47, Christophe JAILLET wrote:
Le 28/07/2023 à 14:59, Huibin Shi a écrit :
Christophe,
Thanks for the comments. See my comments below.
Updated patch will be sent out later after review comments from other reviewers are addressed.
Henry
-----Original Message-----
From: Christophe JAILLET <christophe.jaillet@xxxxxxxxxx>
Sent: Tuesday, July 25, 2023 5:03 PM
To: Henry Shi <henryshi2018@xxxxxxxxx>; hbshi69@xxxxxxxxxxx; tglx@xxxxxxxxxxxxx; mingo@xxxxxxxxxx; bp@xxxxxxxxx; dave.hansen@xxxxxxxxxxxxxxx; x86@xxxxxxxxxx; hpa@xxxxxxxxx; hdegoede@xxxxxxxxxx; markgross@xxxxxxxxxx; jdelvare@xxxxxxxx; linux@xxxxxxxxxxxx; linux-kernel@xxxxxxxxxxxxxxx; platform-driver-x86@xxxxxxxxxxxxxxx; linux-hwmon@xxxxxxxxxxxxxxx
Cc: hb_shi2003@xxxxxxxxx; Huibin Shi <henrys@xxxxxxxxxxxxxxx>; Wen Wang <wenw@xxxxxxxxxxxxxxx>
Subject: Re: [PATCH] Add Silicom Platform Driver
Caution: This is an external email. Please take care when clicking links or opening attachments.
Le 18/07/2023 à 18:01, Henry Shi a écrit :
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>
---
[...]
+static int __init silicom_mc_leds_register(struct device *dev,
+ const struct led_classdev_mc
+*mc_leds) {
+ struct led_classdev_mc *led;
+ int i, err;
+
+ for (i = 0; mc_leds[i].led_cdev.name; i++) {
+ /* allocate and copy data from the init constansts */
+ led = devm_kzalloc(dev, sizeof(struct led_classdev_mc),
+ GFP_KERNEL);
sizeof(*led) is shorter.
Mostly a matter of taste.
Maybe even devm_kmemdup()?
Henry: thanks. Devm_kmemdup() API requires additional argument that is not necessary of this driver. I prefer devm_kzalloc for now.
CJ: The only additionnal parameter I can think of are the one of memcpy() ...
+ if (IS_ERR_OR_NULL(led)) {
if (!led)
is enough.
Henry: OK, changed
+ dev_err(dev, "Failed to alloc
+ led_classdev_mc[%d]: %ld\n", i, PTR_ERR(led));
This kind of message is useless and should be removed (checkpatch should warn about it)
Henry: OK, removed.
+ return -ENOMEM;
+ }
+ memcpy(led, &mc_leds[i], sizeof(*led));
... here.
devm_kzalloc() + this memcpy() could be done with only devm_kmemdup().
This is mostly a matter of taste.
I don't think that using (or not using) available API functions should
be a matter of taste, or the next step might be to re-implement memcpy().
Guenter
