Date:   Wed, 1 Mar 2023 19:28:12 +0200
From:   Andy Shevchenko <andriy.shevchenko@linux.intel.com>
To:     Henning Schild <henning.schild@siemens.com>
Cc:     Pavel Machek <pavel@ucw.cz>, Lee Jones <lee@kernel.org>,
        Hans de Goede <hdegoede@redhat.com>,
        Mark Gross <markgross@kernel.org>,
        linux-kernel@vger.kernel.org, linux-leds@vger.kernel.org,
        platform-driver-x86@vger.kernel.org
Subject: Re: [PATCH v2 2/3] leds: simatic-ipc-leds-gpio: split up into
 multiple drivers
Message-ID: <Y/+LLICwh3T25IGv@smile.fi.intel.com>
References: <20230301170215.23382-1-henning.schild@siemens.com>
 <20230301170215.23382-3-henning.schild@siemens.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20230301170215.23382-3-henning.schild@siemens.com>
Organization: Intel Finland Oy - BIC 0357606-4 - Westendinkatu 7, 02160 Espoo
Precedence: bulk

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?

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

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

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

-- 
With Best Regards,
Andy Shevchenko