changed since v1:
- rebased
- split p1 into p1-3
This switches the simatic-ipc modules to using the p2sb interface
introduced by Andy with "platform/x86: introduce p2sb_bar() helper".
It also switches to one apollo lake device to using gpio leds.
I am kind of hoping Andy will take this on top and propose it in his
series.
Henning Schild (4):
leds: simatic-ipc-leds: convert to use P2SB accessor
watchdog: simatic-ipc-wdt: convert to use P2SB accessor
platform/x86: simatic-ipc: drop custom P2SB bar code
leds: simatic-ipc-leds-gpio: add GPIO version of Siemens driver
drivers/leds/simple/Kconfig | 11 ++
drivers/leds/simple/Makefile | 3 +-
drivers/leds/simple/simatic-ipc-leds-gpio.c | 108 ++++++++++++++++++
drivers/leds/simple/simatic-ipc-leds.c | 80 +------------
drivers/platform/x86/simatic-ipc.c | 43 +------
drivers/watchdog/Kconfig | 1 +
drivers/watchdog/simatic-ipc-wdt.c | 15 +--
.../platform_data/x86/simatic-ipc-base.h | 2 -
8 files changed, 138 insertions(+), 125 deletions(-)
create mode 100644 drivers/leds/simple/simatic-ipc-leds-gpio.c
--
2.35.1
On Apollo Lake the pinctrl drivers will now come up without ACPI. Use
that instead of open coding it.
Create a new driver for that which can later be filled with more GPIO
based models, and which has different dependencies.
Signed-off-by: Henning Schild <[email protected]>
---
drivers/leds/simple/Kconfig | 12 ++-
drivers/leds/simple/Makefile | 3 +-
drivers/leds/simple/simatic-ipc-leds-gpio.c | 108 ++++++++++++++++++++
drivers/leds/simple/simatic-ipc-leds.c | 80 +--------------
drivers/platform/x86/simatic-ipc.c | 5 +-
5 files changed, 129 insertions(+), 79 deletions(-)
create mode 100644 drivers/leds/simple/simatic-ipc-leds-gpio.c
diff --git a/drivers/leds/simple/Kconfig b/drivers/leds/simple/Kconfig
index 9293e6b36c75..9d2487908743 100644
--- a/drivers/leds/simple/Kconfig
+++ b/drivers/leds/simple/Kconfig
@@ -3,10 +3,20 @@ config LEDS_SIEMENS_SIMATIC_IPC
tristate "LED driver for Siemens Simatic IPCs"
depends on LEDS_CLASS
depends on SIEMENS_SIMATIC_IPC
- select P2SB if X86
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 module
will be called simatic-ipc-leds.
+
+config LEDS_SIEMENS_SIMATIC_IPC_GPIO
+ tristate "LED driver for Siemens Simatic IPCs, GPIO based"
+ depends on SIEMENS_SIMATIC_IPC
+ depends on LEDS_GPIO
+ 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 module
+ will be called simatic-ipc-leds-gpio.
diff --git a/drivers/leds/simple/Makefile b/drivers/leds/simple/Makefile
index 8481f1e9e360..e1df74fb5915 100644
--- a/drivers/leds/simple/Makefile
+++ b/drivers/leds/simple/Makefile
@@ -1,2 +1,3 @@
# 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.o
+obj-$(CONFIG_LEDS_SIEMENS_SIMATIC_IPC_GPIO) += simatic-ipc-leds-gpio.o
diff --git a/drivers/leds/simple/simatic-ipc-leds-gpio.c b/drivers/leds/simple/simatic-ipc-leds-gpio.c
new file mode 100644
index 000000000000..552b65a72e04
--- /dev/null
+++ b/drivers/leds/simple/simatic-ipc-leds-gpio.c
@@ -0,0 +1,108 @@
+// 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>
+
+static struct gpiod_lookup_table simatic_ipc_led_gpio_table = {
+ .dev_id = "leds-gpio",
+ .table = {
+ GPIO_LOOKUP_IDX("apollolake-pinctrl.0", 51, NULL, 0, GPIO_ACTIVE_LOW),
+ GPIO_LOOKUP_IDX("apollolake-pinctrl.0", 52, NULL, 1, GPIO_ACTIVE_LOW),
+ GPIO_LOOKUP_IDX("apollolake-pinctrl.0", 53, NULL, 2, GPIO_ACTIVE_LOW),
+ GPIO_LOOKUP_IDX("apollolake-pinctrl.0", 57, NULL, 3, GPIO_ACTIVE_LOW),
+ GPIO_LOOKUP_IDX("apollolake-pinctrl.0", 58, NULL, 4, GPIO_ACTIVE_LOW),
+ GPIO_LOOKUP_IDX("apollolake-pinctrl.0", 60, NULL, 5, GPIO_ACTIVE_LOW),
+ 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 const struct gpio_led simatic_ipc_gpio_leds[] = {
+ { .name = "green:" LED_FUNCTION_STATUS "-3" },
+ { .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" },
+};
+
+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);
+ platform_device_unregister(simatic_leds_pdev);
+
+ return 0;
+}
+
+static int simatic_ipc_leds_gpio_probe(struct platform_device *pdev)
+{
+ struct gpio_desc *gpiod;
+ int err;
+
+ 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;
+ }
+
+ /* PM_BIOS_BOOT_N */
+ gpiod = gpiod_get_index(&simatic_leds_pdev->dev, NULL, 6, 0);
+ if (IS_ERR(gpiod)) {
+ err = PTR_ERR(gpiod);
+ goto out;
+ }
+ gpiod_set_value(gpiod, 0);
+ gpiod_put(gpiod);
+
+ /* PM_WDT_OUT */
+ gpiod = gpiod_get_index(&simatic_leds_pdev->dev, NULL, 7, 0);
+ if (IS_ERR(gpiod)) {
+ err = PTR_ERR(gpiod);
+ goto out;
+ }
+ gpiod_set_value(gpiod, 0);
+ 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.c b/drivers/leds/simple/simatic-ipc-leds.c
index 2e7597c143d8..4894c228c165 100644
--- a/drivers/leds/simple/simatic-ipc-leds.c
+++ b/drivers/leds/simple/simatic-ipc-leds.c
@@ -15,7 +15,6 @@
#include <linux/leds.h>
#include <linux/module.h>
#include <linux/pci.h>
-#include <linux/platform_data/x86/p2sb.h>
#include <linux/platform_data/x86/simatic-ipc-base.h>
#include <linux/platform_device.h>
#include <linux/sizes.h>
@@ -24,7 +23,7 @@
#define SIMATIC_IPC_LED_PORT_BASE 0x404E
struct simatic_ipc_led {
- unsigned int value; /* mask for io and offset for mem */
+ unsigned int value; /* mask for io */
char *name;
struct led_classdev cdev;
};
@@ -39,21 +38,6 @@ static struct simatic_ipc_led simatic_ipc_leds_io[] = {
{ }
};
-/* the actual start will be discovered with p2sb, 0 is a placeholder */
-static struct resource simatic_ipc_led_mem_res = DEFINE_RES_MEM_NAMED(0, 0, KBUILD_MODNAME);
-
-static void __iomem *simatic_ipc_led_memory;
-
-static struct simatic_ipc_led simatic_ipc_leds_mem[] = {
- {0x500 + 0x1A0, "red:" LED_FUNCTION_STATUS "-1"},
- {0x500 + 0x1A8, "green:" LED_FUNCTION_STATUS "-1"},
- {0x500 + 0x1C8, "red:" LED_FUNCTION_STATUS "-2"},
- {0x500 + 0x1D0, "green:" LED_FUNCTION_STATUS "-2"},
- {0x500 + 0x1E0, "red:" LED_FUNCTION_STATUS "-3"},
- {0x500 + 0x198, "green:" LED_FUNCTION_STATUS "-3"},
- { }
-};
-
static struct resource simatic_ipc_led_io_res =
DEFINE_RES_IO_NAMED(SIMATIC_IPC_LED_PORT_BASE, SZ_2, KBUILD_MODNAME);
@@ -89,28 +73,6 @@ static enum led_brightness simatic_ipc_led_get_io(struct led_classdev *led_cd)
return inw(SIMATIC_IPC_LED_PORT_BASE) & led->value ? LED_OFF : led_cd->max_brightness;
}
-static void simatic_ipc_led_set_mem(struct led_classdev *led_cd,
- enum led_brightness brightness)
-{
- struct simatic_ipc_led *led = cdev_to_led(led_cd);
- void __iomem *reg = simatic_ipc_led_memory + led->value;
- u32 val;
-
- val = readl(reg);
- val = (val & ~1) | (brightness == LED_OFF);
- writel(val, reg);
-}
-
-static enum led_brightness simatic_ipc_led_get_mem(struct led_classdev *led_cd)
-{
- struct simatic_ipc_led *led = cdev_to_led(led_cd);
- void __iomem *reg = simatic_ipc_led_memory + led->value;
- u32 val;
-
- val = readl(reg);
- return (val & 1) ? LED_OFF : led_cd->max_brightness;
-}
-
static int simatic_ipc_leds_probe(struct platform_device *pdev)
{
const struct simatic_ipc_platform *plat = pdev->dev.platform_data;
@@ -118,9 +80,7 @@ static int simatic_ipc_leds_probe(struct platform_device *pdev)
struct simatic_ipc_led *ipcled;
struct led_classdev *cdev;
struct resource *res;
- void __iomem *reg;
- int err, type;
- u32 val;
+ int err;
switch (plat->devmode) {
case SIMATIC_IPC_DEVICE_227D:
@@ -135,51 +95,19 @@ static int simatic_ipc_leds_probe(struct platform_device *pdev)
}
ipcled = simatic_ipc_leds_io;
}
- type = IORESOURCE_IO;
if (!devm_request_region(dev, res->start, resource_size(res), KBUILD_MODNAME)) {
dev_err(dev, "Unable to register IO resource at %pR\n", res);
return -EBUSY;
}
break;
- case SIMATIC_IPC_DEVICE_127E:
- res = &simatic_ipc_led_mem_res;
- ipcled = simatic_ipc_leds_mem;
- type = IORESOURCE_MEM;
-
- err = p2sb_bar(NULL, 0, res);
- if (err)
- return err;
-
- /* do the final address calculation */
- res->start = res->start + (0xC5 << 16);
- res->end = res->start + SZ_4K - 1;
-
- simatic_ipc_led_memory = devm_ioremap_resource(dev, res);
- if (IS_ERR(simatic_ipc_led_memory))
- return PTR_ERR(simatic_ipc_led_memory);
-
- /* initialize power/watchdog LED */
- reg = simatic_ipc_led_memory + 0x500 + 0x1D8; /* PM_WDT_OUT */
- val = readl(reg);
- writel(val & ~1, reg);
-
- reg = simatic_ipc_led_memory + 0x500 + 0x1C0; /* PM_BIOS_BOOT_N */
- val = readl(reg);
- writel(val | 1, reg);
- break;
default:
return -ENODEV;
}
while (ipcled->value) {
cdev = &ipcled->cdev;
- if (type == IORESOURCE_MEM) {
- cdev->brightness_set = simatic_ipc_led_set_mem;
- cdev->brightness_get = simatic_ipc_led_get_mem;
- } else {
- cdev->brightness_set = simatic_ipc_led_set_io;
- cdev->brightness_get = simatic_ipc_led_get_io;
- }
+ cdev->brightness_set = simatic_ipc_led_set_io;
+ cdev->brightness_get = simatic_ipc_led_get_io;
cdev->max_brightness = LED_ON;
cdev->name = ipcled->name;
diff --git a/drivers/platform/x86/simatic-ipc.c b/drivers/platform/x86/simatic-ipc.c
index 26c35e1660cb..ca3647b751d5 100644
--- a/drivers/platform/x86/simatic-ipc.c
+++ b/drivers/platform/x86/simatic-ipc.c
@@ -51,6 +51,7 @@ static int register_platform_devices(u32 station_id)
{
u8 ledmode = SIMATIC_IPC_DEVICE_NONE;
u8 wdtmode = SIMATIC_IPC_DEVICE_NONE;
+ char *pdevname = KBUILD_MODNAME "_leds";
int i;
platform_data.devmode = SIMATIC_IPC_DEVICE_NONE;
@@ -64,10 +65,12 @@ static int register_platform_devices(u32 station_id)
}
if (ledmode != SIMATIC_IPC_DEVICE_NONE) {
+ if (ledmode == SIMATIC_IPC_DEVICE_127E)
+ pdevname = KBUILD_MODNAME "_leds_gpio";
platform_data.devmode = ledmode;
ipc_led_platform_device =
platform_device_register_data(NULL,
- KBUILD_MODNAME "_leds", PLATFORM_DEVID_NONE,
+ pdevname, PLATFORM_DEVID_NONE,
&platform_data,
sizeof(struct simatic_ipc_platform));
if (IS_ERR(ipc_led_platform_device))
--
2.35.1
Since we have a common P2SB accessor in tree we may use it instead of
open coded variants.
Replace custom code by p2sb_bar() call.
Signed-off-by: Henning Schild <[email protected]>
---
drivers/watchdog/Kconfig | 1 +
drivers/watchdog/simatic-ipc-wdt.c | 15 ++++++++-------
2 files changed, 9 insertions(+), 7 deletions(-)
diff --git a/drivers/watchdog/Kconfig b/drivers/watchdog/Kconfig
index c4e82a8d863f..643a8f5a97b1 100644
--- a/drivers/watchdog/Kconfig
+++ b/drivers/watchdog/Kconfig
@@ -1628,6 +1628,7 @@ config SIEMENS_SIMATIC_IPC_WDT
tristate "Siemens Simatic IPC Watchdog"
depends on SIEMENS_SIMATIC_IPC
select WATCHDOG_CORE
+ select P2SB if X86
help
This driver adds support for several watchdogs found in Industrial
PCs from Siemens.
diff --git a/drivers/watchdog/simatic-ipc-wdt.c b/drivers/watchdog/simatic-ipc-wdt.c
index 8bac793c63fb..6599695dc672 100644
--- a/drivers/watchdog/simatic-ipc-wdt.c
+++ b/drivers/watchdog/simatic-ipc-wdt.c
@@ -16,6 +16,7 @@
#include <linux/kernel.h>
#include <linux/module.h>
#include <linux/pci.h>
+#include <linux/platform_data/x86/p2sb.h>
#include <linux/platform_data/x86/simatic-ipc-base.h>
#include <linux/platform_device.h>
#include <linux/sizes.h>
@@ -54,9 +55,9 @@ static struct resource io_resource_trigger =
DEFINE_RES_IO_NAMED(WD_TRIGGER_IOADR, SZ_1,
KBUILD_MODNAME " WD_TRIGGER_IOADR");
-/* the actual start will be discovered with pci, 0 is a placeholder */
+/* the actual start will be discovered with p2sb, 0 is a placeholder */
static struct resource mem_resource =
- DEFINE_RES_MEM_NAMED(0, SZ_4, "WD_RESET_BASE_ADR");
+ DEFINE_RES_MEM_NAMED(0, 0, "WD_RESET_BASE_ADR");
static u32 wd_timeout_table[] = {2, 4, 6, 8, 16, 32, 48, 64 };
static void __iomem *wd_reset_base_addr;
@@ -150,6 +151,7 @@ static int simatic_ipc_wdt_probe(struct platform_device *pdev)
struct simatic_ipc_platform *plat = pdev->dev.platform_data;
struct device *dev = &pdev->dev;
struct resource *res;
+ int ret;
switch (plat->devmode) {
case SIMATIC_IPC_DEVICE_227E:
@@ -190,15 +192,14 @@ static int simatic_ipc_wdt_probe(struct platform_device *pdev)
if (plat->devmode == SIMATIC_IPC_DEVICE_427E) {
res = &mem_resource;
- /* get GPIO base from PCI */
- res->start = simatic_ipc_get_membase0(PCI_DEVFN(0x1f, 1));
- if (res->start == 0)
- return -ENODEV;
+ ret = p2sb_bar(NULL, 0, res);
+ if (ret)
+ return ret;
/* do the final address calculation */
res->start = res->start + (GPIO_COMMUNITY0_PORT_ID << 16) +
PAD_CFG_DW0_GPP_A_23;
- res->end += res->start;
+ res->end = res->start + SZ_4 - 1;
wd_reset_base_addr = devm_ioremap_resource(dev, res);
if (IS_ERR(wd_reset_base_addr))
--
2.35.1
On Wed, May 11, 2022 at 6:40 PM Henning Schild
<[email protected]> wrote:
>
> On Apollo Lake the pinctrl drivers will now come up without ACPI. Use
> that instead of open coding it.
> Create a new driver for that which can later be filled with more GPIO
> based models, and which has different dependencies.
...
> +static struct gpiod_lookup_table simatic_ipc_led_gpio_table = {
> + .dev_id = "leds-gpio",
> + .table = {
> + GPIO_LOOKUP_IDX("apollolake-pinctrl.0", 51, NULL, 0, GPIO_ACTIVE_LOW),
> + GPIO_LOOKUP_IDX("apollolake-pinctrl.0", 52, NULL, 1, GPIO_ACTIVE_LOW),
> + GPIO_LOOKUP_IDX("apollolake-pinctrl.0", 53, NULL, 2, GPIO_ACTIVE_LOW),
> + GPIO_LOOKUP_IDX("apollolake-pinctrl.0", 57, NULL, 3, GPIO_ACTIVE_LOW),
> + GPIO_LOOKUP_IDX("apollolake-pinctrl.0", 58, NULL, 4, GPIO_ACTIVE_LOW),
> + GPIO_LOOKUP_IDX("apollolake-pinctrl.0", 60, NULL, 5, GPIO_ACTIVE_LOW),
> + GPIO_LOOKUP_IDX("apollolake-pinctrl.0", 56, NULL, 6, GPIO_ACTIVE_LOW),
> + GPIO_LOOKUP_IDX("apollolake-pinctrl.0", 59, NULL, 7, GPIO_ACTIVE_HIGH),
> + }
Keeping a comma is good here.
> +};
...
> + /* PM_BIOS_BOOT_N */
> + gpiod = gpiod_get_index(&simatic_leds_pdev->dev, NULL, 6, 0);
This is basically GPIOD_ASIS...
> + if (IS_ERR(gpiod)) {
> + err = PTR_ERR(gpiod);
> + goto out;
> + }
> + gpiod_set_value(gpiod, 0);
...but you may apply GPIOD_OUTPUT_LOW there and drop this call.
> + gpiod_put(gpiod);
Ditto for the rest GPIO requests.
...
> +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,
> + }
> +};
> +
No need to have this blank line.
> +module_platform_driver(simatic_ipc_led_gpio_driver);
--
With Best Regards,
Andy Shevchenko
On 5/11/22 08:39, Henning Schild wrote:
> Since we have a common P2SB accessor in tree we may use it instead of
> open coded variants.
>
> Replace custom code by p2sb_bar() call.
>
> Signed-off-by: Henning Schild <[email protected]>
> ---
> drivers/watchdog/Kconfig | 1 +
> drivers/watchdog/simatic-ipc-wdt.c | 15 ++++++++-------
> 2 files changed, 9 insertions(+), 7 deletions(-)
>
> diff --git a/drivers/watchdog/Kconfig b/drivers/watchdog/Kconfig
> index c4e82a8d863f..643a8f5a97b1 100644
> --- a/drivers/watchdog/Kconfig
> +++ b/drivers/watchdog/Kconfig
> @@ -1628,6 +1628,7 @@ config SIEMENS_SIMATIC_IPC_WDT
> tristate "Siemens Simatic IPC Watchdog"
> depends on SIEMENS_SIMATIC_IPC
> select WATCHDOG_CORE
> + select P2SB if X86
Why "if X86" ? SIEMENS_SIMATIC_IPC already depends on it.
Also, I just noticed that P2SB is neither in mainline nor
in linux-next, meaning this code won't even compile right now.
That should be mentioned in the introduction e-mail (the use
of "introduced" suggests that it is already there; you could
just use "will be introduced" instead).
Thanks,
Guenter
Am Wed, 11 May 2022 17:39:05 +0200
schrieb Henning Schild <[email protected]>:
> On Apollo Lake the pinctrl drivers will now come up without ACPI. Use
> that instead of open coding it.
> Create a new driver for that which can later be filled with more GPIO
> based models, and which has different dependencies.
>
> Signed-off-by: Henning Schild <[email protected]>
> ---
> drivers/leds/simple/Kconfig | 12 ++-
> drivers/leds/simple/Makefile | 3 +-
> drivers/leds/simple/simatic-ipc-leds-gpio.c | 108
> ++++++++++++++++++++ drivers/leds/simple/simatic-ipc-leds.c |
> 80 +-------------- drivers/platform/x86/simatic-ipc.c | 5
> +- 5 files changed, 129 insertions(+), 79 deletions(-)
> create mode 100644 drivers/leds/simple/simatic-ipc-leds-gpio.c
>
> diff --git a/drivers/leds/simple/Kconfig b/drivers/leds/simple/Kconfig
> index 9293e6b36c75..9d2487908743 100644
> --- a/drivers/leds/simple/Kconfig
> +++ b/drivers/leds/simple/Kconfig
> @@ -3,10 +3,20 @@ config LEDS_SIEMENS_SIMATIC_IPC
> tristate "LED driver for Siemens Simatic IPCs"
> depends on LEDS_CLASS
> depends on SIEMENS_SIMATIC_IPC
> - select P2SB if X86
> 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
> module will be called simatic-ipc-leds.
> +
> +config LEDS_SIEMENS_SIMATIC_IPC_GPIO
> + tristate "LED driver for Siemens Simatic IPCs, GPIO based"
> + depends on SIEMENS_SIMATIC_IPC
> + depends on LEDS_GPIO
> + 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
> module
> + will be called simatic-ipc-leds-gpio.
> diff --git a/drivers/leds/simple/Makefile
> b/drivers/leds/simple/Makefile index 8481f1e9e360..e1df74fb5915 100644
> --- a/drivers/leds/simple/Makefile
> +++ b/drivers/leds/simple/Makefile
> @@ -1,2 +1,3 @@
> # 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.o
> +obj-$(CONFIG_LEDS_SIEMENS_SIMATIC_IPC_GPIO) +=
> simatic-ipc-leds-gpio.o diff --git
> a/drivers/leds/simple/simatic-ipc-leds-gpio.c
> b/drivers/leds/simple/simatic-ipc-leds-gpio.c new file mode 100644
> index 000000000000..552b65a72e04 --- /dev/null +++
> b/drivers/leds/simple/simatic-ipc-leds-gpio.c @@ -0,0 +1,108 @@
> +// 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>
> +
> +static struct gpiod_lookup_table simatic_ipc_led_gpio_table = {
> + .dev_id = "leds-gpio",
> + .table = {
> + GPIO_LOOKUP_IDX("apollolake-pinctrl.0", 51, NULL, 0,
> GPIO_ACTIVE_LOW),
> + GPIO_LOOKUP_IDX("apollolake-pinctrl.0", 52, NULL, 1,
> GPIO_ACTIVE_LOW),
> + GPIO_LOOKUP_IDX("apollolake-pinctrl.0", 53, NULL, 2,
> GPIO_ACTIVE_LOW),
> + GPIO_LOOKUP_IDX("apollolake-pinctrl.0", 57, NULL, 3,
> GPIO_ACTIVE_LOW),
> + GPIO_LOOKUP_IDX("apollolake-pinctrl.0", 58, NULL, 4,
> GPIO_ACTIVE_LOW),
> + GPIO_LOOKUP_IDX("apollolake-pinctrl.0", 60, NULL, 5,
> GPIO_ACTIVE_LOW),
> + GPIO_LOOKUP_IDX("apollolake-pinctrl.0", 56, NULL, 6,
> GPIO_ACTIVE_LOW),
> + GPIO_LOOKUP_IDX("apollolake-pinctrl.0", 59, NULL, 7,
> GPIO_ACTIVE_HIGH),
> + }
One thing i found is that rmmod will turn those LEDs off. Not sure how
relevant that is and if someone would ever unload the driver. But i
kind of think the driver should not change the last state of the LEDs,
not on init nor on exit. If there is a way to make that happen i would
probably try and add that.
I guess i might have to look into LED_RETAIN_AT_SHUTDOWN and/or
LED_CORE_SUSPENDRESUME.
A thing to maybe do in the next round, or on top if these patches get
merged. At least the prior implementation, not based on GPIO did
"retain".
regards,
Henning
> +};
> +
> +static const struct gpio_led simatic_ipc_gpio_leds[] = {
> + { .name = "green:" LED_FUNCTION_STATUS "-3" },
> + { .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" },
> +};
> +
> +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);
> + platform_device_unregister(simatic_leds_pdev);
> +
> + return 0;
> +}
> +
> +static int simatic_ipc_leds_gpio_probe(struct platform_device *pdev)
> +{
> + struct gpio_desc *gpiod;
> + int err;
> +
> + 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;
> + }
> +
> + /* PM_BIOS_BOOT_N */
> + gpiod = gpiod_get_index(&simatic_leds_pdev->dev, NULL, 6, 0);
> + if (IS_ERR(gpiod)) {
> + err = PTR_ERR(gpiod);
> + goto out;
> + }
> + gpiod_set_value(gpiod, 0);
> + gpiod_put(gpiod);
> +
> + /* PM_WDT_OUT */
> + gpiod = gpiod_get_index(&simatic_leds_pdev->dev, NULL, 7, 0);
> + if (IS_ERR(gpiod)) {
> + err = PTR_ERR(gpiod);
> + goto out;
> + }
> + gpiod_set_value(gpiod, 0);
> + 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.c
> b/drivers/leds/simple/simatic-ipc-leds.c index
> 2e7597c143d8..4894c228c165 100644 ---
> a/drivers/leds/simple/simatic-ipc-leds.c +++
> b/drivers/leds/simple/simatic-ipc-leds.c @@ -15,7 +15,6 @@
> #include <linux/leds.h>
> #include <linux/module.h>
> #include <linux/pci.h>
> -#include <linux/platform_data/x86/p2sb.h>
> #include <linux/platform_data/x86/simatic-ipc-base.h>
> #include <linux/platform_device.h>
> #include <linux/sizes.h>
> @@ -24,7 +23,7 @@
> #define SIMATIC_IPC_LED_PORT_BASE 0x404E
>
> struct simatic_ipc_led {
> - unsigned int value; /* mask for io and offset for mem */
> + unsigned int value; /* mask for io */
> char *name;
> struct led_classdev cdev;
> };
> @@ -39,21 +38,6 @@ static struct simatic_ipc_led
> simatic_ipc_leds_io[] = { { }
> };
>
> -/* the actual start will be discovered with p2sb, 0 is a placeholder
> */ -static struct resource simatic_ipc_led_mem_res =
> DEFINE_RES_MEM_NAMED(0, 0, KBUILD_MODNAME); -
> -static void __iomem *simatic_ipc_led_memory;
> -
> -static struct simatic_ipc_led simatic_ipc_leds_mem[] = {
> - {0x500 + 0x1A0, "red:" LED_FUNCTION_STATUS "-1"},
> - {0x500 + 0x1A8, "green:" LED_FUNCTION_STATUS "-1"},
> - {0x500 + 0x1C8, "red:" LED_FUNCTION_STATUS "-2"},
> - {0x500 + 0x1D0, "green:" LED_FUNCTION_STATUS "-2"},
> - {0x500 + 0x1E0, "red:" LED_FUNCTION_STATUS "-3"},
> - {0x500 + 0x198, "green:" LED_FUNCTION_STATUS "-3"},
> - { }
> -};
> -
> static struct resource simatic_ipc_led_io_res =
> DEFINE_RES_IO_NAMED(SIMATIC_IPC_LED_PORT_BASE, SZ_2,
> KBUILD_MODNAME);
> @@ -89,28 +73,6 @@ static enum led_brightness
> simatic_ipc_led_get_io(struct led_classdev *led_cd) return
> inw(SIMATIC_IPC_LED_PORT_BASE) & led->value ? LED_OFF :
> led_cd->max_brightness; }
> -static void simatic_ipc_led_set_mem(struct led_classdev *led_cd,
> - enum led_brightness brightness)
> -{
> - struct simatic_ipc_led *led = cdev_to_led(led_cd);
> - void __iomem *reg = simatic_ipc_led_memory + led->value;
> - u32 val;
> -
> - val = readl(reg);
> - val = (val & ~1) | (brightness == LED_OFF);
> - writel(val, reg);
> -}
> -
> -static enum led_brightness simatic_ipc_led_get_mem(struct
> led_classdev *led_cd) -{
> - struct simatic_ipc_led *led = cdev_to_led(led_cd);
> - void __iomem *reg = simatic_ipc_led_memory + led->value;
> - u32 val;
> -
> - val = readl(reg);
> - return (val & 1) ? LED_OFF : led_cd->max_brightness;
> -}
> -
> static int simatic_ipc_leds_probe(struct platform_device *pdev)
> {
> const struct simatic_ipc_platform *plat =
> pdev->dev.platform_data; @@ -118,9 +80,7 @@ static int
> simatic_ipc_leds_probe(struct platform_device *pdev) struct
> simatic_ipc_led *ipcled; struct led_classdev *cdev;
> struct resource *res;
> - void __iomem *reg;
> - int err, type;
> - u32 val;
> + int err;
>
> switch (plat->devmode) {
> case SIMATIC_IPC_DEVICE_227D:
> @@ -135,51 +95,19 @@ static int simatic_ipc_leds_probe(struct
> platform_device *pdev) }
> ipcled = simatic_ipc_leds_io;
> }
> - type = IORESOURCE_IO;
> if (!devm_request_region(dev, res->start,
> resource_size(res), KBUILD_MODNAME)) { dev_err(dev, "Unable to
> register IO resource at %pR\n", res); return -EBUSY;
> }
> break;
> - case SIMATIC_IPC_DEVICE_127E:
> - res = &simatic_ipc_led_mem_res;
> - ipcled = simatic_ipc_leds_mem;
> - type = IORESOURCE_MEM;
> -
> - err = p2sb_bar(NULL, 0, res);
> - if (err)
> - return err;
> -
> - /* do the final address calculation */
> - res->start = res->start + (0xC5 << 16);
> - res->end = res->start + SZ_4K - 1;
> -
> - simatic_ipc_led_memory = devm_ioremap_resource(dev,
> res);
> - if (IS_ERR(simatic_ipc_led_memory))
> - return PTR_ERR(simatic_ipc_led_memory);
> -
> - /* initialize power/watchdog LED */
> - reg = simatic_ipc_led_memory + 0x500 + 0x1D8; /*
> PM_WDT_OUT */
> - val = readl(reg);
> - writel(val & ~1, reg);
> -
> - reg = simatic_ipc_led_memory + 0x500 + 0x1C0; /*
> PM_BIOS_BOOT_N */
> - val = readl(reg);
> - writel(val | 1, reg);
> - break;
> default:
> return -ENODEV;
> }
>
> while (ipcled->value) {
> cdev = &ipcled->cdev;
> - if (type == IORESOURCE_MEM) {
> - cdev->brightness_set =
> simatic_ipc_led_set_mem;
> - cdev->brightness_get =
> simatic_ipc_led_get_mem;
> - } else {
> - cdev->brightness_set =
> simatic_ipc_led_set_io;
> - cdev->brightness_get =
> simatic_ipc_led_get_io;
> - }
> + cdev->brightness_set = simatic_ipc_led_set_io;
> + cdev->brightness_get = simatic_ipc_led_get_io;
> cdev->max_brightness = LED_ON;
> cdev->name = ipcled->name;
>
> diff --git a/drivers/platform/x86/simatic-ipc.c
> b/drivers/platform/x86/simatic-ipc.c index 26c35e1660cb..ca3647b751d5
> 100644 --- a/drivers/platform/x86/simatic-ipc.c
> +++ b/drivers/platform/x86/simatic-ipc.c
> @@ -51,6 +51,7 @@ static int register_platform_devices(u32 station_id)
> {
> u8 ledmode = SIMATIC_IPC_DEVICE_NONE;
> u8 wdtmode = SIMATIC_IPC_DEVICE_NONE;
> + char *pdevname = KBUILD_MODNAME "_leds";
> int i;
>
> platform_data.devmode = SIMATIC_IPC_DEVICE_NONE;
> @@ -64,10 +65,12 @@ static int register_platform_devices(u32
> station_id) }
>
> if (ledmode != SIMATIC_IPC_DEVICE_NONE) {
> + if (ledmode == SIMATIC_IPC_DEVICE_127E)
> + pdevname = KBUILD_MODNAME "_leds_gpio";
> platform_data.devmode = ledmode;
> ipc_led_platform_device =
> platform_device_register_data(NULL,
> - KBUILD_MODNAME "_leds",
> PLATFORM_DEVID_NONE,
> + pdevname, PLATFORM_DEVID_NONE,
> &platform_data,
> sizeof(struct simatic_ipc_platform));
> if (IS_ERR(ipc_led_platform_device))
Since we have a common P2SB accessor in tree we may use it instead of
open coded variants.
Replace custom code by p2sb_bar() call.
Signed-off-by: Henning Schild <[email protected]>
---
drivers/leds/simple/Kconfig | 1 +
drivers/leds/simple/simatic-ipc-leds.c | 14 +++++++-------
2 files changed, 8 insertions(+), 7 deletions(-)
diff --git a/drivers/leds/simple/Kconfig b/drivers/leds/simple/Kconfig
index 9f6a68336659..9293e6b36c75 100644
--- a/drivers/leds/simple/Kconfig
+++ b/drivers/leds/simple/Kconfig
@@ -3,6 +3,7 @@ config LEDS_SIEMENS_SIMATIC_IPC
tristate "LED driver for Siemens Simatic IPCs"
depends on LEDS_CLASS
depends on SIEMENS_SIMATIC_IPC
+ select P2SB if X86
help
This option enables support for the LEDs of several Industrial PCs
from Siemens.
diff --git a/drivers/leds/simple/simatic-ipc-leds.c b/drivers/leds/simple/simatic-ipc-leds.c
index 078d43f5ba38..2e7597c143d8 100644
--- a/drivers/leds/simple/simatic-ipc-leds.c
+++ b/drivers/leds/simple/simatic-ipc-leds.c
@@ -15,6 +15,7 @@
#include <linux/leds.h>
#include <linux/module.h>
#include <linux/pci.h>
+#include <linux/platform_data/x86/p2sb.h>
#include <linux/platform_data/x86/simatic-ipc-base.h>
#include <linux/platform_device.h>
#include <linux/sizes.h>
@@ -38,8 +39,8 @@ static struct simatic_ipc_led simatic_ipc_leds_io[] = {
{ }
};
-/* the actual start will be discovered with PCI, 0 is a placeholder */
-static struct resource simatic_ipc_led_mem_res = DEFINE_RES_MEM_NAMED(0, SZ_4K, KBUILD_MODNAME);
+/* the actual start will be discovered with p2sb, 0 is a placeholder */
+static struct resource simatic_ipc_led_mem_res = DEFINE_RES_MEM_NAMED(0, 0, KBUILD_MODNAME);
static void __iomem *simatic_ipc_led_memory;
@@ -145,14 +146,13 @@ static int simatic_ipc_leds_probe(struct platform_device *pdev)
ipcled = simatic_ipc_leds_mem;
type = IORESOURCE_MEM;
- /* get GPIO base from PCI */
- res->start = simatic_ipc_get_membase0(PCI_DEVFN(13, 0));
- if (res->start == 0)
- return -ENODEV;
+ err = p2sb_bar(NULL, 0, res);
+ if (err)
+ return err;
/* do the final address calculation */
res->start = res->start + (0xC5 << 16);
- res->end += res->start;
+ res->end = res->start + SZ_4K - 1;
simatic_ipc_led_memory = devm_ioremap_resource(dev, res);
if (IS_ERR(simatic_ipc_led_memory))
--
2.35.1
On Thu, May 12, 2022 at 10:58:36AM +0200, Henning Schild wrote:
> Am Wed, 11 May 2022 11:03:04 -0700
> schrieb Guenter Roeck <[email protected]>:
> > On 5/11/22 08:39, Henning Schild wrote:
...
> > Also, I just noticed that P2SB is neither in mainline nor
> > in linux-next, meaning this code won't even compile right now.
> > That should be mentioned in the introduction e-mail (the use
> > of "introduced" suggests that it is already there; you could
> > just use "will be introduced" instead).
>
> It was kind of in the cover letter, but maybe not verbose enough. Sorry
> for the confusion. v1 was sent in-reply-to on top of P2SB, maybe i will
> do that again for v3 but for sure point out the order in case i send it
> without the reply.
A new version of a series should start a new thread. Just use cover letter
to explain the dependencies. My expectations here is to see v3 with comments
addressed and I will incorporate it into v6 of mine. Then LKP and other
bots will be happy to test all bunch. Also, I would wait a bit for your v3,
so maintainers will have time to give their tags and/or comments.
--
With Best Regards,
Andy Shevchenko
The two drivers that used to use this have been switched over to the
common P2SB accessor, so this code is not needed any longer.
Signed-off-by: Henning Schild <[email protected]>
---
drivers/platform/x86/simatic-ipc.c | 38 -------------------
.../platform_data/x86/simatic-ipc-base.h | 2 -
2 files changed, 40 deletions(-)
diff --git a/drivers/platform/x86/simatic-ipc.c b/drivers/platform/x86/simatic-ipc.c
index b599cda5ba3c..26c35e1660cb 100644
--- a/drivers/platform/x86/simatic-ipc.c
+++ b/drivers/platform/x86/simatic-ipc.c
@@ -101,44 +101,6 @@ static int register_platform_devices(u32 station_id)
return 0;
}
-/* FIXME: this should eventually be done with generic P2SB discovery code
- * the individual drivers for watchdogs and LEDs access memory that implements
- * GPIO, but pinctrl will not come up because of missing ACPI entries
- *
- * While there is no conflict a cleaner solution would be to somehow bring up
- * pinctrl even with these ACPI entries missing, and base the drivers on pinctrl.
- * After which the following function could be dropped, together with the code
- * poking the memory.
- */
-/*
- * Get membase address from PCI, used in leds and wdt module. Here we read
- * the bar0. The final address calculation is done in the appropriate modules
- */
-u32 simatic_ipc_get_membase0(unsigned int p2sb)
-{
- struct pci_bus *bus;
- u32 bar0 = 0;
- /*
- * The GPIO memory is in bar0 of the hidden P2SB device.
- * Unhide the device to have a quick look at it, before we hide it
- * again.
- * Also grab the pci rescan lock so that device does not get discovered
- * and remapped while it is visible.
- * This code is inspired by drivers/mfd/lpc_ich.c
- */
- bus = pci_find_bus(0, 0);
- pci_lock_rescan_remove();
- pci_bus_write_config_byte(bus, p2sb, 0xE1, 0x0);
- pci_bus_read_config_dword(bus, p2sb, PCI_BASE_ADDRESS_0, &bar0);
-
- bar0 &= ~0xf;
- pci_bus_write_config_byte(bus, p2sb, 0xE1, 0x1);
- pci_unlock_rescan_remove();
-
- return bar0;
-}
-EXPORT_SYMBOL(simatic_ipc_get_membase0);
-
static int __init simatic_ipc_init_module(void)
{
const struct dmi_system_id *match;
diff --git a/include/linux/platform_data/x86/simatic-ipc-base.h b/include/linux/platform_data/x86/simatic-ipc-base.h
index 62d2bc774067..39fefd48cf4d 100644
--- a/include/linux/platform_data/x86/simatic-ipc-base.h
+++ b/include/linux/platform_data/x86/simatic-ipc-base.h
@@ -24,6 +24,4 @@ struct simatic_ipc_platform {
u8 devmode;
};
-u32 simatic_ipc_get_membase0(unsigned int p2sb);
-
#endif /* __PLATFORM_DATA_X86_SIMATIC_IPC_BASE_H */
--
2.35.1
Hi Henning,
Thank you for the patch! Yet something to improve:
[auto build test ERROR on pavel-leds/for-next]
[also build test ERROR on groeck-staging/hwmon-next linus/master v5.18-rc6 next-20220511]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch]
url: https://github.com/intel-lab-lkp/linux/commits/Henning-Schild/simatic-ipc-additions-to-p2sb-apl-lake-gpio/20220511-234148
base: git://git.kernel.org/pub/scm/linux/kernel/git/pavel/linux-leds.git for-next
config: i386-randconfig-a004-20220509 (https://download.01.org/0day-ci/archive/20220512/[email protected]/config)
compiler: clang version 15.0.0 (https://github.com/llvm/llvm-project 18dd123c56754edf62c7042dcf23185c3727610f)
reproduce (this is a W=1 build):
wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
chmod +x ~/bin/make.cross
# https://github.com/intel-lab-lkp/linux/commit/f9374205615fb91a7d289a6acaeafcd5f9c16ac4
git remote add linux-review https://github.com/intel-lab-lkp/linux
git fetch --no-tags linux-review Henning-Schild/simatic-ipc-additions-to-p2sb-apl-lake-gpio/20220511-234148
git checkout f9374205615fb91a7d289a6acaeafcd5f9c16ac4
# save the config file
mkdir build_dir && cp config build_dir/.config
COMPILER_INSTALL_PATH=$HOME/0day COMPILER=clang make.cross W=1 O=build_dir ARCH=i386 SHELL=/bin/bash drivers/
If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot <[email protected]>
All errors (new ones prefixed by >>):
>> drivers/watchdog/simatic-ipc-wdt.c:19:10: fatal error: 'linux/platform_data/x86/p2sb.h' file not found
#include <linux/platform_data/x86/p2sb.h>
^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
1 error generated.
vim +19 drivers/watchdog/simatic-ipc-wdt.c
> 19 #include <linux/platform_data/x86/p2sb.h>
20 #include <linux/platform_data/x86/simatic-ipc-base.h>
21 #include <linux/platform_device.h>
22 #include <linux/sizes.h>
23 #include <linux/util_macros.h>
24 #include <linux/watchdog.h>
25
--
0-DAY CI Kernel Test Service
https://01.org/lkp
On Thu, May 12, 2022 at 10:44:24AM +0200, Henning Schild wrote:
> Am Wed, 11 May 2022 17:39:05 +0200
> schrieb Henning Schild <[email protected]>:
...
> > +config LEDS_SIEMENS_SIMATIC_IPC_GPIO
>
> I wonder if i really should introduce a new switch here or just carry
> this one under LEDS_SIEMENS_SIMATIC_IPC
>
> For a v3 i will likely put two modules under that one config switch.
For me either is okay, but I'm not a LED maintainer.
--
With Best Regards,
Andy Shevchenko
Am Wed, 11 May 2022 17:39:05 +0200
schrieb Henning Schild <[email protected]>:
> On Apollo Lake the pinctrl drivers will now come up without ACPI. Use
> that instead of open coding it.
> Create a new driver for that which can later be filled with more GPIO
> based models, and which has different dependencies.
>
> Signed-off-by: Henning Schild <[email protected]>
> ---
> drivers/leds/simple/Kconfig | 12 ++-
> drivers/leds/simple/Makefile | 3 +-
> drivers/leds/simple/simatic-ipc-leds-gpio.c | 108
> ++++++++++++++++++++ drivers/leds/simple/simatic-ipc-leds.c |
> 80 +-------------- drivers/platform/x86/simatic-ipc.c | 5
> +- 5 files changed, 129 insertions(+), 79 deletions(-)
> create mode 100644 drivers/leds/simple/simatic-ipc-leds-gpio.c
>
> diff --git a/drivers/leds/simple/Kconfig b/drivers/leds/simple/Kconfig
> index 9293e6b36c75..9d2487908743 100644
> --- a/drivers/leds/simple/Kconfig
> +++ b/drivers/leds/simple/Kconfig
> @@ -3,10 +3,20 @@ config LEDS_SIEMENS_SIMATIC_IPC
> tristate "LED driver for Siemens Simatic IPCs"
> depends on LEDS_CLASS
> depends on SIEMENS_SIMATIC_IPC
> - select P2SB if X86
> 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
> module will be called simatic-ipc-leds.
> +
> +config LEDS_SIEMENS_SIMATIC_IPC_GPIO
I wonder if i really should introduce a new switch here or just carry
this one under LEDS_SIEMENS_SIMATIC_IPC
For a v3 i will likely put two modules under that one config switch.
Henning
> + tristate "LED driver for Siemens Simatic IPCs, GPIO based"
> + depends on SIEMENS_SIMATIC_IPC
> + depends on LEDS_GPIO
> + 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
> module
> + will be called simatic-ipc-leds-gpio.
> diff --git a/drivers/leds/simple/Makefile
> b/drivers/leds/simple/Makefile index 8481f1e9e360..e1df74fb5915 100644
> --- a/drivers/leds/simple/Makefile
> +++ b/drivers/leds/simple/Makefile
> @@ -1,2 +1,3 @@
> # 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.o
> +obj-$(CONFIG_LEDS_SIEMENS_SIMATIC_IPC_GPIO) +=
> simatic-ipc-leds-gpio.o diff --git
> a/drivers/leds/simple/simatic-ipc-leds-gpio.c
> b/drivers/leds/simple/simatic-ipc-leds-gpio.c new file mode 100644
> index 000000000000..552b65a72e04 --- /dev/null +++
> b/drivers/leds/simple/simatic-ipc-leds-gpio.c @@ -0,0 +1,108 @@
> +// 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>
> +
> +static struct gpiod_lookup_table simatic_ipc_led_gpio_table = {
> + .dev_id = "leds-gpio",
> + .table = {
> + GPIO_LOOKUP_IDX("apollolake-pinctrl.0", 51, NULL, 0,
> GPIO_ACTIVE_LOW),
> + GPIO_LOOKUP_IDX("apollolake-pinctrl.0", 52, NULL, 1,
> GPIO_ACTIVE_LOW),
> + GPIO_LOOKUP_IDX("apollolake-pinctrl.0", 53, NULL, 2,
> GPIO_ACTIVE_LOW),
> + GPIO_LOOKUP_IDX("apollolake-pinctrl.0", 57, NULL, 3,
> GPIO_ACTIVE_LOW),
> + GPIO_LOOKUP_IDX("apollolake-pinctrl.0", 58, NULL, 4,
> GPIO_ACTIVE_LOW),
> + GPIO_LOOKUP_IDX("apollolake-pinctrl.0", 60, NULL, 5,
> GPIO_ACTIVE_LOW),
> + 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 const struct gpio_led simatic_ipc_gpio_leds[] = {
> + { .name = "green:" LED_FUNCTION_STATUS "-3" },
> + { .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" },
> +};
> +
> +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);
> + platform_device_unregister(simatic_leds_pdev);
> +
> + return 0;
> +}
> +
> +static int simatic_ipc_leds_gpio_probe(struct platform_device *pdev)
> +{
> + struct gpio_desc *gpiod;
> + int err;
> +
> + 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;
> + }
> +
> + /* PM_BIOS_BOOT_N */
> + gpiod = gpiod_get_index(&simatic_leds_pdev->dev, NULL, 6, 0);
> + if (IS_ERR(gpiod)) {
> + err = PTR_ERR(gpiod);
> + goto out;
> + }
> + gpiod_set_value(gpiod, 0);
> + gpiod_put(gpiod);
> +
> + /* PM_WDT_OUT */
> + gpiod = gpiod_get_index(&simatic_leds_pdev->dev, NULL, 7, 0);
> + if (IS_ERR(gpiod)) {
> + err = PTR_ERR(gpiod);
> + goto out;
> + }
> + gpiod_set_value(gpiod, 0);
> + 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.c
> b/drivers/leds/simple/simatic-ipc-leds.c index
> 2e7597c143d8..4894c228c165 100644 ---
> a/drivers/leds/simple/simatic-ipc-leds.c +++
> b/drivers/leds/simple/simatic-ipc-leds.c @@ -15,7 +15,6 @@
> #include <linux/leds.h>
> #include <linux/module.h>
> #include <linux/pci.h>
> -#include <linux/platform_data/x86/p2sb.h>
> #include <linux/platform_data/x86/simatic-ipc-base.h>
> #include <linux/platform_device.h>
> #include <linux/sizes.h>
> @@ -24,7 +23,7 @@
> #define SIMATIC_IPC_LED_PORT_BASE 0x404E
>
> struct simatic_ipc_led {
> - unsigned int value; /* mask for io and offset for mem */
> + unsigned int value; /* mask for io */
> char *name;
> struct led_classdev cdev;
> };
> @@ -39,21 +38,6 @@ static struct simatic_ipc_led
> simatic_ipc_leds_io[] = { { }
> };
>
> -/* the actual start will be discovered with p2sb, 0 is a placeholder
> */ -static struct resource simatic_ipc_led_mem_res =
> DEFINE_RES_MEM_NAMED(0, 0, KBUILD_MODNAME); -
> -static void __iomem *simatic_ipc_led_memory;
> -
> -static struct simatic_ipc_led simatic_ipc_leds_mem[] = {
> - {0x500 + 0x1A0, "red:" LED_FUNCTION_STATUS "-1"},
> - {0x500 + 0x1A8, "green:" LED_FUNCTION_STATUS "-1"},
> - {0x500 + 0x1C8, "red:" LED_FUNCTION_STATUS "-2"},
> - {0x500 + 0x1D0, "green:" LED_FUNCTION_STATUS "-2"},
> - {0x500 + 0x1E0, "red:" LED_FUNCTION_STATUS "-3"},
> - {0x500 + 0x198, "green:" LED_FUNCTION_STATUS "-3"},
> - { }
> -};
> -
> static struct resource simatic_ipc_led_io_res =
> DEFINE_RES_IO_NAMED(SIMATIC_IPC_LED_PORT_BASE, SZ_2,
> KBUILD_MODNAME);
> @@ -89,28 +73,6 @@ static enum led_brightness
> simatic_ipc_led_get_io(struct led_classdev *led_cd) return
> inw(SIMATIC_IPC_LED_PORT_BASE) & led->value ? LED_OFF :
> led_cd->max_brightness; }
> -static void simatic_ipc_led_set_mem(struct led_classdev *led_cd,
> - enum led_brightness brightness)
> -{
> - struct simatic_ipc_led *led = cdev_to_led(led_cd);
> - void __iomem *reg = simatic_ipc_led_memory + led->value;
> - u32 val;
> -
> - val = readl(reg);
> - val = (val & ~1) | (brightness == LED_OFF);
> - writel(val, reg);
> -}
> -
> -static enum led_brightness simatic_ipc_led_get_mem(struct
> led_classdev *led_cd) -{
> - struct simatic_ipc_led *led = cdev_to_led(led_cd);
> - void __iomem *reg = simatic_ipc_led_memory + led->value;
> - u32 val;
> -
> - val = readl(reg);
> - return (val & 1) ? LED_OFF : led_cd->max_brightness;
> -}
> -
> static int simatic_ipc_leds_probe(struct platform_device *pdev)
> {
> const struct simatic_ipc_platform *plat =
> pdev->dev.platform_data; @@ -118,9 +80,7 @@ static int
> simatic_ipc_leds_probe(struct platform_device *pdev) struct
> simatic_ipc_led *ipcled; struct led_classdev *cdev;
> struct resource *res;
> - void __iomem *reg;
> - int err, type;
> - u32 val;
> + int err;
>
> switch (plat->devmode) {
> case SIMATIC_IPC_DEVICE_227D:
> @@ -135,51 +95,19 @@ static int simatic_ipc_leds_probe(struct
> platform_device *pdev) }
> ipcled = simatic_ipc_leds_io;
> }
> - type = IORESOURCE_IO;
> if (!devm_request_region(dev, res->start,
> resource_size(res), KBUILD_MODNAME)) { dev_err(dev, "Unable to
> register IO resource at %pR\n", res); return -EBUSY;
> }
> break;
> - case SIMATIC_IPC_DEVICE_127E:
> - res = &simatic_ipc_led_mem_res;
> - ipcled = simatic_ipc_leds_mem;
> - type = IORESOURCE_MEM;
> -
> - err = p2sb_bar(NULL, 0, res);
> - if (err)
> - return err;
> -
> - /* do the final address calculation */
> - res->start = res->start + (0xC5 << 16);
> - res->end = res->start + SZ_4K - 1;
> -
> - simatic_ipc_led_memory = devm_ioremap_resource(dev,
> res);
> - if (IS_ERR(simatic_ipc_led_memory))
> - return PTR_ERR(simatic_ipc_led_memory);
> -
> - /* initialize power/watchdog LED */
> - reg = simatic_ipc_led_memory + 0x500 + 0x1D8; /*
> PM_WDT_OUT */
> - val = readl(reg);
> - writel(val & ~1, reg);
> -
> - reg = simatic_ipc_led_memory + 0x500 + 0x1C0; /*
> PM_BIOS_BOOT_N */
> - val = readl(reg);
> - writel(val | 1, reg);
> - break;
> default:
> return -ENODEV;
> }
>
> while (ipcled->value) {
> cdev = &ipcled->cdev;
> - if (type == IORESOURCE_MEM) {
> - cdev->brightness_set =
> simatic_ipc_led_set_mem;
> - cdev->brightness_get =
> simatic_ipc_led_get_mem;
> - } else {
> - cdev->brightness_set =
> simatic_ipc_led_set_io;
> - cdev->brightness_get =
> simatic_ipc_led_get_io;
> - }
> + cdev->brightness_set = simatic_ipc_led_set_io;
> + cdev->brightness_get = simatic_ipc_led_get_io;
> cdev->max_brightness = LED_ON;
> cdev->name = ipcled->name;
>
> diff --git a/drivers/platform/x86/simatic-ipc.c
> b/drivers/platform/x86/simatic-ipc.c index 26c35e1660cb..ca3647b751d5
> 100644 --- a/drivers/platform/x86/simatic-ipc.c
> +++ b/drivers/platform/x86/simatic-ipc.c
> @@ -51,6 +51,7 @@ static int register_platform_devices(u32 station_id)
> {
> u8 ledmode = SIMATIC_IPC_DEVICE_NONE;
> u8 wdtmode = SIMATIC_IPC_DEVICE_NONE;
> + char *pdevname = KBUILD_MODNAME "_leds";
> int i;
>
> platform_data.devmode = SIMATIC_IPC_DEVICE_NONE;
> @@ -64,10 +65,12 @@ static int register_platform_devices(u32
> station_id) }
>
> if (ledmode != SIMATIC_IPC_DEVICE_NONE) {
> + if (ledmode == SIMATIC_IPC_DEVICE_127E)
> + pdevname = KBUILD_MODNAME "_leds_gpio";
> platform_data.devmode = ledmode;
> ipc_led_platform_device =
> platform_device_register_data(NULL,
> - KBUILD_MODNAME "_leds",
> PLATFORM_DEVID_NONE,
> + pdevname, PLATFORM_DEVID_NONE,
> &platform_data,
> sizeof(struct simatic_ipc_platform));
> if (IS_ERR(ipc_led_platform_device))
Am Wed, 11 May 2022 11:03:04 -0700
schrieb Guenter Roeck <[email protected]>:
> On 5/11/22 08:39, Henning Schild wrote:
> > Since we have a common P2SB accessor in tree we may use it instead
> > of open coded variants.
> >
> > Replace custom code by p2sb_bar() call.
> >
> > Signed-off-by: Henning Schild <[email protected]>
> > ---
> > drivers/watchdog/Kconfig | 1 +
> > drivers/watchdog/simatic-ipc-wdt.c | 15 ++++++++-------
> > 2 files changed, 9 insertions(+), 7 deletions(-)
> >
> > diff --git a/drivers/watchdog/Kconfig b/drivers/watchdog/Kconfig
> > index c4e82a8d863f..643a8f5a97b1 100644
> > --- a/drivers/watchdog/Kconfig
> > +++ b/drivers/watchdog/Kconfig
> > @@ -1628,6 +1628,7 @@ config SIEMENS_SIMATIC_IPC_WDT
> > tristate "Siemens Simatic IPC Watchdog"
> > depends on SIEMENS_SIMATIC_IPC
> > select WATCHDOG_CORE
> > + select P2SB if X86
>
> Why "if X86" ? SIEMENS_SIMATIC_IPC already depends on it.
Thanks, will remove that.
> Also, I just noticed that P2SB is neither in mainline nor
> in linux-next, meaning this code won't even compile right now.
> That should be mentioned in the introduction e-mail (the use
> of "introduced" suggests that it is already there; you could
> just use "will be introduced" instead).
It was kind of in the cover letter, but maybe not verbose enough. Sorry
for the confusion. v1 was sent in-reply-to on top of P2SB, maybe i will
do that again for v3 but for sure point out the order in case i send it
without the reply.
Thanks,
Henning
> Thanks,
> Guenter