Re: [PATCH v9 07/15] leds: lp50xx: Add the LP50XX family of the RGB LED driver

[Jacek Anaszewski]

Dan,

On 9/26/19 1:59 PM, Dan Murphy wrote:
> Jacek
> 
> On 9/25/19 4:27 PM, Jacek Anaszewski wrote:
>> Dan,
>>
>> On 9/25/19 7:46 PM, Dan Murphy wrote:
> 
> <big_snip>
> 
>>> +
>>> +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);
>>> +ÂÂÂ const struct lp50xx_chip_info *led_chip = led->priv->chip_info;
>>> +ÂÂÂ struct led_mc_color_entry *color_data;
>>> +ÂÂÂ u8 led_offset, reg_val, reg_color_offset;
>>> +ÂÂÂ int ret = 0;
>>> +
>>> +ÂÂÂ mutex_lock(&led->priv->lock);
>>> +
>>> +ÂÂÂ if (led->ctrl_bank_enabled)
>>> +ÂÂÂÂÂÂÂ reg_val = led_chip->bank_brt_reg;
>>> +ÂÂÂ else
>>> +ÂÂÂÂÂÂÂ reg_val = led_chip->led_brightness0_reg +
>>> +ÂÂÂÂÂÂÂÂÂÂÂÂÂ led->led_number;
>>> +
>>> +ÂÂÂ ret = regmap_write(led->priv->regmap, reg_val, brightness);
>>> +ÂÂÂ if (ret) {
>>> +ÂÂÂÂÂÂÂ dev_err(&led->priv->client->dev,
>>> +ÂÂÂÂÂÂÂÂÂÂÂ "Cannot write brightness value %d\n", ret);
>>> +ÂÂÂÂÂÂÂ goto out;
>>> +ÂÂÂ }
>>> +
>>> +ÂÂÂ list_for_each_entry(color_data, &led->mc_cdev.color_list, list) {
>>> +ÂÂÂÂÂÂÂ if (color_data->led_color_id == LED_COLOR_ID_RED)
>>> +ÂÂÂÂÂÂÂÂÂÂÂ reg_color_offset = 0;
>>> +ÂÂÂÂÂÂÂ else if (color_data->led_color_id == LED_COLOR_ID_GREEN)
>>> +ÂÂÂÂÂÂÂÂÂÂÂ reg_color_offset = 1;
>>> +ÂÂÂÂÂÂÂ else if (color_data->led_color_id == LED_COLOR_ID_BLUE)
>>> +ÂÂÂÂÂÂÂÂÂÂÂ reg_color_offset = 2;
>>> +ÂÂÂÂÂÂÂ else
>>> +ÂÂÂÂÂÂÂÂÂÂÂ continue;
>> This else case is quite erroneous. Sheer continue is just ignoring
>> serious problem. I'd log the problem and return -EINVAL.
> Ack.
>>
>> Also, you could have a macro for mapping color_id to offset.
> If the code needed to do this more then once I could justify a macro.Â
> But this is the only instance of this check.

OK, at first glance I thought we could have a formula for that,
but this is not the case.

-- 
Best regards,
Jacek Anaszewski