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

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

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