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

[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.

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
> > > >    
> > >     
> >   
>