Hello Pavel! Thank you for your reply. On 12.08.17 12:30, Pavel Machek wrote: > Hi! > > On Sat 2017-08-12 12:09:35, Oleh Kravchenko wrote: >> This patch adds a LED class driver for the RGB LEDs found on >> the Crane Merchandising System CR0014114 LEDs board. > > What kind of hardware is this? It's from vending machines produced by Crane Merchandising System http://cranems.com/ >> Driver creates LED devices with name written using the following >> pattern "LABEL-{N}:{red,green,blue}:". > > Is the "-{N} suffix needed? It's number of RGB LED, board has 6 LEDs. >> Signed-off-by: Oleh Kravchenko <oleg@xxxxxxxxxx> >> --- >> .../devicetree/bindings/leds/leds-cr0014114.txt | 23 ++ >> .../devicetree/bindings/vendor-prefixes.txt | 1 + > > You'll need to cc device tree maintainers to get their acks. Also you > should probably cc: Jacek and me. Okay, I will add them. >> @@ -76,6 +76,18 @@ config LEDS_BCM6358 >> This option enables support for LEDs connected to the BCM6358 >> LED HW controller accessed via MMIO registers. >> >> +config LEDS_CR0014114 >> + tristate "LED Support for Crane Merchandising Systems CR0014114" >> + depends on LEDS_CLASS >> + depends on SPI >> + depends on OF >> + help >> + The CR0014114 LED board used in vending machines produced >> + by Crane Merchandising Systems. >> + >> + This driver creates LED devices with name written using the >> + following pattern "LABEL-{N}:{red,green,blue}:". > > How special hardware is this? Does it make sense to ask this question > for x86 users, for example? I think it's possible to connect it to x86 hardware, but I don't try it. Board need not only SPI connection, but good power supply. >> +/* CR0014114 SPI commands */ >> +#define CR0014114_SET_BRIGHTNESS 0x80 >> +#define CR0014114_INIT_REENUMERATE 0x81 >> +#define CR0014114_NEXT_REENUMERATE 0x82 > > can we s/cr0014114/cr00/g, or something? There are local to the > module, so they can be shorter... Sure, may be CMD_SET_BRIGHTNESS >> +static void cr0014114_test(struct cr0014114 *priv) >> +{ >> + unsigned int mdelay; >> + size_t i; >> + struct led_classdev *ldev; >> + >> + /* blink all LEDs in 500 milliseconds */ >> + mdelay = 500 / priv->leds_count - CR0014114_FW_DELAY_MSEC; >> + if (mdelay < CR0014114_FW_DELAY_MSEC) >> + mdelay = CR0014114_FW_DELAY_MSEC; >> + >> + for (i = 0; i < priv->leds_count; i++) { >> + ldev = &priv->leds[i].ldev; >> + >> + ldev->brightness_set(ldev, CR0014114_MAX_BRIGHTNESS); >> + msleep(mdelay); >> + ldev->brightness_set(ldev, LED_OFF); >> + } >> +} > > I'd remove this. I will do it. >> +fail: >> + while (i--) >> + led_classdev_unregister(&priv->leds[i].ldev); > > Can devm_* be used to simplify this? I think no, because it will cause race condition. > Thanks, > Pavel > -- Best regards, Oleh Kravchenko
Attachment:
signature.asc
Description: OpenPGP digital signature
