This series mainly splits the one GPIO driver into two. The split allows
to clearly model runtime and compile time dependencies on the GPIO chip
drivers.
p1 is kind of not too related to that split but also prepares for more
GPIO based drivers to come.
p2 takes the driver we had and puts most of its content into a header,
to be included by two drivers.
p3 deals with more fine-grained configuration posibilities and compile
time dependencies.
It is based on
[PATCH v4] leds: simatic-ipc-leds-gpio: make sure we have the GPIO providing driver
Henning Schild (3):
leds: simatic-ipc-leds-gpio: move two extra gpio pins into another
table
leds: simatic-ipc-leds-gpio: split up into multiple drivers
leds: simatic-ipc-leds-gpio: introduce more Kconfig switches
drivers/leds/simple/Kconfig | 31 ++++++++-
drivers/leds/simple/Makefile | 5 +-
.../simple/simatic-ipc-leds-gpio-apollolake.c | 34 ++++++++++
.../simple/simatic-ipc-leds-gpio-f7188x.c | 34 ++++++++++
...pc-leds-gpio.c => simatic-ipc-leds-gpio.h} | 67 ++++++-------------
drivers/platform/x86/simatic-ipc.c | 7 +-
6 files changed, 123 insertions(+), 55 deletions(-)
create mode 100644 drivers/leds/simple/simatic-ipc-leds-gpio-apollolake.c
create mode 100644 drivers/leds/simple/simatic-ipc-leds-gpio-f7188x.c
rename drivers/leds/simple/{simatic-ipc-leds-gpio.c => simatic-ipc-leds-gpio.h} (51%)
--
2.39.2
In order to clearly describe the dependencies between the gpio
controller drivers and the users the driver is split up into two and one
common header.
Signed-off-by: Henning Schild <[email protected]>
---
drivers/leds/simple/Makefile | 3 +-
.../simple/simatic-ipc-leds-gpio-apollolake.c | 34 ++++
.../simple/simatic-ipc-leds-gpio-f7188x.c | 34 ++++
drivers/leds/simple/simatic-ipc-leds-gpio.c | 159 ------------------
drivers/leds/simple/simatic-ipc-leds-gpio.h | 112 ++++++++++++
drivers/platform/x86/simatic-ipc.c | 7 +-
6 files changed, 186 insertions(+), 163 deletions(-)
create mode 100644 drivers/leds/simple/simatic-ipc-leds-gpio-apollolake.c
create mode 100644 drivers/leds/simple/simatic-ipc-leds-gpio-f7188x.c
delete mode 100644 drivers/leds/simple/simatic-ipc-leds-gpio.c
create mode 100644 drivers/leds/simple/simatic-ipc-leds-gpio.h
diff --git a/drivers/leds/simple/Makefile b/drivers/leds/simple/Makefile
index 1c7ef5e1324b..21853956c1ea 100644
--- a/drivers/leds/simple/Makefile
+++ b/drivers/leds/simple/Makefile
@@ -1,3 +1,4 @@
# SPDX-License-Identifier: GPL-2.0
obj-$(CONFIG_LEDS_SIEMENS_SIMATIC_IPC) += simatic-ipc-leds.o
-obj-$(CONFIG_LEDS_SIEMENS_SIMATIC_IPC) += simatic-ipc-leds-gpio.o
+obj-$(CONFIG_LEDS_SIEMENS_SIMATIC_IPC) += simatic-ipc-leds-gpio-apollolake.o
+obj-$(CONFIG_LEDS_SIEMENS_SIMATIC_IPC) += simatic-ipc-leds-gpio-f7188x.o
diff --git a/drivers/leds/simple/simatic-ipc-leds-gpio-apollolake.c b/drivers/leds/simple/simatic-ipc-leds-gpio-apollolake.c
new file mode 100644
index 000000000000..b5f2448e0aa4
--- /dev/null
+++ b/drivers/leds/simple/simatic-ipc-leds-gpio-apollolake.c
@@ -0,0 +1,34 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * Siemens SIMATIC IPC driver for GPIO based LEDs
+ *
+ * Copyright (c) Siemens AG, 2023
+ *
+ * Authors:
+ * Henning Schild <[email protected]>
+ */
+
+#include "simatic-ipc-leds-gpio.h"
+
+static struct gpiod_lookup_table simatic_ipc_led_gpio_table = {
+ .dev_id = "leds-gpio",
+ .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),
+ },
+};
+
+static struct gpiod_lookup_table simatic_ipc_led_gpio_table_extra = {
+ .dev_id = NULL,
+ .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),
+ },
+};
+
+MODULE_SOFTDEP("pre: platform:leds-gpio platform:apollolake-pinctrl");
+MODULE_AUTHOR("Henning Schild <[email protected]>");
diff --git a/drivers/leds/simple/simatic-ipc-leds-gpio-f7188x.c b/drivers/leds/simple/simatic-ipc-leds-gpio-f7188x.c
new file mode 100644
index 000000000000..bc6eeb47ce91
--- /dev/null
+++ b/drivers/leds/simple/simatic-ipc-leds-gpio-f7188x.c
@@ -0,0 +1,34 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * Siemens SIMATIC IPC driver for GPIO based LEDs
+ *
+ * Copyright (c) Siemens AG, 2023
+ *
+ * Authors:
+ * Henning Schild <[email protected]>
+ */
+
+#include "simatic-ipc-leds-gpio.h"
+
+static struct gpiod_lookup_table simatic_ipc_led_gpio_table = {
+ .dev_id = "leds-gpio",
+ .table = {
+ GPIO_LOOKUP_IDX("gpio-f7188x-2", 0, NULL, 0, GPIO_ACTIVE_LOW),
+ GPIO_LOOKUP_IDX("gpio-f7188x-2", 1, NULL, 1, GPIO_ACTIVE_LOW),
+ GPIO_LOOKUP_IDX("gpio-f7188x-2", 2, NULL, 2, GPIO_ACTIVE_LOW),
+ GPIO_LOOKUP_IDX("gpio-f7188x-2", 3, NULL, 3, GPIO_ACTIVE_LOW),
+ GPIO_LOOKUP_IDX("gpio-f7188x-2", 4, NULL, 4, GPIO_ACTIVE_LOW),
+ GPIO_LOOKUP_IDX("gpio-f7188x-2", 5, NULL, 5, GPIO_ACTIVE_LOW),
+ },
+};
+
+static struct gpiod_lookup_table simatic_ipc_led_gpio_table_extra = {
+ .dev_id = NULL,
+ .table = {
+ GPIO_LOOKUP_IDX("gpio-f7188x-3", 6, NULL, 6, GPIO_ACTIVE_HIGH),
+ GPIO_LOOKUP_IDX("gpio-f7188x-3", 7, NULL, 7, GPIO_ACTIVE_HIGH),
+ },
+};
+
+MODULE_SOFTDEP("pre: platform:leds-gpio gpio_f7188x");
+MODULE_AUTHOR("Henning Schild <[email protected]>");
diff --git a/drivers/leds/simple/simatic-ipc-leds-gpio.c b/drivers/leds/simple/simatic-ipc-leds-gpio.c
deleted file mode 100644
index ba0f24638af5..000000000000
--- a/drivers/leds/simple/simatic-ipc-leds-gpio.c
+++ /dev/null
@@ -1,159 +0,0 @@
-// SPDX-License-Identifier: GPL-2.0
-/*
- * Siemens SIMATIC IPC driver for GPIO based LEDs
- *
- * Copyright (c) Siemens AG, 2022
- *
- * Authors:
- * Henning Schild <[email protected]>
- */
-
-#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>
-
-static struct gpiod_lookup_table *simatic_ipc_led_gpio_table;
-static struct gpiod_lookup_table *simatic_ipc_led_gpio_table_extra;
-
-static struct gpiod_lookup_table simatic_ipc_led_gpio_table_127e = {
- .dev_id = "leds-gpio",
- .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),
- },
-};
-
-static struct gpiod_lookup_table simatic_ipc_led_gpio_table_127e_extra = {
- .dev_id = NULL,
- .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),
- },
-};
-
-static struct gpiod_lookup_table simatic_ipc_led_gpio_table_227g = {
- .dev_id = "leds-gpio",
- .table = {
- GPIO_LOOKUP_IDX("gpio-f7188x-2", 0, NULL, 0, GPIO_ACTIVE_LOW),
- GPIO_LOOKUP_IDX("gpio-f7188x-2", 1, NULL, 1, GPIO_ACTIVE_LOW),
- GPIO_LOOKUP_IDX("gpio-f7188x-2", 2, NULL, 2, GPIO_ACTIVE_LOW),
- GPIO_LOOKUP_IDX("gpio-f7188x-2", 3, NULL, 3, GPIO_ACTIVE_LOW),
- GPIO_LOOKUP_IDX("gpio-f7188x-2", 4, NULL, 4, GPIO_ACTIVE_LOW),
- GPIO_LOOKUP_IDX("gpio-f7188x-2", 5, NULL, 5, GPIO_ACTIVE_LOW),
- },
-};
-
-static struct gpiod_lookup_table simatic_ipc_led_gpio_table_227g_extra = {
- .dev_id = NULL,
- .table = {
- GPIO_LOOKUP_IDX("gpio-f7188x-3", 6, NULL, 6, GPIO_ACTIVE_HIGH),
- GPIO_LOOKUP_IDX("gpio-f7188x-3", 7, NULL, 7, GPIO_ACTIVE_HIGH),
- },
-};
-
-static const struct gpio_led simatic_ipc_gpio_leds[] = {
- { .name = "red:" LED_FUNCTION_STATUS "-1" },
- { .name = "green:" LED_FUNCTION_STATUS "-1" },
- { .name = "red:" LED_FUNCTION_STATUS "-2" },
- { .name = "green:" LED_FUNCTION_STATUS "-2" },
- { .name = "red:" LED_FUNCTION_STATUS "-3" },
- { .name = "green:" LED_FUNCTION_STATUS "-3" },
-};
-
-static const struct gpio_led_platform_data simatic_ipc_gpio_leds_pdata = {
- .num_leds = ARRAY_SIZE(simatic_ipc_gpio_leds),
- .leds = simatic_ipc_gpio_leds,
-};
-
-static struct platform_device *simatic_leds_pdev;
-
-static int simatic_ipc_leds_gpio_remove(struct platform_device *pdev)
-{
- gpiod_remove_lookup_table(simatic_ipc_led_gpio_table);
- gpiod_remove_lookup_table(simatic_ipc_led_gpio_table_extra);
- platform_device_unregister(simatic_leds_pdev);
-
- return 0;
-}
-
-static int simatic_ipc_leds_gpio_probe(struct platform_device *pdev)
-{
- const struct simatic_ipc_platform *plat = pdev->dev.platform_data;
- struct device *dev = &pdev->dev;
- struct gpio_desc *gpiod;
- int err;
-
- switch (plat->devmode) {
- case SIMATIC_IPC_DEVICE_127E:
- if (!IS_ENABLED(CONFIG_PINCTRL_BROXTON))
- return -ENODEV;
- simatic_ipc_led_gpio_table = &simatic_ipc_led_gpio_table_127e;
- simatic_ipc_led_gpio_table_extra = &simatic_ipc_led_gpio_table_127e_extra;
- break;
- case SIMATIC_IPC_DEVICE_227G:
- if (!IS_ENABLED(CONFIG_GPIO_F7188X))
- return -ENODEV;
- request_module("gpio-f7188x");
- simatic_ipc_led_gpio_table = &simatic_ipc_led_gpio_table_227g;
- simatic_ipc_led_gpio_table_extra = &simatic_ipc_led_gpio_table_227g_extra;
- break;
- default:
- return -ENODEV;
- }
-
- gpiod_add_lookup_table(simatic_ipc_led_gpio_table);
- simatic_leds_pdev = platform_device_register_resndata(NULL,
- "leds-gpio", PLATFORM_DEVID_NONE, NULL, 0,
- &simatic_ipc_gpio_leds_pdata,
- sizeof(simatic_ipc_gpio_leds_pdata));
- if (IS_ERR(simatic_leds_pdev)) {
- err = PTR_ERR(simatic_leds_pdev);
- goto out;
- }
-
- simatic_ipc_led_gpio_table_extra->dev_id = dev_name(dev);
- gpiod_add_lookup_table(simatic_ipc_led_gpio_table_extra);
-
- /* PM_BIOS_BOOT_N */
- gpiod = gpiod_get_index(dev, NULL, 6, GPIOD_OUT_LOW);
- if (IS_ERR(gpiod)) {
- err = PTR_ERR(gpiod);
- goto out;
- }
- gpiod_put(gpiod);
-
- /* PM_WDT_OUT */
- gpiod = gpiod_get_index(dev, NULL, 7, GPIOD_OUT_LOW);
- if (IS_ERR(gpiod)) {
- err = PTR_ERR(gpiod);
- goto out;
- }
- gpiod_put(gpiod);
-
- return 0;
-out:
- simatic_ipc_leds_gpio_remove(pdev);
-
- return err;
-}
-
-static struct platform_driver simatic_ipc_led_gpio_driver = {
- .probe = simatic_ipc_leds_gpio_probe,
- .remove = simatic_ipc_leds_gpio_remove,
- .driver = {
- .name = KBUILD_MODNAME,
- }
-};
-module_platform_driver(simatic_ipc_led_gpio_driver);
-
-MODULE_LICENSE("GPL v2");
-MODULE_ALIAS("platform:" KBUILD_MODNAME);
-MODULE_SOFTDEP("pre: platform:leds-gpio");
-MODULE_AUTHOR("Henning Schild <[email protected]>");
diff --git a/drivers/leds/simple/simatic-ipc-leds-gpio.h b/drivers/leds/simple/simatic-ipc-leds-gpio.h
new file mode 100644
index 000000000000..c474836c7a59
--- /dev/null
+++ b/drivers/leds/simple/simatic-ipc-leds-gpio.h
@@ -0,0 +1,112 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * Siemens SIMATIC IPC driver for GPIO based LEDs
+ *
+ * Copyright (c) Siemens AG, 2023
+ *
+ * Authors:
+ * Henning Schild <[email protected]>
+ */
+
+#ifndef __DRIVERS_LEDS_SIMPLE_SIMATIC_IPC_LEDS_GPIO
+#define __DRIVERS_LEDS_SIMPLE_SIMATIC_IPC_LEDS_GPIO
+
+#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>
+
+static struct platform_device *simatic_leds_pdev;
+static struct gpiod_lookup_table simatic_ipc_led_gpio_table;
+static struct gpiod_lookup_table simatic_ipc_led_gpio_table_extra;
+
+static const struct gpio_led simatic_ipc_gpio_leds[] = {
+ { .name = "red:" LED_FUNCTION_STATUS "-1" },
+ { .name = "green:" LED_FUNCTION_STATUS "-1" },
+ { .name = "red:" LED_FUNCTION_STATUS "-2" },
+ { .name = "green:" LED_FUNCTION_STATUS "-2" },
+ { .name = "red:" LED_FUNCTION_STATUS "-3" },
+ { .name = "green:" LED_FUNCTION_STATUS "-3" },
+};
+
+static const struct gpio_led_platform_data simatic_ipc_gpio_leds_pdata = {
+ .num_leds = ARRAY_SIZE(simatic_ipc_gpio_leds),
+ .leds = simatic_ipc_gpio_leds,
+};
+
+static int simatic_ipc_leds_gpio_remove(struct platform_device *pdev)
+{
+ gpiod_remove_lookup_table(&simatic_ipc_led_gpio_table);
+ gpiod_remove_lookup_table(&simatic_ipc_led_gpio_table_extra);
+ platform_device_unregister(simatic_leds_pdev);
+
+ return 0;
+}
+
+static int simatic_ipc_leds_gpio_probe(struct platform_device *pdev)
+{
+ const struct simatic_ipc_platform *plat = pdev->dev.platform_data;
+ struct device *dev = &pdev->dev;
+ struct gpio_desc *gpiod;
+ int err;
+
+ switch (plat->devmode) {
+ case SIMATIC_IPC_DEVICE_127E:
+ case SIMATIC_IPC_DEVICE_227G:
+ break;
+ default:
+ return -ENODEV;
+ }
+
+ gpiod_add_lookup_table(&simatic_ipc_led_gpio_table);
+ simatic_leds_pdev = platform_device_register_resndata(NULL,
+ "leds-gpio", PLATFORM_DEVID_NONE, NULL, 0,
+ &simatic_ipc_gpio_leds_pdata,
+ sizeof(simatic_ipc_gpio_leds_pdata));
+ if (IS_ERR(simatic_leds_pdev)) {
+ err = PTR_ERR(simatic_leds_pdev);
+ goto out;
+ }
+
+ simatic_ipc_led_gpio_table_extra.dev_id = dev_name(dev);
+ gpiod_add_lookup_table(&simatic_ipc_led_gpio_table_extra);
+
+ /* PM_BIOS_BOOT_N */
+ gpiod = gpiod_get_index(dev, NULL, 6, GPIOD_OUT_LOW);
+ if (IS_ERR(gpiod)) {
+ err = PTR_ERR(gpiod);
+ goto out;
+ }
+ gpiod_put(gpiod);
+
+ /* PM_WDT_OUT */
+ gpiod = gpiod_get_index(dev, NULL, 7, GPIOD_OUT_LOW);
+ if (IS_ERR(gpiod)) {
+ err = PTR_ERR(gpiod);
+ goto out;
+ }
+ gpiod_put(gpiod);
+
+ return 0;
+out:
+ simatic_ipc_leds_gpio_remove(pdev);
+
+ return err;
+}
+
+static struct platform_driver simatic_ipc_led_gpio_driver = {
+ .probe = simatic_ipc_leds_gpio_probe,
+ .remove = simatic_ipc_leds_gpio_remove,
+ .driver = {
+ .name = KBUILD_MODNAME,
+ },
+};
+
+module_platform_driver(simatic_ipc_led_gpio_driver);
+
+MODULE_LICENSE("GPL v2");
+MODULE_ALIAS("platform:" KBUILD_MODNAME);
+
+#endif /* __DRIVERS_LEDS_SIMPLE_SIMATIC_IPC_LEDS_GPIO */
diff --git a/drivers/platform/x86/simatic-ipc.c b/drivers/platform/x86/simatic-ipc.c
index b3622419cd1a..c773995b230d 100644
--- a/drivers/platform/x86/simatic-ipc.c
+++ b/drivers/platform/x86/simatic-ipc.c
@@ -68,9 +68,10 @@ static int register_platform_devices(u32 station_id)
}
if (ledmode != SIMATIC_IPC_DEVICE_NONE) {
- if (ledmode == SIMATIC_IPC_DEVICE_127E ||
- ledmode == SIMATIC_IPC_DEVICE_227G)
- pdevname = KBUILD_MODNAME "_leds_gpio";
+ if (ledmode == SIMATIC_IPC_DEVICE_127E)
+ pdevname = KBUILD_MODNAME "_leds_gpio_apollolake";
+ if (ledmode == SIMATIC_IPC_DEVICE_227G)
+ pdevname = KBUILD_MODNAME "_leds_gpio_f7188x";
platform_data.devmode = ledmode;
ipc_led_platform_device =
platform_device_register_data(NULL,
--
2.39.2
To describe the dependency chain better and allow for potential
fine-grained config tuning, introduce Kconfig switch for the individual
gpio based drivers.
Signed-off-by: Henning Schild <[email protected]>
---
drivers/leds/simple/Kconfig | 31 ++++++++++++++++++++++++++++---
drivers/leds/simple/Makefile | 6 +++---
2 files changed, 31 insertions(+), 6 deletions(-)
diff --git a/drivers/leds/simple/Kconfig b/drivers/leds/simple/Kconfig
index fd2b8225d926..fc80a5d1d78b 100644
--- a/drivers/leds/simple/Kconfig
+++ b/drivers/leds/simple/Kconfig
@@ -1,11 +1,36 @@
# SPDX-License-Identifier: GPL-2.0-only
config LEDS_SIEMENS_SIMATIC_IPC
tristate "LED driver for Siemens Simatic IPCs"
- depends on LEDS_GPIO
depends on SIEMENS_SIMATIC_IPC
help
This option enables support for the LEDs of several Industrial PCs
from Siemens.
- To compile this driver as a module, choose M here: the modules
- will be called simatic-ipc-leds and simatic-ipc-leds-gpio.
+ To compile this driver as a module, choose M here: the module
+ will be called simatic-ipc-leds.
+
+config LEDS_SIEMENS_SIMATIC_IPC_APOLLOLAKE
+ tristate "LED driver for Siemens Simatic IPCs based on Intel apollolake GPIO"
+ depends on LEDS_GPIO
+ depends on PINCTRL_BROXTON
+ depends on SIEMENS_SIMATIC_IPC
+ default LEDS_SIEMENS_SIMATIC_IPC
+ help
+ This option enables support for the LEDs of several Industrial PCs
+ from Siemens based on apollolake GPIO i.e. IPC127E.
+
+ To compile this driver as a module, choose M here: the module
+ will be called simatic-ipc-leds-gpio-apollolake.
+
+config LEDS_SIEMENS_SIMATIC_IPC_F7188X
+ tristate "LED driver for Siemens Simatic IPCs based on Nuvoton GPIO"
+ depends on LEDS_GPIO
+ depends on GPIO_F7188X
+ depends on SIEMENS_SIMATIC_IPC
+ default LEDS_SIEMENS_SIMATIC_IPC
+ help
+ This option enables support for the LEDs of several Industrial PCs
+ from Siemens based on Nuvoton GPIO i.e. IPC227G.
+
+ To compile this driver as a module, choose M here: the module
+ will be called simatic-ipc-leds-gpio-f7188x.
diff --git a/drivers/leds/simple/Makefile b/drivers/leds/simple/Makefile
index 21853956c1ea..1e78bc5904be 100644
--- a/drivers/leds/simple/Makefile
+++ b/drivers/leds/simple/Makefile
@@ -1,4 +1,4 @@
# SPDX-License-Identifier: GPL-2.0
-obj-$(CONFIG_LEDS_SIEMENS_SIMATIC_IPC) += simatic-ipc-leds.o
-obj-$(CONFIG_LEDS_SIEMENS_SIMATIC_IPC) += simatic-ipc-leds-gpio-apollolake.o
-obj-$(CONFIG_LEDS_SIEMENS_SIMATIC_IPC) += simatic-ipc-leds-gpio-f7188x.o
+obj-$(CONFIG_LEDS_SIEMENS_SIMATIC_IPC) += simatic-ipc-leds.o
+obj-$(CONFIG_LEDS_SIEMENS_SIMATIC_IPC_APOLLOLAKE) += simatic-ipc-leds-gpio-apollolake.o
+obj-$(CONFIG_LEDS_SIEMENS_SIMATIC_IPC_F7188X) += simatic-ipc-leds-gpio-f7188x.o
--
2.39.2
On Tue, Feb 21, 2023 at 01:24:13PM +0100, Henning Schild wrote:
> In order to clearly describe the dependencies between the gpio
GPIO
> controller drivers and the users the driver is split up into two and one
one --> a
> common header.
...
> + * Authors:
(everywhere where it is a single author, 's' is redundant)
...
> +#ifndef __DRIVERS_LEDS_SIMPLE_SIMATIC_IPC_LEDS_GPIO
> +#define __DRIVERS_LEDS_SIMPLE_SIMATIC_IPC_LEDS_GPIO
> +#endif /* __DRIVERS_LEDS_SIMPLE_SIMATIC_IPC_LEDS_GPIO */
This header doesn't look right.
Have you run `make W=1 ...` against your patches?
Even if it doesn't show defined but unused errors
the idea is that this should be a C-file, called,
let's say, ...-core.c.
--
With Best Regards,
Andy Shevchenko
On Tue, Feb 21, 2023 at 01:24:14PM +0100, Henning Schild wrote:
> To describe the dependency chain better and allow for potential
> fine-grained config tuning, introduce Kconfig switch for the individual
> gpio based drivers.
GPIO
...
> + tristate "LED driver for Siemens Simatic IPCs based on Intel apollolake GPIO"
Apollo Lake
--
With Best Regards,
Andy Shevchenko
Am Tue, 21 Feb 2023 15:51:03 +0200
schrieb Andy Shevchenko <[email protected]>:
> On Tue, Feb 21, 2023 at 01:24:13PM +0100, Henning Schild wrote:
> > In order to clearly describe the dependencies between the gpio
>
> GPIO
>
> > controller drivers and the users the driver is split up into two
> > and one
>
> one --> a
>
> > common header.
>
> ...
>
> > + * Authors:
>
> (everywhere where it is a single author, 's' is redundant)
>
> ...
>
> > +#ifndef __DRIVERS_LEDS_SIMPLE_SIMATIC_IPC_LEDS_GPIO
> > +#define __DRIVERS_LEDS_SIMPLE_SIMATIC_IPC_LEDS_GPIO
>
> > +#endif /* __DRIVERS_LEDS_SIMPLE_SIMATIC_IPC_LEDS_GPIO */
>
> This header doesn't look right.
>
> Have you run `make W=1 ...` against your patches?
No reports.
> Even if it doesn't show defined but unused errors
> the idea is that this should be a C-file, called,
> let's say, ...-core.c.
When i started i kind of had a -common.c in mind as well. But then the
header idea came and i gave it a try, expecting questions in the review.
It might be a bit unconventional but it seems to do the trick pretty
well. Do you see a concrete problem or a violation of a rule?
Henning
On Tue, Feb 21, 2023 at 03:43:54PM +0100, Henning Schild wrote:
> Am Tue, 21 Feb 2023 15:51:03 +0200
> schrieb Andy Shevchenko <[email protected]>:
> > On Tue, Feb 21, 2023 at 01:24:13PM +0100, Henning Schild wrote:
> > > In order to clearly describe the dependencies between the gpio
...
> > > +#ifndef __DRIVERS_LEDS_SIMPLE_SIMATIC_IPC_LEDS_GPIO
> > > +#define __DRIVERS_LEDS_SIMPLE_SIMATIC_IPC_LEDS_GPIO
> >
> > > +#endif /* __DRIVERS_LEDS_SIMPLE_SIMATIC_IPC_LEDS_GPIO */
> >
> > This header doesn't look right.
> >
> > Have you run `make W=1 ...` against your patches?
>
> No reports.
>
> > Even if it doesn't show defined but unused errors
> > the idea is that this should be a C-file, called,
> > let's say, ...-core.c.
>
> When i started i kind of had a -common.c in mind as well. But then the
> header idea came and i gave it a try, expecting questions in the review.
>
> It might be a bit unconventional but it seems to do the trick pretty
> well. Do you see a concrete problem or a violation of a rule?
Exactly as described above. The header approach means that *all* static
definitions must be used by each user of that file. Otherwise you will
get "defined but not used" compiler warning.
And approach itself is considered (at least by me) as a hackish way to
achieve what usually should be done via C-file.
So, if maintainers are okay, I wouldn't have objections, but again
I do not think it's a correct approach.
--
With Best Regards,
Andy Shevchenko
Hi,
On 2/21/23 15:51, Andy Shevchenko wrote:
> On Tue, Feb 21, 2023 at 03:43:54PM +0100, Henning Schild wrote:
>> Am Tue, 21 Feb 2023 15:51:03 +0200
>> schrieb Andy Shevchenko <[email protected]>:
>>> On Tue, Feb 21, 2023 at 01:24:13PM +0100, Henning Schild wrote:
>>>> In order to clearly describe the dependencies between the gpio
>
> ...
>
>>>> +#ifndef __DRIVERS_LEDS_SIMPLE_SIMATIC_IPC_LEDS_GPIO
>>>> +#define __DRIVERS_LEDS_SIMPLE_SIMATIC_IPC_LEDS_GPIO
>>>
>>>> +#endif /* __DRIVERS_LEDS_SIMPLE_SIMATIC_IPC_LEDS_GPIO */
>>>
>>> This header doesn't look right.
>>>
>>> Have you run `make W=1 ...` against your patches?
>>
>> No reports.
>>
>>> Even if it doesn't show defined but unused errors
>>> the idea is that this should be a C-file, called,
>>> let's say, ...-core.c.
>>
>> When i started i kind of had a -common.c in mind as well. But then the
>> header idea came and i gave it a try, expecting questions in the review.
>>
>> It might be a bit unconventional but it seems to do the trick pretty
>> well. Do you see a concrete problem or a violation of a rule?
>
> Exactly as described above. The header approach means that *all* static
> definitions must be used by each user of that file. Otherwise you will
> get "defined but not used" compiler warning.
>
> And approach itself is considered (at least by me) as a hackish way to
> achieve what usually should be done via C-file.
>
> So, if maintainers are okay, I wouldn't have objections, but again
> I do not think it's a correct approach.
I agree with Andy here, please add a -common.o file with a shared
probe() helper which gets the 2 different gpiod_lookup_table-s
as parameter and then put the actual probe() function calling
the helper inside the 2 different .c files.
And all the:
+static struct platform_driver simatic_ipc_led_gpio_driver = {
+ .probe = simatic_ipc_leds_gpio_probe,
+ .remove = simatic_ipc_leds_gpio_remove,
+ .driver = {
+ .name = KBUILD_MODNAME,
+ },
+};
+
+module_platform_driver(simatic_ipc_led_gpio_driver);
+
+MODULE_LICENSE("GPL v2");
+MODULE_ALIAS("platform:" KBUILD_MODNAME);
bits should then also go into the 2 different .c file files.
Really putting something like module_platform_driver() or
MODULE_LICENSE() / MODULE_ALIAS() inside a .h file is
just wrong IMHO.
Regards,
Hans
On Wed, 01 Mar 2023, Hans de Goede wrote:
> Hi,
>
> On 2/21/23 15:51, Andy Shevchenko wrote:
> > On Tue, Feb 21, 2023 at 03:43:54PM +0100, Henning Schild wrote:
> >> Am Tue, 21 Feb 2023 15:51:03 +0200
> >> schrieb Andy Shevchenko <[email protected]>:
> >>> On Tue, Feb 21, 2023 at 01:24:13PM +0100, Henning Schild wrote:
> >>>> In order to clearly describe the dependencies between the gpio
> >
> > ...
> >
> >>>> +#ifndef __DRIVERS_LEDS_SIMPLE_SIMATIC_IPC_LEDS_GPIO
> >>>> +#define __DRIVERS_LEDS_SIMPLE_SIMATIC_IPC_LEDS_GPIO
> >>>
> >>>> +#endif /* __DRIVERS_LEDS_SIMPLE_SIMATIC_IPC_LEDS_GPIO */
> >>>
> >>> This header doesn't look right.
> >>>
> >>> Have you run `make W=1 ...` against your patches?
> >>
> >> No reports.
> >>
> >>> Even if it doesn't show defined but unused errors
> >>> the idea is that this should be a C-file, called,
> >>> let's say, ...-core.c.
> >>
> >> When i started i kind of had a -common.c in mind as well. But then the
> >> header idea came and i gave it a try, expecting questions in the review.
> >>
> >> It might be a bit unconventional but it seems to do the trick pretty
> >> well. Do you see a concrete problem or a violation of a rule?
> >
> > Exactly as described above. The header approach means that *all* static
> > definitions must be used by each user of that file. Otherwise you will
> > get "defined but not used" compiler warning.
> >
> > And approach itself is considered (at least by me) as a hackish way to
> > achieve what usually should be done via C-file.
> >
> > So, if maintainers are okay, I wouldn't have objections, but again
> > I do not think it's a correct approach.
>
> I agree with Andy here, please add a -common.o file with a shared
> probe() helper which gets the 2 different gpiod_lookup_table-s
> as parameter and then put the actual probe() function calling
> the helper inside the 2 different .c files.
Thanks for your reviews guys, I really appreciate the help.
--
Lee Jones [李琼斯]
On Wed, Mar 01, 2023 at 04:06:09PM +0000, Lee Jones wrote:
> On Wed, 01 Mar 2023, Hans de Goede wrote:
...
> Thanks for your reviews guys, I really appreciate the help.
You're welcome!
--
With Best Regards,
Andy Shevchenko
Am Wed, 1 Mar 2023 15:53:04 +0100
schrieb Hans de Goede <[email protected]>:
> Hi,
>
> On 2/21/23 15:51, Andy Shevchenko wrote:
> > On Tue, Feb 21, 2023 at 03:43:54PM +0100, Henning Schild wrote:
> >> Am Tue, 21 Feb 2023 15:51:03 +0200
> >> schrieb Andy Shevchenko <[email protected]>:
> >>> On Tue, Feb 21, 2023 at 01:24:13PM +0100, Henning Schild wrote:
> >>>> In order to clearly describe the dependencies between the gpio
> >>>>
> >
> > ...
> >
> >>>> +#ifndef __DRIVERS_LEDS_SIMPLE_SIMATIC_IPC_LEDS_GPIO
> >>>> +#define __DRIVERS_LEDS_SIMPLE_SIMATIC_IPC_LEDS_GPIO
> >>>
> >>>> +#endif /* __DRIVERS_LEDS_SIMPLE_SIMATIC_IPC_LEDS_GPIO */
> >>>
> >>> This header doesn't look right.
> >>>
> >>> Have you run `make W=1 ...` against your patches?
> >>
> >> No reports.
> >>
> >>> Even if it doesn't show defined but unused errors
> >>> the idea is that this should be a C-file, called,
> >>> let's say, ...-core.c.
> >>
> >> When i started i kind of had a -common.c in mind as well. But then
> >> the header idea came and i gave it a try, expecting questions in
> >> the review.
> >>
> >> It might be a bit unconventional but it seems to do the trick
> >> pretty well. Do you see a concrete problem or a violation of a
> >> rule?
> >
> > Exactly as described above. The header approach means that *all*
> > static definitions must be used by each user of that file.
> > Otherwise you will get "defined but not used" compiler warning.
> >
> > And approach itself is considered (at least by me) as a hackish way
> > to achieve what usually should be done via C-file.
> >
> > So, if maintainers are okay, I wouldn't have objections, but again
> > I do not think it's a correct approach.
>
> I agree with Andy here, please add a -common.o file with a shared
> probe() helper which gets the 2 different gpiod_lookup_table-s
> as parameter and then put the actual probe() function calling
> the helper inside the 2 different .c files.
>
> And all the:
>
> +static struct platform_driver simatic_ipc_led_gpio_driver = {
> + .probe = simatic_ipc_leds_gpio_probe,
> + .remove = simatic_ipc_leds_gpio_remove,
> + .driver = {
> + .name = KBUILD_MODNAME,
> + },
> +};
> +
> +module_platform_driver(simatic_ipc_led_gpio_driver);
> +
> +MODULE_LICENSE("GPL v2");
> +MODULE_ALIAS("platform:" KBUILD_MODNAME);
>
> bits should then also go into the 2 different .c file files.
>
> Really putting something like module_platform_driver() or
> MODULE_LICENSE() / MODULE_ALIAS() inside a .h file is
> just wrong IMHO.
Thanks for getting back, after Andys review i happen to have just that
already prepared for v2 as "likely needed". Will send that v2 soon.
Henning
> Regards,
>
> Hans
>