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