[PATCH v2 0/3] leds: simatic-ipc-leds-gpio: split up

[PATCH v2 0/3] leds: simatic-ipc-leds-gpio: split up

[Henning Schild]

changes since v1:
 - move from header- to -core.c-based implementation
 - style changes from review

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 some of its content into a -core,
to be used by the 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 |  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   | 139 ------------------
 drivers/leds/simple/simatic-ipc-leds-gpio.h   |  22 +++
 drivers/platform/x86/simatic-ipc.c            |   7 +-
 8 files changed, 288 insertions(+), 147 deletions(-)
 create mode 100644 drivers/leds/simple/simatic-ipc-leds-gpio-apollolake.c
 create mode 100644 drivers/leds/simple/simatic-ipc-leds-gpio-core.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

-- 
2.39.2

[PATCH v2 1/3] leds: simatic-ipc-leds-gpio: move two extra gpio pins into another table

[Henning Schild]

There are two special pins needed to init the LEDs. We used to have them
at the end of the gpiod_lookup table to give to "leds-gpio". A cleaner
way is to have a dedicated table for the special pins.

Signed-off-by: Henning Schild <henning.schild@siemens.com>
---
 drivers/leds/simple/simatic-ipc-leds-gpio.c | 26 ++++++++++++++++++---
 1 file changed, 23 insertions(+), 3 deletions(-)

diff --git a/drivers/leds/simple/simatic-ipc-leds-gpio.c b/drivers/leds/simple/simatic-ipc-leds-gpio.c
index e8d329b5a68c..ba0f24638af5 100644
--- a/drivers/leds/simple/simatic-ipc-leds-gpio.c
+++ b/drivers/leds/simple/simatic-ipc-leds-gpio.c
@@ -16,6 +16,7 @@
 #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",
@@ -26,6 +27,12 @@ static struct gpiod_lookup_table simatic_ipc_led_gpio_table_127e = {
 		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),
 	},
@@ -40,9 +47,15 @@ static struct gpiod_lookup_table simatic_ipc_led_gpio_table_227g = {
 		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[] = {
@@ -64,6 +77,7 @@ 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;
@@ -72,6 +86,7 @@ static int simatic_ipc_leds_gpio_remove(struct platform_device *pdev)
 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;
 
@@ -80,12 +95,14 @@ static int simatic_ipc_leds_gpio_probe(struct platform_device *pdev)
 		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;
@@ -101,8 +118,11 @@ static int simatic_ipc_leds_gpio_probe(struct platform_device *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(&simatic_leds_pdev->dev, NULL, 6, GPIOD_OUT_LOW);
+	gpiod = gpiod_get_index(dev, NULL, 6, GPIOD_OUT_LOW);
 	if (IS_ERR(gpiod)) {
 		err = PTR_ERR(gpiod);
 		goto out;
@@ -110,7 +130,7 @@ static int simatic_ipc_leds_gpio_probe(struct platform_device *pdev)
 	gpiod_put(gpiod);
 
 	/* PM_WDT_OUT */
-	gpiod = gpiod_get_index(&simatic_leds_pdev->dev, NULL, 7, GPIOD_OUT_LOW);
+	gpiod = gpiod_get_index(dev, NULL, 7, GPIOD_OUT_LOW);
 	if (IS_ERR(gpiod)) {
 		err = PTR_ERR(gpiod);
 		goto out;
-- 
2.39.2

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

[Henning Schild]

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.

Signed-off-by: Henning Schild <henning.schild@siemens.com>
---
 drivers/leds/simple/Makefile                  |   4 +-
 .../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 ------------------
 drivers/leds/simple/simatic-ipc-leds-gpio.h   |  22 +++
 drivers/platform/x86/simatic-ipc.c            |   7 +-
 7 files changed, 260 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-core.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..ed9057f7b6da 100644
--- a/drivers/leds/simple/Makefile
+++ b/drivers/leds/simple/Makefile
@@ -1,3 +1,5 @@
 # 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-core.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..8b874cdb38a1
--- /dev/null
+++ b/drivers/leds/simple/simatic-ipc-leds-gpio-apollolake.c
@@ -0,0 +1,64 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * Siemens SIMATIC IPC driver for GPIO based LEDs
+ *
+ * Copyright (c) Siemens AG, 2023
+ *
+ * Author:
+ *  Henning Schild <henning.schild@siemens.com>
+ */
+
+#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>
+#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),
+	},
+};
+
+static int simatic_ipc_leds_gpio_apollolake_probe(struct platform_device *pdev)
+{
+	return simatic_ipc_leds_gpio_probe(pdev, &simatic_ipc_led_gpio_table,
+					   &simatic_ipc_led_gpio_table_extra);
+}
+
+static int simatic_ipc_leds_gpio_apollolake_remove(struct platform_device *pdev)
+{
+	return simatic_ipc_leds_gpio_remove(pdev, &simatic_ipc_led_gpio_table,
+					    &simatic_ipc_led_gpio_table_extra);
+}
+
+static struct platform_driver simatic_ipc_led_gpio_apollolake_driver = {
+	.probe = simatic_ipc_leds_gpio_apollolake_probe,
+	.remove = simatic_ipc_leds_gpio_apollolake_remove,
+	.driver = {
+		.name = KBUILD_MODNAME,
+	},
+};
+
+module_platform_driver(simatic_ipc_led_gpio_apollolake_driver);
+
+MODULE_LICENSE("GPL v2");
+MODULE_ALIAS("platform:" KBUILD_MODNAME);
+MODULE_SOFTDEP("pre: simatic-ipc-leds-gpio-core platform:apollolake-pinctrl");
+MODULE_AUTHOR("Henning Schild <henning.schild@siemens.com>");
diff --git a/drivers/leds/simple/simatic-ipc-leds-gpio-core.c b/drivers/leds/simple/simatic-ipc-leds-gpio-core.c
new file mode 100644
index 000000000000..ad5c9f8986d4
--- /dev/null
+++ b/drivers/leds/simple/simatic-ipc-leds-gpio-core.c
@@ -0,0 +1,103 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * Siemens SIMATIC IPC driver for GPIO based LEDs
+ *
+ * Copyright (c) Siemens AG, 2023
+ *
+ * Author:
+ *  Henning Schild <henning.schild@siemens.com>
+ */
+
+#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>
+#include "simatic-ipc-leds-gpio.h"
+
+static struct platform_device *simatic_leds_pdev;
+
+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,
+};
+
+int simatic_ipc_leds_gpio_remove(struct platform_device *pdev,
+				 struct gpiod_lookup_table *table,
+				 struct gpiod_lookup_table *table_extra)
+{
+	gpiod_remove_lookup_table(table);
+	gpiod_remove_lookup_table(table_extra);
+	platform_device_unregister(simatic_leds_pdev);
+
+	return 0;
+}
+EXPORT_SYMBOL_GPL(simatic_ipc_leds_gpio_remove);
+
+int simatic_ipc_leds_gpio_probe(struct platform_device *pdev,
+				struct gpiod_lookup_table *table,
+				struct gpiod_lookup_table *table_extra)
+{
+	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(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;
+	}
+
+	table_extra->dev_id = dev_name(dev);
+	gpiod_add_lookup_table(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, table, table_extra);
+
+	return err;
+}
+EXPORT_SYMBOL_GPL(simatic_ipc_leds_gpio_probe);
+
+MODULE_LICENSE("GPL v2");
+MODULE_SOFTDEP("pre: platform:leds-gpio");
+MODULE_AUTHOR("Henning Schild <henning.schild@siemens.com>");
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..d3dff1bffae3
--- /dev/null
+++ b/drivers/leds/simple/simatic-ipc-leds-gpio-f7188x.c
@@ -0,0 +1,64 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * Siemens SIMATIC IPC driver for GPIO based LEDs
+ *
+ * Copyright (c) Siemens AG, 2023
+ *
+ * Author:
+ *  Henning Schild <henning.schild@siemens.com>
+ */
+
+#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>
+#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),
+	},
+};
+
+static int simatic_ipc_leds_gpio_f7188x_probe(struct platform_device *pdev)
+{
+	return simatic_ipc_leds_gpio_probe(pdev, &simatic_ipc_led_gpio_table,
+					   &simatic_ipc_led_gpio_table_extra);
+}
+
+static int simatic_ipc_leds_gpio_f7188x_remove(struct platform_device *pdev)
+{
+	return simatic_ipc_leds_gpio_remove(pdev, &simatic_ipc_led_gpio_table,
+					    &simatic_ipc_led_gpio_table_extra);
+}
+
+static struct platform_driver simatic_ipc_led_gpio_driver = {
+	.probe = simatic_ipc_leds_gpio_f7188x_probe,
+	.remove = simatic_ipc_leds_gpio_f7188x_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: simatic-ipc-leds-gpio-core gpio_f7188x");
+MODULE_AUTHOR("Henning Schild <henning.schild@siemens.com>");
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 <henning.schild@siemens.com>
- */
-
-#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 <henning.schild@siemens.com>");
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..bf258c32f83d
--- /dev/null
+++ b/drivers/leds/simple/simatic-ipc-leds-gpio.h
@@ -0,0 +1,22 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * Siemens SIMATIC IPC driver for GPIO based LEDs
+ *
+ * Copyright (c) Siemens AG, 2023
+ *
+ * Author:
+ *  Henning Schild <henning.schild@siemens.com>
+ */
+
+#ifndef _SIMATIC_IPC_LEDS_GPIO_H
+#define _SIMATIC_IPC_LEDS_GPIO_H
+
+int simatic_ipc_leds_gpio_probe(struct platform_device *pdev,
+				struct gpiod_lookup_table *table,
+				struct gpiod_lookup_table *table_extra);
+
+int simatic_ipc_leds_gpio_remove(struct platform_device *pdev,
+				 struct gpiod_lookup_table *table,
+				 struct gpiod_lookup_table *table_extra);
+
+#endif /* _SIMATIC_IPC_LEDS_GPIO_H */
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

[PATCH v2 3/3] leds: simatic-ipc-leds-gpio: introduce more Kconfig switches

[Henning Schild]

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 <henning.schild@siemens.com>
---
 drivers/leds/simple/Kconfig  | 31 ++++++++++++++++++++++++++++---
 drivers/leds/simple/Makefile |  7 +++----
 2 files changed, 31 insertions(+), 7 deletions(-)

diff --git a/drivers/leds/simple/Kconfig b/drivers/leds/simple/Kconfig
index fd2b8225d926..44fa0f93cb3b 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 Apollo Lake 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 Apollo Lake 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 ed9057f7b6da..e3e840cea275 100644
--- a/drivers/leds/simple/Makefile
+++ b/drivers/leds/simple/Makefile
@@ -1,5 +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-core.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-core.o simatic-ipc-leds-gpio-apollolake.o
+obj-$(CONFIG_LEDS_SIEMENS_SIMATIC_IPC_F7188X)		+= simatic-ipc-leds-gpio-core.o simatic-ipc-leds-gpio-f7188x.o
-- 
2.39.2

Re: [PATCH v2 1/3] leds: simatic-ipc-leds-gpio: move two extra gpio pins into another table

[Andy Shevchenko]

On Wed, Mar 01, 2023 at 06:02:13PM +0100, Henning Schild wrote:
> There are two special pins needed to init the LEDs. We used to have them
> at the end of the gpiod_lookup table to give to "leds-gpio". A cleaner
> way is to have a dedicated table for the special pins.

...

> +	.dev_id = NULL,

Technically speaking this is redundant. Maybe you wanted to be more explicit
with a comment like

	.dev_id = NULL,		/* Filled during initialization */

...

> +	.dev_id = NULL,

Ditto.

-- 
With Best Regards,
Andy Shevchenko

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

[Andy Shevchenko]

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

Re: [PATCH v2 3/3] leds: simatic-ipc-leds-gpio: introduce more Kconfig switches

[Andy Shevchenko]

On Wed, Mar 01, 2023 at 06:02:15PM +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.

...

> +obj-$(CONFIG_LEDS_SIEMENS_SIMATIC_IPC)			+= simatic-ipc-leds.o
> +obj-$(CONFIG_LEDS_SIEMENS_SIMATIC_IPC_APOLLOLAKE)	+= simatic-ipc-leds-gpio-core.o simatic-ipc-leds-gpio-apollolake.o
> +obj-$(CONFIG_LEDS_SIEMENS_SIMATIC_IPC_F7188X)		+= simatic-ipc-leds-gpio-core.o simatic-ipc-leds-gpio-f7188x.o

Have you tried the configuration where all three are Y?

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v2 0/3] leds: simatic-ipc-leds-gpio: split up

[Andy Shevchenko]

On Wed, Mar 01, 2023 at 06:02:12PM +0100, Henning Schild wrote:

> 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 some of its content into a -core,
> to be used by the 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

Thank you for an update.
Not big issues found (except maybe the Makefile).

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v2 3/3] leds: simatic-ipc-leds-gpio: introduce more Kconfig switches

[Hans de Goede]

Hi,

On 3/1/23 18:02, 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.
> 
> Signed-off-by: Henning Schild <henning.schild@siemens.com>
> ---
>  drivers/leds/simple/Kconfig  | 31 ++++++++++++++++++++++++++++---
>  drivers/leds/simple/Makefile |  7 +++----
>  2 files changed, 31 insertions(+), 7 deletions(-)
> 
> diff --git a/drivers/leds/simple/Kconfig b/drivers/leds/simple/Kconfig
> index fd2b8225d926..44fa0f93cb3b 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

Since it is simatic-ipc-leds-gpio-core.c which actually registers
the LEDs GPIO platform device, this one should stay IMHO.


>  	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 Apollo Lake GPIO"
> +	depends on LEDS_GPIO

And then it can be dropped here.

> +	depends on PINCTRL_BROXTON

> +	depends on SIEMENS_SIMATIC_IPC

This should be "depends on LEDS_SIEMENS_SIMATIC_IPC" since it
actually uses symbol from that module.

> +	default LEDS_SIEMENS_SIMATIC_IPC
> +	help
> +	  This option enables support for the LEDs of several Industrial PCs
> +	  from Siemens based on Apollo Lake 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

Idem.

> +	depends on GPIO_F7188X
> +	depends on SIEMENS_SIMATIC_IPC

Idem.

> +	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 ed9057f7b6da..e3e840cea275 100644
> --- a/drivers/leds/simple/Makefile
> +++ b/drivers/leds/simple/Makefile
> @@ -1,5 +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-core.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-core.o simatic-ipc-leds-gpio-apollolake.o
> +obj-$(CONFIG_LEDS_SIEMENS_SIMATIC_IPC_F7188X)		+= simatic-ipc-leds-gpio-core.o simatic-ipc-leds-gpio-f7188x.o

Regards,

Hans

Re: [PATCH v2 3/3] leds: simatic-ipc-leds-gpio: introduce more Kconfig switches

[Henning Schild]

Am Wed, 1 Mar 2023 19:29:40 +0200
schrieb Andy Shevchenko <andriy.shevchenko@linux.intel.com>:

> On Wed, Mar 01, 2023 at 06:02:15PM +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.  
> 
> ...
> 
> > +obj-$(CONFIG_LEDS_SIEMENS_SIMATIC_IPC)			+=
> > simatic-ipc-leds.o
> > +obj-$(CONFIG_LEDS_SIEMENS_SIMATIC_IPC_APOLLOLAKE)	+=
> > simatic-ipc-leds-gpio-core.o simatic-ipc-leds-gpio-apollolake.o
> > +obj-$(CONFIG_LEDS_SIEMENS_SIMATIC_IPC_F7188X)		+=
> > simatic-ipc-leds-gpio-core.o simatic-ipc-leds-gpio-f7188x.o  
> 
> Have you tried the configuration where all three are Y?
> 

I tried all m and will also try all Y. Why do you ask?

Henning

Re: [PATCH v2 1/3] leds: simatic-ipc-leds-gpio: move two extra gpio pins into another table

[Henning Schild]

Am Wed, 1 Mar 2023 19:21:53 +0200
schrieb Andy Shevchenko <andriy.shevchenko@linux.intel.com>:

> On Wed, Mar 01, 2023 at 06:02:13PM +0100, Henning Schild wrote:
> > There are two special pins needed to init the LEDs. We used to have
> > them at the end of the gpiod_lookup table to give to "leds-gpio". A
> > cleaner way is to have a dedicated table for the special pins.  
> 
> ...
> 
> > +	.dev_id = NULL,  
> 
> Technically speaking this is redundant. Maybe you wanted to be more
> explicit with a comment like
> 
> 	.dev_id = NULL,		/* Filled during
> initialization */
> 
> ...
> 
> > +	.dev_id = NULL,  
> 
> Ditto.

Sorry you already pointed that out earlier, it slipped back in with
the rewrite.

Henning

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@linux.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?
> 
> > +#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.

Switching from one module to three the names have to change. People who
explicitly loaded the old module which supported multiple machines,
will now how to load either both oŕ know which one to load.

I personally think the ABI change is acceptable, the assumption would
be that the drivers load automatically anyhow. And since there are no
params i doubt users will have /etc/modprobe.d/ or /sys/module/ stuff
around.

And with the split i guess an ABI change can not be fully avoided.
Whether the names is explicit or implicit is another discussion and
just a matter of style. I prefer to stay with the currently used
pattern, it is not un-common in the kernel.

> > +	},  
> 
> ...
> 
> > +  
> 
> Redundant blank line.
> 
> > +module_platform_driver(simatic_ipc_led_gpio_apollolake_driver);  
> 
> ...
> 
> > +MODULE_ALIAS("platform:" KBUILD_MODNAME);  
> 
> Why? HAve you missed MODULE_DEVICE_TABLE()?

I do not know what that is, but i will have a look.

Henning

> ...
> 
> > +++ b/drivers/leds/simple/simatic-ipc-leds-gpio-f7188x.c  
> 
> Similar comments as per above.
> 

Re: [PATCH v2 3/3] leds: simatic-ipc-leds-gpio: introduce more Kconfig switches

[Henning Schild]

Am Wed, 1 Mar 2023 19:04:01 +0100
schrieb Hans de Goede <hdegoede@redhat.com>:

> Hi,
> 
> On 3/1/23 18:02, 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.
> > 
> > Signed-off-by: Henning Schild <henning.schild@siemens.com>
> > ---
> >  drivers/leds/simple/Kconfig  | 31 ++++++++++++++++++++++++++++---
> >  drivers/leds/simple/Makefile |  7 +++----
> >  2 files changed, 31 insertions(+), 7 deletions(-)
> > 
> > diff --git a/drivers/leds/simple/Kconfig
> > b/drivers/leds/simple/Kconfig index fd2b8225d926..44fa0f93cb3b
> > 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  
> 
> Since it is simatic-ipc-leds-gpio-core.c which actually registers
> the LEDs GPIO platform device, this one should stay IMHO.

No this one is now only for the port-IO based driver. The GPIO core is
built under the two new switches only.

> 
> >  	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 Apollo Lake GPIO"
> > +	depends on LEDS_GPIO  
> 
> And then it can be dropped here.
> 
> > +	depends on PINCTRL_BROXTON  
> 
> > +	depends on SIEMENS_SIMATIC_IPC  
> 
> This should be "depends on LEDS_SIEMENS_SIMATIC_IPC" since it
> actually uses symbol from that module.

Same as above, the GPIO based drivers do not depend on the port-IO
based driver.

Henning

> > +	default LEDS_SIEMENS_SIMATIC_IPC
> > +	help
> > +	  This option enables support for the LEDs of several
> > Industrial PCs
> > +	  from Siemens based on Apollo Lake 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  
> 
> Idem.
> 
> > +	depends on GPIO_F7188X
> > +	depends on SIEMENS_SIMATIC_IPC  
> 
> Idem.
> 
> > +	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 ed9057f7b6da..e3e840cea275
> > 100644 --- a/drivers/leds/simple/Makefile
> > +++ b/drivers/leds/simple/Makefile
> > @@ -1,5 +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-core.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-core.o simatic-ipc-leds-gpio-apollolake.o
> > +obj-$(CONFIG_LEDS_SIEMENS_SIMATIC_IPC_F7188X)		+=
> > simatic-ipc-leds-gpio-core.o simatic-ipc-leds-gpio-f7188x.o  
> 
> Regards,
> 
> Hans
> 

Re: [PATCH v2 3/3] leds: simatic-ipc-leds-gpio: introduce more Kconfig switches

[Hans de Goede]

Hi,

On 3/2/23 09:47, Henning Schild wrote:
> Am Wed, 1 Mar 2023 19:04:01 +0100
> schrieb Hans de Goede <hdegoede@redhat.com>:
> 
>> Hi,
>>
>> On 3/1/23 18:02, 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.
>>>
>>> Signed-off-by: Henning Schild <henning.schild@siemens.com>
>>> ---
>>>  drivers/leds/simple/Kconfig  | 31 ++++++++++++++++++++++++++++---
>>>  drivers/leds/simple/Makefile |  7 +++----
>>>  2 files changed, 31 insertions(+), 7 deletions(-)
>>>
>>> diff --git a/drivers/leds/simple/Kconfig
>>> b/drivers/leds/simple/Kconfig index fd2b8225d926..44fa0f93cb3b
>>> 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  
>>
>> Since it is simatic-ipc-leds-gpio-core.c which actually registers
>> the LEDs GPIO platform device, this one should stay IMHO.
> 
> No this one is now only for the port-IO based driver. The GPIO core is
> built under the two new switches only.

You are right, I thought this would enable building
simatic-ipc-leds-gpio-core.o into its own .ko which would
then be used by both other gpio LED drivers. But upon a closer
look at the Makefile changes I see that is not the case.

Note that with your current solution you are linking that into
the kernel twice.

As long this is build into modules that is fine. But if both are builtin
I *think* you may get linker errors because of duplicate symbols?

I believe this is why Andy asked to try a build with all 3 options set to Y.

>>>  	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 Apollo Lake GPIO"
>>> +	depends on LEDS_GPIO  
>>
>> And then it can be dropped here.
>>
>>> +	depends on PINCTRL_BROXTON  
>>
>>> +	depends on SIEMENS_SIMATIC_IPC  
>>
>> This should be "depends on LEDS_SIEMENS_SIMATIC_IPC" since it
>> actually uses symbol from that module.
> 
> Same as above, the GPIO based drivers do not depend on the port-IO
> based driver.

Ack.

Regards,

Hans






>>> +	default LEDS_SIEMENS_SIMATIC_IPC
>>> +	help
>>> +	  This option enables support for the LEDs of several
>>> Industrial PCs
>>> +	  from Siemens based on Apollo Lake 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  
>>
>> Idem.
>>
>>> +	depends on GPIO_F7188X
>>> +	depends on SIEMENS_SIMATIC_IPC  
>>
>> Idem.
>>
>>> +	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 ed9057f7b6da..e3e840cea275
>>> 100644 --- a/drivers/leds/simple/Makefile
>>> +++ b/drivers/leds/simple/Makefile
>>> @@ -1,5 +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-core.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-core.o simatic-ipc-leds-gpio-apollolake.o
>>> +obj-$(CONFIG_LEDS_SIEMENS_SIMATIC_IPC_F7188X)		+=
>>> simatic-ipc-leds-gpio-core.o simatic-ipc-leds-gpio-f7188x.o  
>>
>> Regards,
>>
>> Hans
>>
> 

Re: [PATCH v2 3/3] leds: simatic-ipc-leds-gpio: introduce more Kconfig switches

[Henning Schild]

Am Thu, 2 Mar 2023 10:02:51 +0100
schrieb Hans de Goede <hdegoede@redhat.com>:

> Hi,
> 
> On 3/2/23 09:47, Henning Schild wrote:
> > Am Wed, 1 Mar 2023 19:04:01 +0100
> > schrieb Hans de Goede <hdegoede@redhat.com>:
> >   
> >> Hi,
> >>
> >> On 3/1/23 18:02, 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.
> >>>
> >>> Signed-off-by: Henning Schild <henning.schild@siemens.com>
> >>> ---
> >>>  drivers/leds/simple/Kconfig  | 31 ++++++++++++++++++++++++++++---
> >>>  drivers/leds/simple/Makefile |  7 +++----
> >>>  2 files changed, 31 insertions(+), 7 deletions(-)
> >>>
> >>> diff --git a/drivers/leds/simple/Kconfig
> >>> b/drivers/leds/simple/Kconfig index fd2b8225d926..44fa0f93cb3b
> >>> 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    
> >>
> >> Since it is simatic-ipc-leds-gpio-core.c which actually registers
> >> the LEDs GPIO platform device, this one should stay IMHO.  
> > 
> > No this one is now only for the port-IO based driver. The GPIO core
> > is built under the two new switches only.  
> 
> You are right, I thought this would enable building
> simatic-ipc-leds-gpio-core.o into its own .ko which would
> then be used by both other gpio LED drivers. But upon a closer
> look at the Makefile changes I see that is not the case.
> 
> Note that with your current solution you are linking that into
> the kernel twice.
> 
> As long this is build into modules that is fine. But if both are
> builtin I *think* you may get linker errors because of duplicate
> symbols?
> 
> I believe this is why Andy asked to try a build with all 3 options
> set to Y.

Thanks. There are no linker errors so i think there is no need for yet
another CONFIG especially for the core. I would try to keep it simple
as the name of the directory suggests, so go with what we have.

Henning

> 
> >>>  	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 Apollo Lake GPIO"
> >>> +	depends on LEDS_GPIO    
> >>
> >> And then it can be dropped here.
> >>  
> >>> +	depends on PINCTRL_BROXTON    
> >>  
> >>> +	depends on SIEMENS_SIMATIC_IPC    
> >>
> >> This should be "depends on LEDS_SIEMENS_SIMATIC_IPC" since it
> >> actually uses symbol from that module.  
> > 
> > Same as above, the GPIO based drivers do not depend on the port-IO
> > based driver.  
> 
> Ack.
> 
> Regards,
> 
> Hans
> 
> 
> 
> 
> 
> 
> >>> +	default LEDS_SIEMENS_SIMATIC_IPC
> >>> +	help
> >>> +	  This option enables support for the LEDs of several
> >>> Industrial PCs
> >>> +	  from Siemens based on Apollo Lake 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    
> >>
> >> Idem.
> >>  
> >>> +	depends on GPIO_F7188X
> >>> +	depends on SIEMENS_SIMATIC_IPC    
> >>
> >> Idem.
> >>  
> >>> +	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 ed9057f7b6da..e3e840cea275
> >>> 100644 --- a/drivers/leds/simple/Makefile
> >>> +++ b/drivers/leds/simple/Makefile
> >>> @@ -1,5 +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-core.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-core.o simatic-ipc-leds-gpio-apollolake.o
> >>> +obj-$(CONFIG_LEDS_SIEMENS_SIMATIC_IPC_F7188X)		+=
> >>> simatic-ipc-leds-gpio-core.o simatic-ipc-leds-gpio-f7188x.o    
> >>
> >> Regards,
> >>
> >> Hans
> >>  
> >   
> 

Re: [PATCH v2 0/3] leds: simatic-ipc-leds-gpio: split up

[Henning Schild]

Am Wed, 1 Mar 2023 19:30:38 +0200
schrieb Andy Shevchenko <andriy.shevchenko@linux.intel.com>:

> On Wed, Mar 01, 2023 at 06:02:12PM +0100, Henning Schild wrote:
> 
> > 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 some of its content into a
> > -core, to be used by the 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  
> 
> Thank you for an update.
> Not big issues found (except maybe the Makefile).
> 

The Makefile does work with all Y. So if that was all that was open,
and the rest is just optional improvement. I would be happy if this can
be merged as is, and deal with improvements later.

Thanks!

Henning

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

[Andy Shevchenko]

On Thu, Mar 02, 2023 at 09:40:09AM +0100, Henning Schild wrote:
> Am Wed, 1 Mar 2023 19:28:12 +0200
> schrieb Andy Shevchenko <andriy.shevchenko@linux.intel.com>:
> > On Wed, Mar 01, 2023 at 06:02:14PM +0100, Henning Schild wrote:

...

> > > +	.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.
> 
> Switching from one module to three the names have to change. People who
> explicitly loaded the old module which supported multiple machines,
> will now how to load either both oŕ know which one to load.

Wait, are you telling that now users load modules _manually_?!

> I personally think the ABI change is acceptable, the assumption would
> be that the drivers load automatically anyhow. And since there are no
> params i doubt users will have /etc/modprobe.d/ or /sys/module/ stuff
> around.
> 
> And with the split i guess an ABI change can not be fully avoided.
> Whether the names is explicit or implicit is another discussion and
> just a matter of style. I prefer to stay with the currently used
> pattern, it is not un-common in the kernel.

The problem with that pattern is possible, while unlikely, renaming of the file
which triggers this to be updated.

Under sysfs the folder will change its name. If user has a script relying on
this, it will be broken. So, I prefer mine.

> > > +	},  

-- 
With Best Regards,
Andy Shevchenko

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

[Henning Schild]

Am Thu, 2 Mar 2023 17:46:54 +0200
schrieb Andy Shevchenko <andriy.shevchenko@linux.intel.com>:

> On Thu, Mar 02, 2023 at 09:40:09AM +0100, Henning Schild wrote:
> > Am Wed, 1 Mar 2023 19:28:12 +0200
> > schrieb Andy Shevchenko <andriy.shevchenko@linux.intel.com>:  
> > > On Wed, Mar 01, 2023 at 06:02:14PM +0100, Henning Schild wrote:  
> 
> ...
> 
> > > > +	.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.  
> > 
> > Switching from one module to three the names have to change. People
> > who explicitly loaded the old module which supported multiple
> > machines, will now how to load either both oŕ know which one to
> > load.  
> 
> Wait, are you telling that now users load modules _manually_?!

No i am not, the modules all load automatically. I was trying to
construct a hypothetical case where the name change could affect users
doing unexpected things.

> > I personally think the ABI change is acceptable, the assumption
> > would be that the drivers load automatically anyhow. And since
> > there are no params i doubt users will have /etc/modprobe.d/ or
> > /sys/module/ stuff around.
> > 
> > And with the split i guess an ABI change can not be fully avoided.
> > Whether the names is explicit or implicit is another discussion and
> > just a matter of style. I prefer to stay with the currently used
> > pattern, it is not un-common in the kernel.  
> 
> The problem with that pattern is possible, while unlikely, renaming
> of the file which triggers this to be updated.
> 
> Under sysfs the folder will change its name. If user has a script
> relying on this, it will be broken. So, I prefer mine.

Yes, the name of the module will change ... and the location of module
metadata and params in sysfs, both not a big deal here. Because there
are no params, and there is not need to modprobe manually.

Henning

> > > > +	},    
> 

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

[Andy Shevchenko]

On Thu, Mar 02, 2023 at 04:58:24PM +0100, Henning Schild wrote:
> Am Thu, 2 Mar 2023 17:46:54 +0200
> schrieb Andy Shevchenko <andriy.shevchenko@linux.intel.com>:
> > On Thu, Mar 02, 2023 at 09:40:09AM +0100, Henning Schild wrote:
> > > Am Wed, 1 Mar 2023 19:28:12 +0200
> > > schrieb Andy Shevchenko <andriy.shevchenko@linux.intel.com>:  
> > > > On Wed, Mar 01, 2023 at 06:02:14PM +0100, Henning Schild wrote:  

...

> > > > > +	.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.  
> > > 
> > > Switching from one module to three the names have to change. People
> > > who explicitly loaded the old module which supported multiple
> > > machines, will now how to load either both oŕ know which one to
> > > load.  
> > 
> > Wait, are you telling that now users load modules _manually_?!
> 
> No i am not, the modules all load automatically. I was trying to
> construct a hypothetical case where the name change could affect users
> doing unexpected things.
> 
> > > I personally think the ABI change is acceptable, the assumption
> > > would be that the drivers load automatically anyhow. And since
> > > there are no params i doubt users will have /etc/modprobe.d/ or
> > > /sys/module/ stuff around.
> > > 
> > > And with the split i guess an ABI change can not be fully avoided.
> > > Whether the names is explicit or implicit is another discussion and
> > > just a matter of style. I prefer to stay with the currently used
> > > pattern, it is not un-common in the kernel.  
> > 
> > The problem with that pattern is possible, while unlikely, renaming
> > of the file which triggers this to be updated.
> > 
> > Under sysfs the folder will change its name. If user has a script
> > relying on this, it will be broken. So, I prefer mine.
> 
> Yes, the name of the module will change ... and the location of module
> metadata and params in sysfs, both not a big deal here. Because there
> are no params, and there is not need to modprobe manually.

It's not only one folder AFAIU. Also folder in the drivers will change
its name. Parameters is one thing, the folder presence is another.

Yes, the case is quite unlikely to happen (to break anyone's setup)
that's why I started this with 'Strictly speaking...'. So, seems you
are staying on your side, I will leave this to maintainers. If they
are fine, I will have no objections.

> > > > > +	},    

-- 
With Best Regards,
Andy Shevchenko

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

[Henning Schild]

Am Thu, 2 Mar 2023 18:21:21 +0200
schrieb Andy Shevchenko <andriy.shevchenko@linux.intel.com>:

> On Thu, Mar 02, 2023 at 04:58:24PM +0100, Henning Schild wrote:
> > Am Thu, 2 Mar 2023 17:46:54 +0200
> > schrieb Andy Shevchenko <andriy.shevchenko@linux.intel.com>:  
> > > On Thu, Mar 02, 2023 at 09:40:09AM +0100, Henning Schild wrote:  
> > > > Am Wed, 1 Mar 2023 19:28:12 +0200
> > > > schrieb Andy Shevchenko <andriy.shevchenko@linux.intel.com>:    
> > > > > On Wed, Mar 01, 2023 at 06:02:14PM +0100, Henning Schild
> > > > > wrote:    
> 
> ...
> 
> > > > > > +	.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.    
> > > > 
> > > > Switching from one module to three the names have to change.
> > > > People who explicitly loaded the old module which supported
> > > > multiple machines, will now how to load either both oŕ know
> > > > which one to load.    
> > > 
> > > Wait, are you telling that now users load modules _manually_?!  
> > 
> > No i am not, the modules all load automatically. I was trying to
> > construct a hypothetical case where the name change could affect
> > users doing unexpected things.
> >   
> > > > I personally think the ABI change is acceptable, the assumption
> > > > would be that the drivers load automatically anyhow. And since
> > > > there are no params i doubt users will have /etc/modprobe.d/ or
> > > > /sys/module/ stuff around.
> > > > 
> > > > And with the split i guess an ABI change can not be fully
> > > > avoided. Whether the names is explicit or implicit is another
> > > > discussion and just a matter of style. I prefer to stay with
> > > > the currently used pattern, it is not un-common in the kernel.
> > > >   
> > > 
> > > The problem with that pattern is possible, while unlikely,
> > > renaming of the file which triggers this to be updated.
> > > 
> > > Under sysfs the folder will change its name. If user has a script
> > > relying on this, it will be broken. So, I prefer mine.  
> > 
> > Yes, the name of the module will change ... and the location of
> > module metadata and params in sysfs, both not a big deal here.
> > Because there are no params, and there is not need to modprobe
> > manually.  
> 
> It's not only one folder AFAIU. Also folder in the drivers will change
> its name. Parameters is one thing, the folder presence is another.
> 
> Yes, the case is quite unlikely to happen (to break anyone's setup)
> that's why I started this with 'Strictly speaking...'. So, seems you
> are staying on your side, I will leave this to maintainers. If they
> are fine, I will have no objections.

We are splitting one module into three, so we will end up with three
names. Or i miss something here.

The only thing one could talk about is whether that string should be
hardcoded or derived from the name of the c-file.

Anyhow, thanks and we should probably just wait what others have to say.

Henning

> 
> > > > > > +	},      
> 

Re: [PATCH v2 0/3] leds: simatic-ipc-leds-gpio: split up

[Henning Schild]

Ping.

As far as i remember this one should be ready to merge.

ACKed by Hans, and Andy had no objections given the maintainers would
accept the patches.

So i think it is the maintainers turn now, Pavel and/or Lee i guess.

Henning

Am Wed, 1 Mar 2023 18:02:12 +0100
schrieb Henning Schild <henning.schild@siemens.com>:

> changes since v1:
>  - move from header- to -core.c-based implementation
>  - style changes from review
> 
> 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 some of its content into a -core,
> to be used by the 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 |  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   | 139
> ------------------ drivers/leds/simple/simatic-ipc-leds-gpio.h   |
> 22 +++ drivers/platform/x86/simatic-ipc.c            |   7 +-
>  8 files changed, 288 insertions(+), 147 deletions(-)
>  create mode 100644
> drivers/leds/simple/simatic-ipc-leds-gpio-apollolake.c create mode
> 100644 drivers/leds/simple/simatic-ipc-leds-gpio-core.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
> 

Re: [PATCH v2 0/3] leds: simatic-ipc-leds-gpio: split up

[Lee Jones]

On Sat, 13 May 2023, Henning Schild wrote:

> Ping.
> 
> As far as i remember this one should be ready to merge.
> 
> ACKed by Hans, and Andy had no objections given the maintainers would
> accept the patches.
> 
> So i think it is the maintainers turn now, Pavel and/or Lee i guess.

Looks like there are lots of mail threads in reply to each of the
patches.  If these culminated in acceptance, please collect all of the
Acks you've received until this point and submit a [RESEND].  If there
are changes to be made still, please do the same but submit as a new
version.

-- 
Lee Jones [李琼斯]

Re: [PATCH v2 0/3] leds: simatic-ipc-leds-gpio: split up

[Henning Schild]

Am Mon, 15 May 2023 10:50:42 +0100
schrieb Lee Jones <lee@kernel.org>:

> On Sat, 13 May 2023, Henning Schild wrote:
> 
> > Ping.
> > 
> > As far as i remember this one should be ready to merge.
> > 
> > ACKed by Hans, and Andy had no objections given the maintainers
> > would accept the patches.
> > 
> > So i think it is the maintainers turn now, Pavel and/or Lee i
> > guess.  
> 
> Looks like there are lots of mail threads in reply to each of the
> patches.  If these culminated in acceptance, please collect all of the
> Acks you've received until this point and submit a [RESEND].  If there
> are changes to be made still, please do the same but submit as a new
> version.

Ok i will send it again and include the ACK from Hans. There would be no
code change. Please review the code already, if you find it acceptable
we might receive a second ACK from Andy.

Henning

Re: [PATCH v2 3/3] leds: simatic-ipc-leds-gpio: introduce more Kconfig switches

[Henning Schild]

Am Thu, 2 Mar 2023 10:02:51 +0100
schrieb Hans de Goede <hdegoede@redhat.com>:

> Hi,
> 
> On 3/2/23 09:47, Henning Schild wrote:
> > Am Wed, 1 Mar 2023 19:04:01 +0100
> > schrieb Hans de Goede <hdegoede@redhat.com>:
> >   
> >> Hi,
> >>
> >> On 3/1/23 18:02, 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.
> >>>
> >>> Signed-off-by: Henning Schild <henning.schild@siemens.com>
> >>> ---
> >>>  drivers/leds/simple/Kconfig  | 31 ++++++++++++++++++++++++++++---
> >>>  drivers/leds/simple/Makefile |  7 +++----
> >>>  2 files changed, 31 insertions(+), 7 deletions(-)
> >>>
> >>> diff --git a/drivers/leds/simple/Kconfig
> >>> b/drivers/leds/simple/Kconfig index fd2b8225d926..44fa0f93cb3b
> >>> 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    
> >>
> >> Since it is simatic-ipc-leds-gpio-core.c which actually registers
> >> the LEDs GPIO platform device, this one should stay IMHO.  
> > 
> > No this one is now only for the port-IO based driver. The GPIO core
> > is built under the two new switches only.  
> 
> You are right, I thought this would enable building
> simatic-ipc-leds-gpio-core.o into its own .ko which would
> then be used by both other gpio LED drivers. But upon a closer
> look at the Makefile changes I see that is not the case.
> 
> Note that with your current solution you are linking that into
> the kernel twice.
> 
> As long this is build into modules that is fine. But if both are
> builtin I *think* you may get linker errors because of duplicate
> symbols?
> 
> I believe this is why Andy asked to try a build with all 3 options
> set to Y.
> 
> >>>  	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 Apollo Lake GPIO"
> >>> +	depends on LEDS_GPIO    
> >>
> >> And then it can be dropped here.
> >>  
> >>> +	depends on PINCTRL_BROXTON    
> >>  
> >>> +	depends on SIEMENS_SIMATIC_IPC    
> >>
> >> This should be "depends on LEDS_SIEMENS_SIMATIC_IPC" since it
> >> actually uses symbol from that module.  
> > 
> > Same as above, the GPIO based drivers do not depend on the port-IO
> > based driver.  

Hi Hans,

> Ack.

was that for that last patch or all three of them? I am preparing a v3
which will include ACKs, i will have to assume p3 only because it came
in reply to that patch.

regards,
Henning

> 
> Regards,
> 
> Hans
> 
> 
> 
> 
> 
> 
> >>> +	default LEDS_SIEMENS_SIMATIC_IPC
> >>> +	help
> >>> +	  This option enables support for the LEDs of several
> >>> Industrial PCs
> >>> +	  from Siemens based on Apollo Lake 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    
> >>
> >> Idem.
> >>  
> >>> +	depends on GPIO_F7188X
> >>> +	depends on SIEMENS_SIMATIC_IPC    
> >>
> >> Idem.
> >>  
> >>> +	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 ed9057f7b6da..e3e840cea275
> >>> 100644 --- a/drivers/leds/simple/Makefile
> >>> +++ b/drivers/leds/simple/Makefile
> >>> @@ -1,5 +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-core.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-core.o simatic-ipc-leds-gpio-apollolake.o
> >>> +obj-$(CONFIG_LEDS_SIEMENS_SIMATIC_IPC_F7188X)		+=
> >>> simatic-ipc-leds-gpio-core.o simatic-ipc-leds-gpio-f7188x.o    
> >>
> >> Regards,
> >>
> >> Hans
> >>  
> >   
> 

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

Re: [PATCH v2 3/3] leds: simatic-ipc-leds-gpio: introduce more Kconfig switches

[Hans de Goede]

Hi,

On 5/15/23 12:41, Henning Schild wrote:
> Am Thu, 2 Mar 2023 10:02:51 +0100
> schrieb Hans de Goede <hdegoede@redhat.com>:
> 
>> Hi,
>>
>> On 3/2/23 09:47, Henning Schild wrote:
>>> Am Wed, 1 Mar 2023 19:04:01 +0100
>>> schrieb Hans de Goede <hdegoede@redhat.com>:
>>>   
>>>> Hi,
>>>>
>>>> On 3/1/23 18:02, 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.
>>>>>
>>>>> Signed-off-by: Henning Schild <henning.schild@siemens.com>
>>>>> ---
>>>>>  drivers/leds/simple/Kconfig  | 31 ++++++++++++++++++++++++++++---
>>>>>  drivers/leds/simple/Makefile |  7 +++----
>>>>>  2 files changed, 31 insertions(+), 7 deletions(-)
>>>>>
>>>>> diff --git a/drivers/leds/simple/Kconfig
>>>>> b/drivers/leds/simple/Kconfig index fd2b8225d926..44fa0f93cb3b
>>>>> 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    
>>>>
>>>> Since it is simatic-ipc-leds-gpio-core.c which actually registers
>>>> the LEDs GPIO platform device, this one should stay IMHO.  
>>>
>>> No this one is now only for the port-IO based driver. The GPIO core
>>> is built under the two new switches only.  
>>
>> You are right, I thought this would enable building
>> simatic-ipc-leds-gpio-core.o into its own .ko which would
>> then be used by both other gpio LED drivers. But upon a closer
>> look at the Makefile changes I see that is not the case.
>>
>> Note that with your current solution you are linking that into
>> the kernel twice.
>>
>> As long this is build into modules that is fine. But if both are
>> builtin I *think* you may get linker errors because of duplicate
>> symbols?
>>
>> I believe this is why Andy asked to try a build with all 3 options
>> set to Y.
>>
>>>>>  	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 Apollo Lake GPIO"
>>>>> +	depends on LEDS_GPIO    
>>>>
>>>> And then it can be dropped here.
>>>>  
>>>>> +	depends on PINCTRL_BROXTON    
>>>>  
>>>>> +	depends on SIEMENS_SIMATIC_IPC    
>>>>
>>>> This should be "depends on LEDS_SIEMENS_SIMATIC_IPC" since it
>>>> actually uses symbol from that module.  
>>>
>>> Same as above, the GPIO based drivers do not depend on the port-IO
>>> based driver.  
> 
> Hi Hans,
> 
>> Ack.
> 
> was that for that last patch or all three of them? I am preparing a v3
> which will include ACKs, i will have to assume p3 only because it came
> in reply to that patch.

You may add my:

Acked-by: Hans de Goede <hdegoede@redhat.com>

To all 3 patches in the series.

Regards,

Hans






>>>>> +	default LEDS_SIEMENS_SIMATIC_IPC
>>>>> +	help
>>>>> +	  This option enables support for the LEDs of several
>>>>> Industrial PCs
>>>>> +	  from Siemens based on Apollo Lake 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    
>>>>
>>>> Idem.
>>>>  
>>>>> +	depends on GPIO_F7188X
>>>>> +	depends on SIEMENS_SIMATIC_IPC    
>>>>
>>>> Idem.
>>>>  
>>>>> +	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 ed9057f7b6da..e3e840cea275
>>>>> 100644 --- a/drivers/leds/simple/Makefile
>>>>> +++ b/drivers/leds/simple/Makefile
>>>>> @@ -1,5 +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-core.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-core.o simatic-ipc-leds-gpio-apollolake.o
>>>>> +obj-$(CONFIG_LEDS_SIEMENS_SIMATIC_IPC_F7188X)		+=
>>>>> simatic-ipc-leds-gpio-core.o simatic-ipc-leds-gpio-f7188x.o    
>>>>
>>>> Regards,
>>>>
>>>> Hans
>>>>  
>>>   
>>
> 

Re: [PATCH v2 0/3] leds: simatic-ipc-leds-gpio: split up

[Lee Jones]

On Mon, 15 May 2023, Henning Schild wrote:

> Am Mon, 15 May 2023 10:50:42 +0100
> schrieb Lee Jones <lee@kernel.org>:
> 
> > On Sat, 13 May 2023, Henning Schild wrote:
> > 
> > > Ping.
> > > 
> > > As far as i remember this one should be ready to merge.
> > > 
> > > ACKed by Hans, and Andy had no objections given the maintainers
> > > would accept the patches.
> > > 
> > > So i think it is the maintainers turn now, Pavel and/or Lee i
> > > guess.  
> > 
> > Looks like there are lots of mail threads in reply to each of the
> > patches.  If these culminated in acceptance, please collect all of the
> > Acks you've received until this point and submit a [RESEND].  If there
> > are changes to be made still, please do the same but submit as a new
> > version.
> 
> Ok i will send it again and include the ACK from Hans. There would be no
> code change. Please review the code already, if you find it acceptable
> we might receive a second ACK from Andy.

Not sure what you mean by 'already'.

I shall not be reviewing this set until it is resent, thanks.

-- 
Lee Jones [李琼斯]

Re: [PATCH v2 0/3] leds: simatic-ipc-leds-gpio: split up

[Henning Schild]

Am Mon, 15 May 2023 13:12:47 +0100
schrieb Lee Jones <lee@kernel.org>:

> On Mon, 15 May 2023, Henning Schild wrote:
> 
> > Am Mon, 15 May 2023 10:50:42 +0100
> > schrieb Lee Jones <lee@kernel.org>:
> >   
> > > On Sat, 13 May 2023, Henning Schild wrote:
> > >   
> > > > Ping.
> > > > 
> > > > As far as i remember this one should be ready to merge.
> > > > 
> > > > ACKed by Hans, and Andy had no objections given the maintainers
> > > > would accept the patches.
> > > > 
> > > > So i think it is the maintainers turn now, Pavel and/or Lee i
> > > > guess.    
> > > 
> > > Looks like there are lots of mail threads in reply to each of the
> > > patches.  If these culminated in acceptance, please collect all
> > > of the Acks you've received until this point and submit a
> > > [RESEND].  If there are changes to be made still, please do the
> > > same but submit as a new version.  
> > 
> > Ok i will send it again and include the ACK from Hans. There would
> > be no code change. Please review the code already, if you find it
> > acceptable we might receive a second ACK from Andy.  
> 
> Not sure what you mean by 'already'.
> 
> I shall not be reviewing this set until it is resent, thanks.

I just sent that v3 you requested. It has one Ack from Hans in p3. And
some minor style things from Andy, which i decided to take in when
having to rebase and test once more.

Looking forward to your feedback on that v3.

regards,
Henning

Re: [PATCH v2 0/3] leds: simatic-ipc-leds-gpio: split up

[Lee Jones]

On Mon, 15 May 2023, Henning Schild wrote:

> Am Mon, 15 May 2023 13:12:47 +0100
> schrieb Lee Jones <lee@kernel.org>:
> 
> > On Mon, 15 May 2023, Henning Schild wrote:
> > 
> > > Am Mon, 15 May 2023 10:50:42 +0100
> > > schrieb Lee Jones <lee@kernel.org>:
> > >   
> > > > On Sat, 13 May 2023, Henning Schild wrote:
> > > >   
> > > > > Ping.
> > > > > 
> > > > > As far as i remember this one should be ready to merge.
> > > > > 
> > > > > ACKed by Hans, and Andy had no objections given the maintainers
> > > > > would accept the patches.
> > > > > 
> > > > > So i think it is the maintainers turn now, Pavel and/or Lee i
> > > > > guess.    
> > > > 
> > > > Looks like there are lots of mail threads in reply to each of the
> > > > patches.  If these culminated in acceptance, please collect all
> > > > of the Acks you've received until this point and submit a
> > > > [RESEND].  If there are changes to be made still, please do the
> > > > same but submit as a new version.  
> > > 
> > > Ok i will send it again and include the ACK from Hans. There would
> > > be no code change. Please review the code already, if you find it
> > > acceptable we might receive a second ACK from Andy.  
> > 
> > Not sure what you mean by 'already'.
> > 
> > I shall not be reviewing this set until it is resent, thanks.
> 
> I just sent that v3 you requested. It has one Ack from Hans in p3. And
> some minor style things from Andy, which i decided to take in when
> having to rebase and test once more.
> 
> Looking forward to your feedback on that v3.

Thanks.  It's marked for review.

Just to set expectation, you are number 49 in the queue.

-- 
Lee Jones [李琼斯]