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

[PATCH 1/2] 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>
---
 .../ABI/testing/sysfs-class-led-driver-qcom-lpg    |  47 ++
 drivers/leds/Kconfig                               |   7 +
 drivers/leds/Makefile                              |   3 +
 drivers/leds/leds-qcom-lpg-lut.c                   | 299 ++++++++
 drivers/leds/leds-qcom-lpg.c                       | 779 +++++++++++++++++++++
 drivers/leds/leds-qcom-lpg.h                       |  30 +
 drivers/leds/leds-qcom-triled.c                    | 193 +++++
 7 files changed, 1358 insertions(+)
 create mode 100644 Documentation/ABI/testing/sysfs-class-led-driver-qcom-lpg
 create mode 100644 drivers/leds/leds-qcom-lpg-lut.c
 create mode 100644 drivers/leds/leds-qcom-lpg.c
 create mode 100644 drivers/leds/leds-qcom-lpg.h
 create mode 100644 drivers/leds/leds-qcom-triled.c

diff --git a/Documentation/ABI/testing/sysfs-class-led-driver-qcom-lpg b/Documentation/ABI/testing/sysfs-class-led-driver-qcom-lpg
new file mode 100644
index 000000000000..35bfe6e8e148
--- /dev/null
+++ b/Documentation/ABI/testing/sysfs-class-led-driver-qcom-lpg
@@ -0,0 +1,47 @@
+What:		/sys/class/leds/<led>/duration
+Date:		March 2017
+KernelVersion:	4.12
+Contact:	Bjorn Andersson <bjorn.andersson@linaro.org>
+Description:
+		Duration of one cycle through the pattern in the ramp
+		generator.
+
+What:		/sys/class/leds/<led>/oneshot
+Date:		March 2017
+KernelVersion:	4.12
+Contact:	Bjorn Andersson <bjorn.andersson@linaro.org>
+Description:
+		Stop the ramp generator after a single pass.
+
+What:		/sys/class/leds/<led>/pattern
+Date:		March 2017
+KernelVersion:	4.12
+Contact:	Bjorn Andersson <bjorn.andersson@linaro.org>
+Description:
+		Comma-separated list of duty cycle values to output from
+		the ramp generator. Values should be in the range of 0
+		to 511.
+
+What:		/sys/class/leds/<led>/pause_lo
+What:		/sys/class/leds/<led>/pause_hi
+Date:		March 2017
+KernelVersion:	4.12
+Contact:	Bjorn Andersson <bjorn.andersson@linaro.org>
+Description:
+		Pause time, in milliseconds, before and after one run
+		over the pattern.
+
+What:		/sys/class/leds/<led>/ping_pong
+Date:		March 2017
+KernelVersion:	4.12
+Contact:	Bjorn Andersson <bjorn.andersson@linaro.org>
+Description:
+		Reverse direction when the ramp generator reaches the
+		end of the pattern, rather than wrapping to the start.
+
+What:		/sys/class/leds/<led>/reverse
+Date:		March 2017
+KernelVersion:	4.12
+Contact:	Bjorn Andersson <bjorn.andersson@linaro.org>
+Description:
+		Run the ramp generator backwards over the pattern.
diff --git a/drivers/leds/Kconfig b/drivers/leds/Kconfig
index 275f467956ee..4b08d9802d5b 100644
--- a/drivers/leds/Kconfig
+++ b/drivers/leds/Kconfig
@@ -634,6 +634,13 @@ 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
+	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 6b8273736478..6390c1a36b13 100644
--- a/drivers/leds/Makefile
+++ b/drivers/leds/Makefile
@@ -61,6 +61,9 @@ obj-$(CONFIG_LEDS_MAX77693)		+= leds-max77693.o
 obj-$(CONFIG_LEDS_MAX8997)		+= leds-max8997.o
 obj-$(CONFIG_LEDS_LM355x)		+= leds-lm355x.o
 obj-$(CONFIG_LEDS_BLINKM)		+= leds-blinkm.o
+obj-$(CONFIG_LEDS_QCOM_LPG)		+= leds-qcom-lpg.o
+obj-$(CONFIG_LEDS_QCOM_LPG)		+= leds-qcom-lpg-lut.o
+obj-$(CONFIG_LEDS_QCOM_LPG)		+= leds-qcom-triled.o
 obj-$(CONFIG_LEDS_SYSCON)		+= leds-syscon.o
 obj-$(CONFIG_LEDS_VERSATILE)		+= leds-versatile.o
 obj-$(CONFIG_LEDS_MENF21BMC)		+= leds-menf21bmc.o
diff --git a/drivers/leds/leds-qcom-lpg-lut.c b/drivers/leds/leds-qcom-lpg-lut.c
new file mode 100644
index 000000000000..020f9cefda23
--- /dev/null
+++ b/drivers/leds/leds-qcom-lpg-lut.c
@@ -0,0 +1,299 @@
+/* Copyright (c) 2017 Linaro Ltd
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License version 2 and
+ * only version 2 as published by the Free Software Foundation.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ * GNU General Public License for more details.
+ */
+#include <linux/leds.h>
+#include <linux/module.h>
+#include <linux/of.h>
+#include <linux/of_device.h>
+#include <linux/platform_device.h>
+#include <linux/pm.h>
+#include <linux/pwm.h>
+#include <linux/regmap.h>
+#include <linux/slab.h>
+
+#include "leds-qcom-lpg.h"
+
+#define LPG_LUT_REG(x)		(0x40 + (x) * 2)
+#define RAMP_CONTROL_REG	0xc8
+
+static struct platform_driver lpg_lut_driver;
+
+/*
+ * lpg_lut_dev - LUT device context
+ * @dev:	struct device for the LUT device
+ * @map:	regmap for register access
+ * @reg:	base address for the LUT block
+ * @size:	number of LUT entries in LUT block
+ * @bitmap:	bitmap tracking occupied LUT entries
+ */
+struct lpg_lut_dev {
+	struct device *dev;
+	struct regmap *map;
+
+	u32 reg;
+	u32 size;
+
+	unsigned long bitmap[];
+};
+
+/*
+ * qcom_lpg_lut - context for a client and LUT device pair
+ * @ldev:	reference to a LUT device
+ * @start_mask:	mask of bits to use for synchronizing ramp generators
+ */
+struct qcom_lpg_lut {
+	struct lpg_lut_dev *ldev;
+	int start_mask;
+};
+
+static void lpg_lut_release(struct device *dev, void *res)
+{
+	struct qcom_lpg_lut *lut = res;
+
+	put_device(lut->ldev->dev);
+}
+
+/**
+ * qcom_lpg_lut_get() - acquire a handle to the LUT implementation
+ * @dev:	struct device reference of the client
+ *
+ * Returns a LUT context, or ERR_PTR on failure.
+ */
+struct qcom_lpg_lut *qcom_lpg_lut_get(struct device *dev)
+{
+	struct platform_device *pdev;
+	struct device_node *lut_node;
+	struct qcom_lpg_lut *lut;
+	u32 cell;
+	int ret;
+
+	lut_node = of_parse_phandle(dev->of_node, "qcom,lut", 0);
+	if (!lut_node)
+		return NULL;
+
+	ret = of_property_read_u32(dev->of_node, "cell-index", &cell);
+	if (ret) {
+		dev_err(dev, "lpg without cell-index\n");
+		return ERR_PTR(ret);
+	}
+
+	pdev = of_find_device_by_node(lut_node);
+	of_node_put(lut_node);
+	if (!pdev || !pdev->dev.driver)
+		return ERR_PTR(-EPROBE_DEFER);
+
+	if (pdev->dev.driver != &lpg_lut_driver.driver) {
+		dev_err(dev, "referenced node is not a lpg lut\n");
+		return ERR_PTR(-EINVAL);
+	}
+
+	lut = devres_alloc(lpg_lut_release, sizeof(*lut), GFP_KERNEL);
+	if (!lut)
+		return ERR_PTR(-ENOMEM);
+
+	lut->ldev = platform_get_drvdata(pdev);
+	lut->start_mask = BIT(cell - 1);
+
+	devres_add(dev, lut);
+
+	return lut;
+}
+EXPORT_SYMBOL_GPL(qcom_lpg_lut_get);
+
+/**
+ * qcom_lpg_lut_store() - store a sequence of levels in the LUT
+ * @lut:	LUT context acquired from qcom_lpg_lut_get()
+ * @values:	an array of values, in the range 0 <= x < 512
+ * @len:	length of the @values array
+ *
+ * Returns a qcom_lpg_pattern object, or ERR_PTR on failure.
+ *
+ * Patterns must be freed by calling qcom_lpg_lut_free()
+ */
+struct qcom_lpg_pattern *qcom_lpg_lut_store(struct qcom_lpg_lut *lut,
+					    const u16 *values, size_t len)
+{
+	struct qcom_lpg_pattern *pattern;
+	struct lpg_lut_dev *ldev = lut->ldev;
+	unsigned long lo_idx;
+	u8 val[2];
+	int i;
+
+	/* Hardware does not behave when LO_IDX == HI_IDX */
+	if (len == 1)
+		return ERR_PTR(-EINVAL);
+
+	lo_idx = bitmap_find_next_zero_area(ldev->bitmap, ldev->size, 0, len, 0);
+	if (lo_idx >= ldev->size)
+		return ERR_PTR(-ENOMEM);
+
+	pattern = kzalloc(sizeof(*pattern), GFP_KERNEL);
+	if (!pattern)
+		return ERR_PTR(-ENOMEM);
+
+	pattern->lut = lut;
+	pattern->lo_idx = lo_idx;
+	pattern->hi_idx = lo_idx + len - 1;
+
+	for (i = 0; i < len; i++) {
+		val[0] = values[i] & 0xff;
+		val[1] = values[i] >> 8;
+
+		regmap_bulk_write(ldev->map,
+				  ldev->reg + LPG_LUT_REG(lo_idx + i), val, 2);
+	}
+
+	bitmap_set(ldev->bitmap, lo_idx, len);
+
+	return pattern;
+}
+EXPORT_SYMBOL_GPL(qcom_lpg_lut_store);
+
+ssize_t qcom_lpg_lut_show(struct qcom_lpg_pattern *pattern, char *buf)
+{
+	struct qcom_lpg_lut *lut;
+	struct lpg_lut_dev *ldev;
+	unsigned long lo_idx;
+	char chunk[6]; /* 3 digits, a comma, a space and NUL */
+	char *bp = buf;
+	int len;
+	u8 val[2];
+	int ret;
+	int i;
+	int n;
+
+	if (!pattern)
+		return 0;
+
+	lut = pattern->lut;
+	ldev = lut->ldev;
+	lo_idx = pattern->lo_idx;
+
+	len = pattern->hi_idx - pattern->lo_idx + 1;
+	for (i = 0; i < len; i++) {
+		ret = regmap_bulk_read(ldev->map,
+				       ldev->reg + LPG_LUT_REG(lo_idx + i),
+				       &val, 2);
+		if (ret)
+			return ret;
+
+		n = snprintf(chunk, sizeof(chunk), "%d", val[0] | val[1] << 8);
+
+		/* ensure we have space for value, comma and NUL */
+		if (bp + n + 2 >= buf + PAGE_SIZE)
+			return -E2BIG;
+
+		memcpy(bp, chunk, n);
+		bp += n;
+
+		if (i < len - 1)
+			*bp++ = ',';
+		else
+			*bp++ = '\n';
+	}
+
+	*bp = '\0';
+
+	return bp - buf;
+}
+EXPORT_SYMBOL_GPL(qcom_lpg_lut_show);
+
+/**
+ * qcom_lpg_lut_free() - release LUT pattern and free entries
+ * @pattern:	reference to pattern to release
+ */
+void qcom_lpg_lut_free(struct qcom_lpg_pattern *pattern)
+{
+	struct qcom_lpg_lut *lut;
+	struct lpg_lut_dev *ldev;
+	int len;
+
+	if (!pattern)
+		return;
+
+	lut = pattern->lut;
+	ldev = lut->ldev;
+
+	len = pattern->hi_idx - pattern->lo_idx + 1;
+	bitmap_clear(ldev->bitmap, pattern->lo_idx, len);
+}
+EXPORT_SYMBOL_GPL(qcom_lpg_lut_free);
+
+/**
+ * qcom_lpg_lut_sync() - (re)start the ramp generator, to sync pattern
+ * @lut:	LUT device reference, to sync
+ */
+int qcom_lpg_lut_sync(struct qcom_lpg_lut *lut)
+{
+	struct lpg_lut_dev *ldev = lut->ldev;
+
+	return regmap_update_bits(ldev->map, ldev->reg + RAMP_CONTROL_REG,
+				  lut->start_mask, 0xff);
+}
+EXPORT_SYMBOL_GPL(qcom_lpg_lut_sync);
+
+static int lpg_lut_probe(struct platform_device *pdev)
+{
+	struct device_node *np = pdev->dev.of_node;
+	struct lpg_lut_dev *ldev;
+	size_t bitmap_size;
+	u32 size;
+	int ret;
+
+	ret = of_property_read_u32(np, "qcom,lut-size", &size);
+	if (ret) {
+		dev_err(&pdev->dev, "invalid LUT size\n");
+		return -EINVAL;
+	}
+
+	bitmap_size = BITS_TO_LONGS(size) / sizeof(unsigned long);
+	ldev = devm_kzalloc(&pdev->dev, sizeof(*ldev) + bitmap_size, GFP_KERNEL);
+	if (!ldev)
+		return -ENOMEM;
+
+	ldev->dev = &pdev->dev;
+	ldev->size = size;
+
+	ldev->map = dev_get_regmap(pdev->dev.parent, NULL);
+	if (!ldev->map) {
+		dev_err(&pdev->dev, "parent regmap unavailable\n");
+		return -ENXIO;
+	}
+
+	ret = of_property_read_u32(np, "reg", &ldev->reg);
+	if (ret) {
+		dev_err(&pdev->dev, "no register offset specified\n");
+		return -EINVAL;
+	}
+
+	platform_set_drvdata(pdev, ldev);
+
+	return 0;
+}
+
+static const struct of_device_id lpg_lut_of_table[] = {
+	{ .compatible = "qcom,spmi-lpg-lut" },
+	{},
+};
+MODULE_DEVICE_TABLE(of, lpg_lut_of_table);
+
+static struct platform_driver lpg_lut_driver = {
+	.probe = lpg_lut_probe,
+	.driver = {
+		.name = "qcom_lpg_lut",
+		.of_match_table = lpg_lut_of_table,
+	},
+};
+module_platform_driver(lpg_lut_driver);
+
+MODULE_DESCRIPTION("Qualcomm TRI LED driver");
+MODULE_LICENSE("GPL v2");
+
diff --git a/drivers/leds/leds-qcom-lpg.c b/drivers/leds/leds-qcom-lpg.c
new file mode 100644
index 000000000000..b60a446c5e8b
--- /dev/null
+++ b/drivers/leds/leds-qcom-lpg.c
@@ -0,0 +1,779 @@
+/*
+ * Copyright (c) 2017 Linaro Ltd
+ * Copyright (c) 2010-2012, The Linux Foundation. All rights reserved.
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License version 2 and
+ * only version 2 as published by the Free Software Foundation.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ * GNU General Public License for more details.
+ */
+#include <linux/leds.h>
+#include <linux/module.h>
+#include <linux/of.h>
+#include <linux/of_device.h>
+#include <linux/platform_device.h>
+#include <linux/pm.h>
+#include <linux/pwm.h>
+#include <linux/regmap.h>
+#include <linux/slab.h>
+
+#include "leds-qcom-lpg.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)
+
+/*
+ * lpg - LPG device context
+ * @dev:	struct device for LPG device
+ * @map:	regmap for register access
+ * @reg:	base address of the LPG device
+ * @dtest_line:	DTEST line for output, or 0 if disabled
+ * @dtest_value: DTEST line configuration
+ * @is_lpg:	operating as LPG, in contrast to simple PWM
+ * @cdev:	LED class object
+ * @tri_led:	reference to TRILED color object, optional
+ * @chip:	PWM-chip object, if operating in PWM mode
+ * @period_us:	period (in microseconds) of the generated pulses
+ * @pwm_value:	duty (in microseconds) of the generated pulses, overriden by LUT
+ * @enabled:	output enabled?
+ * @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
+ * @lut:	LUT context reference
+ * @pattern:	reference to allocated pattern with LUT
+ */
+
+struct lpg {
+	struct device *dev;
+	struct regmap *map;
+
+	u32 reg;
+	int dtest_line;
+	int dtest_value;
+
+	bool is_lpg;
+
+	struct led_classdev cdev;
+
+	struct qcom_tri_led *tri_led;
+
+	struct pwm_chip chip;
+
+	unsigned int period_us;
+
+	u16 pwm_value;
+	bool enabled;
+
+	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;
+
+	struct qcom_lpg_lut *lut;
+	struct qcom_lpg_pattern *pattern;
+};
+
+#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 *lpg, 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 == lpg->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 = 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;
+	}
+
+	lpg->clk = best_clk;
+	lpg->pre_div = best_div;
+	lpg->pre_div_exp = best_m;
+	lpg->pwm_size = n;
+
+	lpg->period_us = period_us;
+}
+
+static void lpg_calc_duty(struct lpg *lpg, unsigned long duty_us)
+{
+	unsigned long max = (1 << lpg->pwm_size) - 1;
+	unsigned long val;
+
+	/* Figure out pwm_value with overflow handling */
+	if (duty_us < 1 << (sizeof(val) * 8 - lpg->pwm_size))
+		val = (duty_us << lpg->pwm_size) / lpg->period_us;
+	else
+		val = duty_us / (lpg->period_us >> lpg->pwm_size);
+
+	if (val > max)
+		val = max;
+
+	lpg->pwm_value = val;
+}
+
+#define LPG_RESOLUTION_9BIT	BIT(4)
+
+static void lpg_apply_freq(struct lpg *lpg)
+{
+	unsigned long val;
+
+	if (!lpg->enabled)
+		return;
+
+	/* Clock register values are off-by-one from lpg_clk_table */
+	val = lpg->clk + 1;
+
+	if (lpg->pwm_size == 9)
+		val |= LPG_RESOLUTION_9BIT;
+	regmap_write(lpg->map, lpg->reg + LPG_SIZE_CLK_REG, val);
+
+	val = lpg->pre_div << 5 | lpg->pre_div_exp;
+	regmap_write(lpg->map, lpg->reg + LPG_PREDIV_CLK_REG, val);
+}
+
+#define LPG_ENABLE_GLITCH_REMOVAL	BIT(5)
+
+static void lpg_enable_glitch(struct lpg *lpg)
+{
+	regmap_update_bits(lpg->map, lpg->reg + PWM_TYPE_CONFIG_REG,
+			   LPG_ENABLE_GLITCH_REMOVAL, 0);
+}
+
+static void lpg_disable_glitch(struct lpg *lpg)
+{
+	regmap_update_bits(lpg->map, lpg->reg + PWM_TYPE_CONFIG_REG,
+			   LPG_ENABLE_GLITCH_REMOVAL,
+			   LPG_ENABLE_GLITCH_REMOVAL);
+}
+
+static void lpg_apply_pwm_value(struct lpg *lpg)
+{
+	u8 val[] = { lpg->pwm_value & 0xff, lpg->pwm_value >> 8 };
+
+	if (!lpg->enabled)
+		return;
+
+	regmap_bulk_write(lpg->map, lpg->reg + 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 *lpg)
+{
+	struct qcom_lpg_pattern *pattern = lpg->pattern;
+	unsigned int hi_pause;
+	unsigned int lo_pause;
+	unsigned int step;
+	unsigned int conf = 0;
+	int pattern_len;
+
+	if (!lpg->ramp_enabled || !pattern)
+		return;
+
+	pattern_len = pattern->hi_idx - pattern->lo_idx + 1;
+
+	step = DIV_ROUND_UP(lpg->ramp_duration_ms, pattern_len);
+	hi_pause = DIV_ROUND_UP(lpg->ramp_hi_pause_ms, step);
+	lo_pause = DIV_ROUND_UP(lpg->ramp_lo_pause_ms, step);
+
+	if (!lpg->ramp_reverse)
+		conf |= LPG_PATTERN_CONFIG_LO_TO_HI;
+	if (!lpg->ramp_oneshot)
+		conf |= LPG_PATTERN_CONFIG_REPEAT;
+	if (lpg->ramp_ping_pong)
+		conf |= LPG_PATTERN_CONFIG_TOGGLE;
+	if (lpg->ramp_hi_pause_ms)
+		conf |= LPG_PATTERN_CONFIG_PAUSE_HI;
+	if (lpg->ramp_lo_pause_ms)
+		conf |= LPG_PATTERN_CONFIG_PAUSE_LO;
+
+	regmap_write(lpg->map, lpg->reg + LPG_PATTERN_CONFIG_REG, conf);
+	regmap_write(lpg->map, lpg->reg + LPG_HI_IDX_REG, pattern->hi_idx);
+	regmap_write(lpg->map, lpg->reg + LPG_LO_IDX_REG, pattern->lo_idx);
+
+	regmap_write(lpg->map, lpg->reg + LPG_RAMP_DURATION_REG, step);
+	regmap_write(lpg->map, lpg->reg + LPG_HI_PAUSE_REG, hi_pause);
+	regmap_write(lpg->map, lpg->reg + LPG_LO_PAUSE_REG, lo_pause);
+
+	/* Trigger start of ramp generator(s) */
+	qcom_lpg_lut_sync(lpg->lut);
+}
+
+#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 *lpg)
+{
+	unsigned int ctrl;
+
+	ctrl = LPG_ENABLE_CONTROL_BUFFER_TRISTATE;
+
+	if (lpg->enabled)
+		ctrl |= LPG_ENABLE_CONTROL_OUTPUT;
+
+	if (lpg->pattern)
+		ctrl |= LPG_ENABLE_CONTROL_RAMP_GEN;
+	else
+		ctrl |= LPG_ENABLE_CONTROL_SRC_PWM;
+
+	regmap_write(lpg->map, lpg->reg + 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 (lpg->enabled)
+		lpg_apply_pwm_value(lpg);
+}
+
+#define LPG_SYNC_PWM	BIT(0)
+
+static void lpg_apply_sync(struct lpg *lpg)
+{
+	regmap_write(lpg->map, lpg->reg + PWM_SYNC_REG, LPG_SYNC_PWM);
+}
+
+static void lpg_apply_dtest(struct lpg *lpg)
+{
+	if (!lpg->dtest_line)
+		return;
+
+	regmap_write(lpg->map, lpg->reg + PWM_SEC_ACCESS_REG, 0xa5);
+	regmap_write(lpg->map, lpg->reg + PWM_DTEST_REG(lpg->dtest_line),
+		     lpg->dtest_value);
+}
+
+static void lpg_apply(struct lpg *lpg)
+{
+	lpg_disable_glitch(lpg);
+	lpg_apply_freq(lpg);
+	lpg_apply_pwm_value(lpg);
+	lpg_apply_control(lpg);
+	lpg_apply_sync(lpg);
+	lpg_apply_lut_control(lpg);
+	lpg_enable_glitch(lpg);
+
+	if (lpg->tri_led)
+		qcom_tri_led_set(lpg->tri_led, lpg->enabled);
+}
+
+static void lpg_brightnes_set(struct led_classdev *cdev,
+			      enum led_brightness value)
+{
+	struct lpg *lpg = container_of(cdev, struct lpg, cdev);
+	unsigned int duty_us;
+
+	if (value == LED_OFF) {
+		lpg->enabled = false;
+		lpg->ramp_enabled = false;
+	} else if (lpg->pattern) {
+		lpg_calc_freq(lpg, NSEC_PER_USEC);
+
+		lpg->enabled = true;
+		lpg->ramp_enabled = true;
+	} else {
+		lpg_calc_freq(lpg, NSEC_PER_USEC);
+
+		duty_us = value * lpg->period_us / cdev->max_brightness;
+		lpg_calc_duty(lpg, duty_us);
+		lpg->enabled = true;
+		lpg->ramp_enabled = false;
+	}
+
+	lpg_apply(lpg);
+}
+
+static int lpg_blink_set(struct led_classdev *cdev,
+			 unsigned long *delay_on, unsigned long *delay_off)
+{
+	struct lpg *lpg = container_of(cdev, struct lpg, cdev);
+	unsigned int period_us;
+	unsigned int duty_us;
+
+	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;
+
+	lpg_calc_freq(lpg, period_us);
+	lpg_calc_duty(lpg, duty_us);
+
+	lpg->enabled = true;
+	lpg->ramp_enabled = false;
+
+	lpg_apply(lpg);
+
+	return 0;
+}
+
+static enum led_brightness lpg_brightnes_get(struct led_classdev *cdev)
+{
+	struct lpg *lpg = container_of(cdev, struct lpg, cdev);
+	unsigned long max = (1 << lpg->pwm_size) - 1;
+
+	if (!lpg->enabled)
+		return LED_OFF;
+
+	return lpg->pwm_value * cdev->max_brightness / max;
+}
+
+static int lpg_pwm_apply(struct pwm_chip *chip, struct pwm_device *pwm,
+			 struct pwm_state *state)
+{
+	struct lpg *lpg = container_of(chip, struct lpg, chip);
+
+	lpg_calc_freq(lpg, state->period / NSEC_PER_USEC);
+	lpg_calc_duty(lpg, state->duty_cycle / NSEC_PER_USEC);
+	lpg->enabled = state->enabled;
+
+	lpg_apply(lpg);
+
+	state->polarity = PWM_POLARITY_NORMAL;
+	state->period = lpg->period_us * NSEC_PER_USEC;
+
+	return 0;
+}
+
+static const struct pwm_ops lpg_pwm_ops = {
+	.apply = lpg_pwm_apply,
+	.owner = THIS_MODULE,
+};
+
+static ssize_t lpg_attr_get(struct device *dev,
+			    struct device_attribute *attr,
+			    char *buf);
+static ssize_t lpg_attr_set(struct device *dev,
+			    struct device_attribute *attr,
+			    const char *buf, size_t count);
+
+static DEVICE_ATTR(ping_pong,	0600, lpg_attr_get, lpg_attr_set);
+static DEVICE_ATTR(oneshot,	0600, lpg_attr_get, lpg_attr_set);
+static DEVICE_ATTR(reverse,	0600, lpg_attr_get, lpg_attr_set);
+static DEVICE_ATTR(pattern,	0600, lpg_attr_get, lpg_attr_set);
+static DEVICE_ATTR(duration,	0600, lpg_attr_get, lpg_attr_set);
+static DEVICE_ATTR(pause_lo,	0600, lpg_attr_get, lpg_attr_set);
+static DEVICE_ATTR(pause_hi,	0600, lpg_attr_get, lpg_attr_set);
+
+static ssize_t lpg_pattern_store(struct lpg *lpg, const char *buf, size_t count)
+{
+	struct qcom_lpg_pattern *new_pattern;
+	unsigned long val;
+	char *sbegin;
+	u16 *pattern;
+	char *elem;
+	char *s;
+	int len = 0;
+	int ret = 0;
+
+	s = sbegin = kstrndup(buf, count, GFP_KERNEL);
+	if (!s)
+		return -ENOMEM;
+
+	pattern = kcalloc(count, sizeof(u16), GFP_KERNEL);
+	if (!pattern) {
+		ret = -ENOMEM;
+		goto out;
+	}
+
+	if (s[0] == '\0' || (s[0] == '\n' && s[1] == '\0')) {
+		qcom_lpg_lut_free(lpg->pattern);
+		lpg->pattern = NULL;
+	} else {
+		while ((elem = strsep(&s, " ,")) != NULL) {
+			ret = kstrtoul(elem, 10, &val);
+			if (ret)
+				goto out;
+
+			pattern[len++] = val;
+		}
+
+		new_pattern = qcom_lpg_lut_store(lpg->lut, pattern, len);
+		if (IS_ERR(new_pattern)) {
+			ret = PTR_ERR(new_pattern);
+			goto out;
+		}
+
+		qcom_lpg_lut_free(lpg->pattern);
+		lpg->pattern = new_pattern;
+	}
+
+out:
+	kfree(pattern);
+	kfree(sbegin);
+	return ret < 0 ? ret : count;
+}
+
+static ssize_t lpg_attr_get(struct device *dev,
+			    struct device_attribute *attr,
+			    char *buf)
+{
+	struct lpg *lpg = dev_get_drvdata(dev);
+
+	if (attr == &dev_attr_ping_pong)
+		return sprintf(buf, "%d\n", lpg->ramp_ping_pong);
+	else if (attr == &dev_attr_oneshot)
+		return sprintf(buf, "%d\n", lpg->ramp_oneshot);
+	else if (attr == &dev_attr_reverse)
+		return sprintf(buf, "%d\n", lpg->ramp_reverse);
+	else if (attr == &dev_attr_duration)
+		return sprintf(buf, "%ld\n", lpg->ramp_duration_ms);
+	else if (attr == &dev_attr_pause_lo)
+		return sprintf(buf, "%ld\n", lpg->ramp_lo_pause_ms);
+	else if (attr == &dev_attr_pause_hi)
+		return sprintf(buf, "%ld\n", lpg->ramp_hi_pause_ms);
+	else if (attr == &dev_attr_pattern)
+		return qcom_lpg_lut_show(lpg->pattern, buf);
+
+	return -EINVAL;
+}
+
+static ssize_t lpg_attr_set(struct device *dev,
+			    struct device_attribute *attr,
+			    const char *buf, size_t count)
+{
+	struct lpg *lpg = dev_get_drvdata(dev);
+	int ret = -EINVAL;
+
+	if (attr == &dev_attr_ping_pong)
+		ret = strtobool(buf, &lpg->ramp_ping_pong);
+	else if (attr == &dev_attr_oneshot)
+		ret = strtobool(buf, &lpg->ramp_oneshot);
+	else if (attr == &dev_attr_reverse)
+		ret = strtobool(buf, &lpg->ramp_reverse);
+	else if (attr == &dev_attr_duration)
+		ret = kstrtoul(buf, 10, &lpg->ramp_duration_ms);
+	else if (attr == &dev_attr_pause_lo)
+		ret = kstrtoul(buf, 10, &lpg->ramp_lo_pause_ms);
+	else if (attr == &dev_attr_pause_hi)
+		ret = kstrtoul(buf, 10, &lpg->ramp_hi_pause_ms);
+	else if (attr == &dev_attr_pattern)
+		ret = lpg_pattern_store(lpg, buf, count);
+
+	if (ret < 0)
+		return -EINVAL;
+
+	lpg_apply(lpg);
+	return count;
+}
+
+static struct attribute *lpg_attributes[] = {
+	&dev_attr_ping_pong.attr,
+	&dev_attr_oneshot.attr,
+	&dev_attr_reverse.attr,
+	&dev_attr_pattern.attr,
+	&dev_attr_duration.attr,
+	&dev_attr_pause_lo.attr,
+	&dev_attr_pause_hi.attr,
+	NULL
+};
+
+static const struct attribute_group lpg_attr_group = {
+	.attrs = lpg_attributes,
+};
+
+static const struct attribute_group *lpg_attr_groups[] = {
+	&lpg_attr_group,
+	NULL
+};
+
+static int lpg_register_pwm(struct lpg *lpg)
+{
+	int ret;
+
+	lpg->chip.base = -1;
+	lpg->chip.dev = lpg->dev;
+	lpg->chip.npwm = 1;
+	lpg->chip.ops = &lpg_pwm_ops;
+
+	ret = pwmchip_add(&lpg->chip);
+	if (ret)
+		dev_err(lpg->dev, "failed to add PWM chip: ret %d\n", ret);
+
+	return ret;
+}
+
+static int lpg_parse_lut(struct lpg *lpg)
+{
+	struct device_node *np = lpg->dev->of_node;
+	u16 *pattern;
+	u32 val;
+	int len;
+
+	lpg->lut = qcom_lpg_lut_get(lpg->dev);
+	if (IS_ERR_OR_NULL(lpg->lut))
+		return PTR_ERR(lpg->lut);
+
+	if (!of_find_property(np, "qcom,pattern", NULL))
+		return 0;
+
+	len = of_property_count_elems_of_size(np, "qcom,pattern", sizeof(u16));
+	if (len < 0)
+		return -EINVAL;
+
+	pattern = kcalloc(len, sizeof(u16), GFP_KERNEL);
+	if (!pattern)
+		return -ENOMEM;
+
+	of_property_read_u16_array(np, "qcom,pattern", pattern, len);
+
+	lpg->pattern = qcom_lpg_lut_store(lpg->lut, pattern, len);
+	kfree(pattern);
+	if (IS_ERR(lpg->pattern))
+		return PTR_ERR(lpg->pattern);
+
+	if (!of_property_read_u32(np, "qcom,pattern-length-ms", &val))
+		lpg->ramp_duration_ms = val;
+	if (!of_property_read_u32(np, "qcom,pattern-pause-lo-ms", &val))
+		lpg->ramp_lo_pause_ms = val;
+	if (!of_property_read_u32(np, "qcom,pattern-pause-hi-ms", &val))
+		lpg->ramp_hi_pause_ms = val;
+
+	lpg->ramp_ping_pong = of_property_read_bool(np, "qcom,pattern-ping-pong");
+	lpg->ramp_oneshot = of_property_read_bool(np, "qcom,pattern-oneshot");
+	lpg->ramp_reverse = of_property_read_bool(np, "qcom,pattern-reverse");
+
+	return 0;
+}
+
+static int lpg_register_led(struct lpg *lpg)
+{
+	struct device_node *np = lpg->dev->of_node;
+	const char *state;
+	int ret;
+
+	ret = lpg_parse_lut(lpg);
+	if (ret)
+		return ret;
+
+	/* Use label else node name */
+	lpg->cdev.name = of_get_property(np, "label", NULL) ? : np->name;
+	lpg->cdev.default_trigger = of_get_property(np, "linux,default-trigger", NULL);
+	lpg->cdev.brightness_set = lpg_brightnes_set;
+	lpg->cdev.brightness_get = lpg_brightnes_get;
+	lpg->cdev.blink_set = lpg_blink_set;
+	lpg->cdev.max_brightness = 255;
+	lpg->cdev.groups = lpg_attr_groups;
+
+	if (!of_property_read_string(np, "default-state", &state) &&
+	    !strcmp(state, "on"))
+		lpg->cdev.brightness = LED_FULL;
+	else
+		lpg->cdev.brightness = LED_OFF;
+
+	lpg_brightnes_set(&lpg->cdev, lpg->cdev.brightness);
+
+	ret = devm_led_classdev_register(lpg->dev, &lpg->cdev);
+	if (ret)
+		dev_err(lpg->dev, "unable to register \"%s\"\n", lpg->cdev.name);
+
+	return ret;
+}
+
+static int lpg_probe(struct platform_device *pdev)
+{
+	struct device_node *np = pdev->dev.of_node;
+	struct lpg *lpg;
+	u32 dtest[2];
+	int ret;
+
+	lpg = devm_kzalloc(&pdev->dev, sizeof(*lpg), GFP_KERNEL);
+	if (!lpg)
+		return -ENOMEM;
+
+	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 = of_property_read_u32(np, "reg", &lpg->reg);
+	if (ret) {
+		dev_err(&pdev->dev, "no register offset specified\n");
+		return -EINVAL;
+	}
+
+	if (!of_find_property(np, "#pwm-cells", NULL))
+		lpg->is_lpg = true;
+
+	lpg->tri_led = qcom_tri_led_get(&pdev->dev);
+	if (IS_ERR(lpg->tri_led))
+		return PTR_ERR(lpg->tri_led);
+
+	ret = of_property_read_u32_array(np, "qcom,dtest", dtest, 2);
+	if (!ret) {
+		lpg->dtest_line = dtest[0];
+		lpg->dtest_value = dtest[1];
+	}
+
+	if (lpg->is_lpg) {
+		ret = lpg_register_led(lpg);
+		if (ret)
+			return ret;
+	} else {
+		ret = lpg_register_pwm(lpg);
+		if (ret)
+			return ret;
+	}
+
+	lpg_apply_dtest(lpg);
+
+	platform_set_drvdata(pdev, lpg);
+
+	return 0;
+}
+
+static int lpg_remove(struct platform_device *pdev)
+{
+	struct lpg *lpg = platform_get_drvdata(pdev);
+
+	if (!lpg->is_lpg)
+		pwmchip_remove(&lpg->chip);
+
+	qcom_lpg_lut_free(lpg->pattern);
+
+	return 0;
+}
+
+static const struct of_device_id lpg_of_table[] = {
+	{ .compatible = "qcom,spmi-lpg" },
+	{},
+};
+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 TRI LED driver");
+MODULE_LICENSE("GPL v2");
diff --git a/drivers/leds/leds-qcom-lpg.h b/drivers/leds/leds-qcom-lpg.h
new file mode 100644
index 000000000000..f2abb106133d
--- /dev/null
+++ b/drivers/leds/leds-qcom-lpg.h
@@ -0,0 +1,30 @@
+#ifndef __LEDS_QCOM_LPG_H__
+#define __LEDS_QCOM_LPG_H__
+
+struct qcom_tri_led;
+struct qcom_lpg_lut;
+
+/*
+ * qcom_lpg_pattern - object tracking allocated LUT entries
+ * @lut:	reference to the client & LUT device context
+ * @lo_idx:	index of first entry in the LUT used by pattern
+ * @hi_idx:	index of the last entry in the LUT used by pattern
+ */
+struct qcom_lpg_pattern {
+	struct qcom_lpg_lut *lut;
+
+	unsigned int lo_idx;
+	unsigned int hi_idx;
+};
+
+struct qcom_tri_led *qcom_tri_led_get(struct device *dev);
+int qcom_tri_led_set(struct qcom_tri_led *tri, bool enabled);
+
+struct qcom_lpg_lut *qcom_lpg_lut_get(struct device *dev);
+struct qcom_lpg_pattern *qcom_lpg_lut_store(struct qcom_lpg_lut *lut,
+					    const u16 *values, size_t len);
+ssize_t qcom_lpg_lut_show(struct qcom_lpg_pattern *pattern, char *buf);
+void qcom_lpg_lut_free(struct qcom_lpg_pattern *pattern);
+int qcom_lpg_lut_sync(struct qcom_lpg_lut *lut);
+
+#endif
diff --git a/drivers/leds/leds-qcom-triled.c b/drivers/leds/leds-qcom-triled.c
new file mode 100644
index 000000000000..ce3de613be5b
--- /dev/null
+++ b/drivers/leds/leds-qcom-triled.c
@@ -0,0 +1,193 @@
+/* Copyright (c) 2017 Linaro Ltd
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License version 2 and
+ * only version 2 as published by the Free Software Foundation.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ * GNU General Public License for more details.
+ */
+#include <linux/leds.h>
+#include <linux/module.h>
+#include <linux/of.h>
+#include <linux/of_device.h>
+#include <linux/platform_device.h>
+#include <linux/pm.h>
+#include <linux/pwm.h>
+#include <linux/regmap.h>
+#include <linux/slab.h>
+
+#include "leds-qcom-lpg.h"
+
+#define TRI_LED_SRC_SEL	0x45
+#define TRI_LED_EN_CTL	0x46
+#define TRI_LED_ATC_CTL	0x47
+
+#define TRI_LED_COUNT	3
+
+static struct platform_driver tri_led_driver;
+
+/*
+ * tri_led_dev - TRILED device context
+ * @dev:	struct device reference
+ * @map:	regmap for register access
+ * @reg:	base address of TRILED block
+ */
+struct tri_led_dev {
+	struct device *dev;
+	struct regmap *map;
+
+	u32 reg;
+};
+
+/*
+ * qcom_tri_led - representation of a single color
+ * @tdev:	TRILED device reference
+ * @color:	color of this object 0 <= color < 3
+ */
+struct qcom_tri_led {
+	struct tri_led_dev *tdev;
+	u8 color;
+};
+
+static void tri_led_release(struct device *dev, void *res)
+{
+	struct qcom_tri_led *tri = res;
+	struct tri_led_dev *tdev = tri->tdev;
+
+	put_device(tdev->dev);
+}
+
+/**
+ * qcom_tri_led_get() - acquire a reference to a single color of the TRILED
+ * @dev:	struct device of the client
+ *
+ * Returned devres allocated TRILED color object, NULL if client lacks TRILED
+ * reference or ERR_PTR on failure.
+ */
+struct qcom_tri_led *qcom_tri_led_get(struct device *dev)
+{
+	struct platform_device *pdev;
+	struct of_phandle_args args;
+	struct qcom_tri_led *tri;
+	int ret;
+
+	ret = of_parse_phandle_with_fixed_args(dev->of_node,
+					       "qcom,tri-led", 1, 0, &args);
+	if (ret)
+		return NULL;
+
+	pdev = of_find_device_by_node(args.np);
+	of_node_put(args.np);
+	if (!pdev || !pdev->dev.driver)
+		return ERR_PTR(-EPROBE_DEFER);
+
+	if (pdev->dev.driver != &tri_led_driver.driver) {
+		dev_err(dev, "referenced node is not a tri-led\n");
+		return ERR_PTR(-EINVAL);
+	}
+
+	if (args.args[0] >= TRI_LED_COUNT) {
+		dev_err(dev, "invalid color\n");
+		return ERR_PTR(-EINVAL);
+	}
+
+	tri = devres_alloc(tri_led_release, sizeof(*tri), GFP_KERNEL);
+	if (!tri)
+		return ERR_PTR(-ENOMEM);
+
+	tri->tdev = platform_get_drvdata(pdev);
+	tri->color = args.args[0];
+
+	devres_add(dev, tri);
+
+	return tri;
+}
+EXPORT_SYMBOL_GPL(qcom_tri_led_get);
+
+/**
+ * qcom_tri_led_set() - enable/disable a TRILED output
+ * @tri:	TRILED color object reference
+ * @enable:	new state of the output
+ *
+ * Returns 0 on success, negative errno on failure.
+ */
+int qcom_tri_led_set(struct qcom_tri_led *tri, bool enable)
+{
+	struct tri_led_dev *tdev = tri->tdev;
+	unsigned int mask;
+	unsigned int val;
+
+	/* red, green, blue are mapped to bits 7, 6 and 5 respectively */
+	mask = BIT(7 - tri->color);
+	val = enable ? mask : 0;
+
+	return regmap_update_bits(tdev->map, tdev->reg + TRI_LED_EN_CTL,
+				  mask, val);
+}
+EXPORT_SYMBOL_GPL(qcom_tri_led_set);
+
+static int tri_led_probe(struct platform_device *pdev)
+{
+	struct device_node *np = pdev->dev.of_node;
+	struct tri_led_dev *tri;
+	u32 src_sel;
+	int ret;
+
+	tri = devm_kzalloc(&pdev->dev, sizeof(*tri), GFP_KERNEL);
+	if (!tri)
+		return -ENOMEM;
+
+	tri->dev = &pdev->dev;
+
+	tri->map = dev_get_regmap(pdev->dev.parent, NULL);
+	if (!tri->map) {
+		dev_err(&pdev->dev, "parent regmap unavailable\n");
+		return -ENXIO;
+	}
+
+	ret = of_property_read_u32(np, "reg", &tri->reg);
+	if (ret) {
+		dev_err(&pdev->dev, "no register offset specified\n");
+		return -EINVAL;
+	}
+
+	ret = of_property_read_u32(np, "qcom,power-source", &src_sel);
+	if (ret || src_sel == 2 || src_sel > 3) {
+		dev_err(&pdev->dev, "invalid power source\n");
+		return -EINVAL;
+	}
+
+	/* Disable automatic trickle charge LED */
+	regmap_write(tri->map, tri->reg + TRI_LED_ATC_CTL, 0);
+
+	/* Configure power source */
+	regmap_write(tri->map, tri->reg + TRI_LED_SRC_SEL, src_sel);
+
+	/* Default all outputs to off */
+	regmap_write(tri->map, tri->reg + TRI_LED_EN_CTL, 0);
+
+	platform_set_drvdata(pdev, tri);
+
+	return 0;
+}
+
+static const struct of_device_id tri_led_of_table[] = {
+	{ .compatible = "qcom,spmi-tri-led" },
+	{},
+};
+MODULE_DEVICE_TABLE(of, tri_led_of_table);
+
+static struct platform_driver tri_led_driver = {
+	.probe = tri_led_probe,
+	.driver = {
+		.name = "qcom_tri_led",
+		.of_match_table = tri_led_of_table,
+	},
+};
+module_platform_driver(tri_led_driver);
+
+MODULE_DESCRIPTION("Qualcomm TRI LED driver");
+MODULE_LICENSE("GPL v2");
-- 
2.12.0

[PATCH 2/2] DT: 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>
---
 .../devicetree/bindings/leds/leds-qcom-lpg.txt     | 194 +++++++++++++++++++++
 1 file changed, 194 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/leds/leds-qcom-lpg.txt

diff --git a/Documentation/devicetree/bindings/leds/leds-qcom-lpg.txt b/Documentation/devicetree/bindings/leds/leds-qcom-lpg.txt
new file mode 100644
index 000000000000..fb9edd89119d
--- /dev/null
+++ b/Documentation/devicetree/bindings/leds/leds-qcom-lpg.txt
@@ -0,0 +1,194 @@
+Binding for Qualcomm Light Pulse Generator
+
+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.
+Each of these are described individually below.
+
+= Lookup Table (LUT)
+
+- compatible:
+	Usage: required
+	Value type: <stringlist>
+	Definition: must be "qcom,spmi-lpg-lut"
+
+- reg:
+	Usage: required
+	Value type: <prop-encoded-array>
+	Definition: base address of the LUT block
+
+- qcom,lut-size:
+	Usage: required
+	Value type: <u32>
+	Definition: number of elements available in the lookup table
+
+= Light Pulse Generator (LPG)
+The Light Pulse Generator can operate either as a standard PWM controller or in
+a more advanced lookup-table based mode. These are described separately below.
+
+- compatible:
+	Usage: required
+	Value type: <stringlist>
+	Definition: must be "qcom,spmi-lpg"
+
+- reg:
+	Usage: required
+	Value type: <prop-encoded-array>
+	Definition: base address of the LPG block
+
+== PWM mode
+
+- #pwm-cells:
+	Usage: required
+	Value type: <u32>
+	Definition: must be 1
+
+== Lookup-table mode
+
+- cell-index:
+	Usage: required, when referencing a LUT
+	Value type: <u32>
+	Definition: id of the LPG, used to associate the LPG with a particular
+		    ramp generator in the LUT block
+
+- default-state:
+	Usage: optional
+	Value type: <string>
+	Definition: default state, as defined in common.txt
+
+- label:
+	Usage: optional
+	Value type: <string>
+	Definition: label of the LED, as defined in common.txt
+
+- linux,default-trigger:
+	Usage: optional
+	Value type: <string>
+	Definition: default trigger, as defined in common.txt
+
+- qcom,tri-led:
+	Usage: optional
+	Value type: <prop-encoded-array>
+	Definition: a phandle of a TRILED node and a single u32 denoting which
+		    output channel to control
+
+- qcom,lut:
+	Usage: optional
+	Value type: <prop-encoded-array>
+	Definition: phandle of a LUT node
+
+- qcom,dtest:
+	Usage: optional
+	Value type: <prop-encoded-array>
+	Definition: configures the output into an internal test line of the
+		    pmic. A first u32 defines which test line to use and the
+		    second cell configures how the value should be outputed
+		    (available lines and configuration differs between PMICs)
+
+- qcom,pattern:
+	Usage: optional
+	Value type: <u16-list>
+	Definition: list of 16 bit duty cycle values to make up the pattern to
+		    be programmed into the LUT. Values should be in the range
+		    [0,512).
+
+- qcom,pattern-length-ms:
+	Usage: optional
+	Value type: <u32>
+	Definition: duration, in milliseconds, of the ramp generator running
+		    one pass over the defined pattern
+
+- qcom,pattern-pause-lo-ms:
+	Usage: optional
+	Value type: <u32>
+	Definition: duration, in milliseconds, for the ramp generator to pause
+		    before iterating over the pattern
+
+- qcom,pattern-pause-hi-ms:
+	Usage: optional
+	Value type: <u32>
+	Definition: duration, in milliseconds, for the ramp generator to pause
+		    after iterating over the pattern
+
+- qcom,pattern-ping-pong:
+	Usage: optional
+	Value type: <boolean>
+	Definition: denotes that the ramp generator should reverse direction
+		    when reaching the end of the pattern, instead of wrapping
+		    to the beginning
+
+- qcom,pattern-oneshot:
+	Usage: optional
+	Value type: <boolean>
+	Definition: denotes that the ramp generator should stop after a single
+		    pass over the pattern
+
+- qcom,pattern-reverse:
+	Usage: optional
+	Value type: <boolean>
+	Definition: denotes that the ramp generator should operate backwards
+		    over the pattern
+
+= LED Current Sink (TRILED)
+
+- compatible:
+	Usage: required
+	Value type: <stringlist>
+	Definition: must be "qcom,spmi-tri-led"
+
+- reg:
+	Usage: required
+	Value type: <prop-encoded-array>
+	Definition: base address of the TRILED block
+
+- qcom,power-source:
+	Usage: required
+	Value type: <u32>
+	Definition: power-source used to drive the output, as defined in the
+		    datasheet
+
+= EXAMPLE:
+The following example defines a single output of the PMI8994, sinking current
+into a LED in a natural pulsating pattern:
+
+&spmi_bus {
+	pmic@3 {
+		compatible = "qcom,pmi8994", "qcom,spmi-pmic";
+		reg = <0x3 SPMI_USID>;
+		#address-cells = <1>;
+		#size-cells = <0>;
+
+		pmi8994_lpg_lut: lpg-lut@b000 {
+			compatible = "qcom,spmi-lpg-lut";
+			reg = <0xb000>;
+
+			qcom,lut-size = <24>;
+		};
+
+		lpg@b200 {
+			compatible = "qcom,spmi-lpg";
+			reg = <0xb200>;
+
+			cell-index = <2>;
+
+			label = "lpg:green:user0";
+
+			qcom,tri-led = <&pmi8994_tri_led 1>;
+			qcom,lut = <&pmi8994_lpg_lut>;
+
+			qcom,pattern = /bits/ 16 <9 20 42 86 158 256 353
+						  425 469 491 502 507>;
+			qcom,pattern-length-ms = <1337>;
+			qcom,pattern-ping-pong;
+
+			default-state = "on";
+		};
+
+		pmi8994_tri_led: tri-led@d000 {
+			compatible = "qcom,spmi-tri-led";
+			reg = <0xd000>;
+
+			qcom,power-source = <1>;
+		};
+	};
+};
-- 
2.12.0

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

[Pavel Machek]

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

Hi!

> 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.

Ok, this is not first hardware that supports something like this. We
have similar hardware that can do blinking on Nokia N900 -- please
take a look at leds-lp55*.c

And it would be really good to provide hardware abstraction. We really
don't want to have different userspace for LPG and for N900 and for
...

Which probably means finding subset that makes sense for everyone.

Hmm. What is difference between "ping_pong" and "reverse"? And do we
really want it? That seems little .. too specialized.

How are different channels on RGB LED synchronized?

> +What:		/sys/class/leds/<led>/pattern
> +Date:		March 2017
> +KernelVersion:	4.12
> +Contact:	Bjorn Andersson <bjorn.andersson@linaro.org>
> +Description:
> +		Comma-separated list of duty cycle values to output from
> +		the ramp generator. Values should be in the range of 0
> +		to 511.

We normally do "space separated" in sysfs.

Can your engine do "smooth transitions"? For example if you want to
slowly turn on the LED on, can you do something more clever than

0, 1, 2, 3, 4, 5, 6, 7, 8, 9, 10, 11, 12, 13, 14, 15, 16, 17, 18, 19,
20, 21, 22, 23, 24, 25, 26, 27, 28, 29, 30, 31, 32, 33, 34, 35, 36,
...
496, 497, 498, 499, 500, 501, 502, 503, 504, 505, 506, 507, 508, 509,
510, 511

? What is the maximum length of the pattern?

Could we do patterns in form of "delay brightness delay brightness"
.... ?

> +static enum led_brightness lpg_brightnes_get(struct led_classdev *cdev)
> +{
> +	struct lpg *lpg = container_of(cdev, struct lpg, cdev);
> +	unsigned long max = (1 << lpg->pwm_size) - 1;
> +
> +	if (!lpg->enabled)
> +		return LED_OFF;
> +
> +	return lpg->pwm_value * cdev->max_brightness / max;
> +}

Does this return something reasonable when pattern is running?

									Pavel
-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 181 bytes --]

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

[Bjorn Andersson]

On Thu 23 Mar 13:37 PDT 2017, Pavel Machek wrote:

> Hi!
> 
> > 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.
> 
> Ok, this is not first hardware that supports something like this. We
> have similar hardware that can do blinking on Nokia N900 -- please
> take a look at leds-lp55*.c
> 

I have not worked with the LP55xx chips before, they look quite capable!

> And it would be really good to provide hardware abstraction. We really
> don't want to have different userspace for LPG and for N900 and for
> ...
> 
> Which probably means finding subset that makes sense for everyone.
> 

While I share your concern of userspace differences I'm not sure how to
expose the advanced features of the LP55xx series and the LPG's limited
pattern-controlled PWM.

> Hmm. What is difference between "ping_pong" and "reverse"? And do we
> really want it? That seems little .. too specialized.
> 

Writing the appropriate bit in RAMP_CONTROL_REG of the LUT block
qcom_lpg_lut_sync() resets the ramp-walker; ping-pong causes the
ramp-walker to make one round-trip run over the pattern, while reverse
means that the ramp-walker should start from the hi-index.

I.e. with the pattern [1,2,3] we get:

ping-pong: [1,2,3,2,1]
reverse: [3,2,1]
ping-pong and reverse: [3,2,1,2,3]

> How are different channels on RGB LED synchronized?
> 

You can reset multiple ramp-generators simultaneously, which would cause
the channels to be synchronized. I have not implemented this though.

> > +What:		/sys/class/leds/<led>/pattern
> > +Date:		March 2017
> > +KernelVersion:	4.12
> > +Contact:	Bjorn Andersson <bjorn.andersson@linaro.org>
> > +Description:
> > +		Comma-separated list of duty cycle values to output from
> > +		the ramp generator. Values should be in the range of 0
> > +		to 511.
> 
> We normally do "space separated" in sysfs.
> 

Okay, I'm fine with that.

> Can your engine do "smooth transitions"? For example if you want to
> slowly turn on the LED on, can you do something more clever than
> 
> 0, 1, 2, 3, 4, 5, 6, 7, 8, 9, 10, 11, 12, 13, 14, 15, 16, 17, 18, 19,
> 20, 21, 22, 23, 24, 25, 26, 27, 28, 29, 30, 31, 32, 33, 34, 35, 36,
> ...
> 496, 497, 498, 499, 500, 501, 502, 503, 504, 505, 506, 507, 508, 509,
> 510, 511
> 

There's nothing beyond "run the pattern", so this is exactly what you
would have to do if you need a perfectly smooth transition.

> ? What is the maximum length of the pattern?
> 

It differs between the various PMICs implementing this. I've seen cases
of 24 slots and 64 slots.

> Could we do patterns in form of "delay brightness delay brightness"
> .... ?
> 

The ramp-walker is configured to tick with a fixed clock (in
milliseconds) and the PWM will be configured with the duty-cycle of the
current element of the ramp.

So you can do this, given that all your "delay" is the same fixed delay,
which is max 511 milliseconds.

> > +static enum led_brightness lpg_brightnes_get(struct led_classdev *cdev)
> > +{
> > +	struct lpg *lpg = container_of(cdev, struct lpg, cdev);
> > +	unsigned long max = (1 << lpg->pwm_size) - 1;
> > +
> > +	if (!lpg->enabled)
> > +		return LED_OFF;
> > +
> > +	return lpg->pwm_value * cdev->max_brightness / max;
> > +}
> 
> Does this return something reasonable when pattern is running?
> 

No, I'll fix that. I treat brightness as a boolean if a pattern is
provided, but I'm concerned about modifying max_brightness to reflect
this.


As you can see from these answers, the hardware is quite limited in
comparison to the LP55xx series. It would make some sense to feed e.g. a
mathematical formula to the kernel and have the driver map that to
patterns for the LPG or program code in the case of LP55xx.

But this would add quite a bit of complexity and with hardware as
limited as the 24-slot LPG we likely need that direct control of the
patterns.

Regards,
Bjorn

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

[Rob Herring]

On Thu, Mar 23, 2017 at 09:37:49PM +0100, Pavel Machek wrote:
> Hi!
> 
> > 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.
> 
> Ok, this is not first hardware that supports something like this. We
> have similar hardware that can do blinking on Nokia N900 -- please
> take a look at leds-lp55*.c

And perhaps some alignment on the bindings too if the N900 has bindings.

> And it would be really good to provide hardware abstraction. We really
> don't want to have different userspace for LPG and for N900 and for

I'm interested in what this looks like as several AOSP platforms do 
tri-color LEDs with custom sysfs extensions.

Do any of the Dragonboards have tri-color LEDs?

Rob

Re: [PATCH 2/2] DT: leds: Add Qualcomm Light Pulse Generator binding

[Rob Herring]

On Wed, Mar 22, 2017 at 10:54:35PM -0700, Bjorn Andersson wrote:
> 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>
> ---
>  .../devicetree/bindings/leds/leds-qcom-lpg.txt     | 194 +++++++++++++++++++++
>  1 file changed, 194 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/leds/leds-qcom-lpg.txt
> 
> diff --git a/Documentation/devicetree/bindings/leds/leds-qcom-lpg.txt b/Documentation/devicetree/bindings/leds/leds-qcom-lpg.txt
> new file mode 100644
> index 000000000000..fb9edd89119d
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/leds/leds-qcom-lpg.txt
> @@ -0,0 +1,194 @@
> +Binding for Qualcomm Light Pulse Generator
> +
> +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.
> +Each of these are described individually below.
> +
> += Lookup Table (LUT)
> +
> +- compatible:
> +	Usage: required
> +	Value type: <stringlist>
> +	Definition: must be "qcom,spmi-lpg-lut"
> +
> +- reg:
> +	Usage: required
> +	Value type: <prop-encoded-array>
> +	Definition: base address of the LUT block
> +
> +- qcom,lut-size:
> +	Usage: required
> +	Value type: <u32>
> +	Definition: number of elements available in the lookup table
> +
> += Light Pulse Generator (LPG)
> +The Light Pulse Generator can operate either as a standard PWM controller or in
> +a more advanced lookup-table based mode. These are described separately below.
> +
> +- compatible:
> +	Usage: required
> +	Value type: <stringlist>
> +	Definition: must be "qcom,spmi-lpg"
> +
> +- reg:
> +	Usage: required
> +	Value type: <prop-encoded-array>
> +	Definition: base address of the LPG block
> +
> +== PWM mode
> +
> +- #pwm-cells:
> +	Usage: required
> +	Value type: <u32>
> +	Definition: must be 1
> +
> +== Lookup-table mode
> +
> +- cell-index:

This is a standard though not used property name. Perhaps "reg" or a 
vendor property instead.

> +	Usage: required, when referencing a LUT
> +	Value type: <u32>
> +	Definition: id of the LPG, used to associate the LPG with a particular
> +		    ramp generator in the LUT block
> +
> +- default-state:
> +	Usage: optional
> +	Value type: <string>
> +	Definition: default state, as defined in common.txt
> +
> +- label:
> +	Usage: optional
> +	Value type: <string>
> +	Definition: label of the LED, as defined in common.txt
> +
> +- linux,default-trigger:
> +	Usage: optional
> +	Value type: <string>
> +	Definition: default trigger, as defined in common.txt
> +
> +- qcom,tri-led:
> +	Usage: optional
> +	Value type: <prop-encoded-array>
> +	Definition: a phandle of a TRILED node and a single u32 denoting which
> +		    output channel to control
> +
> +- qcom,lut:
> +	Usage: optional
> +	Value type: <prop-encoded-array>
> +	Definition: phandle of a LUT node
> +
> +- qcom,dtest:
> +	Usage: optional
> +	Value type: <prop-encoded-array>
> +	Definition: configures the output into an internal test line of the
> +		    pmic. A first u32 defines which test line to use and the
> +		    second cell configures how the value should be outputed
> +		    (available lines and configuration differs between PMICs)
> +
> +- qcom,pattern:
> +	Usage: optional
> +	Value type: <u16-list>
> +	Definition: list of 16 bit duty cycle values to make up the pattern to
> +		    be programmed into the LUT. Values should be in the range
> +		    [0,512).
> +
> +- qcom,pattern-length-ms:
> +	Usage: optional
> +	Value type: <u32>
> +	Definition: duration, in milliseconds, of the ramp generator running
> +		    one pass over the defined pattern
> +
> +- qcom,pattern-pause-lo-ms:
> +	Usage: optional
> +	Value type: <u32>
> +	Definition: duration, in milliseconds, for the ramp generator to pause
> +		    before iterating over the pattern
> +
> +- qcom,pattern-pause-hi-ms:
> +	Usage: optional
> +	Value type: <u32>
> +	Definition: duration, in milliseconds, for the ramp generator to pause
> +		    after iterating over the pattern
> +
> +- qcom,pattern-ping-pong:
> +	Usage: optional
> +	Value type: <boolean>
> +	Definition: denotes that the ramp generator should reverse direction
> +		    when reaching the end of the pattern, instead of wrapping
> +		    to the beginning
> +
> +- qcom,pattern-oneshot:
> +	Usage: optional
> +	Value type: <boolean>
> +	Definition: denotes that the ramp generator should stop after a single
> +		    pass over the pattern
> +
> +- qcom,pattern-reverse:
> +	Usage: optional
> +	Value type: <boolean>
> +	Definition: denotes that the ramp generator should operate backwards
> +		    over the pattern

The pattern related properties should be common if we put them in DT 
which I think is debatable.

> +
> += LED Current Sink (TRILED)
> +
> +- compatible:
> +	Usage: required
> +	Value type: <stringlist>
> +	Definition: must be "qcom,spmi-tri-led"
> +
> +- reg:
> +	Usage: required
> +	Value type: <prop-encoded-array>
> +	Definition: base address of the TRILED block
> +
> +- qcom,power-source:
> +	Usage: required
> +	Value type: <u32>
> +	Definition: power-source used to drive the output, as defined in the
> +		    datasheet
> +
> += EXAMPLE:
> +The following example defines a single output of the PMI8994, sinking current
> +into a LED in a natural pulsating pattern:
> +
> +&spmi_bus {
> +	pmic@3 {
> +		compatible = "qcom,pmi8994", "qcom,spmi-pmic";

typo.

> +		reg = <0x3 SPMI_USID>;
> +		#address-cells = <1>;
> +		#size-cells = <0>;
> +
> +		pmi8994_lpg_lut: lpg-lut@b000 {
> +			compatible = "qcom,spmi-lpg-lut";
> +			reg = <0xb000>;
> +
> +			qcom,lut-size = <24>;
> +		};
> +
> +		lpg@b200 {
> +			compatible = "qcom,spmi-lpg";
> +			reg = <0xb200>;
> +
> +			cell-index = <2>;
> +
> +			label = "lpg:green:user0";
> +
> +			qcom,tri-led = <&pmi8994_tri_led 1>;
> +			qcom,lut = <&pmi8994_lpg_lut>;
> +
> +			qcom,pattern = /bits/ 16 <9 20 42 86 158 256 353
> +						  425 469 491 502 507>;
> +			qcom,pattern-length-ms = <1337>;
> +			qcom,pattern-ping-pong;
> +
> +			default-state = "on";
> +		};
> +
> +		pmi8994_tri_led: tri-led@d000 {

It may make more sense to make the LED(s) and their properties a sub 
node of this. You could always use the PWM binding to link back to the 
LPG. The pattern/LUT is really just a queue of PWM settings. That's not 
all that different than a PWM based audio buzzer. There was a DMA based 
PWM binding the other day for audio.

Rob

> +			compatible = "qcom,spmi-tri-led";
> +			reg = <0xd000>;
> +
> +			qcom,power-source = <1>;
> +		};
> +	};
> +};
> -- 
> 2.12.0
> 

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

[Bjorn Andersson]

On Tue 28 Mar 19:17 PDT 2017, Rob Herring wrote:

> On Thu, Mar 23, 2017 at 09:37:49PM +0100, Pavel Machek wrote:
> > Hi!
> > 
> > > 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.
> > 
> > Ok, this is not first hardware that supports something like this. We
> > have similar hardware that can do blinking on Nokia N900 -- please
> > take a look at leds-lp55*.c
> 
> And perhaps some alignment on the bindings too if the N900 has bindings.
> 

There is a binding for ti,lp55xx, but there's nothing I can reuse from
that binding...because it's completely different hardware.

> > And it would be really good to provide hardware abstraction. We really
> > don't want to have different userspace for LPG and for N900 and for
> 
> I'm interested in what this looks like as several AOSP platforms do 
> tri-color LEDs with custom sysfs extensions.
> 

How to model RGB LEDs has been discussed many times before and I was
hoping for that discussion to come to some conclusion during the last 2
years, but now I couldn't wait more - we need this driver for db820c.

With this driver, as with many existing, you will have 3 LEDs that you
set independently.

I did implement blinking by using the PWM straight off, so you can't set
brightness or synchronize the multiple channels. Perhaps this should be
changed to use the ramp generator.

To synchronize patterns I suggest that we extend the LUT binding to
describe groups and when any LPG trigger a restart of the pattern-walker
we trigger all that are grouped.

These two changes combined allows you to set brightness and blink with a
RGB-LED.


But I will have to dig up some hardware that uses the LPG for driving a
RGB-LED to be able to test this (and I do prefer that to be done with
some incremental patches at some later time, if acceptable).

> Do any of the Dragonboards have tri-color LEDs?
> 

No.

Regards,
Bjorn

Re: [PATCH 2/2] DT: leds: Add Qualcomm Light Pulse Generator binding

[Bjorn Andersson]

On Tue 28 Mar 19:26 PDT 2017, Rob Herring wrote:

> On Wed, Mar 22, 2017 at 10:54:35PM -0700, Bjorn Andersson wrote:
> > 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>
> > ---
> >  .../devicetree/bindings/leds/leds-qcom-lpg.txt     | 194 +++++++++++++++++++++
> >  1 file changed, 194 insertions(+)
> >  create mode 100644 Documentation/devicetree/bindings/leds/leds-qcom-lpg.txt
> > 
> > diff --git a/Documentation/devicetree/bindings/leds/leds-qcom-lpg.txt b/Documentation/devicetree/bindings/leds/leds-qcom-lpg.txt
> > new file mode 100644
> > index 000000000000..fb9edd89119d
> > --- /dev/null
> > +++ b/Documentation/devicetree/bindings/leds/leds-qcom-lpg.txt
> > @@ -0,0 +1,194 @@
> > +Binding for Qualcomm Light Pulse Generator
> > +
> > +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.
> > +Each of these are described individually below.
> > +
> > += Lookup Table (LUT)
> > +
> > +- compatible:
> > +	Usage: required
> > +	Value type: <stringlist>
> > +	Definition: must be "qcom,spmi-lpg-lut"
> > +
> > +- reg:
> > +	Usage: required
> > +	Value type: <prop-encoded-array>
> > +	Definition: base address of the LUT block
> > +
> > +- qcom,lut-size:
> > +	Usage: required
> > +	Value type: <u32>
> > +	Definition: number of elements available in the lookup table
> > +
> > += Light Pulse Generator (LPG)
> > +The Light Pulse Generator can operate either as a standard PWM controller or in
> > +a more advanced lookup-table based mode. These are described separately below.
> > +
> > +- compatible:
> > +	Usage: required
> > +	Value type: <stringlist>
> > +	Definition: must be "qcom,spmi-lpg"
> > +
> > +- reg:
> > +	Usage: required
> > +	Value type: <prop-encoded-array>
> > +	Definition: base address of the LPG block
> > +
> > +== PWM mode
> > +
> > +- #pwm-cells:
> > +	Usage: required
> > +	Value type: <u32>
> > +	Definition: must be 1
> > +
> > +== Lookup-table mode
> > +
> > +- cell-index:
> 
> This is a standard though not used property name. Perhaps "reg" or a 
> vendor property instead.
> 

The node already has a "reg", this is the "natural" id of the
LPG-channel, as used to reference a certain ramp-generator in the LUT.

I did model this as an argument of the qcom,lut property below, but felt
it's not a question of "which LUT" or any "configuration of the LUT" it
is a property of the LPG.

I can convert this to a qcom,lpg-id or something like that if you
prefer.

> > +	Usage: required, when referencing a LUT
> > +	Value type: <u32>
> > +	Definition: id of the LPG, used to associate the LPG with a particular
> > +		    ramp generator in the LUT block
> > +
> > +- default-state:
> > +	Usage: optional
> > +	Value type: <string>
> > +	Definition: default state, as defined in common.txt
> > +
> > +- label:
> > +	Usage: optional
> > +	Value type: <string>
> > +	Definition: label of the LED, as defined in common.txt
> > +
> > +- linux,default-trigger:
> > +	Usage: optional
> > +	Value type: <string>
> > +	Definition: default trigger, as defined in common.txt
> > +
> > +- qcom,tri-led:
> > +	Usage: optional
> > +	Value type: <prop-encoded-array>
> > +	Definition: a phandle of a TRILED node and a single u32 denoting which
> > +		    output channel to control
> > +
> > +- qcom,lut:
> > +	Usage: optional
> > +	Value type: <prop-encoded-array>
> > +	Definition: phandle of a LUT node
> > +
> > +- qcom,dtest:
> > +	Usage: optional
> > +	Value type: <prop-encoded-array>
> > +	Definition: configures the output into an internal test line of the
> > +		    pmic. A first u32 defines which test line to use and the
> > +		    second cell configures how the value should be outputed
> > +		    (available lines and configuration differs between PMICs)
> > +
> > +- qcom,pattern:
> > +	Usage: optional
> > +	Value type: <u16-list>
> > +	Definition: list of 16 bit duty cycle values to make up the pattern to
> > +		    be programmed into the LUT. Values should be in the range
> > +		    [0,512).
> > +
> > +- qcom,pattern-length-ms:
> > +	Usage: optional
> > +	Value type: <u32>
> > +	Definition: duration, in milliseconds, of the ramp generator running
> > +		    one pass over the defined pattern
> > +
> > +- qcom,pattern-pause-lo-ms:
> > +	Usage: optional
> > +	Value type: <u32>
> > +	Definition: duration, in milliseconds, for the ramp generator to pause
> > +		    before iterating over the pattern
> > +
> > +- qcom,pattern-pause-hi-ms:
> > +	Usage: optional
> > +	Value type: <u32>
> > +	Definition: duration, in milliseconds, for the ramp generator to pause
> > +		    after iterating over the pattern
> > +
> > +- qcom,pattern-ping-pong:
> > +	Usage: optional
> > +	Value type: <boolean>
> > +	Definition: denotes that the ramp generator should reverse direction
> > +		    when reaching the end of the pattern, instead of wrapping
> > +		    to the beginning
> > +
> > +- qcom,pattern-oneshot:
> > +	Usage: optional
> > +	Value type: <boolean>
> > +	Definition: denotes that the ramp generator should stop after a single
> > +		    pass over the pattern
> > +
> > +- qcom,pattern-reverse:
> > +	Usage: optional
> > +	Value type: <boolean>
> > +	Definition: denotes that the ramp generator should operate backwards
> > +		    over the pattern
> 
> The pattern related properties should be common if we put them in DT 
> which I think is debatable.
> 

A few years back I saw one other chip that had a similar pattern style,
using these properties for the LP5xx - that is what's being requested -
would have to be implemented by something reading the pattern and
generating firmware to be run on the chip. This should be possible to
do, but there are a lot functionality in the LP55xx chips that you would
not be able to use with such an approach.

> > +
> > += LED Current Sink (TRILED)
> > +
> > +- compatible:
> > +	Usage: required
> > +	Value type: <stringlist>
> > +	Definition: must be "qcom,spmi-tri-led"
> > +
> > +- reg:
> > +	Usage: required
> > +	Value type: <prop-encoded-array>
> > +	Definition: base address of the TRILED block
> > +
> > +- qcom,power-source:
> > +	Usage: required
> > +	Value type: <u32>
> > +	Definition: power-source used to drive the output, as defined in the
> > +		    datasheet
> > +
> > += EXAMPLE:
> > +The following example defines a single output of the PMI8994, sinking current
> > +into a LED in a natural pulsating pattern:
> > +
> > +&spmi_bus {
> > +	pmic@3 {
> > +		compatible = "qcom,pmi8994", "qcom,spmi-pmic";
> 
> typo.
> 

Sorry, I don't see the typo.

> > +		reg = <0x3 SPMI_USID>;
> > +		#address-cells = <1>;
> > +		#size-cells = <0>;
> > +
> > +		pmi8994_lpg_lut: lpg-lut@b000 {
> > +			compatible = "qcom,spmi-lpg-lut";
> > +			reg = <0xb000>;
> > +
> > +			qcom,lut-size = <24>;
> > +		};
> > +
> > +		lpg@b200 {
> > +			compatible = "qcom,spmi-lpg";
> > +			reg = <0xb200>;
> > +
> > +			cell-index = <2>;
> > +
> > +			label = "lpg:green:user0";
> > +
> > +			qcom,tri-led = <&pmi8994_tri_led 1>;
> > +			qcom,lut = <&pmi8994_lpg_lut>;
> > +
> > +			qcom,pattern = /bits/ 16 <9 20 42 86 158 256 353
> > +						  425 469 491 502 507>;
> > +			qcom,pattern-length-ms = <1337>;
> > +			qcom,pattern-ping-pong;
> > +
> > +			default-state = "on";
> > +		};
> > +
> > +		pmi8994_tri_led: tri-led@d000 {
> 
> It may make more sense to make the LED(s) and their properties a sub 
> node of this. You could always use the PWM binding to link back to the 
> LPG. The pattern/LUT is really just a queue of PWM settings. That's not 
> all that different than a PWM based audio buzzer. There was a DMA based 
> PWM binding the other day for audio.
> 

The TRILED is a separate hardware block from the LPG and for 3
(predefined) LPG channel it serves as one of the options for routing the
signal out of the PMIC. 

As an example, on DB820c I drive the fourth user-LED by routing one of
the LPG channels to a MPP configured as current-sink.

Regards,
Bjorn

Re: [PATCH 2/2] DT: leds: Add Qualcomm Light Pulse Generator binding

[Pavel Machek]

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

Hi!

> +- qcom,pattern:
> +	Usage: optional
> +	Value type: <u16-list>
> +	Definition: list of 16 bit duty cycle values to make up the pattern to
> +		    be programmed into the LUT. Values should be in the range
> +		    [0,512).
> +
> +- qcom,pattern-length-ms:
> +	Usage: optional
> +	Value type: <u32>
> +	Definition: duration, in milliseconds, of the ramp generator running
> +		    one pass over the defined pattern
> +
> +- qcom,pattern-pause-lo-ms:
> +	Usage: optional
> +	Value type: <u32>
> +	Definition: duration, in milliseconds, for the ramp generator to pause
> +		    before iterating over the pattern
> +
> +- qcom,pattern-pause-hi-ms:
> +	Usage: optional
> +	Value type: <u32>
> +	Definition: duration, in milliseconds, for the ramp generator to pause
> +		    after iterating over the pattern
> +
> +- qcom,pattern-ping-pong:
> +	Usage: optional
> +	Value type: <boolean>
> +	Definition: denotes that the ramp generator should reverse direction
> +		    when reaching the end of the pattern, instead of wrapping
> +		    to the beginning
> +
> +- qcom,pattern-oneshot:
> +	Usage: optional
> +	Value type: <boolean>
> +	Definition: denotes that the ramp generator should stop after a single
> +		    pass over the pattern
> +
> +- qcom,pattern-reverse:
> +	Usage: optional
> +	Value type: <boolean>
> +	Definition: denotes that the ramp generator should operate backwards
> +		    over the pattern

I'd not do this. While we _may_ want to specify default trigger and
default blinking behaviour, we should do it in a way that is common
for all the drivers.

									Pavel
-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 181 bytes --]

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

[Pavel Machek]

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

On Wed 2017-03-29 12:07:25, Bjorn Andersson wrote:
> On Tue 28 Mar 19:17 PDT 2017, Rob Herring wrote:
> 
> > On Thu, Mar 23, 2017 at 09:37:49PM +0100, Pavel Machek wrote:
> > > Hi!
> > > 
> > > > 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.
> > > 
> > > Ok, this is not first hardware that supports something like this. We
> > > have similar hardware that can do blinking on Nokia N900 -- please
> > > take a look at leds-lp55*.c
> > 
> > And perhaps some alignment on the bindings too if the N900 has bindings.
> > 
> 
> There is a binding for ti,lp55xx, but there's nothing I can reuse from
> that binding...because it's completely different hardware.

Agreed, if you drop the pattern stuff from the binding, at least for now. 

> > > And it would be really good to provide hardware abstraction. We really
> > > don't want to have different userspace for LPG and for N900 and for
> > 
> > I'm interested in what this looks like as several AOSP platforms do 
> > tri-color LEDs with custom sysfs extensions.
> 
> How to model RGB LEDs has been discussed many times before and I was
> hoping for that discussion to come to some conclusion during the last 2
> years, but now I couldn't wait more - we need this driver for
> db820c.

If you want driver merged quickly, I believe the best way would be to
leave out pattern support for now. We can merge the basic driver
easily to 4.12.

> With this driver, as with many existing, you will have 3 LEDs that you
> set independently.
> 
> I did implement blinking by using the PWM straight off, so you can't set
> brightness or synchronize the multiple channels. Perhaps this should be
> changed to use the ramp generator.
> 
> To synchronize patterns I suggest that we extend the LUT binding to
> describe groups and when any LPG trigger a restart of the pattern-walker
> we trigger all that are grouped.
> 
> These two changes combined allows you to set brightness and blink with a
> RGB-LED.
> 
> 
> But I will have to dig up some hardware that uses the LPG for driving a
> RGB-LED to be able to test this (and I do prefer that to be done with
> some incremental patches at some later time, if acceptable).

Incremental patches sound like a good idea, yes.

I'd say that testing with actual RGB LED is not a requirement... as
long as we design reasonable interface where the synchronizaction will
be easy.

Thanks and best regards,

									Pavel
-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 181 bytes --]

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

[Bjorn Andersson]

On Wed 29 Mar 15:23 PDT 2017, Pavel Machek wrote:

> On Wed 2017-03-29 12:07:25, Bjorn Andersson wrote:
> > On Tue 28 Mar 19:17 PDT 2017, Rob Herring wrote:
> > 
> > > On Thu, Mar 23, 2017 at 09:37:49PM +0100, Pavel Machek wrote:
> > > > Hi!
> > > > 
> > > > > 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.
> > > > 
> > > > Ok, this is not first hardware that supports something like this. We
> > > > have similar hardware that can do blinking on Nokia N900 -- please
> > > > take a look at leds-lp55*.c
> > > 
> > > And perhaps some alignment on the bindings too if the N900 has bindings.
> > > 
> > 
> > There is a binding for ti,lp55xx, but there's nothing I can reuse from
> > that binding...because it's completely different hardware.
> 
> Agreed, if you drop the pattern stuff from the binding, at least for now. 
> 

I do not have a strong preference to expose these knobs in devicetree
and I do fear that finding some common "pattern" bindings that suits
everyone will be very difficult.

So I'll drop them from the binding for now.

> > > > And it would be really good to provide hardware abstraction. We really
> > > > don't want to have different userspace for LPG and for N900 and for
> > > 
> > > I'm interested in what this looks like as several AOSP platforms do 
> > > tri-color LEDs with custom sysfs extensions.
> > 
> > How to model RGB LEDs has been discussed many times before and I was
> > hoping for that discussion to come to some conclusion during the last 2
> > years, but now I couldn't wait more - we need this driver for
> > db820c.
> 
> If you want driver merged quickly, I believe the best way would be to
> leave out pattern support for now. We can merge the basic driver
> easily to 4.12.
> 

I'm not that much in a hurry and would rather see that we resolve any
outstanding issues with the implementation of the pattern handling.


But regardless of this we still have the problem that the typical
Qualcomm PMIC has 8 LPG-blocks and any triple could be driving a
RGB-LED. So we would have to create some sort of in-driver-wrapper
around any three instances exposing them as a single LED to the user.

I rather expose the individual channels and make sure that when we
trigger a blink operation or enable a pattern (i.e. the two operations
that do require synchronization) we will perform that synchronization
under the hood.

> > With this driver, as with many existing, you will have 3 LEDs that you
> > set independently.
> > 
> > I did implement blinking by using the PWM straight off, so you can't set
> > brightness or synchronize the multiple channels. Perhaps this should be
> > changed to use the ramp generator.
> > 
> > To synchronize patterns I suggest that we extend the LUT binding to
> > describe groups and when any LPG trigger a restart of the pattern-walker
> > we trigger all that are grouped.
> > 
> > These two changes combined allows you to set brightness and blink with a
> > RGB-LED.
> > 
> > 
> > But I will have to dig up some hardware that uses the LPG for driving a
> > RGB-LED to be able to test this (and I do prefer that to be done with
> > some incremental patches at some later time, if acceptable).
> 
> Incremental patches sound like a good idea, yes.
> 
> I'd say that testing with actual RGB LED is not a requirement... as
> long as we design reasonable interface where the synchronizaction will
> be easy.
> 

As this relates to the board layout (which LPG-channels are hooked to a
RGB) I think it makes sense to expose a mechanism in devicetree to
indicate which channels should have their pattern/blink synchronized.

We should be able to extend the LUT (the hardware that actually
implements the pattern-walker logic) with a DT-property like:

  qcom,synchronize-group-0 = <1, 2, 3>;
  qcom,synchronize-group-1 = <5, 6, 7>;

And whenever we configure a pattern involving one of the affected LEDs
from a group we start all of them.

I'll implement this in a separate patch and include in version 2 as
well.

Regards,
Bjorn

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

[Pavel Machek]

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

Hi!

> > > There is a binding for ti,lp55xx, but there's nothing I can reuse from
> > > that binding...because it's completely different hardware.
> > 
> > Agreed, if you drop the pattern stuff from the binding, at least for now. 
> 
> I do not have a strong preference to expose these knobs in devicetree
> and I do fear that finding some common "pattern" bindings that suits
> everyone will be very difficult.
> 
> So I'll drop them from the binding for now.

Ok.

> > If you want driver merged quickly, I believe the best way would be to
> > leave out pattern support for now. We can merge the basic driver
> > easily to 4.12.
> > 
> 
> I'm not that much in a hurry and would rather see that we resolve any
> outstanding issues with the implementation of the pattern handling.

Ok, good.

> But regardless of this we still have the problem that the typical
> Qualcomm PMIC has 8 LPG-blocks and any triple could be driving a
> RGB-LED. So we would have to create some sort of in-driver-wrapper
> around any three instances exposing them as a single LED to the user.

Yes, I believe we should do the wrapping. In N900 case, 

> I rather expose the individual channels and make sure that when we
> trigger a blink operation or enable a pattern (i.e. the two operations
> that do require synchronization) we will perform that synchronization
> under the hood.

First, we need a way to tell userspace which LEDs are synchronized,
because otherwise it will be confusing.

Second, there are more issues than just patterns with the RGB
LED. Most important is ability to set particular colors. You want to
set the RGB LED to "white", but that does not mean you can set
red=green=blue=1.0. You want color to look the same on LCD and on the
LED, which means coefficients for white and some kind of function for
brightness-to-PWM conversion.

> > Incremental patches sound like a good idea, yes.
> > 
> > I'd say that testing with actual RGB LED is not a requirement... as
> > long as we design reasonable interface where the synchronizaction will
> > be easy.
> > 
> 
> As this relates to the board layout (which LPG-channels are hooked to a
> RGB) I think it makes sense to expose a mechanism in devicetree to
> indicate which channels should have their pattern/blink synchronized.
> 
> We should be able to extend the LUT (the hardware that actually
> implements the pattern-walker logic) with a DT-property like:
> 
>   qcom,synchronize-group-0 = <1, 2, 3>;
>   qcom,synchronize-group-1 = <5, 6, 7>;
> 
> And whenever we configure a pattern involving one of the affected LEDs
> from a group we start all of them.

Yes we need some kind of grouping.

Additional complexity in the N900 case... groups can actually be
configured at run time. Original Maemo used that ability to group 6
keyboard backlight leds, and then run pattern on them. OTOH... I don't
think we _need_ to support that functionality.

Best regards,
									Pavel

-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 181 bytes --]

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

[Jacek Anaszewski]

Hi Bjorn and Pavel,

On 03/30/2017 09:43 AM, Pavel Machek wrote:
> Hi!
> 
>>>> There is a binding for ti,lp55xx, but there's nothing I can reuse from
>>>> that binding...because it's completely different hardware.
>>>
>>> Agreed, if you drop the pattern stuff from the binding, at least for now. 
>>
>> I do not have a strong preference to expose these knobs in devicetree
>> and I do fear that finding some common "pattern" bindings that suits
>> everyone will be very difficult.
>>
>> So I'll drop them from the binding for now.
> 
> Ok.
> 
>>> If you want driver merged quickly, I believe the best way would be to
>>> leave out pattern support for now. We can merge the basic driver
>>> easily to 4.12.
>>>
>>
>> I'm not that much in a hurry and would rather see that we resolve any
>> outstanding issues with the implementation of the pattern handling.
> 
> Ok, good.
> 
>> But regardless of this we still have the problem that the typical
>> Qualcomm PMIC has 8 LPG-blocks and any triple could be driving a
>> RGB-LED. So we would have to create some sort of in-driver-wrapper
>> around any three instances exposing them as a single LED to the user.
> 
> Yes, I believe we should do the wrapping. In N900 case, 
> 
>> I rather expose the individual channels and make sure that when we
>> trigger a blink operation or enable a pattern (i.e. the two operations
>> that do require synchronization) we will perform that synchronization
>> under the hood.
> 
> First, we need a way to tell userspace which LEDs are synchronized,
> because otherwise it will be confusing.

There is one year old discussion [0] about the possible approaches
to RGB sub-LEDs synchronization problem and patterns in general.
My last message with API design proposal has been left unanswered.

Probably we continue that discussion here.

Generally Bjorn's drivers touch two yet to be addressed issues:
- RGB LED support
- Generic support for patterns

It is likely that both issues can be solved by utilizing trigger
mechanism. The possible solution to the problem Bjorn tried to
address with /sys/class/leds/<led>/pattern comma separated list
could be a trigger with adjustable number of pattern intervals.

The trigger once activated would create a directory with the
number of files corresponding to the number of requested intervals,
and then user could write an interval value by writing it to the
corresponding file. Somehow related approach has been implemented
for USB port LED trigger:

0f247626cbbf ('usb: core: Introduce a USB port LED trigger")

In both RGB and pattern approaches we should assess
if it is acceptable to provide a pattern for trigger name,
e.g. blink-pattern-{num_intervals}.

If so, then "echo transition-pattern-15" would create a directory
e.g. transition_intervals with files interval_0 to interval_14,
that could be adjusted by userspace.

> Second, there are more issues than just patterns with the RGB
> LED. Most important is ability to set particular colors. You want to
> set the RGB LED to "white", but that does not mean you can set
> red=green=blue=1.0. You want color to look the same on LCD and on the
> LED, which means coefficients for white and some kind of function for
> brightness-to-PWM conversion.

Shouldn't we leave that entirely to the userspace? Can we come up
with coefficients that will guarantee the same result on all existing
LCD devices?

>>> Incremental patches sound like a good idea, yes.
>>>
>>> I'd say that testing with actual RGB LED is not a requirement... as
>>> long as we design reasonable interface where the synchronizaction will
>>> be easy.
>>>
>>
>> As this relates to the board layout (which LPG-channels are hooked to a
>> RGB) I think it makes sense to expose a mechanism in devicetree to
>> indicate which channels should have their pattern/blink synchronized.
>>
>> We should be able to extend the LUT (the hardware that actually
>> implements the pattern-walker logic) with a DT-property like:
>>
>>   qcom,synchronize-group-0 = <1, 2, 3>;
>>   qcom,synchronize-group-1 = <5, 6, 7>;
>>
>> And whenever we configure a pattern involving one of the affected LEDs
>> from a group we start all of them.
> 
> Yes we need some kind of grouping.
> 
> Additional complexity in the N900 case... groups can actually be
> configured at run time. Original Maemo used that ability to group 6
> keyboard backlight leds, and then run pattern on them. OTOH... I don't
> think we _need_ to support that functionality.
> 
> Best regards,
> 									Pavel
> 

[0] https://lkml.org/lkml/2016/4/18/179

-- 
Best regards,
Jacek Anaszewski

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

[Jacek Anaszewski]

On 03/31/2017 11:28 AM, Jacek Anaszewski wrote:
> Hi Bjorn and Pavel,
> 
> On 03/30/2017 09:43 AM, Pavel Machek wrote:
>> Hi!
>>
>>>>> There is a binding for ti,lp55xx, but there's nothing I can reuse from
>>>>> that binding...because it's completely different hardware.
>>>>
>>>> Agreed, if you drop the pattern stuff from the binding, at least for now. 
>>>
>>> I do not have a strong preference to expose these knobs in devicetree
>>> and I do fear that finding some common "pattern" bindings that suits
>>> everyone will be very difficult.
>>>
>>> So I'll drop them from the binding for now.
>>
>> Ok.
>>
>>>> If you want driver merged quickly, I believe the best way would be to
>>>> leave out pattern support for now. We can merge the basic driver
>>>> easily to 4.12.
>>>>
>>>
>>> I'm not that much in a hurry and would rather see that we resolve any
>>> outstanding issues with the implementation of the pattern handling.
>>
>> Ok, good.
>>
>>> But regardless of this we still have the problem that the typical
>>> Qualcomm PMIC has 8 LPG-blocks and any triple could be driving a
>>> RGB-LED. So we would have to create some sort of in-driver-wrapper
>>> around any three instances exposing them as a single LED to the user.
>>
>> Yes, I believe we should do the wrapping. In N900 case, 
>>
>>> I rather expose the individual channels and make sure that when we
>>> trigger a blink operation or enable a pattern (i.e. the two operations
>>> that do require synchronization) we will perform that synchronization
>>> under the hood.
>>
>> First, we need a way to tell userspace which LEDs are synchronized,
>> because otherwise it will be confusing.
> 
> There is one year old discussion [0] about the possible approaches
> to RGB sub-LEDs synchronization problem and patterns in general.
> My last message with API design proposal has been left unanswered.
> 
> Probably we continue that discussion here.
> 
> Generally Bjorn's drivers touch two yet to be addressed issues:
> - RGB LED support
> - Generic support for patterns
> 
> It is likely that both issues can be solved by utilizing trigger
> mechanism. The possible solution to the problem Bjorn tried to
> address with /sys/class/leds/<led>/pattern comma separated list
> could be a trigger with adjustable number of pattern intervals.
> 
> The trigger once activated would create a directory with the
> number of files corresponding to the number of requested intervals,
> and then user could write an interval value by writing it to the
> corresponding file. Somehow related approach has been implemented
> for USB port LED trigger:
> 
> 0f247626cbbf ('usb: core: Introduce a USB port LED trigger")
> 
> In both RGB and pattern approaches we should assess
> if it is acceptable to provide a pattern for trigger name,
> e.g. blink-pattern-{num_intervals}.

Actually we could achieve the goal by listing all available pattern
configurations for given LED class device, so in case of Qualcomm LPG
driver we could have transition-pattern-1 to transition-pattern-15
listed after executing "cat trigger".

In case of RGB trigger we would have qcom-rgb among other triggers
listed in a result of executing "cat trigger". Then qcom-rgb-1 LED class
device could appear in /sys/class/leds, which would expose files
red-led-name, green-led-name and blue-led-name. Once all files are
initialized with appropriate LED class device names the RGB brightness
could be updated synchronously in all involved LEDs by executing
e.g. "echo 1 > update_color". Of course all LEDs that set qcom-rgb
trigger would have to avoid changing device state on write to
their brightness file.

I wonder if I'm not missing some vital constraints here that could
make this design unfeasible.

Best regards,
Jacek Anaszewski

> If so, then "echo transition-pattern-15" would create a directory
> e.g. transition_intervals with files interval_0 to interval_14,
> that could be adjusted by userspace.
> 
>> Second, there are more issues than just patterns with the RGB
>> LED. Most important is ability to set particular colors. You want to
>> set the RGB LED to "white", but that does not mean you can set
>> red=green=blue=1.0. You want color to look the same on LCD and on the
>> LED, which means coefficients for white and some kind of function for
>> brightness-to-PWM conversion.
> 
> Shouldn't we leave that entirely to the userspace? Can we come up
> with coefficients that will guarantee the same result on all existing
> LCD devices?
> 
>>>> Incremental patches sound like a good idea, yes.
>>>>
>>>> I'd say that testing with actual RGB LED is not a requirement... as
>>>> long as we design reasonable interface where the synchronizaction will
>>>> be easy.
>>>>
>>>
>>> As this relates to the board layout (which LPG-channels are hooked to a
>>> RGB) I think it makes sense to expose a mechanism in devicetree to
>>> indicate which channels should have their pattern/blink synchronized.
>>>
>>> We should be able to extend the LUT (the hardware that actually
>>> implements the pattern-walker logic) with a DT-property like:
>>>
>>>   qcom,synchronize-group-0 = <1, 2, 3>;
>>>   qcom,synchronize-group-1 = <5, 6, 7>;
>>>
>>> And whenever we configure a pattern involving one of the affected LEDs
>>> from a group we start all of them.
>>
>> Yes we need some kind of grouping.
>>
>> Additional complexity in the N900 case... groups can actually be
>> configured at run time. Original Maemo used that ability to group 6
>> keyboard backlight leds, and then run pattern on them. OTOH... I don't
>> think we _need_ to support that functionality.
>>
>> Best regards,
>> 									Pavel
>>
> 
> [0] https://lkml.org/lkml/2016/4/18/179
> 

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

[Bjorn Andersson]

On Sun 02 Apr 05:54 PDT 2017, Jacek Anaszewski wrote:

> On 03/31/2017 11:28 AM, Jacek Anaszewski wrote:
> > Hi Bjorn and Pavel,
> > 
> > On 03/30/2017 09:43 AM, Pavel Machek wrote:
[..]
> > In both RGB and pattern approaches we should assess
> > if it is acceptable to provide a pattern for trigger name,
> > e.g. blink-pattern-{num_intervals}.
> 
> Actually we could achieve the goal by listing all available pattern
> configurations for given LED class device, so in case of Qualcomm LPG
> driver we could have transition-pattern-1 to transition-pattern-15
> listed after executing "cat trigger".
> 

There's a common pattern-table of 24 (or 64) entries, that is shared
among the 8 LPGs (each LPG simply has to indices pointing into the
shared table). Each entry in the table holds a value between 0 and 511.
So that's a lot of "available pattern configurations".

Unless you go with the path Qualcomm did in their downstream driver,
where the table is filled statically from DeviceTree and each LPG is
statically configured with some range from the table. But I don't like
this and as far as I can tell neither do you guys.

And lastly the request is to create a common interface for userspace to
control patterns among different LED hardware and I do not see how this
would be acceptable to the LP55xx users.


Perhaps I'm not getting what you're proposing?

> In case of RGB trigger we would have qcom-rgb among other triggers
> listed in a result of executing "cat trigger". Then qcom-rgb-1 LED class
> device could appear in /sys/class/leds, which would expose files
> red-led-name, green-led-name and blue-led-name. Once all files are
> initialized with appropriate LED class device names the RGB brightness
> could be updated synchronously in all involved LEDs by executing
> e.g. "echo 1 > update_color". Of course all LEDs that set qcom-rgb
> trigger would have to avoid changing device state on write to
> their brightness file.
> 

I don't see that the brightness of the individual LEDs is the problem,
writing individual colors to the 3 LEDs in a serial fashion isn't
user-noticeable. What is a problem is if you start a pulse of some
combined color it's important to synchronize the starting of the pattern
generator, of you will have a color that shifts over time - or in the
case of blink where the colors get out of sync.

And as I said, I suggest that we just make it possible to configure any
LPG channel to be grouped with any others and within a group we always
synchronize the pattern-generator when enabling any LED.


The issue left open is that we expose 3 independent LEDs to userspace
and it seems desired to expose it as a single "RGB" LED. Providing a
"RGB trigger" that any LED in the system can be associated with and then
have that trigger wrap the individual LEDs sounds like a reasonable path
forward. But if we're not going to do things like color "calibration" it
feels like we're replacing the 3 writes in userspace with a single write
and then 3 calls in the kernel; i.e. the only win in my view is some
conceptual benefit.

> I wonder if I'm not missing some vital constraints here that could
> make this design unfeasible.
> 

Regardless of how we expose RGBs to userspace, the 8 LPG hardware blocks
are independent of each other. The fact that they end up controlling
something that is perceived by the human eye as some mixed color is to
me a matter of system integration, and as such should not convolute the
implementation of the individual instances.

Regards,
Bjorn

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

[Bjorn Andersson]

On Fri 31 Mar 02:28 PDT 2017, Jacek Anaszewski wrote:

> Hi Bjorn and Pavel,
> 
> On 03/30/2017 09:43 AM, Pavel Machek wrote:
> > Hi!
> > 
> >>>> There is a binding for ti,lp55xx, but there's nothing I can reuse from
> >>>> that binding...because it's completely different hardware.
> >>>
> >>> Agreed, if you drop the pattern stuff from the binding, at least for now. 
> >>
> >> I do not have a strong preference to expose these knobs in devicetree
> >> and I do fear that finding some common "pattern" bindings that suits
> >> everyone will be very difficult.
> >>
> >> So I'll drop them from the binding for now.
> > 
> > Ok.
> > 
> >>> If you want driver merged quickly, I believe the best way would be to
> >>> leave out pattern support for now. We can merge the basic driver
> >>> easily to 4.12.
> >>>
> >>
> >> I'm not that much in a hurry and would rather see that we resolve any
> >> outstanding issues with the implementation of the pattern handling.
> > 
> > Ok, good.
> > 
> >> But regardless of this we still have the problem that the typical
> >> Qualcomm PMIC has 8 LPG-blocks and any triple could be driving a
> >> RGB-LED. So we would have to create some sort of in-driver-wrapper
> >> around any three instances exposing them as a single LED to the user.
> > 
> > Yes, I believe we should do the wrapping. In N900 case, 
> > 
> >> I rather expose the individual channels and make sure that when we
> >> trigger a blink operation or enable a pattern (i.e. the two operations
> >> that do require synchronization) we will perform that synchronization
> >> under the hood.
> > 
> > First, we need a way to tell userspace which LEDs are synchronized,
> > because otherwise it will be confusing.
> 
> There is one year old discussion [0] about the possible approaches
> to RGB sub-LEDs synchronization problem and patterns in general.
> My last message with API design proposal has been left unanswered.
> 
> Probably we continue that discussion here.
> 
> Generally Bjorn's drivers touch two yet to be addressed issues:
> - RGB LED support
> - Generic support for patterns
> 
> It is likely that both issues can be solved by utilizing trigger
> mechanism. The possible solution to the problem Bjorn tried to
> address with /sys/class/leds/<led>/pattern comma separated list
> could be a trigger with adjustable number of pattern intervals.
> 
> The trigger once activated would create a directory with the
> number of files corresponding to the number of requested intervals,
> and then user could write an interval value by writing it to the
> corresponding file. Somehow related approach has been implemented
> for USB port LED trigger:
> 
> 0f247626cbbf ('usb: core: Introduce a USB port LED trigger")
> 
> In both RGB and pattern approaches we should assess
> if it is acceptable to provide a pattern for trigger name,
> e.g. blink-pattern-{num_intervals}.
> 
> If so, then "echo transition-pattern-15" would create a directory
> e.g. transition_intervals with files interval_0 to interval_14,
> that could be adjusted by userspace.
> 

Having a RGB-trigger that proxy a accepts a userspace request of a
brightness-tripple and sets the brightness on the individual associated
LEDs sounds reasonable - but should probably be generalized to any
number of LEDs.

A slightly related matter is the question on how to use a single LED for
multiple trigger sources, e.g. how do I get a single LED to show
activity of two MMCs?.


For the patterns I don't know how a trigger for this would look like,
how would setting the pattern of a trigger be propagated down to the
hardware?

> > Second, there are more issues than just patterns with the RGB
> > LED. Most important is ability to set particular colors. You want to
> > set the RGB LED to "white", but that does not mean you can set
> > red=green=blue=1.0. You want color to look the same on LCD and on the
> > LED, which means coefficients for white and some kind of function for
> > brightness-to-PWM conversion.
> 
> Shouldn't we leave that entirely to the userspace? Can we come up
> with coefficients that will guarantee the same result on all existing
> LCD devices?
> 

How about we just force user space perform the 3 writes and save us the
cost of another trigger in that case? Configuring the brightness of 3
LEDs is not board specific - and even with a RGB-interface we still need
to specify which RGB-LED should be controlled.

Regards,
Bjorn

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

[Jacek Anaszewski]

On 04/03/2017 08:21 PM, Bjorn Andersson wrote:
> On Sun 02 Apr 05:54 PDT 2017, Jacek Anaszewski wrote:
> 
>> On 03/31/2017 11:28 AM, Jacek Anaszewski wrote:
>>> Hi Bjorn and Pavel,
>>>
>>> On 03/30/2017 09:43 AM, Pavel Machek wrote:
> [..]
>>> In both RGB and pattern approaches we should assess
>>> if it is acceptable to provide a pattern for trigger name,
>>> e.g. blink-pattern-{num_intervals}.
>>
>> Actually we could achieve the goal by listing all available pattern
>> configurations for given LED class device, so in case of Qualcomm LPG
>> driver we could have transition-pattern-1 to transition-pattern-15
>> listed after executing "cat trigger".
>>
> 
> There's a common pattern-table of 24 (or 64) entries, that is shared
> among the 8 LPGs (each LPG simply has to indices pointing into the
> shared table). Each entry in the table holds a value between 0 and 511.
> So that's a lot of "available pattern configurations".

By "available pattern configurations" I meant the number of possible
"pattern resolution" options, E.g. an equivalent of

echo "40 71 12" > pattern

would be

echo transition-pattern-3 > trigger

and then pattern_intervals directory with three files would
be created: interval_1, interval_2, interval_3.

In the next steps the user would have to write 40, 71 and 12
to interval_(1, 2, 3) respectively.

Now it is clear that initialization of such a trigger would
be cumbersome. One file accepting space separated list of values
should be fine.

> Unless you go with the path Qualcomm did in their downstream driver,
> where the table is filled statically from DeviceTree and each LPG is
> statically configured with some range from the table. But I don't like
> this and as far as I can tell neither do you guys.

Right, what we're trying to implement is flexible sysfs interface.

> And lastly the request is to create a common interface for userspace to
> control patterns among different LED hardware and I do not see how this
> would be acceptable to the LP55xx users.

For that we will certainly need some additions to the LED Trigger core.
We will need to limit usage of certain type of triggers only among
specified LED class devices, to allow assigning them to pattern engines.

We will also need a new op, similarly to existing blink_set().

The question is whether we will be providing software fallbacks,
and to what extent.

> Perhaps I'm not getting what you're proposing?

Yeah, after going through Documentation/leds/leds-lp55xx.txt and friends
it looks like we will need really sophisticated mechanism.
I'm not sure if providing generic interface for all use cases
documented there makes sense.

>> In case of RGB trigger we would have qcom-rgb among other triggers
>> listed in a result of executing "cat trigger". Then qcom-rgb-1 LED class
>> device could appear in /sys/class/leds, which would expose files
>> red-led-name, green-led-name and blue-led-name. Once all files are
>> initialized with appropriate LED class device names the RGB brightness
>> could be updated synchronously in all involved LEDs by executing
>> e.g. "echo 1 > update_color". Of course all LEDs that set qcom-rgb
>> trigger would have to avoid changing device state on write to
>> their brightness file.
>>
> 
> I don't see that the brightness of the individual LEDs is the problem,
> writing individual colors to the 3 LEDs in a serial fashion isn't
> user-noticeable. What is a problem is if you start a pulse of some
> combined color it's important to synchronize the starting of the pattern
> generator, of you will have a color that shifts over time - or in the
> case of blink where the colors get out of sync.

I had exactly these use cases on mind.

> And as I said, I suggest that we just make it possible to configure any
> LPG channel to be grouped with any others and within a group we always
> synchronize the pattern-generator when enabling any LED.

IMHO it would be useful to have also specialized RGB trigger. Then we
could call led_rgb_event() from any place in kernel and be sure that
all groups of three R,G,B LED class devices, registered on that trigger,
will get new brightness.

We have two concepts here: trigger and grouping LED class device into
a single one. In the latter case such a device could be registered on
any trigger from current mainline and all grouped LEDs would get the
same brightness (e.g. as a result of backlight event).

> The issue left open is that we expose 3 independent LEDs to userspace
> and it seems desired to expose it as a single "RGB" LED. Providing a
> "RGB trigger" that any LED in the system can be associated with and then
> have that trigger wrap the individual LEDs sounds like a reasonable path
> forward. But if we're not going to do things like color "calibration" it
> feels like we're replacing the 3 writes in userspace with a single write
> and then 3 calls in the kernel; i.e. the only win in my view is some
> conceptual benefit.

You have one syscall vs three syscalls. The synchronization is the main
target here.

>> I wonder if I'm not missing some vital constraints here that could
>> make this design unfeasible.
>>
> 
> Regardless of how we expose RGBs to userspace, the 8 LPG hardware blocks
> are independent of each other. The fact that they end up controlling
> something that is perceived by the human eye as some mixed color is to
> me a matter of system integration, and as such should not convolute the
> implementation of the individual instances.

-- 
Best regards,
Jacek Anaszewski

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

[Jacek Anaszewski]

On 04/03/2017 09:00 PM, Bjorn Andersson wrote:
> On Fri 31 Mar 02:28 PDT 2017, Jacek Anaszewski wrote:
> 
>> Hi Bjorn and Pavel,
>>
>> On 03/30/2017 09:43 AM, Pavel Machek wrote:
>>> Hi!
>>>
>>>>>> There is a binding for ti,lp55xx, but there's nothing I can reuse from
>>>>>> that binding...because it's completely different hardware.
>>>>>
>>>>> Agreed, if you drop the pattern stuff from the binding, at least for now. 
>>>>
>>>> I do not have a strong preference to expose these knobs in devicetree
>>>> and I do fear that finding some common "pattern" bindings that suits
>>>> everyone will be very difficult.
>>>>
>>>> So I'll drop them from the binding for now.
>>>
>>> Ok.
>>>
>>>>> If you want driver merged quickly, I believe the best way would be to
>>>>> leave out pattern support for now. We can merge the basic driver
>>>>> easily to 4.12.
>>>>>
>>>>
>>>> I'm not that much in a hurry and would rather see that we resolve any
>>>> outstanding issues with the implementation of the pattern handling.
>>>
>>> Ok, good.
>>>
>>>> But regardless of this we still have the problem that the typical
>>>> Qualcomm PMIC has 8 LPG-blocks and any triple could be driving a
>>>> RGB-LED. So we would have to create some sort of in-driver-wrapper
>>>> around any three instances exposing them as a single LED to the user.
>>>
>>> Yes, I believe we should do the wrapping. In N900 case, 
>>>
>>>> I rather expose the individual channels and make sure that when we
>>>> trigger a blink operation or enable a pattern (i.e. the two operations
>>>> that do require synchronization) we will perform that synchronization
>>>> under the hood.
>>>
>>> First, we need a way to tell userspace which LEDs are synchronized,
>>> because otherwise it will be confusing.
>>
>> There is one year old discussion [0] about the possible approaches
>> to RGB sub-LEDs synchronization problem and patterns in general.
>> My last message with API design proposal has been left unanswered.
>>
>> Probably we continue that discussion here.
>>
>> Generally Bjorn's drivers touch two yet to be addressed issues:
>> - RGB LED support
>> - Generic support for patterns
>>
>> It is likely that both issues can be solved by utilizing trigger
>> mechanism. The possible solution to the problem Bjorn tried to
>> address with /sys/class/leds/<led>/pattern comma separated list
>> could be a trigger with adjustable number of pattern intervals.
>>
>> The trigger once activated would create a directory with the
>> number of files corresponding to the number of requested intervals,
>> and then user could write an interval value by writing it to the
>> corresponding file. Somehow related approach has been implemented
>> for USB port LED trigger:
>>
>> 0f247626cbbf ('usb: core: Introduce a USB port LED trigger")
>>
>> In both RGB and pattern approaches we should assess
>> if it is acceptable to provide a pattern for trigger name,
>> e.g. blink-pattern-{num_intervals}.
>>
>> If so, then "echo transition-pattern-15" would create a directory
>> e.g. transition_intervals with files interval_0 to interval_14,
>> that could be adjusted by userspace.
>>
> 
> Having a RGB-trigger that proxy a accepts a userspace request of a
> brightness-tripple and sets the brightness on the individual associated
> LEDs sounds reasonable - but should probably be generalized to any
> number of LEDs.
> 
> A slightly related matter is the question on how to use a single LED for
> multiple trigger sources, e.g. how do I get a single LED to show
> activity of two MMCs?.

You would have to add a dedicated trigger, similar to usb port trigger,
I mentioned in the previous message.

> 
> For the patterns I don't know how a trigger for this would look like,
> how would setting the pattern of a trigger be propagated down to the
> hardware?

We'd need a new op and API similar to blink_set()/led_blink_set().

>>> Second, there are more issues than just patterns with the RGB
>>> LED. Most important is ability to set particular colors. You want to
>>> set the RGB LED to "white", but that does not mean you can set
>>> red=green=blue=1.0. You want color to look the same on LCD and on the
>>> LED, which means coefficients for white and some kind of function for
>>> brightness-to-PWM conversion.
>>
>> Shouldn't we leave that entirely to the userspace? Can we come up
>> with coefficients that will guarantee the same result on all existing
>> LCD devices?
>>
> 
> How about we just force user space perform the 3 writes and save us the
> cost of another trigger in that case? Configuring the brightness of 3
> LEDs is not board specific - and even with a RGB-interface we still need
> to specify which RGB-LED should be controlled.

This is what we have now, so we can live with it. Addition of a new
RGB trigger would be an improvement of the existing state.

-- 
Best regards,
Jacek Anaszewski

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

[Pavel Machek]

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

Hi!

> > Second, there are more issues than just patterns with the RGB
> > LED. Most important is ability to set particular colors. You want to
> > set the RGB LED to "white", but that does not mean you can set
> > red=green=blue=1.0. You want color to look the same on LCD and on the
> > LED, which means coefficients for white and some kind of function for
> > brightness-to-PWM conversion.
> 
> Shouldn't we leave that entirely to the userspace? Can we come up
> with coefficients that will guarantee the same result on all existing
> LCD devices?

I don't think we should. We want (red = 70%, green = 80%, blue = 20%)
to look approximately the same on all the hardware. That's currently
not the case; even (red = green = blue = 100%) is not white.

I believe easiest solution is "the kernel does the work", as it does
for LCD screens.

[Now... as long as userspace has enough information to display white
and specific colors, I don't care much -- having kernel present
coefficients for userspace would work, too.]

								Pavel

-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 181 bytes --]

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

[Pavel Machek]

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

Hi!

> > In both RGB and pattern approaches we should assess
> > if it is acceptable to provide a pattern for trigger name,
> > e.g. blink-pattern-{num_intervals}.
> > 
> > If so, then "echo transition-pattern-15" would create a directory
> > e.g. transition_intervals with files interval_0 to interval_14,
> > that could be adjusted by userspace.
> 
> Having a RGB-trigger that proxy a accepts a userspace request of a
> brightness-tripple and sets the brightness on the individual associated
> LEDs sounds reasonable - but should probably be generalized to any
> number of LEDs.

Well..  Generalizing for any number of leds would be nice -- because
hardware can do that. OTOH, if we do that, we'll not have a place
where to do "white-adjustment".

> A slightly related matter is the question on how to use a single LED for
> multiple trigger sources, e.g. how do I get a single LED to show
> activity of two MMCs?.

We normally don't do that. We'd either have a trigger for a single
MMC, or trigger of all the MMCs..

> For the patterns I don't know how a trigger for this would look like,
> how would setting the pattern of a trigger be propagated down to the
> hardware?

Well... I'm not sure if we _want_ to do triggers for
patterns. LED triggers change rather quickly (100 times a second?) so
doing them in kernel makes sense. Patterns take 10s of seconds, so we
do not need to handle them in kernel. 

Thanks,
									Pavel
-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 181 bytes --]

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

[Bjorn Andersson]

On Mon 03 Apr 13:38 PDT 2017, Jacek Anaszewski wrote:

> On 04/03/2017 09:00 PM, Bjorn Andersson wrote:
[..]
> > For the patterns I don't know how a trigger for this would look like,
> > how would setting the pattern of a trigger be propagated down to the
> > hardware?
> 
> We'd need a new op and API similar to blink_set()/led_blink_set().
> 

I've tried to find different LED circuits with some sort of pattern
generator in an attempt to figure out how to design this interface, but
turned out to be quite hard to find examples; the three I can compare
are:

* LP5xx series "implements" pattern generation by executing code.

* Qualcomm LPG iterates over 2-64 brightness-values in a pattern, at a
  fixed rate with knobs to configure what happens before starting and
  after finishing iterating over the defined values. It does not support
  smooth transitions between values.

* AS3676 supports a pattern of 32 values controlling if the output
  should be enabled or disabled for each 32.5ms (or 250ms) time period.
  The delay before repeating the pattern can be configured. It support
  smooth transitions between the states.


So, while I think I see how you would like to architect this interface I
am not sure how to figure out the details.

The pattern definition would have to be expressive enough to support the
features of LP5xx and direct enough to support the limited AS3676. It
would likely have to express transitions, so that the LPG could generate
intermediate steps (and we will have to adapt the resolution of the
ramps based on the other LPGs in the system).

How do we do with patterns that are implementable by the LP5xx but are
not with the LPG? Should we reject those or should we do some sort of
best-effort approach in the kernel?

> >>> Second, there are more issues than just patterns with the RGB
> >>> LED. Most important is ability to set particular colors. You want to
> >>> set the RGB LED to "white", but that does not mean you can set
> >>> red=green=blue=1.0. You want color to look the same on LCD and on the
> >>> LED, which means coefficients for white and some kind of function for
> >>> brightness-to-PWM conversion.
> >>
> >> Shouldn't we leave that entirely to the userspace? Can we come up
> >> with coefficients that will guarantee the same result on all existing
> >> LCD devices?
> >>
> > 
> > How about we just force user space perform the 3 writes and save us the
> > cost of another trigger in that case? Configuring the brightness of 3
> > LEDs is not board specific - and even with a RGB-interface we still need
> > to specify which RGB-LED should be controlled.
> 
> This is what we have now, so we can live with it. Addition of a new
> RGB trigger would be an improvement of the existing state.
> 

If we do the brightness compensation (for e.g. white balance
adjustments) in a trigger then there's added value.

The part where I see this affects the LPG driver is that the brightness
of the patterns might have to be adjusted accordingly - which probably
would be easier to implement if the kernel just exposed the compensation
values to user space.

Regards,
Bjorn

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

[Bjorn Andersson]

On Fri 07 Apr 06:32 PDT 2017, Pavel Machek wrote:

> > For the patterns I don't know how a trigger for this would look like,
> > how would setting the pattern of a trigger be propagated down to the
> > hardware?
> 
> Well... I'm not sure if we _want_ to do triggers for
> patterns. LED triggers change rather quickly (100 times a second?) so
> doing them in kernel makes sense. Patterns take 10s of seconds, so we
> do not need to handle them in kernel. 
> 

On any current Qualcomm based phone (using the Qualcomm PMIC to drive
the RGB notification LED) the patterns are hard coded in DeviceTree and
the option you have in runtime is to enable/disable the usage of the
configured pattern and a few knobs of how to traverse the configured
pattern.

When you enter e.g. a low-battery scenario you trigger the red LED to
run its low-battery-pattern and you don't touch it until there's a
higher prio notification (e.g. someone connects the charger).

So in the current implementation patterns "never" changes and they are
triggered only every time you get some event/notification.


A benefit of not using triggers for patterns is that I can assign
patterns to triggered events, e.g. I can configure my LEDs to flash &
fade out when some trigger happens.

Regards,
Bjorn

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

[Pavel Machek]

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

On Fri 2017-04-07 13:36:49, Bjorn Andersson wrote:
> On Fri 07 Apr 06:32 PDT 2017, Pavel Machek wrote:
> 
> > > For the patterns I don't know how a trigger for this would look like,
> > > how would setting the pattern of a trigger be propagated down to the
> > > hardware?
> > 
> > Well... I'm not sure if we _want_ to do triggers for
> > patterns. LED triggers change rather quickly (100 times a second?) so
> > doing them in kernel makes sense. Patterns take 10s of seconds, so we
> > do not need to handle them in kernel. 
> > 
> 
> On any current Qualcomm based phone (using the Qualcomm PMIC to drive
> the RGB notification LED) the patterns are hard coded in DeviceTree and
> the option you have in runtime is to enable/disable the usage of the
> configured pattern and a few knobs of how to traverse the configured
> pattern.

Yes... that's easy, but I believe too limiting. Users will want to
configure their own patterns for their own events.

> When you enter e.g. a low-battery scenario you trigger the red LED to
> run its low-battery-pattern and you don't touch it until there's a
> higher prio notification (e.g. someone connects the charger).

Yes, I have something like that, too.

https://gitlab.com/tui/tui/blob/master/ofone/watchdog.py

									Pavel

-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 181 bytes --]

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

[Pavel Machek]

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

Hi!

> > On 04/03/2017 09:00 PM, Bjorn Andersson wrote:
> [..]
> > > For the patterns I don't know how a trigger for this would look like,
> > > how would setting the pattern of a trigger be propagated down to the
> > > hardware?
> > 
> > We'd need a new op and API similar to blink_set()/led_blink_set().
> > 
> 
> I've tried to find different LED circuits with some sort of pattern
> generator in an attempt to figure out how to design this interface, but
> turned out to be quite hard to find examples; the three I can compare
> are:
> 
> * LP5xx series "implements" pattern generation by executing code.

It supports "linear" and "exponential" transitions between
values. Variable number of steps.

> * Qualcomm LPG iterates over 2-64 brightness-values in a pattern, at a
>   fixed rate with knobs to configure what happens before starting and
>   after finishing iterating over the defined values. It does not support
>   smooth transitions between values.
> 
> * AS3676 supports a pattern of 32 values controlling if the output
>   should be enabled or disabled for each 32.5ms (or 250ms) time period.
>   The delay before repeating the pattern can be configured. It support
>   smooth transitions between the states.

Ok, that's "really interesting" one. As far as I can see, the pattern
really should only contain justtwo intensities...

> So, while I think I see how you would like to architect this interface I
> am not sure how to figure out the details.
> 
> The pattern definition would have to be expressive enough to support the
> features of LP5xx and direct enough to support the limited AS3676. It
> would likely have to express transitions, so that the LPG could generate
> intermediate steps (and we will have to adapt the resolution of the
> ramps based on the other LPGs in the system).

That's why I believe it is important to present whole pattern engine
as one unit to the userspace. Userspace should always upload pattern
for _all_ the LEDs at once.

> How do we do with patterns that are implementable by the LP5xx but are
> not with the LPG? Should we reject those or should we do some sort of
> best-effort approach in the kernel?

Up to you, I guess. Both rejecting and best-effort make some sense.

OTOH if pattern is "(off, 0msec), (white, +1000msec), (off,
+0msec)"... you can't really do it "exactly" even on LP5xx, due to
non-trivial conversion between PWM and what user sees....

So on LPG you'd really do "(off, 0msec), (10% white, +100msec), (20%
white, +100msec), ..."

AS3676... I guess after we reject all patterns that have more than 0
and one specific brightness, we can use similar approximation we'd do
on LPG? 

> > This is what we have now, so we can live with it. Addition of a new
> > RGB trigger would be an improvement of the existing state.
> > 
> 
> If we do the brightness compensation (for e.g. white balance
> adjustments) in a trigger then there's added value.
> 
> The part where I see this affects the LPG driver is that the brightness
> of the patterns might have to be adjusted accordingly - which probably
> would be easier to implement if the kernel just exposed the compensation
> values to user space.

Well, compensation needs to happen "during the transitions",
too.
									Pavel
-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 181 bytes --]

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

[Pavel Machek]

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

Hi!

> [..]
> > > For the patterns I don't know how a trigger for this would look like,
> > > how would setting the pattern of a trigger be propagated down to the
> > > hardware?
> > 
> > We'd need a new op and API similar to blink_set()/led_blink_set().
> > 
> 
> I've tried to find different LED circuits with some sort of pattern
> generator in an attempt to figure out how to design this interface, but
> turned out to be quite hard to find examples; the three I can compare
> are:
> 
> * LP5xx series "implements" pattern generation by executing code.
> 
> * Qualcomm LPG iterates over 2-64 brightness-values in a pattern, at a
>   fixed rate with knobs to configure what happens before starting and
>   after finishing iterating over the defined values. It does not support
>   smooth transitions between values.
> 
> * AS3676 supports a pattern of 32 values controlling if the output
>   should be enabled or disabled for each 32.5ms (or 250ms) time period.
>   The delay before repeating the pattern can be configured. It support
>   smooth transitions between the states.
> 
> 
> So, while I think I see how you would like to architect this interface I
> am not sure how to figure out the details.
> 
> The pattern definition would have to be expressive enough to support the
> features of LP5xx and direct enough to support the limited AS3676. It
> would likely have to express transitions, so that the LPG could generate
> intermediate steps (and we will have to adapt the resolution of the
> ramps based on the other LPGs in the system).
> 
> How do we do with patterns that are implementable by the LP5xx but are
> not with the LPG? Should we reject those or should we do some sort of
> best-effort approach in the kernel?

Lets say you get series of

(red, green, blue, delta_t )

points, meaning "in delta_t msec, change color to red, green,
blue. Lets ignore other channels for now. delta_t of 0 would be step
change. Would such interface work for you?

Simple compiler from this to LP5XX code should not be hard to
do. AS3676 ... I'm not sure what to do, AFAICT it is too limited.

									Pavel
-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 181 bytes --]

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

[Jacek Anaszewski]

Hi,

On 04/08/2017 03:39 PM, Pavel Machek wrote:
> Hi!
> 
>> [..]
>>>> For the patterns I don't know how a trigger for this would look like,
>>>> how would setting the pattern of a trigger be propagated down to the
>>>> hardware?
>>>
>>> We'd need a new op and API similar to blink_set()/led_blink_set().
>>>
>>
>> I've tried to find different LED circuits with some sort of pattern
>> generator in an attempt to figure out how to design this interface, but
>> turned out to be quite hard to find examples; the three I can compare
>> are:
>>
>> * LP5xx series "implements" pattern generation by executing code.
>>
>> * Qualcomm LPG iterates over 2-64 brightness-values in a pattern, at a
>>   fixed rate with knobs to configure what happens before starting and
>>   after finishing iterating over the defined values. It does not support
>>   smooth transitions between values.
>>
>> * AS3676 supports a pattern of 32 values controlling if the output
>>   should be enabled or disabled for each 32.5ms (or 250ms) time period.
>>   The delay before repeating the pattern can be configured. It support
>>   smooth transitions between the states.
>>
>>
>> So, while I think I see how you would like to architect this interface I
>> am not sure how to figure out the details.
>>
>> The pattern definition would have to be expressive enough to support the
>> features of LP5xx and direct enough to support the limited AS3676. It
>> would likely have to express transitions, so that the LPG could generate
>> intermediate steps (and we will have to adapt the resolution of the
>> ramps based on the other LPGs in the system).
>>
>> How do we do with patterns that are implementable by the LP5xx but are
>> not with the LPG? Should we reject those or should we do some sort of
>> best-effort approach in the kernel?
> 
> Lets say you get series of
> 
> (red, green, blue, delta_t )

In order to make it possible we'd have to have a means for mapping
LED class devices to red, green and blue. In effect I see the problem
of introducing a new mechanism for creating compound LED class device
out of existing LED class devices as the first one to address.

Once we have compound LED class device, that would expose an interface
for operating on the particular color brightnesses, then we can build
upon it the pattern engine. Actually, the same compound LED mechanism
would be necessary for defining blink patterns for strings of monochrome
LEDs.

> points, meaning "in delta_t msec, change color to red, green,
> blue. Lets ignore other channels for now. delta_t of 0 would be step
> change. Would such interface work for you?
> 
> Simple compiler from this to LP5XX code should not be hard to
> do. AS3676 ... I'm not sure what to do, AFAICT it is too limited.

Our new API for setting blink patterns could be defined so that it
does not guarantee setting the requested pattern, but applies only
what the hardware can support and returns the applied settings.
E.g. this way the driver could reduce the requested brightness
transition resolution.

There is also a question if we should provide software fallback
for patterns not supported by the hardware and to what extent.
In case of blink support for a single LED we do that but in case
of more complex patterns it would require more complex logic,
and at least for now I'd avoid it.

-- 
Best regards,
Jacek Anaszewski

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

[Pavel Machek]

Hi!

> > Actually we could achieve the goal by listing all available pattern
> > configurations for given LED class device, so in case of Qualcomm LPG
> > driver we could have transition-pattern-1 to transition-pattern-15
> > listed after executing "cat trigger".
> > 
> 
> There's a common pattern-table of 24 (or 64) entries, that is shared
> among the 8 LPGs (each LPG simply has to indices pointing into the
> shared table). Each entry in the table holds a value between 0 and 511.
> So that's a lot of "available pattern configurations".

> > I wonder if I'm not missing some vital constraints here that could
> > make this design unfeasible.
> > 
> 
> Regardless of how we expose RGBs to userspace, the 8 LPG hardware blocks
> are independent of each other. The fact that they end up controlling
> something that is perceived by the human eye as some mixed color is to
> me a matter of system integration, and as such should not convolute the
> implementation of the individual instances.

Well... the 8 LPG blocks share the pattern-table.. and the pattern-table is very
limited. We could statically allocate 3 entries to each LPG block, but that
would not be too useful. And if we dynamically allocate entries depending on
patterns, then the LPG blocks are no longer independent.

									Pavel

-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html

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

[Bjorn Andersson]

On Sat 08 Apr 06:39 PDT 2017, Pavel Machek wrote:

> Hi!
> 
> > [..]
> > > > For the patterns I don't know how a trigger for this would look like,
> > > > how would setting the pattern of a trigger be propagated down to the
> > > > hardware?
> > > 
> > > We'd need a new op and API similar to blink_set()/led_blink_set().
> > > 
> > 
> > I've tried to find different LED circuits with some sort of pattern
> > generator in an attempt to figure out how to design this interface, but
> > turned out to be quite hard to find examples; the three I can compare
> > are:
> > 
> > * LP5xx series "implements" pattern generation by executing code.
> > 
> > * Qualcomm LPG iterates over 2-64 brightness-values in a pattern, at a
> >   fixed rate with knobs to configure what happens before starting and
> >   after finishing iterating over the defined values. It does not support
> >   smooth transitions between values.
> > 
> > * AS3676 supports a pattern of 32 values controlling if the output
> >   should be enabled or disabled for each 32.5ms (or 250ms) time period.
> >   The delay before repeating the pattern can be configured. It support
> >   smooth transitions between the states.
> > 
> > 
> > So, while I think I see how you would like to architect this interface I
> > am not sure how to figure out the details.
> > 
> > The pattern definition would have to be expressive enough to support the
> > features of LP5xx and direct enough to support the limited AS3676. It
> > would likely have to express transitions, so that the LPG could generate
> > intermediate steps (and we will have to adapt the resolution of the
> > ramps based on the other LPGs in the system).
> > 
> > How do we do with patterns that are implementable by the LP5xx but are
> > not with the LPG? Should we reject those or should we do some sort of
> > best-effort approach in the kernel?
> 
> Lets say you get series of
> 
> (red, green, blue, delta_t )
> 
> points, meaning "in delta_t msec, change color to red, green,
> blue. Lets ignore other channels for now. delta_t of 0 would be step
> change. Would such interface work for you?
> 

So I presume this would be input to the RGB trigger that we discussed.
But in my current device I have 6 LEDs, that are not in any RGB-like
configuration. So we would need to come up with an interface that looks
to be the same in both single-LED and RGB-LED setups.


This should be sufficient to describe a subset of the patterns I've seen
so far in products.

But let's consider the standard use case for an RGB LED on an Android
phone; continuously blinking (pulsing based on patterns) as you have
some notifications waiting. In this case you want the LED hardware to do
all the work, so that you can deep-idle the CPU. So we would need to
introduce a "repeat pattern"-command.

Then consider the fact that you want your patterns to have decent
resolution, but you have a limited amount of storage. So we either have
to be able to detect palindromes or have a way to represent this.

> Simple compiler from this to LP5XX code should not be hard to
> do.

It sounds fairly straight forward to convert a pattern to instructions,
but we do have an extremely limited amount of storage so it must be a
quite good implementation for people to be able to use it for anything
real.

We could implement some optimization steps where we try to detect slopes
and generate ramp-instructions instead of set-pwm + wait instructions,
use some variables to handle ramp up/down and we could probably generate
some jump instructions to implement loops.

But do we really want this logic in the kernel, for each LED chip
supporting patterns?

> AS3676 ... I'm not sure what to do, AFAICT it is too limited.
> 

So out of the three examples I've looked at we're skipping one and we're
abstracting away most functionality from another.

I'm sorry for being pessimistic about this, but while I can see the
theoretical benefit of providing a uniform interface for this to user
space I see three very different pieces of hardware that would be used
in three different ways in products.

Regards,
Bjorn

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

[Pavel Machek]

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

Hi!

> > > How do we do with patterns that are implementable by the LP5xx but are
> > > not with the LPG? Should we reject those or should we do some sort of
> > > best-effort approach in the kernel?
> > 
> > Lets say you get series of
> > 
> > (red, green, blue, delta_t )
> > 
> > points, meaning "in delta_t msec, change color to red, green,
> > blue. Lets ignore other channels for now. delta_t of 0 would be step
> > change. Would such interface work for you?
> 
> So I presume this would be input to the RGB trigger that we discussed.
> But in my current device I have 6 LEDs, that are not in any RGB-like
> configuration. So we would need to come up with an interface that looks
> to be the same in both single-LED and RGB-LED setups.

Ok.

> This should be sufficient to describe a subset of the patterns I've seen
> so far in products.
> 
> But let's consider the standard use case for an RGB LED on an Android
> phone; continuously blinking (pulsing based on patterns) as you have
> some notifications waiting. In this case you want the LED hardware to do
> all the work, so that you can deep-idle the CPU. So we would need to
> introduce a "repeat pattern"-command.

I'd say have additional parameter with number of repetitions. Yes. In
your case you can do 1 and infinity, LP5XX can do 1-255 or infinity.

> Then consider the fact that you want your patterns to have decent
> resolution, but you have a limited amount of storage. So we either have
> to be able to detect palindromes or have a way to represent this.

I'm not sure how common hardware support for palindromes is going to
be. I'd say "detect", but...

> > Simple compiler from this to LP5XX code should not be hard to
> > do.
> 
> It sounds fairly straight forward to convert a pattern to instructions,
> but we do have an extremely limited amount of storage so it must be a
> quite good implementation for people to be able to use it for anything
> real.
> 
> We could implement some optimization steps where we try to detect slopes
> and generate ramp-instructions instead of set-pwm + wait instructions,
> use some variables to handle ramp up/down and we could probably generate
> some jump instructions to implement loops.

Actually it is easier than that. Hardware can do slopes itself. If we
see change with non-zero delta_t, we issue slope, otherwise we issue
set_value.

Here's example "compiler": https://gitlab.com/tui/tui/blob/master/ofone/notcc.py
Here's example "program": https://gitlab.com/tui/tui/blob/master/ofone/tests.notcc/primes.nc

> But do we really want this logic in the kernel, for each LED chip
> supporting patterns?

I'd say so, yes. It should be, dunno, 200? 500? lines of code for
LP5XX?  Sounds acceptable.

Otherwise we'd have to have led-chip-dependend part in userspace. That
would be ok... but we'd _still_ need led-chip-dependend part in the
kernel... and driver spread between kernel and userland is difficult.

The code needs to be created, anyway, so lets put it in kernel.

> > AS3676 ... I'm not sure what to do, AFAICT it is too limited.
> > 
> 
> So out of the three examples I've looked at we're skipping one and we're
> abstracting away most functionality from another.

Well. We don't need to _skip_ AS3676, but its pattern engine is
basically useless for anything involving different PWM levels.

And abstracting away most of LP5XX functionality... well, you can
compute prime numbers on that chip (see example above), but you better
should not. And patterns we'll pretty much expose all the functionality.

> I'm sorry for being pessimistic about this, but while I can see the
> theoretical benefit of providing a uniform interface for this to user
> space I see three very different pieces of hardware that would be used
> in three different ways in products.

Three different pieces of hardware, at least two of them used in
phones to provide blinking leds... I'd say common interface is the
right thing to do.
								Pavel
-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 181 bytes --]

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

[Bjorn Andersson]

On Tue 11 Apr 10:54 PDT 2017, Pavel Machek wrote:

> Hi!
> 
> > > > How do we do with patterns that are implementable by the LP5xx but are
> > > > not with the LPG? Should we reject those or should we do some sort of
> > > > best-effort approach in the kernel?
> > > 
> > > Lets say you get series of
> > > 
> > > (red, green, blue, delta_t )
> > > 
> > > points, meaning "in delta_t msec, change color to red, green,
> > > blue. Lets ignore other channels for now. delta_t of 0 would be step
> > > change. Would such interface work for you?
> > 
> > So I presume this would be input to the RGB trigger that we discussed.
> > But in my current device I have 6 LEDs, that are not in any RGB-like
> > configuration. So we would need to come up with an interface that looks
> > to be the same in both single-LED and RGB-LED setups.
> 
> Ok.
> 
> > This should be sufficient to describe a subset of the patterns I've seen
> > so far in products.
> > 
> > But let's consider the standard use case for an RGB LED on an Android
> > phone; continuously blinking (pulsing based on patterns) as you have
> > some notifications waiting. In this case you want the LED hardware to do
> > all the work, so that you can deep-idle the CPU. So we would need to
> > introduce a "repeat pattern"-command.
> 
> I'd say have additional parameter with number of repetitions. Yes. In
> your case you can do 1 and infinity, LP5XX can do 1-255 or infinity.
> 

Sounds reasonable.

> > Then consider the fact that you want your patterns to have decent
> > resolution, but you have a limited amount of storage. So we either have
> > to be able to detect palindromes or have a way to represent this.
> 
> I'm not sure how common hardware support for palindromes is going to
> be. I'd say "detect", but...
> 

Even with the LP5xx I presume you would want to implement a natural
pulse by running back and forth over something like a section of a
sigmoid function - rather than encoding all the values for both raise
and falling edge.

But its easy to detect these patterns, so lets just have the drivers do
it for now.

> > > Simple compiler from this to LP5XX code should not be hard to
> > > do.
> > 
> > It sounds fairly straight forward to convert a pattern to instructions,
> > but we do have an extremely limited amount of storage so it must be a
> > quite good implementation for people to be able to use it for anything
> > real.
> > 
> > We could implement some optimization steps where we try to detect slopes
> > and generate ramp-instructions instead of set-pwm + wait instructions,
> > use some variables to handle ramp up/down and we could probably generate
> > some jump instructions to implement loops.
> 
> Actually it is easier than that. Hardware can do slopes itself. If we
> see change with non-zero delta_t, we issue slope, otherwise we issue
> set_value.
> 

So given a sequence of x, delta_t, y, delta_t', z, delta_t'' the
hardware should:

* set the brightness to x at time 0
* set the brightness to y at time delta_t
* set the brightness to z at time delta_t + delta_t'
* if repeating we will start the next sequence at time delta_t +
  delta_t' + delta_t''


The question is how to define the intermediate segments. We could say
that the transition between two points must always be some ramp or we
expect user space to use small enough delta_t so any differences between
hardware with or without ramp support are not noticeable.

In the prior LPG would have to make up intermediate values to create a
ramp effect, with some heuristics for not consuming to much lookup-table
space.

With the latter approach the LP55xx would probably be expected to drop
intermediate values that lay on the path between two other values in an
effort to reduce memory used.


I see a problem with the first approach in that with the LPG we could
overcommit to a smooth curve and later not have enough room to configure
some other curve (for some other LED).

So I would like to suggest that we go with the latter approach.

> Here's example "compiler": https://gitlab.com/tui/tui/blob/master/ofone/notcc.py
> Here's example "program": https://gitlab.com/tui/tui/blob/master/ofone/tests.notcc/primes.nc
> 
> > But do we really want this logic in the kernel, for each LED chip
> > supporting patterns?
> 
> I'd say so, yes. It should be, dunno, 200? 500? lines of code for
> LP5XX?  Sounds acceptable.
> 
> Otherwise we'd have to have led-chip-dependend part in userspace. That
> would be ok... but we'd _still_ need led-chip-dependend part in the
> kernel... and driver spread between kernel and userland is difficult.
> 
> The code needs to be created, anyway, so lets put it in kernel.
> 

My background is in consumer devices, where humans would generate the
sequence and just encode this in user space, you wouldn't recompile this
on the fly - and the LP5xx use of firmware files seems to indicate a
similar behavior.

But I think what we're discussing here fits the driver I'm trying to
upstream reasonably well, so if you think this is ok for the LP5xx we
should be good.

> > > AS3676 ... I'm not sure what to do, AFAICT it is too limited.
> > > 
> > 
> > So out of the three examples I've looked at we're skipping one and we're
> > abstracting away most functionality from another.
> 
> Well. We don't need to _skip_ AS3676, but its pattern engine is
> basically useless for anything involving different PWM levels.
> 

Yes

> And abstracting away most of LP5XX functionality... well, you can
> compute prime numbers on that chip (see example above), but you better
> should not. And patterns we'll pretty much expose all the functionality.
> 

Ok

> > I'm sorry for being pessimistic about this, but while I can see the
> > theoretical benefit of providing a uniform interface for this to user
> > space I see three very different pieces of hardware that would be used
> > in three different ways in products.
> 
> Three different pieces of hardware, at least two of them used in
> phones to provide blinking leds... I'd say common interface is the
> right thing to do.

All three of them are used in phones, to provide blinking light. With
some allowance for fitting points to the hardware's capabilities and the
ability to reject incompatible patterns it should be possible to support
some common cases.

I'll take a stab at this for the LPG and we can discuss from there.

Regards,
Bjorn

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

[Pavel Machek]

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

Hi!

> 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>

I'd really like to see the patch fixing the pattern interface (or
disabling it). I don't want to push the tree to Linus with that bug.

Thanks,
								Pavel
-- 
People of Russia, stop Putin before his war on Ukraine escalates.

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

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

[Marijn Suijten]

On 2022-05-23 18:30:38, Pavel Machek wrote:
> Hi!
> 
> > 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>
> 
> I'd really like to see the patch fixing the pattern interface (or
> disabling it). I don't want to push the tree to Linus with that bug.

(I couldn't help but be confused for a minute by this being a reply to
the original v1 patchset from March 2017 :) )

Does that mean there's still some time to review / pick up [1]
(LPG enablement for PM660L)?  And even more so for [2] (fixing the use
of a software-pattern variable in the hardware-pattern code) which
complements Bjorn's series but hasn't been looked at ever since last
year.

I wouldn't mind picking up this issue (discussed in the v14 series at
[3]) and unblock you sending the tree to Linus without reverting, if
Bjorn doesn't have the bandwidth for it currently.  But I will need
confirmation that patches sent in my name actually get looked at...
Thanks!

- Marijn

[1]: https://lore.kernel.org/linux-leds/20220511190718.764445-1-marijn.suijten@somainline.org/
[2]: https://lore.kernel.org/linux-leds/20210915080252.69147-1-marijn.suijten@somainline.org/
[3]: https://lore.kernel.org/linux-leds/YnvhleAI5RW0ZvkV@ripper/T/#m6cb0d8df051bbcd3772d068640ccd784678ad47b

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

[Pavel Machek]

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

On Tue 2022-05-24 00:01:07, Marijn Suijten wrote:
> On 2022-05-23 18:30:38, Pavel Machek wrote:
> > Hi!
> > 
> > > 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>
> > 
> > I'd really like to see the patch fixing the pattern interface (or
> > disabling it). I don't want to push the tree to Linus with that bug.
> 
> (I couldn't help but be confused for a minute by this being a reply to
> the original v1 patchset from March 2017 :) )
> 
> Does that mean there's still some time to review / pick up [1]
> (LPG enablement for PM660L)?  And even more so for [2] (fixing the use
> of a software-pattern variable in the hardware-pattern code) which
> complements Bjorn's series but hasn't been looked at ever since last
> year.

There's still time if the patches are perfect.

								Pavel
-- 
People of Russia, stop Putin before his war on Ukraine escalates.

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

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

[Bjorn Andersson]

On Mon, May 23, 2022 at 11:30 AM Pavel Machek <pavel@ucw.cz> wrote:
>
> Hi!
>
> > 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>
>
> I'd really like to see the patch fixing the pattern interface (or
> disabling it). I don't want to push the tree to Linus with that bug.
>

Please find a proposed update to lpg_pattern_set() and the documentation at:
https://lore.kernel.org/linux-arm-msm/20220523233719.1496297-1-bjorn.andersson@linaro.org/T/#u

@Marijn, would love to get your input on this proposal.

Regards,
Bjorn

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

[Marijn Suijten]

On 2022-05-24 00:18:35, Pavel Machek wrote:
[...]
> > > I'd really like to see the patch fixing the pattern interface (or
> > > disabling it). I don't want to push the tree to Linus with that bug.
> > 
> > (I couldn't help but be confused for a minute by this being a reply to
> > the original v1 patchset from March 2017 :) )
> > 
> > Does that mean there's still some time to review / pick up [1]
> > (LPG enablement for PM660L)?  And even more so for [2] (fixing the use
> > of a software-pattern variable in the hardware-pattern code) which
> > complements Bjorn's series but hasn't been looked at ever since last
> > year.
> 
> There's still time if the patches are perfect.

How should I know if a patch is perfect when it has been ignored for
over a year [1]?  Only Bjorn has reviewed and tested it.

I'm not asking for an instant merge, but feedback is appreciated.  I
don't see any glaring issues with the patch so I'm kindly (re-)asking
you to point them out so that I can correct the patch.

- Marijn

[1]: https://lore.kernel.org/linux-leds/20210418213427.220638-1-marijn.suijten@somainline.org/

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

[Marijn Suijten]

On 2022-05-24 10:02:51, Bjorn Andersson wrote:
[..]
> > I'd really like to see the patch fixing the pattern interface (or
> > disabling it). I don't want to push the tree to Linus with that bug.
> >
> 
> Please find a proposed update to lpg_pattern_set() and the documentation at:
> https://lore.kernel.org/linux-arm-msm/20220523233719.1496297-1-bjorn.andersson@linaro.org/T/#u
> 
> @Marijn, would love to get your input on this proposal.

Thanks, it looks good from a first observation but I'll test out the
implementation later.  Meanwhile, curious to your progress on processing
the last review :)

- Marijn

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

[Pavel Machek]

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

Hi!

> > > 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>
> >
> > I'd really like to see the patch fixing the pattern interface (or
> > disabling it). I don't want to push the tree to Linus with that bug.
> >
> 
> Please find a proposed update to lpg_pattern_set() and the documentation at:
> https://lore.kernel.org/linux-arm-msm/20220523233719.1496297-1-bjorn.andersson@linaro.org/T/#u

Thank you, applied.

Best regards,
								Pavel
-- 
People of Russia, stop Putin before his war on Ukraine escalates.

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