lkml.org 
[lkml]   [2019]   [Sep]   [28]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
SubjectRe: [PATCH v9 07/15] leds: lp50xx: Add the LP50XX family of the RGB LED driver
From
Date
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

\
 
 \ /
  Last update: 2019-09-28 12:30    [W:0.138 / U:1.476 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site