Hi Pavel,
thanks for looking into this.
Am Tue, 2 Mar 2021 21:54:52 +0100
schrieb Pavel Machek <pavel@xxxxxx>:
> 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
>
