lkml.org 
[lkml]   [2023]   [Aug]   [8]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    From
    SubjectRE: [PATCH] Add Silicom Platform Driver
    Date
    See my comments below:

    -----Original Message-----
    From: Guenter Roeck <groeck7@gmail.com> On Behalf Of Guenter Roeck
    Sent: Friday, July 28, 2023 1:53 PM
    To: Christophe JAILLET <christophe.jaillet@wanadoo.fr>; Huibin Shi <henrys@silicom-usa.com>; Henry Shi <henryshi2018@gmail.com>; hbshi69@hotmail.com; tglx@linutronix.de; mingo@redhat.com; bp@alien8.de; dave.hansen@linux.intel.com; x86@kernel.org; hpa@zytor.com; hdegoede@redhat.com; markgross@kernel.org; jdelvare@suse.com; linux-kernel@vger.kernel.org; platform-driver-x86@vger.kernel.org; linux-hwmon@vger.kernel.org
    Cc: hb_shi2003@yahoo.com; Wen Wang <wenw@silicom-usa.com>
    Subject: Re: [PATCH] Add Silicom Platform Driver

    Caution: This is an external email. Please take care when clicking links or opening attachments.


    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@wanadoo.fr>
    >> Sent: Tuesday, July 25, 2023 5:03 PM
    >> To: Henry Shi <henryshi2018@gmail.com>; hbshi69@hotmail.com;
    >> tglx@linutronix.de; mingo@redhat.com; bp@alien8.de;
    >> dave.hansen@linux.intel.com; x86@kernel.org; hpa@zytor.com;
    >> hdegoede@redhat.com; markgross@kernel.org; jdelvare@suse.com;
    >> linux@roeck-us.net; linux-kernel@vger.kernel.org;
    >> platform-driver-x86@vger.kernel.org; linux-hwmon@vger.kernel.org
    >> Cc: hb_shi2003@yahoo.com; Huibin Shi <henrys@silicom-usa.com>; Wen
    >> Wang <wenw@silicom-usa.com>
    >> 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@gmail.com>
    >>> ---
    >>
    >> [...]
    >>
    >>> +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().

    Henry: Agree.

    Guenter

    \
     
     \ /
      Last update: 2023-08-08 22:19    [W:5.005 / U:0.652 seconds]
    ©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site