[PATCH v4 2/2] backlight: Add new lm3509 backlight driver
Daniel Thompson
daniel.thompson at linaro.org
Mon Mar 25 11:28:24 UTC 2024
^^^
Also not copied to LKML...
On Sun, Mar 10, 2024 at 02:52:57PM +0100, Patrick Gansterer wrote:
> This is a general driver for LM3509 backlight chip of TI.
> LM3509 is High Efficiency Boost for White LEDs and/or OLED Displays with
> Dual Current Sinks. This driver supports OLED/White LED select, brightness
> control and sub/main control.
> The datasheet can be found at http://www.ti.com/product/lm3509.
>
> Signed-off-by: Patrick Gansterer <paroga at paroga.com>
Overall looks good but there are some review comments inline below.
> diff --git a/drivers/video/backlight/lm3509_bl.c b/drivers/video/backlight/lm3509_bl.c
> new file mode 100644
> index 000000000000..696ec8aab6aa
> --- /dev/null
> +++ b/drivers/video/backlight/lm3509_bl.c
> @@ -0,0 +1,338 @@
> <snip>
> +struct lm3509_bl {
> + struct regmap *regmap;
> + struct backlight_device *bl_main;
> + struct backlight_device *bl_sub;
> + struct gpio_desc *reset_gpio;
> +};
> +
> +struct lm3509_bl_led_pdata {
What does the p stand for here?
(only asking because pdata was the idiomatic form for platform data and
this driver only uses DT-only so I'm finding pdata values everywhere
really confusing)
> + const char *label;
> + int led_sources;
> + u32 brightness;
> + u32 max_brightness;
> +};
> +
> +static void lm3509_reset(struct lm3509_bl *data)
> +{
> + if (data->reset_gpio) {
> + gpiod_set_value(data->reset_gpio, 1);
> + udelay(1);
> + gpiod_set_value(data->reset_gpio, 0);
> + udelay(10);
> + }
> +}
> +
> <snip>
> +
> +static struct backlight_device *
> +lm3509_backlight_register(struct device *dev, const char *name_suffix,
> + struct lm3509_bl *data,
> + const struct backlight_ops *ops,
> + const struct lm3509_bl_led_pdata *pdata)
> +
> +{
> + struct backlight_device *bd;
> + struct backlight_properties props;
> + const char *label = pdata->label;
> + char name[64];
> +
> + memset(&props, 0, sizeof(props));
> + props.type = BACKLIGHT_RAW;
> + props.brightness = pdata->brightness;
> + props.max_brightness = pdata->max_brightness;
Please set props.scale appropriately for this device (given it only has
32 brightness levels I assume it is non-linear?).
> +
> + if (!label) {
> + snprintf(name, sizeof(name), "lm3509-%s-%s", dev_name(dev),
> + name_suffix);
> + label = name;
> + }
> +
> + bd = devm_backlight_device_register(dev, label, dev, data, ops, &props);
> + if (bd)
> + backlight_update_status(bd);
> +
> + return bd;
> +}
> +
> <snip>
> +
> +static int lm3509_probe(struct i2c_client *client)
> +{
> + struct lm3509_bl *data;
> + struct device *dev = &client->dev;
> + int ret;
> + bool oled_mode = false;
> + unsigned int reg_gp_val = 0;
> + struct lm3509_bl_led_pdata pdata[LM3509_NUM_SINKS];
> + u32 rate_of_change = 0;
> +
> + if (!i2c_check_functionality(client->adapter, I2C_FUNC_I2C)) {
> + dev_err(dev, "i2c functionality check failed\n");
> + return -EOPNOTSUPP;
> + }
> +
> + data = devm_kzalloc(dev, sizeof(struct lm3509_bl), GFP_KERNEL);
> + if (!data)
> + return -ENOMEM;
> +
> + data->regmap = devm_regmap_init_i2c(client, &lm3509_regmap);
> + if (IS_ERR(data->regmap))
> + return PTR_ERR(data->regmap);
> + i2c_set_clientdata(client, data);
> +
> + data->reset_gpio = devm_gpiod_get_optional(dev, "reset", GPIOD_OUT_LOW);
> + if (IS_ERR(data->reset_gpio))
> + return dev_err_probe(dev, PTR_ERR(data->reset_gpio),
> + "Failed to get 'reset' gpio\n");
> +
> + lm3509_reset(data);
> +
> + memset(pdata, 0, sizeof(pdata));
> + ret = lm3509_parse_dt_node(dev, pdata);
> + if (ret)
> + return ret;
> +
> + oled_mode = of_property_read_bool(dev->of_node, "ti,oled-mode");
> +
> + if (!of_property_read_u32(dev->of_node,
> + "ti,brightness-rate-of-change-us",
> + &rate_of_change)) {
> + switch (rate_of_change) {
> + case 51:
> + reg_gp_val = 0;
> + break;
> + case 13000:
> + reg_gp_val = BIT(REG_GP_RMP1_BIT);
> + break;
> + case 26000:
> + reg_gp_val = BIT(REG_GP_RMP0_BIT);
> + break;
> + case 52000:
> + reg_gp_val = BIT(REG_GP_RMP0_BIT) |
> + BIT(REG_GP_RMP1_BIT);
> + break;
> + default:
> + dev_warn(dev, "invalid rate of change %u\n",
> + rate_of_change);
> + break;
> + }
> + }
> +
> + if (pdata[0].led_sources ==
> + (BIT(LM3509_SINK_MAIN) | BIT(LM3509_SINK_SUB)))
> + reg_gp_val |= BIT(REG_GP_UNI_BIT);
> + if (oled_mode)
> + reg_gp_val |= BIT(REG_GP_OLED_BIT);
> +
> + ret = regmap_write(data->regmap, REG_GP, reg_gp_val);
> + if (ret < 0)
> + return ret;
Is this the first time we write to the peripheral? If so the error path
is probably worth a dev_err_probe() (I don't think regmap_write() logs
anything on failure to write).
> + if (pdata[0].led_sources) {
> + data->bl_main = lm3509_backlight_register(
> + dev, "main", data, &lm3509_main_ops, &pdata[0]);
> + if (IS_ERR(data->bl_main)) {
> + dev_err(dev, "failed to register main backlight\n");
> + return PTR_ERR(data->bl_main);
This should use dev_err_probe().
> + }
> + }
> +
> + if (pdata[1].led_sources) {
> + data->bl_sub = lm3509_backlight_register(
> + dev, "sub", data, &lm3509_sub_ops, &pdata[1]);
> + if (IS_ERR(data->bl_sub)) {
> + dev_err(dev,
> + "failed to register secondary backlight\n");
> + return PTR_ERR(data->bl_sub);
Another good place for dev_err_probe().
Daniel.
More information about the dri-devel
mailing list