Am Fri, 5 Mar 2021 19:25:55 +0100
schrieb Henning Schild <henning.schild@xxxxxxxxxxx>:
> Am Wed, 3 Mar 2021 21:56:15 +0100
> schrieb Henning Schild <henning.schild@xxxxxxxxxxx>:
>
> > Am Wed, 3 Mar 2021 21:48:21 +0100
> > schrieb Henning Schild <henning.schild@xxxxxxxxxxx>:
> >
> > > Am Wed, 3 Mar 2021 20:31:34 +0100
> > > schrieb Pavel Machek <pavel@xxxxxx>:
> > >
> > > > 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
> > > >
> > >
> >
>
