Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider: - [Low] Out-of-bounds bitmask shift causes regcache pollution and unnecessary I2C bus writes for LM3533_REG_OUTPUT_CONF1. --
commit a74553be2d2b60a05a193e9dc35527d3d4a425da Author: Svyatoslav Ryhel <[email protected]> video: leds: backlight: lm3533: Support getting LED sources from DT This commit adds Control Bank to HVLED/LVLED muxing support based on the led-sources defined in the device tree. > diff --git a/drivers/leds/leds-lm3533.c b/drivers/leds/leds-lm3533.c > index ed810c23f30f3..9e07953814fdd 100644 > --- a/drivers/leds/leds-lm3533.c > +++ b/drivers/leds/leds-lm3533.c [ ... ] > @@ -623,8 +631,36 @@ static const struct attribute_group > *lm3533_led_attribute_groups[] = { > > static int lm3533_led_setup(struct lm3533_led *led) > { > + u32 output_cfg_shift = 0; > + u32 output_cfg_val = 0; > + u32 output_cfg_mask = 0; > int ret; > > + if (led->num_leds) { > + for (int i = 0; i < led->num_leds; i++) { > + if (led->leds[i] >= LM3533_LVCTRLBANK_MAX) > + continue; > + > + output_cfg_shift = led->leds[i] * 2; > + output_cfg_val |= led->id << output_cfg_shift; > + output_cfg_mask |= OUTPUT_LVLED_MASK << > output_cfg_shift; > + } > + > + /* LVLED1, LVLED2 and LVLED3 */ > + ret = regmap_update_bits(led->regmap, LM3533_REG_OUTPUT_CONF1, > + output_cfg_mask << OUTPUT_CONF1_SHIFT, > + output_cfg_val << OUTPUT_CONF1_SHIFT); [Severity: Low] Can this shift cause bits to fall outside the 8-bit register width? When led-sources specifies LVLED4 or LVLED5, output_cfg_mask accumulates bits that, when shifted left by OUTPUT_CONF1_SHIFT (which is 2), will exceed 8 bits (for example, forming masks like 0x300 or 0xC00). Because regmap_update_bits operates on unsigned int internally, these out-of-bounds bits are applied to the cached register value. This causes the internal comparison (tmp != orig) to incorrectly evaluate to true, triggering an unnecessary I2C write. Although the I2C subsystem harmlessly truncates the value to 8 bits so the hardware state is not corrupted, does it make sense to apply an explicit 8-bit mask (e.g., & 0xFF) before passing the shifted mask and value to regmap_update_bits() to prevent regcache pollution and redundant bus traffic? > + if (ret) > + return ret; > + > + /* LVLED4 and LVLED5 */ > + ret = regmap_update_bits(led->regmap, LM3533_REG_OUTPUT_CONF2, > + output_cfg_mask >> OUTPUT_CONF2_SHIFT, > + output_cfg_val >> OUTPUT_CONF2_SHIFT); > + if (ret) > + return ret; > + } [ ... ] -- Sashiko AI review ยท https://sashiko.dev/#/patchset/[email protected]?part=14
