| Message ID | 20210302163309.25528-3-henning.schild@siemens.com |
|---|---|
| State | New |
| Headers | show |
| Series | add device drivers for Siemens Industrial PCs | expand |
Hi! > This driver adds initial support for several devices from Siemens. It is > based on a platform driver introduced in an earlier commit. Ok. > diff --git a/drivers/leds/Makefile b/drivers/leds/Makefile > index 2a698df9da57..c15e1e3c5958 100644 > --- a/drivers/leds/Makefile > +++ b/drivers/leds/Makefile > @@ -93,6 +93,7 @@ obj-$(CONFIG_LEDS_TURRIS_OMNIA) += leds-turris-omnia.o > obj-$(CONFIG_LEDS_WM831X_STATUS) += leds-wm831x-status.o > obj-$(CONFIG_LEDS_WM8350) += leds-wm8350.o > obj-$(CONFIG_LEDS_WRAP) += leds-wrap.o > +obj-$(CONFIG_LEDS_SIEMENS_SIMATIC_IPC) += simatic-ipc-leds.o Let's put this into drivers/leds/simple. You'll have to create it. > + * > + * This program is free software; you can redistribute it and/or modify > + * it under the terms of the GNU General Public License version 2 as > + * published by the Free Software Foundation. > + */ Remove? > +static struct simatic_ipc_led simatic_ipc_leds_io[] = { > + {1 << 15, "simatic-ipc:green:run-stop"}, > + {1 << 7, "simatic-ipc:yellow:run-stop"}, > + {1 << 14, "simatic-ipc:red:error"}, > + {1 << 6, "simatic-ipc:yellow:error"}, > + {1 << 13, "simatic-ipc:red:maint"}, > + {1 << 5, "simatic-ipc:yellow:maint"}, > + {0, ""}, > +}; Please use names consistent with other systems, this is user visible. If you have two-color power led, it should be :green:power... See include/dt-bindings/leds/common.h . Please avoid // comments in the code. > +module_init(simatic_ipc_led_init_module); > +module_exit(simatic_ipc_led_exit_module); No need for such verbosity for functions that are static. > +MODULE_LICENSE("GPL"); GPL v2? Best regards, Pavel -- http://www.livejournal.com/~pavelmachek
Hi Pavel, thanks for looking into this. Am Tue, 2 Mar 2021 21:54:52 +0100 schrieb Pavel Machek <pavel@ucw.cz>: > Hi! > > > This driver adds initial support for several devices from Siemens. > > It is based on a platform driver introduced in an earlier commit. > > Ok. > > > diff --git a/drivers/leds/Makefile b/drivers/leds/Makefile > > index 2a698df9da57..c15e1e3c5958 100644 > > --- a/drivers/leds/Makefile > > +++ b/drivers/leds/Makefile > > @@ -93,6 +93,7 @@ obj-$(CONFIG_LEDS_TURRIS_OMNIA) += > > leds-turris-omnia.o obj-$(CONFIG_LEDS_WM831X_STATUS) += > > leds-wm831x-status.o obj-$(CONFIG_LEDS_WM8350) += > > leds-wm8350.o obj-$(CONFIG_LEDS_WRAP) += > > leds-wrap.o +obj-$(CONFIG_LEDS_SIEMENS_SIMATIC_IPC) += > > simatic-ipc-leds.o > > Let's put this into drivers/leds/simple. You'll have to create it. Ok will do > > + * > > + * This program is free software; you can redistribute it and/or > > modify > > + * it under the terms of the GNU General Public License version 2 > > as > > + * published by the Free Software Foundation. > > + */ > > Remove? Sure, was found in wdt as well. Thx > > +static struct simatic_ipc_led simatic_ipc_leds_io[] = { > > + {1 << 15, "simatic-ipc:green:run-stop"}, > > + {1 << 7, "simatic-ipc:yellow:run-stop"}, > > + {1 << 14, "simatic-ipc:red:error"}, > > + {1 << 6, "simatic-ipc:yellow:error"}, > > + {1 << 13, "simatic-ipc:red:maint"}, > > + {1 << 5, "simatic-ipc:yellow:maint"}, > > + {0, ""}, > > +}; > > Please use names consistent with other systems, this is user > visible. If you have two-color power led, it should be > :green:power... See include/dt-bindings/leds/common.h . Well we wanted to pick names that are printed on the devices and would like to stick to those. Has been a discussion ... Can we have symlinks to have multiple names per LED? How strong would you feel about us using our names? > Please avoid // comments in the code. Ok > > +module_init(simatic_ipc_led_init_module); > > +module_exit(simatic_ipc_led_exit_module); > > No need for such verbosity for functions that are static. > > > +MODULE_LICENSE("GPL"); > > GPL v2? Will do. Stay tuned for v2. regards, Henning > Best regards, > Pavel >
Am Tue, 2 Mar 2021 21:54:52 +0100 schrieb Pavel Machek <pavel@ucw.cz>: > Hi! > > > This driver adds initial support for several devices from Siemens. > > It is based on a platform driver introduced in an earlier commit. > > Ok. > > > diff --git a/drivers/leds/Makefile b/drivers/leds/Makefile > > index 2a698df9da57..c15e1e3c5958 100644 > > --- a/drivers/leds/Makefile > > +++ b/drivers/leds/Makefile > > @@ -93,6 +93,7 @@ obj-$(CONFIG_LEDS_TURRIS_OMNIA) += > > leds-turris-omnia.o obj-$(CONFIG_LEDS_WM831X_STATUS) += > > leds-wm831x-status.o obj-$(CONFIG_LEDS_WM8350) += > > leds-wm8350.o obj-$(CONFIG_LEDS_WRAP) += > > leds-wrap.o +obj-$(CONFIG_LEDS_SIEMENS_SIMATIC_IPC) += > > simatic-ipc-leds.o > > Let's put this into drivers/leds/simple. You'll have to create it. Can you please go into detail why? We plan to add more devices in the future, which might in fact make this a little less simple. But we can discuss that when the time is right and start with simple. regards, Henning > > + * > > + * This program is free software; you can redistribute it and/or > > modify > > + * it under the terms of the GNU General Public License version 2 > > as > > + * published by the Free Software Foundation. > > + */ > > Remove? > > > +static struct simatic_ipc_led simatic_ipc_leds_io[] = { > > + {1 << 15, "simatic-ipc:green:run-stop"}, > > + {1 << 7, "simatic-ipc:yellow:run-stop"}, > > + {1 << 14, "simatic-ipc:red:error"}, > > + {1 << 6, "simatic-ipc:yellow:error"}, > > + {1 << 13, "simatic-ipc:red:maint"}, > > + {1 << 5, "simatic-ipc:yellow:maint"}, > > + {0, ""}, > > +}; > > Please use names consistent with other systems, this is user > visible. If you have two-color power led, it should be > :green:power... See include/dt-bindings/leds/common.h . > > Please avoid // comments in the code. > > > +module_init(simatic_ipc_led_init_module); > > +module_exit(simatic_ipc_led_exit_module); > > No need for such verbosity for functions that are static. > > > +MODULE_LICENSE("GPL"); > > GPL v2? > > Best regards, > Pavel >
Hi! > > > diff --git a/drivers/leds/Makefile b/drivers/leds/Makefile > > > index 2a698df9da57..c15e1e3c5958 100644 > > > --- a/drivers/leds/Makefile > > > +++ b/drivers/leds/Makefile > > > @@ -93,6 +93,7 @@ obj-$(CONFIG_LEDS_TURRIS_OMNIA) += > > > leds-turris-omnia.o obj-$(CONFIG_LEDS_WM831X_STATUS) += > > > leds-wm831x-status.o obj-$(CONFIG_LEDS_WM8350) += > > > leds-wm8350.o obj-$(CONFIG_LEDS_WRAP) += > > > leds-wrap.o +obj-$(CONFIG_LEDS_SIEMENS_SIMATIC_IPC) += > > > simatic-ipc-leds.o > > > > Let's put this into drivers/leds/simple. You'll have to create it. > > Can you please go into detail why? We plan to add more devices in the > future, which might in fact make this a little less simple. But we can > discuss that when the time is right and start with simple. There's already way too many drivers in the directory, and your driver is very different from drivers for camera flash (for example). Best regards, Pavel -- http://www.livejournal.com/~pavelmachek
Am Wed, 3 Mar 2021 18:40:40 +0100 schrieb Pavel Machek <pavel@ucw.cz>: > Hi! > > > > > diff --git a/drivers/leds/Makefile b/drivers/leds/Makefile > > > > index 2a698df9da57..c15e1e3c5958 100644 > > > > --- a/drivers/leds/Makefile > > > > +++ b/drivers/leds/Makefile > > > > @@ -93,6 +93,7 @@ obj-$(CONFIG_LEDS_TURRIS_OMNIA) > > > > += leds-turris-omnia.o > > > > obj-$(CONFIG_LEDS_WM831X_STATUS) += leds-wm831x-status.o > > > > obj-$(CONFIG_LEDS_WM8350) += leds-wm8350.o > > > > obj-$(CONFIG_LEDS_WRAP) += leds-wrap.o > > > > +obj-$(CONFIG_LEDS_SIEMENS_SIMATIC_IPC) += > > > > simatic-ipc-leds.o > > > > > > Let's put this into drivers/leds/simple. You'll have to create > > > it. > > > > Can you please go into detail why? We plan to add more devices in > > the future, which might in fact make this a little less simple. But > > we can discuss that when the time is right and start with simple. > > There's already way too many drivers in the directory, and your driver > is very different from drivers for camera flash (for example). Understood, the whole Makefile Kconfig thingy? Henning > Best regards, > Pavel
On Wed 2021-03-03 19:49:56, Henning Schild wrote: > Am Wed, 3 Mar 2021 18:40:40 +0100 > schrieb Pavel Machek <pavel@ucw.cz>: > > > Hi! > > > > > > > diff --git a/drivers/leds/Makefile b/drivers/leds/Makefile > > > > > index 2a698df9da57..c15e1e3c5958 100644 > > > > > --- a/drivers/leds/Makefile > > > > > +++ b/drivers/leds/Makefile > > > > > @@ -93,6 +93,7 @@ obj-$(CONFIG_LEDS_TURRIS_OMNIA) > > > > > += leds-turris-omnia.o > > > > > obj-$(CONFIG_LEDS_WM831X_STATUS) += leds-wm831x-status.o > > > > > obj-$(CONFIG_LEDS_WM8350) += leds-wm8350.o > > > > > obj-$(CONFIG_LEDS_WRAP) += leds-wrap.o > > > > > +obj-$(CONFIG_LEDS_SIEMENS_SIMATIC_IPC) += > > > > > simatic-ipc-leds.o > > > > > > > > Let's put this into drivers/leds/simple. You'll have to create > > > > it. > > > > > > Can you please go into detail why? We plan to add more devices in > > > the future, which might in fact make this a little less simple. But > > > we can discuss that when the time is right and start with simple. > > > > There's already way too many drivers in the directory, and your driver > > is very different from drivers for camera flash (for example). > > Understood, the whole Makefile Kconfig thingy? You'll need Makefile + Kconfig, yes. No need for CONFIG_LEDS_SIMPLE. Best regards, Pavel -- http://www.livejournal.com/~pavelmachek
Hi! > > > +static struct simatic_ipc_led simatic_ipc_leds_io[] = { > > > + {1 << 15, "simatic-ipc:green:run-stop"}, > > > + {1 << 7, "simatic-ipc:yellow:run-stop"}, > > > + {1 << 14, "simatic-ipc:red:error"}, > > > + {1 << 6, "simatic-ipc:yellow:error"}, > > > + {1 << 13, "simatic-ipc:red:maint"}, > > > + {1 << 5, "simatic-ipc:yellow:maint"}, > > > + {0, ""}, > > > +}; > > > > Please use names consistent with other systems, this is user > > visible. If you have two-color power led, it should be > > :green:power... See include/dt-bindings/leds/common.h . > > Well we wanted to pick names that are printed on the devices and would > like to stick to those. Has been a discussion ... > Can we have symlinks to have multiple names per LED? No symlinks. We plan to have command line tool to manipulate LEDs, aliases might be possible there. > How strong would you feel about us using our names? Strongly. :-) Do you have a picture how the leds look like? Best regards, Pavel -- http://www.livejournal.com/~pavelmachek
Am Wed, 3 Mar 2021 20:31:34 +0100 schrieb Pavel Machek <pavel@ucw.cz>: > Hi! > > > > > +static struct simatic_ipc_led simatic_ipc_leds_io[] = { > > > > + {1 << 15, "simatic-ipc:green:run-stop"}, > > > > + {1 << 7, "simatic-ipc:yellow:run-stop"}, > > > > + {1 << 14, "simatic-ipc:red:error"}, > > > > + {1 << 6, "simatic-ipc:yellow:error"}, > > > > + {1 << 13, "simatic-ipc:red:maint"}, > > > > + {1 << 5, "simatic-ipc:yellow:maint"}, > > > > + {0, ""}, > > > > +}; > > > > > > Please use names consistent with other systems, this is user > > > visible. If you have two-color power led, it should be > > > :green:power... See include/dt-bindings/leds/common.h . > > > > Well we wanted to pick names that are printed on the devices and > > would like to stick to those. Has been a discussion ... > > Can we have symlinks to have multiple names per LED? > > No symlinks. We plan to have command line tool to manipulate LEDs, > aliases might be possible there. Sounds like a future plan. sysfs and "cat" "echo" are mighty tools and "everything is a file" is the best idea ever. So i would say any aliasing should live in the kernel, but that is just me. Tools will just get out of sync, be missing in busybox or a random yocto ... or whichever distro you like. On the other hand you have "complexity should be userland" ... i do not have the answer. > > How strong would you feel about us using our names? > > Strongly. :-) OK, will try to find a match where possible. > Do you have a picture how the leds look like? I could even find chassis photos in our internal review but that would be too much. Our idea is probably the same as yours. We want the same names across all devices. But we struggle with colors because on some boxes we have red+green, while other offer yellow ... implemented in HW and messing with red+green in some cases. But so far we only looked at Siemens devices and thought we could get our own "namespace". To be honest i could not even tell how our names map on the known ones, but we will do our best to find a match. They all are "high-level" so "power" and other basic things are not exposed. regards, Henning > Best regards, > Pavel
Am Wed, 3 Mar 2021 21:48:21 +0100 schrieb Henning Schild <henning.schild@siemens.com>: > Am Wed, 3 Mar 2021 20:31:34 +0100 > schrieb Pavel Machek <pavel@ucw.cz>: > > > Hi! > > > > > > > +static struct simatic_ipc_led simatic_ipc_leds_io[] = { > > > > > + {1 << 15, "simatic-ipc:green:run-stop"}, > > > > > + {1 << 7, "simatic-ipc:yellow:run-stop"}, > > > > > + {1 << 14, "simatic-ipc:red:error"}, > > > > > + {1 << 6, "simatic-ipc:yellow:error"}, > > > > > + {1 << 13, "simatic-ipc:red:maint"}, > > > > > + {1 << 5, "simatic-ipc:yellow:maint"}, > > > > > + {0, ""}, > > > > > +}; > > > > > > > > Please use names consistent with other systems, this is user > > > > visible. If you have two-color power led, it should be > > > > :green:power... See include/dt-bindings/leds/common.h . > > > > > > Well we wanted to pick names that are printed on the devices and > > > would like to stick to those. Has been a discussion ... > > > Can we have symlinks to have multiple names per LED? > > > > No symlinks. We plan to have command line tool to manipulate LEDs, > > aliases might be possible there. > > Sounds like a future plan. sysfs and "cat" "echo" are mighty tools and > "everything is a file" is the best idea ever. So i would say any > aliasing should live in the kernel, but that is just me. Tools will > just get out of sync, be missing in busybox or a random yocto ... or > whichever distro you like. > On the other hand you have "complexity should be userland" ... i do > not have the answer. My personal horror would be systemd-ledd or some dracut snipet for initrd. But that would be a generic led class discussion ... that tool. > > > How strong would you feel about us using our names? > > > > Strongly. :-) > > OK, will try to find a match where possible. Do we happen to have a description of the existing names, to find a fit for ours? In the header you pointed out i only found names without "meaning" regards, Henning > > > Do you have a picture how the leds look like? > > I could even find chassis photos in our internal review but that would > be too much. > > Our idea is probably the same as yours. We want the same names across > all devices. But we struggle with colors because on some boxes we have > red+green, while other offer yellow ... implemented in HW and messing > with red+green in some cases. > > But so far we only looked at Siemens devices and thought we could get > our own "namespace". > > To be honest i could not even tell how our names map on the known > ones, but we will do our best to find a match. They all are > "high-level" so "power" and other basic things are not exposed. > > regards, > Henning > > > Best regards, > > Pavel >
Am Wed, 3 Mar 2021 21:56:15 +0100 schrieb Henning Schild <henning.schild@siemens.com>: > Am Wed, 3 Mar 2021 21:48:21 +0100 > schrieb Henning Schild <henning.schild@siemens.com>: > > > Am Wed, 3 Mar 2021 20:31:34 +0100 > > schrieb Pavel Machek <pavel@ucw.cz>: > > > > > Hi! > > > > > > > > > +static struct simatic_ipc_led simatic_ipc_leds_io[] = { > > > > > > + {1 << 15, "simatic-ipc:green:run-stop"}, > > > > > > + {1 << 7, "simatic-ipc:yellow:run-stop"}, > > > > > > + {1 << 14, "simatic-ipc:red:error"}, > > > > > > + {1 << 6, "simatic-ipc:yellow:error"}, > > > > > > + {1 << 13, "simatic-ipc:red:maint"}, > > > > > > + {1 << 5, "simatic-ipc:yellow:maint"}, > > > > > > + {0, ""}, > > > > > > +}; > > > > > > > > > > Please use names consistent with other systems, this is user > > > > > visible. If you have two-color power led, it should be > > > > > :green:power... See include/dt-bindings/leds/common.h . > > > > > > > > Well we wanted to pick names that are printed on the devices and > > > > would like to stick to those. Has been a discussion ... > > > > Can we have symlinks to have multiple names per LED? > > > > > > No symlinks. We plan to have command line tool to manipulate LEDs, > > > aliases might be possible there. > > > > Sounds like a future plan. sysfs and "cat" "echo" are mighty tools > > and "everything is a file" is the best idea ever. So i would say any > > aliasing should live in the kernel, but that is just me. Tools will > > just get out of sync, be missing in busybox or a random yocto ... or > > whichever distro you like. > > On the other hand you have "complexity should be userland" ... i do > > not have the answer. > > My personal horror would be systemd-ledd or some dracut snipet for > initrd. But that would be a generic led class discussion ... that > tool. > > > > > How strong would you feel about us using our names? > > > > > > Strongly. :-) > > > > OK, will try to find a match where possible. > > Do we happen to have a description of the existing names, to find a > fit for ours? In the header you pointed out i only found names without > "meaning" I had a closer look at the several LED_FUNCTION_ while i could probably find a match for the names we had in mind ... - {1 << 14, "simatic-ipc:red:error"}, + {1 << 14, "simatic-ipc:red:" LED_FUNCTION_FAULT }, I still do not understand what those mean. Going over the kernel sources many have only one single grep-hit in the tree. LED_FUNCTION_ not having a single one in drivers/leds Others are found in one dts and in that header ... 2 hits in the tree, maybe i should add my favorite strings ;) LED_FUNCTION_FLASH vs LED_FUNCTION_TORCH ...? Sound like timing, not function. Let us say i match the three "error", "run-stop", "maint" to LED_FUNCTION_* I would have a really hard time finding matches for other LEDs i did not even propose. One example being disks ... many of them, would i be allowed to LED_FUNCTION_DISK "0" LED_FUNCTION_DISK "1" ... they would all have the same colors. Maybe you explain the idea behind choosing only from that namespace? My guess would be high-level software being able to toggle leds totally indep of the device it runs on. Such software would have to do some really nasty directory listing, name parsing, dealing with multiple hits. Does such generic software already exist, maybe that would help me understand my "mapping problems" ? The current class encodes, color, function and name into "name". Maybe i am all wrong and should go for {1 << 14, "simatic-ipc-error:red:" LED_FUNCTION_STATUS } {1 << 15, "simatic-ipc-run-stop:green:" LED_FUNCTION_STATUS} {... , "simatic-ipc-hdd0:red:" LED_FUNCTION_DISK } {... , "simatic-ipc-hdd1:red:" LED_FUNCTION_DISK } so appending my wanted name to the name before the first :, and use functions i "understand" after the second : regards, Henning > regards, > Henning > > > > > > Do you have a picture how the leds look like? > > > > I could even find chassis photos in our internal review but that > > would be too much. > > > > Our idea is probably the same as yours. We want the same names > > across all devices. But we struggle with colors because on some > > boxes we have red+green, while other offer yellow ... implemented > > in HW and messing with red+green in some cases. > > > > But so far we only looked at Siemens devices and thought we could > > get our own "namespace". > > > > To be honest i could not even tell how our names map on the known > > ones, but we will do our best to find a match. They all are > > "high-level" so "power" and other basic things are not exposed. > > > > regards, > > Henning > > > > > Best regards, > > > Pavel > > >
Am Fri, 5 Mar 2021 19:25:55 +0100 schrieb Henning Schild <henning.schild@siemens.com>: > Am Wed, 3 Mar 2021 21:56:15 +0100 > schrieb Henning Schild <henning.schild@siemens.com>: > > > Am Wed, 3 Mar 2021 21:48:21 +0100 > > schrieb Henning Schild <henning.schild@siemens.com>: > > > > > Am Wed, 3 Mar 2021 20:31:34 +0100 > > > schrieb Pavel Machek <pavel@ucw.cz>: > > > > > > > Hi! > > > > > > > > > > > +static struct simatic_ipc_led simatic_ipc_leds_io[] = { > > > > > > > + {1 << 15, "simatic-ipc:green:run-stop"}, > > > > > > > + {1 << 7, "simatic-ipc:yellow:run-stop"}, > > > > > > > + {1 << 14, "simatic-ipc:red:error"}, > > > > > > > + {1 << 6, "simatic-ipc:yellow:error"}, > > > > > > > + {1 << 13, "simatic-ipc:red:maint"}, > > > > > > > + {1 << 5, "simatic-ipc:yellow:maint"}, > > > > > > > + {0, ""}, > > > > > > > +}; > > > > > > > > > > > > Please use names consistent with other systems, this is user > > > > > > visible. If you have two-color power led, it should be > > > > > > :green:power... See include/dt-bindings/leds/common.h . > > > > > > > > > > > > > > > > Well we wanted to pick names that are printed on the devices > > > > > and would like to stick to those. Has been a discussion ... > > > > > Can we have symlinks to have multiple names per LED? > > > > > > > > No symlinks. We plan to have command line tool to manipulate > > > > LEDs, aliases might be possible there. > > > > > > Sounds like a future plan. sysfs and "cat" "echo" are mighty tools > > > and "everything is a file" is the best idea ever. So i would say > > > any aliasing should live in the kernel, but that is just me. > > > Tools will just get out of sync, be missing in busybox or a > > > random yocto ... or whichever distro you like. > > > On the other hand you have "complexity should be userland" ... i > > > do not have the answer. > > > > My personal horror would be systemd-ledd or some dracut snipet for > > initrd. But that would be a generic led class discussion ... that > > tool. > > > > > > > How strong would you feel about us using our names? > > > > > > > > Strongly. :-) > > > > > > OK, will try to find a match where possible. > > > > Do we happen to have a description of the existing names, to find a > > fit for ours? In the header you pointed out i only found names > > without "meaning" > > I had a closer look at the several LED_FUNCTION_ while i could > probably find a match for the names we had in mind ... > > - {1 << 14, "simatic-ipc:red:error"}, > + {1 << 14, "simatic-ipc:red:" LED_FUNCTION_FAULT }, > > I still do not understand what those mean. Going over the kernel > sources many have only one single grep-hit in the tree. > LED_FUNCTION_ not having a single one in drivers/leds > Others are found in one dts and in that header ... 2 hits in the tree, > maybe i should add my favorite strings ;) > > LED_FUNCTION_FLASH vs LED_FUNCTION_TORCH ...? Sound like timing, not > function. > > Let us say i match the three "error", "run-stop", "maint" to > LED_FUNCTION_* > > I would have a really hard time finding matches for other LEDs i did > not even propose. One example being disks ... many of them, would i be > allowed to > > LED_FUNCTION_DISK "0" > LED_FUNCTION_DISK "1" > ... > > they would all have the same colors. > > Maybe you explain the idea behind choosing only from that namespace? > My guess would be high-level software being able to toggle leds > totally indep of the device it runs on. Such software would have to > do some really nasty directory listing, name parsing, dealing with > multiple hits. Does such generic software already exist, maybe that > would help me understand my "mapping problems" ? > > The current class encodes, color, function and name into "name". > > Maybe i am all wrong and should go for > > {1 << 14, "simatic-ipc-error:red:" LED_FUNCTION_STATUS } > {1 << 15, "simatic-ipc-run-stop:green:" LED_FUNCTION_STATUS} > {... , "simatic-ipc-hdd0:red:" LED_FUNCTION_DISK } > {... , "simatic-ipc-hdd1:red:" LED_FUNCTION_DISK } > > so appending my wanted name to the name before the first :, and use > functions i "understand" after the second : Found the docs and the check script. It has been a while since i read those docs. But that script fails on bus=platform quick workaround would be fi +elif [ "$bus" = "platform" ]; then + true else echo "Unknown device type." exit 1 But i guess it would be nice to get some sort of platform information, device vendor etc. I see two options for pattern i could choose "green:" LED_FUNCTION_STATUS "-0" -> platform bus patch needed, no plaform information simatic-ipc:green:" LED_FUNCTION_STATUS "-0" -> platform bus patch needed, will fail with "Unknown devicename" Without further advice i will choose the second for v2. That is also what i.e. "tpacpi" on my laptop looks like. I would also be happy to include a fix to that script. My suggestion would be to allow bus=platform, in which case a "devicename" will be required and is allowed to have any value. regards, Henning > regards, > Henning > > > > regards, > > Henning > > > > > > > > > Do you have a picture how the leds look like? > > > > > > I could even find chassis photos in our internal review but that > > > would be too much. > > > > > > Our idea is probably the same as yours. We want the same names > > > across all devices. But we struggle with colors because on some > > > boxes we have red+green, while other offer yellow ... implemented > > > in HW and messing with red+green in some cases. > > > > > > But so far we only looked at Siemens devices and thought we could > > > get our own "namespace". > > > > > > To be honest i could not even tell how our names map on the known > > > ones, but we will do our best to find a match. They all are > > > "high-level" so "power" and other basic things are not exposed. > > > > > > regards, > > > Henning > > > > > > > Best regards, > > > > Pavel > > > > > > > > > >
Am Sat, 6 Mar 2021 13:54:53 +0100 schrieb Henning Schild <henning.schild@siemens.com>: > Am Fri, 5 Mar 2021 19:25:55 +0100 > schrieb Henning Schild <henning.schild@siemens.com>: > > > Am Wed, 3 Mar 2021 21:56:15 +0100 > > schrieb Henning Schild <henning.schild@siemens.com>: > > > > > Am Wed, 3 Mar 2021 21:48:21 +0100 > > > schrieb Henning Schild <henning.schild@siemens.com>: > > > > > > > Am Wed, 3 Mar 2021 20:31:34 +0100 > > > > schrieb Pavel Machek <pavel@ucw.cz>: > > > > > > > > > Hi! > > > > > > > > > > > > > +static struct simatic_ipc_led simatic_ipc_leds_io[] = { > > > > > > > > + {1 << 15, "simatic-ipc:green:run-stop"}, > > > > > > > > + {1 << 7, "simatic-ipc:yellow:run-stop"}, > > > > > > > > + {1 << 14, "simatic-ipc:red:error"}, > > > > > > > > + {1 << 6, "simatic-ipc:yellow:error"}, > > > > > > > > + {1 << 13, "simatic-ipc:red:maint"}, > > > > > > > > + {1 << 5, "simatic-ipc:yellow:maint"}, > > > > > > > > + {0, ""}, > > > > > > > > +}; > > > > > > > > > > > > > > Please use names consistent with other systems, this is > > > > > > > user visible. If you have two-color power led, it should > > > > > > > be :green:power... See include/dt-bindings/leds/common.h . > > > > > > > > > > > > > > > > > > > Well we wanted to pick names that are printed on the devices > > > > > > and would like to stick to those. Has been a discussion ... > > > > > > Can we have symlinks to have multiple names per LED? > > > > > > > > > > > > > > > > No symlinks. We plan to have command line tool to manipulate > > > > > LEDs, aliases might be possible there. > > > > > > > > Sounds like a future plan. sysfs and "cat" "echo" are mighty > > > > tools and "everything is a file" is the best idea ever. So i > > > > would say any aliasing should live in the kernel, but that is > > > > just me. Tools will just get out of sync, be missing in busybox > > > > or a random yocto ... or whichever distro you like. > > > > On the other hand you have "complexity should be userland" ... i > > > > do not have the answer. > > > > > > My personal horror would be systemd-ledd or some dracut snipet for > > > initrd. But that would be a generic led class discussion ... that > > > tool. > > > > > > > > > How strong would you feel about us using our names? > > > > > > > > > > > > > > > > Strongly. :-) > > > > > > > > OK, will try to find a match where possible. > > > > > > Do we happen to have a description of the existing names, to find > > > a fit for ours? In the header you pointed out i only found names > > > without "meaning" > > > > I had a closer look at the several LED_FUNCTION_ while i could > > probably find a match for the names we had in mind ... > > > > - {1 << 14, "simatic-ipc:red:error"}, > > + {1 << 14, "simatic-ipc:red:" LED_FUNCTION_FAULT }, > > > > I still do not understand what those mean. Going over the kernel > > sources many have only one single grep-hit in the tree. > > LED_FUNCTION_ not having a single one in drivers/leds > > Others are found in one dts and in that header ... 2 hits in the > > tree, maybe i should add my favorite strings ;) > > > > LED_FUNCTION_FLASH vs LED_FUNCTION_TORCH ...? Sound like timing, not > > function. > > > > Let us say i match the three "error", "run-stop", "maint" to > > LED_FUNCTION_* > > > > I would have a really hard time finding matches for other LEDs i did > > not even propose. One example being disks ... many of them, would i > > be allowed to > > > > LED_FUNCTION_DISK "0" > > LED_FUNCTION_DISK "1" > > ... > > > > they would all have the same colors. > > > > Maybe you explain the idea behind choosing only from that namespace? > > My guess would be high-level software being able to toggle leds > > totally indep of the device it runs on. Such software would have to > > do some really nasty directory listing, name parsing, dealing with > > multiple hits. Does such generic software already exist, maybe that > > would help me understand my "mapping problems" ? > > > > The current class encodes, color, function and name into "name". > > > > Maybe i am all wrong and should go for > > > > {1 << 14, "simatic-ipc-error:red:" LED_FUNCTION_STATUS } > > {1 << 15, "simatic-ipc-run-stop:green:" LED_FUNCTION_STATUS} > > {... , "simatic-ipc-hdd0:red:" LED_FUNCTION_DISK } > > {... , "simatic-ipc-hdd1:red:" LED_FUNCTION_DISK } > > > > so appending my wanted name to the name before the first :, and use > > functions i "understand" after the second : > > Found the docs and the check script. It has been a while since i read > those docs. > > But that script fails on bus=platform > > quick workaround would be > > fi > +elif [ "$bus" = "platform" ]; then > + true > else > echo "Unknown device type." > exit 1 > > But i guess it would be nice to get some sort of platform information, > device vendor etc. > > I see two options for pattern i could choose > > "green:" LED_FUNCTION_STATUS "-0" > -> platform bus patch needed, no plaform information > > simatic-ipc:green:" LED_FUNCTION_STATUS "-0" > -> platform bus patch needed, will fail with "Unknown devicename" > > Without further advice i will choose the second for v2. That is also > what i.e. "tpacpi" on my laptop looks like. > > I would also be happy to include a fix to that script. My suggestion > would be to allow bus=platform, in which case a "devicename" will be > required and is allowed to have any value. Furthermore it might be good to catch that in the led core instead of that script. Maybe warn() on dev registration when function/color/name seem off. Could later become "return -EINVAL" Henning > regards, > Henning > > > regards, > > Henning > > > > > > > regards, > > > Henning > > > > > > > > > > > > Do you have a picture how the leds look like? > > > > > > > > I could even find chassis photos in our internal review but that > > > > would be too much. > > > > > > > > Our idea is probably the same as yours. We want the same names > > > > across all devices. But we struggle with colors because on some > > > > boxes we have red+green, while other offer yellow ... > > > > implemented in HW and messing with red+green in some cases. > > > > > > > > But so far we only looked at Siemens devices and thought we > > > > could get our own "namespace". > > > > > > > > To be honest i could not even tell how our names map on the > > > > known ones, but we will do our best to find a match. They all > > > > are "high-level" so "power" and other basic things are not > > > > exposed. > > > > > > > > regards, > > > > Henning > > > > > > > > > Best regards, > > > > > Pavel > > > > > > > > > > > > > > >
Hi! > > I would also be happy to include a fix to that script. My suggestion > > would be to allow bus=platform, in which case a "devicename" will be > > required and is allowed to have any value. > > Furthermore it might be good to catch that in the led core instead of > that script. Maybe warn() on dev registration when function/color/name > seem off. Could later become "return -EINVAL" I'm not sure if we want to change _existing_ funny names. Would document such as below be helpful? Could you describe the LEDs you have in similar format? Best regards, Pavel -*- org -*- It is somehow important to provide consistent interface to the userland. LED devices have one problem there, and that is naming of directories in /sys/class/leds. It would be nice if userland would just know right "name" for given LED function, but situation got more complex. Anyway, if backwards compatibility is not an issue, new code should use one of the "good" names from this list, and you should extend the list where applicable. Bad names are listed, too; in case you are writing application that wants to use particular feature, you should probe for good name, first, but then try the bad ones, too. * Keyboards Good: "input*:*:capslock" Good: "input*:*:scrolllock" Good: "input*:*:numlock" Bad: "shift-key-light" (Motorola Droid 4, capslock) Set of common keyboard LEDs, going back to PC AT or so. Good: "platform::kbd_backlight" Bad: "tpacpi::thinklight" (IBM/Lenovo Thinkpads) Bad: "lp5523:kb{1,2,3,4,5,6}" (Nokia N900) Frontlight/backlight of main keyboard. Bad: "button-backlight" (Motorola Droid 4) Some phones have touch buttons below screen; it is different from main keyboard. And this is their backlight. * Sound subsystem Good: "platform:*:mute" Good: "platform:*:micmute" LEDs on notebook body, indicating that sound input / output is muted. * System notification Good: "status-led:{red,green,blue}" (Motorola Droid 4) Bad: "lp5523:{r,g,b}" (Nokia N900) Phones usually have multi-color status LED. * Power management Good: "platform:*:charging" (allwinner sun50i) * Screen Good: ":backlight" (Motorola Droid 4)
diff --git a/drivers/leds/Kconfig b/drivers/leds/Kconfig index b6742b4231bf..3ee6fc613a0a 100644 --- a/drivers/leds/Kconfig +++ b/drivers/leds/Kconfig @@ -928,6 +928,17 @@ config LEDS_ACER_A500 This option enables support for the Power Button LED of Acer Iconia Tab A500. +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. + comment "Flash and Torch LED drivers" source "drivers/leds/flash/Kconfig" diff --git a/drivers/leds/Makefile b/drivers/leds/Makefile index 2a698df9da57..c15e1e3c5958 100644 --- a/drivers/leds/Makefile +++ b/drivers/leds/Makefile @@ -93,6 +93,7 @@ obj-$(CONFIG_LEDS_TURRIS_OMNIA) += leds-turris-omnia.o obj-$(CONFIG_LEDS_WM831X_STATUS) += leds-wm831x-status.o obj-$(CONFIG_LEDS_WM8350) += leds-wm8350.o obj-$(CONFIG_LEDS_WRAP) += leds-wrap.o +obj-$(CONFIG_LEDS_SIEMENS_SIMATIC_IPC) += simatic-ipc-leds.o # LED SPI Drivers obj-$(CONFIG_LEDS_CR0014114) += leds-cr0014114.o diff --git a/drivers/leds/simatic-ipc-leds.c b/drivers/leds/simatic-ipc-leds.c new file mode 100644 index 000000000000..760aef1d4ae1 --- /dev/null +++ b/drivers/leds/simatic-ipc-leds.c @@ -0,0 +1,224 @@ +// 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> + * + * This program is free software; you can redistribute it and/or modify + * it under the terms of the GNU General Public License version 2 as + * published by the Free Software Foundation. + */ + +#include <linux/module.h> +#include <linux/kernel.h> +#include <linux/ioport.h> +#include <linux/sizes.h> +#include <linux/leds.h> +#include <linux/platform_device.h> +#include <linux/spinlock.h> +#include <linux/platform_data/x86/simatic-ipc-base.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:run-stop"}, + {1 << 7, "simatic-ipc:yellow:run-stop"}, + {1 << 14, "simatic-ipc:red:error"}, + {1 << 6, "simatic-ipc:yellow:error"}, + {1 << 13, "simatic-ipc:red:maint"}, + {1 << 5, "simatic-ipc:yellow:maint"}, + {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:run-stop"}, + {0x500 + 0x1A8, "simatic-ipc:green:run-stop"}, + {0x500 + 0x1C8, "simatic-ipc:red:error"}, + {0x500 + 0x1D0, "simatic-ipc:green:error"}, + {0x500 + 0x1E0, "simatic-ipc:red:maint"}, + {0x500 + 0x198, "simatic-ipc:green:maint"}, + {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 device *dev = &pdev->dev; + struct led_classdev *cdev; + struct resource *res; + int err, type; + struct simatic_ipc_led *ipcled; + struct simatic_ipc_platform *plat; + 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 (!simatic_ipc_led_memory) + return -ENOMEM; + + /* 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, + }, +}; + +static int __init simatic_ipc_led_init_module(void) +{ + return platform_driver_register(&led_driver); +} + +static void simatic_ipc_led_exit_module(void) +{ + platform_driver_unregister(&led_driver); +} + +module_init(simatic_ipc_led_init_module); +module_exit(simatic_ipc_led_exit_module); + +MODULE_LICENSE("GPL"); +MODULE_ALIAS("platform:" KBUILD_MODNAME); +MODULE_AUTHOR("Henning Schild <henning.schild@siemens.com>");