Hi Jacek,
Thanks for your comments, please see my replies below.
On 14.08.17 22:44, Jacek Anaszewski wrote:
> Hi Oleh,
>
> Thanks for the patch. Please see my comments below.
>
> On 08/14/2017 01:53 PM, Oleh Kravchenko wrote:
>> This patch adds a LED class driver for the RGB LEDs found on
>> the Crane Merchandising System CR0014114 LEDs board.
>>
>> Driver creates LED devices with name written using the following
>> pattern "LABEL-{N}:{red,green,blue}:".
> LED device naming is already defined to "devicename:colour:function"
> in Documentation/leds/leds-class.txt. Please stick to this scheme.
I tried to follow this rule with this pattern "LABEL-{N}:{red,green,blue}:".
What is wrong with it?
Can I use this pattern?
cr0014114:red:0
cr0014114:green:0
cr0014114:blue:0
cr0014114:red:1
cr0014114:green:1
cr0014114:blue:1
cr0014114:red:2
cr0014114:green:2
cr0014114:blue:2
...
>
>> +static void cr0014114_calc_crc(u8 *buf, const size_t len)
>> +{
>> + u8 crc;
>> + size_t i;
>> +
>> + for (i = 1, crc = 1; i < len - 1; i++)
>> + crc += buf[i];
>> +
>> + crc |= BIT(7);
>> +
>> + /* special case when CRC matches to SPI commands */
>> + switch (crc) {
>> + case CMD_SET_BRIGHTNESS:
>> + case CMD_INIT_REENUMERATE:
>> + case CMD_NEXT_REENUMERATE:
>> + crc = 0xfe;
>> + break;
>> + }
> Wouldn't "if" fit better here?
I tried it's looks ugly for me, but if you insist I will use "if".
if (crc == CMD_SET_BRIGHTNESS ||
crc == CMD_INIT_REENUMERATE ||
crc == CMD_NEXT_REENUMERATE)
crc = 0xfe;
>> +
>> + buf[len - 1] = crc;
>> +}
>> +
>> +static void cr0014114_leds_work(struct work_struct *work)
>> +{
>> + int ret;
>> + size_t i;
>> + struct cr0014114 *priv = container_of(work,
>> + struct cr0014114, leds_work);
>> + u8 data[priv->leds_count + 2];
>> + unsigned long udelay, now = jiffies;
>> +
>> + /* to avoid SPI mistiming with firmware we should wait some time */
>> + if (time_after(priv->leds_delay, now)) {
>> + udelay = jiffies_to_usecs(priv->leds_delay - now);
>> + usleep_range(udelay, udelay + 1);
>> + }
>> +
>> + data[0] = CMD_SET_BRIGHTNESS;
>> + for (i = 0; i < priv->leds_count; i++)
>> + data[i + 1] = priv->leds[i].brightness;
> Why do you set three LEDs at once? We expose each LED as a single
> LED class device in the LED subsystem.
Unfortunately board should receive brightness
for all 18 LEDs (or 6 RGB LEDs) at one time.
On CR0014114 you can't update only one LED, you should update them all :)
>> + cr0014114_calc_crc(data, sizeof(data));
>> +
>> + ret = spi_write(priv->spi, data, sizeof(data));
> Could you please describe the layout of registers driving the brightness
> of each LED ?
+----+-----------------------------------+----+
| CMD| BRIGHTNESS |CRC |
+----+-----------------------------------+----+
| | LED0| LED1| LED2| LED3| LED4| LED5| |
| +-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+ |
| |R|G|B|R|G|B|R|G|B|R|G|B|R|G|B|R|G|B| |
| 1 +-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+ 1 |
| |1|1|1|1|1|1|1|1|1|1|1|1|1|1|1|1|1|1| |
| +-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+ |
| | 18 | |
+----+-----------------------------------+----+
| 20 |
+---------------------------------------------+
Boards can be connected to the chain:
SPI -> board0 -> board1 -> board2 ..
in this case only BRIGHTNESS length (18 -> 36 -> 54) will be increased.
>> + if (ret)
>> + dev_err(priv->dev, "spi_write() error %d\n", ret);
>> +
>> + priv->leds_delay = jiffies + msecs_to_jiffies(DEF_FW_DELAY_MSEC);
>> +}
>> +
>> +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 - DEF_FW_DELAY_MSEC;
>> + if (mdelay < DEF_FW_DELAY_MSEC)
>> + mdelay = DEF_FW_DELAY_MSEC;
>> +
>> + for (i = 0; i < priv->leds_count; i++) {
>> + ldev = &priv->leds[i].ldev;
>> +
>> + ldev->brightness_set(ldev, DEF_MAX_BRIGHTNESS);
>> + msleep(mdelay);
>> + ldev->brightness_set(ldev, LED_OFF);
>> + }
>> +}
> Is this function really required? It seems to mimic timer trigger.
No, this function is not really needed.
I will remove it.
>> + queue_work(led->priv->wq, &led->priv->leds_work);
>> +}
>> +
>> +static int cr0014114_led_create(struct cr0014114 *priv,
>> + struct cr0014114_led *led,
>> + size_t id,
>> + const char *color)
>> +{
>> + int ret;
>> +
>> + led->priv = priv;
>> + snprintf(led->name, sizeof(led->name), "%s-%zd:%s:",
>> + priv->label, id, color);
>> +
>> + led->ldev.name = led->name;
>> + led->ldev.brightness = LED_OFF;
>> + led->ldev.max_brightness = DEF_MAX_BRIGHTNESS;
>> + led->ldev.brightness_set = cr0014114_set_brightness;
> Please use brightness_set_blocking op and remove workqueue.
If driver will send data often than 10 ms, board will hangs up.
How I can workaround it with brightness_set_blocking?
>> +
>> + /* setup recount timer to workaround buggy firmware */
>> + mod_timer(&priv->recount_timer, jiffies + DEF_RECOUNT_DELAY);
> Could you share some details on why it is needed?
Board (or boards if they in chain) time to time losing synchronization
with SPI.
So we should recount LEDs on them to avoid this.
--
Best regards,
Oleh Kravchenko
