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

[sashiko-bot@kernel.org]

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.
--

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