| Message ID | 20211126141027.16161-3-henning.schild@siemens.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series | add device drivers for Siemens Industrial PCs | expand |
On Fri, Nov 26, 2021 at 4:10 PM Henning Schild <henning.schild@siemens.com> 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?
Am Fri, 26 Nov 2021 17:02:00 +0200 schrieb Andy Shevchenko <andy.shevchenko@gmail.com>: > On Fri, Nov 26, 2021 at 4:10 PM Henning Schild > <henning.schild@siemens.com> 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
Am Fri, 26 Nov 2021 17:02:00 +0200 schrieb Andy Shevchenko <andy.shevchenko@gmail.com>: > On Fri, Nov 26, 2021 at 4:10 PM Henning Schild > <henning.schild@siemens.com> 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 > >
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 <andy.shevchenko@gmail.com>: > > On Fri, Nov 26, 2021 at 4:10 PM Henning Schild > > <henning.schild@siemens.com> 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!
Am Tue, 30 Nov 2021 17:15:14 +0200 schrieb Andy Shevchenko <andy.shevchenko@gmail.com>: > 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 <andy.shevchenko@gmail.com>: > > > On Fri, Nov 26, 2021 at 4:10 PM Henning Schild > > > <henning.schild@siemens.com> 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! >
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 <henning.schild@siemens.com> + * Jan Kiszka <jan.kiszka@siemens.com> + * Gerd Haeussler <gerd.haeussler.ext@siemens.com> + */ + +#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(®_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(®_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 <henning.schild@siemens.com>");
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 <henning.schild@siemens.com> --- 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