| Message ID | 20211004192741.621870-10-marijn.suijten@somainline.org (mailing list archive) |
|---|---|
| State | Superseded, archived |
| Headers | show |
| Series | backlight: qcom-wled: fix and solidify handling of enabled-strings | expand |
On Mon, Oct 04, 2021 at 09:27:40PM +0200, Marijn Suijten wrote: > 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. This isn't true until patch 10 is applied! Given both patches fix the same issue in different functions I'd prefer these to be squashed together (and doubly so because the autodetect code uses set_brightness() as a helper function). Daniel.
On 2021-10-05 10:33:31, Daniel Thompson wrote: > On Mon, Oct 04, 2021 at 09:27:40PM +0200, Marijn Suijten wrote: > > 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. > > This isn't true until patch 10 is applied! Patch 9 and 10 were split up at a last resort to prevent a clash in the title, apologies for that. > Given both patches fix the same issue in different functions I'd prefer > these to be squashed together (and doubly so because the autodetect code > uses set_brightness() as a helper function). That's a fair reason, and solution I agree on. I'll figure out how to generify the title and re-spin this patchset except if there are other reviewers/maintainers I should wait for. - Marijn > Daniel.
diff --git a/drivers/video/backlight/qcom-wled.c b/drivers/video/backlight/qcom-wled.c index dbe503007ada..c0b8d10c20a2 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; @@ -259,7 +259,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;
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. 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 <marijn.suijten@somainline.org> Reviewed-by: AngeloGioacchino Del Regno <angelogioacchino.delregno@somainline.org> --- drivers/video/backlight/qcom-wled.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)