Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

drivers: led: Fix uninitialized variable #82067

Closed
wants to merge 1 commit into from
Closed

drivers: led: Fix uninitialized variable #82067

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion drivers/led/is31fl3194.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -80,7 +80,7 @@ static int is31fl3194_set_color(const struct device *dev, uint32_t led, uint8_t
{
const struct is31fl3194_config *config = dev->config;
const struct led_info *info = is31fl3194_led_to_info(config, led);
int ret;
int ret = 0;
Copy link
Collaborator

@simonguinot simonguinot Nov 26, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Well, first there is actually no case where ret is tested/used uninitialized with the current code. This can only happen if the color_mapping is invalid. But it is tested by is31fl3194_check_config().

That being said, it is interesting to consolidate the code...

About this patch, it is not optimal to initialize ret with 0. This opens the door to update the LED even if the channels have not been updated.

Since there is no point in continuing at sending I2C I/Os if one of them fails, then I would prefer something like that:

                ret = i2c_reg_write_byte_dt(&config->bus, led_channels[i], value);
                if (ret != 0) 
                        goto err;
        }

        ret = i2c_reg_write_byte_dt(&config->bus,
                                    IS31FL3194_UPDATE_REG,
                                    IS31FL3194_UPDATE_VAL);

err:
        if (ret != 0) {
                LOG_ERR("%s: LED write failed: %d", dev->name, ret);
        }

        return ret;
}

Copy link
Collaborator Author

@rruuaanng rruuaanng Nov 30, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry to have kept you waiting, I think your suggestion is very good. After ret is initialized to 0, we should remove the if, I will fix it in two commits :)

Edit

Hold on, we seem to have forgotten the i2c_reg_write_byte_dt above. Its main function is to execute i2c_reg_write_byte_dt in the if below after all the i2c_reg_write_byte_dt above succeed. It seems that it makes sense to initialize ret to 0.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry I don't see what is wrong this the code example I proposed. The last call to i2c_reg_write_byte_dt is only executed if the color registers are updated.


if (info == NULL) {
return -ENODEV;
Expand Down
Loading