Re: [PATCH v8 8/9] leds: lp50xx: Add the LP50XX family of the RGB LED driver

[Jacek Anaszewski <jacek.anaszewski@gmail.com>]

Dan,

On 9/23/19 7:56 PM, Dan Murphy wrote:
> Jacek
> 
> On 9/21/19 10:11 AM, Jacek Anaszewski wrote:
>> Dan,
>>
>> On 9/20/19 7:41 PM, Dan Murphy wrote:
>>> Introduce the LP5036/30/24/18/12/9 RGB LED driver.
>>> The difference in these parts are the number of
>>> LED outputs where the:
>>>
>>> LP5036 can control 36 LEDs
>>> LP5030 can control 30 LEDs
>>> LP5024 can control 24 LEDs
>>> LP5018 can control 18 LEDs
>>> LP5012 can control 12 LEDs
>>> LP509 can control 9 LEDs
>>>
>>> The device has the ability to group LED output into control banks
>>> so that multiple LED banks can be controlled with the same mixing and
>>> brightness.  Inversely the LEDs can also be controlled independently.
>>>
>>> Signed-off-by: Dan Murphy<dmurphy@ti.com>
>>> ---
>>>   drivers/leds/Kconfig       |   7 +
>>>   drivers/leds/Makefile      |   1 +
>>>   drivers/leds/leds-lp50xx.c | 785 +++++++++++++++++++++++++++++++++++++
>>>   3 files changed, 793 insertions(+)
>>>   create mode 100644 drivers/leds/leds-lp50xx.c
>>>
>>> diff --git a/drivers/leds/Kconfig b/drivers/leds/Kconfig
>>> index cfb1ebb6517f..1c0cacb100e6 100644
>>> --- a/drivers/leds/Kconfig
>>> +++ b/drivers/leds/Kconfig
>>> @@ -363,6 +363,13 @@ config LEDS_LP3952
>>>         To compile this driver as a module, choose M here: the
>>>         module will be called leds-lp3952.
>>>   +config LEDS_LP50XX
>>> +    tristate "LED Support for TI LP5036/30/24/18 LED driver chip"
>>> +    depends on LEDS_CLASS && REGMAP_I2C
>> && OF
> 
> Not sure why I would add that since we are using fw_node calls not
> of_property calls.
> 
> The fw_node calls are built in as default kernel so these should always
> be available.

Ah, right. Forget it.

[...]
>>> +static int lp50xx_brightness_set(struct led_classdev *cdev,
>>> +                 enum led_brightness brightness)
>>> +{
>>> +    struct lp50xx_led *led = container_of(cdev, struct lp50xx_led,
>>> led_dev);
>>> +    int ret = 0;
>>> +    u8 reg_val;
>>> +
>>> +    mutex_lock(&led->priv->lock);
>>> +
>>> +    if (led->ctrl_bank_enabled)
>>> +        reg_val = led->priv->chip_info->bank_brt_reg;
>>> +    else
>>> +        reg_val = led->priv->chip_info->led_brightness0_reg +
>>> +              led->led_number;
>>> +
>>> +    ret = regmap_write(led->priv->regmap, reg_val, brightness);
>>> +    if (ret)
>>> +        goto err_out;
>>> +
>>> +    ret = lp50xx_set_intensity(led);
>>> +err_out:
>>> +    mutex_unlock(&led->priv->lock);
>>> +    return ret;
>>> +}
>>> +
>>> +static enum led_brightness lp50xx_brightness_get(struct led_classdev
>>> *cdev)
>> Do we really need this op? Is it possible that the device will alter
>> brightness autonomously ? IOW can't we rely on what we've written
>> previously to the hw?
> 
> How can we be sure that the previous I/O actually wrote to the device?

brightness_set* op returned 0?

> If set_brightness fails does the LED class not modify the current
> brightness setting?

It does modify it on every brightness setting op in
led_set_brightness_nosleep().

> So we have mismatched values and with this call back we can refresh the
> right setting.
> 
> But I can remove it if you see no value in doing get_brightness call back.

If write to the device fails it signifies much more serious problem
than resulting mismatched values. Have you experienced that?

>>> +{
>>> +    struct lp50xx_led *led = container_of(cdev, struct lp50xx_led,
>>> led_dev);
>>> +    unsigned int brt_val;
>>> +    u8 reg_val;
>>> +    int ret;
>>> +
>>> +    mutex_lock(&led->priv->lock);
>>> +
>>> +    if (led->ctrl_bank_enabled)
>>> +        reg_val = led->priv->chip_info->bank_brt_reg;
>>> +    else
>>> +        reg_val = led->priv->chip_info->led_brightness0_reg +
>>> led->led_number;
>>> +
>>> +    ret = regmap_read(led->priv->regmap, reg_val, &brt_val);
>>> +
>>> +    mutex_unlock(&led->priv->lock);
>>> +
>>> +    return brt_val;
>>> +}
>>> +
>>> +static int lp50xx_set_color(struct led_classdev_mc *mcled_cdev,
>>> +                int color, int value)
>>> +{
>>> +    struct lp50xx_led *led = mcled_cdev_to_led(mcled_cdev);
>>> +
>>> +    led->led_intensity[color] = value;
>>> +
>>> +    return 0;
>>> +}
>>> +
>>> +static int lp50xx_set_banks(struct lp50xx *priv, u32 led_strings[])
>> This is a bit misleading to introduce "strings" when the function
>> claims to set "banks". Let's have the parameter name "led_banks".
> Ack
>>
>>> +{
>>> +    u8 led_ctrl_enable = 0;
>>> +    u8 led1_ctrl_enable = 0;
>>> +    u8 ctrl_ext = 0;
>> Let's have below instead of the above three variables:
>>
>>     u32 bank_enable_mask = 0;
>>     u8 led_config_lo, led_config_hi;
> 
> Ack but I have to keep the initialization to 0 as the compiler
> complained that these values may not be set.

It was because in your code values are assigned in a loop that
may or may not execute at all, depending on num_leds.
In my example variables will be assigned unconditionally
so there should be no compiler complaints.


-- 
Best regards,
Jacek Anaszewski