linux-kernel - Re: [PATCH v2 2/3] leds: simatic-ipc-leds-gpio: split up into multiple drivers

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20230515131456.5aaabb4c@md1za8fc.ad001.siemens.net>
Date:   Mon, 15 May 2023 13:14:56 +0200
From:   Henning Schild <henning.schild@...mens.com>
To:     Andy Shevchenko <andriy.shevchenko@...ux.intel.com>
Cc:     Pavel Machek <pavel@....cz>, Lee Jones <lee@...nel.org>,
        Hans de Goede <hdegoede@...hat.com>,
        Mark Gross <markgross@...nel.org>,
        linux-kernel@...r.kernel.org, linux-leds@...r.kernel.org,
        platform-driver-x86@...r.kernel.org
Subject: Re: [PATCH v2 2/3] leds: simatic-ipc-leds-gpio: split up into
 multiple drivers

Am Wed, 1 Mar 2023 19:28:12 +0200
schrieb Andy Shevchenko <andriy.shevchenko@...ux.intel.com>:

> On Wed, Mar 01, 2023 at 06:02:14PM +0100, Henning Schild wrote:
> > In order to clearly describe the dependencies between the GPIO
> > controller drivers and the users the driver is split up into a core,
> > two drivers and a common header.  
> 
> ...
> 
> >  .../simple/simatic-ipc-leds-gpio-apollolake.c |  64 +++++++
> >  .../leds/simple/simatic-ipc-leds-gpio-core.c  | 103 ++++++++++++
> >  .../simple/simatic-ipc-leds-gpio-f7188x.c     |  64 +++++++
> >  drivers/leds/simple/simatic-ipc-leds-gpio.c   | 159
> > ------------------  
> 
> I'm wondering if you have used -M -C when creating this patch.
> 
> ...
> 
> > +#include <linux/gpio/machine.h>
> > +#include <linux/gpio/consumer.h>
> > +#include <linux/leds.h>
> > +#include <linux/module.h>
> > +#include <linux/platform_device.h>
> > +#include <linux/platform_data/x86/simatic-ipc-base.h>  
> 
> + Blank line?

Will be added in 3 files in v3.

> > +#include "simatic-ipc-leds-gpio.h"  
> 
> ...
> 
> > +	.table = {
> > +		GPIO_LOOKUP_IDX("apollolake-pinctrl.0", 52, NULL,
> > 0, GPIO_ACTIVE_LOW),
> > +		GPIO_LOOKUP_IDX("apollolake-pinctrl.0", 53, NULL,
> > 1, GPIO_ACTIVE_LOW),
> > +		GPIO_LOOKUP_IDX("apollolake-pinctrl.0", 57, NULL,
> > 2, GPIO_ACTIVE_LOW),
> > +		GPIO_LOOKUP_IDX("apollolake-pinctrl.0", 58, NULL,
> > 3, GPIO_ACTIVE_LOW),
> > +		GPIO_LOOKUP_IDX("apollolake-pinctrl.0", 60, NULL,
> > 4, GPIO_ACTIVE_LOW),
> > +		GPIO_LOOKUP_IDX("apollolake-pinctrl.0", 51, NULL,
> > 5, GPIO_ACTIVE_LOW),
> > +	},  
> 
> Shouldn't this have the terminator entry?
> 
> ...
> 
> > +static struct gpiod_lookup_table simatic_ipc_led_gpio_table_extra
> > = {
> > +	.dev_id = NULL,  
> 
> As per previous patch.

comment will be added in p1 and p2 and be in v3.

> > +	.table = {
> > +		GPIO_LOOKUP_IDX("apollolake-pinctrl.0", 56, NULL,
> > 6, GPIO_ACTIVE_LOW),
> > +		GPIO_LOOKUP_IDX("apollolake-pinctrl.0", 59, NULL,
> > 7, GPIO_ACTIVE_HIGH),
> > +	},  
> 
> As per above.
> 
> > +};  
> 
> ...
> 
> > +	.driver = {
> > +		.name = KBUILD_MODNAME,  
> 
> Strictly speaking this is an ABI (as something may instantiate the
> driver from the user space or elsewhere by this name. At the same
> time this may change with the file name change.
> 
> Personally I prefer explicit string literal here.
> 
> > +	},  
> 
> ...
> 
> > +  
> 
> Redundant blank line.

Will be removed in v3 in 3 files.

Henning

> > +module_platform_driver(simatic_ipc_led_gpio_apollolake_driver);  
> 
> ...
> 
> > +MODULE_ALIAS("platform:" KBUILD_MODNAME);  
> 
> Why? HAve you missed MODULE_DEVICE_TABLE()?
> 
> ...
> 
> > +++ b/drivers/leds/simple/simatic-ipc-leds-gpio-f7188x.c  
> 
> Similar comments as per above.
>