diff mbox series

[2/4] leds: simatic-ipc-leds: add new driver for Siemens Industial PCs

Message ID 20210302163309.25528-3-henning.schild@siemens.com
State New
Headers show
Series add device drivers for Siemens Industrial PCs | expand

Commit Message

Henning Schild March 2, 2021, 4:33 p.m. UTC
From: 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: Gerd Haeussler <gerd.haeussler.ext@siemens.com>
Signed-off-by: Henning Schild <henning.schild@siemens.com>
---
 drivers/leds/Kconfig            |  11 ++
 drivers/leds/Makefile           |   1 +
 drivers/leds/simatic-ipc-leds.c | 224 ++++++++++++++++++++++++++++++++
 3 files changed, 236 insertions(+)
 create mode 100644 drivers/leds/simatic-ipc-leds.c

Comments

Pavel Machek March 2, 2021, 8:54 p.m. UTC | #1
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
Henning Schild March 3, 2021, 1:11 p.m. UTC | #2
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

>
Henning Schild March 3, 2021, 5:37 p.m. UTC | #3
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

>
Pavel Machek March 3, 2021, 5:40 p.m. UTC | #4
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
Henning Schild March 3, 2021, 6:49 p.m. UTC | #5
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
Pavel Machek March 3, 2021, 7:27 p.m. UTC | #6
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
Pavel Machek March 3, 2021, 7:31 p.m. UTC | #7
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
Henning Schild March 3, 2021, 8:48 p.m. UTC | #8
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
Henning Schild March 3, 2021, 8:56 p.m. UTC | #9
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  

>
Henning Schild March 5, 2021, 6:25 p.m. UTC | #10
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    

> >   

>
Henning Schild March 6, 2021, 12:54 p.m. UTC | #11
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

> > > >    

> > >     

> >   

>
Henning Schild March 6, 2021, 1:06 p.m. UTC | #12
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

> > > > >      

> > > >       

> > >     

> >   

>
Pavel Machek March 15, 2021, 11:49 a.m. UTC | #13
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 mbox series

Patch

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(&reg_lock, flags);
+
+	val = inw(SIMATIC_IPC_LED_PORT_BASE);
+	if (brightness == LED_OFF)
+		outw(val | led->value, SIMATIC_IPC_LED_PORT_BASE);
+	else
+		outw(val & ~led->value, SIMATIC_IPC_LED_PORT_BASE);
+
+	spin_unlock_irqrestore(&reg_lock, flags);
+}
+
+static enum led_brightness simatic_ipc_led_get_io(struct led_classdev *led_cd)
+{
+	struct simatic_ipc_led *led =
+		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>");