lkml.org 
[lkml]   [2021]   [May]   [1]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
From
SubjectRe: [PATCH v7 2/6] leds: Add driver for Qualcomm LPG
Date
Hi Bjorn,

On 4/29/21 11:15 PM, Bjorn Andersson wrote:
> The Light Pulse Generator (LPG) is a PWM-block found in a wide range of
> PMICs from Qualcomm. It can operate on fixed parameters or based on a
> lookup-table, altering the duty cycle over time - which provides the
> means for e.g. hardware assisted transitions of LED brightness.
>
> Signed-off-by: Bjorn Andersson <bjorn.andersson@linaro.org>
> ---
>
> Changes since v6:
> - Moved code into drivers/leds/rgb/
> - Reverted to earlier qcom,dtest handling to support routing pwm signals
> through dtest lines.
> - Remember the duration of each step of the pattern, rather than adding up and
> then dividing when the value is used.
> - Added missing error prints on DT parse errors.
> - Added sm8150[lb] and made led source and atc presence optional
> - Added missing parenthesis around (len + 1) / 2 in search for hi_pause in the
> pattern.
>
> drivers/leds/Kconfig | 3 +
> drivers/leds/Makefile | 3 +
> drivers/leds/rgb/leds-qcom-lpg.c | 1286 ++++++++++++++++++++++++++++++
> 3 files changed, 1292 insertions(+)
> create mode 100644 drivers/leds/rgb/leds-qcom-lpg.c
>
> diff --git a/drivers/leds/Kconfig b/drivers/leds/Kconfig
> index 49d99cb084db..8ab06b3f162d 100644
> --- a/drivers/leds/Kconfig
> +++ b/drivers/leds/Kconfig
> @@ -933,6 +933,9 @@ source "drivers/leds/blink/Kconfig"
> comment "Flash and Torch LED drivers"
> source "drivers/leds/flash/Kconfig"
>
> +comment "RGB LED drivers"
> +source "drivers/leds/rgb/Kconfig"


It looks like this file is not included in any of the patches.

> +
> comment "LED Triggers"
> source "drivers/leds/trigger/Kconfig"
>
> diff --git a/drivers/leds/Makefile b/drivers/leds/Makefile
> index 7e604d3028c8..8cad0465aae0 100644
> --- a/drivers/leds/Makefile
> +++ b/drivers/leds/Makefile
> @@ -106,6 +106,9 @@ obj-$(CONFIG_LEDS_USER) += uleds.o
> # Flash and Torch LED Drivers
> obj-$(CONFIG_LEDS_CLASS_FLASH) += flash/
>
> +# RGB LED Drivers
> +obj-$(CONFIG_LEDS_CLASS_MULTICOLOR) += rgb/


This file appears to be missing from this patch(set), too.

> +static int lpg_lut_store(struct lpg *lpg, struct led_pattern *pattern,
> + size_t len, unsigned int *lo_idx, unsigned int *hi_idx)
> +{
> + unsigned int idx;
> + u16 val;
> + int i;
> +
> + /* Hardware does not behave when LO_IDX == HI_IDX */
> + if (len == 1)
> + return -EINVAL;
> +
> + idx = bitmap_find_next_zero_area(lpg->lut_bitmap, lpg->lut_size,
> + 0, len, 0);
> + if (idx >= lpg->lut_size)
> + return -ENOMEM;
> +
> + for (i = 0; i < len; i++) {
> + val = pattern[i].brightness;
> +
> + regmap_bulk_write(lpg->map, lpg->lut_base + LPG_LUT_REG(idx + i), &val, 1);


This and the other regmap_bulk_write in lpg_apply_pwm_value used
sizeof(val) before. As far as I'm aware qcom-spmi-pmic specifies 16-bit
addresses (.reg_bits) but 8-bit register sizes (.val_bits). Writing one
register means only 8 out of 16 bits in u16 val are written?

> +static void lpg_apply_lut_control(struct lpg_channel *chan)
> +{
> + struct lpg *lpg = chan->lpg;
> + unsigned int hi_pause;
> + unsigned int lo_pause;
> + unsigned int step;
> + unsigned int conf = 0;
> + unsigned int lo_idx = chan->pattern_lo_idx;
> + unsigned int hi_idx = chan->pattern_hi_idx;
> + int pattern_len;
> +
> + if (!chan->ramp_enabled || chan->pattern_lo_idx == chan->pattern_hi_idx)
> + return;
> +
> + pattern_len = hi_idx - lo_idx + 1 > +
> + step = chan->ramp_tick_ms;


Since this is not dividing a full pattern duration by pattern_len
anymore, that variable is now never read and best removed.

> +static int lpg_parse_channel(struct lpg *lpg, struct device_node *np,
> + struct lpg_channel **channel)
> +{
> + struct lpg_channel *chan;
> + u32 color = LED_COLOR_ID_GREEN;
> + u32 reg;
> + int ret;
> +
> + ret = of_property_read_u32(np, "reg", &reg);
> + if (ret || !reg || reg > lpg->num_channels) {
> + dev_err(lpg->dev, "invalid reg of %pOFn\n", np);


Like \"color\" below, escape reg with \"reg\"?

> +static int lpg_add_led(struct lpg *lpg, struct device_node *np)
> +{
> + struct led_classdev *cdev;
> + struct device_node *child;
> + struct mc_subled *info;
> + struct lpg_led *led;
> + const char *state;
> + int num_channels;
> + u32 color = 0;
> + int ret;
> + int i;
> +
> + ret = of_property_read_u32(np, "color", &color);
> + if (ret < 0 && ret != -EINVAL) {
> + dev_err(lpg->dev, "failed to parse \"color\" of %pOF\n", np);
> + return ret;
> + }
> +
> + if (color == LED_COLOR_ID_MULTI)


Since this driver now lives under rgb/, and is specifically for RGB leds
(afaik), should this and the rest of the code use LED_COLOR_ID_RGB
instead? There was a patch floating around on (if I remember correctly)
##linux-msm by Luca Weiss that performs the conversion, with some
related changes.

> +static int lpg_init_lut(struct lpg *lpg)
> +{
> + const struct lpg_data *data = lpg->data;
> + size_t bitmap_size;
> +
> + if (!data->lut_base)
> + return 0;
> +
> + lpg->lut_base = data->lut_base;
> + lpg->lut_size = data->lut_size;
> +
> + bitmap_size = BITS_TO_BYTES(lpg->lut_size);
> + lpg->lut_bitmap = devm_kzalloc(lpg->dev, bitmap_size, GFP_KERNEL);
> + if (!lpg->lut_bitmap)
> + return -ENOMEM;
> +
> + bitmap_clear(lpg->lut_bitmap, 0, lpg->lut_size);


devm_kzalloc already zeroes the bitmap. Is it necessary to clear it
again (assuming a "cleared" bitmap is implementation-dependent and does
not imply zeroed memory) or could the memory be allocated with
devm_kalloc instead?

Thanks!
Marijn

\
 
 \ /
  Last update: 2021-05-01 23:10    [W:0.091 / U:0.116 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site