Re: [PATCH RFC 02/10] backlight: add Kinetic KTD3136 driver

[Lee Jones <lee@kernel.org>]

On Fri, 11 Sep 2026, sashiko-bot@kernel.org wrote:

> Thank you for your contribution! Sashiko AI review found 5 potential issue(s) to consider:
> - [High] The 'vin' regulator is enabled during probe but never disabled on error paths or during module removal.
> - [High] Missing `i2c_set_clientdata()` in probe causes `ktd3136_remove()` to silently skip hardware power-off.
> - [Medium] Missing lock acquisition in `ktd3136_remove` causes a data race with concurrent backlight sysfs updates.
> - [Medium] State mismatch leaves backlight physically ON when initial brightness is 0.
> - [Low] Unnecessary and unused `select REGMAP_I2C` in Kconfig.

Please review, explain or fix the Sashiko review(s).

If this is a set, this request includes the other patches too.

> --
> 
> commit 15d6d172969378ae4661195e2320fcd1b4ed08c1
> Author: YİĞİTCAN KAVAKLI <kavakliyigitcan@gmail.com>
> 
> backlight: add Kinetic KTD3136 driver
> 
> This commit implements a generic Linux backlight driver for the Kinetic
> Technologies KTD3136 36V, 6-channel LED driver. It uses the standard
> backlight class interface over I2C.
> 
> > diff --git a/drivers/video/backlight/Kconfig b/drivers/video/backlight/Kconfig
> > index dbf4ca23a9b6d..010e2ae3fa8cb 100644
> > --- a/drivers/video/backlight/Kconfig
> > +++ b/drivers/video/backlight/Kconfig
> [ ... ]
> > +config BACKLIGHT_KTD3136
> > +	tristate "Backlight Driver for Kinetic KTD3136"
> > +	depends on I2C
> > +	select REGMAP_I2C
> 
> [Severity: Low]
> Does this driver actually use the regmap API? Looking at the code, it appears
> to exclusively use raw I2C SMBus operations (e.g., i2c_smbus_read_byte_data).
> Could this select REGMAP_I2C be safely removed to avoid forcing an unnecessary
> dependency?
> 
> > +	help
> > +	  Say Y to enable the backlight driver for the Kinetic Technologies
> [ ... ]
> > diff --git a/drivers/video/backlight/ktd3136-backlight.c b/drivers/video/backlight/ktd3136-backlight.c
> > new file mode 100644
> > index 0000000000000..a8ebe3b23c99f
> > --- /dev/null
> > +++ b/drivers/video/backlight/ktd3136-backlight.c
> [ ... ]
> > +static int ktd3136_probe(struct i2c_client *client)
> > +{
> > +	struct device *dev = &client->dev;
> > +	struct backlight_properties props;
> > +	struct ktd3136_data *chip;
> > +	int ret, val;
> > +	u32 def_brightness = KTD3136_DEFAULT_BRIGHTNESS;
> > +	u32 max_brightness = KTD3136_DEFAULT_MAX_BRIGHTNESS;
> > +
> > +	chip = devm_kzalloc(dev, sizeof(*chip), GFP_KERNEL);
> > +	if (!chip)
> > +		return -ENOMEM;
> > +
> > +	chip->client = client;
> > +	mutex_init(&chip->lock);
> 
> [Severity: High]
> Is a call to i2c_set_clientdata(client, chip) missing here? Later in
> ktd3136_remove(), the code calls i2c_get_clientdata(client), which will
> return NULL if it wasn't set, silently bypassing the hardware power-off.
> 
> > +
> > +	chip->vin = devm_regulator_get_optional(dev, "vin");
> > +	if (IS_ERR(chip->vin)) {
> > +		ret = PTR_ERR(chip->vin);
> > +		if (ret != -ENODEV)
> > +			return dev_err_probe(dev, ret, "Failed to get vin regulator\n");
> > +		chip->vin = NULL;
> > +	}
> > +
> > +	if (chip->vin) {
> > +		ret = regulator_enable(chip->vin);
> > +		if (ret)
> > +			return dev_err_probe(dev, ret, "Failed to enable vin regulator\n");
> > +	}
> > +
> > +	chip->enable_gpio = devm_gpiod_get_optional(dev, "enable", GPIOD_OUT_HIGH);
> > +	if (IS_ERR(chip->enable_gpio))
> > +		return dev_err_probe(dev, PTR_ERR(chip->enable_gpio),
> > +				     "Failed to get enable GPIO\n");
> 
> [Severity: High]
> If devm_gpiod_get_optional() fails here, the function returns immediately.
> Does this leave the vin regulator enabled since there is no devm action or
> manual regulator_disable(chip->vin) on this error path?
> 
> [Severity: Medium]
> The enable GPIO is asserted HIGH here (powering the hardware), but
> chip->is_enabled remains false. If the default brightness evaluates to 0,
> backlight_update_status() calls ktd3136_power_off(). Because chip->is_enabled
> is false, ktd3136_power_off() returns early without de-asserting the GPIO.
> Could this state mismatch leave the backlight physically ON when the initial
> brightness is 0?
> 
> > +
> > +	if (chip->enable_gpio)
> > +		usleep_range(2000, 3000);
> [ ... ]
> > +static void ktd3136_remove(struct i2c_client *client)
> > +{
> > +	struct ktd3136_data *chip = i2c_get_clientdata(client);
> > +
> > +	if (chip)
> > +		ktd3136_power_off(chip);
> 
> [Severity: Medium]
> Does ktd3136_power_off(chip) need to be protected by mutex_lock(&chip->lock)
> here? Since the backlight device is registered with
> devm_backlight_device_register, its sysfs interface might still be active
> during .remove(). Could this cause a data race with concurrent sysfs writes
> to the brightness attribute?
> 
> [Severity: High]
> Does the vin regulator need to be disabled here? It was enabled during probe
> but there doesn't seem to be a corresponding regulator_disable() call when the
> driver is unbound.
> 
> > +}
> 
> -- 
> Sashiko AI review · https://sashiko.dev/#/patchset/20260911-ginkgo-submission-final-v1-0-0e68e63a18d4@gmail.com?part=2

-- 
Lee Jones