| Message ID | 20210315095710.7140-3-henning.schild@siemens.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | add device drivers for Siemens Industrial PCs | expand |
On Mon, Mar 15, 2021 at 11:57 AM 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. ... > +struct simatic_ipc_led { > + unsigned int value; /* mask for io and offset for mem */ > + char name[32]; Hmm... Dunno if LED framework defines its own constraints for the length of the name. > + struct led_classdev cdev; > +}; > + > +static struct simatic_ipc_led simatic_ipc_leds_io[] = { > + {1 << 15, "simatic-ipc:green:" LED_FUNCTION_STATUS "-1" }, > + {1 << 7, "simatic-ipc:yellow:" LED_FUNCTION_STATUS "-1" }, > + {1 << 14, "simatic-ipc:red:" LED_FUNCTION_STATUS "-2" }, > + {1 << 6, "simatic-ipc:yellow:" LED_FUNCTION_STATUS "-2" }, > + {1 << 13, "simatic-ipc:red:" LED_FUNCTION_STATUS "-3" }, > + {1 << 5, "simatic-ipc:yellow:" LED_FUNCTION_STATUS "-3" }, Can you use BIT() macro here? And can it be sorted by the bit order? > + {0, ""}, { } is enough (no comma for terminator lines in general, and no need to show structure member assignments separately in particular). > +}; > + > +/* the actual start will be discovered with pci, 0 is a placeholder */ PCI > +struct resource simatic_ipc_led_mem_res = > + DEFINE_RES_MEM_NAMED(0, SZ_4K, KBUILD_MODNAME); One line? ... > +static struct simatic_ipc_led simatic_ipc_leds_mem[] = { > + {0x500 + 0x1A0, "simatic-ipc:red:" LED_FUNCTION_STATUS "-1"}, > + {0x500 + 0x1A8, "simatic-ipc:green:" LED_FUNCTION_STATUS "-1"}, > + {0x500 + 0x1C8, "simatic-ipc:red:" LED_FUNCTION_STATUS "-2"}, > + {0x500 + 0x1D0, "simatic-ipc:green:" LED_FUNCTION_STATUS "-2"}, > + {0x500 + 0x1E0, "simatic-ipc:red:" LED_FUNCTION_STATUS "-3"}, > + {0x500 + 0x198, "simatic-ipc:green:" LED_FUNCTION_STATUS "-3"}, > + {0, ""}, As per above. > +}; ... > + struct simatic_ipc_led *led = > + container_of(led_cd, struct simatic_ipc_led, cdev); One line? ... > + struct simatic_ipc_led *led = > + container_of(led_cd, struct simatic_ipc_led, cdev); One line? ... > + struct simatic_ipc_led *led = > + container_of(led_cd, struct simatic_ipc_led, cdev); Ditto. Btw, usually for such cases we create an inline helper ... to_simatic_ipc_led(...) { return container_of(...); } ... > +static int simatic_ipc_leds_probe(struct platform_device *pdev) > +{ > + struct simatic_ipc_platform *plat; const? > + struct device *dev = &pdev->dev; > + struct simatic_ipc_led *ipcled; > + struct led_classdev *cdev; > + struct resource *res; > + int err, type; > + u32 *p; > + plat = pdev->dev.platform_data; Can be done directly in the definition block. > + switch (plat->devmode) { > + case SIMATIC_IPC_DEVICE_227D: > + case SIMATIC_IPC_DEVICE_427E: > + res = &simatic_ipc_led_io_res; > + ipcled = simatic_ipc_leds_io; > + /* the 227D is high on while 427E is low on, invert the struct > + * we have > + */ > + if (plat->devmode == SIMATIC_IPC_DEVICE_227D) { > + while (ipcled->value) { > + ipcled->value = swab16(ipcled->value); > + ipcled++; > + } This seems fishy. If you have a BE CPU it won't work the same way. Better: a) to use cpu_to_le16 / be16 b) create this as a helper that we may move to the generic header of byteorder. But looking at the use of it, perhaps you rather need to redefine IO accessors, i.e. ioread16()/iowrite16() vs. ioread16be()/iowrite16be(). > + 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); Magic. As I told you this is an actual offseet in the P2SB's bar for GPIO registers. I have a question, why we can't provide a GPIO driver which is already in the kernel and, with use of the patch series I sent, to convert this all magic to GPIO LEDs as it's done for all normal cases? > + 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; > + } > +}
> > + struct led_classdev cdev; > > +}; > > + > > +static struct simatic_ipc_led simatic_ipc_leds_io[] = { > > + {1 << 15, "simatic-ipc:green:" LED_FUNCTION_STATUS "-1" }, > > + {1 << 7, "simatic-ipc:yellow:" LED_FUNCTION_STATUS "-1" }, > > + {1 << 14, "simatic-ipc:red:" LED_FUNCTION_STATUS "-2" }, > > + {1 << 6, "simatic-ipc:yellow:" LED_FUNCTION_STATUS "-2" }, > > + {1 << 13, "simatic-ipc:red:" LED_FUNCTION_STATUS "-3" }, > > + {1 << 5, "simatic-ipc:yellow:" LED_FUNCTION_STATUS "-3" }, > > Can you use BIT() macro here? And can it be sorted by the bit order? There's nothing wrong with << and this order is fine. But I still don't like the naming. simantic-ipc: prefix is useless. Having 6 status leds is not good, either. > > + struct simatic_ipc_led *led = > > + container_of(led_cd, struct simatic_ipc_led, cdev); > > One line? 80 columns. It is fine as it is. Best regards, Pavel
On Mon, Mar 15, 2021 at 1:19 PM Pavel Machek <pavel@ucw.cz> wrote: > > > + struct simatic_ipc_led *led = > > > + container_of(led_cd, struct simatic_ipc_led, cdev); > > > > One line? > > 80 columns. It is fine as it is. With the inline helper it will be possible to have this neat and short.
Am Mon, 15 Mar 2021 12:19:15 +0100 schrieb Pavel Machek <pavel@ucw.cz>: > > > + struct led_classdev cdev; > > > +}; > > > + > > > +static struct simatic_ipc_led simatic_ipc_leds_io[] = { > > > + {1 << 15, "simatic-ipc:green:" LED_FUNCTION_STATUS "-1" }, > > > + {1 << 7, "simatic-ipc:yellow:" LED_FUNCTION_STATUS "-1" > > > }, > > > + {1 << 14, "simatic-ipc:red:" LED_FUNCTION_STATUS "-2" }, > > > + {1 << 6, "simatic-ipc:yellow:" LED_FUNCTION_STATUS "-2" > > > }, > > > + {1 << 13, "simatic-ipc:red:" LED_FUNCTION_STATUS "-3" }, > > > + {1 << 5, "simatic-ipc:yellow:" LED_FUNCTION_STATUS "-3" > > > }, > > > > Can you use BIT() macro here? And can it be sorted by the bit > > order? > > There's nothing wrong with << and this order is fine. > > But I still don't like the naming. simantic-ipc: prefix is > useless. Having 6 status leds is not good, either. You asked about a picture before, so here is one example https://support.industry.siemens.com/cs/document/67235073/simatic-ipc427d?dti=0&pnid=16756&lc=en-WW page 19 shows how the box looks like page 135 it what the implementation is based on Externally human visible are 3 "lights", which can be off, red, green, yellow. Internally every single "light" has two leds and yellow is a mix when red and green are on. Unfortunately hw does not allow all 4 states for all 3 lights. Some boxes implement "yellow" mixing in hw. That is why the same name is used with two colors. maybe those LEDs qualify for multi-color? "status" was the best name i found in the dt-bindings header i guess i can be creative with the names and take ie "status", "fault", and "indicator" i will also drop that prefix "simatic-ipc" that was inspired by "tpacpi", or should it be "platform:<color>:<name>"? regards, Henning > > > + struct simatic_ipc_led *led = > > > + container_of(led_cd, struct simatic_ipc_led, > > > cdev); > > > > One line? > > 80 columns. It is fine as it is. > > Best regards, > > Pavel
On 15.03.21 10:57, Henning Schild wrote: Hi, > diff --git a/drivers/leds/simple/simatic-ipc-leds.c b/drivers/leds/simple/simatic-ipc-leds.c > new file mode 100644 > index 000000000000..0f7e6320e10d > --- /dev/null > +++ b/drivers/leds/simple/simatic-ipc-leds.c > @@ -0,0 +1,210 @@ > +// 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[32]; > + struct led_classdev cdev; > +}; > + > +static struct simatic_ipc_led simatic_ipc_leds_io[] = { > + {1 << 15, "simatic-ipc:green:" LED_FUNCTION_STATUS "-1" }, > + {1 << 7, "simatic-ipc:yellow:" LED_FUNCTION_STATUS "-1" }, > + {1 << 14, "simatic-ipc:red:" LED_FUNCTION_STATUS "-2" }, > + {1 << 6, "simatic-ipc:yellow:" LED_FUNCTION_STATUS "-2" }, > + {1 << 13, "simatic-ipc:red:" LED_FUNCTION_STATUS "-3" }, > + {1 << 5, "simatic-ipc:yellow:" LED_FUNCTION_STATUS "-3" }, > + {0, ""}, > +}; Wouldn't it be better to name them like they're labeled on the device, as shown on page #19 of the manual, or perhaps a little bit more generic nameing (eg. power, status, error, maint) ? > +/* 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; > + hmm, could there *ever* be multiple instances of the driver ? Wouldn't it be better to put this in the device priv data instead ? > +static struct simatic_ipc_led simatic_ipc_leds_mem[] = { > + {0x500 + 0x1A0, "simatic-ipc:red:" LED_FUNCTION_STATUS "-1"}, > + {0x500 + 0x1A8, "simatic-ipc:green:" LED_FUNCTION_STATUS "-1"}, > + {0x500 + 0x1C8, "simatic-ipc:red:" LED_FUNCTION_STATUS "-2"}, > + {0x500 + 0x1D0, "simatic-ipc:green:" LED_FUNCTION_STATUS "-2"}, > + {0x500 + 0x1E0, "simatic-ipc:red:" LED_FUNCTION_STATUS "-3"}, > + {0x500 + 0x198, "simatic-ipc:green:" LED_FUNCTION_STATUS "-3"}, > + {0, ""}, > +}; > + > +static struct resource simatic_ipc_led_io_res = > + DEFINE_RES_IO_NAMED(SIMATIC_IPC_LED_PORT_BASE, SZ_1, KBUILD_MODNAME); > + > +static DEFINE_SPINLOCK(reg_lock); Does this protect global data structures ? If not, I'd rather put it into the device priv data instead. BTW: doesn't have struct led_classdev already have a lock that can be used ? Can multiple calls to led ops (within the same device) at the same time happen at all, or does led core already serialize that ? > +static void simatic_ipc_led_set_io(struct led_classdev *led_cd, > + enum led_brightness brightness) > +{ > + struct simatic_ipc_led *led = > + container_of(led_cd, struct simatic_ipc_led, cdev); > + 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); Don't we already have an helper for setting or clearing bits in IO registers (that already does the read + set/clear + write at once) ? Does that really need to be protected by lock ? (can happen multiple calls to that func from different threads happen at all ?) Is the port really *always* the same, so it really can be a const ? <snip> > +static int simatic_ipc_leds_probe(struct platform_device *pdev) > +{ > + struct simatic_ipc_platform *plat; > + struct device *dev = &pdev->dev; > + struct simatic_ipc_led *ipcled; > + struct led_classdev *cdev; > + struct resource *res; > + int err, type; > + u32 *p; > + > + plat = pdev->dev.platform_data; Maybe put this into swnode ? IIRC, the consensus is not to introduce new platform data structs anymore, instead legacy pdata to swnode some day. > + switch (plat->devmode) { > + case SIMATIC_IPC_DEVICE_227D: > + case SIMATIC_IPC_DEVICE_427E: > + res = &simatic_ipc_led_io_res; > + ipcled = simatic_ipc_leds_io; > + /* the 227D is high on while 427E is low on, invert the struct > + * we have > + */ > + if (plat->devmode == SIMATIC_IPC_DEVICE_227D) { > + while (ipcled->value) { > + ipcled->value = swab16(ipcled->value); Uff, better use explicit endian conversion macros (eg. be*_to_cpu()) for that. Also, I wouldn't change those global structs, instead put those data into device priv data and make the global stuff const. You could also use the same field for both port-io and mmap'ed variants. And adding regmap to the equation, could use the same led ops for both. (IMHO, the little bit of overhead by regmap shouldn't matter here) > + 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; Where does that device actually sit on ? Some generic card ? Some ASIC or FPGA ? It seems this driver is instantiated by another one, which already knows what device we're actually dealing with (as it sets plat->devmode). Why not letting that parent device also tell the io resource to this driver ? > + while (ipcled->value) { > + cdev = &ipcled->cdev; > + cdev->brightness_set = simatic_ipc_led_set_io; > + cdev->brightness_get = simatic_ipc_led_get_io; > + if (type == IORESOURCE_MEM) { > + cdev->brightness_set = simatic_ipc_led_set_mem; > + cdev->brightness_get = simatic_ipc_led_get_mem; > + } Why not if/else ? > + 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 led_driver = { Why not calling it simatic_ipc_led_driver ? > + .probe = simatic_ipc_leds_probe, > + .driver = { > + .name = KBUILD_MODNAME, > + }, > +}; > + > +module_platform_driver(led_driver); > + > +MODULE_LICENSE("GPL v2"); > +MODULE_ALIAS("platform:" KBUILD_MODNAME); > +MODULE_AUTHOR("Henning Schild <henning.schild@siemens.com>"); > --mtx
On 15.03.21 11:48, Andy Shevchenko wrote: Hi, > I have a question, why we can't provide a GPIO driver which is already > in the kernel and, with use of the patch series I sent, to convert > this all magic to GPIO LEDs as it's done for all normal cases? Do we alread have a generic led driver that for cases that just set/clear bits in some mem/io location ? If not, that would be really great to have. --mtx
On 15.03.21 12:19, Pavel Machek wrote: > But I still don't like the naming. simantic-ipc: prefix is > useless. Having 6 status leds is not good, either. Do we have some standard naming policy those kinds of LEDs ? In this case, they seem to be assigned to certain specific functions (by physical labels on the box), so IMHO the LED names should reflect that in some ways. There're other cases (eg. apu board familiy) that just have several front panel leds w/o any dedication, so we can just count them up. --mtx
Hei hei, > Enrico Weigelt, metux IT consult <lkml@metux.net> hat am 18.03.2021 11:27 geschrieben: > > > On 15.03.21 11:48, Andy Shevchenko wrote: > > Hi, > > > I have a question, why we can't provide a GPIO driver which is already > > in the kernel and, with use of the patch series I sent, to convert > > this all magic to GPIO LEDs as it's done for all normal cases? > > Do we alread have a generic led driver that for cases that just > set/clear bits in some mem/io location ? If not, that would be really > great to have. Yes, there is. Look out for compatible "register-bit-led" in device tree. That's from driver in drivers/leds/leds-syscon.c and you can use it inside a syscon node in dts. It assumes one bit per LED. Greets Alex
Am Thu, 18 Mar 2021 13:40:58 +0100 schrieb Alexander Dahl <ada@thorsis.com>: > Hei hei, > > > Enrico Weigelt, metux IT consult <lkml@metux.net> hat am 18.03.2021 > > 11:27 geschrieben: > > > > > > On 15.03.21 11:48, Andy Shevchenko wrote: > > > > Hi, > > > > > I have a question, why we can't provide a GPIO driver which is > > > already in the kernel and, with use of the patch series I sent, > > > to convert this all magic to GPIO LEDs as it's done for all > > > normal cases? > > > > Do we alread have a generic led driver that for cases that just > > set/clear bits in some mem/io location ? If not, that would be > > really great to have. > > Yes, there is. Look out for compatible "register-bit-led" in device > tree. That's from driver in drivers/leds/leds-syscon.c and you can > use it inside a syscon node in dts. > > It assumes one bit per LED. Sorry guys, i am lost here. Is there a driver i can base mine on, if so which one? Maybe you can point me to a good example that is conceptually similar. As i already wrote in the reviews of v1, the ACPI tables will not change on the machines in question. So there is a need for a driver. Either one like i did propose or maybe something that patches ACPI or loads device-tree snippets, again please point me to good examples. We are talking about x86-only here. Henning > Greets > Alex
Am Mon, 15 Mar 2021 12:48:19 +0200 schrieb Andy Shevchenko <andy.shevchenko@gmail.com>: > On Mon, Mar 15, 2021 at 11:57 AM 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. > > ... > > > +struct simatic_ipc_led { > > + unsigned int value; /* mask for io and offset for mem */ > > > + char name[32]; > > Hmm... Dunno if LED framework defines its own constraints for the > length of the name. > > > + struct led_classdev cdev; > > +}; > > + > > +static struct simatic_ipc_led simatic_ipc_leds_io[] = { > > + {1 << 15, "simatic-ipc:green:" LED_FUNCTION_STATUS "-1" }, > > + {1 << 7, "simatic-ipc:yellow:" LED_FUNCTION_STATUS "-1" }, > > + {1 << 14, "simatic-ipc:red:" LED_FUNCTION_STATUS "-2" }, > > + {1 << 6, "simatic-ipc:yellow:" LED_FUNCTION_STATUS "-2" }, > > + {1 << 13, "simatic-ipc:red:" LED_FUNCTION_STATUS "-3" }, > > + {1 << 5, "simatic-ipc:yellow:" LED_FUNCTION_STATUS "-3" }, > > > > Can you use BIT() macro here? And can it be sorted by the bit order? > > > + {0, ""}, > > { } is enough (no comma for terminator lines in general, and no need > to show structure member assignments separately in particular). > > > +}; > > + > > +/* the actual start will be discovered with pci, 0 is a > > placeholder */ > > PCI > > > +struct resource simatic_ipc_led_mem_res = > > + DEFINE_RES_MEM_NAMED(0, SZ_4K, KBUILD_MODNAME); > > One line? > > ... > > > +static struct simatic_ipc_led simatic_ipc_leds_mem[] = { > > + {0x500 + 0x1A0, "simatic-ipc:red:" LED_FUNCTION_STATUS > > "-1"}, > > + {0x500 + 0x1A8, "simatic-ipc:green:" LED_FUNCTION_STATUS > > "-1"}, > > + {0x500 + 0x1C8, "simatic-ipc:red:" LED_FUNCTION_STATUS > > "-2"}, > > + {0x500 + 0x1D0, "simatic-ipc:green:" LED_FUNCTION_STATUS > > "-2"}, > > + {0x500 + 0x1E0, "simatic-ipc:red:" LED_FUNCTION_STATUS > > "-3"}, > > + {0x500 + 0x198, "simatic-ipc:green:" LED_FUNCTION_STATUS > > "-3"}, > > + {0, ""}, > > As per above. > > > +}; > > ... > > > + struct simatic_ipc_led *led = > > + container_of(led_cd, struct simatic_ipc_led, cdev); > > > > One line? > > ... > > > + struct simatic_ipc_led *led = > > + container_of(led_cd, struct simatic_ipc_led, cdev); > > > > One line? > > ... > > > + struct simatic_ipc_led *led = > > + container_of(led_cd, struct simatic_ipc_led, cdev); > > > > Ditto. > > > Btw, usually for such cases we create an inline helper > ... to_simatic_ipc_led(...) > { > return container_of(...); > } > > ... > > > +static int simatic_ipc_leds_probe(struct platform_device *pdev) > > +{ > > + struct simatic_ipc_platform *plat; > > const? > > > + struct device *dev = &pdev->dev; > > + struct simatic_ipc_led *ipcled; > > + struct led_classdev *cdev; > > + struct resource *res; > > + int err, type; > > + u32 *p; > > > + plat = pdev->dev.platform_data; > > Can be done directly in the definition block. > > > + switch (plat->devmode) { > > + case SIMATIC_IPC_DEVICE_227D: > > + case SIMATIC_IPC_DEVICE_427E: > > + res = &simatic_ipc_led_io_res; > > + ipcled = simatic_ipc_leds_io; > > + /* the 227D is high on while 427E is low on, invert > > the struct > > + * we have > > + */ > > + if (plat->devmode == SIMATIC_IPC_DEVICE_227D) { > > > + while (ipcled->value) { > > + ipcled->value = > > swab16(ipcled->value); > > + ipcled++; > > + } > > This seems fishy. If you have a BE CPU it won't work the same way. > Better: > a) to use cpu_to_le16 / be16 > b) create this as a helper that we may move to the generic header of > byteorder. > > But looking at the use of it, perhaps you rather need to redefine IO > accessors, i.e. ioread16()/iowrite16() vs. ioread16be()/iowrite16be(). Got my hands on such a special-case device today. The comment is wrong it talks about high and low, will fix that. This one machine almost shares LED logic with some others. We have those 6 bits spread over 2 consecutive bytes. For this one guy swapping the two bytes is the shortest way to share the code. I tried a few things, extra getters/setters, extra array defining bits the other way around. It all ends up with way more code or conditions in the getter/setter. So i think i will leave it like it is, clarify that comment. And that swap16 is fine because we are on x86 only and are basically swapping (1<<7 with 1<<15) ... where "<<" is already endianessy. Henning > > + 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); > > Magic. As I told you this is an actual offseet in the P2SB's bar for > GPIO registers. > I have a question, why we can't provide a GPIO driver which is already > in the kernel and, with use of the patch series I sent, to convert > this all magic to GPIO LEDs as it's done for all normal cases? > > > + 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; > > + } > > > +} >
Am Thu, 18 Mar 2021 12:38:53 +0100 schrieb "Enrico Weigelt, metux IT consult" <lkml@metux.net>: > On 15.03.21 12:19, Pavel Machek wrote: > > > But I still don't like the naming. simantic-ipc: prefix is > > useless. Having 6 status leds is not good, either. > > Do we have some standard naming policy those kinds of LEDs ? There is include/dt-bindings/leds/common.h with LED_FUNCTION_* > In this case, they seem to be assigned to certain specific functions > (by physical labels on the box), so IMHO the LED names should reflect > that in some ways. The choice for "status" was because of >> /* Miscelleaus functions. Use functions above if you can. */ And those known names do not really come with an explanation of their meaning. Names like "bluetooth" seem obvious, but "activity" or "indicator" leave a lot of room for speculation. The choice in numbers was inspired by labels on the box, which i wanted to reflect in some way. Henning > There're other cases (eg. apu board familiy) that just have several > front panel leds w/o any dedication, so we can just count them up. > > > --mtx >
Am Mon, 15 Mar 2021 12:19:15 +0100 schrieb Pavel Machek <pavel@ucw.cz>: > > > + struct led_classdev cdev; > > > +}; > > > + > > > +static struct simatic_ipc_led simatic_ipc_leds_io[] = { > > > + {1 << 15, "simatic-ipc:green:" LED_FUNCTION_STATUS "-1" }, > > > + {1 << 7, "simatic-ipc:yellow:" LED_FUNCTION_STATUS "-1" > > > }, > > > + {1 << 14, "simatic-ipc:red:" LED_FUNCTION_STATUS "-2" }, > > > + {1 << 6, "simatic-ipc:yellow:" LED_FUNCTION_STATUS "-2" > > > }, > > > + {1 << 13, "simatic-ipc:red:" LED_FUNCTION_STATUS "-3" }, > > > + {1 << 5, "simatic-ipc:yellow:" LED_FUNCTION_STATUS "-3" > > > }, > > > > Can you use BIT() macro here? And can it be sorted by the bit > > order? > > There's nothing wrong with << and this order is fine. > > But I still don't like the naming. simantic-ipc: prefix is > useless. Having 6 status leds is not good, either. With some of my questions still not being answered i will probably remove that prefix entirely, not even use "platform:". And i might stick with 6x "status". Since that allows reflecting the labels on the machines, while using "above functions if you can" regards, Henning > > > + struct simatic_ipc_led *led = > > > + container_of(led_cd, struct simatic_ipc_led, > > > cdev); > > > > One line? > > 80 columns. It is fine as it is. > > Best regards, > > Pavel
Am Thu, 18 Mar 2021 11:25:10 +0100 schrieb "Enrico Weigelt, metux IT consult" <lkml@metux.net>: > On 15.03.21 10:57, Henning Schild wrote: > > Hi, > > > diff --git a/drivers/leds/simple/simatic-ipc-leds.c > > b/drivers/leds/simple/simatic-ipc-leds.c new file mode 100644 > > index 000000000000..0f7e6320e10d > > --- /dev/null > > +++ b/drivers/leds/simple/simatic-ipc-leds.c > > @@ -0,0 +1,210 @@ > > +// 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[32]; > > + struct led_classdev cdev; > > +}; > > + > > +static struct simatic_ipc_led simatic_ipc_leds_io[] = { > > + {1 << 15, "simatic-ipc:green:" LED_FUNCTION_STATUS "-1" }, > > + {1 << 7, "simatic-ipc:yellow:" LED_FUNCTION_STATUS "-1" }, > > + {1 << 14, "simatic-ipc:red:" LED_FUNCTION_STATUS "-2" }, > > + {1 << 6, "simatic-ipc:yellow:" LED_FUNCTION_STATUS "-2" }, > > + {1 << 13, "simatic-ipc:red:" LED_FUNCTION_STATUS "-3" }, > > + {1 << 5, "simatic-ipc:yellow:" LED_FUNCTION_STATUS "-3" }, > > + {0, ""}, > > +}; > > Wouldn't it be better to name them like they're labeled on the device, > as shown on page #19 of the manual, or perhaps a little bit more > generic nameing (eg. power, status, error, maint) ? That was proposed in v1 but modern LED drivers should stick to known LED_FUNCTION_* Here the numbers reflect what is on the device labels. We are talking about roughly 10 boxes at the moment, not all having the same labels ... but all share 1, 2, 3 At the end of the day the software view might be more important than the labels. So that people can write generic "echo 1 > brightness"-code that works across the full range, possibly even on other machines from different vendors. I guess that is also why Pavel wants people to use established names. > > +/* 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; > > + > > hmm, could there *ever* be multiple instances of the driver ? No, the platform bus makes sure there can only be one. > Wouldn't it be better to put this in the device priv data instead ? I guess no need for priv because of "highlander ... only 1" > > +static struct simatic_ipc_led simatic_ipc_leds_mem[] = { > > + {0x500 + 0x1A0, "simatic-ipc:red:" LED_FUNCTION_STATUS > > "-1"}, > > + {0x500 + 0x1A8, "simatic-ipc:green:" LED_FUNCTION_STATUS > > "-1"}, > > + {0x500 + 0x1C8, "simatic-ipc:red:" LED_FUNCTION_STATUS > > "-2"}, > > + {0x500 + 0x1D0, "simatic-ipc:green:" LED_FUNCTION_STATUS > > "-2"}, > > + {0x500 + 0x1E0, "simatic-ipc:red:" LED_FUNCTION_STATUS > > "-3"}, > > + {0x500 + 0x198, "simatic-ipc:green:" LED_FUNCTION_STATUS > > "-3"}, > > + {0, ""}, > > +}; > > + > > +static struct resource simatic_ipc_led_io_res = > > + DEFINE_RES_IO_NAMED(SIMATIC_IPC_LED_PORT_BASE, SZ_1, > > KBUILD_MODNAME); + > > +static DEFINE_SPINLOCK(reg_lock); > > Does this protect global data structures ? If not, I'd rather put it > into the device priv data instead. highlander, no need for priv It protects the setter, which needs to inw + outw and is not atomic > BTW: doesn't have struct led_classdev already have a lock that > can be used ? Can multiple calls to led ops (within the same device) > at the same time happen at all, or does led core already serialize > that ? Not sure, i probably did check that in 4.19 at the time of writing. Pavel did not complain so far. And other drivers have locks in their setters. > > +static void simatic_ipc_led_set_io(struct led_classdev *led_cd, > > + enum led_brightness brightness) > > +{ > > + struct simatic_ipc_led *led = > > + container_of(led_cd, struct simatic_ipc_led, cdev); > > + 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); > > Don't we already have an helper for setting or clearing bits in IO > registers (that already does the read + set/clear + write at once) ? Did not find one > Does that really need to be protected by lock ? > (can happen multiple calls to that func from different threads happen > at all ?) i think so, see above > Is the port really *always* the same, so it really can be a const ? Yes, but making that ressource const would just cause casting in the probe function, or a second pointer for the io case. In the mem-case the static data gets written to. > <snip> > > > +static int simatic_ipc_leds_probe(struct platform_device *pdev) > > +{ > > + struct simatic_ipc_platform *plat; > > + struct device *dev = &pdev->dev; > > + struct simatic_ipc_led *ipcled; > > + struct led_classdev *cdev; > > + struct resource *res; > > + int err, type; > > + u32 *p; > > + > > + plat = pdev->dev.platform_data; > > Maybe put this into swnode ? > > IIRC, the consensus is not to introduce new platform data structs > anymore, instead legacy pdata to swnode some day. > > > + switch (plat->devmode) { > > + case SIMATIC_IPC_DEVICE_227D: > > + case SIMATIC_IPC_DEVICE_427E: > > + res = &simatic_ipc_led_io_res; > > + ipcled = simatic_ipc_leds_io; > > + /* the 227D is high on while 427E is low on, > > invert the struct > > + * we have > > + */ > > + if (plat->devmode == SIMATIC_IPC_DEVICE_227D) { > > + while (ipcled->value) { > > + ipcled->value = > > swab16(ipcled->value); > > Uff, better use explicit endian conversion macros (eg. be*_to_cpu()) > for that. > > Also, I wouldn't change those global structs, instead put those data > into device priv data and make the global stuff const. You could also > use the same field for both port-io and mmap'ed variants. And adding > regmap to the equation, could use the same led ops for both. (IMHO, > the little bit of overhead by regmap shouldn't matter here) You are the second person to point that out. (the swab) What is done here is swapping two bytes, no endianess or anything and x86 only. As for the const and the regmap or priv data. This driver will only ever load once, if at all. There is really no need for abstractions, additional memory use or cycles. So i will ignore the complaints. Feel free to emphasize them if you do not agree with me doing that. > > + 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; > > Where does that device actually sit on ? Some generic card ? Some ASIC > or FPGA ? It is called P2SB and there some GPIO chip which i believe has no generic driver to it yet. > It seems this driver is instantiated by another one, which already > knows what device we're actually dealing with (as it sets > plat->devmode). Why not letting that parent device also tell the io > resource to this driver ? > > > + while (ipcled->value) { > > + cdev = &ipcled->cdev; > > + cdev->brightness_set = simatic_ipc_led_set_io; > > + cdev->brightness_get = simatic_ipc_led_get_io; > > + if (type == IORESOURCE_MEM) { > > + cdev->brightness_set = > > simatic_ipc_led_set_mem; > > + cdev->brightness_get = > > simatic_ipc_led_get_mem; > > + } > > Why not if/else ? Ok. > > + 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 led_driver = { > > Why not calling it simatic_ipc_led_driver ? Because it is a name with scope only in that file, but hey i will change it. Also in wdt regards, Henning > > + .probe = simatic_ipc_leds_probe, > > + .driver = { > > + .name = KBUILD_MODNAME, > > + }, > > +}; > > + > > +module_platform_driver(led_driver); > > + > > +MODULE_LICENSE("GPL v2"); > > +MODULE_ALIAS("platform:" KBUILD_MODNAME); > > +MODULE_AUTHOR("Henning Schild <henning.schild@siemens.com>"); > > > > > --mtx >
Am Sat, 27 Mar 2021 12:16:23 +0100 schrieb Henning Schild <henning.schild@siemens.com>: > Am Thu, 18 Mar 2021 11:25:10 +0100 > schrieb "Enrico Weigelt, metux IT consult" <lkml@metux.net>: > > > On 15.03.21 10:57, Henning Schild wrote: > > > > Hi, > > > > > diff --git a/drivers/leds/simple/simatic-ipc-leds.c > > > b/drivers/leds/simple/simatic-ipc-leds.c new file mode 100644 > > > index 000000000000..0f7e6320e10d > > > --- /dev/null > > > +++ b/drivers/leds/simple/simatic-ipc-leds.c > > > @@ -0,0 +1,210 @@ > > > +// 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[32]; > > > + struct led_classdev cdev; > > > +}; > > > + > > > +static struct simatic_ipc_led simatic_ipc_leds_io[] = { > > > + {1 << 15, "simatic-ipc:green:" LED_FUNCTION_STATUS "-1" > > > }, > > > + {1 << 7, "simatic-ipc:yellow:" LED_FUNCTION_STATUS "-1" > > > }, > > > + {1 << 14, "simatic-ipc:red:" LED_FUNCTION_STATUS "-2" }, > > > + {1 << 6, "simatic-ipc:yellow:" LED_FUNCTION_STATUS "-2" > > > }, > > > + {1 << 13, "simatic-ipc:red:" LED_FUNCTION_STATUS "-3" }, > > > + {1 << 5, "simatic-ipc:yellow:" LED_FUNCTION_STATUS "-3" > > > }, > > > + {0, ""}, > > > +}; > > > > Wouldn't it be better to name them like they're labeled on the > > device, as shown on page #19 of the manual, or perhaps a little bit > > more generic nameing (eg. power, status, error, maint) ? > > That was proposed in v1 but modern LED drivers should stick to known > LED_FUNCTION_* > Here the numbers reflect what is on the device labels. We are talking > about roughly 10 boxes at the moment, not all having the same labels > ... but all share 1, 2, 3 > > At the end of the day the software view might be more important than > the labels. So that people can write generic "echo 1 > > brightness"-code that works across the full range, possibly even on > other machines from different vendors. I guess that is also why Pavel > wants people to use established names. > > > > +/* 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; > > > + > > > > hmm, could there *ever* be multiple instances of the driver ? > > No, the platform bus makes sure there can only be one. > > > Wouldn't it be better to put this in the device priv data instead ? > > > > I guess no need for priv because of "highlander ... only 1" > > > > +static struct simatic_ipc_led simatic_ipc_leds_mem[] = { > > > + {0x500 + 0x1A0, "simatic-ipc:red:" LED_FUNCTION_STATUS > > > "-1"}, > > > + {0x500 + 0x1A8, "simatic-ipc:green:" LED_FUNCTION_STATUS > > > "-1"}, > > > + {0x500 + 0x1C8, "simatic-ipc:red:" LED_FUNCTION_STATUS > > > "-2"}, > > > + {0x500 + 0x1D0, "simatic-ipc:green:" LED_FUNCTION_STATUS > > > "-2"}, > > > + {0x500 + 0x1E0, "simatic-ipc:red:" LED_FUNCTION_STATUS > > > "-3"}, > > > + {0x500 + 0x198, "simatic-ipc:green:" LED_FUNCTION_STATUS > > > "-3"}, > > > + {0, ""}, > > > +}; > > > + > > > +static struct resource simatic_ipc_led_io_res = > > > + DEFINE_RES_IO_NAMED(SIMATIC_IPC_LED_PORT_BASE, SZ_1, > > > KBUILD_MODNAME); + > > > +static DEFINE_SPINLOCK(reg_lock); > > > > Does this protect global data structures ? If not, I'd rather put it > > into the device priv data instead. > > highlander, no need for priv > > It protects the setter, which needs to inw + outw and is not atomic > > > BTW: doesn't have struct led_classdev already have a lock that > > can be used ? Can multiple calls to led ops (within the same device) > > at the same time happen at all, or does led core already serialize > > that ? > > Not sure, i probably did check that in 4.19 at the time of writing. > Pavel did not complain so far. And other drivers have locks in their > setters. > > > > +static void simatic_ipc_led_set_io(struct led_classdev *led_cd, > > > + enum led_brightness > > > brightness) +{ > > > + struct simatic_ipc_led *led = > > > + container_of(led_cd, struct simatic_ipc_led, > > > cdev); > > > + 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); > > > > Don't we already have an helper for setting or clearing bits in IO > > registers (that already does the read + set/clear + write at once) > > ? > > Did not find one > > > Does that really need to be protected by lock ? > > (can happen multiple calls to that func from different threads > > happen at all ?) > > i think so, see above > > > Is the port really *always* the same, so it really can be a const ? > > > > Yes, but making that ressource const would just cause casting in the > probe function, or a second pointer for the io case. In the mem-case > the static data gets written to. > > > <snip> > > > > > +static int simatic_ipc_leds_probe(struct platform_device *pdev) > > > +{ > > > + struct simatic_ipc_platform *plat; > > > + struct device *dev = &pdev->dev; > > > + struct simatic_ipc_led *ipcled; > > > + struct led_classdev *cdev; > > > + struct resource *res; > > > + int err, type; > > > + u32 *p; > > > + > > > + plat = pdev->dev.platform_data; > > > > Maybe put this into swnode ? > > > > IIRC, the consensus is not to introduce new platform data structs > > anymore, instead legacy pdata to swnode some day. > > > > > + switch (plat->devmode) { > > > + case SIMATIC_IPC_DEVICE_227D: > > > + case SIMATIC_IPC_DEVICE_427E: > > > + res = &simatic_ipc_led_io_res; > > > + ipcled = simatic_ipc_leds_io; > > > + /* the 227D is high on while 427E is low on, > > > invert the struct > > > + * we have > > > + */ > > > + if (plat->devmode == SIMATIC_IPC_DEVICE_227D) { > > > + while (ipcled->value) { > > > + ipcled->value = > > > swab16(ipcled->value); > > > > Uff, better use explicit endian conversion macros (eg. be*_to_cpu()) > > for that. > > > > Also, I wouldn't change those global structs, instead put those data > > into device priv data and make the global stuff const. You could > > also use the same field for both port-io and mmap'ed variants. And > > adding regmap to the equation, could use the same led ops for both. > > (IMHO, the little bit of overhead by regmap shouldn't matter here) > > You are the second person to point that out. (the swab) What is done > here is swapping two bytes, no endianess or anything and x86 only. > > As for the const and the regmap or priv data. This driver will only > ever load once, if at all. There is really no need for abstractions, > additional memory use or cycles. > > So i will ignore the complaints. Feel free to emphasize them if you do > not agree with me doing that. One more point is that the memory based PC has different colors. So the led names can not be shared between io and mem. This is an inconsistency between those machines. mem offers individual control of red and green, where yellow becomes visible when turning both on io on the other hand exposes yellow and (green|red) The driver currently exposes what hardware offers and nothing more. I still have the idea to potentially add yellow for mem as a helper to get the user-level view consistent. But not sure if that idea is any good, even if it would probably be a patch on top. Henning > > > + 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; > > > > Where does that device actually sit on ? Some generic card ? Some > > ASIC or FPGA ? > > It is called P2SB and there some GPIO chip which i believe has no > generic driver to it yet. > > > It seems this driver is instantiated by another one, which already > > knows what device we're actually dealing with (as it sets > > plat->devmode). Why not letting that parent device also tell the io > > resource to this driver ? > > > > > + while (ipcled->value) { > > > + cdev = &ipcled->cdev; > > > + cdev->brightness_set = simatic_ipc_led_set_io; > > > + cdev->brightness_get = simatic_ipc_led_get_io; > > > + if (type == IORESOURCE_MEM) { > > > + cdev->brightness_set = > > > simatic_ipc_led_set_mem; > > > + cdev->brightness_get = > > > simatic_ipc_led_get_mem; > > > + } > > > > Why not if/else ? > > Ok. > > > > + 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 led_driver = { > > > > Why not calling it simatic_ipc_led_driver ? > > Because it is a name with scope only in that file, but hey i will > change it. Also in wdt > > regards, > Henning > > > > + .probe = simatic_ipc_leds_probe, > > > + .driver = { > > > + .name = KBUILD_MODNAME, > > > + }, > > > +}; > > > + > > > +module_platform_driver(led_driver); > > > + > > > +MODULE_LICENSE("GPL v2"); > > > +MODULE_ALIAS("platform:" KBUILD_MODNAME); > > > +MODULE_AUTHOR("Henning Schild <henning.schild@siemens.com>"); > > > > > > > > > --mtx > > >
On 27.03.21 10:46, Henning Schild wrote: >> In this case, they seem to be assigned to certain specific functions >> (by physical labels on the box), so IMHO the LED names should reflect >> that in some ways. > > The choice for "status" was because of > >>> /* Miscelleaus functions. Use functions above if you can. */ > > And those known names do not really come with an explanation of their > meaning. Names like "bluetooth" seem obvious, but "activity" or > "indicator" leave a lot of room for speculation. Maybe we should revise these and add more functions ? Can you find out some more details, what these LEDs really had been intented for ? --mtx
diff --git a/drivers/leds/Kconfig b/drivers/leds/Kconfig index b6742b4231bf..5c8558a4fa60 100644 --- a/drivers/leds/Kconfig +++ b/drivers/leds/Kconfig @@ -937,4 +937,7 @@ source "drivers/leds/trigger/Kconfig" comment "LED Blink" source "drivers/leds/blink/Kconfig" +comment "Simple LED drivers" +source "drivers/leds/simple/Kconfig" + endif # NEW_LEDS diff --git a/drivers/leds/Makefile b/drivers/leds/Makefile index 2a698df9da57..2de7fdd8d629 100644 --- a/drivers/leds/Makefile +++ b/drivers/leds/Makefile @@ -111,3 +111,6 @@ obj-$(CONFIG_LEDS_TRIGGERS) += trigger/ # LED Blink obj-$(CONFIG_LEDS_BLINK) += 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..0f7e6320e10d --- /dev/null +++ b/drivers/leds/simple/simatic-ipc-leds.c @@ -0,0 +1,210 @@ +// 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[32]; + struct led_classdev cdev; +}; + +static struct simatic_ipc_led simatic_ipc_leds_io[] = { + {1 << 15, "simatic-ipc:green:" LED_FUNCTION_STATUS "-1" }, + {1 << 7, "simatic-ipc:yellow:" LED_FUNCTION_STATUS "-1" }, + {1 << 14, "simatic-ipc:red:" LED_FUNCTION_STATUS "-2" }, + {1 << 6, "simatic-ipc:yellow:" LED_FUNCTION_STATUS "-2" }, + {1 << 13, "simatic-ipc:red:" LED_FUNCTION_STATUS "-3" }, + {1 << 5, "simatic-ipc:yellow:" LED_FUNCTION_STATUS "-3" }, + {0, ""}, +}; + +/* 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, "simatic-ipc:red:" LED_FUNCTION_STATUS "-1"}, + {0x500 + 0x1A8, "simatic-ipc:green:" LED_FUNCTION_STATUS "-1"}, + {0x500 + 0x1C8, "simatic-ipc:red:" LED_FUNCTION_STATUS "-2"}, + {0x500 + 0x1D0, "simatic-ipc:green:" LED_FUNCTION_STATUS "-2"}, + {0x500 + 0x1E0, "simatic-ipc:red:" LED_FUNCTION_STATUS "-3"}, + {0x500 + 0x198, "simatic-ipc:green:" LED_FUNCTION_STATUS "-3"}, + {0, ""}, +}; + +static struct resource simatic_ipc_led_io_res = + DEFINE_RES_IO_NAMED(SIMATIC_IPC_LED_PORT_BASE, SZ_1, KBUILD_MODNAME); + +static DEFINE_SPINLOCK(reg_lock); + +static void simatic_ipc_led_set_io(struct led_classdev *led_cd, + enum led_brightness brightness) +{ + struct simatic_ipc_led *led = + container_of(led_cd, struct simatic_ipc_led, cdev); + 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 = + container_of(led_cd, struct simatic_ipc_led, cdev); + + 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 = + container_of(led_cd, struct simatic_ipc_led, cdev); + + 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 = + container_of(led_cd, struct simatic_ipc_led, cdev); + + 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) +{ + struct simatic_ipc_platform *plat; + struct device *dev = &pdev->dev; + struct simatic_ipc_led *ipcled; + struct led_classdev *cdev; + struct resource *res; + int err, type; + u32 *p; + + plat = pdev->dev.platform_data; + + switch (plat->devmode) { + case SIMATIC_IPC_DEVICE_227D: + case SIMATIC_IPC_DEVICE_427E: + res = &simatic_ipc_led_io_res; + ipcled = simatic_ipc_leds_io; + /* the 227D is high on while 427E is low on, invert the struct + * we have + */ + 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; + cdev->brightness_set = simatic_ipc_led_set_io; + cdev->brightness_get = simatic_ipc_led_get_io; + if (type == IORESOURCE_MEM) { + cdev->brightness_set = simatic_ipc_led_set_mem; + cdev->brightness_get = simatic_ipc_led_get_mem; + } + 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 led_driver = { + .probe = simatic_ipc_leds_probe, + .driver = { + .name = KBUILD_MODNAME, + }, +}; + +module_platform_driver(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 | 210 +++++++++++++++++++++++++ 5 files changed, 229 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