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

[Pavel Machek <pavel@ucw.cz>]

[-- Attachment #1: Type: text/plain, Size: 2472 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? 

> Driver creates LED devices with name written using the following
> pattern "LABEL-{N}:{red,green,blue}:".

Is the "-{N} suffix needed?

> Signed-off-by: Oleh Kravchenko <oleg@kaa.org.ua>
> ---
>  .../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.

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

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

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

> +fail:
> +	while (i--)
> +		led_classdev_unregister(&priv->leds[i].ldev);

Can devm_* be used to simplify this?

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