Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider: - [Medium] leds: lm3533: unmasked upper bits in lm3533_led_setup() regmap update --
commit 65c330f7377fc6ec85de7b076ec8dcd4d456b54b Author: Svyatoslav Ryhel <[email protected]> video: leds: backlight: lm3533: Support getting LED sources from DT This commit adds Control Bank to HVLED and LVLED muxing support to the lm3533 LED and backlight drivers based on the led-sources device tree property. > diff --git a/drivers/leds/leds-lm3533.c b/drivers/leds/leds-lm3533.c > index 7259097bd6a71..a1aeff70a90fc 100644 > --- a/drivers/leds/leds-lm3533.c > +++ b/drivers/leds/leds-lm3533.c > @@ -26,6 +26,11 @@ > #define LM3533_ALS_CHANNEL_LV_MIN 1 > #define LM3533_ALS_CHANNEL_LV_MAX 2 > > +#define LM3533_REG_OUTPUT_CONF1 0x10 > +#define OUTPUT_CONF1_SHIFT 2 > +#define OUTPUT_LVLED_MASK 0x3 > +#define LM3533_REG_OUTPUT_CONF2 0x11 > +#define OUTPUT_CONF2_SHIFT 6 > #define LM3533_REG_CTRLBANK_BCONF_BASE 0x1b > #define LM3533_REG_PATTERN_ENABLE 0x28 > #define LM3533_REG_PATTERN_LOW_TIME_BASE 0x71 > @@ -54,6 +59,9 @@ struct lm3533_led { > u32 max_current; > u32 pwm; > > + int num_leds; > + u32 leds[LM3533_LVCTRLBANK_MAX]; > + > bool have_als; > }; > > @@ -622,8 +630,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: Medium] In drivers/leds/leds-lm3533.c, when lm3533_led_setup() configures LVLED4 (3) or LVLED5 (4) on control bank D, E, or F (led->id > 0), output_cfg_mask and output_cfg_val have bits [9:6] set. Left-shifting them by OUTPUT_CONF1_SHIFT (2) without masking to 8 bits leaves bits [11:8] set when updating the 8-bit register LM3533_REG_OUTPUT_CONF1. In drivers/base/regmap/regmap.c:_regmap_update_bits(): ret = _regmap_read(map, reg, &orig); if (ret != 0) return ret; tmp = orig & ~mask; tmp |= val & mask; if (force_write || (tmp != orig) || map->force_write_field) { ret = _regmap_write(map, reg, tmp); Because orig is an 8-bit value (<= 0xff) and val & mask has bits [11:8] set, tmp exceeds 0xff and tmp != orig is always true. This forces a register write with tmp > 0xff, which causes regmap_smbus_byte_reg_write() in drivers/base/regmap/regmap-i2c.c: if (val > 0xff || reg > 0xff) return -EINVAL; to reject the write with -EINVAL and fail lm3533_led_probe() on SMBus byte-data I2C controllers, or issue an unnecessary write on raw I2C controllers. Should the mask and value passed to regmap_update_bits() for LM3533_REG_OUTPUT_CONF1 be masked to 8 bits (e.g. & 0xff or u8)? -- Sashiko AI review ยท https://sashiko.dev/#/patchset/[email protected]?part=1
