Re: [PATCH] leds: add LED driver for CR0014114 board

[Pavel Machek <pavel@ucw.cz>]

[-- Attachment #1: Type: text/plain, Size: 1700 bytes --]

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.

Normally label differentiates them...?

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

Aha, ok.

> >> +/* 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

Please do the replace for all static symbols.> >> +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.

Please take a look at devm_led_classdev_register() and friends. It
should be possible to simplify code without races.

Thanks,
									Pavel
-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 181 bytes --]