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@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.
> @@ -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:
signature.asc
Description: Digital signature
