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

Am Mon, 15 Mar 2021 12:19:15 +0100
schrieb Pavel Machek [off-list ref]:

quoted

quoted

+       struct led_classdev cdev;
+};
+
+static struct simatic_ipc_led simatic_ipc_leds_io[] = {
+       {1 << 15, "simatic-ipc:green:" LED_FUNCTION_STATUS "-1" },
+       {1 << 7,  "simatic-ipc:yellow:" LED_FUNCTION_STATUS "-1"
},
+       {1 << 14, "simatic-ipc:red:" LED_FUNCTION_STATUS "-2" },
+       {1 << 6,  "simatic-ipc:yellow:" LED_FUNCTION_STATUS "-2"
},
+       {1 << 13, "simatic-ipc:red:" LED_FUNCTION_STATUS "-3" },
+       {1 << 5,  "simatic-ipc:yellow:" LED_FUNCTION_STATUS "-3"
},  

Can you use BIT() macro here? And can it be sorted by the bit
order?  

There's nothing wrong with << and this order is fine.

But I still don't like the naming. simantic-ipc: prefix is
useless. Having 6 status leds is not good, either.

With some of my questions still not being answered i will probably
remove that prefix entirely, not even use "platform:".

And i might stick with 6x "status". Since that allows reflecting the
labels on the machines, while using "above functions if you can"

regards,
Henning

quoted

quoted

+       struct simatic_ipc_led *led =
+               container_of(led_cd, struct simatic_ipc_led,
cdev);  

One line?  

80 columns. It is fine as it is.

Best regards,

								Pavel