[PATCH v5 0/4] Qualcomm Light Pulse Generator

[PATCH v5 0/4] Qualcomm Light Pulse Generator

[Bjorn Andersson]

This series introduces a generic pattern interface in the LED class and
a driver for the Qualcomm Light Pulse Generator.

It seems like it's been almost 3 years since I posted v3, which was hung
up on the lack of conclusion on the hw_pattern and multicolor support.
Now that those are concluded I hope we can make some progress on the LPG
support again.

The dts patches are included in the series as "examples", ultimately my
expectation is that the dt binding and driver patches are picked up
through the leds tree, while Andy or myself take the dts patches.

Bjorn Andersson (4):
  dt-bindings: leds: Add Qualcomm Light Pulse Generator binding
  leds: Add driver for Qualcomm LPG
  arm64: dts: qcom: pm(i)8994: Add mpp and lpg blocks
  arm64: dts: qcom: Add user LEDs on db820c

 .../bindings/leds/leds-qcom-lpg.yaml          |  170 +++
 arch/arm64/boot/dts/qcom/apq8096-db820c.dtsi  |   49 +
 arch/arm64/boot/dts/qcom/pm8994.dtsi          |    9 +
 arch/arm64/boot/dts/qcom/pmi8994.dtsi         |   20 +
 drivers/leds/Kconfig                          |    9 +
 drivers/leds/Makefile                         |    1 +
 drivers/leds/leds-qcom-lpg.c                  | 1206 +++++++++++++++++
 7 files changed, 1464 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/leds/leds-qcom-lpg.yaml
 create mode 100644 drivers/leds/leds-qcom-lpg.c

-- 
2.28.0

[PATCH v5 1/4] dt-bindings: leds: Add Qualcomm Light Pulse Generator binding

[Bjorn Andersson]

This adds the binding document describing the three hardware blocks
related to the Light Pulse Generator found in a wide range of Qualcomm
PMICs.

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

Changes since v4:
- Dropped quotes around power-source
- Moved "multi-led" to properties
- Corrected tab-indented line in example

 .../bindings/leds/leds-qcom-lpg.yaml          | 170 ++++++++++++++++++
 1 file changed, 170 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/leds/leds-qcom-lpg.yaml

diff --git a/Documentation/devicetree/bindings/leds/leds-qcom-lpg.yaml b/Documentation/devicetree/bindings/leds/leds-qcom-lpg.yaml
new file mode 100644
index 000000000000..5ccf0f3d8f1b
--- /dev/null
+++ b/Documentation/devicetree/bindings/leds/leds-qcom-lpg.yaml
@@ -0,0 +1,170 @@
+# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/leds/leds-qcom-lpg.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: Qualcomm Light Pulse Generator
+
+maintainers:
+  - Bjorn Andersson <bjorn.andersson@linaro.org>
+
+description: >
+  The Qualcomm Light Pulse Generator consists of three different hardware blocks;
+  a ramp generator with lookup table, the light pulse generator and a three
+  channel current sink. These blocks are found in a wide range of Qualcomm PMICs.
+
+properties:
+  compatible:
+    enum:
+      - qcom,pm8916-pwm
+      - qcom,pm8941-lpg
+      - qcom,pm8994-lpg
+      - qcom,pmi8994-lpg
+      - qcom,pmi8998-lpg
+
+  "#pwm-cells":
+    const: 2
+
+  "#address-cells":
+    const: 1
+
+  "#size-cells":
+    const: 0
+
+  qcom,power-source:
+    $ref: /schemas/types.yaml#definitions/uint32
+    description: >
+      power-source used to drive the output, as defined in the datasheet.
+      Should be specified if the TRILED block is present
+    enum:
+      - 0
+      - 1
+      - 3
+
+  multi-led:
+    type: object
+    $ref: leds-class-multicolor.yaml#
+    properties:
+      "#address-cells":
+        const: 1
+
+      "#size-cells":
+        const: 0
+
+      "^led@[0-9a-f]$":
+        type: object
+        $ref: common.yaml#
+
+        properties:
+          "qcom,dtest":
+            $ref: /schemas/types.yaml#definitions/uint32-array
+            description: >
+              configures the output into an internal test line of the pmic. Specified
+              by a list of u32 pairs, one pair per channel, where each pair denotes the
+              test line to drive and the second configures how the value should be
+              outputed, as defined in the datasheet
+            minItems: 2
+            maxItems: 2
+
+        required:
+          - reg
+
+patternProperties:
+  "^led@[0-9a-f]$":
+    type: object
+    $ref: common.yaml#
+    properties:
+      "qcom,dtest":
+        $ref: /schemas/types.yaml#definitions/uint32-array
+        description: >
+          configures the output into an internal test line of the pmic. Specified
+          by a list of u32 pairs, one pair per channel, where each pair denotes the
+          test line to drive and the second configures how the value should be
+          outputed, as defined in the datasheet
+        minItems: 2
+        maxItems: 2
+
+    required:
+      - reg
+
+required:
+  - compatible
+
+additionalProperties: false
+
+examples:
+  - |
+    #include <dt-bindings/leds/common.h>
+
+    lpg {
+      compatible = "qcom,pmi8994-lpg";
+
+      #address-cells = <1>;
+      #size-cells = <0>;
+
+      qcom,power-source = <1>;
+
+      led@1 {
+        reg = <1>;
+        label = "green:user1";
+      };
+
+      led@2 {
+        reg = <2>;
+        label = "green:user0";
+        default-state = "on";
+      };
+
+      led@3 {
+        reg = <3>;
+        label = "green:user2";
+      };
+
+      led@4 {
+        reg = <4>;
+        label = "green:user3";
+
+        qcom,dtest = <4 1>;
+      };
+    };
+  - |
+    #include <dt-bindings/leds/common.h>
+
+    lpg {
+      compatible = "qcom,pmi8994-lpg";
+
+      #address-cells = <1>;
+      #size-cells = <0>;
+
+      qcom,power-source = <1>;
+
+      multi-led {
+        color = <LED_COLOR_ID_MULTI>;
+        label = "rgb:notification";
+
+        #address-cells = <1>;
+        #size-cells = <0>;
+
+        led@1 {
+          reg = <1>;
+          color = <LED_COLOR_ID_RED>;
+        };
+
+        led@2 {
+          reg = <2>;
+          color = <LED_COLOR_ID_GREEN>;
+        };
+
+        led@3 {
+          reg = <3>;
+          color = <LED_COLOR_ID_BLUE>;
+        };
+      };
+    };
+  - |
+    lpg {
+      compatible = "qcom,pm8916-pwm";
+      #pwm-cells = <2>;
+    };
+...
-- 
2.28.0

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

[Bjorn Andersson]

The Light Pulse Generator (LPG) is a PWM-block found in a wide range of
PMICs from Qualcomm. It can operate on fixed parameters or based on a
lookup-table, altering the duty cycle over time - which provides the
means for e.g. hardware assisted transitions of LED brightness.

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

Changes since v4:
- Introduced div_u64() to avoid compilation errors on 32 bit machines
- Fixed SPDX license header and a couple of spacing issues pointed out by checkpatch

 drivers/leds/Kconfig         |    9 +
 drivers/leds/Makefile        |    1 +
 drivers/leds/leds-qcom-lpg.c | 1206 ++++++++++++++++++++++++++++++++++
 3 files changed, 1216 insertions(+)
 create mode 100644 drivers/leds/leds-qcom-lpg.c

diff --git a/drivers/leds/Kconfig b/drivers/leds/Kconfig
index 849d3c5f908e..d500648c586f 100644
--- a/drivers/leds/Kconfig
+++ b/drivers/leds/Kconfig
@@ -803,6 +803,15 @@ config LEDS_POWERNV
 	  To compile this driver as a module, choose 'm' here: the module
 	  will be called leds-powernv.
 
+config LEDS_QCOM_LPG
+	tristate "LED support for Qualcomm LPG"
+	depends on LEDS_CLASS_MULTICOLOR
+	depends on OF
+	depends on SPMI
+	help
+	  This option enables support for the Light Pulse Generator found in a
+	  wide variety of Qualcomm PMICs.
+
 config LEDS_SYSCON
 	bool "LED support for LEDs on system controllers"
 	depends on LEDS_CLASS=y
diff --git a/drivers/leds/Makefile b/drivers/leds/Makefile
index 73e603e1727e..52d0ea26fc35 100644
--- a/drivers/leds/Makefile
+++ b/drivers/leds/Makefile
@@ -79,6 +79,7 @@ obj-$(CONFIG_LEDS_PCA963X)		+= leds-pca963x.o
 obj-$(CONFIG_LEDS_PM8058)		+= leds-pm8058.o
 obj-$(CONFIG_LEDS_POWERNV)		+= leds-powernv.o
 obj-$(CONFIG_LEDS_PWM)			+= leds-pwm.o
+obj-$(CONFIG_LEDS_QCOM_LPG)		+= leds-qcom-lpg.o
 obj-$(CONFIG_LEDS_REGULATOR)		+= leds-regulator.o
 obj-$(CONFIG_LEDS_S3C24XX)		+= leds-s3c24xx.o
 obj-$(CONFIG_LEDS_SC27XX_BLTC)		+= leds-sc27xx-bltc.o
diff --git a/drivers/leds/leds-qcom-lpg.c b/drivers/leds/leds-qcom-lpg.c
new file mode 100644
index 000000000000..49282cedeb6e
--- /dev/null
+++ b/drivers/leds/leds-qcom-lpg.c
@@ -0,0 +1,1206 @@
+// SPDX-License-Identifier: GPL-2.0-only
+/*
+ * Copyright (c) 2017-2020 Linaro Ltd
+ * Copyright (c) 2010-2012, The Linux Foundation. All rights reserved.
+ */
+#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
+
+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
+ * @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
+ * @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;
+
+	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_us:	period (in microseconds) of the generated pulses
+ * @pwm_size:	resolution of the @pwm_value, 6 or 9 bits
+ * @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_duration_ms: length (in milliseconds) of one pattern run
+ * @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;
+
+	unsigned int period_us;
+	unsigned int pwm_size;
+	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 long ramp_duration_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
+ * @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;
+	unsigned int pwm_9bit_mask;
+	int num_channels;
+	struct lpg_channel_data *channels;
+};
+
+static int triled_set(struct lpg *lpg, unsigned int mask, bool 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 ? mask : 0);
+}
+
+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;
+	u8 val[2];
+	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[0] = pattern[i].brightness & 0xff;
+		val[1] = pattern[i].brightness >> 8;
+
+		regmap_bulk_write(lpg->map,
+				  lpg->lut_base + LPG_LUT_REG(idx + i), val, 2);
+	}
+
+	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);
+}
+
+#define NUM_PWM_PREDIV	4
+#define NUM_PWM_CLK	3
+#define NUM_EXP		7
+
+static const unsigned int lpg_clk_table[NUM_PWM_PREDIV][NUM_PWM_CLK] = {
+	{
+		1 * (NSEC_PER_SEC / 1024),
+		1 * (NSEC_PER_SEC / 32768),
+		1 * (NSEC_PER_SEC / 19200000),
+	},
+	{
+		3 * (NSEC_PER_SEC / 1024),
+		3 * (NSEC_PER_SEC / 32768),
+		3 * (NSEC_PER_SEC / 19200000),
+	},
+	{
+		5 * (NSEC_PER_SEC / 1024),
+		5 * (NSEC_PER_SEC / 32768),
+		5 * (NSEC_PER_SEC / 19200000),
+	},
+	{
+		6 * (NSEC_PER_SEC / 1024),
+		6 * (NSEC_PER_SEC / 32768),
+		6 * (NSEC_PER_SEC / 19200000),
+	},
+};
+
+/*
+ * PWM Frequency = Clock Frequency / (N * T)
+ *      or
+ * PWM Period = Clock Period * (N * T)
+ *      where
+ * N = 2^9 or 2^6 for 9-bit or 6-bit PWM size
+ * T = Pre-divide * 2^m, where m = 0..7 (exponent)
+ *
+ * This is the formula to figure out m for the best pre-divide and clock:
+ * (PWM Period / N) = (Pre-divide * Clock Period) * 2^m
+ */
+static void lpg_calc_freq(struct lpg_channel *chan, unsigned int period_us)
+{
+	int             n, m, clk, div;
+	int             best_m, best_div, best_clk;
+	unsigned int    last_err, cur_err, min_err;
+	unsigned int    tmp_p, period_n;
+
+	if (period_us == chan->period_us)
+		return;
+
+	/* PWM Period / N */
+	if (period_us < ((unsigned int)(-1) / NSEC_PER_USEC)) {
+		period_n = (period_us * NSEC_PER_USEC) >> 6;
+		n = 6;
+	} else {
+		period_n = (period_us >> 9) * NSEC_PER_USEC;
+		n = 9;
+	}
+
+	min_err = (unsigned int)(-1);
+	last_err = (unsigned int)(-1);
+	best_m = 0;
+	best_clk = 0;
+	best_div = 0;
+	for (clk = 0; clk < NUM_PWM_CLK; clk++) {
+		for (div = 0; div < NUM_PWM_PREDIV; div++) {
+			/* period_n = (PWM Period / N) */
+			/* tmp_p = (Pre-divide * Clock Period) * 2^m */
+			tmp_p = lpg_clk_table[div][clk];
+			for (m = 0; m <= NUM_EXP; m++) {
+				if (period_n > tmp_p)
+					cur_err = period_n - tmp_p;
+				else
+					cur_err = tmp_p - period_n;
+
+				if (cur_err < min_err) {
+					min_err = cur_err;
+					best_m = m;
+					best_clk = clk;
+					best_div = div;
+				}
+
+				if (m && cur_err > last_err)
+					/* Break for bigger cur_err */
+					break;
+
+				last_err = cur_err;
+				tmp_p <<= 1;
+			}
+		}
+	}
+
+	/* Use higher resolution */
+	if (best_m >= 3 && n == 6) {
+		n += 3;
+		best_m -= 3;
+	}
+
+	chan->clk = best_clk;
+	chan->pre_div = best_div;
+	chan->pre_div_exp = best_m;
+	chan->pwm_size = n;
+
+	chan->period_us = period_us;
+}
+
+static void lpg_calc_duty(struct lpg_channel *chan, unsigned int duty_us)
+{
+	unsigned long max = (1 << chan->pwm_size) - 1;
+	unsigned long val;
+
+	/* Figure out pwm_value with overflow handling */
+	if (duty_us < 1 << (sizeof(val) * 8 - chan->pwm_size))
+		val = (duty_us << chan->pwm_size) / chan->period_us;
+	else
+		val = duty_us / (chan->period_us >> chan->pwm_size);
+
+	if (val > max)
+		val = max;
+
+	chan->pwm_value = val;
+}
+
+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;
+
+	if (chan->pwm_size == 9)
+		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)
+{
+	u8 val[] = { chan->pwm_value & 0xff, chan->pwm_value >> 8 };
+	struct lpg *lpg = chan->lpg;
+
+	if (!chan->enabled)
+		return;
+
+	regmap_bulk_write(lpg->map, chan->base + PWM_VALUE_REG, val, 2);
+}
+
+#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 step;
+	unsigned int conf = 0;
+	unsigned int lo_idx = chan->pattern_lo_idx;
+	unsigned int hi_idx = chan->pattern_hi_idx;
+	int pattern_len;
+
+	if (!chan->ramp_enabled || chan->pattern_lo_idx == chan->pattern_hi_idx)
+		return;
+
+	pattern_len = hi_idx - lo_idx + 1;
+
+	step = DIV_ROUND_UP(chan->ramp_duration_ms, pattern_len);
+	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_write(lpg->map, chan->base + LPG_RAMP_DURATION_REG, 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 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_mask = 0;
+	unsigned int lut_mask = 0;
+	unsigned int duty_us;
+	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_USEC);
+
+			chan->enabled = true;
+			chan->ramp_enabled = true;
+
+			lut_mask |= chan->lut_mask;
+			triled_mask |= chan->triled_mask;
+		} else {
+			lpg_calc_freq(chan, NSEC_PER_USEC);
+
+			duty_us = brightness * chan->period_us / cdev->max_brightness;
+			lpg_calc_duty(chan, duty_us);
+			chan->enabled = true;
+			chan->ramp_enabled = false;
+
+			triled_mask |= chan->triled_mask;
+		}
+
+		lpg_apply(chan);
+	}
+
+	/* Toggle triled lines */
+	if (triled_mask)
+		triled_set(lpg, triled_mask, chan->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_us;
+	unsigned int duty_us;
+	int i;
+
+	if (!delay_on && !delay_off) {
+		delay_on = 500;
+		delay_off = 500;
+	}
+
+	duty_us = delay_on * USEC_PER_MSEC;
+	period_us = (delay_on + delay_off) * USEC_PER_MSEC;
+
+	for (i = 0; i < led->num_channels; i++) {
+		chan = led->channels[i];
+
+		lpg_calc_freq(chan, period_us);
+		lpg_calc_duty(chan, duty_us);
+
+		chan->enabled = true;
+		chan->ramp_enabled = false;
+
+		lpg_apply(chan);
+	}
+
+	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);
+}
+
+#define interpolate(x1, y1, x2, y2, x) \
+	((y1) + ((y2) - (y1)) * ((x) - (x1)) / ((x2) - (x1)))
+
+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;
+
+	/*
+	 * 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)
+		goto out;
+
+	for (i = 0; i < led->num_channels; i++) {
+		chan = led->channels[i];
+
+		chan->ramp_duration_ms = pattern[0].delta_t * len;
+		chan->ramp_ping_pong = ping_pong;
+		chan->ramp_oneshot = repeat != -1;
+
+		chan->ramp_lo_pause_ms = lo_pause;
+		chan->ramp_hi_pause_ms = hi_pause;
+
+		chan->pattern_lo_idx = lo_idx;
+		chan->pattern_hi_idx = hi_idx;
+	}
+
+out:
+	return ret;
+}
+
+static int lpg_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;
+}
+
+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];
+
+	lpg_calc_freq(chan, div_u64(state->period, NSEC_PER_USEC));
+	lpg_calc_duty(chan, div_u64(state->duty_cycle, NSEC_PER_USEC));
+	chan->enabled = state->enabled;
+
+	lpg_apply(chan);
+
+	triled_set(lpg, chan->triled_mask, chan->enabled);
+
+	return 0;
+}
+
+static const struct pwm_ops lpg_pwm_ops = {
+	.request = lpg_pwm_request,
+	.apply = lpg_pwm_apply,
+	.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 dtest[2];
+	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)
+		return ret;
+
+	chan->color = color;
+
+	ret = of_property_read_u32_array(np, "qcom,dtest", dtest, 2);
+	if (ret < 0 && ret != -EINVAL) {
+		dev_err(lpg->dev, "malformed qcom,dtest of %pOFn\n", np);
+		return ret;
+	} else if (!ret) {
+		chan->dtest_line = dtest[0];
+		chan->dtest_value = dtest[1];
+	}
+
+	*channel = chan;
+
+	return 0;
+}
+
+static int lpg_add_led(struct lpg *lpg, struct device_node *np)
+{
+	struct led_classdev *cdev;
+	struct device_node *child;
+	struct mc_subled *info;
+	struct lpg_led *led;
+	const char *state;
+	int num_channels;
+	u32 color = 0;
+	int size;
+	int ret;
+	int i;
+
+	ret = of_property_read_u32(np, "color", &color);
+	if (ret < 0 && ret != -EINVAL)
+		return ret;
+
+	if (color == LED_COLOR_ID_MULTI)
+		num_channels = of_get_available_child_count(np);
+	else
+		num_channels = 1;
+
+	size = sizeof(*led) + num_channels * sizeof(struct lpg_channel *);
+	led = devm_kzalloc(lpg->dev, size, GFP_KERNEL);
+	if (!led)
+		return -ENOMEM;
+
+	led->lpg = lpg;
+	led->num_channels = num_channels;
+
+	if (color == LED_COLOR_ID_MULTI) {
+		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;
+		}
+	}
+
+	/* Use label else node name */
+	cdev->name = of_get_property(np, "label", NULL) ? : np->name;
+	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);
+
+	if (color == LED_COLOR_ID_MULTI)
+		ret = devm_led_classdev_multicolor_register(lpg->dev, &led->mcdev);
+	else
+		ret = devm_led_classdev_register(lpg->dev, &led->cdev);
+	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;
+
+	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 */
+	regmap_write(lpg->map, lpg->triled_base + TRI_LED_ATC_CTL, 0);
+
+	/* Configure power source */
+	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_LONGS(lpg->lut_size) * sizeof(unsigned long);
+	lpg->lut_bitmap = devm_kzalloc(lpg->dev, bitmap_size, GFP_KERNEL);
+
+	bitmap_clear(lpg->lut_bitmap, 0, lpg->lut_size);
+	return lpg->lut_bitmap ? 0 : -ENOMEM;
+}
+
+static int lpg_probe(struct platform_device *pdev)
+{
+	struct device_node *np;
+	struct lpg *lpg;
+	int ret;
+	int i;
+
+	lpg = devm_kzalloc(&pdev->dev, sizeof(*lpg), GFP_KERNEL);
+	if (!lpg)
+		return -ENOMEM;
+
+	lpg->data = of_device_get_match_data(&pdev->dev);
+	if (!lpg->data)
+		return -EINVAL;
+
+	lpg->dev = &pdev->dev;
+
+	lpg->map = dev_get_regmap(pdev->dev.parent, NULL);
+	if (!lpg->map) {
+		dev_err(&pdev->dev, "parent regmap unavailable\n");
+		return -ENXIO;
+	}
+
+	ret = lpg_init_channels(lpg);
+	if (ret < 0)
+		return ret;
+
+	ret = lpg_init_triled(lpg);
+	if (ret < 0)
+		return ret;
+
+	ret = lpg_init_lut(lpg);
+	if (ret < 0)
+		return ret;
+
+	for_each_available_child_of_node(pdev->dev.of_node, np) {
+		ret = lpg_add_led(lpg, np);
+		if (ret)
+			return ret;
+	}
+
+	for (i = 0; i < lpg->num_channels; i++)
+		lpg_apply_dtest(&lpg->channels[i]);
+
+	ret = lpg_add_pwm(lpg);
+	if (ret)
+		return ret;
+
+	platform_set_drvdata(pdev, lpg);
+
+	return 0;
+}
+
+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 = (struct lpg_channel_data[]) {
+		{ .base = 0xbc00 },
+	},
+};
+
+static const struct lpg_data pm8941_lpg_data = {
+	.lut_base = 0xb000,
+	.lut_size = 64,
+
+	.triled_base = 0xd000,
+
+	.pwm_9bit_mask = 3 << 4,
+
+	.num_channels = 8,
+	.channels = (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 = (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,
+
+	.pwm_9bit_mask = BIT(4),
+
+	.num_channels = 4,
+	.channels = (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,
+
+	.pwm_9bit_mask = BIT(4),
+
+	.num_channels = 6,
+	.channels = (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 of_device_id lpg_of_table[] = {
+	{ .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 },
+	{},
+};
+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.28.0

[PATCH v5 3/4] arm64: dts: qcom: pm(i)8994: Add mpp and lpg blocks

[Bjorn Andersson]

The pm8994 contains a 6 LPG channels and the pmi8994 contains 4 MPP
channels and a 4 channel LPG, with TRILED and LUT blocks.

Add nodes for these blocks.

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

Changes since v4:
- Replaced msm8996 with pm(i)8994 in subject

 arch/arm64/boot/dts/qcom/pm8994.dtsi  |  9 +++++++++
 arch/arm64/boot/dts/qcom/pmi8994.dtsi | 20 ++++++++++++++++++++
 2 files changed, 29 insertions(+)

diff --git a/arch/arm64/boot/dts/qcom/pm8994.dtsi b/arch/arm64/boot/dts/qcom/pm8994.dtsi
index 7e4f777746cb..b5bef687aa3c 100644
--- a/arch/arm64/boot/dts/qcom/pm8994.dtsi
+++ b/arch/arm64/boot/dts/qcom/pm8994.dtsi
@@ -86,6 +86,15 @@ pmic@1 {
 		#address-cells = <1>;
 		#size-cells = <0>;
 
+		pm8994_lpg: lpg {
+			compatible = "qcom,pm8994-lpg";
+
+			#address-cells = <1>;
+			#size-cells = <0>;
+
+			status = "disabled";
+		};
+
 		pm8994_spmi_regulators: regulators {
 			compatible = "qcom,pm8994-regulators";
 		};
diff --git a/arch/arm64/boot/dts/qcom/pmi8994.dtsi b/arch/arm64/boot/dts/qcom/pmi8994.dtsi
index e5ed28ab9b2d..23f41328d191 100644
--- a/arch/arm64/boot/dts/qcom/pmi8994.dtsi
+++ b/arch/arm64/boot/dts/qcom/pmi8994.dtsi
@@ -19,6 +19,17 @@ pmi8994_gpios: gpios@c000 {
 			interrupt-controller;
 			#interrupt-cells = <2>;
 		};
+
+		pmi8994_mpps: mpps@a000 {
+			compatible = "qcom,pm8994-mpp";
+			reg = <0xa000>;
+			gpio-controller;
+			#gpio-cells = <2>;
+			interrupts = <0 0xa0 0 IRQ_TYPE_NONE>,
+				     <0 0xa1 0 IRQ_TYPE_NONE>,
+				     <0 0xa2 0 IRQ_TYPE_NONE>,
+				     <0 0xa3 0 IRQ_TYPE_NONE>;
+		};
 	};
 
 	pmic@3 {
@@ -27,6 +38,15 @@ pmic@3 {
 		#address-cells = <1>;
 		#size-cells = <0>;
 
+		pmi8994_lpg: lpg@b100 {
+			compatible = "qcom,pmi8994-lpg";
+
+			#address-cells = <1>;
+			#size-cells = <0>;
+
+			status = "disabled";
+		};
+
 		pmi8994_spmi_regulators: regulators {
 			compatible = "qcom,pmi8994-regulators";
 			#address-cells = <1>;
-- 
2.28.0

[PATCH v5 4/4] arm64: dts: qcom: Add user LEDs on db820c

[Bjorn Andersson]

The db820c has 4 "user LEDs", all connected to the PMI8994. The first
three are connected to the three current sinks provided by the TRILED
and the fourth is connected to MPP2.

By utilizing the DTEST bus the MPP is fed the control signal from the
fourth LPG block, providing a consistent interface to the user.

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

Changes since v4:
- None

 arch/arm64/boot/dts/qcom/apq8096-db820c.dtsi | 49 ++++++++++++++++++++
 1 file changed, 49 insertions(+)

diff --git a/arch/arm64/boot/dts/qcom/apq8096-db820c.dtsi b/arch/arm64/boot/dts/qcom/apq8096-db820c.dtsi
index defcbd15edf9..7e51677d256e 100644
--- a/arch/arm64/boot/dts/qcom/apq8096-db820c.dtsi
+++ b/arch/arm64/boot/dts/qcom/apq8096-db820c.dtsi
@@ -8,6 +8,7 @@
 #include "pmi8994.dtsi"
 #include <dt-bindings/input/input.h>
 #include <dt-bindings/gpio/gpio.h>
+#include <dt-bindings/leds/common.h>
 #include <dt-bindings/pinctrl/qcom,pmic-gpio.h>
 #include <dt-bindings/sound/qcom,q6afe.h>
 #include <dt-bindings/sound/qcom,q6asm.h>
@@ -682,6 +683,54 @@ pinconf {
 	};
 };
 
+&pmi8994_mpps {
+	pmi8994_mpp2_userled4: mpp2-userled4 {
+		pins = "mpp2";
+		function = "sink";
+
+		output-low;
+		qcom,dtest = <4>;
+	};
+};
+
+&pmi8994_lpg {
+	qcom,power-source = <1>;
+
+	pinctrl-names = "default";
+	pinctrl-0 = <&pmi8994_mpp2_userled4>;
+
+	status = "okay";
+
+	#address-cells = <1>;
+	#size-cells = <0>;
+
+	led@1 {
+		reg = <1>;
+		label = "green:user1";
+
+		linux,default-trigger = "heartbeat";
+		default-state = "on";
+	};
+
+	led@2 {
+		reg = <2>;
+		label = "green:user0";
+		default-state = "on";
+	};
+
+	led@3 {
+		reg = <3>;
+		label = "green:user2";
+	};
+
+	led@4 {
+		reg = <4>;
+		label = "green:user3";
+
+		qcom,dtest = <4 1>;
+	};
+};
+
 &pmi8994_spmi_regulators {
 	vdd_gfx: s2@1700 {
 		reg = <0x1700 0x100>;
-- 
2.28.0

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

[Martin Botka]

With the changes done to in V5 i have nothing to add.

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

[Andy Shevchenko]

On Sat, Oct 17, 2020 at 8:41 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. It can operate on fixed parameters or based on a
> lookup-table, altering the duty cycle over time - which provides the
> means for e.g. hardware assisted transitions of LED brightness.

> +config LEDS_QCOM_LPG
> +       tristate "LED support for Qualcomm LPG"
> +       depends on LEDS_CLASS_MULTICOLOR
> +       depends on OF
> +       depends on SPMI


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

...

> +struct lpg {
> +       struct device *dev;
> +       struct regmap *map;

Can't you derive the former from the latter?

> +
> +       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;
> +
> +       struct lpg_channel *channels;
> +       unsigned int num_channels;
> +};

...

> +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;
> +       u8 val[2];

__be16 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[0] = pattern[i].brightness & 0xff;
> +               val[1] = pattern[i].brightness >> 8;

cpu_to_be16();

> +
> +               regmap_bulk_write(lpg->map,
> +                                 lpg->lut_base + LPG_LUT_REG(idx + i), val, 2);
> +       }
> +
> +       bitmap_set(lpg->lut_bitmap, idx, len);
> +
> +       *lo_idx = idx;
> +       *hi_idx = idx + len - 1;
> +
> +       return 0;
> +}

...

> +static void lpg_calc_freq(struct lpg_channel *chan, unsigned int period_us)
> +{
> +       int             n, m, clk, div;
> +       int             best_m, best_div, best_clk;
> +       unsigned int    last_err, cur_err, min_err;
> +       unsigned int    tmp_p, period_n;
> +
> +       if (period_us == chan->period_us)
> +               return;
> +
> +       /* PWM Period / N */
> +       if (period_us < ((unsigned int)(-1) / NSEC_PER_USEC)) {

Please, replace all these -1 with castings to unsigned types with
corresponding limits, like
UINT_MAX here.

> +               period_n = (period_us * NSEC_PER_USEC) >> 6;
> +               n = 6;
> +       } else {
> +               period_n = (period_us >> 9) * NSEC_PER_USEC;
> +               n = 9;
> +       }

Why inconsistency in branches? Can you rather derive n and calculate
only once like

           period_n = (period_us >> n) * NSEC_PER_USEC;

?

> +       min_err = (unsigned int)(-1);
> +       last_err = (unsigned int)(-1);
> +       best_m = 0;
> +       best_clk = 0;
> +       best_div = 0;
> +       for (clk = 0; clk < NUM_PWM_CLK; clk++) {
> +               for (div = 0; div < NUM_PWM_PREDIV; div++) {
> +                       /* period_n = (PWM Period / N) */
> +                       /* tmp_p = (Pre-divide * Clock Period) * 2^m */
> +                       tmp_p = lpg_clk_table[div][clk];
> +                       for (m = 0; m <= NUM_EXP; m++) {
> +                               if (period_n > tmp_p)
> +                                       cur_err = period_n - tmp_p;
> +                               else
> +                                       cur_err = tmp_p - period_n;
> +
> +                               if (cur_err < min_err) {
> +                                       min_err = cur_err;
> +                                       best_m = m;
> +                                       best_clk = clk;
> +                                       best_div = div;
> +                               }
> +
> +                               if (m && cur_err > last_err)
> +                                       /* Break for bigger cur_err */
> +                                       break;
> +
> +                               last_err = cur_err;
> +                               tmp_p <<= 1;
> +                       }
> +               }
> +       }
> +
> +       /* Use higher resolution */
> +       if (best_m >= 3 && n == 6) {
> +               n += 3;
> +               best_m -= 3;
> +       }
> +
> +       chan->clk = best_clk;
> +       chan->pre_div = best_div;
> +       chan->pre_div_exp = best_m;
> +       chan->pwm_size = n;
> +
> +       chan->period_us = period_us;
> +}
> +
> +static void lpg_calc_duty(struct lpg_channel *chan, unsigned int duty_us)
> +{
> +       unsigned long max = (1 << chan->pwm_size) - 1;

BIT() ?

> +       unsigned long val;
> +
> +       /* Figure out pwm_value with overflow handling */

> +       if (duty_us < 1 << (sizeof(val) * 8 - chan->pwm_size))

BITS_PER_TYPE, but actually BITS_PER_LONG here.

BIT(BITS_PER_LONG - ...)

> +               val = (duty_us << chan->pwm_size) / chan->period_us;
> +       else
> +               val = duty_us / (chan->period_us >> chan->pwm_size);
> +
> +       if (val > max)
> +               val = max;
> +
> +       chan->pwm_value = val;
> +}

...

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

Here and everywhere else when function declared as void there is no
possibility to know if we do something useful or already screwed up
the things.

> +static void lpg_apply_pwm_value(struct lpg_channel *chan)
> +{
> +       u8 val[] = { chan->pwm_value & 0xff, chan->pwm_value >> 8 };


__le16 and cpu_to_le16()

> +       struct lpg *lpg = chan->lpg;
> +
> +       if (!chan->enabled)
> +               return;
> +
> +       regmap_bulk_write(lpg->map, chan->base + PWM_VALUE_REG, val, 2);
> +}

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

Did I miss bits.h inclusion at the beginning of the file?

...

> +static int lpg_blink_set(struct lpg_led *led,
> +                        unsigned long delay_on, unsigned long delay_off)
> +{
> +       struct lpg_channel *chan;
> +       unsigned int period_us;
> +       unsigned int duty_us;
> +       int i;
> +
> +       if (!delay_on && !delay_off) {
> +               delay_on = 500;
> +               delay_off = 500;
> +       }

Homegrown duty cycle?
I mean, why simply not to pass the duty cycle in percentage in the first place?

> +       duty_us = delay_on * USEC_PER_MSEC;
> +       period_us = (delay_on + delay_off) * USEC_PER_MSEC;
> +
> +       for (i = 0; i < led->num_channels; i++) {
> +               chan = led->channels[i];
> +
> +               lpg_calc_freq(chan, period_us);
> +               lpg_calc_duty(chan, duty_us);
> +
> +               chan->enabled = true;
> +               chan->ramp_enabled = false;
> +
> +               lpg_apply(chan);
> +       }
> +
> +       return 0;
> +}

> +#define interpolate(x1, y1, x2, y2, x) \
> +       ((y1) + ((y2) - (y1)) * ((x) - (x1)) / ((x2) - (x1)))

Can you rather create a generic one under lib/ or start include/linux/math.h ?

https://elixir.bootlin.com/linux/latest/A/ident/interpolate

...

> +out:

Useless label.

> +       return ret;

...

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

This check is fishy. Either you have optional property or not, in the
latter case return any error code.

> +               return ret;
> +
> +       chan->color = color;

So, it may be -EINVAL?!

> +       ret = of_property_read_u32_array(np, "qcom,dtest", dtest, 2);
> +       if (ret < 0 && ret != -EINVAL) {
> +               dev_err(lpg->dev, "malformed qcom,dtest of %pOFn\n", np);
> +               return ret;
> +       } else if (!ret) {
> +               chan->dtest_line = dtest[0];
> +               chan->dtest_value = dtest[1];
> +       }

Ditto.

...

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

Ditto.

...

> +       size = sizeof(*led) + num_channels * sizeof(struct lpg_channel *);

Use respective helpers from overflow.h.

> +       led = devm_kzalloc(lpg->dev, size, GFP_KERNEL);
> +       if (!led)
> +               return -ENOMEM;

...

> +static const struct of_device_id lpg_of_table[] = {
> +       { .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 },

> +       {},

No comma needed for terminator lines.

> +};

-- 
With Best Regards,
Andy Shevchenko

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

[Bjorn Andersson]

On Sun 18 Oct 15:12 CDT 2020, Andy Shevchenko wrote:

> On Sat, Oct 17, 2020 at 8:41 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. It can operate on fixed parameters or based on a
> > lookup-table, altering the duty cycle over time - which provides the
> > means for e.g. hardware assisted transitions of LED brightness.
> 
> > +config LEDS_QCOM_LPG
> > +       tristate "LED support for Qualcomm LPG"
> > +       depends on LEDS_CLASS_MULTICOLOR
> > +       depends on OF
> > +       depends on SPMI
> 
> 
> > +#include <linux/of.h>
> > +#include <linux/of_device.h>
> 
> ...
> 
> > +struct lpg {
> > +       struct device *dev;
> > +       struct regmap *map;
> 
> Can't you derive the former from the latter?
> 

No, because map->dev is actually the dev->parent.

> > +
> > +       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;
> > +
> > +       struct lpg_channel *channels;
> > +       unsigned int num_channels;
> > +};
> 
> ...
> 
> > +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;
> > +       u8 val[2];
> 
> __be16 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[0] = pattern[i].brightness & 0xff;
> > +               val[1] = pattern[i].brightness >> 8;
> 
> cpu_to_be16();
> 

I like it, but isn't that a le16?

> > +
> > +               regmap_bulk_write(lpg->map,
> > +                                 lpg->lut_base + LPG_LUT_REG(idx + i), val, 2);
> > +       }
> > +
> > +       bitmap_set(lpg->lut_bitmap, idx, len);
> > +
> > +       *lo_idx = idx;
> > +       *hi_idx = idx + len - 1;
> > +
> > +       return 0;
> > +}
> 
> ...
> 
> > +static void lpg_calc_freq(struct lpg_channel *chan, unsigned int period_us)
> > +{
> > +       int             n, m, clk, div;
> > +       int             best_m, best_div, best_clk;
> > +       unsigned int    last_err, cur_err, min_err;
> > +       unsigned int    tmp_p, period_n;
> > +
> > +       if (period_us == chan->period_us)
> > +               return;
> > +
> > +       /* PWM Period / N */
> > +       if (period_us < ((unsigned int)(-1) / NSEC_PER_USEC)) {
> 
> Please, replace all these -1 with castings to unsigned types with
> corresponding limits, like
> UINT_MAX here.
> 

Sure thing.

> > +               period_n = (period_us * NSEC_PER_USEC) >> 6;
> > +               n = 6;
> > +       } else {
> > +               period_n = (period_us >> 9) * NSEC_PER_USEC;
> > +               n = 9;
> > +       }
> 
> Why inconsistency in branches? Can you rather derive n and calculate
> only once like
> 
>            period_n = (period_us >> n) * NSEC_PER_USEC;
> 
> ?

I inherited this piece from the downstream driver and I assume that the
purpose was to avoid loss of precision. I will review this and if
nothing else it seems like I would be able to cast period_us to more
bits, do the multiply and then shift - in both cases.

> 
> > +       min_err = (unsigned int)(-1);
> > +       last_err = (unsigned int)(-1);
> > +       best_m = 0;
> > +       best_clk = 0;
> > +       best_div = 0;
> > +       for (clk = 0; clk < NUM_PWM_CLK; clk++) {
> > +               for (div = 0; div < NUM_PWM_PREDIV; div++) {
> > +                       /* period_n = (PWM Period / N) */
> > +                       /* tmp_p = (Pre-divide * Clock Period) * 2^m */
> > +                       tmp_p = lpg_clk_table[div][clk];
> > +                       for (m = 0; m <= NUM_EXP; m++) {
> > +                               if (period_n > tmp_p)
> > +                                       cur_err = period_n - tmp_p;
> > +                               else
> > +                                       cur_err = tmp_p - period_n;
> > +
> > +                               if (cur_err < min_err) {
> > +                                       min_err = cur_err;
> > +                                       best_m = m;
> > +                                       best_clk = clk;
> > +                                       best_div = div;
> > +                               }
> > +
> > +                               if (m && cur_err > last_err)
> > +                                       /* Break for bigger cur_err */
> > +                                       break;
> > +
> > +                               last_err = cur_err;
> > +                               tmp_p <<= 1;
> > +                       }
> > +               }
> > +       }
> > +
> > +       /* Use higher resolution */
> > +       if (best_m >= 3 && n == 6) {
> > +               n += 3;
> > +               best_m -= 3;
> > +       }
> > +
> > +       chan->clk = best_clk;
> > +       chan->pre_div = best_div;
> > +       chan->pre_div_exp = best_m;
> > +       chan->pwm_size = n;
> > +
> > +       chan->period_us = period_us;
> > +}
> > +
> > +static void lpg_calc_duty(struct lpg_channel *chan, unsigned int duty_us)
> > +{
> > +       unsigned long max = (1 << chan->pwm_size) - 1;
> 
> BIT() ?
> 
> > +       unsigned long val;
> > +
> > +       /* Figure out pwm_value with overflow handling */
> 
> > +       if (duty_us < 1 << (sizeof(val) * 8 - chan->pwm_size))
> 
> BITS_PER_TYPE, but actually BITS_PER_LONG here.
> 
> BIT(BITS_PER_LONG - ...)
> 

Again, this seems to just be a question of avoiding overflow of the 32
bit duty_us. I'll double check the math here as well.

> > +               val = (duty_us << chan->pwm_size) / chan->period_us;
> > +       else
> > +               val = duty_us / (chan->period_us >> chan->pwm_size);
> > +
> > +       if (val > max)
> > +               val = max;
> > +
> > +       chan->pwm_value = val;
> > +}
> 
> ...
> 
> > +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);
> > +}
> 
> Here and everywhere else when function declared as void there is no
> possibility to know if we do something useful or already screwed up
> the things.
> 

Ultimately these are all coming from led_classdev->brightness_set, which
is a void function. So there isn't much benefit of propagating this
error upwards, today.

> > +static void lpg_apply_pwm_value(struct lpg_channel *chan)
> > +{
> > +       u8 val[] = { chan->pwm_value & 0xff, chan->pwm_value >> 8 };
> 
> 
> __le16 and cpu_to_le16()
> 

Sounds good.

> > +       struct lpg *lpg = chan->lpg;
> > +
> > +       if (!chan->enabled)
> > +               return;
> > +
> > +       regmap_bulk_write(lpg->map, chan->base + PWM_VALUE_REG, val, 2);
> > +}
> 
> > +#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)
> 
> Did I miss bits.h inclusion at the beginning of the file?
> 

Will add this.

> ...
> 
> > +static int lpg_blink_set(struct lpg_led *led,
> > +                        unsigned long delay_on, unsigned long delay_off)
> > +{
> > +       struct lpg_channel *chan;
> > +       unsigned int period_us;
> > +       unsigned int duty_us;
> > +       int i;
> > +
> > +       if (!delay_on && !delay_off) {
> > +               delay_on = 500;
> > +               delay_off = 500;
> > +       }
> 
> Homegrown duty cycle?
> I mean, why simply not to pass the duty cycle in percentage in the first place?
> 

Can you explain what you're saying here.

> > +       duty_us = delay_on * USEC_PER_MSEC;
> > +       period_us = (delay_on + delay_off) * USEC_PER_MSEC;
> > +
> > +       for (i = 0; i < led->num_channels; i++) {
> > +               chan = led->channels[i];
> > +
> > +               lpg_calc_freq(chan, period_us);
> > +               lpg_calc_duty(chan, duty_us);
> > +
> > +               chan->enabled = true;
> > +               chan->ramp_enabled = false;
> > +
> > +               lpg_apply(chan);
> > +       }
> > +
> > +       return 0;
> > +}
> 
> > +#define interpolate(x1, y1, x2, y2, x) \
> > +       ((y1) + ((y2) - (y1)) * ((x) - (x1)) / ((x2) - (x1)))
> 
> Can you rather create a generic one under lib/ or start include/linux/math.h ?
> 

Forgot about this, but I've seen one on LKML, will find it and work on
getting that accepted.

> https://elixir.bootlin.com/linux/latest/A/ident/interpolate
> 
> ...
> 
> > +out:
> 
> Useless label.
> 
> > +       return ret;
> 
> ...
> 
> > +       ret = of_property_read_u32(np, "color", &color);
> > +       if (ret < 0 && ret != -EINVAL)
> 
> This check is fishy. Either you have optional property or not, in the
> latter case return any error code.
> 

There's three possible outcomes here:
1) We found _one_ integer in the property, color is assigned and 0 is
returned.
2) We found no property named "color", -EINVAL is returned without color
being modified.
3) We found a property but it wasn't a single u32 value so a negative
error (not EINVAL) is returned.

> > +               return ret;
> > +
> > +       chan->color = color;
> 
> So, it may be -EINVAL?!
> 

So color will either be the value or the property color, or if omitted
LED_COLOR_ID_GREEN.

> > +       ret = of_property_read_u32_array(np, "qcom,dtest", dtest, 2);
> > +       if (ret < 0 && ret != -EINVAL) {
> > +               dev_err(lpg->dev, "malformed qcom,dtest of %pOFn\n", np);
> > +               return ret;
> > +       } else if (!ret) {
> > +               chan->dtest_line = dtest[0];
> > +               chan->dtest_value = dtest[1];
> > +       }
> 
> Ditto.
> 

We're in !ret and as such dtest is initialized.

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

As above, if no property color is specified, color remains 0 here which
is not LED_COLOR_ID_MULTI and this is a single channel LED without its
color specified.

> ...
> 
> > +       size = sizeof(*led) + num_channels * sizeof(struct lpg_channel *);
> 
> Use respective helpers from overflow.h.
> 

Will do, thanks.

> > +       led = devm_kzalloc(lpg->dev, size, GFP_KERNEL);
> > +       if (!led)
> > +               return -ENOMEM;
> 
> ...
> 
> > +static const struct of_device_id lpg_of_table[] = {
> > +       { .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 },
> 
> > +       {},
> 
> No comma needed for terminator lines.
> 

Will drop.

> > +};

Thank you Andy for the review!

Regards,
Bjorn

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

[Pavel Machek]

[-- Attachment #1: Type: text/plain, Size: 206 bytes --]

Hi!

> With the changes done to in V5 i have nothing to add.

Good way to say that is "Patches 7 and 28, Reviewed-by:"...

Best regards,
							Pavel

-- 
http://www.livejournal.com/~pavelmachek

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 195 bytes --]

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

[Martin Botka]

> Good way to say that is "Patches 7 and 28, Reviewed-by:"...

7 and 28 ? I dont see any patches 7 and 28 (I assume thats a typo for 8)

Either way.

Reviewed-by: Martin Botka <martin.botka1@gmail.com>

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

[Pavel Machek]

[-- Attachment #1: Type: text/plain, Size: 396 bytes --]

Hi!

> > Good way to say that is "Patches 7 and 28, Reviewed-by:"...
> 
> 7 and 28 ? I dont see any patches 7 and 28 (I assume thats a typo for 8)
> 
> Either way.
> 
> Reviewed-by: Martin Botka <martin.botka1@gmail.com>

I was trying to use obviously bogus numbers to make you to specify
which patches you reviewed :-).

								Pavel
-- 
http://www.livejournal.com/~pavelmachek

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 195 bytes --]

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

[Martin Botka]

Hi!

> I was trying to use obviously bogus numbers to make you to specify
> which patches you reviewed :-).

Ahhhh now that makes much more sense.

I reviewed patch 2.
Tho from the quick look i dont see anything wrong with the DTS changes either.

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

[Andy Shevchenko]

On Tue, Oct 20, 2020 at 7:29 AM Bjorn Andersson
<bjorn.andersson@linaro.org> wrote:
> On Sun 18 Oct 15:12 CDT 2020, Andy Shevchenko wrote:
> > On Sat, Oct 17, 2020 at 8:41 AM Bjorn Andersson
> > <bjorn.andersson@linaro.org> wrote:

...

> > > +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;
> > > +       u8 val[2];
> >
> > __be16 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[0] = pattern[i].brightness & 0xff;
> > > +               val[1] = pattern[i].brightness >> 8;
> >
> > cpu_to_be16();
> >
>
> I like it, but isn't that a le16?

Oh, yes.

> > > +               regmap_bulk_write(lpg->map,
> > > +                                 lpg->lut_base + LPG_LUT_REG(idx + i), val, 2);
> > > +       }
> > > +
> > > +       bitmap_set(lpg->lut_bitmap, idx, len);
> > > +
> > > +       *lo_idx = idx;
> > > +       *hi_idx = idx + len - 1;
> > > +
> > > +       return 0;
> > > +}

...

> > > +               period_n = (period_us * NSEC_PER_USEC) >> 6;
> > > +               n = 6;
> > > +       } else {
> > > +               period_n = (period_us >> 9) * NSEC_PER_USEC;
> > > +               n = 9;

> > Why inconsistency in branches? Can you rather derive n and calculate
> > only once like
> >
> >            period_n = (period_us >> n) * NSEC_PER_USEC;
> >
> > ?
>
> I inherited this piece from the downstream driver and I assume that the
> purpose was to avoid loss of precision. I will review this and if
> nothing else it seems like I would be able to cast period_us to more
> bits, do the multiply and then shift - in both cases.

Understood. Yes, please check if precision doesn't suffer and update
accordingly.

...

> > > +static void lpg_calc_duty(struct lpg_channel *chan, unsigned int duty_us)
> > > +{
> > > +       unsigned long max = (1 << chan->pwm_size) - 1;
> >
> > BIT() ?

Actually if you don't use BIT() here (or U suffix) it is UB for pwm_size == 31.

> >
> > > +       unsigned long val;
> > > +
> > > +       /* Figure out pwm_value with overflow handling */
> >
> > > +       if (duty_us < 1 << (sizeof(val) * 8 - chan->pwm_size))
> >
> > BITS_PER_TYPE, but actually BITS_PER_LONG here.
> >
> > BIT(BITS_PER_LONG - ...)
> >
>
> Again, this seems to just be a question of avoiding overflow of the 32
> bit duty_us. I'll double check the math here as well.

Can pwm_size be equal to 0?

> > > +               val = (duty_us << chan->pwm_size) / chan->period_us;
> > > +       else
> > > +               val = duty_us / (chan->period_us >> chan->pwm_size);
> > > +
> > > +       if (val > max)
> > > +               val = max;
> > > +
> > > +       chan->pwm_value = val;
> > > +}

...

> > > +static int lpg_blink_set(struct lpg_led *led,
> > > +                        unsigned long delay_on, unsigned long delay_off)
> > > +{
> > > +       struct lpg_channel *chan;
> > > +       unsigned int period_us;
> > > +       unsigned int duty_us;
> > > +       int i;
> > > +
> > > +       if (!delay_on && !delay_off) {
> > > +               delay_on = 500;
> > > +               delay_off = 500;
> > > +       }
> >
> > Homegrown duty cycle?
> > I mean, why simply not to pass the duty cycle in percentage in the first place?
>
> Can you explain what you're saying here.

Why not to use duty cycle (in %) and period (in us) as a parameter to
the function directly?

> > > +       duty_us = delay_on * USEC_PER_MSEC;
> > > +       period_us = (delay_on + delay_off) * USEC_PER_MSEC;
> > > +
> > > +       for (i = 0; i < led->num_channels; i++) {
> > > +               chan = led->channels[i];
> > > +
> > > +               lpg_calc_freq(chan, period_us);
> > > +               lpg_calc_duty(chan, duty_us);
> > > +
> > > +               chan->enabled = true;
> > > +               chan->ramp_enabled = false;
> > > +
> > > +               lpg_apply(chan);
> > > +       }
> > > +
> > > +       return 0;
> > > +}

...

> > Can you rather create a generic one under lib/ or start include/linux/math.h ?

> Forgot about this, but I've seen one on LKML, will find it and work on
> getting that accepted.

Note, I have submitted the patch that splits out math.h from kernel.h
(it's in Andrew's quilt and in Linux Next as of today), you may send a
follow up patch that adds this functionality.

...

> > > +       ret = of_property_read_u32(np, "color", &color);
> > > +       if (ret < 0 && ret != -EINVAL)
> >
> > This check is fishy. Either you have optional property or not, in the
> > latter case return any error code.
> >
>
> There's three possible outcomes here:
> 1) We found _one_ integer in the property, color is assigned and 0 is
> returned.

I didn't get this. Doesn't your YAML schema say that it must be a
single integer?

> 2) We found no property named "color", -EINVAL is returned without color
> being modified.
> 3) We found a property but it wasn't a single u32 value so a negative
> error (not EINVAL) is returned.
>
> > > +               return ret;
> > > +
> > > +       chan->color = color;
> >
> > So, it may be -EINVAL?!
> >
>
> So color will either be the value or the property color, or if omitted
> LED_COLOR_ID_GREEN.

If property is optional, we do simple

ret = of_read_property_...(&x);
if (ret)
 x = default_value;

Otherwise simple
ret = ...
if (ret)
  return ret;

is sufficient.

What you have done is a little bit unusual.

...

> > > +       ret = of_property_read_u32_array(np, "qcom,dtest", dtest, 2);
> > > +       if (ret < 0 && ret != -EINVAL) {
> > > +               dev_err(lpg->dev, "malformed qcom,dtest of %pOFn\n", np);
> > > +               return ret;
> > > +       } else if (!ret) {
> > > +               chan->dtest_line = dtest[0];
> > > +               chan->dtest_value = dtest[1];
> > > +       }
> >
> > Ditto.
> >
>
> We're in !ret and as such dtest is initialized.

As above.

...

> > > +       ret = of_property_read_u32(np, "color", &color);
> > > +       if (ret < 0 && ret != -EINVAL)
> > > +               return ret;
> >
> > Ditto.
> >
>
> As above, if no property color is specified, color remains 0 here which
> is not LED_COLOR_ID_MULTI and this is a single channel LED without its
> color specified.

As above.

-- 
With Best Regards,
Andy Shevchenko