From dec5779e6a7b73c6c64c4f75e7fcbf04d3b0aa7c Mon Sep 17 00:00:00 2001 From: Hans de Goede Date: Tue, 2 Nov 2021 23:55:02 +0100 Subject: [PATCH 01/12] backlight: lp855x: Move device_config setting out of lp855x_configure() Move the setting of the lp->cfg pointer to the chip specific lp855x_device_config struct from lp855x_configure() to lp855x_probe(), before calling lp855x_parse_dt(). This is a preperation patch for adding ACPI enumeration support. Signed-off-by: Hans de Goede Reviewed-by: Daniel Thompson Signed-off-by: Lee Jones Link: https://lore.kernel.org/r/20211102225504.18920-1-hdegoede@redhat.com --- drivers/video/backlight/lp855x_bl.c | 32 ++++++++++++++--------------- 1 file changed, 16 insertions(+), 16 deletions(-) diff --git a/drivers/video/backlight/lp855x_bl.c b/drivers/video/backlight/lp855x_bl.c index e94932c69f54..808ff00b2003 100644 --- a/drivers/video/backlight/lp855x_bl.c +++ b/drivers/video/backlight/lp855x_bl.c @@ -170,22 +170,6 @@ static int lp855x_configure(struct lp855x *lp) int i, ret; struct lp855x_platform_data *pd = lp->pdata; - switch (lp->chip_id) { - case LP8550: - case LP8551: - case LP8552: - case LP8553: - case LP8556: - lp->cfg = &lp855x_dev_cfg; - break; - case LP8555: - case LP8557: - lp->cfg = &lp8557_dev_cfg; - break; - default: - return -EINVAL; - } - if (lp->cfg->pre_init_device) { ret = lp->cfg->pre_init_device(lp); if (ret) { @@ -413,6 +397,22 @@ static int lp855x_probe(struct i2c_client *cl, const struct i2c_device_id *id) lp->chip_id = id->driver_data; lp->pdata = dev_get_platdata(&cl->dev); + switch (lp->chip_id) { + case LP8550: + case LP8551: + case LP8552: + case LP8553: + case LP8556: + lp->cfg = &lp855x_dev_cfg; + break; + case LP8555: + case LP8557: + lp->cfg = &lp8557_dev_cfg; + break; + default: + return -EINVAL; + } + if (!lp->pdata) { ret = lp855x_parse_dt(lp); if (ret < 0) From 92add941b6be185e511a7564bf68963fa1633d53 Mon Sep 17 00:00:00 2001 From: Hans de Goede Date: Tue, 2 Nov 2021 23:55:03 +0100 Subject: [PATCH 02/12] backlight: lp855x: Add dev helper variable to lp855x_probe() Add a dev local variable to the lp855x_probe(), to replace "&cl->dev" and "lp->dev" in various places. Also switch to dev_err_probe() in one case which takes care of not printing -EPROBE_DEFER errors for us. This is mostly a preparation for adding ACPI enumeration support which will use the new "dev" variable more. Signed-off-by: Hans de Goede Reviewed-by: Daniel Thompson Signed-off-by: Lee Jones Link: https://lore.kernel.org/r/20211102225504.18920-2-hdegoede@redhat.com --- drivers/video/backlight/lp855x_bl.c | 29 +++++++++++++---------------- 1 file changed, 13 insertions(+), 16 deletions(-) diff --git a/drivers/video/backlight/lp855x_bl.c b/drivers/video/backlight/lp855x_bl.c index 808ff00b2003..d1d27d5eb0f2 100644 --- a/drivers/video/backlight/lp855x_bl.c +++ b/drivers/video/backlight/lp855x_bl.c @@ -381,21 +381,22 @@ static int lp855x_parse_dt(struct lp855x *lp) static int lp855x_probe(struct i2c_client *cl, const struct i2c_device_id *id) { + struct device *dev = &cl->dev; struct lp855x *lp; int ret; if (!i2c_check_functionality(cl->adapter, I2C_FUNC_SMBUS_I2C_BLOCK)) return -EIO; - lp = devm_kzalloc(&cl->dev, sizeof(struct lp855x), GFP_KERNEL); + lp = devm_kzalloc(dev, sizeof(struct lp855x), GFP_KERNEL); if (!lp) return -ENOMEM; lp->client = cl; - lp->dev = &cl->dev; + lp->dev = dev; lp->chipname = id->name; lp->chip_id = id->driver_data; - lp->pdata = dev_get_platdata(&cl->dev); + lp->pdata = dev_get_platdata(dev); switch (lp->chip_id) { case LP8550: @@ -424,30 +425,27 @@ static int lp855x_probe(struct i2c_client *cl, const struct i2c_device_id *id) else lp->mode = REGISTER_BASED; - lp->supply = devm_regulator_get(lp->dev, "power"); + lp->supply = devm_regulator_get(dev, "power"); if (IS_ERR(lp->supply)) { if (PTR_ERR(lp->supply) == -EPROBE_DEFER) return -EPROBE_DEFER; lp->supply = NULL; } - lp->enable = devm_regulator_get_optional(lp->dev, "enable"); + lp->enable = devm_regulator_get_optional(dev, "enable"); if (IS_ERR(lp->enable)) { ret = PTR_ERR(lp->enable); if (ret == -ENODEV) { lp->enable = NULL; } else { - if (ret != -EPROBE_DEFER) - dev_err(lp->dev, "error getting enable regulator: %d\n", - ret); - return ret; + return dev_err_probe(dev, ret, "getting enable regulator\n"); } } if (lp->supply) { ret = regulator_enable(lp->supply); if (ret < 0) { - dev_err(&cl->dev, "failed to enable supply: %d\n", ret); + dev_err(dev, "failed to enable supply: %d\n", ret); return ret; } } @@ -455,7 +453,7 @@ static int lp855x_probe(struct i2c_client *cl, const struct i2c_device_id *id) if (lp->enable) { ret = regulator_enable(lp->enable); if (ret < 0) { - dev_err(lp->dev, "failed to enable vddio: %d\n", ret); + dev_err(dev, "failed to enable vddio: %d\n", ret); goto disable_supply; } @@ -470,20 +468,19 @@ static int lp855x_probe(struct i2c_client *cl, const struct i2c_device_id *id) ret = lp855x_configure(lp); if (ret) { - dev_err(lp->dev, "device config err: %d", ret); + dev_err(dev, "device config err: %d", ret); goto disable_vddio; } ret = lp855x_backlight_register(lp); if (ret) { - dev_err(lp->dev, - "failed to register backlight. err: %d\n", ret); + dev_err(dev, "failed to register backlight. err: %d\n", ret); goto disable_vddio; } - ret = sysfs_create_group(&lp->dev->kobj, &lp855x_attr_group); + ret = sysfs_create_group(&dev->kobj, &lp855x_attr_group); if (ret) { - dev_err(lp->dev, "failed to register sysfs. err: %d\n", ret); + dev_err(dev, "failed to register sysfs. err: %d\n", ret); goto disable_vddio; } From 6202b5de73cfb0d83245b8ea834017183ec67885 Mon Sep 17 00:00:00 2001 From: Hans de Goede Date: Tue, 2 Nov 2021 23:55:04 +0100 Subject: [PATCH 03/12] backlight: lp855x: Add support ACPI enumeration The Xiaomi Mi Pad 2 tablet uses an ACPI enumerated LP8556 backlight controller for its LCD-panel, with a Xiaomi specific ACPI HID of "XMCC0001", add support for this. Note the new "if (id)" check also fixes a NULL pointer deref when a user tries to manually bind the driver from sysfs. When CONFIG_ACPI is disabled acpi_match_device() will always return NULL, so the lp855x_parse_acpi() call will get optimized away. Reviewed-by: Daniel Thompson Signed-off-by: Hans de Goede Signed-off-by: Lee Jones Link: https://lore.kernel.org/r/20211102225504.18920-3-hdegoede@redhat.com --- drivers/video/backlight/lp855x_bl.c | 73 ++++++++++++++++++++++++----- 1 file changed, 61 insertions(+), 12 deletions(-) diff --git a/drivers/video/backlight/lp855x_bl.c b/drivers/video/backlight/lp855x_bl.c index d1d27d5eb0f2..2b9e2bbbb03e 100644 --- a/drivers/video/backlight/lp855x_bl.c +++ b/drivers/video/backlight/lp855x_bl.c @@ -5,6 +5,7 @@ * Copyright (C) 2011 Texas Instruments */ +#include #include #include #include @@ -330,7 +331,7 @@ static int lp855x_parse_dt(struct lp855x *lp) { struct device *dev = lp->dev; struct device_node *node = dev->of_node; - struct lp855x_platform_data *pdata; + struct lp855x_platform_data *pdata = lp->pdata; int rom_length; if (!node) { @@ -338,10 +339,6 @@ static int lp855x_parse_dt(struct lp855x *lp) return -EINVAL; } - pdata = devm_kzalloc(dev, sizeof(*pdata), GFP_KERNEL); - if (!pdata) - return -ENOMEM; - of_property_read_string(node, "bl-name", &pdata->name); of_property_read_u8(node, "dev-ctrl", &pdata->device_control); of_property_read_u8(node, "init-brt", &pdata->initial_brightness); @@ -368,8 +365,6 @@ static int lp855x_parse_dt(struct lp855x *lp) pdata->rom_data = &rom[0]; } - lp->pdata = pdata; - return 0; } #else @@ -379,8 +374,32 @@ static int lp855x_parse_dt(struct lp855x *lp) } #endif +static int lp855x_parse_acpi(struct lp855x *lp) +{ + int ret; + + /* + * On ACPI the device has already been initialized by the firmware + * and is in register mode, so we can read back the settings from + * the registers. + */ + ret = i2c_smbus_read_byte_data(lp->client, lp->cfg->reg_brightness); + if (ret < 0) + return ret; + + lp->pdata->initial_brightness = ret; + + ret = i2c_smbus_read_byte_data(lp->client, lp->cfg->reg_devicectrl); + if (ret < 0) + return ret; + + lp->pdata->device_control = ret; + return 0; +} + static int lp855x_probe(struct i2c_client *cl, const struct i2c_device_id *id) { + const struct acpi_device_id *acpi_id = NULL; struct device *dev = &cl->dev; struct lp855x *lp; int ret; @@ -394,10 +413,20 @@ static int lp855x_probe(struct i2c_client *cl, const struct i2c_device_id *id) lp->client = cl; lp->dev = dev; - lp->chipname = id->name; - lp->chip_id = id->driver_data; lp->pdata = dev_get_platdata(dev); + if (id) { + lp->chipname = id->name; + lp->chip_id = id->driver_data; + } else { + acpi_id = acpi_match_device(dev->driver->acpi_match_table, dev); + if (!acpi_id) + return -ENODEV; + + lp->chipname = acpi_id->id; + lp->chip_id = acpi_id->driver_data; + } + switch (lp->chip_id) { case LP8550: case LP8551: @@ -415,9 +444,19 @@ static int lp855x_probe(struct i2c_client *cl, const struct i2c_device_id *id) } if (!lp->pdata) { - ret = lp855x_parse_dt(lp); - if (ret < 0) - return ret; + lp->pdata = devm_kzalloc(dev, sizeof(*lp->pdata), GFP_KERNEL); + if (!lp->pdata) + return -ENOMEM; + + if (id) { + ret = lp855x_parse_dt(lp); + if (ret < 0) + return ret; + } else { + ret = lp855x_parse_acpi(lp); + if (ret < 0) + return ret; + } } if (lp->pdata->period_ns > 0) @@ -537,10 +576,20 @@ static const struct i2c_device_id lp855x_ids[] = { }; MODULE_DEVICE_TABLE(i2c, lp855x_ids); +#ifdef CONFIG_ACPI +static const struct acpi_device_id lp855x_acpi_match[] = { + /* Xiaomi specific HID used for the LP8556 on the Mi Pad 2 */ + { "XMCC0001", LP8556 }, + { } +}; +MODULE_DEVICE_TABLE(acpi, lp855x_acpi_match); +#endif + static struct i2c_driver lp855x_driver = { .driver = { .name = "lp855x", .of_match_table = of_match_ptr(lp855x_dt_ids), + .acpi_match_table = ACPI_PTR(lp855x_acpi_match), }, .probe = lp855x_probe, .remove = lp855x_remove, From c05b21ebc5bce3ecc78c2c71afd76d92c790a2ac Mon Sep 17 00:00:00 2001 From: Marijn Suijten Date: Mon, 15 Nov 2021 21:34:51 +0100 Subject: [PATCH 04/12] backlight: qcom-wled: Validate enabled string indices in DT The strings passed in DT may possibly cause out-of-bounds register accesses and should be validated before use. Fixes: 775d2ffb4af6 ("backlight: qcom-wled: Restructure the driver for WLED3") Signed-off-by: Marijn Suijten Reviewed-by: AngeloGioacchino Del Regno Reviewed-by: Daniel Thompson Signed-off-by: Lee Jones Link: https://lore.kernel.org/r/20211115203459.1634079-2-marijn.suijten@somainline.org --- drivers/video/backlight/qcom-wled.c | 18 +++++++++++++++++- 1 file changed, 17 insertions(+), 1 deletion(-) diff --git a/drivers/video/backlight/qcom-wled.c b/drivers/video/backlight/qcom-wled.c index d094299c2a48..8a42ed89c59c 100644 --- a/drivers/video/backlight/qcom-wled.c +++ b/drivers/video/backlight/qcom-wled.c @@ -1528,12 +1528,28 @@ static int wled_configure(struct wled *wled) string_len = of_property_count_elems_of_size(dev->of_node, "qcom,enabled-strings", sizeof(u32)); - if (string_len > 0) + if (string_len > 0) { + if (string_len > wled->max_string_count) { + dev_err(dev, "Cannot have more than %d strings\n", + wled->max_string_count); + return -EINVAL; + } + of_property_read_u32_array(dev->of_node, "qcom,enabled-strings", wled->cfg.enabled_strings, sizeof(u32)); + for (i = 0; i < string_len; ++i) { + if (wled->cfg.enabled_strings[i] >= wled->max_string_count) { + dev_err(dev, + "qcom,enabled-strings index %d at %d is out of bounds\n", + wled->cfg.enabled_strings[i], i); + return -EINVAL; + } + } + } + return 0; } From e29e24bdabfeddbf8b1a4ecac1af439a85150438 Mon Sep 17 00:00:00 2001 From: Marijn Suijten Date: Mon, 15 Nov 2021 21:34:52 +0100 Subject: [PATCH 05/12] backlight: qcom-wled: Pass number of elements to read to read_u32_array of_property_read_u32_array takes the number of elements to read as last argument. This does not always need to be 4 (sizeof(u32)) but should instead be the size of the array in DT as read just above with of_property_count_elems_of_size. To not make such an error go unnoticed again the driver now bails accordingly when of_property_read_u32_array returns an error. Surprisingly the indentation of newlined arguments is lining up again after prepending `rc = `. Fixes: 775d2ffb4af6 ("backlight: qcom-wled: Restructure the driver for WLED3") Signed-off-by: Marijn Suijten Reviewed-by: AngeloGioacchino Del Regno Reviewed-by: Daniel Thompson Signed-off-by: Lee Jones Link: https://lore.kernel.org/r/20211115203459.1634079-3-marijn.suijten@somainline.org --- drivers/video/backlight/qcom-wled.c | 9 +++++++-- 1 file changed, 7 insertions(+), 2 deletions(-) diff --git a/drivers/video/backlight/qcom-wled.c b/drivers/video/backlight/qcom-wled.c index 8a42ed89c59c..d413b913fef3 100644 --- a/drivers/video/backlight/qcom-wled.c +++ b/drivers/video/backlight/qcom-wled.c @@ -1535,10 +1535,15 @@ static int wled_configure(struct wled *wled) return -EINVAL; } - of_property_read_u32_array(dev->of_node, + rc = of_property_read_u32_array(dev->of_node, "qcom,enabled-strings", wled->cfg.enabled_strings, - sizeof(u32)); + string_len); + if (rc) { + dev_err(dev, "Failed to read %d elements from qcom,enabled-strings: %d\n", + string_len, rc); + return rc; + } for (i = 0; i < string_len; ++i) { if (wled->cfg.enabled_strings[i] >= wled->max_string_count) { From 0a139358548968b2ff308257b4fbeec7badcc3e1 Mon Sep 17 00:00:00 2001 From: Marijn Suijten Date: Mon, 15 Nov 2021 21:34:53 +0100 Subject: [PATCH 06/12] backlight: qcom-wled: Use cpu_to_le16 macro to perform conversion The kernel already provides appropriate primitives to perform endianness conversion which should be used in favour of manual bit-wrangling. Signed-off-by: Marijn Suijten Reviewed-by: AngeloGioacchino Del Regno Reviewed-by: Daniel Thompson Signed-off-by: Lee Jones Link: https://lore.kernel.org/r/20211115203459.1634079-4-marijn.suijten@somainline.org --- drivers/video/backlight/qcom-wled.c | 23 +++++++++++------------ 1 file changed, 11 insertions(+), 12 deletions(-) diff --git a/drivers/video/backlight/qcom-wled.c b/drivers/video/backlight/qcom-wled.c index d413b913fef3..9d883e702134 100644 --- a/drivers/video/backlight/qcom-wled.c +++ b/drivers/video/backlight/qcom-wled.c @@ -231,14 +231,14 @@ struct wled { static int wled3_set_brightness(struct wled *wled, u16 brightness) { int rc, i; - u8 v[2]; + __le16 v; - v[0] = brightness & 0xff; - v[1] = (brightness >> 8) & 0xf; + v = cpu_to_le16(brightness & WLED3_SINK_REG_BRIGHT_MAX); for (i = 0; i < wled->cfg.num_strings; ++i) { rc = regmap_bulk_write(wled->regmap, wled->ctrl_addr + - WLED3_SINK_REG_BRIGHT(i), v, 2); + WLED3_SINK_REG_BRIGHT(i), + &v, sizeof(v)); if (rc < 0) return rc; } @@ -250,18 +250,18 @@ static int wled4_set_brightness(struct wled *wled, u16 brightness) { int rc, i; u16 low_limit = wled->max_brightness * 4 / 1000; - u8 v[2]; + __le16 v; /* WLED4's lower limit of operation is 0.4% */ if (brightness > 0 && brightness < low_limit) brightness = low_limit; - v[0] = brightness & 0xff; - v[1] = (brightness >> 8) & 0xf; + v = cpu_to_le16(brightness & WLED3_SINK_REG_BRIGHT_MAX); for (i = 0; i < wled->cfg.num_strings; ++i) { rc = regmap_bulk_write(wled->regmap, wled->sink_addr + - WLED4_SINK_REG_BRIGHT(i), v, 2); + WLED4_SINK_REG_BRIGHT(i), + &v, sizeof(v)); if (rc < 0) return rc; } @@ -273,21 +273,20 @@ static int wled5_set_brightness(struct wled *wled, u16 brightness) { int rc, offset; u16 low_limit = wled->max_brightness * 1 / 1000; - u8 v[2]; + __le16 v; /* WLED5's lower limit is 0.1% */ if (brightness < low_limit) brightness = low_limit; - v[0] = brightness & 0xff; - v[1] = (brightness >> 8) & 0x7f; + v = cpu_to_le16(brightness & WLED5_SINK_REG_BRIGHT_MAX_15B); offset = (wled->cfg.mod_sel == MOD_A) ? WLED5_SINK_REG_MOD_A_BRIGHTNESS_LSB : WLED5_SINK_REG_MOD_B_BRIGHTNESS_LSB; rc = regmap_bulk_write(wled->regmap, wled->sink_addr + offset, - v, 2); + &v, sizeof(v)); return rc; } From 5ada78b26f935f8751852dffa24f6b545b1d2517 Mon Sep 17 00:00:00 2001 From: Marijn Suijten Date: Mon, 15 Nov 2021 21:34:54 +0100 Subject: [PATCH 07/12] backlight: qcom-wled: Fix off-by-one maximum with default num_strings When not specifying num-strings in the DT the default is used, but +1 is added to it which turns WLED3 into 4 and WLED4/5 into 5 strings instead of 3 and 4 respectively, causing out-of-bounds reads and register read/writes. This +1 exists for a deficiency in the DT parsing code, and is simply omitted entirely - solving this oob issue - by parsing the property separately much like qcom,enabled-strings. This also enables more stringent checks on the maximum value when qcom,enabled-strings is provided in the DT, by parsing num-strings after enabled-strings to allow it to check against (and in a subsequent patch override) the length of enabled-strings: it is invalid to set num-strings higher than that. The DT currently utilizes it to get around an incorrect fixed read of four elements from that array (has been addressed in a prior patch) by setting a lower num-strings where desired. Fixes: 93c64f1ea1e8 ("leds: add Qualcomm PM8941 WLED driver") Signed-off-by: Marijn Suijten Reviewed-By: AngeloGioacchino Del Regno Reviewed-by: Daniel Thompson Signed-off-by: Lee Jones Link: https://lore.kernel.org/r/20211115203459.1634079-5-marijn.suijten@somainline.org --- drivers/video/backlight/qcom-wled.c | 48 ++++++++++------------------- 1 file changed, 16 insertions(+), 32 deletions(-) diff --git a/drivers/video/backlight/qcom-wled.c b/drivers/video/backlight/qcom-wled.c index 9d883e702134..ab10910971e9 100644 --- a/drivers/video/backlight/qcom-wled.c +++ b/drivers/video/backlight/qcom-wled.c @@ -1255,21 +1255,6 @@ static const struct wled_var_cfg wled5_ovp_cfg = { .size = 16, }; -static u32 wled3_num_strings_values_fn(u32 idx) -{ - return idx + 1; -} - -static const struct wled_var_cfg wled3_num_strings_cfg = { - .fn = wled3_num_strings_values_fn, - .size = 3, -}; - -static const struct wled_var_cfg wled4_num_strings_cfg = { - .fn = wled3_num_strings_values_fn, - .size = 4, -}; - static u32 wled3_switch_freq_values_fn(u32 idx) { return 19200 / (2 * (1 + idx)); @@ -1343,11 +1328,6 @@ static int wled_configure(struct wled *wled) .val_ptr = &cfg->switch_freq, .cfg = &wled3_switch_freq_cfg, }, - { - .name = "qcom,num-strings", - .val_ptr = &cfg->num_strings, - .cfg = &wled3_num_strings_cfg, - }, }; const struct wled_u32_opts wled4_opts[] = { @@ -1371,11 +1351,6 @@ static int wled_configure(struct wled *wled) .val_ptr = &cfg->switch_freq, .cfg = &wled3_switch_freq_cfg, }, - { - .name = "qcom,num-strings", - .val_ptr = &cfg->num_strings, - .cfg = &wled4_num_strings_cfg, - }, }; const struct wled_u32_opts wled5_opts[] = { @@ -1399,11 +1374,6 @@ static int wled_configure(struct wled *wled) .val_ptr = &cfg->switch_freq, .cfg = &wled3_switch_freq_cfg, }, - { - .name = "qcom,num-strings", - .val_ptr = &cfg->num_strings, - .cfg = &wled4_num_strings_cfg, - }, { .name = "qcom,modulator-sel", .val_ptr = &cfg->mod_sel, @@ -1522,8 +1492,6 @@ static int wled_configure(struct wled *wled) *bool_opts[i].val_ptr = true; } - cfg->num_strings = cfg->num_strings + 1; - string_len = of_property_count_elems_of_size(dev->of_node, "qcom,enabled-strings", sizeof(u32)); @@ -1554,6 +1522,22 @@ static int wled_configure(struct wled *wled) } } + rc = of_property_read_u32(dev->of_node, "qcom,num-strings", &val); + if (!rc) { + if (val < 1 || val > wled->max_string_count) { + dev_err(dev, "qcom,num-strings must be between 1 and %d\n", + wled->max_string_count); + return -EINVAL; + } + + if (string_len > 0 && val > string_len) { + dev_err(dev, "qcom,num-strings exceeds qcom,enabled-strings\n"); + return -EINVAL; + } + + cfg->num_strings = val; + } + return 0; } From 2b4b49602f9feca7b7a84eaa33ad9e666c8aa695 Mon Sep 17 00:00:00 2001 From: Marijn Suijten Date: Mon, 15 Nov 2021 21:34:55 +0100 Subject: [PATCH 08/12] backlight: qcom-wled: Override default length with qcom,enabled-strings The length of qcom,enabled-strings as property array is enough to determine the number of strings to be enabled, without needing to set qcom,num-strings to override the default number of strings when less than the default (which is also the maximum) is provided in DT. This also introduces an extra warning when qcom,num-strings is set, denoting that it is not necessary to set both anymore. It is usually more concise to set just qcom,num-length when a zero-based, contiguous range of strings is needed (the majority of the cases), or to only set qcom,enabled-strings when a specific set of indices is desired. Fixes: 775d2ffb4af6 ("backlight: qcom-wled: Restructure the driver for WLED3") Signed-off-by: Marijn Suijten Reviewed-by: AngeloGioacchino Del Regno Reviewed-by: Daniel Thompson Signed-off-by: Lee Jones Link: https://lore.kernel.org/r/20211115203459.1634079-6-marijn.suijten@somainline.org --- drivers/video/backlight/qcom-wled.c | 12 +++++++++--- 1 file changed, 9 insertions(+), 3 deletions(-) diff --git a/drivers/video/backlight/qcom-wled.c b/drivers/video/backlight/qcom-wled.c index ab10910971e9..5306b06044b4 100644 --- a/drivers/video/backlight/qcom-wled.c +++ b/drivers/video/backlight/qcom-wled.c @@ -1520,6 +1520,8 @@ static int wled_configure(struct wled *wled) return -EINVAL; } } + + cfg->num_strings = string_len; } rc = of_property_read_u32(dev->of_node, "qcom,num-strings", &val); @@ -1530,9 +1532,13 @@ static int wled_configure(struct wled *wled) return -EINVAL; } - if (string_len > 0 && val > string_len) { - dev_err(dev, "qcom,num-strings exceeds qcom,enabled-strings\n"); - return -EINVAL; + if (string_len > 0) { + dev_warn(dev, "Only one of qcom,num-strings or qcom,enabled-strings" + " should be set\n"); + if (val > string_len) { + dev_err(dev, "qcom,num-strings exceeds qcom,enabled-strings\n"); + return -EINVAL; + } } cfg->num_strings = val; From 96571489a06999bf5c62e2058622990734556f8f Mon Sep 17 00:00:00 2001 From: Marijn Suijten Date: Mon, 15 Nov 2021 21:34:56 +0100 Subject: [PATCH 09/12] backlight: qcom-wled: Remove unnecessary 4th default string in WLED3 The previous commit improves num_strings parsing to not go over the maximum of 3 strings for WLED3 anymore. Likewise this default index for a hypothetical 4th string is invalid and could access registers that are not mapped to the desired purpose. Removing this value gets rid of undesired confusion and avoids the possibility of accessing registers at this offset even if the 4th array element is used by accident. Signed-off-by: Marijn Suijten Reviewed-by: AngeloGioacchino Del Regno Reviewed-by: Daniel Thompson Signed-off-by: Lee Jones Link: https://lore.kernel.org/r/20211115203459.1634079-7-marijn.suijten@somainline.org --- drivers/video/backlight/qcom-wled.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/drivers/video/backlight/qcom-wled.c b/drivers/video/backlight/qcom-wled.c index 5306b06044b4..5c5df5a3deab 100644 --- a/drivers/video/backlight/qcom-wled.c +++ b/drivers/video/backlight/qcom-wled.c @@ -948,7 +948,7 @@ static const struct wled_config wled3_config_defaults = { .cs_out_en = false, .ext_gen = false, .cabc = false, - .enabled_strings = {0, 1, 2, 3}, + .enabled_strings = {0, 1, 2}, }; static int wled4_setup(struct wled *wled) From c70aefdedb24ec545d0958f17faae3b3a3141d2e Mon Sep 17 00:00:00 2001 From: Marijn Suijten Date: Mon, 15 Nov 2021 21:34:57 +0100 Subject: [PATCH 10/12] backlight: qcom-wled: Provide enabled_strings default for WLED 4 and 5 Only WLED 3 sets a sensible default that allows operating this driver with just qcom,num-strings in the DT; WLED 4 and 5 require qcom,enabled-strings to be provided otherwise enabled_strings remains zero-initialized, resulting in every string-specific register write (currently only the setup and config functions, brightness follows in a future patch) to only configure the zero'th string multiple times. Signed-off-by: Marijn Suijten Reviewed-by: AngeloGioacchino Del Regno Reviewed-by: Daniel Thompson Signed-off-by: Lee Jones Link: https://lore.kernel.org/r/20211115203459.1634079-8-marijn.suijten@somainline.org --- drivers/video/backlight/qcom-wled.c | 2 ++ 1 file changed, 2 insertions(+) diff --git a/drivers/video/backlight/qcom-wled.c b/drivers/video/backlight/qcom-wled.c index 5c5df5a3deab..f975c1f6398b 100644 --- a/drivers/video/backlight/qcom-wled.c +++ b/drivers/video/backlight/qcom-wled.c @@ -1079,6 +1079,7 @@ static const struct wled_config wled4_config_defaults = { .cabc = false, .external_pfet = false, .auto_detection_enabled = false, + .enabled_strings = {0, 1, 2, 3}, }; static int wled5_setup(struct wled *wled) @@ -1192,6 +1193,7 @@ static const struct wled_config wled5_config_defaults = { .cabc = false, .external_pfet = false, .auto_detection_enabled = false, + .enabled_strings = {0, 1, 2, 3}, }; static const u32 wled3_boost_i_limit_values[] = { From b7002cd5e9d80b790349b0a77a2eba34dc0471c0 Mon Sep 17 00:00:00 2001 From: Marijn Suijten Date: Mon, 15 Nov 2021 21:34:58 +0100 Subject: [PATCH 11/12] backlight: qcom-wled: Remove unnecessary double whitespace Remove redundant spaces inside for loop conditions. No other double spaces were found that are not part of indentation with `[^\s] `. Signed-off-by: Marijn Suijten Reviewed-by: AngeloGioacchino Del Regno Reviewed-by: Daniel Thompson Signed-off-by: Lee Jones Link: https://lore.kernel.org/r/20211115203459.1634079-9-marijn.suijten@somainline.org --- drivers/video/backlight/qcom-wled.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/drivers/video/backlight/qcom-wled.c b/drivers/video/backlight/qcom-wled.c index f975c1f6398b..e2a78f4a9668 100644 --- a/drivers/video/backlight/qcom-wled.c +++ b/drivers/video/backlight/qcom-wled.c @@ -235,7 +235,7 @@ static int wled3_set_brightness(struct wled *wled, u16 brightness) v = cpu_to_le16(brightness & WLED3_SINK_REG_BRIGHT_MAX); - for (i = 0; i < wled->cfg.num_strings; ++i) { + for (i = 0; i < wled->cfg.num_strings; ++i) { rc = regmap_bulk_write(wled->regmap, wled->ctrl_addr + WLED3_SINK_REG_BRIGHT(i), &v, sizeof(v)); @@ -258,7 +258,7 @@ static int wled4_set_brightness(struct wled *wled, u16 brightness) v = cpu_to_le16(brightness & WLED3_SINK_REG_BRIGHT_MAX); - for (i = 0; i < wled->cfg.num_strings; ++i) { + for (i = 0; i < wled->cfg.num_strings; ++i) { rc = regmap_bulk_write(wled->regmap, wled->sink_addr + WLED4_SINK_REG_BRIGHT(i), &v, sizeof(v)); From ec961cf3241153e0f27d850f1bf0f172e7d27a21 Mon Sep 17 00:00:00 2001 From: Marijn Suijten Date: Mon, 15 Nov 2021 21:34:59 +0100 Subject: [PATCH 12/12] backlight: qcom-wled: Respect enabled-strings in set_brightness The hardware is capable of controlling any non-contiguous sequence of LEDs specified in the DT using qcom,enabled-strings as u32 array, and this also follows from the DT-bindings documentation. The numbers specified in this array represent indices of the LED strings that are to be enabled and disabled. Its value is appropriately used to setup and enable string modules, but completely disregarded in the set_brightness paths which only iterate over the number of strings linearly. Take an example where only string 2 is enabled with qcom,enabled_strings=<2>: this string is appropriately enabled but subsequent brightness changes would have only touched the zero'th brightness register because num_strings is 1 here. This is simply addressed by looking up the string for this index in the enabled_strings array just like the other codepaths that iterate over num_strings. Likewise enabled_strings is now also used in the autodetection path for consistent behaviour: when a list of strings is specified in DT only those strings will be probed for autodetection, analogous to how the number of strings that need to be probed is already bound by qcom,num-strings. After all autodetection uses the set_brightness helpers to set an initial value, which could otherwise end up changing brightness on a different set of strings. Fixes: 775d2ffb4af6 ("backlight: qcom-wled: Restructure the driver for WLED3") Fixes: 03b2b5e86986 ("backlight: qcom-wled: Add support for WLED4 peripheral") Signed-off-by: Marijn Suijten Reviewed-by: AngeloGioacchino Del Regno Reviewed-by: Daniel Thompson Signed-off-by: Lee Jones Link: https://lore.kernel.org/r/20211115203459.1634079-10-marijn.suijten@somainline.org --- drivers/video/backlight/qcom-wled.c | 22 ++++++++++++---------- 1 file changed, 12 insertions(+), 10 deletions(-) diff --git a/drivers/video/backlight/qcom-wled.c b/drivers/video/backlight/qcom-wled.c index e2a78f4a9668..306bcc6ccb92 100644 --- a/drivers/video/backlight/qcom-wled.c +++ b/drivers/video/backlight/qcom-wled.c @@ -237,7 +237,7 @@ static int wled3_set_brightness(struct wled *wled, u16 brightness) for (i = 0; i < wled->cfg.num_strings; ++i) { rc = regmap_bulk_write(wled->regmap, wled->ctrl_addr + - WLED3_SINK_REG_BRIGHT(i), + WLED3_SINK_REG_BRIGHT(wled->cfg.enabled_strings[i]), &v, sizeof(v)); if (rc < 0) return rc; @@ -260,7 +260,7 @@ static int wled4_set_brightness(struct wled *wled, u16 brightness) for (i = 0; i < wled->cfg.num_strings; ++i) { rc = regmap_bulk_write(wled->regmap, wled->sink_addr + - WLED4_SINK_REG_BRIGHT(i), + WLED4_SINK_REG_BRIGHT(wled->cfg.enabled_strings[i]), &v, sizeof(v)); if (rc < 0) return rc; @@ -571,7 +571,7 @@ unlock_mutex: static void wled_auto_string_detection(struct wled *wled) { - int rc = 0, i, delay_time_us; + int rc = 0, i, j, delay_time_us; u32 sink_config = 0; u8 sink_test = 0, sink_valid = 0, val; bool fault_set; @@ -618,14 +618,15 @@ static void wled_auto_string_detection(struct wled *wled) /* Iterate through the strings one by one */ for (i = 0; i < wled->cfg.num_strings; i++) { - sink_test = BIT((WLED4_SINK_REG_CURR_SINK_SHFT + i)); + j = wled->cfg.enabled_strings[i]; + sink_test = BIT((WLED4_SINK_REG_CURR_SINK_SHFT + j)); /* Enable feedback control */ rc = regmap_write(wled->regmap, wled->ctrl_addr + - WLED3_CTRL_REG_FEEDBACK_CONTROL, i + 1); + WLED3_CTRL_REG_FEEDBACK_CONTROL, j + 1); if (rc < 0) { dev_err(wled->dev, "Failed to enable feedback for SINK %d rc = %d\n", - i + 1, rc); + j + 1, rc); goto failed_detect; } @@ -634,7 +635,7 @@ static void wled_auto_string_detection(struct wled *wled) WLED4_SINK_REG_CURR_SINK, sink_test); if (rc < 0) { dev_err(wled->dev, "Failed to configure SINK %d rc=%d\n", - i + 1, rc); + j + 1, rc); goto failed_detect; } @@ -661,7 +662,7 @@ static void wled_auto_string_detection(struct wled *wled) if (fault_set) dev_dbg(wled->dev, "WLED OVP fault detected with SINK %d\n", - i + 1); + j + 1); else sink_valid |= sink_test; @@ -701,15 +702,16 @@ static void wled_auto_string_detection(struct wled *wled) /* Enable valid sinks */ if (wled->version == 4) { for (i = 0; i < wled->cfg.num_strings; i++) { + j = wled->cfg.enabled_strings[i]; if (sink_config & - BIT(WLED4_SINK_REG_CURR_SINK_SHFT + i)) + BIT(WLED4_SINK_REG_CURR_SINK_SHFT + j)) val = WLED4_SINK_REG_STR_MOD_MASK; else /* Disable modulator_en for unused sink */ val = 0; rc = regmap_write(wled->regmap, wled->sink_addr + - WLED4_SINK_REG_STR_MOD_EN(i), val); + WLED4_SINK_REG_STR_MOD_EN(j), val); if (rc < 0) { dev_err(wled->dev, "Failed to configure MODULATOR_EN rc=%d\n", rc);