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

[Henning Schild]

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

> 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.
>