2021-11-26 14:13:08

by Henning Schild

[permalink] [raw]
Subject: [PATCH v4 2/4] leds: simatic-ipc-leds: add new driver for Siemens Industial PCs

This driver adds initial support for several devices from Siemens. It is
based on a platform driver introduced in an earlier commit.

Signed-off-by: Henning Schild <[email protected]>
---
drivers/leds/Kconfig | 3 +
drivers/leds/Makefile | 3 +
drivers/leds/simple/Kconfig | 11 ++
drivers/leds/simple/Makefile | 2 +
drivers/leds/simple/simatic-ipc-leds.c | 202 +++++++++++++++++++++++++
5 files changed, 221 insertions(+)
create mode 100644 drivers/leds/simple/Kconfig
create mode 100644 drivers/leds/simple/Makefile
create mode 100644 drivers/leds/simple/simatic-ipc-leds.c

diff --git a/drivers/leds/Kconfig b/drivers/leds/Kconfig
index ed800f5da7d8..ac6688d7a3f4 100644
--- a/drivers/leds/Kconfig
+++ b/drivers/leds/Kconfig
@@ -879,4 +879,7 @@ source "drivers/leds/flash/Kconfig"
comment "LED Triggers"
source "drivers/leds/trigger/Kconfig"

+comment "Simple LED drivers"
+source "drivers/leds/simple/Kconfig"
+
endif # NEW_LEDS
diff --git a/drivers/leds/Makefile b/drivers/leds/Makefile
index c636ec069612..1a719caf14c0 100644
--- a/drivers/leds/Makefile
+++ b/drivers/leds/Makefile
@@ -105,3 +105,6 @@ obj-$(CONFIG_LEDS_TRIGGERS) += trigger/

# LED Blink
obj-y += blink/
+
+# Simple LED drivers
+obj-y += simple/
diff --git a/drivers/leds/simple/Kconfig b/drivers/leds/simple/Kconfig
new file mode 100644
index 000000000000..9f6a68336659
--- /dev/null
+++ b/drivers/leds/simple/Kconfig
@@ -0,0 +1,11 @@
+# SPDX-License-Identifier: GPL-2.0-only
+config LEDS_SIEMENS_SIMATIC_IPC
+ tristate "LED driver for Siemens Simatic IPCs"
+ depends on LEDS_CLASS
+ 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 module
+ will be called simatic-ipc-leds.
diff --git a/drivers/leds/simple/Makefile b/drivers/leds/simple/Makefile
new file mode 100644
index 000000000000..8481f1e9e360
--- /dev/null
+++ b/drivers/leds/simple/Makefile
@@ -0,0 +1,2 @@
+# SPDX-License-Identifier: GPL-2.0
+obj-$(CONFIG_LEDS_SIEMENS_SIMATIC_IPC) += simatic-ipc-leds.o
diff --git a/drivers/leds/simple/simatic-ipc-leds.c b/drivers/leds/simple/simatic-ipc-leds.c
new file mode 100644
index 000000000000..ff2c96e73241
--- /dev/null
+++ b/drivers/leds/simple/simatic-ipc-leds.c
@@ -0,0 +1,202 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * Siemens SIMATIC IPC driver for LEDs
+ *
+ * Copyright (c) Siemens AG, 2018-2021
+ *
+ * Authors:
+ * Henning Schild <[email protected]>
+ * Jan Kiszka <[email protected]>
+ * Gerd Haeussler <[email protected]>
+ */
+
+#include <linux/ioport.h>
+#include <linux/kernel.h>
+#include <linux/leds.h>
+#include <linux/module.h>
+#include <linux/pci.h>
+#include <linux/platform_data/x86/simatic-ipc-base.h>
+#include <linux/platform_device.h>
+#include <linux/sizes.h>
+#include <linux/spinlock.h>
+
+#define SIMATIC_IPC_LED_PORT_BASE 0x404E
+
+struct simatic_ipc_led {
+ unsigned int value; /* mask for io and offset for mem */
+ char *name;
+ struct led_classdev cdev;
+};
+
+static struct simatic_ipc_led simatic_ipc_leds_io[] = {
+ {1 << 15, "green:" LED_FUNCTION_STATUS "-1" },
+ {1 << 7, "yellow:" LED_FUNCTION_STATUS "-1" },
+ {1 << 14, "red:" LED_FUNCTION_STATUS "-2" },
+ {1 << 6, "yellow:" LED_FUNCTION_STATUS "-2" },
+ {1 << 13, "red:" LED_FUNCTION_STATUS "-3" },
+ {1 << 5, "yellow:" LED_FUNCTION_STATUS "-3" },
+ { }
+};
+
+/* the actual start will be discovered with PCI, 0 is a placeholder */
+struct resource simatic_ipc_led_mem_res = DEFINE_RES_MEM_NAMED(0, SZ_4K, KBUILD_MODNAME);
+
+static void *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);
+
+static DEFINE_SPINLOCK(reg_lock);
+
+static inline struct simatic_ipc_led *cdev_to_led(struct led_classdev *led_cd)
+{
+ return container_of(led_cd, struct simatic_ipc_led, cdev);
+}
+
+static void simatic_ipc_led_set_io(struct led_classdev *led_cd,
+ enum led_brightness brightness)
+{
+ struct simatic_ipc_led *led = cdev_to_led(led_cd);
+ unsigned long flags;
+ unsigned int val;
+
+ spin_lock_irqsave(&reg_lock, flags);
+
+ val = inw(SIMATIC_IPC_LED_PORT_BASE);
+ if (brightness == LED_OFF)
+ outw(val | led->value, SIMATIC_IPC_LED_PORT_BASE);
+ else
+ outw(val & ~led->value, SIMATIC_IPC_LED_PORT_BASE);
+
+ spin_unlock_irqrestore(&reg_lock, flags);
+}
+
+static enum led_brightness simatic_ipc_led_get_io(struct led_classdev *led_cd)
+{
+ struct simatic_ipc_led *led = cdev_to_led(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);
+
+ u32 *p;
+
+ p = simatic_ipc_led_memory + led->value;
+ *p = (*p & ~1) | (brightness == LED_OFF);
+}
+
+static enum led_brightness simatic_ipc_led_get_mem(struct led_classdev *led_cd)
+{
+ struct simatic_ipc_led *led = cdev_to_led(led_cd);
+
+ u32 *p;
+
+ p = simatic_ipc_led_memory + led->value;
+ return (*p & 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;
+ struct device *dev = &pdev->dev;
+ struct simatic_ipc_led *ipcled;
+ struct led_classdev *cdev;
+ struct resource *res;
+ int err, type;
+ u32 *p;
+
+ switch (plat->devmode) {
+ case SIMATIC_IPC_DEVICE_227D:
+ case SIMATIC_IPC_DEVICE_427E:
+ res = &simatic_ipc_led_io_res;
+ ipcled = simatic_ipc_leds_io;
+ /* on 227D the two bytes work the other way araound */
+ if (plat->devmode == SIMATIC_IPC_DEVICE_227D) {
+ while (ipcled->value) {
+ ipcled->value = swab16(ipcled->value);
+ ipcled++;
+ }
+ 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;
+
+ /* get GPIO base from PCI */
+ res->start = simatic_ipc_get_membase0(PCI_DEVFN(13, 0));
+ if (res->start == 0)
+ return -ENODEV;
+
+ /* do the final address calculation */
+ res->start = res->start + (0xC5 << 16);
+ res->end += res->start;
+
+ 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 */
+ p = simatic_ipc_led_memory + 0x500 + 0x1D8; /* PM_WDT_OUT */
+ *p = (*p & ~1);
+ p = simatic_ipc_led_memory + 0x500 + 0x1C0; /* PM_BIOS_BOOT_N */
+ *p = (*p | 1);
+
+ 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->max_brightness = LED_ON;
+ cdev->name = ipcled->name;
+
+ err = devm_led_classdev_register(dev, cdev);
+ if (err < 0)
+ return err;
+ ipcled++;
+ }
+
+ return 0;
+}
+
+static struct platform_driver simatic_ipc_led_driver = {
+ .probe = simatic_ipc_leds_probe,
+ .driver = {
+ .name = KBUILD_MODNAME,
+ }
+};
+
+module_platform_driver(simatic_ipc_led_driver);
+
+MODULE_LICENSE("GPL v2");
+MODULE_ALIAS("platform:" KBUILD_MODNAME);
+MODULE_AUTHOR("Henning Schild <[email protected]>");
--
2.32.0



2021-11-26 15:12:30

by Andy Shevchenko

[permalink] [raw]
Subject: Re: [PATCH v4 2/4] leds: simatic-ipc-leds: add new driver for Siemens Industial PCs

On Fri, Nov 26, 2021 at 4:10 PM Henning Schild
<[email protected]> wrote:
>
> This driver adds initial support for several devices from Siemens. It is
> based on a platform driver introduced in an earlier commit.

...

> +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"},
> + { }
> +};

Like I said, this is not okay.

Why can't you simply enable the pinctrl driver and use it?



--
With Best Regards,
Andy Shevchenko

2021-11-26 17:51:00

by Henning Schild

[permalink] [raw]
Subject: Re: [PATCH v4 2/4] leds: simatic-ipc-leds: add new driver for Siemens Industial PCs

Am Fri, 26 Nov 2021 17:02:00 +0200
schrieb Andy Shevchenko <[email protected]>:

> On Fri, Nov 26, 2021 at 4:10 PM Henning Schild
> <[email protected]> wrote:
> >
> > This driver adds initial support for several devices from Siemens.
> > It is based on a platform driver introduced in an earlier commit.
>
> ...
>
> > +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"},
> > + { }
> > +};
>
> Like I said, this is not okay.
>
> Why can't you simply enable the pinctrl driver and use it?

What do you mean with enable (=m, =y)? The kernel has it built in ...
but it does not come up because it does not find an ACPI table entry
nor a p2sb PCI device.

When with enable you mean ... repair the pinctrl drivers with your
patches, i am afraid i am not qualified and hardly responsible. It is
not "simple" for me. I will test and switch to pinctrl .... but here i
am tired of waiting and pushing/asking to turn the order of patches
around.

Or can i trigger the pinctrl driver to probe from my init/probe?

Henning

2021-11-26 18:14:47

by Henning Schild

[permalink] [raw]
Subject: Re: [PATCH v4 2/4] leds: simatic-ipc-leds: add new driver for Siemens Industial PCs

Am Fri, 26 Nov 2021 17:02:00 +0200
schrieb Andy Shevchenko <[email protected]>:

> On Fri, Nov 26, 2021 at 4:10 PM Henning Schild
> <[email protected]> wrote:
> >
> > This driver adds initial support for several devices from Siemens.
> > It is based on a platform driver introduced in an earlier commit.
>
> ...
>
> > +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"},
> > + { }
> > +};
>
> Like I said, this is not okay.
>
> Why can't you simply enable the pinctrl driver and use it?

I propose we set up a call, that might help clearing up the situation.
If you agree please send me an email and possibly propose a time-slot.
I would take it from there and send you a meeting link.

regards,
Henning

>
>


2021-11-30 15:23:52

by Andy Shevchenko

[permalink] [raw]
Subject: Re: [PATCH v4 2/4] leds: simatic-ipc-leds: add new driver for Siemens Industial PCs

On Fri, Nov 26, 2021 at 07:12:03PM +0100, Henning Schild wrote:
> Am Fri, 26 Nov 2021 17:02:00 +0200
> schrieb Andy Shevchenko <[email protected]>:
> > On Fri, Nov 26, 2021 at 4:10 PM Henning Schild
> > <[email protected]> wrote:
> > >
> > > This driver adds initial support for several devices from Siemens.
> > > It is based on a platform driver introduced in an earlier commit.
> >
> > ...
> >
> > > +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"},
> > > + { }
> > > +};
> >
> > Like I said, this is not okay.
> >
> > Why can't you simply enable the pinctrl driver and use it?

I have talked to my boss today and I have got an approval to prioritize
the task, so I'm all yours starting from tomorrow.

Let's finish it once for all!

> I propose we set up a call, that might help clearing up the situation.
> If you agree please send me an email and possibly propose a time-slot.
> I would take it from there and send you a meeting link.

Sure!

--
With Best Regards,
Andy Shevchenko



2021-12-03 19:36:11

by Henning Schild

[permalink] [raw]
Subject: Re: [PATCH v4 2/4] leds: simatic-ipc-leds: add new driver for Siemens Industial PCs

Am Tue, 30 Nov 2021 17:15:14 +0200
schrieb Andy Shevchenko <[email protected]>:

> On Fri, Nov 26, 2021 at 07:12:03PM +0100, Henning Schild wrote:
> > Am Fri, 26 Nov 2021 17:02:00 +0200
> > schrieb Andy Shevchenko <[email protected]>:
> > > On Fri, Nov 26, 2021 at 4:10 PM Henning Schild
> > > <[email protected]> wrote:
> > > >
> > > > This driver adds initial support for several devices from
> > > > Siemens. It is based on a platform driver introduced in an
> > > > earlier commit.
> > >
> > > ...
> > >
> > > > +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"},
> > > > + { }
> > > > +};
> > >
> > > Like I said, this is not okay.
> > >
> > > Why can't you simply enable the pinctrl driver and use it?
>
> I have talked to my boss today and I have got an approval to
> prioritize the task, so I'm all yours starting from tomorrow.

We had a long and fruitful conversation today. In very short the story
will be that i will send a v5. It will make clear in the cover letter,
in the FIXME, and in commit messages that the P2SB bits are hacky, same
for poking on GPIO memory.
And also say again why it is like that and why it currently can
probably not be done much better.
With these documentation changes Andy said he would be willing to ack.

On top Andy will work on P2SB improvements, and maybe also pinctrl
infrastructure to make the existing drivers actually probe without a
need for ACPI fixes in firmware.

When these patches are ready i will change the Siemens drivers to use
that and take out hackery where possible.

Andy please follow up in case i summarized things wrong, but i bet i do
not have to tell you.

> Let's finish it once for all!

I sure hope we get there!

regards,
Henning

> > I propose we set up a call, that might help clearing up the
> > situation. If you agree please send me an email and possibly
> > propose a time-slot. I would take it from there and send you a
> > meeting link.
>
> Sure!
>