Re: [PATCH v6 2/4] leds: Add driver for Qualcomm LPG

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

Hi Bjorn,

On 10/21/20 10:12 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>
> Tested-by: Luca Weiss <luca@z3ntu.xyz>


Thanks for these patches.  I have tested them successfully on the Sony 
Xperia XA2 (Discovery, Nile platform) with the leds on the PM660l - feel 
free to add my Tested-by.  Should I send the configuration your way for 
inclusion in this patch, or submit them separately (either applied 
after, or included as separate patches in the next version of this series)?

> +/**
> + * struct lpg_data - initialization data
> + * @lut_base:		base address of LUT block
> + * @lut_size:		number of entries in LUT
> + * @triled_base:	base address of TRILED
> + * @pwm_9bit_mask:	bitmask for switching from 6bit to 9bit pwm


Our downstream kernel derives this from the "LPG subtype" for each 
distinct channel, read from register offset +0x5.  A value of 0xb is 
subtype "PWM" with a shift of 2, a value of 0x11 is subtype "LPG_LITE" 
with a shift of 4.  Can we do the same here instead of hardcoding it for 
all channels in the LPG at once?  How should we determine if the mask is 
one or two bits wide, for the 3<<4 case?

> + * @num_channels:	number of channels in LPG
> + * @channels:		list of channel initialization data
> + */

> +	if (ping_pong) {
> +		if (len % 2)
> +			hi_pause = 0;
> +		else
> +			hi_pause = pattern[len + 1 / 2].delta_t;


len + 1 should be wrapped in parentheses just like the reassignment to 
len= below, otherwise this is always an out of bounds read (at len + 0).

> +		lo_pause = pattern[len - 1].delta_t;
> +
> +		len = (len + 1) / 2;
> +	} else {
> +		hi_pause = pattern[len - 1].delta_t;
> +		lo_pause = 0;
> +	}
> +
> +	ret = lpg_lut_store(lpg, pattern, len, &lo_idx, &hi_idx);
> +	if (ret < 0)
> +		goto out;
> +
> +	for (i = 0; i < led->num_channels; i++) {
> +		chan = led->channels[i];
> +
> +		chan->ramp_duration_ms = pattern[0].delta_t * len;


Perhaps this could store the duration of a single step instead, since 
the only use in lpg_apply_lut_control divides it by pattern length again?

> +		chan->ramp_ping_pong = ping_pong;
> +		chan->ramp_oneshot = repeat != -1;
> +
> +		chan->ramp_lo_pause_ms = lo_pause;
> +		chan->ramp_hi_pause_ms = hi_pause;
> +
> +		chan->pattern_lo_idx = lo_idx;
> +		chan->pattern_hi_idx = hi_idx;
> +	}
> +
> +out:
> +	return ret;
> +}

> +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_LONGS(lpg->lut_size) * sizeof(unsigned long);
> +	lpg->lut_bitmap = devm_kzalloc(lpg->dev, bitmap_size, GFP_KERNEL);


Would it be nicer to use BITS_TO_BYTES here, or otherwise 
devm_kcalloc(..., bitmap_size, sizeof(long), ...) without mutiplying 
with sizeof(unsigned long)?

> +
> +	bitmap_clear(lpg->lut_bitmap, 0, lpg->lut_size);
> +	return lpg->lut_bitmap ? 0 : -ENOMEM;
> +}
> +
> +static int lpg_probe(struct platform_device *pdev)
> +{
> +	struct device_node *np;
> +	struct lpg *lpg;
> +	int ret;
> +	int i;
> +
> +	lpg = devm_kzalloc(&pdev->dev, sizeof(*lpg), GFP_KERNEL);
> +	if (!lpg)
> +		return -ENOMEM;
> +
> +	lpg->data = of_device_get_match_data(&pdev->dev);
> +	if (!lpg->data)
> +		return -EINVAL;
> +
> +	lpg->dev = &pdev->dev;
> +
> +	lpg->map = dev_get_regmap(pdev->dev.parent, NULL);
> +	if (!lpg->map) {
> +		dev_err(&pdev->dev, "parent regmap unavailable\n");
> +		return -ENXIO;
> +	}
> +
> +	ret = lpg_init_channels(lpg);
> +	if (ret < 0)
> +		return ret;
> +
> +	ret = lpg_init_triled(lpg);
> +	if (ret < 0)
> +		return ret;
> +
> +	ret = lpg_init_lut(lpg);
> +	if (ret < 0)
> +		return ret;


How about turning these returns into dev_err_probe?  I'm not sure if 
that's the expected way to go nowadays, but having some form of logging 
when a driver fails to probe is always good to have.

Thanks!
Marijn

> +
> +	for_each_available_child_of_node(pdev->dev.of_node, np) {
> +		ret = lpg_add_led(lpg, np);
> +		if (ret)
> +			return ret;
> +	}
> +
> +	for (i = 0; i < lpg->num_channels; i++)
> +		lpg_apply_dtest(&lpg->channels[i]);
> +
> +	ret = lpg_add_pwm(lpg);
> +	if (ret)
> +		return ret;
> +
> +	platform_set_drvdata(pdev, lpg);
> +
> +	return 0;
> +}