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
