Re: [PATCH 2/2] backlight: add ktd3137 support

[Roman Vivchar]

On Tuesday, November 3rd, 2026 at 4:15 PM, Daniel Thompson <danielt@xxxxxxxxxx> wrote:

> On Mon, Nov 02, 2026 at 09:58:49PM +0200, Roman Vivchar via B4 Relay wrote:
> > From: Roman Vivchar <rva333@xxxxxxxxxxxxxx>
> >
> > Add support for the KTD3137 display backlight IC. It's used in various
> > Xiaomi phones, such as Xiaomi Redmi Note 9.

...

> > +static int ktd3137_init(struct ktd3137 *ktd)
> > +{
> > +	u32 val = KTD3137_CONTROL_DEFAULT | KTD3137_CONTROL_LINEAR;
> > +	int ret;
> > +
> > +	ret = regmap_write(ktd->regmap, KTD3137_REG_CONTROL, val);
> 
> Nitpicking but val seems a bit pointless here.

Agreed.

> 
> 
> > +	if (ret)
> > +		return ret;
> > +
> > +	ret = regmap_write(ktd->regmap, KTD3137_REG_PWM, KTD3137_PWM_DEFAULT);
> > +	if (ret)
> > +		return ret;

...

> > +
> > +	props.type = BACKLIGHT_RAW;
> > +	props.max_brightness = KTD3137_MAX_BRIGHTNESS;
> > +	props.brightness = KTD3137_DEFAULT_BRIGHTNESS;
> 
> No need for KTD3137_DEFAULT_BRIGHTNESS. Just use KTD3137_MAX_BRIGHTNESS.

Ack.

> 
> 
> > +	props.scale = BACKLIGHT_SCALE_LINEAR;
> > +	device_property_read_u32(ktd->dev, "default-brightness", &props.brightness);
> > +
> > +	backlight_dev = devm_backlight_device_register(ktd->dev,
> > +						       "ktd3137-backlight",
> > +						       ktd->dev, ktd,
> > +						       &ktd3137_backlight_ops,
> > +						       &props);
> > +	if (IS_ERR(backlight_dev))
> > +		return dev_err_probe(ktd->dev, PTR_ERR(backlight_dev),
> > +				     "failed to register backlight device\n");
> 
> dev_err_cast_probe()

Didn't know about this one, thanks.

> 
> 
> > +
> > +	i2c_set_clientdata(client, backlight_dev);
> > +	return backlight_update_status(backlight_dev);
> > +}
> > +
> > +static void ktd3137_remove(struct i2c_client *client)
> > +{
> > +	struct backlight_device *backlight_dev = i2c_get_clientdata(client);
> > +	struct ktd3137 *ktd = bl_get_data(backlight_dev);
> > +
> > +	backlight_dev->props.brightness = 0;
> > +	backlight_update_status(backlight_dev);
> > +
> > +	gpiod_set_value_cansleep(ktd->enable_gpio, 0);
> 
> Is this needed?

The datasheet says "When HWEN is set high, ... if SCL/SDA’s pull up voltage
is much less than VIN voltage, it can cause a small leakage current ...,
there will be around 7uA additional leakage current from VIN in this standby
mode."

It's better to put chip into shutdown mode, which won't cause power leakage
according to the datasheet, and ensures that I2C registers will be reset in
case chip needs to be cleanly re-initialized.

All other comments will be addressed in v2.

Best regards,
Roman

> 
> > +}
> 
> 
> Daniel.
>