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