diff mbox series

[v11,2/2] leds: Add driver for Qualcomm LPG

Message ID 20220129005429.754727-2-bjorn.andersson@linaro.org
State Changes Requested
Headers show
Series [v11,1/2] dt-bindings: leds: Add Qualcomm Light Pulse Generator binding | expand

Commit Message

Bjorn Andersson Jan. 29, 2022, 12:54 a.m. UTC
The Light Pulse Generator (LPG) is a PWM-block found in a wide range of
PMICs from Qualcomm. These PMICs typically comes with 1-8 LPG instances,
with their output being routed to various other components, such as
current sinks or GPIOs.

Each LPG instance can operate on fixed parameters or based on a shared
lookup-table, altering the duty cycle over time. This provides the means
for hardware assisted transitions of LED brightness.

A typical use case for the fixed parameter mode is to drive a PWM
backlight control signal, the driver therefor allows each LPG instance
to be exposed to the kernel either through the LED framework or the PWM
framework.

A typical use case for the LED configuration is to drive RGB LEDs in
smartphones etc, for which the driver support multiple channels to be
ganged up to a MULTICOLOR LED. In this configuration the pattern
generators will be synchronized, to allow for multi-color patterns.

Signed-off-by: Bjorn Andersson <bjorn.andersson@linaro.org>
---

Changes since v10:
- Check for and reject pattern.delta_t greater than 9 bits
- Write all 9 bits of LPG_RAMP_DURATION_REG

 drivers/leds/Kconfig             |    3 +
 drivers/leds/Makefile            |    3 +
 drivers/leds/rgb/Kconfig         |   13 +
 drivers/leds/rgb/Makefile        |    3 +
 drivers/leds/rgb/leds-qcom-lpg.c | 1306 ++++++++++++++++++++++++++++++
 5 files changed, 1328 insertions(+)
 create mode 100644 drivers/leds/rgb/Kconfig
 create mode 100644 drivers/leds/rgb/Makefile
 create mode 100644 drivers/leds/rgb/leds-qcom-lpg.c

Comments

Marijn Suijten Feb. 2, 2022, 11:18 a.m. UTC | #1
On 2022-01-28 16:54:29, Bjorn Andersson wrote:
> The Light Pulse Generator (LPG) is a PWM-block found in a wide range of
> PMICs from Qualcomm. These PMICs typically comes with 1-8 LPG instances,
> with their output being routed to various other components, such as
> current sinks or GPIOs.
> 
> Each LPG instance can operate on fixed parameters or based on a shared
> lookup-table, altering the duty cycle over time. This provides the means
> for hardware assisted transitions of LED brightness.
> 
> A typical use case for the fixed parameter mode is to drive a PWM
> backlight control signal, the driver therefor allows each LPG instance
> to be exposed to the kernel either through the LED framework or the PWM
> framework.
> 
> A typical use case for the LED configuration is to drive RGB LEDs in
> smartphones etc, for which the driver support multiple channels to be
> ganged up to a MULTICOLOR LED. In this configuration the pattern
> generators will be synchronized, to allow for multi-color patterns.
> 
> Signed-off-by: Bjorn Andersson <bjorn.andersson@linaro.org>

Reviewed-by: Marijn Suijten <marijn.suijten@somainline.org>

There may still be some things to improve based on whether lo_pause
works in non-ping-pong mode - to alleviate the requirement for the first
delta_t to be at most 512ms - but this patch should not be delayed much
longer and that's perhaps for a followup patch.  Same for my request for
documentation and examples which at the same time serve as some form of
tests to see if everything works as desired.

I also vaguely remember other (downstream) drivers to support more than
512ms per step by (drastically?) changing PWM period, but not sure how
that worked again nor if it was reliable.

- Marijn

> ---
> 
> Changes since v10:
> - Check for and reject pattern.delta_t greater than 9 bits
> - Write all 9 bits of LPG_RAMP_DURATION_REG
> 
>  drivers/leds/Kconfig             |    3 +
>  drivers/leds/Makefile            |    3 +
>  drivers/leds/rgb/Kconfig         |   13 +
>  drivers/leds/rgb/Makefile        |    3 +
>  drivers/leds/rgb/leds-qcom-lpg.c | 1306 ++++++++++++++++++++++++++++++
>  5 files changed, 1328 insertions(+)
>  create mode 100644 drivers/leds/rgb/Kconfig
>  create mode 100644 drivers/leds/rgb/Makefile
>  create mode 100644 drivers/leds/rgb/leds-qcom-lpg.c
> 
> diff --git a/drivers/leds/Kconfig b/drivers/leds/Kconfig
> index 6090e647daee..a49979f41eee 100644
> --- a/drivers/leds/Kconfig
> +++ b/drivers/leds/Kconfig
> @@ -869,6 +869,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"
> +
>  comment "LED Triggers"
>  source "drivers/leds/trigger/Kconfig"
>  
> diff --git a/drivers/leds/Makefile b/drivers/leds/Makefile
> index e58ecb36360f..4fd2f92cd198 100644
> --- a/drivers/leds/Makefile
> +++ b/drivers/leds/Makefile
> @@ -99,6 +99,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/
> +
>  # LED Triggers
>  obj-$(CONFIG_LEDS_TRIGGERS)		+= trigger/
>  
> diff --git a/drivers/leds/rgb/Kconfig b/drivers/leds/rgb/Kconfig
> new file mode 100644
> index 000000000000..20be3e11fe4a
> --- /dev/null
> +++ b/drivers/leds/rgb/Kconfig
> @@ -0,0 +1,13 @@
> +# SPDX-License-Identifier: GPL-2.0
> +
> +if LEDS_CLASS_MULTICOLOR
> +
> +config LEDS_QCOM_LPG
> +	tristate "LED support for Qualcomm LPG"
> +	depends on OF
> +	depends on SPMI
> +	help
> +	  This option enables support for the Light Pulse Generator found in a
> +	  wide variety of Qualcomm PMICs.
> +
> +endif # LEDS_CLASS_MULTICOLOR
> diff --git a/drivers/leds/rgb/Makefile b/drivers/leds/rgb/Makefile
> new file mode 100644
> index 000000000000..83114f44c4ea
> --- /dev/null
> +++ b/drivers/leds/rgb/Makefile
> @@ -0,0 +1,3 @@
> +# SPDX-License-Identifier: GPL-2.0
> +
> +obj-$(CONFIG_LEDS_QCOM_LPG)	+= leds-qcom-lpg.o
> diff --git a/drivers/leds/rgb/leds-qcom-lpg.c b/drivers/leds/rgb/leds-qcom-lpg.c
> new file mode 100644
> index 000000000000..06d5fca1d4b5
> --- /dev/null
> +++ b/drivers/leds/rgb/leds-qcom-lpg.c
> @@ -0,0 +1,1306 @@
> +// SPDX-License-Identifier: GPL-2.0-only
> +/*
> + * Copyright (c) 2017-2022 Linaro Ltd
> + * Copyright (c) 2010-2012, The Linux Foundation. All rights reserved.
> + */
> +#include <linux/bits.h>
> +#include <linux/led-class-multicolor.h>
> +#include <linux/module.h>
> +#include <linux/of.h>
> +#include <linux/of_device.h>
> +#include <linux/platform_device.h>
> +#include <linux/pwm.h>
> +#include <linux/regmap.h>
> +#include <linux/slab.h>
> +
> +#define LPG_PATTERN_CONFIG_REG	0x40
> +#define LPG_SIZE_CLK_REG	0x41
> +#define LPG_PREDIV_CLK_REG	0x42
> +#define PWM_TYPE_CONFIG_REG	0x43
> +#define PWM_VALUE_REG		0x44
> +#define PWM_ENABLE_CONTROL_REG	0x46
> +#define PWM_SYNC_REG		0x47
> +#define LPG_RAMP_DURATION_REG	0x50
> +#define LPG_HI_PAUSE_REG	0x52
> +#define LPG_LO_PAUSE_REG	0x54
> +#define LPG_HI_IDX_REG		0x56
> +#define LPG_LO_IDX_REG		0x57
> +#define PWM_SEC_ACCESS_REG	0xd0
> +#define PWM_DTEST_REG(x)	(0xe2 + (x) - 1)
> +
> +#define TRI_LED_SRC_SEL		0x45
> +#define TRI_LED_EN_CTL		0x46
> +#define TRI_LED_ATC_CTL		0x47
> +
> +#define LPG_LUT_REG(x)		(0x40 + (x) * 2)
> +#define RAMP_CONTROL_REG	0xc8
> +
> +#define LPG_RESOLUTION		512
> +#define LPG_MAX_M		7
> +
> +struct lpg_channel;
> +struct lpg_data;
> +
> +/**
> + * struct lpg - LPG device context
> + * @dev:	struct device for LPG device
> + * @map:	regmap for register access
> + * @pwm:	PWM-chip object, if operating in PWM mode
> + * @data:	reference to version specific data
> + * @lut_base:	base address of the LUT block (optional)
> + * @lut_size:	number of entries in the LUT block
> + * @lut_bitmap:	allocation bitmap for LUT entries
> + * @triled_base: base address of the TRILED block (optional)
> + * @triled_src:	power-source for the TRILED
> + * @triled_has_atc_ctl:	true if there is TRI_LED_ATC_CTL register
> + * @triled_has_src_sel:	true if there is TRI_LED_SRC_SEL register
> + * @channels:	list of PWM channels
> + * @num_channels: number of @channels
> + */
> +struct lpg {
> +	struct device *dev;
> +	struct regmap *map;
> +
> +	struct pwm_chip pwm;
> +
> +	const struct lpg_data *data;
> +
> +	u32 lut_base;
> +	u32 lut_size;
> +	unsigned long *lut_bitmap;
> +
> +	u32 triled_base;
> +	u32 triled_src;
> +	bool triled_has_atc_ctl;
> +	bool triled_has_src_sel;
> +
> +	struct lpg_channel *channels;
> +	unsigned int num_channels;
> +};
> +
> +/**
> + * struct lpg_channel - per channel data
> + * @lpg:	reference to parent lpg
> + * @base:	base address of the PWM channel
> + * @triled_mask: mask in TRILED to enable this channel
> + * @lut_mask:	mask in LUT to start pattern generator for this channel
> + * @in_use:	channel is exposed to LED framework
> + * @color:	color of the LED attached to this channel
> + * @dtest_line:	DTEST line for output, or 0 if disabled
> + * @dtest_value: DTEST line configuration
> + * @pwm_value:	duty (in microseconds) of the generated pulses, overridden by LUT
> + * @enabled:	output enabled?
> + * @period:	period (in nanoseconds) of the generated pulses
> + * @clk:	base frequency of the clock generator
> + * @pre_div:	divider of @clk
> + * @pre_div_exp: exponential divider of @clk
> + * @ramp_enabled: duty cycle is driven by iterating over lookup table
> + * @ramp_ping_pong: reverse through pattern, rather than wrapping to start
> + * @ramp_oneshot: perform only a single pass over the pattern
> + * @ramp_reverse: iterate over pattern backwards
> + * @ramp_tick_ms: length (in milliseconds) of one step in the pattern
> + * @ramp_lo_pause_ms: pause (in milliseconds) before iterating over pattern
> + * @ramp_hi_pause_ms: pause (in milliseconds) after iterating over pattern
> + * @pattern_lo_idx: start index of associated pattern
> + * @pattern_hi_idx: last index of associated pattern
> + */
> +struct lpg_channel {
> +	struct lpg *lpg;
> +
> +	u32 base;
> +	unsigned int triled_mask;
> +	unsigned int lut_mask;
> +
> +	bool in_use;
> +
> +	int color;
> +
> +	u32 dtest_line;
> +	u32 dtest_value;
> +
> +	u16 pwm_value;
> +	bool enabled;
> +
> +	u64 period;
> +	unsigned int clk;
> +	unsigned int pre_div;
> +	unsigned int pre_div_exp;
> +
> +	bool ramp_enabled;
> +	bool ramp_ping_pong;
> +	bool ramp_oneshot;
> +	bool ramp_reverse;
> +	unsigned short ramp_tick_ms;
> +	unsigned long ramp_lo_pause_ms;
> +	unsigned long ramp_hi_pause_ms;
> +
> +	unsigned int pattern_lo_idx;
> +	unsigned int pattern_hi_idx;
> +};
> +
> +/**
> + * struct lpg_led - logical LED object
> + * @lpg:		lpg context reference
> + * @cdev:		LED class device
> + * @mcdev:		Multicolor LED class device
> + * @num_channels:	number of @channels
> + * @channels:		list of channels associated with the LED
> + */
> +struct lpg_led {
> +	struct lpg *lpg;
> +
> +	struct led_classdev cdev;
> +	struct led_classdev_mc mcdev;
> +
> +	unsigned int num_channels;
> +	struct lpg_channel *channels[];
> +};
> +
> +/**
> + * struct lpg_channel_data - per channel initialization data
> + * @base:		base address for PWM channel registers
> + * @triled_mask:	bitmask for controlling this channel in TRILED
> + */
> +struct lpg_channel_data {
> +	unsigned int base;
> +	u8 triled_mask;
> +};
> +
> +/**
> + * 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
> + * @triled_has_atc_ctl:	true if there is TRI_LED_ATC_CTL register
> + * @triled_has_src_sel:	true if there is TRI_LED_SRC_SEL register
> + * @pwm_9bit_mask:	bitmask for switching from 6bit to 9bit pwm
> + * @num_channels:	number of channels in LPG
> + * @channels:		list of channel initialization data
> + */
> +struct lpg_data {
> +	unsigned int lut_base;
> +	unsigned int lut_size;
> +	unsigned int triled_base;
> +	bool triled_has_atc_ctl;
> +	bool triled_has_src_sel;
> +	unsigned int pwm_9bit_mask;
> +	int num_channels;
> +	const struct lpg_channel_data *channels;
> +};
> +
> +static int triled_set(struct lpg *lpg, unsigned int mask, unsigned int enable)
> +{
> +	/* Skip if we don't have a triled block */
> +	if (!lpg->triled_base)
> +		return 0;
> +
> +	return regmap_update_bits(lpg->map, lpg->triled_base + TRI_LED_EN_CTL,
> +				  mask, enable);
> +}
> +
> +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, sizeof(val));
> +	}
> +
> +	bitmap_set(lpg->lut_bitmap, idx, len);
> +
> +	*lo_idx = idx;
> +	*hi_idx = idx + len - 1;
> +
> +	return 0;
> +}
> +
> +static void lpg_lut_free(struct lpg *lpg, unsigned int lo_idx, unsigned int hi_idx)
> +{
> +	int len;
> +
> +	if (lo_idx == hi_idx)
> +		return;
> +
> +	len = hi_idx - lo_idx + 1;
> +	bitmap_clear(lpg->lut_bitmap, lo_idx, len);
> +}
> +
> +static int lpg_lut_sync(struct lpg *lpg, unsigned int mask)
> +{
> +	return regmap_write(lpg->map, lpg->lut_base + RAMP_CONTROL_REG, mask);
> +}
> +
> +static const unsigned int lpg_clk_rates[] = {1024, 32768, 19200000};
> +static const unsigned int lpg_pre_divs[] = {1, 3, 5, 6};
> +
> +static int lpg_calc_freq(struct lpg_channel *chan, uint64_t period)
> +{
> +	unsigned int clk, best_clk = 0;
> +	unsigned int div, best_div = 0;
> +	unsigned int m, best_m = 0;
> +	unsigned int error;
> +	unsigned int best_err = UINT_MAX;
> +	u64 best_period = 0;
> +
> +	/*
> +	 * The PWM period is determined by:
> +	 *
> +	 *          resolution * pre_div * 2^M
> +	 * period = --------------------------
> +	 *                   refclk
> +	 *
> +	 * With resolution fixed at 2^9 bits, pre_div = {1, 3, 5, 6} and
> +	 * M = [0..7].
> +	 *
> +	 * This allows for periods between 27uS and 384s, as the PWM framework
> +	 * wants a period of equal or lower length than requested, reject
> +	 * anything below 27uS.
> +	 */
> +	if (period <= (u64)NSEC_PER_SEC * LPG_RESOLUTION / 19200000)
> +		return -EINVAL;
> +
> +	/* Limit period to largest possible value, to avoid overflows */
> +	if (period > (u64)NSEC_PER_SEC * LPG_RESOLUTION * 6 * (1 << LPG_MAX_M) / 1024)
> +		period = (u64)NSEC_PER_SEC * LPG_RESOLUTION * 6 * (1 << LPG_MAX_M) / 2014;
> +
> +	/*
> +	 * Search for the pre_div, clk and M by solving the rewritten formula
> +	 * for each clk and pre_div value:
> +	 *
> +	 *                       period * clk
> +	 * M = log2 -------------------------------------
> +	 *           NSEC_PER_SEC * pre_div * resolution
> +	 */
> +	for (clk = 0; clk < ARRAY_SIZE(lpg_clk_rates); clk++) {
> +		u64 nom = period * lpg_clk_rates[clk];
> +
> +		for (div = 0; div < ARRAY_SIZE(lpg_pre_divs); div++) {
> +			u64 denom = (u64)NSEC_PER_SEC * lpg_pre_divs[div] * (1 << 9);
> +			u64 actual;
> +			u64 ratio;
> +
> +			if (nom < denom)
> +				continue;
> +
> +			ratio = div64_u64(nom, denom);
> +			m = ilog2(ratio);
> +			if (m > LPG_MAX_M)
> +				m = LPG_MAX_M;
> +
> +			actual = DIV_ROUND_UP_ULL(denom * (1 << m), lpg_clk_rates[clk]);
> +
> +			error = period - actual;
> +			if (error < best_err) {
> +				best_err = error;
> +
> +				best_div = div;
> +				best_m = m;
> +				best_clk = clk;
> +				best_period = actual;
> +			}
> +		}
> +	}
> +
> +	chan->clk = best_clk;
> +	chan->pre_div = best_div;
> +	chan->pre_div_exp = best_m;
> +	chan->period = best_period;
> +
> +	return 0;
> +}
> +
> +static void lpg_calc_duty(struct lpg_channel *chan, uint64_t duty)
> +{
> +	unsigned int max = LPG_RESOLUTION - 1;
> +	unsigned int val;
> +
> +	val = div64_u64(duty * lpg_clk_rates[chan->clk],
> +			(u64)NSEC_PER_SEC * lpg_pre_divs[chan->pre_div] * (1 << chan->pre_div_exp));
> +
> +	chan->pwm_value = min(val, max);
> +}
> +
> +static void lpg_apply_freq(struct lpg_channel *chan)
> +{
> +	unsigned long val;
> +	struct lpg *lpg = chan->lpg;
> +
> +	if (!chan->enabled)
> +		return;
> +
> +	/* Clock register values are off-by-one from lpg_clk_table */
> +	val = chan->clk + 1;
> +
> +	/* Enable 9bit resolution */
> +	val |= lpg->data->pwm_9bit_mask;
> +
> +	regmap_write(lpg->map, chan->base + LPG_SIZE_CLK_REG, val);
> +
> +	val = chan->pre_div << 5 | chan->pre_div_exp;
> +	regmap_write(lpg->map, chan->base + LPG_PREDIV_CLK_REG, val);
> +}
> +
> +#define LPG_ENABLE_GLITCH_REMOVAL	BIT(5)
> +
> +static void lpg_enable_glitch(struct lpg_channel *chan)
> +{
> +	struct lpg *lpg = chan->lpg;
> +
> +	regmap_update_bits(lpg->map, chan->base + PWM_TYPE_CONFIG_REG,
> +			   LPG_ENABLE_GLITCH_REMOVAL, 0);
> +}
> +
> +static void lpg_disable_glitch(struct lpg_channel *chan)
> +{
> +	struct lpg *lpg = chan->lpg;
> +
> +	regmap_update_bits(lpg->map, chan->base + PWM_TYPE_CONFIG_REG,
> +			   LPG_ENABLE_GLITCH_REMOVAL,
> +			   LPG_ENABLE_GLITCH_REMOVAL);
> +}
> +
> +static void lpg_apply_pwm_value(struct lpg_channel *chan)
> +{
> +	struct lpg *lpg = chan->lpg;
> +	u16 val = chan->pwm_value;
> +
> +	if (!chan->enabled)
> +		return;
> +
> +	regmap_bulk_write(lpg->map, chan->base + PWM_VALUE_REG, &val, sizeof(val));
> +}
> +
> +#define LPG_PATTERN_CONFIG_LO_TO_HI	BIT(4)
> +#define LPG_PATTERN_CONFIG_REPEAT	BIT(3)
> +#define LPG_PATTERN_CONFIG_TOGGLE	BIT(2)
> +#define LPG_PATTERN_CONFIG_PAUSE_HI	BIT(1)
> +#define LPG_PATTERN_CONFIG_PAUSE_LO	BIT(0)
> +
> +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 conf = 0;
> +	unsigned int lo_idx = chan->pattern_lo_idx;
> +	unsigned int hi_idx = chan->pattern_hi_idx;
> +	u16 step = chan->ramp_tick_ms;
> +
> +	if (!chan->ramp_enabled || chan->pattern_lo_idx == chan->pattern_hi_idx)
> +		return;
> +
> +	hi_pause = DIV_ROUND_UP(chan->ramp_hi_pause_ms, step);
> +	lo_pause = DIV_ROUND_UP(chan->ramp_lo_pause_ms, step);
> +
> +	if (!chan->ramp_reverse)
> +		conf |= LPG_PATTERN_CONFIG_LO_TO_HI;
> +	if (!chan->ramp_oneshot)
> +		conf |= LPG_PATTERN_CONFIG_REPEAT;
> +	if (chan->ramp_ping_pong)
> +		conf |= LPG_PATTERN_CONFIG_TOGGLE;
> +	if (chan->ramp_hi_pause_ms)
> +		conf |= LPG_PATTERN_CONFIG_PAUSE_HI;
> +	if (chan->ramp_lo_pause_ms)
> +		conf |= LPG_PATTERN_CONFIG_PAUSE_LO;
> +
> +	regmap_write(lpg->map, chan->base + LPG_PATTERN_CONFIG_REG, conf);
> +	regmap_write(lpg->map, chan->base + LPG_HI_IDX_REG, hi_idx);
> +	regmap_write(lpg->map, chan->base + LPG_LO_IDX_REG, lo_idx);
> +
> +	regmap_bulk_write(lpg->map, chan->base + LPG_RAMP_DURATION_REG, &step, sizeof(step));
> +	regmap_write(lpg->map, chan->base + LPG_HI_PAUSE_REG, hi_pause);
> +	regmap_write(lpg->map, chan->base + LPG_LO_PAUSE_REG, lo_pause);
> +}
> +
> +#define LPG_ENABLE_CONTROL_OUTPUT		BIT(7)
> +#define LPG_ENABLE_CONTROL_BUFFER_TRISTATE	BIT(5)
> +#define LPG_ENABLE_CONTROL_SRC_PWM		BIT(2)
> +#define LPG_ENABLE_CONTROL_RAMP_GEN		BIT(1)
> +
> +static void lpg_apply_control(struct lpg_channel *chan)
> +{
> +	unsigned int ctrl;
> +	struct lpg *lpg = chan->lpg;
> +
> +	ctrl = LPG_ENABLE_CONTROL_BUFFER_TRISTATE;
> +
> +	if (chan->enabled)
> +		ctrl |= LPG_ENABLE_CONTROL_OUTPUT;
> +
> +	if (chan->pattern_lo_idx != chan->pattern_hi_idx)
> +		ctrl |= LPG_ENABLE_CONTROL_RAMP_GEN;
> +	else
> +		ctrl |= LPG_ENABLE_CONTROL_SRC_PWM;
> +
> +	regmap_write(lpg->map, chan->base + PWM_ENABLE_CONTROL_REG, ctrl);
> +
> +	/*
> +	 * Due to LPG hardware bug, in the PWM mode, having enabled PWM,
> +	 * We have to write PWM values one more time.
> +	 */
> +	if (chan->enabled)
> +		lpg_apply_pwm_value(chan);
> +}
> +
> +#define LPG_SYNC_PWM	BIT(0)
> +
> +static void lpg_apply_sync(struct lpg_channel *chan)
> +{
> +	struct lpg *lpg = chan->lpg;
> +
> +	regmap_write(lpg->map, chan->base + PWM_SYNC_REG, LPG_SYNC_PWM);
> +}
> +
> +static int lpg_parse_dtest(struct lpg *lpg)
> +{
> +	struct lpg_channel *chan;
> +	struct device_node *np = lpg->dev->of_node;
> +	int count;
> +	int ret;
> +	int i;
> +
> +	count = of_property_count_u32_elems(np, "qcom,dtest");
> +	if (count == -EINVAL) {
> +		return 0;
> +	} else if (count < 0) {
> +		ret = count;
> +		goto err_malformed;
> +	} else if (count != lpg->data->num_channels * 2) {
> +		dev_err(lpg->dev, "qcom,dtest needs to be %d items\n",
> +			lpg->data->num_channels * 2);
> +		return -EINVAL;
> +	}
> +
> +	for (i = 0; i < lpg->data->num_channels; i++) {
> +		chan = &lpg->channels[i];
> +
> +		ret = of_property_read_u32_index(np, "qcom,dtest", i * 2,
> +						 &chan->dtest_line);
> +		if (ret)
> +			goto err_malformed;
> +
> +		ret = of_property_read_u32_index(np, "qcom,dtest", i * 2 + 1,
> +						 &chan->dtest_value);
> +		if (ret)
> +			goto err_malformed;
> +	}
> +
> +	return 0;
> +
> +err_malformed:
> +	dev_err(lpg->dev, "malformed qcom,dtest\n");
> +	return ret;
> +}
> +
> +static void lpg_apply_dtest(struct lpg_channel *chan)
> +{
> +	struct lpg *lpg = chan->lpg;
> +
> +	if (!chan->dtest_line)
> +		return;
> +
> +	regmap_write(lpg->map, chan->base + PWM_SEC_ACCESS_REG, 0xa5);
> +	regmap_write(lpg->map, chan->base + PWM_DTEST_REG(chan->dtest_line),
> +		     chan->dtest_value);
> +}
> +
> +static void lpg_apply(struct lpg_channel *chan)
> +{
> +	lpg_disable_glitch(chan);
> +	lpg_apply_freq(chan);
> +	lpg_apply_pwm_value(chan);
> +	lpg_apply_control(chan);
> +	lpg_apply_sync(chan);
> +	lpg_apply_lut_control(chan);
> +	lpg_enable_glitch(chan);
> +}
> +
> +static void lpg_brightness_set(struct lpg_led *led, struct led_classdev *cdev,
> +			       struct mc_subled *subleds)
> +{
> +	enum led_brightness brightness;
> +	struct lpg_channel *chan;
> +	unsigned int triled_enabled = 0;
> +	unsigned int triled_mask = 0;
> +	unsigned int lut_mask = 0;
> +	unsigned int duty;
> +	struct lpg *lpg = led->lpg;
> +	int i;
> +
> +	for (i = 0; i < led->num_channels; i++) {
> +		chan = led->channels[i];
> +		brightness = subleds[i].brightness;
> +
> +		if (brightness == LED_OFF) {
> +			chan->enabled = false;
> +			chan->ramp_enabled = false;
> +		} else if (chan->pattern_lo_idx != chan->pattern_hi_idx) {
> +			lpg_calc_freq(chan, NSEC_PER_MSEC);
> +
> +			chan->enabled = true;
> +			chan->ramp_enabled = true;
> +
> +			lut_mask |= chan->lut_mask;
> +			triled_enabled |= chan->triled_mask;
> +		} else {
> +			lpg_calc_freq(chan, NSEC_PER_MSEC);
> +
> +			duty = div_u64(brightness * chan->period, cdev->max_brightness);
> +			lpg_calc_duty(chan, duty);
> +			chan->enabled = true;
> +			chan->ramp_enabled = false;
> +
> +			triled_enabled |= chan->triled_mask;
> +		}
> +
> +		triled_mask |= chan->triled_mask;
> +
> +		lpg_apply(chan);
> +	}
> +
> +	/* Toggle triled lines */
> +	if (triled_mask)
> +		triled_set(lpg, triled_mask, triled_enabled);
> +
> +	/* Trigger start of ramp generator(s) */
> +	if (lut_mask)
> +		lpg_lut_sync(lpg, lut_mask);
> +}
> +
> +static void lpg_brightness_single_set(struct led_classdev *cdev,
> +				      enum led_brightness value)
> +{
> +	struct lpg_led *led = container_of(cdev, struct lpg_led, cdev);
> +	struct mc_subled info;
> +
> +	info.brightness = value;
> +	lpg_brightness_set(led, cdev, &info);
> +}
> +
> +static void lpg_brightness_mc_set(struct led_classdev *cdev,
> +				  enum led_brightness value)
> +{
> +	struct led_classdev_mc *mc = lcdev_to_mccdev(cdev);
> +	struct lpg_led *led = container_of(mc, struct lpg_led, mcdev);
> +
> +	led_mc_calc_color_components(mc, value);
> +	lpg_brightness_set(led, cdev, mc->subled_info);
> +}
> +
> +static int lpg_blink_set(struct lpg_led *led,
> +			 unsigned long *delay_on, unsigned long *delay_off)
> +{
> +	struct lpg_channel *chan;
> +	unsigned int period;
> +	unsigned int triled_mask = 0;
> +	struct lpg *lpg = led->lpg;
> +	u64 duty;
> +	int i;
> +
> +	if (!*delay_on && !*delay_off) {
> +		*delay_on = 500;
> +		*delay_off = 500;
> +	}
> +
> +	duty = *delay_on * NSEC_PER_MSEC;
> +	period = (*delay_on + *delay_off) * NSEC_PER_MSEC;
> +
> +	for (i = 0; i < led->num_channels; i++) {
> +		chan = led->channels[i];
> +
> +		lpg_calc_freq(chan, period);
> +		lpg_calc_duty(chan, duty);
> +
> +		chan->enabled = true;
> +		chan->ramp_enabled = false;
> +
> +		triled_mask |= chan->triled_mask;
> +
> +		lpg_apply(chan);
> +	}
> +
> +	/* Enable triled lines */
> +	triled_set(lpg, triled_mask, triled_mask);
> +
> +	chan = led->channels[0];
> +	duty = div_u64(chan->pwm_value * chan->period, LPG_RESOLUTION);
> +	*delay_on = div_u64(duty, NSEC_PER_MSEC);
> +	*delay_off = div_u64(chan->period - duty, NSEC_PER_MSEC);
> +
> +	return 0;
> +}
> +
> +static int lpg_blink_single_set(struct led_classdev *cdev,
> +				unsigned long *delay_on, unsigned long *delay_off)
> +{
> +	struct lpg_led *led = container_of(cdev, struct lpg_led, cdev);
> +
> +	return lpg_blink_set(led, delay_on, delay_off);
> +}
> +
> +static int lpg_blink_mc_set(struct led_classdev *cdev,
> +			    unsigned long *delay_on, unsigned long *delay_off)
> +{
> +	struct led_classdev_mc *mc = lcdev_to_mccdev(cdev);
> +	struct lpg_led *led = container_of(mc, struct lpg_led, mcdev);
> +
> +	return lpg_blink_set(led, delay_on, delay_off);
> +}
> +
> +static int lpg_pattern_set(struct lpg_led *led, struct led_pattern *pattern,
> +			   u32 len, int repeat)
> +{
> +	struct lpg_channel *chan;
> +	struct lpg *lpg = led->lpg;
> +	unsigned int hi_pause;
> +	unsigned int lo_pause;
> +	unsigned int lo_idx;
> +	unsigned int hi_idx;
> +	bool ping_pong = true;
> +	int brightness_a;
> +	int brightness_b;
> +	int ret;
> +	int i;
> +
> +	/* Only support oneshot or indefinite loops, due to limited pattern space */
> +	if (repeat != -1 && repeat != 1)
> +		return -EINVAL;
> +
> +	/* LPG_RAMP_DURATION_REG is 9 bit */
> +	if (pattern[0].delta_t >= 512)
> +		return -EINVAL;
> +
> +	/*
> +	 * The LPG plays patterns with at a fixed pace, a "low pause" can be
> +	 * performed before the pattern and a "high pause" after. In order to
> +	 * save space the pattern can be played in "ping pong" mode, in which
> +	 * the pattern is first played forward, then "high pause" is applied,
> +	 * then the pattern is played backwards and finally the "low pause" is
> +	 * applied.
> +	 *
> +	 * The delta_t of the first entry is used to determine the pace of the
> +	 * pattern.
> +	 *
> +	 * If the specified pattern is a palindrome the ping pong mode is
> +	 * enabled. In this scenario the delta_t of the last entry determines
> +	 * the "low pause" time and the delta_t of the middle entry (i.e. the
> +	 * last in the programmed pattern) determines the "high pause". If the
> +	 * pattern consists of an odd number of values, no "high pause" is
> +	 * used.
> +	 *
> +	 * When ping pong mode is not selected, the delta_t of the last entry
> +	 * is used as "high pause". No "low pause" is used.
> +	 *
> +	 * delta_t of any other members of the pattern is ignored.
> +	 */
> +
> +	/* Detect palindromes and use "ping pong" to reduce LUT usage */
> +	for (i = 0; i < len / 2; i++) {
> +		brightness_a = pattern[i].brightness;
> +		brightness_b = pattern[len - i - 1].brightness;
> +
> +		if (brightness_a != brightness_b) {
> +			ping_pong = false;
> +			break;
> +		}
> +	}
> +
> +	if (ping_pong) {
> +		if (len % 2)
> +			hi_pause = 0;
> +		else
> +			hi_pause = pattern[(len + 1) / 2].delta_t;
> +		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)
> +		return ret;
> +
> +	for (i = 0; i < led->num_channels; i++) {
> +		chan = led->channels[i];
> +
> +		chan->ramp_tick_ms = pattern[0].delta_t;
> +		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;
> +	}
> +
> +	return 0;
> +}
> +
> +static int lpg_pattern_single_set(struct led_classdev *cdev,
> +				  struct led_pattern *pattern, u32 len,
> +				  int repeat)
> +{
> +	struct lpg_led *led = container_of(cdev, struct lpg_led, cdev);
> +	int ret;
> +
> +	ret = lpg_pattern_set(led, pattern, len, repeat);
> +	if (ret < 0)
> +		return ret;
> +
> +	lpg_brightness_single_set(cdev, LED_FULL);
> +
> +	return 0;
> +}
> +
> +static int lpg_pattern_mc_set(struct led_classdev *cdev,
> +			      struct led_pattern *pattern, u32 len,
> +			      int repeat)
> +{
> +	struct led_classdev_mc *mc = lcdev_to_mccdev(cdev);
> +	struct lpg_led *led = container_of(mc, struct lpg_led, mcdev);
> +	int ret;
> +
> +	ret = lpg_pattern_set(led, pattern, len, repeat);
> +	if (ret < 0)
> +		return ret;
> +
> +	led_mc_calc_color_components(mc, LED_FULL);
> +	lpg_brightness_set(led, cdev, mc->subled_info);
> +
> +	return 0;
> +}
> +
> +static int lpg_pattern_clear(struct lpg_led *led)
> +{
> +	struct lpg_channel *chan;
> +	struct lpg *lpg = led->lpg;
> +	int i;
> +
> +	chan = led->channels[0];
> +	lpg_lut_free(lpg, chan->pattern_lo_idx, chan->pattern_hi_idx);
> +
> +	for (i = 0; i < led->num_channels; i++) {
> +		chan = led->channels[i];
> +		chan->pattern_lo_idx = 0;
> +		chan->pattern_hi_idx = 0;
> +	}
> +
> +	return 0;
> +}
> +
> +static int lpg_pattern_single_clear(struct led_classdev *cdev)
> +{
> +	struct lpg_led *led = container_of(cdev, struct lpg_led, cdev);
> +
> +	return lpg_pattern_clear(led);
> +}
> +
> +static int lpg_pattern_mc_clear(struct led_classdev *cdev)
> +{
> +	struct led_classdev_mc *mc = lcdev_to_mccdev(cdev);
> +	struct lpg_led *led = container_of(mc, struct lpg_led, mcdev);
> +
> +	return lpg_pattern_clear(led);
> +}
> +
> +static int lpg_pwm_request(struct pwm_chip *chip, struct pwm_device *pwm)
> +{
> +	struct lpg *lpg = container_of(chip, struct lpg, pwm);
> +	struct lpg_channel *chan = &lpg->channels[pwm->hwpwm];
> +
> +	return chan->in_use ? -EBUSY : 0;
> +}
> +
> +/*
> + * Limitations:
> + * - Updating both duty and period is not done atomically, so the output signal
> + *   will momentarily be a mix of the settings.
> + */
> +static int lpg_pwm_apply(struct pwm_chip *chip, struct pwm_device *pwm,
> +			 const struct pwm_state *state)
> +{
> +	struct lpg *lpg = container_of(chip, struct lpg, pwm);
> +	struct lpg_channel *chan = &lpg->channels[pwm->hwpwm];
> +	int ret;
> +
> +	if (state->polarity != PWM_POLARITY_NORMAL)
> +		return -EINVAL;
> +
> +	ret = lpg_calc_freq(chan, state->period);
> +	if (ret < 0)
> +		return ret;
> +
> +	lpg_calc_duty(chan, state->duty_cycle);
> +	chan->enabled = state->enabled;
> +
> +	lpg_apply(chan);
> +
> +	triled_set(lpg, chan->triled_mask, chan->enabled ? chan->triled_mask : 0);
> +
> +	return 0;
> +}
> +
> +static void lpg_pwm_get_state(struct pwm_chip *chip, struct pwm_device *pwm,
> +			      struct pwm_state *state)
> +{
> +	struct lpg *lpg = container_of(chip, struct lpg, pwm);
> +	struct lpg_channel *chan = &lpg->channels[pwm->hwpwm];
> +	u64 duty = DIV_ROUND_UP_ULL(chan->pwm_value * chan->period, LPG_RESOLUTION - 1);
> +
> +	state->period = chan->period;
> +	state->duty_cycle = duty;
> +	state->polarity = PWM_POLARITY_NORMAL;
> +	state->enabled = chan->enabled;
> +}
> +
> +static const struct pwm_ops lpg_pwm_ops = {
> +	.request = lpg_pwm_request,
> +	.apply = lpg_pwm_apply,
> +	.get_state = lpg_pwm_get_state,
> +	.owner = THIS_MODULE,
> +};
> +
> +static int lpg_add_pwm(struct lpg *lpg)
> +{
> +	int ret;
> +
> +	lpg->pwm.base = -1;
> +	lpg->pwm.dev = lpg->dev;
> +	lpg->pwm.npwm = lpg->num_channels;
> +	lpg->pwm.ops = &lpg_pwm_ops;
> +
> +	ret = pwmchip_add(&lpg->pwm);
> +	if (ret)
> +		dev_err(lpg->dev, "failed to add PWM chip: ret %d\n", ret);
> +
> +	return ret;
> +}
> +
> +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);
> +		return -EINVAL;
> +	}
> +
> +	chan = &lpg->channels[reg - 1];
> +	chan->in_use = true;
> +
> +	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;
> +	}
> +
> +	chan->color = color;
> +
> +	*channel = chan;
> +
> +	return 0;
> +}
> +
> +static int lpg_add_led(struct lpg *lpg, struct device_node *np)
> +{
> +	struct led_init_data init_data = {};
> +	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_RGB)
> +		num_channels = of_get_available_child_count(np);
> +	else
> +		num_channels = 1;
> +
> +	led = devm_kzalloc(lpg->dev, struct_size(led, channels, num_channels), GFP_KERNEL);
> +	if (!led)
> +		return -ENOMEM;
> +
> +	led->lpg = lpg;
> +	led->num_channels = num_channels;
> +
> +	if (color == LED_COLOR_ID_RGB) {
> +		info = devm_kcalloc(lpg->dev, num_channels, sizeof(*info), GFP_KERNEL);
> +		if (!info)
> +			return -ENOMEM;
> +		i = 0;
> +		for_each_available_child_of_node(np, child) {
> +			ret = lpg_parse_channel(lpg, child, &led->channels[i]);
> +			if (ret < 0)
> +				return ret;
> +
> +			info[i].color_index = led->channels[i]->color;
> +			info[i].intensity = LED_FULL;
> +			i++;
> +		}
> +
> +		led->mcdev.subled_info = info;
> +		led->mcdev.num_colors = num_channels;
> +
> +		cdev = &led->mcdev.led_cdev;
> +		cdev->brightness_set = lpg_brightness_mc_set;
> +		cdev->blink_set = lpg_blink_mc_set;
> +
> +		/* Register pattern accessors only if we have a LUT block */
> +		if (lpg->lut_base) {
> +			cdev->pattern_set = lpg_pattern_mc_set;
> +			cdev->pattern_clear = lpg_pattern_mc_clear;
> +		}
> +	} else {
> +		ret = lpg_parse_channel(lpg, np, &led->channels[0]);
> +		if (ret < 0)
> +			return ret;
> +
> +		cdev = &led->cdev;
> +		cdev->brightness_set = lpg_brightness_single_set;
> +		cdev->blink_set = lpg_blink_single_set;
> +
> +		/* Register pattern accessors only if we have a LUT block */
> +		if (lpg->lut_base) {
> +			cdev->pattern_set = lpg_pattern_single_set;
> +			cdev->pattern_clear = lpg_pattern_single_clear;
> +		}
> +	}
> +
> +	cdev->default_trigger = of_get_property(np, "linux,default-trigger", NULL);
> +	cdev->max_brightness = 255;
> +
> +	if (!of_property_read_string(np, "default-state", &state) &&
> +	    !strcmp(state, "on"))
> +		cdev->brightness = LED_FULL;
> +	else
> +		cdev->brightness = LED_OFF;
> +
> +	cdev->brightness_set(cdev, cdev->brightness);
> +
> +	init_data.fwnode = of_fwnode_handle(np);
> +
> +	if (color == LED_COLOR_ID_RGB)
> +		ret = devm_led_classdev_multicolor_register_ext(lpg->dev, &led->mcdev, &init_data);
> +	else
> +		ret = devm_led_classdev_register_ext(lpg->dev, &led->cdev, &init_data);
> +	if (ret)
> +		dev_err(lpg->dev, "unable to register %s\n", cdev->name);
> +
> +	return ret;
> +}
> +
> +static int lpg_init_channels(struct lpg *lpg)
> +{
> +	const struct lpg_data *data = lpg->data;
> +	int i;
> +
> +	lpg->num_channels = data->num_channels;
> +	lpg->channels = devm_kcalloc(lpg->dev, data->num_channels,
> +				     sizeof(struct lpg_channel), GFP_KERNEL);
> +	if (!lpg->channels)
> +		return -ENOMEM;
> +
> +	for (i = 0; i < data->num_channels; i++) {
> +		lpg->channels[i].lpg = lpg;
> +		lpg->channels[i].base = data->channels[i].base;
> +		lpg->channels[i].triled_mask = data->channels[i].triled_mask;
> +		lpg->channels[i].lut_mask = BIT(i);
> +	}
> +
> +	return 0;
> +}
> +
> +static int lpg_init_triled(struct lpg *lpg)
> +{
> +	struct device_node *np = lpg->dev->of_node;
> +	int ret;
> +
> +	/* Skip initialization if we don't have a triled block */
> +	if (!lpg->data->triled_base)
> +		return 0;
> +
> +	lpg->triled_base = lpg->data->triled_base;
> +	lpg->triled_has_atc_ctl = lpg->data->triled_has_atc_ctl;
> +	lpg->triled_has_src_sel = lpg->data->triled_has_src_sel;
> +
> +	if (lpg->triled_has_src_sel) {
> +		ret = of_property_read_u32(np, "qcom,power-source", &lpg->triled_src);
> +		if (ret || lpg->triled_src == 2 || lpg->triled_src > 3) {
> +			dev_err(lpg->dev, "invalid power source\n");
> +			return -EINVAL;
> +		}
> +	}
> +
> +	/* Disable automatic trickle charge LED */
> +	if (lpg->triled_has_atc_ctl)
> +		regmap_write(lpg->map, lpg->triled_base + TRI_LED_ATC_CTL, 0);
> +
> +	/* Configure power source */
> +	if (lpg->triled_has_src_sel)
> +		regmap_write(lpg->map, lpg->triled_base + TRI_LED_SRC_SEL, lpg->triled_src);
> +
> +	/* Default all outputs to off */
> +	regmap_write(lpg->map, lpg->triled_base + TRI_LED_EN_CTL, 0);
> +
> +	return 0;
> +}
> +
> +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;
> +
> +	return 0;
> +}
> +
> +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;
> +
> +	platform_set_drvdata(pdev, lpg);
> +
> +	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_parse_dtest(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;
> +
> +	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]);
> +
> +	return lpg_add_pwm(lpg);
> +}
> +
> +static int lpg_remove(struct platform_device *pdev)
> +{
> +	struct lpg *lpg = platform_get_drvdata(pdev);
> +
> +	pwmchip_remove(&lpg->pwm);
> +
> +	return 0;
> +}
> +
> +static const struct lpg_data pm8916_pwm_data = {
> +	.pwm_9bit_mask = BIT(2),
> +
> +	.num_channels = 1,
> +	.channels = (const struct lpg_channel_data[]) {
> +		{ .base = 0xbc00 },
> +	},
> +};
> +
> +static const struct lpg_data pm8941_lpg_data = {
> +	.lut_base = 0xb000,
> +	.lut_size = 64,
> +
> +	.triled_base = 0xd000,
> +	.triled_has_atc_ctl = true,
> +	.triled_has_src_sel = true,
> +
> +	.pwm_9bit_mask = 3 << 4,
> +
> +	.num_channels = 8,
> +	.channels = (const struct lpg_channel_data[]) {
> +		{ .base = 0xb100 },
> +		{ .base = 0xb200 },
> +		{ .base = 0xb300 },
> +		{ .base = 0xb400 },
> +		{ .base = 0xb500, .triled_mask = BIT(5) },
> +		{ .base = 0xb600, .triled_mask = BIT(6) },
> +		{ .base = 0xb700, .triled_mask = BIT(7) },
> +		{ .base = 0xb800 },
> +	},
> +};
> +
> +static const struct lpg_data pm8994_lpg_data = {
> +	.lut_base = 0xb000,
> +	.lut_size = 64,
> +
> +	.pwm_9bit_mask = 3 << 4,
> +
> +	.num_channels = 6,
> +	.channels = (const struct lpg_channel_data[]) {
> +		{ .base = 0xb100 },
> +		{ .base = 0xb200 },
> +		{ .base = 0xb300 },
> +		{ .base = 0xb400 },
> +		{ .base = 0xb500 },
> +		{ .base = 0xb600 },
> +	},
> +};
> +
> +static const struct lpg_data pmi8994_lpg_data = {
> +	.lut_base = 0xb000,
> +	.lut_size = 24,
> +
> +	.triled_base = 0xd000,
> +	.triled_has_atc_ctl = true,
> +	.triled_has_src_sel = true,
> +
> +	.pwm_9bit_mask = BIT(4),
> +
> +	.num_channels = 4,
> +	.channels = (const struct lpg_channel_data[]) {
> +		{ .base = 0xb100, .triled_mask = BIT(5) },
> +		{ .base = 0xb200, .triled_mask = BIT(6) },
> +		{ .base = 0xb300, .triled_mask = BIT(7) },
> +		{ .base = 0xb400 },
> +	},
> +};
> +
> +static const struct lpg_data pmi8998_lpg_data = {
> +	.lut_base = 0xb000,
> +	.lut_size = 49,
> +
> +	.triled_base = 0xd000,
> +
> +	.pwm_9bit_mask = BIT(4),
> +
> +	.num_channels = 6,
> +	.channels = (const struct lpg_channel_data[]) {
> +		{ .base = 0xb100 },
> +		{ .base = 0xb200 },
> +		{ .base = 0xb300, .triled_mask = BIT(5) },
> +		{ .base = 0xb400, .triled_mask = BIT(6) },
> +		{ .base = 0xb500, .triled_mask = BIT(7) },
> +		{ .base = 0xb600 },
> +	},
> +};
> +
> +static const struct lpg_data pm8150b_lpg_data = {
> +	.lut_base = 0xb000,
> +	.lut_size = 24,
> +
> +	.triled_base = 0xd000,
> +
> +	.pwm_9bit_mask = BIT(4),
> +
> +	.num_channels = 2,
> +	.channels = (const struct lpg_channel_data[]) {
> +		{ .base = 0xb100, .triled_mask = BIT(7) },
> +		{ .base = 0xb200, .triled_mask = BIT(6) },
> +	},
> +};
> +
> +static const struct lpg_data pm8150l_lpg_data = {
> +	.lut_base = 0xb000,
> +	.lut_size = 48,
> +
> +	.triled_base = 0xd000,
> +
> +	.pwm_9bit_mask = BIT(4),
> +
> +	.num_channels = 5,
> +	.channels = (const struct lpg_channel_data[]) {
> +		{ .base = 0xb100, .triled_mask = BIT(7) },
> +		{ .base = 0xb200, .triled_mask = BIT(6) },
> +		{ .base = 0xb300, .triled_mask = BIT(5) },
> +		{ .base = 0xbc00 },
> +		{ .base = 0xbd00 },
> +
> +	},
> +};
> +
> +static const struct of_device_id lpg_of_table[] = {
> +	{ .compatible = "qcom,pm8150b-lpg", .data = &pm8150b_lpg_data },
> +	{ .compatible = "qcom,pm8150l-lpg", .data = &pm8150l_lpg_data },
> +	{ .compatible = "qcom,pm8916-pwm", .data = &pm8916_pwm_data },
> +	{ .compatible = "qcom,pm8941-lpg", .data = &pm8941_lpg_data },
> +	{ .compatible = "qcom,pm8994-lpg", .data = &pm8994_lpg_data },
> +	{ .compatible = "qcom,pmi8994-lpg", .data = &pmi8994_lpg_data },
> +	{ .compatible = "qcom,pmi8998-lpg", .data = &pmi8998_lpg_data },
> +	{ .compatible = "qcom,pmc8180c-lpg", .data = &pm8150l_lpg_data },
> +	{}
> +};
> +MODULE_DEVICE_TABLE(of, lpg_of_table);
> +
> +static struct platform_driver lpg_driver = {
> +	.probe = lpg_probe,
> +	.remove = lpg_remove,
> +	.driver = {
> +		.name = "qcom-spmi-lpg",
> +		.of_match_table = lpg_of_table,
> +	},
> +};
> +module_platform_driver(lpg_driver);
> +
> +MODULE_DESCRIPTION("Qualcomm LPG LED driver");
> +MODULE_LICENSE("GPL v2");
> -- 
> 2.33.1
>
Andy Shevchenko Feb. 2, 2022, 2:56 p.m. UTC | #2
On Tue, Feb 1, 2022 at 12:31 AM Bjorn Andersson
<bjorn.andersson@linaro.org> wrote:
>
> The Light Pulse Generator (LPG) is a PWM-block found in a wide range of
> PMICs from Qualcomm. These PMICs typically comes with 1-8 LPG instances,
> with their output being routed to various other components, such as
> current sinks or GPIOs.
>
> Each LPG instance can operate on fixed parameters or based on a shared
> lookup-table, altering the duty cycle over time. This provides the means
> for hardware assisted transitions of LED brightness.
>
> A typical use case for the fixed parameter mode is to drive a PWM
> backlight control signal, the driver therefor allows each LPG instance
> to be exposed to the kernel either through the LED framework or the PWM
> framework.
>
> A typical use case for the LED configuration is to drive RGB LEDs in
> smartphones etc, for which the driver support multiple channels to be

supports

> ganged up to a MULTICOLOR LED. In this configuration the pattern
> generators will be synchronized, to allow for multi-color patterns.

...

> +config LEDS_QCOM_LPG
> +       tristate "LED support for Qualcomm LPG"

> +       depends on OF

|| COMPILE_TEST

> +       depends on SPMI
> +       help
> +         This option enables support for the Light Pulse Generator found in a
> +         wide variety of Qualcomm PMICs.

Module name?

...

> +#include <linux/of.h>
> +#include <linux/of_device.h>

Wondering if these can be changed to mod_devicetable.h + property.,h.

...

> + * @dev:       struct device for LPG device

Description without value and actually wrong. it's a pointer to, and
not a struct device.

...

> +       /* Hardware does not behave when LO_IDX == HI_IDX */

Any clue /. elaboration why?

...

> +static void lpg_lut_free(struct lpg *lpg, unsigned int lo_idx, unsigned int hi_idx)
> +{
> +       int len;
> +
> +       if (lo_idx == hi_idx)
> +               return;
> +
> +       len = hi_idx - lo_idx + 1;

Perhaps swap above and add the similar comment:

/* We never do a single item because ... */
len =
if (len == 1)

> +       bitmap_clear(lpg->lut_bitmap, lo_idx, len);

Who protects this bitmap from simultaneous access by different users?

> +}

...

> +static int lpg_calc_freq(struct lpg_channel *chan, uint64_t period)
> +{
> +       unsigned int clk, best_clk = 0;
> +       unsigned int div, best_div = 0;
> +       unsigned int m, best_m = 0;
> +       unsigned int error;
> +       unsigned int best_err = UINT_MAX;
> +       u64 best_period = 0;
> +
> +       /*
> +        * The PWM period is determined by:
> +        *
> +        *          resolution * pre_div * 2^M
> +        * period = --------------------------
> +        *                   refclk
> +        *
> +        * With resolution fixed at 2^9 bits, pre_div = {1, 3, 5, 6} and
> +        * M = [0..7].
> +        *
> +        * This allows for periods between 27uS and 384s, as the PWM framework
> +        * wants a period of equal or lower length than requested, reject
> +        * anything below 27uS.
> +        */

> +       if (period <= (u64)NSEC_PER_SEC * LPG_RESOLUTION / 19200000)
> +               return -EINVAL;

> +       /* Limit period to largest possible value, to avoid overflows */
> +       if (period > (u64)NSEC_PER_SEC * LPG_RESOLUTION * 6 * (1 << LPG_MAX_M) / 1024)
> +               period = (u64)NSEC_PER_SEC * LPG_RESOLUTION * 6 * (1 << LPG_MAX_M) / 2014;

2014?!

And if it's incorrect, it seems like a good example to avoid
repetition of the long equations.

What about

  best_period = clamp_val(period, ...);
  if (best_period >= period)
    return -EINVAL;

  period = best_period;

?

> +       /*
> +        * Search for the pre_div, clk and M by solving the rewritten formula
> +        * for each clk and pre_div value:
> +        *
> +        *                       period * clk
> +        * M = log2 -------------------------------------
> +        *           NSEC_PER_SEC * pre_div * resolution
> +        */
> +       for (clk = 0; clk < ARRAY_SIZE(lpg_clk_rates); clk++) {
> +               u64 nom = period * lpg_clk_rates[clk];

Can we spell fully nunerator, denominator?

> +               for (div = 0; div < ARRAY_SIZE(lpg_pre_divs); div++) {
> +                       u64 denom = (u64)NSEC_PER_SEC * lpg_pre_divs[div] * (1 << 9);

" * (1 " part is redundant, you may shift left by 9, but see below.

> +                       u64 actual;
> +                       u64 ratio;
> +
> +                       if (nom < denom)
> +                               continue;
> +
> +                       ratio = div64_u64(nom, denom);

Instead of shifting left by 9, you may optimize below to count that in
the equations...

> +                       m = ilog2(ratio);
> +                       if (m > LPG_MAX_M)
> +                               m = LPG_MAX_M;

> +                       actual = DIV_ROUND_UP_ULL(denom * (1 << m), lpg_clk_rates[clk]);

...including this one.

So, I see room for improvement in the calculations.

> +                       error = period - actual;
> +                       if (error < best_err) {
> +                               best_err = error;
> +
> +                               best_div = div;
> +                               best_m = m;
> +                               best_clk = clk;
> +                               best_period = actual;
> +                       }
> +               }
> +       }
> +
> +       chan->clk = best_clk;
> +       chan->pre_div = best_div;
> +       chan->pre_div_exp = best_m;
> +       chan->period = best_period;
> +
> +       return 0;
> +}

...

> +       val = div64_u64(duty * lpg_clk_rates[chan->clk],
> +                       (u64)NSEC_PER_SEC * lpg_pre_divs[chan->pre_div] * (1 << chan->pre_div_exp));

For all code, just shift right directly, it makes code easier to read.

...

> +       regmap_bulk_write(lpg->map, chan->base + PWM_VALUE_REG, &val, sizeof(val));

In some cases the error is handled from regmap calls, in many it's not. Why?

...

> +       count = of_property_count_u32_elems(np, "qcom,dtest");
> +       if (count == -EINVAL) {
> +               return 0;

> +       } else if (count < 0) {

Redundant 'else'

> +               ret = count;

Do it other way around, i.e.

  ret = ...
  ...
  count = ret;

> +               goto err_malformed;
> +       } else if (count != lpg->data->num_channels * 2) {

Redundant 'else'.

> +               dev_err(lpg->dev, "qcom,dtest needs to be %d items\n",
> +                       lpg->data->num_channels * 2);
> +               return -EINVAL;
> +       }

...

> +       /* Only support oneshot or indefinite loops, due to limited pattern space */

one shot

> +       if (repeat != -1 && repeat != 1)

abs(repeat) != 1 ?

> +               return -EINVAL;

...

> +       /* LPG_RAMP_DURATION_REG is 9 bit */
> +       if (pattern[0].delta_t >= 512)

Then compare with bit value? BIT(9)?

> +               return -EINVAL;

...

> +       lpg_brightness_single_set(cdev, LED_FULL);

Isn't LED_FULL deprecated?

...

> +       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);

Confusing message for some of the error conditions.

> +               return -EINVAL;

Shadowed error code.

> +       }

...

> +       ret = of_property_read_u32(np, "color", &color);
> +       if (ret < 0 && ret != -EINVAL) {

Why the specific error code check?

> +               dev_err(lpg->dev, "failed to parse \"color\" of %pOF\n", np);
> +               return ret;
> +       }

...

> +       if (!of_property_read_string(np, "default-state", &state) &&
> +           !strcmp(state, "on"))

of_property_match_string()?

...

> +       bitmap_size = BITS_TO_BYTES(lpg->lut_size);
> +       lpg->lut_bitmap = devm_kzalloc(lpg->dev, bitmap_size, GFP_KERNEL);

devm_bitmap_zalloc()

> +       if (!lpg->lut_bitmap)
> +               return -ENOMEM;

...

> +               dev_err(&pdev->dev, "parent regmap unavailable\n");
> +               return -ENXIO;

return dev_err_probe(...);

...

> +       .pwm_9bit_mask = 3 << 4,

GENMASK()

...

> +       .pwm_9bit_mask = 3 << 4,

Ditto.

--
With Best Regards,
Andy Shevchenko
Uwe Kleine-König Feb. 2, 2022, 4:29 p.m. UTC | #3
Hello,

did you consider my earlier feedback "It would also be good if the PWM
code could live in drivers/pwm"?
(https://lore.kernel.org/r/20210505051958.e5lvwfxuo2skdu2q@pengutronix.de)

At least splitting in two patches would be good IMHO.

On Fri, Jan 28, 2022 at 04:54:29PM -0800, Bjorn Andersson wrote:
> The Light Pulse Generator (LPG) is a PWM-block found in a wide range of
> PMICs from Qualcomm. These PMICs typically comes with 1-8 LPG instances,
> with their output being routed to various other components, such as
> current sinks or GPIOs.
> 
> Each LPG instance can operate on fixed parameters or based on a shared
> lookup-table, altering the duty cycle over time. This provides the means
> for hardware assisted transitions of LED brightness.
> 
> A typical use case for the fixed parameter mode is to drive a PWM
> backlight control signal, the driver therefor allows each LPG instance
> to be exposed to the kernel either through the LED framework or the PWM
> framework.
> 
> A typical use case for the LED configuration is to drive RGB LEDs in
> smartphones etc, for which the driver support multiple channels to be
> ganged up to a MULTICOLOR LED. In this configuration the pattern
> generators will be synchronized, to allow for multi-color patterns.
> 
> Signed-off-by: Bjorn Andersson <bjorn.andersson@linaro.org>
> ---
> 
> Changes since v10:
> - Check for and reject pattern.delta_t greater than 9 bits
> - Write all 9 bits of LPG_RAMP_DURATION_REG
> 
>  drivers/leds/Kconfig             |    3 +
>  drivers/leds/Makefile            |    3 +
>  drivers/leds/rgb/Kconfig         |   13 +
>  drivers/leds/rgb/Makefile        |    3 +
>  drivers/leds/rgb/leds-qcom-lpg.c | 1306 ++++++++++++++++++++++++++++++
>  5 files changed, 1328 insertions(+)
>  create mode 100644 drivers/leds/rgb/Kconfig
>  create mode 100644 drivers/leds/rgb/Makefile
>  create mode 100644 drivers/leds/rgb/leds-qcom-lpg.c
> 
> diff --git a/drivers/leds/Kconfig b/drivers/leds/Kconfig
> index 6090e647daee..a49979f41eee 100644
> --- a/drivers/leds/Kconfig
> +++ b/drivers/leds/Kconfig
> @@ -869,6 +869,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"
> +
>  comment "LED Triggers"
>  source "drivers/leds/trigger/Kconfig"
>  
> diff --git a/drivers/leds/Makefile b/drivers/leds/Makefile
> index e58ecb36360f..4fd2f92cd198 100644
> --- a/drivers/leds/Makefile
> +++ b/drivers/leds/Makefile
> @@ -99,6 +99,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/
> +
>  # LED Triggers
>  obj-$(CONFIG_LEDS_TRIGGERS)		+= trigger/
>  
> diff --git a/drivers/leds/rgb/Kconfig b/drivers/leds/rgb/Kconfig
> new file mode 100644
> index 000000000000..20be3e11fe4a
> --- /dev/null
> +++ b/drivers/leds/rgb/Kconfig
> @@ -0,0 +1,13 @@
> +# SPDX-License-Identifier: GPL-2.0
> +
> +if LEDS_CLASS_MULTICOLOR
> +
> +config LEDS_QCOM_LPG
> +	tristate "LED support for Qualcomm LPG"
> +	depends on OF
> +	depends on SPMI
> +	help
> +	  This option enables support for the Light Pulse Generator found in a
> +	  wide variety of Qualcomm PMICs.
> +
> +endif # LEDS_CLASS_MULTICOLOR
> diff --git a/drivers/leds/rgb/Makefile b/drivers/leds/rgb/Makefile
> new file mode 100644
> index 000000000000..83114f44c4ea
> --- /dev/null
> +++ b/drivers/leds/rgb/Makefile
> @@ -0,0 +1,3 @@
> +# SPDX-License-Identifier: GPL-2.0
> +
> +obj-$(CONFIG_LEDS_QCOM_LPG)	+= leds-qcom-lpg.o
> diff --git a/drivers/leds/rgb/leds-qcom-lpg.c b/drivers/leds/rgb/leds-qcom-lpg.c
> new file mode 100644
> index 000000000000..06d5fca1d4b5
> --- /dev/null
> +++ b/drivers/leds/rgb/leds-qcom-lpg.c
> @@ -0,0 +1,1306 @@
> +// SPDX-License-Identifier: GPL-2.0-only
> +/*
> + * Copyright (c) 2017-2022 Linaro Ltd
> + * Copyright (c) 2010-2012, The Linux Foundation. All rights reserved.
> + */
> +#include <linux/bits.h>
> +#include <linux/led-class-multicolor.h>
> +#include <linux/module.h>
> +#include <linux/of.h>
> +#include <linux/of_device.h>
> +#include <linux/platform_device.h>
> +#include <linux/pwm.h>
> +#include <linux/regmap.h>
> +#include <linux/slab.h>
> +
> +#define LPG_PATTERN_CONFIG_REG	0x40
> +#define LPG_SIZE_CLK_REG	0x41
> +#define LPG_PREDIV_CLK_REG	0x42
> +#define PWM_TYPE_CONFIG_REG	0x43
> +#define PWM_VALUE_REG		0x44
> +#define PWM_ENABLE_CONTROL_REG	0x46
> +#define PWM_SYNC_REG		0x47
> +#define LPG_RAMP_DURATION_REG	0x50
> +#define LPG_HI_PAUSE_REG	0x52
> +#define LPG_LO_PAUSE_REG	0x54
> +#define LPG_HI_IDX_REG		0x56
> +#define LPG_LO_IDX_REG		0x57
> +#define PWM_SEC_ACCESS_REG	0xd0
> +#define PWM_DTEST_REG(x)	(0xe2 + (x) - 1)
> +
> +#define TRI_LED_SRC_SEL		0x45
> +#define TRI_LED_EN_CTL		0x46
> +#define TRI_LED_ATC_CTL		0x47
> +
> +#define LPG_LUT_REG(x)		(0x40 + (x) * 2)
> +#define RAMP_CONTROL_REG	0xc8
> +
> +#define LPG_RESOLUTION		512
> +#define LPG_MAX_M		7
> +
> +struct lpg_channel;
> +struct lpg_data;
> +
> +/**
> + * struct lpg - LPG device context
> + * @dev:	struct device for LPG device
> + * @map:	regmap for register access
> + * @pwm:	PWM-chip object, if operating in PWM mode
> + * @data:	reference to version specific data
> + * @lut_base:	base address of the LUT block (optional)
> + * @lut_size:	number of entries in the LUT block
> + * @lut_bitmap:	allocation bitmap for LUT entries
> + * @triled_base: base address of the TRILED block (optional)
> + * @triled_src:	power-source for the TRILED
> + * @triled_has_atc_ctl:	true if there is TRI_LED_ATC_CTL register
> + * @triled_has_src_sel:	true if there is TRI_LED_SRC_SEL register
> + * @channels:	list of PWM channels
> + * @num_channels: number of @channels
> + */
> +struct lpg {
> +	struct device *dev;
> +	struct regmap *map;
> +
> +	struct pwm_chip pwm;
> +
> +	const struct lpg_data *data;
> +
> +	u32 lut_base;
> +	u32 lut_size;
> +	unsigned long *lut_bitmap;
> +
> +	u32 triled_base;
> +	u32 triled_src;
> +	bool triled_has_atc_ctl;
> +	bool triled_has_src_sel;
> +
> +	struct lpg_channel *channels;
> +	unsigned int num_channels;
> +};
> +
> +/**
> + * struct lpg_channel - per channel data
> + * @lpg:	reference to parent lpg
> + * @base:	base address of the PWM channel
> + * @triled_mask: mask in TRILED to enable this channel
> + * @lut_mask:	mask in LUT to start pattern generator for this channel
> + * @in_use:	channel is exposed to LED framework
> + * @color:	color of the LED attached to this channel
> + * @dtest_line:	DTEST line for output, or 0 if disabled
> + * @dtest_value: DTEST line configuration
> + * @pwm_value:	duty (in microseconds) of the generated pulses, overridden by LUT
> + * @enabled:	output enabled?
> + * @period:	period (in nanoseconds) of the generated pulses
> + * @clk:	base frequency of the clock generator
> + * @pre_div:	divider of @clk
> + * @pre_div_exp: exponential divider of @clk
> + * @ramp_enabled: duty cycle is driven by iterating over lookup table
> + * @ramp_ping_pong: reverse through pattern, rather than wrapping to start
> + * @ramp_oneshot: perform only a single pass over the pattern
> + * @ramp_reverse: iterate over pattern backwards
> + * @ramp_tick_ms: length (in milliseconds) of one step in the pattern
> + * @ramp_lo_pause_ms: pause (in milliseconds) before iterating over pattern
> + * @ramp_hi_pause_ms: pause (in milliseconds) after iterating over pattern
> + * @pattern_lo_idx: start index of associated pattern
> + * @pattern_hi_idx: last index of associated pattern
> + */
> +struct lpg_channel {
> +	struct lpg *lpg;
> +
> +	u32 base;
> +	unsigned int triled_mask;
> +	unsigned int lut_mask;
> +
> +	bool in_use;
> +
> +	int color;
> +
> +	u32 dtest_line;
> +	u32 dtest_value;
> +
> +	u16 pwm_value;
> +	bool enabled;
> +
> +	u64 period;
> +	unsigned int clk;
> +	unsigned int pre_div;
> +	unsigned int pre_div_exp;
> +
> +	bool ramp_enabled;
> +	bool ramp_ping_pong;
> +	bool ramp_oneshot;
> +	bool ramp_reverse;
> +	unsigned short ramp_tick_ms;
> +	unsigned long ramp_lo_pause_ms;
> +	unsigned long ramp_hi_pause_ms;
> +
> +	unsigned int pattern_lo_idx;
> +	unsigned int pattern_hi_idx;
> +};
> +
> +/**
> + * struct lpg_led - logical LED object
> + * @lpg:		lpg context reference
> + * @cdev:		LED class device
> + * @mcdev:		Multicolor LED class device
> + * @num_channels:	number of @channels
> + * @channels:		list of channels associated with the LED
> + */
> +struct lpg_led {
> +	struct lpg *lpg;
> +
> +	struct led_classdev cdev;
> +	struct led_classdev_mc mcdev;
> +
> +	unsigned int num_channels;
> +	struct lpg_channel *channels[];
> +};
> +
> +/**
> + * struct lpg_channel_data - per channel initialization data
> + * @base:		base address for PWM channel registers
> + * @triled_mask:	bitmask for controlling this channel in TRILED
> + */
> +struct lpg_channel_data {
> +	unsigned int base;
> +	u8 triled_mask;
> +};
> +
> +/**
> + * 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
> + * @triled_has_atc_ctl:	true if there is TRI_LED_ATC_CTL register
> + * @triled_has_src_sel:	true if there is TRI_LED_SRC_SEL register
> + * @pwm_9bit_mask:	bitmask for switching from 6bit to 9bit pwm
> + * @num_channels:	number of channels in LPG
> + * @channels:		list of channel initialization data
> + */
> +struct lpg_data {
> +	unsigned int lut_base;
> +	unsigned int lut_size;
> +	unsigned int triled_base;
> +	bool triled_has_atc_ctl;
> +	bool triled_has_src_sel;
> +	unsigned int pwm_9bit_mask;
> +	int num_channels;
> +	const struct lpg_channel_data *channels;
> +};
> +
> +static int triled_set(struct lpg *lpg, unsigned int mask, unsigned int enable)
> +{
> +	/* Skip if we don't have a triled block */
> +	if (!lpg->triled_base)
> +		return 0;
> +
> +	return regmap_update_bits(lpg->map, lpg->triled_base + TRI_LED_EN_CTL,
> +				  mask, enable);
> +}
> +
> +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, sizeof(val));
> +	}
> +
> +	bitmap_set(lpg->lut_bitmap, idx, len);
> +
> +	*lo_idx = idx;
> +	*hi_idx = idx + len - 1;
> +
> +	return 0;
> +}
> +
> +static void lpg_lut_free(struct lpg *lpg, unsigned int lo_idx, unsigned int hi_idx)
> +{
> +	int len;
> +
> +	if (lo_idx == hi_idx)
> +		return;
> +
> +	len = hi_idx - lo_idx + 1;
> +	bitmap_clear(lpg->lut_bitmap, lo_idx, len);
> +}
> +
> +static int lpg_lut_sync(struct lpg *lpg, unsigned int mask)
> +{
> +	return regmap_write(lpg->map, lpg->lut_base + RAMP_CONTROL_REG, mask);
> +}
> +
> +static const unsigned int lpg_clk_rates[] = {1024, 32768, 19200000};
> +static const unsigned int lpg_pre_divs[] = {1, 3, 5, 6};
> +
> +static int lpg_calc_freq(struct lpg_channel *chan, uint64_t period)
> +{
> +	unsigned int clk, best_clk = 0;
> +	unsigned int div, best_div = 0;
> +	unsigned int m, best_m = 0;
> +	unsigned int error;
> +	unsigned int best_err = UINT_MAX;
> +	u64 best_period = 0;
> +
> +	/*
> +	 * The PWM period is determined by:
> +	 *
> +	 *          resolution * pre_div * 2^M
> +	 * period = --------------------------
> +	 *                   refclk
> +	 *
> +	 * With resolution fixed at 2^9 bits, pre_div = {1, 3, 5, 6} and
> +	 * M = [0..7].
> +	 *
> +	 * This allows for periods between 27uS and 384s, as the PWM framework
> +	 * wants a period of equal or lower length than requested, reject
> +	 * anything below 27uS.
> +	 */
> +	if (period <= (u64)NSEC_PER_SEC * LPG_RESOLUTION / 19200000)

u64 divisions must not be done by / in the kernel. Also I wonder if the
following would be more correct (though with the same semantic):

	if (period < DIV64_U64_ROUND_UP((u64)NSEC_PER_SEC * LPG_RESOLUTION, 19200000))


> +		return -EINVAL;
> +
> +	/* Limit period to largest possible value, to avoid overflows */
> +	if (period > (u64)NSEC_PER_SEC * LPG_RESOLUTION * 6 * (1 << LPG_MAX_M) / 1024)
> +		period = (u64)NSEC_PER_SEC * LPG_RESOLUTION * 6 * (1 << LPG_MAX_M) / 2014;
> +
> +	/*
> +	 * Search for the pre_div, clk and M by solving the rewritten formula
> +	 * for each clk and pre_div value:
> +	 *
> +	 *                       period * clk
> +	 * M = log2 -------------------------------------
> +	 *           NSEC_PER_SEC * pre_div * resolution
> +	 */
> +	for (clk = 0; clk < ARRAY_SIZE(lpg_clk_rates); clk++) {
> +		u64 nom = period * lpg_clk_rates[clk];
> +
> +		for (div = 0; div < ARRAY_SIZE(lpg_pre_divs); div++) {
> +			u64 denom = (u64)NSEC_PER_SEC * lpg_pre_divs[div] * (1 << 9);
> +			u64 actual;
> +			u64 ratio;
> +
> +			if (nom < denom)
> +				continue;
> +
> +			ratio = div64_u64(nom, denom);
> +			m = ilog2(ratio);
> +			if (m > LPG_MAX_M)
> +				m = LPG_MAX_M;
> +
> +			actual = DIV_ROUND_UP_ULL(denom * (1 << m), lpg_clk_rates[clk]);
> +
> +			error = period - actual;

This looks good, though I didn't revalidate the calculation (e.g. to
convince myself that error is always >= 0)

> +			if (error < best_err) {
> +				best_err = error;
> +
> +				best_div = div;
> +				best_m = m;
> +				best_clk = clk;
> +				best_period = actual;
> +			}
> +		}
> +	}
> +
> +	chan->clk = best_clk;
> +	chan->pre_div = best_div;
> +	chan->pre_div_exp = best_m;
> +	chan->period = best_period;
> +
> +	return 0;
> +}
> +
> +static void lpg_calc_duty(struct lpg_channel *chan, uint64_t duty)
> +{
> +	unsigned int max = LPG_RESOLUTION - 1;
> +	unsigned int val;
> +
> +	val = div64_u64(duty * lpg_clk_rates[chan->clk],
> +			(u64)NSEC_PER_SEC * lpg_pre_divs[chan->pre_div] * (1 << chan->pre_div_exp));
> +
> +	chan->pwm_value = min(val, max);
> +}
> +
> +static void lpg_apply_freq(struct lpg_channel *chan)
> +{
> +	unsigned long val;
> +	struct lpg *lpg = chan->lpg;
> +
> +	if (!chan->enabled)
> +		return;
> +
> +	/* Clock register values are off-by-one from lpg_clk_table */
> +	val = chan->clk + 1;
> +
> +	/* Enable 9bit resolution */
> +	val |= lpg->data->pwm_9bit_mask;
> +
> +	regmap_write(lpg->map, chan->base + LPG_SIZE_CLK_REG, val);
> +
> +	val = chan->pre_div << 5 | chan->pre_div_exp;
> +	regmap_write(lpg->map, chan->base + LPG_PREDIV_CLK_REG, val);
> +}
> +
> +#define LPG_ENABLE_GLITCH_REMOVAL	BIT(5)
> +
> +static void lpg_enable_glitch(struct lpg_channel *chan)
> +{
> +	struct lpg *lpg = chan->lpg;
> +
> +	regmap_update_bits(lpg->map, chan->base + PWM_TYPE_CONFIG_REG,
> +			   LPG_ENABLE_GLITCH_REMOVAL, 0);
> +}
> +
> +static void lpg_disable_glitch(struct lpg_channel *chan)
> +{
> +	struct lpg *lpg = chan->lpg;
> +
> +	regmap_update_bits(lpg->map, chan->base + PWM_TYPE_CONFIG_REG,
> +			   LPG_ENABLE_GLITCH_REMOVAL,
> +			   LPG_ENABLE_GLITCH_REMOVAL);
> +}
> +
> +static void lpg_apply_pwm_value(struct lpg_channel *chan)
> +{
> +	struct lpg *lpg = chan->lpg;
> +	u16 val = chan->pwm_value;
> +
> +	if (!chan->enabled)
> +		return;
> +
> +	regmap_bulk_write(lpg->map, chan->base + PWM_VALUE_REG, &val, sizeof(val));
> +}
> +
> +#define LPG_PATTERN_CONFIG_LO_TO_HI	BIT(4)
> +#define LPG_PATTERN_CONFIG_REPEAT	BIT(3)
> +#define LPG_PATTERN_CONFIG_TOGGLE	BIT(2)
> +#define LPG_PATTERN_CONFIG_PAUSE_HI	BIT(1)
> +#define LPG_PATTERN_CONFIG_PAUSE_LO	BIT(0)
> +
> +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 conf = 0;
> +	unsigned int lo_idx = chan->pattern_lo_idx;
> +	unsigned int hi_idx = chan->pattern_hi_idx;
> +	u16 step = chan->ramp_tick_ms;
> +
> +	if (!chan->ramp_enabled || chan->pattern_lo_idx == chan->pattern_hi_idx)
> +		return;
> +
> +	hi_pause = DIV_ROUND_UP(chan->ramp_hi_pause_ms, step);
> +	lo_pause = DIV_ROUND_UP(chan->ramp_lo_pause_ms, step);
> +
> +	if (!chan->ramp_reverse)
> +		conf |= LPG_PATTERN_CONFIG_LO_TO_HI;
> +	if (!chan->ramp_oneshot)
> +		conf |= LPG_PATTERN_CONFIG_REPEAT;
> +	if (chan->ramp_ping_pong)
> +		conf |= LPG_PATTERN_CONFIG_TOGGLE;
> +	if (chan->ramp_hi_pause_ms)
> +		conf |= LPG_PATTERN_CONFIG_PAUSE_HI;
> +	if (chan->ramp_lo_pause_ms)
> +		conf |= LPG_PATTERN_CONFIG_PAUSE_LO;
> +
> +	regmap_write(lpg->map, chan->base + LPG_PATTERN_CONFIG_REG, conf);
> +	regmap_write(lpg->map, chan->base + LPG_HI_IDX_REG, hi_idx);
> +	regmap_write(lpg->map, chan->base + LPG_LO_IDX_REG, lo_idx);
> +
> +	regmap_bulk_write(lpg->map, chan->base + LPG_RAMP_DURATION_REG, &step, sizeof(step));
> +	regmap_write(lpg->map, chan->base + LPG_HI_PAUSE_REG, hi_pause);
> +	regmap_write(lpg->map, chan->base + LPG_LO_PAUSE_REG, lo_pause);
> +}
> +
> +#define LPG_ENABLE_CONTROL_OUTPUT		BIT(7)
> +#define LPG_ENABLE_CONTROL_BUFFER_TRISTATE	BIT(5)
> +#define LPG_ENABLE_CONTROL_SRC_PWM		BIT(2)
> +#define LPG_ENABLE_CONTROL_RAMP_GEN		BIT(1)
> +
> +static void lpg_apply_control(struct lpg_channel *chan)
> +{
> +	unsigned int ctrl;
> +	struct lpg *lpg = chan->lpg;
> +
> +	ctrl = LPG_ENABLE_CONTROL_BUFFER_TRISTATE;
> +
> +	if (chan->enabled)
> +		ctrl |= LPG_ENABLE_CONTROL_OUTPUT;
> +
> +	if (chan->pattern_lo_idx != chan->pattern_hi_idx)
> +		ctrl |= LPG_ENABLE_CONTROL_RAMP_GEN;
> +	else
> +		ctrl |= LPG_ENABLE_CONTROL_SRC_PWM;
> +
> +	regmap_write(lpg->map, chan->base + PWM_ENABLE_CONTROL_REG, ctrl);
> +
> +	/*
> +	 * Due to LPG hardware bug, in the PWM mode, having enabled PWM,
> +	 * We have to write PWM values one more time.
> +	 */
> +	if (chan->enabled)
> +		lpg_apply_pwm_value(chan);
> +}
> +
> +#define LPG_SYNC_PWM	BIT(0)
> +
> +static void lpg_apply_sync(struct lpg_channel *chan)
> +{
> +	struct lpg *lpg = chan->lpg;
> +
> +	regmap_write(lpg->map, chan->base + PWM_SYNC_REG, LPG_SYNC_PWM);
> +}
> +
> +static int lpg_parse_dtest(struct lpg *lpg)
> +{
> +	struct lpg_channel *chan;
> +	struct device_node *np = lpg->dev->of_node;
> +	int count;
> +	int ret;
> +	int i;
> +
> +	count = of_property_count_u32_elems(np, "qcom,dtest");
> +	if (count == -EINVAL) {
> +		return 0;
> +	} else if (count < 0) {
> +		ret = count;
> +		goto err_malformed;
> +	} else if (count != lpg->data->num_channels * 2) {
> +		dev_err(lpg->dev, "qcom,dtest needs to be %d items\n",
> +			lpg->data->num_channels * 2);
> +		return -EINVAL;
> +	}
> +
> +	for (i = 0; i < lpg->data->num_channels; i++) {
> +		chan = &lpg->channels[i];
> +
> +		ret = of_property_read_u32_index(np, "qcom,dtest", i * 2,
> +						 &chan->dtest_line);
> +		if (ret)
> +			goto err_malformed;
> +
> +		ret = of_property_read_u32_index(np, "qcom,dtest", i * 2 + 1,
> +						 &chan->dtest_value);
> +		if (ret)
> +			goto err_malformed;
> +	}
> +
> +	return 0;
> +
> +err_malformed:
> +	dev_err(lpg->dev, "malformed qcom,dtest\n");
> +	return ret;
> +}
> +
> +static void lpg_apply_dtest(struct lpg_channel *chan)
> +{
> +	struct lpg *lpg = chan->lpg;
> +
> +	if (!chan->dtest_line)
> +		return;
> +
> +	regmap_write(lpg->map, chan->base + PWM_SEC_ACCESS_REG, 0xa5);
> +	regmap_write(lpg->map, chan->base + PWM_DTEST_REG(chan->dtest_line),
> +		     chan->dtest_value);
> +}
> +
> +static void lpg_apply(struct lpg_channel *chan)
> +{
> +	lpg_disable_glitch(chan);
> +	lpg_apply_freq(chan);
> +	lpg_apply_pwm_value(chan);
> +	lpg_apply_control(chan);
> +	lpg_apply_sync(chan);
> +	lpg_apply_lut_control(chan);
> +	lpg_enable_glitch(chan);
> +}
> +
> +static void lpg_brightness_set(struct lpg_led *led, struct led_classdev *cdev,
> +			       struct mc_subled *subleds)
> +{
> +	enum led_brightness brightness;
> +	struct lpg_channel *chan;
> +	unsigned int triled_enabled = 0;
> +	unsigned int triled_mask = 0;
> +	unsigned int lut_mask = 0;
> +	unsigned int duty;
> +	struct lpg *lpg = led->lpg;
> +	int i;
> +
> +	for (i = 0; i < led->num_channels; i++) {
> +		chan = led->channels[i];
> +		brightness = subleds[i].brightness;
> +
> +		if (brightness == LED_OFF) {
> +			chan->enabled = false;
> +			chan->ramp_enabled = false;
> +		} else if (chan->pattern_lo_idx != chan->pattern_hi_idx) {
> +			lpg_calc_freq(chan, NSEC_PER_MSEC);
> +
> +			chan->enabled = true;
> +			chan->ramp_enabled = true;
> +
> +			lut_mask |= chan->lut_mask;
> +			triled_enabled |= chan->triled_mask;
> +		} else {
> +			lpg_calc_freq(chan, NSEC_PER_MSEC);
> +
> +			duty = div_u64(brightness * chan->period, cdev->max_brightness);
> +			lpg_calc_duty(chan, duty);
> +			chan->enabled = true;
> +			chan->ramp_enabled = false;
> +
> +			triled_enabled |= chan->triled_mask;
> +		}
> +
> +		triled_mask |= chan->triled_mask;
> +
> +		lpg_apply(chan);
> +	}
> +
> +	/* Toggle triled lines */
> +	if (triled_mask)
> +		triled_set(lpg, triled_mask, triled_enabled);
> +
> +	/* Trigger start of ramp generator(s) */
> +	if (lut_mask)
> +		lpg_lut_sync(lpg, lut_mask);
> +}
> +
> +static void lpg_brightness_single_set(struct led_classdev *cdev,
> +				      enum led_brightness value)
> +{
> +	struct lpg_led *led = container_of(cdev, struct lpg_led, cdev);
> +	struct mc_subled info;
> +
> +	info.brightness = value;
> +	lpg_brightness_set(led, cdev, &info);
> +}
> +
> +static void lpg_brightness_mc_set(struct led_classdev *cdev,
> +				  enum led_brightness value)
> +{
> +	struct led_classdev_mc *mc = lcdev_to_mccdev(cdev);
> +	struct lpg_led *led = container_of(mc, struct lpg_led, mcdev);
> +
> +	led_mc_calc_color_components(mc, value);
> +	lpg_brightness_set(led, cdev, mc->subled_info);
> +}
> +
> +static int lpg_blink_set(struct lpg_led *led,
> +			 unsigned long *delay_on, unsigned long *delay_off)
> +{
> +	struct lpg_channel *chan;
> +	unsigned int period;
> +	unsigned int triled_mask = 0;
> +	struct lpg *lpg = led->lpg;
> +	u64 duty;
> +	int i;
> +
> +	if (!*delay_on && !*delay_off) {
> +		*delay_on = 500;
> +		*delay_off = 500;
> +	}
> +
> +	duty = *delay_on * NSEC_PER_MSEC;
> +	period = (*delay_on + *delay_off) * NSEC_PER_MSEC;
> +
> +	for (i = 0; i < led->num_channels; i++) {
> +		chan = led->channels[i];
> +
> +		lpg_calc_freq(chan, period);
> +		lpg_calc_duty(chan, duty);
> +
> +		chan->enabled = true;
> +		chan->ramp_enabled = false;
> +
> +		triled_mask |= chan->triled_mask;
> +
> +		lpg_apply(chan);
> +	}
> +
> +	/* Enable triled lines */
> +	triled_set(lpg, triled_mask, triled_mask);
> +
> +	chan = led->channels[0];
> +	duty = div_u64(chan->pwm_value * chan->period, LPG_RESOLUTION);
> +	*delay_on = div_u64(duty, NSEC_PER_MSEC);
> +	*delay_off = div_u64(chan->period - duty, NSEC_PER_MSEC);
> +
> +	return 0;
> +}
> +
> +static int lpg_blink_single_set(struct led_classdev *cdev,
> +				unsigned long *delay_on, unsigned long *delay_off)
> +{
> +	struct lpg_led *led = container_of(cdev, struct lpg_led, cdev);
> +
> +	return lpg_blink_set(led, delay_on, delay_off);
> +}
> +
> +static int lpg_blink_mc_set(struct led_classdev *cdev,
> +			    unsigned long *delay_on, unsigned long *delay_off)
> +{
> +	struct led_classdev_mc *mc = lcdev_to_mccdev(cdev);
> +	struct lpg_led *led = container_of(mc, struct lpg_led, mcdev);
> +
> +	return lpg_blink_set(led, delay_on, delay_off);
> +}
> +
> +static int lpg_pattern_set(struct lpg_led *led, struct led_pattern *pattern,
> +			   u32 len, int repeat)
> +{
> +	struct lpg_channel *chan;
> +	struct lpg *lpg = led->lpg;
> +	unsigned int hi_pause;
> +	unsigned int lo_pause;
> +	unsigned int lo_idx;
> +	unsigned int hi_idx;
> +	bool ping_pong = true;
> +	int brightness_a;
> +	int brightness_b;
> +	int ret;
> +	int i;
> +
> +	/* Only support oneshot or indefinite loops, due to limited pattern space */
> +	if (repeat != -1 && repeat != 1)
> +		return -EINVAL;
> +
> +	/* LPG_RAMP_DURATION_REG is 9 bit */
> +	if (pattern[0].delta_t >= 512)
> +		return -EINVAL;
> +
> +	/*
> +	 * The LPG plays patterns with at a fixed pace, a "low pause" can be
> +	 * performed before the pattern and a "high pause" after. In order to
> +	 * save space the pattern can be played in "ping pong" mode, in which
> +	 * the pattern is first played forward, then "high pause" is applied,
> +	 * then the pattern is played backwards and finally the "low pause" is
> +	 * applied.
> +	 *
> +	 * The delta_t of the first entry is used to determine the pace of the
> +	 * pattern.
> +	 *
> +	 * If the specified pattern is a palindrome the ping pong mode is
> +	 * enabled. In this scenario the delta_t of the last entry determines
> +	 * the "low pause" time and the delta_t of the middle entry (i.e. the
> +	 * last in the programmed pattern) determines the "high pause". If the
> +	 * pattern consists of an odd number of values, no "high pause" is
> +	 * used.
> +	 *
> +	 * When ping pong mode is not selected, the delta_t of the last entry
> +	 * is used as "high pause". No "low pause" is used.
> +	 *
> +	 * delta_t of any other members of the pattern is ignored.
> +	 */
> +
> +	/* Detect palindromes and use "ping pong" to reduce LUT usage */
> +	for (i = 0; i < len / 2; i++) {
> +		brightness_a = pattern[i].brightness;
> +		brightness_b = pattern[len - i - 1].brightness;
> +
> +		if (brightness_a != brightness_b) {
> +			ping_pong = false;
> +			break;
> +		}
> +	}
> +
> +	if (ping_pong) {
> +		if (len % 2)
> +			hi_pause = 0;
> +		else
> +			hi_pause = pattern[(len + 1) / 2].delta_t;
> +		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)
> +		return ret;
> +
> +	for (i = 0; i < led->num_channels; i++) {
> +		chan = led->channels[i];
> +
> +		chan->ramp_tick_ms = pattern[0].delta_t;
> +		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;
> +	}
> +
> +	return 0;
> +}
> +
> +static int lpg_pattern_single_set(struct led_classdev *cdev,
> +				  struct led_pattern *pattern, u32 len,
> +				  int repeat)
> +{
> +	struct lpg_led *led = container_of(cdev, struct lpg_led, cdev);
> +	int ret;
> +
> +	ret = lpg_pattern_set(led, pattern, len, repeat);
> +	if (ret < 0)
> +		return ret;
> +
> +	lpg_brightness_single_set(cdev, LED_FULL);
> +
> +	return 0;
> +}
> +
> +static int lpg_pattern_mc_set(struct led_classdev *cdev,
> +			      struct led_pattern *pattern, u32 len,
> +			      int repeat)
> +{
> +	struct led_classdev_mc *mc = lcdev_to_mccdev(cdev);
> +	struct lpg_led *led = container_of(mc, struct lpg_led, mcdev);
> +	int ret;
> +
> +	ret = lpg_pattern_set(led, pattern, len, repeat);
> +	if (ret < 0)
> +		return ret;
> +
> +	led_mc_calc_color_components(mc, LED_FULL);
> +	lpg_brightness_set(led, cdev, mc->subled_info);
> +
> +	return 0;
> +}
> +
> +static int lpg_pattern_clear(struct lpg_led *led)
> +{
> +	struct lpg_channel *chan;
> +	struct lpg *lpg = led->lpg;
> +	int i;
> +
> +	chan = led->channels[0];
> +	lpg_lut_free(lpg, chan->pattern_lo_idx, chan->pattern_hi_idx);
> +
> +	for (i = 0; i < led->num_channels; i++) {
> +		chan = led->channels[i];
> +		chan->pattern_lo_idx = 0;
> +		chan->pattern_hi_idx = 0;
> +	}
> +
> +	return 0;
> +}
> +
> +static int lpg_pattern_single_clear(struct led_classdev *cdev)
> +{
> +	struct lpg_led *led = container_of(cdev, struct lpg_led, cdev);
> +
> +	return lpg_pattern_clear(led);
> +}
> +
> +static int lpg_pattern_mc_clear(struct led_classdev *cdev)
> +{
> +	struct led_classdev_mc *mc = lcdev_to_mccdev(cdev);
> +	struct lpg_led *led = container_of(mc, struct lpg_led, mcdev);
> +
> +	return lpg_pattern_clear(led);
> +}
> +
> +static int lpg_pwm_request(struct pwm_chip *chip, struct pwm_device *pwm)
> +{
> +	struct lpg *lpg = container_of(chip, struct lpg, pwm);
> +	struct lpg_channel *chan = &lpg->channels[pwm->hwpwm];
> +
> +	return chan->in_use ? -EBUSY : 0;
> +}
> +
> +/*
> + * Limitations:
> + * - Updating both duty and period is not done atomically, so the output signal
> + *   will momentarily be a mix of the settings.

Is the PWM well-behaved? (i.e. does it emit the inactive level when
disabled?) Does it complete a period before switching to the new
setting?

Did you test with PWM_DEBUG enabled?

> + */
> +static int lpg_pwm_apply(struct pwm_chip *chip, struct pwm_device *pwm,
> +			 const struct pwm_state *state)
> +{
> +	struct lpg *lpg = container_of(chip, struct lpg, pwm);
> +	struct lpg_channel *chan = &lpg->channels[pwm->hwpwm];
> +	int ret;
> +
> +	if (state->polarity != PWM_POLARITY_NORMAL)
> +		return -EINVAL;
> +
> +	ret = lpg_calc_freq(chan, state->period);
> +	if (ret < 0)
> +		return ret;
> +
> +	lpg_calc_duty(chan, state->duty_cycle);
> +	chan->enabled = state->enabled;
> +
> +	lpg_apply(chan);
> +
> +	triled_set(lpg, chan->triled_mask, chan->enabled ? chan->triled_mask : 0);
> +
> +	return 0;

Would it make sense to skip the calculation if state->enabled is false?

> +}
> +
> +static void lpg_pwm_get_state(struct pwm_chip *chip, struct pwm_device *pwm,
> +			      struct pwm_state *state)
> +{
> +	struct lpg *lpg = container_of(chip, struct lpg, pwm);
> +	struct lpg_channel *chan = &lpg->channels[pwm->hwpwm];
> +	u64 duty = DIV_ROUND_UP_ULL(chan->pwm_value * chan->period, LPG_RESOLUTION - 1);
> +
> +	state->period = chan->period;
> +	state->duty_cycle = duty;
> +	state->polarity = PWM_POLARITY_NORMAL;
> +	state->enabled = chan->enabled;

This doesn't work if .get_state() is called before .apply() was called,
does it?

> +}
> +
> +static const struct pwm_ops lpg_pwm_ops = {
> +	.request = lpg_pwm_request,
> +	.apply = lpg_pwm_apply,
> +	.get_state = lpg_pwm_get_state,
> +	.owner = THIS_MODULE,
> +};
> +
> +static int lpg_add_pwm(struct lpg *lpg)
> +{
> +	int ret;
> +
> +	lpg->pwm.base = -1;

I already asked in May to drop this ...

> +	lpg->pwm.dev = lpg->dev;
> +	lpg->pwm.npwm = lpg->num_channels;
> +	lpg->pwm.ops = &lpg_pwm_ops;
> +

Best regards
Uwe
Bjorn Andersson Feb. 2, 2022, 8:05 p.m. UTC | #4
On Wed 02 Feb 03:18 PST 2022, Marijn Suijten wrote:

> On 2022-01-28 16:54:29, Bjorn Andersson wrote:
> > The Light Pulse Generator (LPG) is a PWM-block found in a wide range of
> > PMICs from Qualcomm. These PMICs typically comes with 1-8 LPG instances,
> > with their output being routed to various other components, such as
> > current sinks or GPIOs.
> > 
> > Each LPG instance can operate on fixed parameters or based on a shared
> > lookup-table, altering the duty cycle over time. This provides the means
> > for hardware assisted transitions of LED brightness.
> > 
> > A typical use case for the fixed parameter mode is to drive a PWM
> > backlight control signal, the driver therefor allows each LPG instance
> > to be exposed to the kernel either through the LED framework or the PWM
> > framework.
> > 
> > A typical use case for the LED configuration is to drive RGB LEDs in
> > smartphones etc, for which the driver support multiple channels to be
> > ganged up to a MULTICOLOR LED. In this configuration the pattern
> > generators will be synchronized, to allow for multi-color patterns.
> > 
> > Signed-off-by: Bjorn Andersson <bjorn.andersson@linaro.org>
> 
> Reviewed-by: Marijn Suijten <marijn.suijten@somainline.org>
> 
> There may still be some things to improve based on whether lo_pause
> works in non-ping-pong mode - to alleviate the requirement for the first
> delta_t to be at most 512ms - but this patch should not be delayed much
> longer and that's perhaps for a followup patch.  Same for my request for
> documentation and examples which at the same time serve as some form of
> tests to see if everything works as desired.
> 

I've been considering lopause to be the value before we start the
pattern, but I think you're right in that it denotes how long we should
hold the first value.

So I think it might make sense in the predefined "<value> <delay> <value>
<delay>" scheme to use first <delay> as to calculate lo-pause. I think
it has to be calculated, because the first value will iiuc be held
for (lopause + 1) * delay ms.

> I also vaguely remember other (downstream) drivers to support more than
> 512ms per step by (drastically?) changing PWM period, but not sure how
> that worked again nor if it was reliable.
> 

Thinking about it again, while 512 is the 9th bit, we should be able to
represent [0..1023] with 9 bits...

Regards,
Bjorn

> - Marijn
> 
> > ---
> > 
> > Changes since v10:
> > - Check for and reject pattern.delta_t greater than 9 bits
> > - Write all 9 bits of LPG_RAMP_DURATION_REG
> > 
> >  drivers/leds/Kconfig             |    3 +
> >  drivers/leds/Makefile            |    3 +
> >  drivers/leds/rgb/Kconfig         |   13 +
> >  drivers/leds/rgb/Makefile        |    3 +
> >  drivers/leds/rgb/leds-qcom-lpg.c | 1306 ++++++++++++++++++++++++++++++
> >  5 files changed, 1328 insertions(+)
> >  create mode 100644 drivers/leds/rgb/Kconfig
> >  create mode 100644 drivers/leds/rgb/Makefile
> >  create mode 100644 drivers/leds/rgb/leds-qcom-lpg.c
> > 
> > diff --git a/drivers/leds/Kconfig b/drivers/leds/Kconfig
> > index 6090e647daee..a49979f41eee 100644
> > --- a/drivers/leds/Kconfig
> > +++ b/drivers/leds/Kconfig
> > @@ -869,6 +869,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"
> > +
> >  comment "LED Triggers"
> >  source "drivers/leds/trigger/Kconfig"
> >  
> > diff --git a/drivers/leds/Makefile b/drivers/leds/Makefile
> > index e58ecb36360f..4fd2f92cd198 100644
> > --- a/drivers/leds/Makefile
> > +++ b/drivers/leds/Makefile
> > @@ -99,6 +99,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/
> > +
> >  # LED Triggers
> >  obj-$(CONFIG_LEDS_TRIGGERS)		+= trigger/
> >  
> > diff --git a/drivers/leds/rgb/Kconfig b/drivers/leds/rgb/Kconfig
> > new file mode 100644
> > index 000000000000..20be3e11fe4a
> > --- /dev/null
> > +++ b/drivers/leds/rgb/Kconfig
> > @@ -0,0 +1,13 @@
> > +# SPDX-License-Identifier: GPL-2.0
> > +
> > +if LEDS_CLASS_MULTICOLOR
> > +
> > +config LEDS_QCOM_LPG
> > +	tristate "LED support for Qualcomm LPG"
> > +	depends on OF
> > +	depends on SPMI
> > +	help
> > +	  This option enables support for the Light Pulse Generator found in a
> > +	  wide variety of Qualcomm PMICs.
> > +
> > +endif # LEDS_CLASS_MULTICOLOR
> > diff --git a/drivers/leds/rgb/Makefile b/drivers/leds/rgb/Makefile
> > new file mode 100644
> > index 000000000000..83114f44c4ea
> > --- /dev/null
> > +++ b/drivers/leds/rgb/Makefile
> > @@ -0,0 +1,3 @@
> > +# SPDX-License-Identifier: GPL-2.0
> > +
> > +obj-$(CONFIG_LEDS_QCOM_LPG)	+= leds-qcom-lpg.o
> > diff --git a/drivers/leds/rgb/leds-qcom-lpg.c b/drivers/leds/rgb/leds-qcom-lpg.c
> > new file mode 100644
> > index 000000000000..06d5fca1d4b5
> > --- /dev/null
> > +++ b/drivers/leds/rgb/leds-qcom-lpg.c
> > @@ -0,0 +1,1306 @@
> > +// SPDX-License-Identifier: GPL-2.0-only
> > +/*
> > + * Copyright (c) 2017-2022 Linaro Ltd
> > + * Copyright (c) 2010-2012, The Linux Foundation. All rights reserved.
> > + */
> > +#include <linux/bits.h>
> > +#include <linux/led-class-multicolor.h>
> > +#include <linux/module.h>
> > +#include <linux/of.h>
> > +#include <linux/of_device.h>
> > +#include <linux/platform_device.h>
> > +#include <linux/pwm.h>
> > +#include <linux/regmap.h>
> > +#include <linux/slab.h>
> > +
> > +#define LPG_PATTERN_CONFIG_REG	0x40
> > +#define LPG_SIZE_CLK_REG	0x41
> > +#define LPG_PREDIV_CLK_REG	0x42
> > +#define PWM_TYPE_CONFIG_REG	0x43
> > +#define PWM_VALUE_REG		0x44
> > +#define PWM_ENABLE_CONTROL_REG	0x46
> > +#define PWM_SYNC_REG		0x47
> > +#define LPG_RAMP_DURATION_REG	0x50
> > +#define LPG_HI_PAUSE_REG	0x52
> > +#define LPG_LO_PAUSE_REG	0x54
> > +#define LPG_HI_IDX_REG		0x56
> > +#define LPG_LO_IDX_REG		0x57
> > +#define PWM_SEC_ACCESS_REG	0xd0
> > +#define PWM_DTEST_REG(x)	(0xe2 + (x) - 1)
> > +
> > +#define TRI_LED_SRC_SEL		0x45
> > +#define TRI_LED_EN_CTL		0x46
> > +#define TRI_LED_ATC_CTL		0x47
> > +
> > +#define LPG_LUT_REG(x)		(0x40 + (x) * 2)
> > +#define RAMP_CONTROL_REG	0xc8
> > +
> > +#define LPG_RESOLUTION		512
> > +#define LPG_MAX_M		7
> > +
> > +struct lpg_channel;
> > +struct lpg_data;
> > +
> > +/**
> > + * struct lpg - LPG device context
> > + * @dev:	struct device for LPG device
> > + * @map:	regmap for register access
> > + * @pwm:	PWM-chip object, if operating in PWM mode
> > + * @data:	reference to version specific data
> > + * @lut_base:	base address of the LUT block (optional)
> > + * @lut_size:	number of entries in the LUT block
> > + * @lut_bitmap:	allocation bitmap for LUT entries
> > + * @triled_base: base address of the TRILED block (optional)
> > + * @triled_src:	power-source for the TRILED
> > + * @triled_has_atc_ctl:	true if there is TRI_LED_ATC_CTL register
> > + * @triled_has_src_sel:	true if there is TRI_LED_SRC_SEL register
> > + * @channels:	list of PWM channels
> > + * @num_channels: number of @channels
> > + */
> > +struct lpg {
> > +	struct device *dev;
> > +	struct regmap *map;
> > +
> > +	struct pwm_chip pwm;
> > +
> > +	const struct lpg_data *data;
> > +
> > +	u32 lut_base;
> > +	u32 lut_size;
> > +	unsigned long *lut_bitmap;
> > +
> > +	u32 triled_base;
> > +	u32 triled_src;
> > +	bool triled_has_atc_ctl;
> > +	bool triled_has_src_sel;
> > +
> > +	struct lpg_channel *channels;
> > +	unsigned int num_channels;
> > +};
> > +
> > +/**
> > + * struct lpg_channel - per channel data
> > + * @lpg:	reference to parent lpg
> > + * @base:	base address of the PWM channel
> > + * @triled_mask: mask in TRILED to enable this channel
> > + * @lut_mask:	mask in LUT to start pattern generator for this channel
> > + * @in_use:	channel is exposed to LED framework
> > + * @color:	color of the LED attached to this channel
> > + * @dtest_line:	DTEST line for output, or 0 if disabled
> > + * @dtest_value: DTEST line configuration
> > + * @pwm_value:	duty (in microseconds) of the generated pulses, overridden by LUT
> > + * @enabled:	output enabled?
> > + * @period:	period (in nanoseconds) of the generated pulses
> > + * @clk:	base frequency of the clock generator
> > + * @pre_div:	divider of @clk
> > + * @pre_div_exp: exponential divider of @clk
> > + * @ramp_enabled: duty cycle is driven by iterating over lookup table
> > + * @ramp_ping_pong: reverse through pattern, rather than wrapping to start
> > + * @ramp_oneshot: perform only a single pass over the pattern
> > + * @ramp_reverse: iterate over pattern backwards
> > + * @ramp_tick_ms: length (in milliseconds) of one step in the pattern
> > + * @ramp_lo_pause_ms: pause (in milliseconds) before iterating over pattern
> > + * @ramp_hi_pause_ms: pause (in milliseconds) after iterating over pattern
> > + * @pattern_lo_idx: start index of associated pattern
> > + * @pattern_hi_idx: last index of associated pattern
> > + */
> > +struct lpg_channel {
> > +	struct lpg *lpg;
> > +
> > +	u32 base;
> > +	unsigned int triled_mask;
> > +	unsigned int lut_mask;
> > +
> > +	bool in_use;
> > +
> > +	int color;
> > +
> > +	u32 dtest_line;
> > +	u32 dtest_value;
> > +
> > +	u16 pwm_value;
> > +	bool enabled;
> > +
> > +	u64 period;
> > +	unsigned int clk;
> > +	unsigned int pre_div;
> > +	unsigned int pre_div_exp;
> > +
> > +	bool ramp_enabled;
> > +	bool ramp_ping_pong;
> > +	bool ramp_oneshot;
> > +	bool ramp_reverse;
> > +	unsigned short ramp_tick_ms;
> > +	unsigned long ramp_lo_pause_ms;
> > +	unsigned long ramp_hi_pause_ms;
> > +
> > +	unsigned int pattern_lo_idx;
> > +	unsigned int pattern_hi_idx;
> > +};
> > +
> > +/**
> > + * struct lpg_led - logical LED object
> > + * @lpg:		lpg context reference
> > + * @cdev:		LED class device
> > + * @mcdev:		Multicolor LED class device
> > + * @num_channels:	number of @channels
> > + * @channels:		list of channels associated with the LED
> > + */
> > +struct lpg_led {
> > +	struct lpg *lpg;
> > +
> > +	struct led_classdev cdev;
> > +	struct led_classdev_mc mcdev;
> > +
> > +	unsigned int num_channels;
> > +	struct lpg_channel *channels[];
> > +};
> > +
> > +/**
> > + * struct lpg_channel_data - per channel initialization data
> > + * @base:		base address for PWM channel registers
> > + * @triled_mask:	bitmask for controlling this channel in TRILED
> > + */
> > +struct lpg_channel_data {
> > +	unsigned int base;
> > +	u8 triled_mask;
> > +};
> > +
> > +/**
> > + * 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
> > + * @triled_has_atc_ctl:	true if there is TRI_LED_ATC_CTL register
> > + * @triled_has_src_sel:	true if there is TRI_LED_SRC_SEL register
> > + * @pwm_9bit_mask:	bitmask for switching from 6bit to 9bit pwm
> > + * @num_channels:	number of channels in LPG
> > + * @channels:		list of channel initialization data
> > + */
> > +struct lpg_data {
> > +	unsigned int lut_base;
> > +	unsigned int lut_size;
> > +	unsigned int triled_base;
> > +	bool triled_has_atc_ctl;
> > +	bool triled_has_src_sel;
> > +	unsigned int pwm_9bit_mask;
> > +	int num_channels;
> > +	const struct lpg_channel_data *channels;
> > +};
> > +
> > +static int triled_set(struct lpg *lpg, unsigned int mask, unsigned int enable)
> > +{
> > +	/* Skip if we don't have a triled block */
> > +	if (!lpg->triled_base)
> > +		return 0;
> > +
> > +	return regmap_update_bits(lpg->map, lpg->triled_base + TRI_LED_EN_CTL,
> > +				  mask, enable);
> > +}
> > +
> > +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, sizeof(val));
> > +	}
> > +
> > +	bitmap_set(lpg->lut_bitmap, idx, len);
> > +
> > +	*lo_idx = idx;
> > +	*hi_idx = idx + len - 1;
> > +
> > +	return 0;
> > +}
> > +
> > +static void lpg_lut_free(struct lpg *lpg, unsigned int lo_idx, unsigned int hi_idx)
> > +{
> > +	int len;
> > +
> > +	if (lo_idx == hi_idx)
> > +		return;
> > +
> > +	len = hi_idx - lo_idx + 1;
> > +	bitmap_clear(lpg->lut_bitmap, lo_idx, len);
> > +}
> > +
> > +static int lpg_lut_sync(struct lpg *lpg, unsigned int mask)
> > +{
> > +	return regmap_write(lpg->map, lpg->lut_base + RAMP_CONTROL_REG, mask);
> > +}
> > +
> > +static const unsigned int lpg_clk_rates[] = {1024, 32768, 19200000};
> > +static const unsigned int lpg_pre_divs[] = {1, 3, 5, 6};
> > +
> > +static int lpg_calc_freq(struct lpg_channel *chan, uint64_t period)
> > +{
> > +	unsigned int clk, best_clk = 0;
> > +	unsigned int div, best_div = 0;
> > +	unsigned int m, best_m = 0;
> > +	unsigned int error;
> > +	unsigned int best_err = UINT_MAX;
> > +	u64 best_period = 0;
> > +
> > +	/*
> > +	 * The PWM period is determined by:
> > +	 *
> > +	 *          resolution * pre_div * 2^M
> > +	 * period = --------------------------
> > +	 *                   refclk
> > +	 *
> > +	 * With resolution fixed at 2^9 bits, pre_div = {1, 3, 5, 6} and
> > +	 * M = [0..7].
> > +	 *
> > +	 * This allows for periods between 27uS and 384s, as the PWM framework
> > +	 * wants a period of equal or lower length than requested, reject
> > +	 * anything below 27uS.
> > +	 */
> > +	if (period <= (u64)NSEC_PER_SEC * LPG_RESOLUTION / 19200000)
> > +		return -EINVAL;
> > +
> > +	/* Limit period to largest possible value, to avoid overflows */
> > +	if (period > (u64)NSEC_PER_SEC * LPG_RESOLUTION * 6 * (1 << LPG_MAX_M) / 1024)
> > +		period = (u64)NSEC_PER_SEC * LPG_RESOLUTION * 6 * (1 << LPG_MAX_M) / 2014;
> > +
> > +	/*
> > +	 * Search for the pre_div, clk and M by solving the rewritten formula
> > +	 * for each clk and pre_div value:
> > +	 *
> > +	 *                       period * clk
> > +	 * M = log2 -------------------------------------
> > +	 *           NSEC_PER_SEC * pre_div * resolution
> > +	 */
> > +	for (clk = 0; clk < ARRAY_SIZE(lpg_clk_rates); clk++) {
> > +		u64 nom = period * lpg_clk_rates[clk];
> > +
> > +		for (div = 0; div < ARRAY_SIZE(lpg_pre_divs); div++) {
> > +			u64 denom = (u64)NSEC_PER_SEC * lpg_pre_divs[div] * (1 << 9);
> > +			u64 actual;
> > +			u64 ratio;
> > +
> > +			if (nom < denom)
> > +				continue;
> > +
> > +			ratio = div64_u64(nom, denom);
> > +			m = ilog2(ratio);
> > +			if (m > LPG_MAX_M)
> > +				m = LPG_MAX_M;
> > +
> > +			actual = DIV_ROUND_UP_ULL(denom * (1 << m), lpg_clk_rates[clk]);
> > +
> > +			error = period - actual;
> > +			if (error < best_err) {
> > +				best_err = error;
> > +
> > +				best_div = div;
> > +				best_m = m;
> > +				best_clk = clk;
> > +				best_period = actual;
> > +			}
> > +		}
> > +	}
> > +
> > +	chan->clk = best_clk;
> > +	chan->pre_div = best_div;
> > +	chan->pre_div_exp = best_m;
> > +	chan->period = best_period;
> > +
> > +	return 0;
> > +}
> > +
> > +static void lpg_calc_duty(struct lpg_channel *chan, uint64_t duty)
> > +{
> > +	unsigned int max = LPG_RESOLUTION - 1;
> > +	unsigned int val;
> > +
> > +	val = div64_u64(duty * lpg_clk_rates[chan->clk],
> > +			(u64)NSEC_PER_SEC * lpg_pre_divs[chan->pre_div] * (1 << chan->pre_div_exp));
> > +
> > +	chan->pwm_value = min(val, max);
> > +}
> > +
> > +static void lpg_apply_freq(struct lpg_channel *chan)
> > +{
> > +	unsigned long val;
> > +	struct lpg *lpg = chan->lpg;
> > +
> > +	if (!chan->enabled)
> > +		return;
> > +
> > +	/* Clock register values are off-by-one from lpg_clk_table */
> > +	val = chan->clk + 1;
> > +
> > +	/* Enable 9bit resolution */
> > +	val |= lpg->data->pwm_9bit_mask;
> > +
> > +	regmap_write(lpg->map, chan->base + LPG_SIZE_CLK_REG, val);
> > +
> > +	val = chan->pre_div << 5 | chan->pre_div_exp;
> > +	regmap_write(lpg->map, chan->base + LPG_PREDIV_CLK_REG, val);
> > +}
> > +
> > +#define LPG_ENABLE_GLITCH_REMOVAL	BIT(5)
> > +
> > +static void lpg_enable_glitch(struct lpg_channel *chan)
> > +{
> > +	struct lpg *lpg = chan->lpg;
> > +
> > +	regmap_update_bits(lpg->map, chan->base + PWM_TYPE_CONFIG_REG,
> > +			   LPG_ENABLE_GLITCH_REMOVAL, 0);
> > +}
> > +
> > +static void lpg_disable_glitch(struct lpg_channel *chan)
> > +{
> > +	struct lpg *lpg = chan->lpg;
> > +
> > +	regmap_update_bits(lpg->map, chan->base + PWM_TYPE_CONFIG_REG,
> > +			   LPG_ENABLE_GLITCH_REMOVAL,
> > +			   LPG_ENABLE_GLITCH_REMOVAL);
> > +}
> > +
> > +static void lpg_apply_pwm_value(struct lpg_channel *chan)
> > +{
> > +	struct lpg *lpg = chan->lpg;
> > +	u16 val = chan->pwm_value;
> > +
> > +	if (!chan->enabled)
> > +		return;
> > +
> > +	regmap_bulk_write(lpg->map, chan->base + PWM_VALUE_REG, &val, sizeof(val));
> > +}
> > +
> > +#define LPG_PATTERN_CONFIG_LO_TO_HI	BIT(4)
> > +#define LPG_PATTERN_CONFIG_REPEAT	BIT(3)
> > +#define LPG_PATTERN_CONFIG_TOGGLE	BIT(2)
> > +#define LPG_PATTERN_CONFIG_PAUSE_HI	BIT(1)
> > +#define LPG_PATTERN_CONFIG_PAUSE_LO	BIT(0)
> > +
> > +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 conf = 0;
> > +	unsigned int lo_idx = chan->pattern_lo_idx;
> > +	unsigned int hi_idx = chan->pattern_hi_idx;
> > +	u16 step = chan->ramp_tick_ms;
> > +
> > +	if (!chan->ramp_enabled || chan->pattern_lo_idx == chan->pattern_hi_idx)
> > +		return;
> > +
> > +	hi_pause = DIV_ROUND_UP(chan->ramp_hi_pause_ms, step);
> > +	lo_pause = DIV_ROUND_UP(chan->ramp_lo_pause_ms, step);
> > +
> > +	if (!chan->ramp_reverse)
> > +		conf |= LPG_PATTERN_CONFIG_LO_TO_HI;
> > +	if (!chan->ramp_oneshot)
> > +		conf |= LPG_PATTERN_CONFIG_REPEAT;
> > +	if (chan->ramp_ping_pong)
> > +		conf |= LPG_PATTERN_CONFIG_TOGGLE;
> > +	if (chan->ramp_hi_pause_ms)
> > +		conf |= LPG_PATTERN_CONFIG_PAUSE_HI;
> > +	if (chan->ramp_lo_pause_ms)
> > +		conf |= LPG_PATTERN_CONFIG_PAUSE_LO;
> > +
> > +	regmap_write(lpg->map, chan->base + LPG_PATTERN_CONFIG_REG, conf);
> > +	regmap_write(lpg->map, chan->base + LPG_HI_IDX_REG, hi_idx);
> > +	regmap_write(lpg->map, chan->base + LPG_LO_IDX_REG, lo_idx);
> > +
> > +	regmap_bulk_write(lpg->map, chan->base + LPG_RAMP_DURATION_REG, &step, sizeof(step));
> > +	regmap_write(lpg->map, chan->base + LPG_HI_PAUSE_REG, hi_pause);
> > +	regmap_write(lpg->map, chan->base + LPG_LO_PAUSE_REG, lo_pause);
> > +}
> > +
> > +#define LPG_ENABLE_CONTROL_OUTPUT		BIT(7)
> > +#define LPG_ENABLE_CONTROL_BUFFER_TRISTATE	BIT(5)
> > +#define LPG_ENABLE_CONTROL_SRC_PWM		BIT(2)
> > +#define LPG_ENABLE_CONTROL_RAMP_GEN		BIT(1)
> > +
> > +static void lpg_apply_control(struct lpg_channel *chan)
> > +{
> > +	unsigned int ctrl;
> > +	struct lpg *lpg = chan->lpg;
> > +
> > +	ctrl = LPG_ENABLE_CONTROL_BUFFER_TRISTATE;
> > +
> > +	if (chan->enabled)
> > +		ctrl |= LPG_ENABLE_CONTROL_OUTPUT;
> > +
> > +	if (chan->pattern_lo_idx != chan->pattern_hi_idx)
> > +		ctrl |= LPG_ENABLE_CONTROL_RAMP_GEN;
> > +	else
> > +		ctrl |= LPG_ENABLE_CONTROL_SRC_PWM;
> > +
> > +	regmap_write(lpg->map, chan->base + PWM_ENABLE_CONTROL_REG, ctrl);
> > +
> > +	/*
> > +	 * Due to LPG hardware bug, in the PWM mode, having enabled PWM,
> > +	 * We have to write PWM values one more time.
> > +	 */
> > +	if (chan->enabled)
> > +		lpg_apply_pwm_value(chan);
> > +}
> > +
> > +#define LPG_SYNC_PWM	BIT(0)
> > +
> > +static void lpg_apply_sync(struct lpg_channel *chan)
> > +{
> > +	struct lpg *lpg = chan->lpg;
> > +
> > +	regmap_write(lpg->map, chan->base + PWM_SYNC_REG, LPG_SYNC_PWM);
> > +}
> > +
> > +static int lpg_parse_dtest(struct lpg *lpg)
> > +{
> > +	struct lpg_channel *chan;
> > +	struct device_node *np = lpg->dev->of_node;
> > +	int count;
> > +	int ret;
> > +	int i;
> > +
> > +	count = of_property_count_u32_elems(np, "qcom,dtest");
> > +	if (count == -EINVAL) {
> > +		return 0;
> > +	} else if (count < 0) {
> > +		ret = count;
> > +		goto err_malformed;
> > +	} else if (count != lpg->data->num_channels * 2) {
> > +		dev_err(lpg->dev, "qcom,dtest needs to be %d items\n",
> > +			lpg->data->num_channels * 2);
> > +		return -EINVAL;
> > +	}
> > +
> > +	for (i = 0; i < lpg->data->num_channels; i++) {
> > +		chan = &lpg->channels[i];
> > +
> > +		ret = of_property_read_u32_index(np, "qcom,dtest", i * 2,
> > +						 &chan->dtest_line);
> > +		if (ret)
> > +			goto err_malformed;
> > +
> > +		ret = of_property_read_u32_index(np, "qcom,dtest", i * 2 + 1,
> > +						 &chan->dtest_value);
> > +		if (ret)
> > +			goto err_malformed;
> > +	}
> > +
> > +	return 0;
> > +
> > +err_malformed:
> > +	dev_err(lpg->dev, "malformed qcom,dtest\n");
> > +	return ret;
> > +}
> > +
> > +static void lpg_apply_dtest(struct lpg_channel *chan)
> > +{
> > +	struct lpg *lpg = chan->lpg;
> > +
> > +	if (!chan->dtest_line)
> > +		return;
> > +
> > +	regmap_write(lpg->map, chan->base + PWM_SEC_ACCESS_REG, 0xa5);
> > +	regmap_write(lpg->map, chan->base + PWM_DTEST_REG(chan->dtest_line),
> > +		     chan->dtest_value);
> > +}
> > +
> > +static void lpg_apply(struct lpg_channel *chan)
> > +{
> > +	lpg_disable_glitch(chan);
> > +	lpg_apply_freq(chan);
> > +	lpg_apply_pwm_value(chan);
> > +	lpg_apply_control(chan);
> > +	lpg_apply_sync(chan);
> > +	lpg_apply_lut_control(chan);
> > +	lpg_enable_glitch(chan);
> > +}
> > +
> > +static void lpg_brightness_set(struct lpg_led *led, struct led_classdev *cdev,
> > +			       struct mc_subled *subleds)
> > +{
> > +	enum led_brightness brightness;
> > +	struct lpg_channel *chan;
> > +	unsigned int triled_enabled = 0;
> > +	unsigned int triled_mask = 0;
> > +	unsigned int lut_mask = 0;
> > +	unsigned int duty;
> > +	struct lpg *lpg = led->lpg;
> > +	int i;
> > +
> > +	for (i = 0; i < led->num_channels; i++) {
> > +		chan = led->channels[i];
> > +		brightness = subleds[i].brightness;
> > +
> > +		if (brightness == LED_OFF) {
> > +			chan->enabled = false;
> > +			chan->ramp_enabled = false;
> > +		} else if (chan->pattern_lo_idx != chan->pattern_hi_idx) {
> > +			lpg_calc_freq(chan, NSEC_PER_MSEC);
> > +
> > +			chan->enabled = true;
> > +			chan->ramp_enabled = true;
> > +
> > +			lut_mask |= chan->lut_mask;
> > +			triled_enabled |= chan->triled_mask;
> > +		} else {
> > +			lpg_calc_freq(chan, NSEC_PER_MSEC);
> > +
> > +			duty = div_u64(brightness * chan->period, cdev->max_brightness);
> > +			lpg_calc_duty(chan, duty);
> > +			chan->enabled = true;
> > +			chan->ramp_enabled = false;
> > +
> > +			triled_enabled |= chan->triled_mask;
> > +		}
> > +
> > +		triled_mask |= chan->triled_mask;
> > +
> > +		lpg_apply(chan);
> > +	}
> > +
> > +	/* Toggle triled lines */
> > +	if (triled_mask)
> > +		triled_set(lpg, triled_mask, triled_enabled);
> > +
> > +	/* Trigger start of ramp generator(s) */
> > +	if (lut_mask)
> > +		lpg_lut_sync(lpg, lut_mask);
> > +}
> > +
> > +static void lpg_brightness_single_set(struct led_classdev *cdev,
> > +				      enum led_brightness value)
> > +{
> > +	struct lpg_led *led = container_of(cdev, struct lpg_led, cdev);
> > +	struct mc_subled info;
> > +
> > +	info.brightness = value;
> > +	lpg_brightness_set(led, cdev, &info);
> > +}
> > +
> > +static void lpg_brightness_mc_set(struct led_classdev *cdev,
> > +				  enum led_brightness value)
> > +{
> > +	struct led_classdev_mc *mc = lcdev_to_mccdev(cdev);
> > +	struct lpg_led *led = container_of(mc, struct lpg_led, mcdev);
> > +
> > +	led_mc_calc_color_components(mc, value);
> > +	lpg_brightness_set(led, cdev, mc->subled_info);
> > +}
> > +
> > +static int lpg_blink_set(struct lpg_led *led,
> > +			 unsigned long *delay_on, unsigned long *delay_off)
> > +{
> > +	struct lpg_channel *chan;
> > +	unsigned int period;
> > +	unsigned int triled_mask = 0;
> > +	struct lpg *lpg = led->lpg;
> > +	u64 duty;
> > +	int i;
> > +
> > +	if (!*delay_on && !*delay_off) {
> > +		*delay_on = 500;
> > +		*delay_off = 500;
> > +	}
> > +
> > +	duty = *delay_on * NSEC_PER_MSEC;
> > +	period = (*delay_on + *delay_off) * NSEC_PER_MSEC;
> > +
> > +	for (i = 0; i < led->num_channels; i++) {
> > +		chan = led->channels[i];
> > +
> > +		lpg_calc_freq(chan, period);
> > +		lpg_calc_duty(chan, duty);
> > +
> > +		chan->enabled = true;
> > +		chan->ramp_enabled = false;
> > +
> > +		triled_mask |= chan->triled_mask;
> > +
> > +		lpg_apply(chan);
> > +	}
> > +
> > +	/* Enable triled lines */
> > +	triled_set(lpg, triled_mask, triled_mask);
> > +
> > +	chan = led->channels[0];
> > +	duty = div_u64(chan->pwm_value * chan->period, LPG_RESOLUTION);
> > +	*delay_on = div_u64(duty, NSEC_PER_MSEC);
> > +	*delay_off = div_u64(chan->period - duty, NSEC_PER_MSEC);
> > +
> > +	return 0;
> > +}
> > +
> > +static int lpg_blink_single_set(struct led_classdev *cdev,
> > +				unsigned long *delay_on, unsigned long *delay_off)
> > +{
> > +	struct lpg_led *led = container_of(cdev, struct lpg_led, cdev);
> > +
> > +	return lpg_blink_set(led, delay_on, delay_off);
> > +}
> > +
> > +static int lpg_blink_mc_set(struct led_classdev *cdev,
> > +			    unsigned long *delay_on, unsigned long *delay_off)
> > +{
> > +	struct led_classdev_mc *mc = lcdev_to_mccdev(cdev);
> > +	struct lpg_led *led = container_of(mc, struct lpg_led, mcdev);
> > +
> > +	return lpg_blink_set(led, delay_on, delay_off);
> > +}
> > +
> > +static int lpg_pattern_set(struct lpg_led *led, struct led_pattern *pattern,
> > +			   u32 len, int repeat)
> > +{
> > +	struct lpg_channel *chan;
> > +	struct lpg *lpg = led->lpg;
> > +	unsigned int hi_pause;
> > +	unsigned int lo_pause;
> > +	unsigned int lo_idx;
> > +	unsigned int hi_idx;
> > +	bool ping_pong = true;
> > +	int brightness_a;
> > +	int brightness_b;
> > +	int ret;
> > +	int i;
> > +
> > +	/* Only support oneshot or indefinite loops, due to limited pattern space */
> > +	if (repeat != -1 && repeat != 1)
> > +		return -EINVAL;
> > +
> > +	/* LPG_RAMP_DURATION_REG is 9 bit */
> > +	if (pattern[0].delta_t >= 512)
> > +		return -EINVAL;
> > +
> > +	/*
> > +	 * The LPG plays patterns with at a fixed pace, a "low pause" can be
> > +	 * performed before the pattern and a "high pause" after. In order to
> > +	 * save space the pattern can be played in "ping pong" mode, in which
> > +	 * the pattern is first played forward, then "high pause" is applied,
> > +	 * then the pattern is played backwards and finally the "low pause" is
> > +	 * applied.
> > +	 *
> > +	 * The delta_t of the first entry is used to determine the pace of the
> > +	 * pattern.
> > +	 *
> > +	 * If the specified pattern is a palindrome the ping pong mode is
> > +	 * enabled. In this scenario the delta_t of the last entry determines
> > +	 * the "low pause" time and the delta_t of the middle entry (i.e. the
> > +	 * last in the programmed pattern) determines the "high pause". If the
> > +	 * pattern consists of an odd number of values, no "high pause" is
> > +	 * used.
> > +	 *
> > +	 * When ping pong mode is not selected, the delta_t of the last entry
> > +	 * is used as "high pause". No "low pause" is used.
> > +	 *
> > +	 * delta_t of any other members of the pattern is ignored.
> > +	 */
> > +
> > +	/* Detect palindromes and use "ping pong" to reduce LUT usage */
> > +	for (i = 0; i < len / 2; i++) {
> > +		brightness_a = pattern[i].brightness;
> > +		brightness_b = pattern[len - i - 1].brightness;
> > +
> > +		if (brightness_a != brightness_b) {
> > +			ping_pong = false;
> > +			break;
> > +		}
> > +	}
> > +
> > +	if (ping_pong) {
> > +		if (len % 2)
> > +			hi_pause = 0;
> > +		else
> > +			hi_pause = pattern[(len + 1) / 2].delta_t;
> > +		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)
> > +		return ret;
> > +
> > +	for (i = 0; i < led->num_channels; i++) {
> > +		chan = led->channels[i];
> > +
> > +		chan->ramp_tick_ms = pattern[0].delta_t;
> > +		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;
> > +	}
> > +
> > +	return 0;
> > +}
> > +
> > +static int lpg_pattern_single_set(struct led_classdev *cdev,
> > +				  struct led_pattern *pattern, u32 len,
> > +				  int repeat)
> > +{
> > +	struct lpg_led *led = container_of(cdev, struct lpg_led, cdev);
> > +	int ret;
> > +
> > +	ret = lpg_pattern_set(led, pattern, len, repeat);
> > +	if (ret < 0)
> > +		return ret;
> > +
> > +	lpg_brightness_single_set(cdev, LED_FULL);
> > +
> > +	return 0;
> > +}
> > +
> > +static int lpg_pattern_mc_set(struct led_classdev *cdev,
> > +			      struct led_pattern *pattern, u32 len,
> > +			      int repeat)
> > +{
> > +	struct led_classdev_mc *mc = lcdev_to_mccdev(cdev);
> > +	struct lpg_led *led = container_of(mc, struct lpg_led, mcdev);
> > +	int ret;
> > +
> > +	ret = lpg_pattern_set(led, pattern, len, repeat);
> > +	if (ret < 0)
> > +		return ret;
> > +
> > +	led_mc_calc_color_components(mc, LED_FULL);
> > +	lpg_brightness_set(led, cdev, mc->subled_info);
> > +
> > +	return 0;
> > +}
> > +
> > +static int lpg_pattern_clear(struct lpg_led *led)
> > +{
> > +	struct lpg_channel *chan;
> > +	struct lpg *lpg = led->lpg;
> > +	int i;
> > +
> > +	chan = led->channels[0];
> > +	lpg_lut_free(lpg, chan->pattern_lo_idx, chan->pattern_hi_idx);
> > +
> > +	for (i = 0; i < led->num_channels; i++) {
> > +		chan = led->channels[i];
> > +		chan->pattern_lo_idx = 0;
> > +		chan->pattern_hi_idx = 0;
> > +	}
> > +
> > +	return 0;
> > +}
> > +
> > +static int lpg_pattern_single_clear(struct led_classdev *cdev)
> > +{
> > +	struct lpg_led *led = container_of(cdev, struct lpg_led, cdev);
> > +
> > +	return lpg_pattern_clear(led);
> > +}
> > +
> > +static int lpg_pattern_mc_clear(struct led_classdev *cdev)
> > +{
> > +	struct led_classdev_mc *mc = lcdev_to_mccdev(cdev);
> > +	struct lpg_led *led = container_of(mc, struct lpg_led, mcdev);
> > +
> > +	return lpg_pattern_clear(led);
> > +}
> > +
> > +static int lpg_pwm_request(struct pwm_chip *chip, struct pwm_device *pwm)
> > +{
> > +	struct lpg *lpg = container_of(chip, struct lpg, pwm);
> > +	struct lpg_channel *chan = &lpg->channels[pwm->hwpwm];
> > +
> > +	return chan->in_use ? -EBUSY : 0;
> > +}
> > +
> > +/*
> > + * Limitations:
> > + * - Updating both duty and period is not done atomically, so the output signal
> > + *   will momentarily be a mix of the settings.
> > + */
> > +static int lpg_pwm_apply(struct pwm_chip *chip, struct pwm_device *pwm,
> > +			 const struct pwm_state *state)
> > +{
> > +	struct lpg *lpg = container_of(chip, struct lpg, pwm);
> > +	struct lpg_channel *chan = &lpg->channels[pwm->hwpwm];
> > +	int ret;
> > +
> > +	if (state->polarity != PWM_POLARITY_NORMAL)
> > +		return -EINVAL;
> > +
> > +	ret = lpg_calc_freq(chan, state->period);
> > +	if (ret < 0)
> > +		return ret;
> > +
> > +	lpg_calc_duty(chan, state->duty_cycle);
> > +	chan->enabled = state->enabled;
> > +
> > +	lpg_apply(chan);
> > +
> > +	triled_set(lpg, chan->triled_mask, chan->enabled ? chan->triled_mask : 0);
> > +
> > +	return 0;
> > +}
> > +
> > +static void lpg_pwm_get_state(struct pwm_chip *chip, struct pwm_device *pwm,
> > +			      struct pwm_state *state)
> > +{
> > +	struct lpg *lpg = container_of(chip, struct lpg, pwm);
> > +	struct lpg_channel *chan = &lpg->channels[pwm->hwpwm];
> > +	u64 duty = DIV_ROUND_UP_ULL(chan->pwm_value * chan->period, LPG_RESOLUTION - 1);
> > +
> > +	state->period = chan->period;
> > +	state->duty_cycle = duty;
> > +	state->polarity = PWM_POLARITY_NORMAL;
> > +	state->enabled = chan->enabled;
> > +}
> > +
> > +static const struct pwm_ops lpg_pwm_ops = {
> > +	.request = lpg_pwm_request,
> > +	.apply = lpg_pwm_apply,
> > +	.get_state = lpg_pwm_get_state,
> > +	.owner = THIS_MODULE,
> > +};
> > +
> > +static int lpg_add_pwm(struct lpg *lpg)
> > +{
> > +	int ret;
> > +
> > +	lpg->pwm.base = -1;
> > +	lpg->pwm.dev = lpg->dev;
> > +	lpg->pwm.npwm = lpg->num_channels;
> > +	lpg->pwm.ops = &lpg_pwm_ops;
> > +
> > +	ret = pwmchip_add(&lpg->pwm);
> > +	if (ret)
> > +		dev_err(lpg->dev, "failed to add PWM chip: ret %d\n", ret);
> > +
> > +	return ret;
> > +}
> > +
> > +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);
> > +		return -EINVAL;
> > +	}
> > +
> > +	chan = &lpg->channels[reg - 1];
> > +	chan->in_use = true;
> > +
> > +	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;
> > +	}
> > +
> > +	chan->color = color;
> > +
> > +	*channel = chan;
> > +
> > +	return 0;
> > +}
> > +
> > +static int lpg_add_led(struct lpg *lpg, struct device_node *np)
> > +{
> > +	struct led_init_data init_data = {};
> > +	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_RGB)
> > +		num_channels = of_get_available_child_count(np);
> > +	else
> > +		num_channels = 1;
> > +
> > +	led = devm_kzalloc(lpg->dev, struct_size(led, channels, num_channels), GFP_KERNEL);
> > +	if (!led)
> > +		return -ENOMEM;
> > +
> > +	led->lpg = lpg;
> > +	led->num_channels = num_channels;
> > +
> > +	if (color == LED_COLOR_ID_RGB) {
> > +		info = devm_kcalloc(lpg->dev, num_channels, sizeof(*info), GFP_KERNEL);
> > +		if (!info)
> > +			return -ENOMEM;
> > +		i = 0;
> > +		for_each_available_child_of_node(np, child) {
> > +			ret = lpg_parse_channel(lpg, child, &led->channels[i]);
> > +			if (ret < 0)
> > +				return ret;
> > +
> > +			info[i].color_index = led->channels[i]->color;
> > +			info[i].intensity = LED_FULL;
> > +			i++;
> > +		}
> > +
> > +		led->mcdev.subled_info = info;
> > +		led->mcdev.num_colors = num_channels;
> > +
> > +		cdev = &led->mcdev.led_cdev;
> > +		cdev->brightness_set = lpg_brightness_mc_set;
> > +		cdev->blink_set = lpg_blink_mc_set;
> > +
> > +		/* Register pattern accessors only if we have a LUT block */
> > +		if (lpg->lut_base) {
> > +			cdev->pattern_set = lpg_pattern_mc_set;
> > +			cdev->pattern_clear = lpg_pattern_mc_clear;
> > +		}
> > +	} else {
> > +		ret = lpg_parse_channel(lpg, np, &led->channels[0]);
> > +		if (ret < 0)
> > +			return ret;
> > +
> > +		cdev = &led->cdev;
> > +		cdev->brightness_set = lpg_brightness_single_set;
> > +		cdev->blink_set = lpg_blink_single_set;
> > +
> > +		/* Register pattern accessors only if we have a LUT block */
> > +		if (lpg->lut_base) {
> > +			cdev->pattern_set = lpg_pattern_single_set;
> > +			cdev->pattern_clear = lpg_pattern_single_clear;
> > +		}
> > +	}
> > +
> > +	cdev->default_trigger = of_get_property(np, "linux,default-trigger", NULL);
> > +	cdev->max_brightness = 255;
> > +
> > +	if (!of_property_read_string(np, "default-state", &state) &&
> > +	    !strcmp(state, "on"))
> > +		cdev->brightness = LED_FULL;
> > +	else
> > +		cdev->brightness = LED_OFF;
> > +
> > +	cdev->brightness_set(cdev, cdev->brightness);
> > +
> > +	init_data.fwnode = of_fwnode_handle(np);
> > +
> > +	if (color == LED_COLOR_ID_RGB)
> > +		ret = devm_led_classdev_multicolor_register_ext(lpg->dev, &led->mcdev, &init_data);
> > +	else
> > +		ret = devm_led_classdev_register_ext(lpg->dev, &led->cdev, &init_data);
> > +	if (ret)
> > +		dev_err(lpg->dev, "unable to register %s\n", cdev->name);
> > +
> > +	return ret;
> > +}
> > +
> > +static int lpg_init_channels(struct lpg *lpg)
> > +{
> > +	const struct lpg_data *data = lpg->data;
> > +	int i;
> > +
> > +	lpg->num_channels = data->num_channels;
> > +	lpg->channels = devm_kcalloc(lpg->dev, data->num_channels,
> > +				     sizeof(struct lpg_channel), GFP_KERNEL);
> > +	if (!lpg->channels)
> > +		return -ENOMEM;
> > +
> > +	for (i = 0; i < data->num_channels; i++) {
> > +		lpg->channels[i].lpg = lpg;
> > +		lpg->channels[i].base = data->channels[i].base;
> > +		lpg->channels[i].triled_mask = data->channels[i].triled_mask;
> > +		lpg->channels[i].lut_mask = BIT(i);
> > +	}
> > +
> > +	return 0;
> > +}
> > +
> > +static int lpg_init_triled(struct lpg *lpg)
> > +{
> > +	struct device_node *np = lpg->dev->of_node;
> > +	int ret;
> > +
> > +	/* Skip initialization if we don't have a triled block */
> > +	if (!lpg->data->triled_base)
> > +		return 0;
> > +
> > +	lpg->triled_base = lpg->data->triled_base;
> > +	lpg->triled_has_atc_ctl = lpg->data->triled_has_atc_ctl;
> > +	lpg->triled_has_src_sel = lpg->data->triled_has_src_sel;
> > +
> > +	if (lpg->triled_has_src_sel) {
> > +		ret = of_property_read_u32(np, "qcom,power-source", &lpg->triled_src);
> > +		if (ret || lpg->triled_src == 2 || lpg->triled_src > 3) {
> > +			dev_err(lpg->dev, "invalid power source\n");
> > +			return -EINVAL;
> > +		}
> > +	}
> > +
> > +	/* Disable automatic trickle charge LED */
> > +	if (lpg->triled_has_atc_ctl)
> > +		regmap_write(lpg->map, lpg->triled_base + TRI_LED_ATC_CTL, 0);
> > +
> > +	/* Configure power source */
> > +	if (lpg->triled_has_src_sel)
> > +		regmap_write(lpg->map, lpg->triled_base + TRI_LED_SRC_SEL, lpg->triled_src);
> > +
> > +	/* Default all outputs to off */
> > +	regmap_write(lpg->map, lpg->triled_base + TRI_LED_EN_CTL, 0);
> > +
> > +	return 0;
> > +}
> > +
> > +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;
> > +
> > +	return 0;
> > +}
> > +
> > +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;
> > +
> > +	platform_set_drvdata(pdev, lpg);
> > +
> > +	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_parse_dtest(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;
> > +
> > +	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]);
> > +
> > +	return lpg_add_pwm(lpg);
> > +}
> > +
> > +static int lpg_remove(struct platform_device *pdev)
> > +{
> > +	struct lpg *lpg = platform_get_drvdata(pdev);
> > +
> > +	pwmchip_remove(&lpg->pwm);
> > +
> > +	return 0;
> > +}
> > +
> > +static const struct lpg_data pm8916_pwm_data = {
> > +	.pwm_9bit_mask = BIT(2),
> > +
> > +	.num_channels = 1,
> > +	.channels = (const struct lpg_channel_data[]) {
> > +		{ .base = 0xbc00 },
> > +	},
> > +};
> > +
> > +static const struct lpg_data pm8941_lpg_data = {
> > +	.lut_base = 0xb000,
> > +	.lut_size = 64,
> > +
> > +	.triled_base = 0xd000,
> > +	.triled_has_atc_ctl = true,
> > +	.triled_has_src_sel = true,
> > +
> > +	.pwm_9bit_mask = 3 << 4,
> > +
> > +	.num_channels = 8,
> > +	.channels = (const struct lpg_channel_data[]) {
> > +		{ .base = 0xb100 },
> > +		{ .base = 0xb200 },
> > +		{ .base = 0xb300 },
> > +		{ .base = 0xb400 },
> > +		{ .base = 0xb500, .triled_mask = BIT(5) },
> > +		{ .base = 0xb600, .triled_mask = BIT(6) },
> > +		{ .base = 0xb700, .triled_mask = BIT(7) },
> > +		{ .base = 0xb800 },
> > +	},
> > +};
> > +
> > +static const struct lpg_data pm8994_lpg_data = {
> > +	.lut_base = 0xb000,
> > +	.lut_size = 64,
> > +
> > +	.pwm_9bit_mask = 3 << 4,
> > +
> > +	.num_channels = 6,
> > +	.channels = (const struct lpg_channel_data[]) {
> > +		{ .base = 0xb100 },
> > +		{ .base = 0xb200 },
> > +		{ .base = 0xb300 },
> > +		{ .base = 0xb400 },
> > +		{ .base = 0xb500 },
> > +		{ .base = 0xb600 },
> > +	},
> > +};
> > +
> > +static const struct lpg_data pmi8994_lpg_data = {
> > +	.lut_base = 0xb000,
> > +	.lut_size = 24,
> > +
> > +	.triled_base = 0xd000,
> > +	.triled_has_atc_ctl = true,
> > +	.triled_has_src_sel = true,
> > +
> > +	.pwm_9bit_mask = BIT(4),
> > +
> > +	.num_channels = 4,
> > +	.channels = (const struct lpg_channel_data[]) {
> > +		{ .base = 0xb100, .triled_mask = BIT(5) },
> > +		{ .base = 0xb200, .triled_mask = BIT(6) },
> > +		{ .base = 0xb300, .triled_mask = BIT(7) },
> > +		{ .base = 0xb400 },
> > +	},
> > +};
> > +
> > +static const struct lpg_data pmi8998_lpg_data = {
> > +	.lut_base = 0xb000,
> > +	.lut_size = 49,
> > +
> > +	.triled_base = 0xd000,
> > +
> > +	.pwm_9bit_mask = BIT(4),
> > +
> > +	.num_channels = 6,
> > +	.channels = (const struct lpg_channel_data[]) {
> > +		{ .base = 0xb100 },
> > +		{ .base = 0xb200 },
> > +		{ .base = 0xb300, .triled_mask = BIT(5) },
> > +		{ .base = 0xb400, .triled_mask = BIT(6) },
> > +		{ .base = 0xb500, .triled_mask = BIT(7) },
> > +		{ .base = 0xb600 },
> > +	},
> > +};
> > +
> > +static const struct lpg_data pm8150b_lpg_data = {
> > +	.lut_base = 0xb000,
> > +	.lut_size = 24,
> > +
> > +	.triled_base = 0xd000,
> > +
> > +	.pwm_9bit_mask = BIT(4),
> > +
> > +	.num_channels = 2,
> > +	.channels = (const struct lpg_channel_data[]) {
> > +		{ .base = 0xb100, .triled_mask = BIT(7) },
> > +		{ .base = 0xb200, .triled_mask = BIT(6) },
> > +	},
> > +};
> > +
> > +static const struct lpg_data pm8150l_lpg_data = {
> > +	.lut_base = 0xb000,
> > +	.lut_size = 48,
> > +
> > +	.triled_base = 0xd000,
> > +
> > +	.pwm_9bit_mask = BIT(4),
> > +
> > +	.num_channels = 5,
> > +	.channels = (const struct lpg_channel_data[]) {
> > +		{ .base = 0xb100, .triled_mask = BIT(7) },
> > +		{ .base = 0xb200, .triled_mask = BIT(6) },
> > +		{ .base = 0xb300, .triled_mask = BIT(5) },
> > +		{ .base = 0xbc00 },
> > +		{ .base = 0xbd00 },
> > +
> > +	},
> > +};
> > +
> > +static const struct of_device_id lpg_of_table[] = {
> > +	{ .compatible = "qcom,pm8150b-lpg", .data = &pm8150b_lpg_data },
> > +	{ .compatible = "qcom,pm8150l-lpg", .data = &pm8150l_lpg_data },
> > +	{ .compatible = "qcom,pm8916-pwm", .data = &pm8916_pwm_data },
> > +	{ .compatible = "qcom,pm8941-lpg", .data = &pm8941_lpg_data },
> > +	{ .compatible = "qcom,pm8994-lpg", .data = &pm8994_lpg_data },
> > +	{ .compatible = "qcom,pmi8994-lpg", .data = &pmi8994_lpg_data },
> > +	{ .compatible = "qcom,pmi8998-lpg", .data = &pmi8998_lpg_data },
> > +	{ .compatible = "qcom,pmc8180c-lpg", .data = &pm8150l_lpg_data },
> > +	{}
> > +};
> > +MODULE_DEVICE_TABLE(of, lpg_of_table);
> > +
> > +static struct platform_driver lpg_driver = {
> > +	.probe = lpg_probe,
> > +	.remove = lpg_remove,
> > +	.driver = {
> > +		.name = "qcom-spmi-lpg",
> > +		.of_match_table = lpg_of_table,
> > +	},
> > +};
> > +module_platform_driver(lpg_driver);
> > +
> > +MODULE_DESCRIPTION("Qualcomm LPG LED driver");
> > +MODULE_LICENSE("GPL v2");
> > -- 
> > 2.33.1
> >
Bjorn Andersson Feb. 2, 2022, 8:40 p.m. UTC | #5
On Wed 02 Feb 06:56 PST 2022, Andy Shevchenko wrote:

> On Tue, Feb 1, 2022 at 12:31 AM Bjorn Andersson
> <bjorn.andersson@linaro.org> wrote:
> >
> > The Light Pulse Generator (LPG) is a PWM-block found in a wide range of
> > PMICs from Qualcomm. These PMICs typically comes with 1-8 LPG instances,
> > with their output being routed to various other components, such as
> > current sinks or GPIOs.
> >
> > Each LPG instance can operate on fixed parameters or based on a shared
> > lookup-table, altering the duty cycle over time. This provides the means
> > for hardware assisted transitions of LED brightness.
> >
> > A typical use case for the fixed parameter mode is to drive a PWM
> > backlight control signal, the driver therefor allows each LPG instance
> > to be exposed to the kernel either through the LED framework or the PWM
> > framework.
> >
> > A typical use case for the LED configuration is to drive RGB LEDs in
> > smartphones etc, for which the driver support multiple channels to be
> 
> supports
> 
> > ganged up to a MULTICOLOR LED. In this configuration the pattern
> > generators will be synchronized, to allow for multi-color patterns.
> 
> ...
> 
> > +config LEDS_QCOM_LPG
> > +       tristate "LED support for Qualcomm LPG"
> 
> > +       depends on OF
> 
> || COMPILE_TEST
> 
> > +       depends on SPMI
> > +       help
> > +         This option enables support for the Light Pulse Generator found in a
> > +         wide variety of Qualcomm PMICs.
> 
> Module name?
> 
> ...
> 
> > +#include <linux/of.h>
> > +#include <linux/of_device.h>
> 
> Wondering if these can be changed to mod_devicetable.h + property.,h.
> 
> ...
> 
> > + * @dev:       struct device for LPG device
> 
> Description without value and actually wrong. it's a pointer to, and
> not a struct device.
> 
> ...
> 
> > +       /* Hardware does not behave when LO_IDX == HI_IDX */
> 
> Any clue /. elaboration why?
> 

As the two indices are inclusive I was expecting to just get a
single-value pattern (i.e. static configuration), but instead it just
keeps looping through the entire pattern memory.

> ...
> 
> > +static void lpg_lut_free(struct lpg *lpg, unsigned int lo_idx, unsigned int hi_idx)
> > +{
> > +       int len;
> > +
> > +       if (lo_idx == hi_idx)
> > +               return;
> > +
> > +       len = hi_idx - lo_idx + 1;
> 
> Perhaps swap above and add the similar comment:
> 

Sounds reasonable.

> /* We never do a single item because ... */
> len =
> if (len == 1)
> 
> > +       bitmap_clear(lpg->lut_bitmap, lo_idx, len);
> 
> Who protects this bitmap from simultaneous access by different users?
> 

It's protected per LED, but apparently not cross LEDs. Will fix.

> > +}
> 
> ...
> 
> > +static int lpg_calc_freq(struct lpg_channel *chan, uint64_t period)
> > +{
> > +       unsigned int clk, best_clk = 0;
> > +       unsigned int div, best_div = 0;
> > +       unsigned int m, best_m = 0;
> > +       unsigned int error;
> > +       unsigned int best_err = UINT_MAX;
> > +       u64 best_period = 0;
> > +
> > +       /*
> > +        * The PWM period is determined by:
> > +        *
> > +        *          resolution * pre_div * 2^M
> > +        * period = --------------------------
> > +        *                   refclk
> > +        *
> > +        * With resolution fixed at 2^9 bits, pre_div = {1, 3, 5, 6} and
> > +        * M = [0..7].
> > +        *
> > +        * This allows for periods between 27uS and 384s, as the PWM framework
> > +        * wants a period of equal or lower length than requested, reject
> > +        * anything below 27uS.
> > +        */
> 
> > +       if (period <= (u64)NSEC_PER_SEC * LPG_RESOLUTION / 19200000)
> > +               return -EINVAL;
> 
> > +       /* Limit period to largest possible value, to avoid overflows */
> > +       if (period > (u64)NSEC_PER_SEC * LPG_RESOLUTION * 6 * (1 << LPG_MAX_M) / 1024)
> > +               period = (u64)NSEC_PER_SEC * LPG_RESOLUTION * 6 * (1 << LPG_MAX_M) / 2014;
> 
> 2014?!
> 

I thought I fixed that...

> And if it's incorrect, it seems like a good example to avoid
> repetition of the long equations.
> 
> What about
> 
>   best_period = clamp_val(period, ...);
>   if (best_period >= period)
>     return -EINVAL;
> 
>   period = best_period;
> 
> ?

Sounds reasonable.

> 
> > +       /*
> > +        * Search for the pre_div, clk and M by solving the rewritten formula
> > +        * for each clk and pre_div value:
> > +        *
> > +        *                       period * clk
> > +        * M = log2 -------------------------------------
> > +        *           NSEC_PER_SEC * pre_div * resolution
> > +        */
> > +       for (clk = 0; clk < ARRAY_SIZE(lpg_clk_rates); clk++) {
> > +               u64 nom = period * lpg_clk_rates[clk];
> 
> Can we spell fully nunerator, denominator?
> 

Sure.

> > +               for (div = 0; div < ARRAY_SIZE(lpg_pre_divs); div++) {
> > +                       u64 denom = (u64)NSEC_PER_SEC * lpg_pre_divs[div] * (1 << 9);
> 
> " * (1 " part is redundant, you may shift left by 9, but see below.
> 

I could, but as written now it matches the formula as written in the
comment above. With (1 << 9) being the resolution part.

That said, I think I introduced the LPG_RESOLUTION constant after
writing this, would be reasonable to reuse that here.

> > +                       u64 actual;
> > +                       u64 ratio;
> > +
> > +                       if (nom < denom)
> > +                               continue;
> > +
> > +                       ratio = div64_u64(nom, denom);
> 
> Instead of shifting left by 9, you may optimize below to count that in
> the equations...
> 
> > +                       m = ilog2(ratio);
> > +                       if (m > LPG_MAX_M)
> > +                               m = LPG_MAX_M;
> 
> > +                       actual = DIV_ROUND_UP_ULL(denom * (1 << m), lpg_clk_rates[clk]);
> 
> ...including this one.
> 
> So, I see room for improvement in the calculations.
> 

So you're saying that I should remove the resolution from the
denominator and then just subtract 9 from M?

I presume it improves things by replacing one bitshift with a
subtraction, but afaict it wouldn't improve the readability of the code.

> > +                       error = period - actual;
> > +                       if (error < best_err) {
> > +                               best_err = error;
> > +
> > +                               best_div = div;
> > +                               best_m = m;
> > +                               best_clk = clk;
> > +                               best_period = actual;
> > +                       }
> > +               }
> > +       }
> > +
> > +       chan->clk = best_clk;
> > +       chan->pre_div = best_div;
> > +       chan->pre_div_exp = best_m;
> > +       chan->period = best_period;
> > +
> > +       return 0;
> > +}
> 
> ...
> 
> > +       val = div64_u64(duty * lpg_clk_rates[chan->clk],
> > +                       (u64)NSEC_PER_SEC * lpg_pre_divs[chan->pre_div] * (1 << chan->pre_div_exp));
> 
> For all code, just shift right directly, it makes code easier to read.
> 

Code might be easier to read, but as written now it matches the formula
described above.

You're right that we should get the same result if I replace the
multiplication from the denominator to be a shift in the numerator, but
at least for me that require me to think 1-2 extra steps when reading
this to convince myself that below is the same as the formula described
in the comments:

val = div64_u64((duty * lpg_clk_rates[chan->clk]) >> chan->pre_div_exp,
                (u64)NSEC_PER_SEC * lpg_pre_divs[chan->pre_div]);

> ...
> 
> > +       regmap_bulk_write(lpg->map, chan->base + PWM_VALUE_REG, &val, sizeof(val));
> 
> In some cases the error is handled from regmap calls, in many it's not. Why?
> 

The brightness_set() in struct led_classdev is a void function, so I
have to throw away the very unlikely error at some point...

> ...
> 
> > +       count = of_property_count_u32_elems(np, "qcom,dtest");
> > +       if (count == -EINVAL) {
> > +               return 0;
> 
> > +       } else if (count < 0) {
> 
> Redundant 'else'
> 
> > +               ret = count;
> 
> Do it other way around, i.e.
> 
>   ret = ...
>   ...
>   count = ret;
> 
> > +               goto err_malformed;
> > +       } else if (count != lpg->data->num_channels * 2) {
> 
> Redundant 'else'.
> 

So you're saying that this form is preferable?

if (a) {
	return 0;
}
if (b) {
	goto err_malformed:
}
if (c) {
	return -EINVAL;
}

The else has absolutely no meaning to the compiler, but it immediately
tells me as a human that we will enter only one of these branches.

> > +               dev_err(lpg->dev, "qcom,dtest needs to be %d items\n",
> > +                       lpg->data->num_channels * 2);
> > +               return -EINVAL;
> > +       }
> 
> ...
> 
> > +       /* Only support oneshot or indefinite loops, due to limited pattern space */
> 
> one shot
> 
> > +       if (repeat != -1 && repeat != 1)
> 
> abs(repeat) != 1 ?
> 

While equivalent, I'm not checking to see if the absolute value of
repeat is 1, I'm checking that repeat is either -1 and 1.

Again, same outcome but different meaning to a human reading the code.

> > +               return -EINVAL;
> 
> ...
> 
> > +       /* LPG_RAMP_DURATION_REG is 9 bit */
> > +       if (pattern[0].delta_t >= 512)
> 
> Then compare with bit value? BIT(9)?
> 
> > +               return -EINVAL;
> 
> ...
> 
> > +       lpg_brightness_single_set(cdev, LED_FULL);
> 
> Isn't LED_FULL deprecated?
> 

I had missed that the LED framework now supports variable
max_brightness. Will update accordingly throughout the driver.

> ...
> 
> > +       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);
> 
> Confusing message for some of the error conditions.
> 
> > +               return -EINVAL;
> 
> Shadowed error code.
> 
> > +       }
> 
> ...
> 
> > +       ret = of_property_read_u32(np, "color", &color);
> > +       if (ret < 0 && ret != -EINVAL) {
> 
> Why the specific error code check?
> 

Because color is an optional property, so -EINVAL would imply that we
didn't find the property and color was left untouched.

> > +               dev_err(lpg->dev, "failed to parse \"color\" of %pOF\n", np);
> > +               return ret;
> > +       }
> 
> ...
> 
> > +       if (!of_property_read_string(np, "default-state", &state) &&
> > +           !strcmp(state, "on"))
> 
> of_property_match_string()?
> 

Neat.

Regards,
Bjorn

> ...
> 
> > +       bitmap_size = BITS_TO_BYTES(lpg->lut_size);
> > +       lpg->lut_bitmap = devm_kzalloc(lpg->dev, bitmap_size, GFP_KERNEL);
> 
> devm_bitmap_zalloc()
> 
> > +       if (!lpg->lut_bitmap)
> > +               return -ENOMEM;
> 
> ...
> 
> > +               dev_err(&pdev->dev, "parent regmap unavailable\n");
> > +               return -ENXIO;
> 
> return dev_err_probe(...);
> 
> ...
> 
> > +       .pwm_9bit_mask = 3 << 4,
> 
> GENMASK()
> 
> ...
> 
> > +       .pwm_9bit_mask = 3 << 4,
> 
> Ditto.
> 
> --
> With Best Regards,
> Andy Shevchenko
Bjorn Andersson Feb. 2, 2022, 9:40 p.m. UTC | #6
On Wed 02 Feb 08:29 PST 2022, Uwe Kleine-K?nig wrote:

> Hello,
> 
> did you consider my earlier feedback "It would also be good if the PWM
> code could live in drivers/pwm"?
> (https://lore.kernel.org/r/20210505051958.e5lvwfxuo2skdu2q@pengutronix.de)
> 

Yes, I did consider this. Because the downstream driver is (at least was
when I looked at it originally) split like that.


We have a number of different Qualcomm PMICs containing the LPG modules,
which consists of N PWM channels, a pattern lookup table and a set of
current sinks.

Each PWM channel can either be used as a traditional PWM, a LED or be
grouped together with other channels to form a multicolor LED. So we
need a design that allows different boards using a particular PMIC to
freely use the N channels according to one of these three operational
modes.

The pattern lookup table is a shared resource containing duty cycle
values and each of the PWM channels can be configured to have their duty
cycle modified from the lookup table on some configured cadence.

In the even that multiple PWM channels are ganged together to form a
multicolor LED, which is driven by a pattern, the pattern generator for
the relevant channels needs to be synchronized.


If we consider the PWM channel to be the basic primitive we need some
mechanism to configure the pattern properties for each of the channels
and we need some mechanism to synchronize the pattern generators for
some subset of the PWM channels.


In other words we need some custom API between the LED driver part and
the PWM driver, to configure these properties. This was the design
of the downstream driver when I started looking at this driver.


Another alternative that has been considered is to create two
independent drivers - for the same hardware. This would allow the system
integrator to pick the right driver for each of the channels.

One problem with this strategy is that the DeviceTree description of the
LPG hardware will have to be modified depending on the use case. In
particular this prevents me from writing a platform dtsi describing the
LPG hardware and then describe the LEDs and pwm channels in a board dts.

And we can't express the individual channels, because the multicolor
definition needs to span multiple channels.


So among all the options, implementing the pwm_chip in the LED driver
makes it possible for us to describe the LPG as one entity, with
board-specific LEDs and a set of PWM channels.

> At least splitting in two patches would be good IMHO.
> 

I guess I can split out the parts related to the pwmchip in a separate
patch. Seems to be a rather small portion of the code though. Is that
what you have in mind?

> On Fri, Jan 28, 2022 at 04:54:29PM -0800, Bjorn Andersson wrote:
> > The Light Pulse Generator (LPG) is a PWM-block found in a wide range of
> > PMICs from Qualcomm. These PMICs typically comes with 1-8 LPG instances,
> > with their output being routed to various other components, such as
> > current sinks or GPIOs.
> > 
> > Each LPG instance can operate on fixed parameters or based on a shared
> > lookup-table, altering the duty cycle over time. This provides the means
> > for hardware assisted transitions of LED brightness.
> > 
> > A typical use case for the fixed parameter mode is to drive a PWM
> > backlight control signal, the driver therefor allows each LPG instance
> > to be exposed to the kernel either through the LED framework or the PWM
> > framework.
> > 
> > A typical use case for the LED configuration is to drive RGB LEDs in
> > smartphones etc, for which the driver support multiple channels to be
> > ganged up to a MULTICOLOR LED. In this configuration the pattern
> > generators will be synchronized, to allow for multi-color patterns.
> > 
> > Signed-off-by: Bjorn Andersson <bjorn.andersson@linaro.org>
> > ---
[..]
> > diff --git a/drivers/leds/rgb/leds-qcom-lpg.c b/drivers/leds/rgb/leds-qcom-lpg.c
[..]
> > +static int lpg_calc_freq(struct lpg_channel *chan, uint64_t period)
> > +{
> > +	unsigned int clk, best_clk = 0;
> > +	unsigned int div, best_div = 0;
> > +	unsigned int m, best_m = 0;
> > +	unsigned int error;
> > +	unsigned int best_err = UINT_MAX;
> > +	u64 best_period = 0;
> > +
> > +	/*
> > +	 * The PWM period is determined by:
> > +	 *
> > +	 *          resolution * pre_div * 2^M
> > +	 * period = --------------------------
> > +	 *                   refclk
> > +	 *
> > +	 * With resolution fixed at 2^9 bits, pre_div = {1, 3, 5, 6} and
> > +	 * M = [0..7].
> > +	 *
> > +	 * This allows for periods between 27uS and 384s, as the PWM framework
> > +	 * wants a period of equal or lower length than requested, reject
> > +	 * anything below 27uS.
> > +	 */
> > +	if (period <= (u64)NSEC_PER_SEC * LPG_RESOLUTION / 19200000)
> 
> u64 divisions must not be done by / in the kernel. Also I wonder if the
> following would be more correct (though with the same semantic):
> 
> 	if (period < DIV64_U64_ROUND_UP((u64)NSEC_PER_SEC * LPG_RESOLUTION, 19200000))
> 

Thanks for spotting that.

> 
> > +		return -EINVAL;
> > +
> > +	/* Limit period to largest possible value, to avoid overflows */
> > +	if (period > (u64)NSEC_PER_SEC * LPG_RESOLUTION * 6 * (1 << LPG_MAX_M) / 1024)
> > +		period = (u64)NSEC_PER_SEC * LPG_RESOLUTION * 6 * (1 << LPG_MAX_M) / 2014;
> > +
> > +	/*
> > +	 * Search for the pre_div, clk and M by solving the rewritten formula
> > +	 * for each clk and pre_div value:
> > +	 *
> > +	 *                       period * clk
> > +	 * M = log2 -------------------------------------
> > +	 *           NSEC_PER_SEC * pre_div * resolution
> > +	 */
> > +	for (clk = 0; clk < ARRAY_SIZE(lpg_clk_rates); clk++) {
> > +		u64 nom = period * lpg_clk_rates[clk];
> > +
> > +		for (div = 0; div < ARRAY_SIZE(lpg_pre_divs); div++) {
> > +			u64 denom = (u64)NSEC_PER_SEC * lpg_pre_divs[div] * (1 << 9);
> > +			u64 actual;
> > +			u64 ratio;
> > +
> > +			if (nom < denom)
> > +				continue;
> > +
> > +			ratio = div64_u64(nom, denom);
> > +			m = ilog2(ratio);
> > +			if (m > LPG_MAX_M)
> > +				m = LPG_MAX_M;
> > +
> > +			actual = DIV_ROUND_UP_ULL(denom * (1 << m), lpg_clk_rates[clk]);
> > +
> > +			error = period - actual;
> 
> This looks good, though I didn't revalidate the calculation (e.g. to
> convince myself that error is always >= 0)
> 

We spent considerable time going through this last time, so I hope we're
good :)

> > +			if (error < best_err) {
> > +				best_err = error;
> > +
> > +				best_div = div;
> > +				best_m = m;
> > +				best_clk = clk;
> > +				best_period = actual;
> > +			}
> > +		}
> > +	}
> > +
> > +	chan->clk = best_clk;
> > +	chan->pre_div = best_div;
> > +	chan->pre_div_exp = best_m;
> > +	chan->period = best_period;
> > +
> > +	return 0;
> > +}
[..]
> > +static int lpg_pwm_request(struct pwm_chip *chip, struct pwm_device *pwm)
> > +{
> > +	struct lpg *lpg = container_of(chip, struct lpg, pwm);
> > +	struct lpg_channel *chan = &lpg->channels[pwm->hwpwm];
> > +
> > +	return chan->in_use ? -EBUSY : 0;
> > +}
> > +
> > +/*
> > + * Limitations:
> > + * - Updating both duty and period is not done atomically, so the output signal
> > + *   will momentarily be a mix of the settings.
> 
> Is the PWM well-behaved? (i.e. does it emit the inactive level when
> disabled?)

Yes, a disabled channel outputs a logical 0.

> Does it complete a period before switching to the new
> setting?
> 

I see nothing indicating the answer to this, in either direction...

> Did you test with PWM_DEBUG enabled?
> 

For previous iterations of the patch yes, v11 didn't touch any of that
so I omitted that step... Will enable it again as I respin v12.

> > + */
> > +static int lpg_pwm_apply(struct pwm_chip *chip, struct pwm_device *pwm,
> > +			 const struct pwm_state *state)
> > +{
> > +	struct lpg *lpg = container_of(chip, struct lpg, pwm);
> > +	struct lpg_channel *chan = &lpg->channels[pwm->hwpwm];
> > +	int ret;
> > +
> > +	if (state->polarity != PWM_POLARITY_NORMAL)
> > +		return -EINVAL;
> > +
> > +	ret = lpg_calc_freq(chan, state->period);
> > +	if (ret < 0)
> > +		return ret;
> > +
> > +	lpg_calc_duty(chan, state->duty_cycle);
> > +	chan->enabled = state->enabled;
> > +
> > +	lpg_apply(chan);
> > +
> > +	triled_set(lpg, chan->triled_mask, chan->enabled ? chan->triled_mask : 0);
> > +
> > +	return 0;
> 
> Would it make sense to skip the calculation if state->enabled is false?
> 

Yes.

> > +}
> > +
> > +static void lpg_pwm_get_state(struct pwm_chip *chip, struct pwm_device *pwm,
> > +			      struct pwm_state *state)
> > +{
> > +	struct lpg *lpg = container_of(chip, struct lpg, pwm);
> > +	struct lpg_channel *chan = &lpg->channels[pwm->hwpwm];
> > +	u64 duty = DIV_ROUND_UP_ULL(chan->pwm_value * chan->period, LPG_RESOLUTION - 1);
> > +
> > +	state->period = chan->period;
> > +	state->duty_cycle = duty;
> > +	state->polarity = PWM_POLARITY_NORMAL;
> > +	state->enabled = chan->enabled;
> 
> This doesn't work if .get_state() is called before .apply() was called,
> does it?
> 

You mean that I would return some bogus state and not the actual
hardware state?

> > +}
> > +
> > +static const struct pwm_ops lpg_pwm_ops = {
> > +	.request = lpg_pwm_request,
> > +	.apply = lpg_pwm_apply,
> > +	.get_state = lpg_pwm_get_state,
> > +	.owner = THIS_MODULE,
> > +};
> > +
> > +static int lpg_add_pwm(struct lpg *lpg)
> > +{
> > +	int ret;
> > +
> > +	lpg->pwm.base = -1;
> 
> I already asked in May to drop this ...
> 

Sorry about that, thought I had resolved that already.

Thanks,
Bjorn

> > +	lpg->pwm.dev = lpg->dev;
> > +	lpg->pwm.npwm = lpg->num_channels;
> > +	lpg->pwm.ops = &lpg_pwm_ops;
> > +
> 
> Best regards
> Uwe
> 
> -- 
> Pengutronix e.K.                           | Uwe Kleine-König            |
> Industrial Linux Solutions                 | https://www.pengutronix.de/ |
Bjorn Andersson Feb. 2, 2022, 10:08 p.m. UTC | #7
On Wed, Feb 2, 2022 at 2:04 PM Bjorn Andersson
<bjorn.andersson@linaro.org> wrote:
>
> On Wed 02 Feb 03:18 PST 2022, Marijn Suijten wrote:
>
> > On 2022-01-28 16:54:29, Bjorn Andersson wrote:
> > > The Light Pulse Generator (LPG) is a PWM-block found in a wide range of
> > > PMICs from Qualcomm. These PMICs typically comes with 1-8 LPG instances,
> > > with their output being routed to various other components, such as
> > > current sinks or GPIOs.
> > >
> > > Each LPG instance can operate on fixed parameters or based on a shared
> > > lookup-table, altering the duty cycle over time. This provides the means
> > > for hardware assisted transitions of LED brightness.
> > >
> > > A typical use case for the fixed parameter mode is to drive a PWM
> > > backlight control signal, the driver therefor allows each LPG instance
> > > to be exposed to the kernel either through the LED framework or the PWM
> > > framework.
> > >
> > > A typical use case for the LED configuration is to drive RGB LEDs in
> > > smartphones etc, for which the driver support multiple channels to be
> > > ganged up to a MULTICOLOR LED. In this configuration the pattern
> > > generators will be synchronized, to allow for multi-color patterns.
> > >
> > > Signed-off-by: Bjorn Andersson <bjorn.andersson@linaro.org>
> >
> > Reviewed-by: Marijn Suijten <marijn.suijten@somainline.org>
> >
> > There may still be some things to improve based on whether lo_pause
> > works in non-ping-pong mode - to alleviate the requirement for the first
> > delta_t to be at most 512ms - but this patch should not be delayed much
> > longer and that's perhaps for a followup patch.  Same for my request for
> > documentation and examples which at the same time serve as some form of
> > tests to see if everything works as desired.
> >
>
> I've been considering lopause to be the value before we start the
> pattern, but I think you're right in that it denotes how long we should
> hold the first value.
>
> So I think it might make sense in the predefined "<value> <delay> <value>
> <delay>" scheme to use first <delay> as to calculate lo-pause. I think
> it has to be calculated, because the first value will iiuc be held
> for (lopause + 1) * delay ms.
>
> > I also vaguely remember other (downstream) drivers to support more than
> > 512ms per step by (drastically?) changing PWM period, but not sure how
> > that worked again nor if it was reliable.
> >
>
> Thinking about it again, while 512 is the 9th bit, we should be able to
> represent [0..1023] with 9 bits...
>

Sorry, my mind was elsewhere as I wrote that. [0..511] is what we got.

Regards,
Bjorn
Uwe Kleine-König Feb. 3, 2022, 8:31 a.m. UTC | #8
Hello Bjorn,

On Wed, Feb 02, 2022 at 01:40:21PM -0800, Bjorn Andersson wrote:
> On Wed 02 Feb 08:29 PST 2022, Uwe Kleine-K?nig wrote:
> > did you consider my earlier feedback "It would also be good if the PWM
> > code could live in drivers/pwm"?
> > (https://lore.kernel.org/r/20210505051958.e5lvwfxuo2skdu2q@pengutronix.de)
> 
> Yes, I did consider this. Because the downstream driver is (at least was
> when I looked at it originally) split like that.
> 
> We have a number of different Qualcomm PMICs containing the LPG modules,
> which consists of N PWM channels, a pattern lookup table and a set of
> current sinks.
> 
> Each PWM channel can either be used as a traditional PWM, a LED or be
> grouped together with other channels to form a multicolor LED. So we
> need a design that allows different boards using a particular PMIC to
> freely use the N channels according to one of these three operational
> modes.
> 
> The pattern lookup table is a shared resource containing duty cycle
> values and each of the PWM channels can be configured to have their duty
> cycle modified from the lookup table on some configured cadence.
> 
> In the even that multiple PWM channels are ganged together to form a
> multicolor LED, which is driven by a pattern, the pattern generator for
> the relevant channels needs to be synchronized.

Is this some material for the commit log to motivate the design
decision?

> If we consider the PWM channel to be the basic primitive we need some
> mechanism to configure the pattern properties for each of the channels
> and we need some mechanism to synchronize the pattern generators for
> some subset of the PWM channels.
> 
> 
> In other words we need some custom API between the LED driver part and
> the PWM driver, to configure these properties. This was the design
> of the downstream driver when I started looking at this driver.
> 
> 
> Another alternative that has been considered is to create two
> independent drivers - for the same hardware. This would allow the system
> integrator to pick the right driver for each of the channels.
> 
> One problem with this strategy is that the DeviceTree description of the
> LPG hardware will have to be modified depending on the use case. In
> particular this prevents me from writing a platform dtsi describing the
> LPG hardware and then describe the LEDs and pwm channels in a board dts.
> 
> And we can't express the individual channels, because the multicolor
> definition needs to span multiple channels.
> 
> 
> So among all the options, implementing the pwm_chip in the LED driver
> makes it possible for us to describe the LPG as one entity, with
> board-specific LEDs and a set of PWM channels.

ok.

> > At least splitting in two patches would be good IMHO.
> 
> I guess I can split out the parts related to the pwmchip in a separate
> patch. Seems to be a rather small portion of the code though. Is that
> what you have in mind?

I didn't try to understand the pattern part. I know that for PWMs there
is no pattern support, wasn't aware it's a thing for LEDs.

Anyhow, not a hard requirement to split from my side.

> > > +/*
> > > + * Limitations:
> > > + * - Updating both duty and period is not done atomically, so the output signal
> > > + *   will momentarily be a mix of the settings.
> > 
> > Is the PWM well-behaved? (i.e. does it emit the inactive level when
> > disabled?)
> 
> Yes, a disabled channel outputs a logical 0.

Please add this to the Limitations section. It's not actually a
limitation, but still this is a good place to put this information.

> > Does it complete a period before switching to the new
> > setting?
> 
> I see nothing indicating the answer to this, in either direction...

Can you test that? It's as easy as configuring a long period with 0%
relative duty cycle and then immediately a 100% relative duty cycle.

> > > +static void lpg_pwm_get_state(struct pwm_chip *chip, struct pwm_device *pwm,
> > > +			      struct pwm_state *state)
> > > +{
> > > +	struct lpg *lpg = container_of(chip, struct lpg, pwm);
> > > +	struct lpg_channel *chan = &lpg->channels[pwm->hwpwm];
> > > +	u64 duty = DIV_ROUND_UP_ULL(chan->pwm_value * chan->period, LPG_RESOLUTION - 1);
> > > +
> > > +	state->period = chan->period;
> > > +	state->duty_cycle = duty;
> > > +	state->polarity = PWM_POLARITY_NORMAL;
> > > +	state->enabled = chan->enabled;
> > 
> > This doesn't work if .get_state() is called before .apply() was called,
> > does it?
> > 
> 
> You mean that I would return some bogus state and not the actual
> hardware state?

Yes. At least I only found lpg_calc_freq() assigning chan->period and
chan->enabled. And unless I missed something this isn't called before
the pwm core calls .get_state().

> > > +}

Best regards
Uwe
Bjorn Andersson Feb. 3, 2022, 9:05 p.m. UTC | #9
On Thu 03 Feb 00:31 PST 2022, Uwe Kleine-K?nig wrote:

> Hello Bjorn,
> 
> On Wed, Feb 02, 2022 at 01:40:21PM -0800, Bjorn Andersson wrote:
> > On Wed 02 Feb 08:29 PST 2022, Uwe Kleine-K?nig wrote:
> > > did you consider my earlier feedback "It would also be good if the PWM
> > > code could live in drivers/pwm"?
> > > (https://lore.kernel.org/r/20210505051958.e5lvwfxuo2skdu2q@pengutronix.de)
> > 
> > Yes, I did consider this. Because the downstream driver is (at least was
> > when I looked at it originally) split like that.
> > 
> > We have a number of different Qualcomm PMICs containing the LPG modules,
> > which consists of N PWM channels, a pattern lookup table and a set of
> > current sinks.
> > 
> > Each PWM channel can either be used as a traditional PWM, a LED or be
> > grouped together with other channels to form a multicolor LED. So we
> > need a design that allows different boards using a particular PMIC to
> > freely use the N channels according to one of these three operational
> > modes.
> > 
> > The pattern lookup table is a shared resource containing duty cycle
> > values and each of the PWM channels can be configured to have their duty
> > cycle modified from the lookup table on some configured cadence.
> > 
> > In the even that multiple PWM channels are ganged together to form a
> > multicolor LED, which is driven by a pattern, the pattern generator for
> > the relevant channels needs to be synchronized.
> 
> Is this some material for the commit log to motivate the design
> decision?
> 

Sounds reasonable. I'll try to capture some of this background in the
commit message, for future reference.

> > If we consider the PWM channel to be the basic primitive we need some
> > mechanism to configure the pattern properties for each of the channels
> > and we need some mechanism to synchronize the pattern generators for
> > some subset of the PWM channels.
> > 
> > 
> > In other words we need some custom API between the LED driver part and
> > the PWM driver, to configure these properties. This was the design
> > of the downstream driver when I started looking at this driver.
> > 
> > 
> > Another alternative that has been considered is to create two
> > independent drivers - for the same hardware. This would allow the system
> > integrator to pick the right driver for each of the channels.
> > 
> > One problem with this strategy is that the DeviceTree description of the
> > LPG hardware will have to be modified depending on the use case. In
> > particular this prevents me from writing a platform dtsi describing the
> > LPG hardware and then describe the LEDs and pwm channels in a board dts.
> > 
> > And we can't express the individual channels, because the multicolor
> > definition needs to span multiple channels.
> > 
> > 
> > So among all the options, implementing the pwm_chip in the LED driver
> > makes it possible for us to describe the LPG as one entity, with
> > board-specific LEDs and a set of PWM channels.
> 
> ok.
> 
> > > At least splitting in two patches would be good IMHO.
> > 
> > I guess I can split out the parts related to the pwmchip in a separate
> > patch. Seems to be a rather small portion of the code though. Is that
> > what you have in mind?
> 
> I didn't try to understand the pattern part. I know that for PWMs there
> is no pattern support, wasn't aware it's a thing for LEDs.
> 

It allow you to have the hardware adjust the duty cycle over time, to
implement things such as fading or pulsing light effects - without
having to use the CPU.

> Anyhow, not a hard requirement to split from my side.
> 

Okay, cool.

> > > > +/*
> > > > + * Limitations:
> > > > + * - Updating both duty and period is not done atomically, so the output signal
> > > > + *   will momentarily be a mix of the settings.
> > > 
> > > Is the PWM well-behaved? (i.e. does it emit the inactive level when
> > > disabled?)
> > 
> > Yes, a disabled channel outputs a logical 0.
> 
> Please add this to the Limitations section. It's not actually a
> limitation, but still this is a good place to put this information.
> 
> > > Does it complete a period before switching to the new
> > > setting?
> > 
> > I see nothing indicating the answer to this, in either direction...
> 
> Can you test that? It's as easy as configuring a long period with 0%
> relative duty cycle and then immediately a 100% relative duty cycle.
> 

I will give it a try and see what I can deduce, and update the comment
accordingly.

> > > > +static void lpg_pwm_get_state(struct pwm_chip *chip, struct pwm_device *pwm,
> > > > +			      struct pwm_state *state)
> > > > +{
> > > > +	struct lpg *lpg = container_of(chip, struct lpg, pwm);
> > > > +	struct lpg_channel *chan = &lpg->channels[pwm->hwpwm];
> > > > +	u64 duty = DIV_ROUND_UP_ULL(chan->pwm_value * chan->period, LPG_RESOLUTION - 1);
> > > > +
> > > > +	state->period = chan->period;
> > > > +	state->duty_cycle = duty;
> > > > +	state->polarity = PWM_POLARITY_NORMAL;
> > > > +	state->enabled = chan->enabled;
> > > 
> > > This doesn't work if .get_state() is called before .apply() was called,
> > > does it?
> > > 
> > 
> > You mean that I would return some bogus state and not the actual
> > hardware state?
> 
> Yes. At least I only found lpg_calc_freq() assigning chan->period and
> chan->enabled. And unless I missed something this isn't called before
> the pwm core calls .get_state().
> 

Right, with a freshly probed driver I would return period = 0,
duty_cycle = 0/511 and enabled = 0.

Am I expected to instead return the hardware state, e.g. to recover
hardware initialization provided by the bootloader?

Thanks,
Bjorn

> > > > +}
> 
> Best regards
> Uwe
> 
> -- 
> Pengutronix e.K.                           | Uwe Kleine-König            |
> Industrial Linux Solutions                 | https://www.pengutronix.de/ |
Marijn Suijten Feb. 3, 2022, 11:20 p.m. UTC | #10
On 2022-02-02 16:08:29, Bjorn Andersson wrote:
> On Wed, Feb 2, 2022 at 2:04 PM Bjorn Andersson
> <bjorn.andersson@linaro.org> wrote:
> >
> > On Wed 02 Feb 03:18 PST 2022, Marijn Suijten wrote:
> >
> > > On 2022-01-28 16:54:29, Bjorn Andersson wrote:
> > > > The Light Pulse Generator (LPG) is a PWM-block found in a wide range of
> > > > PMICs from Qualcomm. These PMICs typically comes with 1-8 LPG instances,
> > > > with their output being routed to various other components, such as
> > > > current sinks or GPIOs.
> > > >
> > > > Each LPG instance can operate on fixed parameters or based on a shared
> > > > lookup-table, altering the duty cycle over time. This provides the means
> > > > for hardware assisted transitions of LED brightness.
> > > >
> > > > A typical use case for the fixed parameter mode is to drive a PWM
> > > > backlight control signal, the driver therefor allows each LPG instance
> > > > to be exposed to the kernel either through the LED framework or the PWM
> > > > framework.
> > > >
> > > > A typical use case for the LED configuration is to drive RGB LEDs in
> > > > smartphones etc, for which the driver support multiple channels to be
> > > > ganged up to a MULTICOLOR LED. In this configuration the pattern
> > > > generators will be synchronized, to allow for multi-color patterns.
> > > >
> > > > Signed-off-by: Bjorn Andersson <bjorn.andersson@linaro.org>
> > >
> > > Reviewed-by: Marijn Suijten <marijn.suijten@somainline.org>
> > >
> > > There may still be some things to improve based on whether lo_pause
> > > works in non-ping-pong mode - to alleviate the requirement for the first
> > > delta_t to be at most 512ms - but this patch should not be delayed much
> > > longer and that's perhaps for a followup patch.  Same for my request for
> > > documentation and examples which at the same time serve as some form of
> > > tests to see if everything works as desired.
> > >
> >
> > I've been considering lopause to be the value before we start the
> > pattern, but I think you're right in that it denotes how long we should
> > hold the first value.
> >
> > So I think it might make sense in the predefined "<value> <delay> <value>
> > <delay>" scheme to use first <delay> as to calculate lo-pause. I think
> > it has to be calculated, because the first value will iiuc be held
> > for (lopause + 1) * delay ms.

As mentioned in v10 that seems like a great idea, as long as we can
carefully validate and communicate these numbers to the user; both
through documentation and kernel error messages when values are
ultimately rejected.

Again, perhaps it might be better to postpone this to a separate
patchset as to not block the use of LPG for backlights which is arguably
more important than some fancy phone notification led patterns.

> > > I also vaguely remember other (downstream) drivers to support more than
> > > 512ms per step by (drastically?) changing PWM period, but not sure how
> > > that worked again nor if it was reliable.
> > >
> >
> > Thinking about it again, while 512 is the 9th bit, we should be able to
> > represent [0..1023] with 9 bits...
> >
> 
> Sorry, my mind was elsewhere as I wrote that. [0..511] is what we got.

Yes, 9 bits in total and BIT(8) being the highest settable ^^

- Marijn
diff mbox series

Patch

diff --git a/drivers/leds/Kconfig b/drivers/leds/Kconfig
index 6090e647daee..a49979f41eee 100644
--- a/drivers/leds/Kconfig
+++ b/drivers/leds/Kconfig
@@ -869,6 +869,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"
+
 comment "LED Triggers"
 source "drivers/leds/trigger/Kconfig"
 
diff --git a/drivers/leds/Makefile b/drivers/leds/Makefile
index e58ecb36360f..4fd2f92cd198 100644
--- a/drivers/leds/Makefile
+++ b/drivers/leds/Makefile
@@ -99,6 +99,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/
+
 # LED Triggers
 obj-$(CONFIG_LEDS_TRIGGERS)		+= trigger/
 
diff --git a/drivers/leds/rgb/Kconfig b/drivers/leds/rgb/Kconfig
new file mode 100644
index 000000000000..20be3e11fe4a
--- /dev/null
+++ b/drivers/leds/rgb/Kconfig
@@ -0,0 +1,13 @@ 
+# SPDX-License-Identifier: GPL-2.0
+
+if LEDS_CLASS_MULTICOLOR
+
+config LEDS_QCOM_LPG
+	tristate "LED support for Qualcomm LPG"
+	depends on OF
+	depends on SPMI
+	help
+	  This option enables support for the Light Pulse Generator found in a
+	  wide variety of Qualcomm PMICs.
+
+endif # LEDS_CLASS_MULTICOLOR
diff --git a/drivers/leds/rgb/Makefile b/drivers/leds/rgb/Makefile
new file mode 100644
index 000000000000..83114f44c4ea
--- /dev/null
+++ b/drivers/leds/rgb/Makefile
@@ -0,0 +1,3 @@ 
+# SPDX-License-Identifier: GPL-2.0
+
+obj-$(CONFIG_LEDS_QCOM_LPG)	+= leds-qcom-lpg.o
diff --git a/drivers/leds/rgb/leds-qcom-lpg.c b/drivers/leds/rgb/leds-qcom-lpg.c
new file mode 100644
index 000000000000..06d5fca1d4b5
--- /dev/null
+++ b/drivers/leds/rgb/leds-qcom-lpg.c
@@ -0,0 +1,1306 @@ 
+// SPDX-License-Identifier: GPL-2.0-only
+/*
+ * Copyright (c) 2017-2022 Linaro Ltd
+ * Copyright (c) 2010-2012, The Linux Foundation. All rights reserved.
+ */
+#include <linux/bits.h>
+#include <linux/led-class-multicolor.h>
+#include <linux/module.h>
+#include <linux/of.h>
+#include <linux/of_device.h>
+#include <linux/platform_device.h>
+#include <linux/pwm.h>
+#include <linux/regmap.h>
+#include <linux/slab.h>
+
+#define LPG_PATTERN_CONFIG_REG	0x40
+#define LPG_SIZE_CLK_REG	0x41
+#define LPG_PREDIV_CLK_REG	0x42
+#define PWM_TYPE_CONFIG_REG	0x43
+#define PWM_VALUE_REG		0x44
+#define PWM_ENABLE_CONTROL_REG	0x46
+#define PWM_SYNC_REG		0x47
+#define LPG_RAMP_DURATION_REG	0x50
+#define LPG_HI_PAUSE_REG	0x52
+#define LPG_LO_PAUSE_REG	0x54
+#define LPG_HI_IDX_REG		0x56
+#define LPG_LO_IDX_REG		0x57
+#define PWM_SEC_ACCESS_REG	0xd0
+#define PWM_DTEST_REG(x)	(0xe2 + (x) - 1)
+
+#define TRI_LED_SRC_SEL		0x45
+#define TRI_LED_EN_CTL		0x46
+#define TRI_LED_ATC_CTL		0x47
+
+#define LPG_LUT_REG(x)		(0x40 + (x) * 2)
+#define RAMP_CONTROL_REG	0xc8
+
+#define LPG_RESOLUTION		512
+#define LPG_MAX_M		7
+
+struct lpg_channel;
+struct lpg_data;
+
+/**
+ * struct lpg - LPG device context
+ * @dev:	struct device for LPG device
+ * @map:	regmap for register access
+ * @pwm:	PWM-chip object, if operating in PWM mode
+ * @data:	reference to version specific data
+ * @lut_base:	base address of the LUT block (optional)
+ * @lut_size:	number of entries in the LUT block
+ * @lut_bitmap:	allocation bitmap for LUT entries
+ * @triled_base: base address of the TRILED block (optional)
+ * @triled_src:	power-source for the TRILED
+ * @triled_has_atc_ctl:	true if there is TRI_LED_ATC_CTL register
+ * @triled_has_src_sel:	true if there is TRI_LED_SRC_SEL register
+ * @channels:	list of PWM channels
+ * @num_channels: number of @channels
+ */
+struct lpg {
+	struct device *dev;
+	struct regmap *map;
+
+	struct pwm_chip pwm;
+
+	const struct lpg_data *data;
+
+	u32 lut_base;
+	u32 lut_size;
+	unsigned long *lut_bitmap;
+
+	u32 triled_base;
+	u32 triled_src;
+	bool triled_has_atc_ctl;
+	bool triled_has_src_sel;
+
+	struct lpg_channel *channels;
+	unsigned int num_channels;
+};
+
+/**
+ * struct lpg_channel - per channel data
+ * @lpg:	reference to parent lpg
+ * @base:	base address of the PWM channel
+ * @triled_mask: mask in TRILED to enable this channel
+ * @lut_mask:	mask in LUT to start pattern generator for this channel
+ * @in_use:	channel is exposed to LED framework
+ * @color:	color of the LED attached to this channel
+ * @dtest_line:	DTEST line for output, or 0 if disabled
+ * @dtest_value: DTEST line configuration
+ * @pwm_value:	duty (in microseconds) of the generated pulses, overridden by LUT
+ * @enabled:	output enabled?
+ * @period:	period (in nanoseconds) of the generated pulses
+ * @clk:	base frequency of the clock generator
+ * @pre_div:	divider of @clk
+ * @pre_div_exp: exponential divider of @clk
+ * @ramp_enabled: duty cycle is driven by iterating over lookup table
+ * @ramp_ping_pong: reverse through pattern, rather than wrapping to start
+ * @ramp_oneshot: perform only a single pass over the pattern
+ * @ramp_reverse: iterate over pattern backwards
+ * @ramp_tick_ms: length (in milliseconds) of one step in the pattern
+ * @ramp_lo_pause_ms: pause (in milliseconds) before iterating over pattern
+ * @ramp_hi_pause_ms: pause (in milliseconds) after iterating over pattern
+ * @pattern_lo_idx: start index of associated pattern
+ * @pattern_hi_idx: last index of associated pattern
+ */
+struct lpg_channel {
+	struct lpg *lpg;
+
+	u32 base;
+	unsigned int triled_mask;
+	unsigned int lut_mask;
+
+	bool in_use;
+
+	int color;
+
+	u32 dtest_line;
+	u32 dtest_value;
+
+	u16 pwm_value;
+	bool enabled;
+
+	u64 period;
+	unsigned int clk;
+	unsigned int pre_div;
+	unsigned int pre_div_exp;
+
+	bool ramp_enabled;
+	bool ramp_ping_pong;
+	bool ramp_oneshot;
+	bool ramp_reverse;
+	unsigned short ramp_tick_ms;
+	unsigned long ramp_lo_pause_ms;
+	unsigned long ramp_hi_pause_ms;
+
+	unsigned int pattern_lo_idx;
+	unsigned int pattern_hi_idx;
+};
+
+/**
+ * struct lpg_led - logical LED object
+ * @lpg:		lpg context reference
+ * @cdev:		LED class device
+ * @mcdev:		Multicolor LED class device
+ * @num_channels:	number of @channels
+ * @channels:		list of channels associated with the LED
+ */
+struct lpg_led {
+	struct lpg *lpg;
+
+	struct led_classdev cdev;
+	struct led_classdev_mc mcdev;
+
+	unsigned int num_channels;
+	struct lpg_channel *channels[];
+};
+
+/**
+ * struct lpg_channel_data - per channel initialization data
+ * @base:		base address for PWM channel registers
+ * @triled_mask:	bitmask for controlling this channel in TRILED
+ */
+struct lpg_channel_data {
+	unsigned int base;
+	u8 triled_mask;
+};
+
+/**
+ * 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
+ * @triled_has_atc_ctl:	true if there is TRI_LED_ATC_CTL register
+ * @triled_has_src_sel:	true if there is TRI_LED_SRC_SEL register
+ * @pwm_9bit_mask:	bitmask for switching from 6bit to 9bit pwm
+ * @num_channels:	number of channels in LPG
+ * @channels:		list of channel initialization data
+ */
+struct lpg_data {
+	unsigned int lut_base;
+	unsigned int lut_size;
+	unsigned int triled_base;
+	bool triled_has_atc_ctl;
+	bool triled_has_src_sel;
+	unsigned int pwm_9bit_mask;
+	int num_channels;
+	const struct lpg_channel_data *channels;
+};
+
+static int triled_set(struct lpg *lpg, unsigned int mask, unsigned int enable)
+{
+	/* Skip if we don't have a triled block */
+	if (!lpg->triled_base)
+		return 0;
+
+	return regmap_update_bits(lpg->map, lpg->triled_base + TRI_LED_EN_CTL,
+				  mask, enable);
+}
+
+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, sizeof(val));
+	}
+
+	bitmap_set(lpg->lut_bitmap, idx, len);
+
+	*lo_idx = idx;
+	*hi_idx = idx + len - 1;
+
+	return 0;
+}
+
+static void lpg_lut_free(struct lpg *lpg, unsigned int lo_idx, unsigned int hi_idx)
+{
+	int len;
+
+	if (lo_idx == hi_idx)
+		return;
+
+	len = hi_idx - lo_idx + 1;
+	bitmap_clear(lpg->lut_bitmap, lo_idx, len);
+}
+
+static int lpg_lut_sync(struct lpg *lpg, unsigned int mask)
+{
+	return regmap_write(lpg->map, lpg->lut_base + RAMP_CONTROL_REG, mask);
+}
+
+static const unsigned int lpg_clk_rates[] = {1024, 32768, 19200000};
+static const unsigned int lpg_pre_divs[] = {1, 3, 5, 6};
+
+static int lpg_calc_freq(struct lpg_channel *chan, uint64_t period)
+{
+	unsigned int clk, best_clk = 0;
+	unsigned int div, best_div = 0;
+	unsigned int m, best_m = 0;
+	unsigned int error;
+	unsigned int best_err = UINT_MAX;
+	u64 best_period = 0;
+
+	/*
+	 * The PWM period is determined by:
+	 *
+	 *          resolution * pre_div * 2^M
+	 * period = --------------------------
+	 *                   refclk
+	 *
+	 * With resolution fixed at 2^9 bits, pre_div = {1, 3, 5, 6} and
+	 * M = [0..7].
+	 *
+	 * This allows for periods between 27uS and 384s, as the PWM framework
+	 * wants a period of equal or lower length than requested, reject
+	 * anything below 27uS.
+	 */
+	if (period <= (u64)NSEC_PER_SEC * LPG_RESOLUTION / 19200000)
+		return -EINVAL;
+
+	/* Limit period to largest possible value, to avoid overflows */
+	if (period > (u64)NSEC_PER_SEC * LPG_RESOLUTION * 6 * (1 << LPG_MAX_M) / 1024)
+		period = (u64)NSEC_PER_SEC * LPG_RESOLUTION * 6 * (1 << LPG_MAX_M) / 2014;
+
+	/*
+	 * Search for the pre_div, clk and M by solving the rewritten formula
+	 * for each clk and pre_div value:
+	 *
+	 *                       period * clk
+	 * M = log2 -------------------------------------
+	 *           NSEC_PER_SEC * pre_div * resolution
+	 */
+	for (clk = 0; clk < ARRAY_SIZE(lpg_clk_rates); clk++) {
+		u64 nom = period * lpg_clk_rates[clk];
+
+		for (div = 0; div < ARRAY_SIZE(lpg_pre_divs); div++) {
+			u64 denom = (u64)NSEC_PER_SEC * lpg_pre_divs[div] * (1 << 9);
+			u64 actual;
+			u64 ratio;
+
+			if (nom < denom)
+				continue;
+
+			ratio = div64_u64(nom, denom);
+			m = ilog2(ratio);
+			if (m > LPG_MAX_M)
+				m = LPG_MAX_M;
+
+			actual = DIV_ROUND_UP_ULL(denom * (1 << m), lpg_clk_rates[clk]);
+
+			error = period - actual;
+			if (error < best_err) {
+				best_err = error;
+
+				best_div = div;
+				best_m = m;
+				best_clk = clk;
+				best_period = actual;
+			}
+		}
+	}
+
+	chan->clk = best_clk;
+	chan->pre_div = best_div;
+	chan->pre_div_exp = best_m;
+	chan->period = best_period;
+
+	return 0;
+}
+
+static void lpg_calc_duty(struct lpg_channel *chan, uint64_t duty)
+{
+	unsigned int max = LPG_RESOLUTION - 1;
+	unsigned int val;
+
+	val = div64_u64(duty * lpg_clk_rates[chan->clk],
+			(u64)NSEC_PER_SEC * lpg_pre_divs[chan->pre_div] * (1 << chan->pre_div_exp));
+
+	chan->pwm_value = min(val, max);
+}
+
+static void lpg_apply_freq(struct lpg_channel *chan)
+{
+	unsigned long val;
+	struct lpg *lpg = chan->lpg;
+
+	if (!chan->enabled)
+		return;
+
+	/* Clock register values are off-by-one from lpg_clk_table */
+	val = chan->clk + 1;
+
+	/* Enable 9bit resolution */
+	val |= lpg->data->pwm_9bit_mask;
+
+	regmap_write(lpg->map, chan->base + LPG_SIZE_CLK_REG, val);
+
+	val = chan->pre_div << 5 | chan->pre_div_exp;
+	regmap_write(lpg->map, chan->base + LPG_PREDIV_CLK_REG, val);
+}
+
+#define LPG_ENABLE_GLITCH_REMOVAL	BIT(5)
+
+static void lpg_enable_glitch(struct lpg_channel *chan)
+{
+	struct lpg *lpg = chan->lpg;
+
+	regmap_update_bits(lpg->map, chan->base + PWM_TYPE_CONFIG_REG,
+			   LPG_ENABLE_GLITCH_REMOVAL, 0);
+}
+
+static void lpg_disable_glitch(struct lpg_channel *chan)
+{
+	struct lpg *lpg = chan->lpg;
+
+	regmap_update_bits(lpg->map, chan->base + PWM_TYPE_CONFIG_REG,
+			   LPG_ENABLE_GLITCH_REMOVAL,
+			   LPG_ENABLE_GLITCH_REMOVAL);
+}
+
+static void lpg_apply_pwm_value(struct lpg_channel *chan)
+{
+	struct lpg *lpg = chan->lpg;
+	u16 val = chan->pwm_value;
+
+	if (!chan->enabled)
+		return;
+
+	regmap_bulk_write(lpg->map, chan->base + PWM_VALUE_REG, &val, sizeof(val));
+}
+
+#define LPG_PATTERN_CONFIG_LO_TO_HI	BIT(4)
+#define LPG_PATTERN_CONFIG_REPEAT	BIT(3)
+#define LPG_PATTERN_CONFIG_TOGGLE	BIT(2)
+#define LPG_PATTERN_CONFIG_PAUSE_HI	BIT(1)
+#define LPG_PATTERN_CONFIG_PAUSE_LO	BIT(0)
+
+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 conf = 0;
+	unsigned int lo_idx = chan->pattern_lo_idx;
+	unsigned int hi_idx = chan->pattern_hi_idx;
+	u16 step = chan->ramp_tick_ms;
+
+	if (!chan->ramp_enabled || chan->pattern_lo_idx == chan->pattern_hi_idx)
+		return;
+
+	hi_pause = DIV_ROUND_UP(chan->ramp_hi_pause_ms, step);
+	lo_pause = DIV_ROUND_UP(chan->ramp_lo_pause_ms, step);
+
+	if (!chan->ramp_reverse)
+		conf |= LPG_PATTERN_CONFIG_LO_TO_HI;
+	if (!chan->ramp_oneshot)
+		conf |= LPG_PATTERN_CONFIG_REPEAT;
+	if (chan->ramp_ping_pong)
+		conf |= LPG_PATTERN_CONFIG_TOGGLE;
+	if (chan->ramp_hi_pause_ms)
+		conf |= LPG_PATTERN_CONFIG_PAUSE_HI;
+	if (chan->ramp_lo_pause_ms)
+		conf |= LPG_PATTERN_CONFIG_PAUSE_LO;
+
+	regmap_write(lpg->map, chan->base + LPG_PATTERN_CONFIG_REG, conf);
+	regmap_write(lpg->map, chan->base + LPG_HI_IDX_REG, hi_idx);
+	regmap_write(lpg->map, chan->base + LPG_LO_IDX_REG, lo_idx);
+
+	regmap_bulk_write(lpg->map, chan->base + LPG_RAMP_DURATION_REG, &step, sizeof(step));
+	regmap_write(lpg->map, chan->base + LPG_HI_PAUSE_REG, hi_pause);
+	regmap_write(lpg->map, chan->base + LPG_LO_PAUSE_REG, lo_pause);
+}
+
+#define LPG_ENABLE_CONTROL_OUTPUT		BIT(7)
+#define LPG_ENABLE_CONTROL_BUFFER_TRISTATE	BIT(5)
+#define LPG_ENABLE_CONTROL_SRC_PWM		BIT(2)
+#define LPG_ENABLE_CONTROL_RAMP_GEN		BIT(1)
+
+static void lpg_apply_control(struct lpg_channel *chan)
+{
+	unsigned int ctrl;
+	struct lpg *lpg = chan->lpg;
+
+	ctrl = LPG_ENABLE_CONTROL_BUFFER_TRISTATE;
+
+	if (chan->enabled)
+		ctrl |= LPG_ENABLE_CONTROL_OUTPUT;
+
+	if (chan->pattern_lo_idx != chan->pattern_hi_idx)
+		ctrl |= LPG_ENABLE_CONTROL_RAMP_GEN;
+	else
+		ctrl |= LPG_ENABLE_CONTROL_SRC_PWM;
+
+	regmap_write(lpg->map, chan->base + PWM_ENABLE_CONTROL_REG, ctrl);
+
+	/*
+	 * Due to LPG hardware bug, in the PWM mode, having enabled PWM,
+	 * We have to write PWM values one more time.
+	 */
+	if (chan->enabled)
+		lpg_apply_pwm_value(chan);
+}
+
+#define LPG_SYNC_PWM	BIT(0)
+
+static void lpg_apply_sync(struct lpg_channel *chan)
+{
+	struct lpg *lpg = chan->lpg;
+
+	regmap_write(lpg->map, chan->base + PWM_SYNC_REG, LPG_SYNC_PWM);
+}
+
+static int lpg_parse_dtest(struct lpg *lpg)
+{
+	struct lpg_channel *chan;
+	struct device_node *np = lpg->dev->of_node;
+	int count;
+	int ret;
+	int i;
+
+	count = of_property_count_u32_elems(np, "qcom,dtest");
+	if (count == -EINVAL) {
+		return 0;
+	} else if (count < 0) {
+		ret = count;
+		goto err_malformed;
+	} else if (count != lpg->data->num_channels * 2) {
+		dev_err(lpg->dev, "qcom,dtest needs to be %d items\n",
+			lpg->data->num_channels * 2);
+		return -EINVAL;
+	}
+
+	for (i = 0; i < lpg->data->num_channels; i++) {
+		chan = &lpg->channels[i];
+
+		ret = of_property_read_u32_index(np, "qcom,dtest", i * 2,
+						 &chan->dtest_line);
+		if (ret)
+			goto err_malformed;
+
+		ret = of_property_read_u32_index(np, "qcom,dtest", i * 2 + 1,
+						 &chan->dtest_value);
+		if (ret)
+			goto err_malformed;
+	}
+
+	return 0;
+
+err_malformed:
+	dev_err(lpg->dev, "malformed qcom,dtest\n");
+	return ret;
+}
+
+static void lpg_apply_dtest(struct lpg_channel *chan)
+{
+	struct lpg *lpg = chan->lpg;
+
+	if (!chan->dtest_line)
+		return;
+
+	regmap_write(lpg->map, chan->base + PWM_SEC_ACCESS_REG, 0xa5);
+	regmap_write(lpg->map, chan->base + PWM_DTEST_REG(chan->dtest_line),
+		     chan->dtest_value);
+}
+
+static void lpg_apply(struct lpg_channel *chan)
+{
+	lpg_disable_glitch(chan);
+	lpg_apply_freq(chan);
+	lpg_apply_pwm_value(chan);
+	lpg_apply_control(chan);
+	lpg_apply_sync(chan);
+	lpg_apply_lut_control(chan);
+	lpg_enable_glitch(chan);
+}
+
+static void lpg_brightness_set(struct lpg_led *led, struct led_classdev *cdev,
+			       struct mc_subled *subleds)
+{
+	enum led_brightness brightness;
+	struct lpg_channel *chan;
+	unsigned int triled_enabled = 0;
+	unsigned int triled_mask = 0;
+	unsigned int lut_mask = 0;
+	unsigned int duty;
+	struct lpg *lpg = led->lpg;
+	int i;
+
+	for (i = 0; i < led->num_channels; i++) {
+		chan = led->channels[i];
+		brightness = subleds[i].brightness;
+
+		if (brightness == LED_OFF) {
+			chan->enabled = false;
+			chan->ramp_enabled = false;
+		} else if (chan->pattern_lo_idx != chan->pattern_hi_idx) {
+			lpg_calc_freq(chan, NSEC_PER_MSEC);
+
+			chan->enabled = true;
+			chan->ramp_enabled = true;
+
+			lut_mask |= chan->lut_mask;
+			triled_enabled |= chan->triled_mask;
+		} else {
+			lpg_calc_freq(chan, NSEC_PER_MSEC);
+
+			duty = div_u64(brightness * chan->period, cdev->max_brightness);
+			lpg_calc_duty(chan, duty);
+			chan->enabled = true;
+			chan->ramp_enabled = false;
+
+			triled_enabled |= chan->triled_mask;
+		}
+
+		triled_mask |= chan->triled_mask;
+
+		lpg_apply(chan);
+	}
+
+	/* Toggle triled lines */
+	if (triled_mask)
+		triled_set(lpg, triled_mask, triled_enabled);
+
+	/* Trigger start of ramp generator(s) */
+	if (lut_mask)
+		lpg_lut_sync(lpg, lut_mask);
+}
+
+static void lpg_brightness_single_set(struct led_classdev *cdev,
+				      enum led_brightness value)
+{
+	struct lpg_led *led = container_of(cdev, struct lpg_led, cdev);
+	struct mc_subled info;
+
+	info.brightness = value;
+	lpg_brightness_set(led, cdev, &info);
+}
+
+static void lpg_brightness_mc_set(struct led_classdev *cdev,
+				  enum led_brightness value)
+{
+	struct led_classdev_mc *mc = lcdev_to_mccdev(cdev);
+	struct lpg_led *led = container_of(mc, struct lpg_led, mcdev);
+
+	led_mc_calc_color_components(mc, value);
+	lpg_brightness_set(led, cdev, mc->subled_info);
+}
+
+static int lpg_blink_set(struct lpg_led *led,
+			 unsigned long *delay_on, unsigned long *delay_off)
+{
+	struct lpg_channel *chan;
+	unsigned int period;
+	unsigned int triled_mask = 0;
+	struct lpg *lpg = led->lpg;
+	u64 duty;
+	int i;
+
+	if (!*delay_on && !*delay_off) {
+		*delay_on = 500;
+		*delay_off = 500;
+	}
+
+	duty = *delay_on * NSEC_PER_MSEC;
+	period = (*delay_on + *delay_off) * NSEC_PER_MSEC;
+
+	for (i = 0; i < led->num_channels; i++) {
+		chan = led->channels[i];
+
+		lpg_calc_freq(chan, period);
+		lpg_calc_duty(chan, duty);
+
+		chan->enabled = true;
+		chan->ramp_enabled = false;
+
+		triled_mask |= chan->triled_mask;
+
+		lpg_apply(chan);
+	}
+
+	/* Enable triled lines */
+	triled_set(lpg, triled_mask, triled_mask);
+
+	chan = led->channels[0];
+	duty = div_u64(chan->pwm_value * chan->period, LPG_RESOLUTION);
+	*delay_on = div_u64(duty, NSEC_PER_MSEC);
+	*delay_off = div_u64(chan->period - duty, NSEC_PER_MSEC);
+
+	return 0;
+}
+
+static int lpg_blink_single_set(struct led_classdev *cdev,
+				unsigned long *delay_on, unsigned long *delay_off)
+{
+	struct lpg_led *led = container_of(cdev, struct lpg_led, cdev);
+
+	return lpg_blink_set(led, delay_on, delay_off);
+}
+
+static int lpg_blink_mc_set(struct led_classdev *cdev,
+			    unsigned long *delay_on, unsigned long *delay_off)
+{
+	struct led_classdev_mc *mc = lcdev_to_mccdev(cdev);
+	struct lpg_led *led = container_of(mc, struct lpg_led, mcdev);
+
+	return lpg_blink_set(led, delay_on, delay_off);
+}
+
+static int lpg_pattern_set(struct lpg_led *led, struct led_pattern *pattern,
+			   u32 len, int repeat)
+{
+	struct lpg_channel *chan;
+	struct lpg *lpg = led->lpg;
+	unsigned int hi_pause;
+	unsigned int lo_pause;
+	unsigned int lo_idx;
+	unsigned int hi_idx;
+	bool ping_pong = true;
+	int brightness_a;
+	int brightness_b;
+	int ret;
+	int i;
+
+	/* Only support oneshot or indefinite loops, due to limited pattern space */
+	if (repeat != -1 && repeat != 1)
+		return -EINVAL;
+
+	/* LPG_RAMP_DURATION_REG is 9 bit */
+	if (pattern[0].delta_t >= 512)
+		return -EINVAL;
+
+	/*
+	 * The LPG plays patterns with at a fixed pace, a "low pause" can be
+	 * performed before the pattern and a "high pause" after. In order to
+	 * save space the pattern can be played in "ping pong" mode, in which
+	 * the pattern is first played forward, then "high pause" is applied,
+	 * then the pattern is played backwards and finally the "low pause" is
+	 * applied.
+	 *
+	 * The delta_t of the first entry is used to determine the pace of the
+	 * pattern.
+	 *
+	 * If the specified pattern is a palindrome the ping pong mode is
+	 * enabled. In this scenario the delta_t of the last entry determines
+	 * the "low pause" time and the delta_t of the middle entry (i.e. the
+	 * last in the programmed pattern) determines the "high pause". If the
+	 * pattern consists of an odd number of values, no "high pause" is
+	 * used.
+	 *
+	 * When ping pong mode is not selected, the delta_t of the last entry
+	 * is used as "high pause". No "low pause" is used.
+	 *
+	 * delta_t of any other members of the pattern is ignored.
+	 */
+
+	/* Detect palindromes and use "ping pong" to reduce LUT usage */
+	for (i = 0; i < len / 2; i++) {
+		brightness_a = pattern[i].brightness;
+		brightness_b = pattern[len - i - 1].brightness;
+
+		if (brightness_a != brightness_b) {
+			ping_pong = false;
+			break;
+		}
+	}
+
+	if (ping_pong) {
+		if (len % 2)
+			hi_pause = 0;
+		else
+			hi_pause = pattern[(len + 1) / 2].delta_t;
+		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)
+		return ret;
+
+	for (i = 0; i < led->num_channels; i++) {
+		chan = led->channels[i];
+
+		chan->ramp_tick_ms = pattern[0].delta_t;
+		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;
+	}
+
+	return 0;
+}
+
+static int lpg_pattern_single_set(struct led_classdev *cdev,
+				  struct led_pattern *pattern, u32 len,
+				  int repeat)
+{
+	struct lpg_led *led = container_of(cdev, struct lpg_led, cdev);
+	int ret;
+
+	ret = lpg_pattern_set(led, pattern, len, repeat);
+	if (ret < 0)
+		return ret;
+
+	lpg_brightness_single_set(cdev, LED_FULL);
+
+	return 0;
+}
+
+static int lpg_pattern_mc_set(struct led_classdev *cdev,
+			      struct led_pattern *pattern, u32 len,
+			      int repeat)
+{
+	struct led_classdev_mc *mc = lcdev_to_mccdev(cdev);
+	struct lpg_led *led = container_of(mc, struct lpg_led, mcdev);
+	int ret;
+
+	ret = lpg_pattern_set(led, pattern, len, repeat);
+	if (ret < 0)
+		return ret;
+
+	led_mc_calc_color_components(mc, LED_FULL);
+	lpg_brightness_set(led, cdev, mc->subled_info);
+
+	return 0;
+}
+
+static int lpg_pattern_clear(struct lpg_led *led)
+{
+	struct lpg_channel *chan;
+	struct lpg *lpg = led->lpg;
+	int i;
+
+	chan = led->channels[0];
+	lpg_lut_free(lpg, chan->pattern_lo_idx, chan->pattern_hi_idx);
+
+	for (i = 0; i < led->num_channels; i++) {
+		chan = led->channels[i];
+		chan->pattern_lo_idx = 0;
+		chan->pattern_hi_idx = 0;
+	}
+
+	return 0;
+}
+
+static int lpg_pattern_single_clear(struct led_classdev *cdev)
+{
+	struct lpg_led *led = container_of(cdev, struct lpg_led, cdev);
+
+	return lpg_pattern_clear(led);
+}
+
+static int lpg_pattern_mc_clear(struct led_classdev *cdev)
+{
+	struct led_classdev_mc *mc = lcdev_to_mccdev(cdev);
+	struct lpg_led *led = container_of(mc, struct lpg_led, mcdev);
+
+	return lpg_pattern_clear(led);
+}
+
+static int lpg_pwm_request(struct pwm_chip *chip, struct pwm_device *pwm)
+{
+	struct lpg *lpg = container_of(chip, struct lpg, pwm);
+	struct lpg_channel *chan = &lpg->channels[pwm->hwpwm];
+
+	return chan->in_use ? -EBUSY : 0;
+}
+
+/*
+ * Limitations:
+ * - Updating both duty and period is not done atomically, so the output signal
+ *   will momentarily be a mix of the settings.
+ */
+static int lpg_pwm_apply(struct pwm_chip *chip, struct pwm_device *pwm,
+			 const struct pwm_state *state)
+{
+	struct lpg *lpg = container_of(chip, struct lpg, pwm);
+	struct lpg_channel *chan = &lpg->channels[pwm->hwpwm];
+	int ret;
+
+	if (state->polarity != PWM_POLARITY_NORMAL)
+		return -EINVAL;
+
+	ret = lpg_calc_freq(chan, state->period);
+	if (ret < 0)
+		return ret;
+
+	lpg_calc_duty(chan, state->duty_cycle);
+	chan->enabled = state->enabled;
+
+	lpg_apply(chan);
+
+	triled_set(lpg, chan->triled_mask, chan->enabled ? chan->triled_mask : 0);
+
+	return 0;
+}
+
+static void lpg_pwm_get_state(struct pwm_chip *chip, struct pwm_device *pwm,
+			      struct pwm_state *state)
+{
+	struct lpg *lpg = container_of(chip, struct lpg, pwm);
+	struct lpg_channel *chan = &lpg->channels[pwm->hwpwm];
+	u64 duty = DIV_ROUND_UP_ULL(chan->pwm_value * chan->period, LPG_RESOLUTION - 1);
+
+	state->period = chan->period;
+	state->duty_cycle = duty;
+	state->polarity = PWM_POLARITY_NORMAL;
+	state->enabled = chan->enabled;
+}
+
+static const struct pwm_ops lpg_pwm_ops = {
+	.request = lpg_pwm_request,
+	.apply = lpg_pwm_apply,
+	.get_state = lpg_pwm_get_state,
+	.owner = THIS_MODULE,
+};
+
+static int lpg_add_pwm(struct lpg *lpg)
+{
+	int ret;
+
+	lpg->pwm.base = -1;
+	lpg->pwm.dev = lpg->dev;
+	lpg->pwm.npwm = lpg->num_channels;
+	lpg->pwm.ops = &lpg_pwm_ops;
+
+	ret = pwmchip_add(&lpg->pwm);
+	if (ret)
+		dev_err(lpg->dev, "failed to add PWM chip: ret %d\n", ret);
+
+	return ret;
+}
+
+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);
+		return -EINVAL;
+	}
+
+	chan = &lpg->channels[reg - 1];
+	chan->in_use = true;
+
+	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;
+	}
+
+	chan->color = color;
+
+	*channel = chan;
+
+	return 0;
+}
+
+static int lpg_add_led(struct lpg *lpg, struct device_node *np)
+{
+	struct led_init_data init_data = {};
+	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_RGB)
+		num_channels = of_get_available_child_count(np);
+	else
+		num_channels = 1;
+
+	led = devm_kzalloc(lpg->dev, struct_size(led, channels, num_channels), GFP_KERNEL);
+	if (!led)
+		return -ENOMEM;
+
+	led->lpg = lpg;
+	led->num_channels = num_channels;
+
+	if (color == LED_COLOR_ID_RGB) {
+		info = devm_kcalloc(lpg->dev, num_channels, sizeof(*info), GFP_KERNEL);
+		if (!info)
+			return -ENOMEM;
+		i = 0;
+		for_each_available_child_of_node(np, child) {
+			ret = lpg_parse_channel(lpg, child, &led->channels[i]);
+			if (ret < 0)
+				return ret;
+
+			info[i].color_index = led->channels[i]->color;
+			info[i].intensity = LED_FULL;
+			i++;
+		}
+
+		led->mcdev.subled_info = info;
+		led->mcdev.num_colors = num_channels;
+
+		cdev = &led->mcdev.led_cdev;
+		cdev->brightness_set = lpg_brightness_mc_set;
+		cdev->blink_set = lpg_blink_mc_set;
+
+		/* Register pattern accessors only if we have a LUT block */
+		if (lpg->lut_base) {
+			cdev->pattern_set = lpg_pattern_mc_set;
+			cdev->pattern_clear = lpg_pattern_mc_clear;
+		}
+	} else {
+		ret = lpg_parse_channel(lpg, np, &led->channels[0]);
+		if (ret < 0)
+			return ret;
+
+		cdev = &led->cdev;
+		cdev->brightness_set = lpg_brightness_single_set;
+		cdev->blink_set = lpg_blink_single_set;
+
+		/* Register pattern accessors only if we have a LUT block */
+		if (lpg->lut_base) {
+			cdev->pattern_set = lpg_pattern_single_set;
+			cdev->pattern_clear = lpg_pattern_single_clear;
+		}
+	}
+
+	cdev->default_trigger = of_get_property(np, "linux,default-trigger", NULL);
+	cdev->max_brightness = 255;
+
+	if (!of_property_read_string(np, "default-state", &state) &&
+	    !strcmp(state, "on"))
+		cdev->brightness = LED_FULL;
+	else
+		cdev->brightness = LED_OFF;
+
+	cdev->brightness_set(cdev, cdev->brightness);
+
+	init_data.fwnode = of_fwnode_handle(np);
+
+	if (color == LED_COLOR_ID_RGB)
+		ret = devm_led_classdev_multicolor_register_ext(lpg->dev, &led->mcdev, &init_data);
+	else
+		ret = devm_led_classdev_register_ext(lpg->dev, &led->cdev, &init_data);
+	if (ret)
+		dev_err(lpg->dev, "unable to register %s\n", cdev->name);
+
+	return ret;
+}
+
+static int lpg_init_channels(struct lpg *lpg)
+{
+	const struct lpg_data *data = lpg->data;
+	int i;
+
+	lpg->num_channels = data->num_channels;
+	lpg->channels = devm_kcalloc(lpg->dev, data->num_channels,
+				     sizeof(struct lpg_channel), GFP_KERNEL);
+	if (!lpg->channels)
+		return -ENOMEM;
+
+	for (i = 0; i < data->num_channels; i++) {
+		lpg->channels[i].lpg = lpg;
+		lpg->channels[i].base = data->channels[i].base;
+		lpg->channels[i].triled_mask = data->channels[i].triled_mask;
+		lpg->channels[i].lut_mask = BIT(i);
+	}
+
+	return 0;
+}
+
+static int lpg_init_triled(struct lpg *lpg)
+{
+	struct device_node *np = lpg->dev->of_node;
+	int ret;
+
+	/* Skip initialization if we don't have a triled block */
+	if (!lpg->data->triled_base)
+		return 0;
+
+	lpg->triled_base = lpg->data->triled_base;
+	lpg->triled_has_atc_ctl = lpg->data->triled_has_atc_ctl;
+	lpg->triled_has_src_sel = lpg->data->triled_has_src_sel;
+
+	if (lpg->triled_has_src_sel) {
+		ret = of_property_read_u32(np, "qcom,power-source", &lpg->triled_src);
+		if (ret || lpg->triled_src == 2 || lpg->triled_src > 3) {
+			dev_err(lpg->dev, "invalid power source\n");
+			return -EINVAL;
+		}
+	}
+
+	/* Disable automatic trickle charge LED */
+	if (lpg->triled_has_atc_ctl)
+		regmap_write(lpg->map, lpg->triled_base + TRI_LED_ATC_CTL, 0);
+
+	/* Configure power source */
+	if (lpg->triled_has_src_sel)
+		regmap_write(lpg->map, lpg->triled_base + TRI_LED_SRC_SEL, lpg->triled_src);
+
+	/* Default all outputs to off */
+	regmap_write(lpg->map, lpg->triled_base + TRI_LED_EN_CTL, 0);
+
+	return 0;
+}
+
+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;
+
+	return 0;
+}
+
+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;
+
+	platform_set_drvdata(pdev, lpg);
+
+	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_parse_dtest(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;
+
+	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]);
+
+	return lpg_add_pwm(lpg);
+}
+
+static int lpg_remove(struct platform_device *pdev)
+{
+	struct lpg *lpg = platform_get_drvdata(pdev);
+
+	pwmchip_remove(&lpg->pwm);
+
+	return 0;
+}
+
+static const struct lpg_data pm8916_pwm_data = {
+	.pwm_9bit_mask = BIT(2),
+
+	.num_channels = 1,
+	.channels = (const struct lpg_channel_data[]) {
+		{ .base = 0xbc00 },
+	},
+};
+
+static const struct lpg_data pm8941_lpg_data = {
+	.lut_base = 0xb000,
+	.lut_size = 64,
+
+	.triled_base = 0xd000,
+	.triled_has_atc_ctl = true,
+	.triled_has_src_sel = true,
+
+	.pwm_9bit_mask = 3 << 4,
+
+	.num_channels = 8,
+	.channels = (const struct lpg_channel_data[]) {
+		{ .base = 0xb100 },
+		{ .base = 0xb200 },
+		{ .base = 0xb300 },
+		{ .base = 0xb400 },
+		{ .base = 0xb500, .triled_mask = BIT(5) },
+		{ .base = 0xb600, .triled_mask = BIT(6) },
+		{ .base = 0xb700, .triled_mask = BIT(7) },
+		{ .base = 0xb800 },
+	},
+};
+
+static const struct lpg_data pm8994_lpg_data = {
+	.lut_base = 0xb000,
+	.lut_size = 64,
+
+	.pwm_9bit_mask = 3 << 4,
+
+	.num_channels = 6,
+	.channels = (const struct lpg_channel_data[]) {
+		{ .base = 0xb100 },
+		{ .base = 0xb200 },
+		{ .base = 0xb300 },
+		{ .base = 0xb400 },
+		{ .base = 0xb500 },
+		{ .base = 0xb600 },
+	},
+};
+
+static const struct lpg_data pmi8994_lpg_data = {
+	.lut_base = 0xb000,
+	.lut_size = 24,
+
+	.triled_base = 0xd000,
+	.triled_has_atc_ctl = true,
+	.triled_has_src_sel = true,
+
+	.pwm_9bit_mask = BIT(4),
+
+	.num_channels = 4,
+	.channels = (const struct lpg_channel_data[]) {
+		{ .base = 0xb100, .triled_mask = BIT(5) },
+		{ .base = 0xb200, .triled_mask = BIT(6) },
+		{ .base = 0xb300, .triled_mask = BIT(7) },
+		{ .base = 0xb400 },
+	},
+};
+
+static const struct lpg_data pmi8998_lpg_data = {
+	.lut_base = 0xb000,
+	.lut_size = 49,
+
+	.triled_base = 0xd000,
+
+	.pwm_9bit_mask = BIT(4),
+
+	.num_channels = 6,
+	.channels = (const struct lpg_channel_data[]) {
+		{ .base = 0xb100 },
+		{ .base = 0xb200 },
+		{ .base = 0xb300, .triled_mask = BIT(5) },
+		{ .base = 0xb400, .triled_mask = BIT(6) },
+		{ .base = 0xb500, .triled_mask = BIT(7) },
+		{ .base = 0xb600 },
+	},
+};
+
+static const struct lpg_data pm8150b_lpg_data = {
+	.lut_base = 0xb000,
+	.lut_size = 24,
+
+	.triled_base = 0xd000,
+
+	.pwm_9bit_mask = BIT(4),
+
+	.num_channels = 2,
+	.channels = (const struct lpg_channel_data[]) {
+		{ .base = 0xb100, .triled_mask = BIT(7) },
+		{ .base = 0xb200, .triled_mask = BIT(6) },
+	},
+};
+
+static const struct lpg_data pm8150l_lpg_data = {
+	.lut_base = 0xb000,
+	.lut_size = 48,
+
+	.triled_base = 0xd000,
+
+	.pwm_9bit_mask = BIT(4),
+
+	.num_channels = 5,
+	.channels = (const struct lpg_channel_data[]) {
+		{ .base = 0xb100, .triled_mask = BIT(7) },
+		{ .base = 0xb200, .triled_mask = BIT(6) },
+		{ .base = 0xb300, .triled_mask = BIT(5) },
+		{ .base = 0xbc00 },
+		{ .base = 0xbd00 },
+
+	},
+};
+
+static const struct of_device_id lpg_of_table[] = {
+	{ .compatible = "qcom,pm8150b-lpg", .data = &pm8150b_lpg_data },
+	{ .compatible = "qcom,pm8150l-lpg", .data = &pm8150l_lpg_data },
+	{ .compatible = "qcom,pm8916-pwm", .data = &pm8916_pwm_data },
+	{ .compatible = "qcom,pm8941-lpg", .data = &pm8941_lpg_data },
+	{ .compatible = "qcom,pm8994-lpg", .data = &pm8994_lpg_data },
+	{ .compatible = "qcom,pmi8994-lpg", .data = &pmi8994_lpg_data },
+	{ .compatible = "qcom,pmi8998-lpg", .data = &pmi8998_lpg_data },
+	{ .compatible = "qcom,pmc8180c-lpg", .data = &pm8150l_lpg_data },
+	{}
+};
+MODULE_DEVICE_TABLE(of, lpg_of_table);
+
+static struct platform_driver lpg_driver = {
+	.probe = lpg_probe,
+	.remove = lpg_remove,
+	.driver = {
+		.name = "qcom-spmi-lpg",
+		.of_match_table = lpg_of_table,
+	},
+};
+module_platform_driver(lpg_driver);
+
+MODULE_DESCRIPTION("Qualcomm LPG LED driver");
+MODULE_LICENSE("GPL v2");