Re: [PATCH v7 2/6] leds: Add driver for Qualcomm LPG

[Marijn Suijten <marijn.suijten@somainline.org>]

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