[PATCH v2 1/2] leds: add DT binding for Qualcomm PM8941 WLED block

[PATCH v2 1/2] leds: add DT binding for Qualcomm PM8941 WLED block

[Bjorn Andersson]

From: Courtney Cavin <courtney.cavin@sonymobile.com>

This adds device tree binding documentation for the WLED ('White' LED)
block on Qualcomm's PM8941 PMICs.

Signed-off-by: Courtney Cavin <courtney.cavin@sonymobile.com>
Signed-off-by: Bjorn Andersson <bjorn.andersson@sonymobile.com>
---
 .../devicetree/bindings/leds/leds-pm8941-wled.txt  | 43 ++++++++++++++++++++++
 1 file changed, 43 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/leds/leds-pm8941-wled.txt

diff --git a/Documentation/devicetree/bindings/leds/leds-pm8941-wled.txt b/Documentation/devicetree/bindings/leds/leds-pm8941-wled.txt
new file mode 100644
index 0000000..a85a964
--- /dev/null
+++ b/Documentation/devicetree/bindings/leds/leds-pm8941-wled.txt
@@ -0,0 +1,43 @@
+Binding for Qualcomm PM8941 WLED driver
+
+Required properties:
+- compatible: should be "qcom,pm8941-wled"
+- reg: slave address
+
+Optional properties:
+- label: The label for this led
+  See Documentation/devicetree/bindings/leds/common.txt
+- linux,default-trigger: Default trigger assigned to the LED
+  See Documentation/devicetree/bindings/leds/common.txt
+- qcom,cs-out: bool; enable current sink output
+- qcom,cabc: bool; enable content adaptive backlight control
+- qcom,ext-gen: bool; use externally generated modulator signal to dim
+- qcom,current-limit: mA; per-string current limit; value from 0 to 25
+	default: 20mA
+- qcom,current-boost-limit: mA; boost current limit; one of:
+	105, 385, 525, 805, 980, 1260, 1400, 1680
+	default: 805mA
+- qcom,switching-freq: kHz; switching frequency; one of:
+	600, 640, 685, 738, 800, 872, 960, 1066, 1200, 1371,
+	1600, 1920, 2400, 3200, 4800, 9600,
+	default: 1600kHz
+- qcom,ovp: V; Over-voltage protection limit; one of:
+	27, 29, 32, 35
+	default: 29V
+- qcom,num-strings: #; number of led strings attached; value from 1 to 3
+	default: 2
+
+Example:
+
+pm8941-wled@d800 {
+	compatible = "qcom,pm8941-wled";
+	reg = <0xd800>;
+	label = "backlight";
+
+	qcom,cs-out;
+	qcom,current-limit = <20>;
+	qcom,current-boost-limit = <805>;
+	qcom,switching-freq = <1600>;
+	qcom,ovp = <29>;
+	qcom,num-strings = <2>;
+};
-- 
1.9.1

[PATCH v2 2/2] leds: add Qualcomm PM8941 WLED driver

[Bjorn Andersson]

From: Courtney Cavin <courtney.cavin@sonymobile.com>

This adds support for the WLED ('White' LED) block on Qualcomm's
PM8941 PMICs.

Signed-off-by: Courtney Cavin <courtney.cavin@sonymobile.com>
Signed-off-by: Bjorn Andersson <bjorn.andersson@sonymobile.com>
---
 drivers/leds/Kconfig            |   8 +
 drivers/leds/Makefile           |   1 +
 drivers/leds/leds-pm8941-wled.c | 459 ++++++++++++++++++++++++++++++++++++++++
 3 files changed, 468 insertions(+)
 create mode 100644 drivers/leds/leds-pm8941-wled.c

diff --git a/drivers/leds/Kconfig b/drivers/leds/Kconfig
index a6c3d2f..901b21a 100644
--- a/drivers/leds/Kconfig
+++ b/drivers/leds/Kconfig
@@ -516,6 +516,14 @@ config LEDS_VERSATILE
 	  This option enabled support for the LEDs on the ARM Versatile
 	  and RealView boards. Say Y to enabled these.
 
+config LEDS_PM8941_WLED
+	tristate "LED support for the Qualcomm PM8941 WLED block"
+	depends on LEDS_CLASS
+	select REGMAP
+	help
+	  This option enables support for the 'White' LED block
+	  on Qualcomm PM8941 PMICs.
+
 comment "LED Triggers"
 source "drivers/leds/trigger/Kconfig"
 
diff --git a/drivers/leds/Makefile b/drivers/leds/Makefile
index 1c65a19..4325e49 100644
--- a/drivers/leds/Makefile
+++ b/drivers/leds/Makefile
@@ -57,6 +57,7 @@ obj-$(CONFIG_LEDS_BLINKM)		+= leds-blinkm.o
 obj-$(CONFIG_LEDS_SYSCON)		+= leds-syscon.o
 obj-$(CONFIG_LEDS_VERSATILE)		+= leds-versatile.o
 obj-$(CONFIG_LEDS_MENF21BMC)		+= leds-menf21bmc.o
+obj-$(CONFIG_LEDS_PM8941_WLED)		+= leds-pm8941-wled.o
 
 # LED SPI Drivers
 obj-$(CONFIG_LEDS_DAC124S085)		+= leds-dac124s085.o
diff --git a/drivers/leds/leds-pm8941-wled.c b/drivers/leds/leds-pm8941-wled.c
new file mode 100644
index 0000000..beaa7de
--- /dev/null
+++ b/drivers/leds/leds-pm8941-wled.c
@@ -0,0 +1,459 @@
+/* Copyright (c) 2015, Sony Mobile Communications, AB.
+ *
+ * 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/kernel.h>
+#include <linux/leds.h>
+#include <linux/module.h>
+#include <linux/of.h>
+#include <linux/of_device.h>
+#include <linux/regmap.h>
+
+#define PM8941_WLED_REG_VAL_BASE		0x40
+#define  PM8941_WLED_REG_VAL_MAX		0xFFF
+
+#define PM8941_WLED_REG_MOD_EN			0x46
+#define  PM8941_WLED_REG_MOD_EN_BIT		BIT(7)
+#define  PM8941_WLED_REG_MOD_EN_MASK		BIT(7)
+
+#define PM8941_WLED_REG_SYNC			0x47
+#define  PM8941_WLED_REG_SYNC_MASK		0x07
+#define  PM8941_WLED_REG_SYNC_LED1		BIT(0)
+#define  PM8941_WLED_REG_SYNC_LED2		BIT(1)
+#define  PM8941_WLED_REG_SYNC_LED3		BIT(2)
+#define  PM8941_WLED_REG_SYNC_ALL		0x07
+#define  PM8941_WLED_REG_SYNC_CLEAR		0x00
+
+#define PM8941_WLED_REG_FREQ			0x4c
+#define  PM8941_WLED_REG_FREQ_MASK		0x0f
+
+#define PM8941_WLED_REG_OVP			0x4d
+#define  PM8941_WLED_REG_OVP_MASK		0x03
+
+#define PM8941_WLED_REG_BOOST			0x4e
+#define  PM8941_WLED_REG_BOOST_MASK		0x07
+
+#define PM8941_WLED_REG_SINK			0x4f
+#define  PM8941_WLED_REG_SINK_MASK		0xe0
+#define  PM8941_WLED_REG_SINK_SHFT		0x05
+
+/* Per-'string' registers below */
+#define PM8941_WLED_REG_STR_OFFSET		0x10
+
+#define PM8941_WLED_REG_STR_MOD_EN_BASE		0x60
+#define  PM8941_WLED_REG_STR_MOD_MASK		BIT(7)
+#define  PM8941_WLED_REG_STR_MOD_EN		BIT(7)
+
+#define PM8941_WLED_REG_STR_SCALE_BASE		0x62
+#define  PM8941_WLED_REG_STR_SCALE_MASK		0x1f
+
+#define PM8941_WLED_REG_STR_MOD_SRC_BASE	0x63
+#define  PM8941_WLED_REG_STR_MOD_SRC_MASK	0x01
+#define  PM8941_WLED_REG_STR_MOD_SRC_INT	0x00
+#define  PM8941_WLED_REG_STR_MOD_SRC_EXT	0x01
+
+#define PM8941_WLED_REG_STR_CABC_BASE		0x66
+#define  PM8941_WLED_REG_STR_CABC_MASK		BIT(7)
+#define  PM8941_WLED_REG_STR_CABC_EN		BIT(7)
+
+struct pm8941_wled_config {
+	u32 i_boost_limit;
+	u32 ovp;
+	u32 switch_freq;
+	u32 num_strings;
+	u32 i_limit;
+	bool cs_out_en;
+	bool ext_gen;
+	bool cabc_en;
+};
+
+struct pm8941_wled {
+	struct regmap *regmap;
+	u16 addr;
+
+	struct led_classdev cdev;
+
+	struct pm8941_wled_config cfg;
+};
+
+static int pm8941_wled_set(struct led_classdev *cdev,
+			   enum led_brightness value)
+{
+	struct pm8941_wled *wled;
+	u8 ctrl = 0;
+	u16 val;
+	int rc;
+	int i;
+
+	wled = container_of(cdev, struct pm8941_wled, cdev);
+
+	if (value != 0)
+		ctrl = PM8941_WLED_REG_MOD_EN_BIT;
+
+	val = value * PM8941_WLED_REG_VAL_MAX / LED_FULL;
+
+	rc = regmap_update_bits(wled->regmap,
+			wled->addr + PM8941_WLED_REG_MOD_EN,
+			PM8941_WLED_REG_MOD_EN_MASK, ctrl);
+	if (rc)
+		return rc;
+
+	for (i = 0; i < wled->cfg.num_strings; ++i) {
+		u8 v[2] = { val & 0xff, (val >> 8) & 0xf };
+
+		rc = regmap_bulk_write(wled->regmap,
+				wled->addr + PM8941_WLED_REG_VAL_BASE + 2 * i,
+				v, 2);
+		if (rc)
+			return rc;
+	}
+
+	rc = regmap_update_bits(wled->regmap,
+			wled->addr + PM8941_WLED_REG_SYNC,
+			PM8941_WLED_REG_SYNC_MASK, PM8941_WLED_REG_SYNC_ALL);
+	if (rc)
+		return rc;
+
+	rc = regmap_update_bits(wled->regmap,
+			wled->addr + PM8941_WLED_REG_SYNC,
+			PM8941_WLED_REG_SYNC_MASK, PM8941_WLED_REG_SYNC_CLEAR);
+	return rc;
+}
+
+static void pm8941_wled_set_brightness(struct led_classdev *cdev,
+				       enum led_brightness value)
+{
+	if (pm8941_wled_set(cdev, value)) {
+		dev_err(cdev->dev, "Unable to set brightness\n");
+		return;
+	}
+	cdev->brightness = value;
+}
+
+static int pm8941_wled_setup(struct pm8941_wled *wled)
+{
+	int rc;
+	int i;
+
+	rc = regmap_update_bits(wled->regmap,
+			wled->addr + PM8941_WLED_REG_OVP,
+			PM8941_WLED_REG_OVP_MASK, wled->cfg.ovp);
+	if (rc)
+		return rc;
+
+	rc = regmap_update_bits(wled->regmap,
+			wled->addr + PM8941_WLED_REG_BOOST,
+			PM8941_WLED_REG_BOOST_MASK, wled->cfg.i_boost_limit);
+	if (rc)
+		return rc;
+
+	rc = regmap_update_bits(wled->regmap,
+			wled->addr + PM8941_WLED_REG_FREQ,
+			PM8941_WLED_REG_FREQ_MASK, wled->cfg.switch_freq);
+	if (rc)
+		return rc;
+
+	if (wled->cfg.cs_out_en) {
+		u8 all = (BIT(wled->cfg.num_strings) - 1)
+				<< PM8941_WLED_REG_SINK_SHFT;
+
+		rc = regmap_update_bits(wled->regmap,
+				wled->addr + PM8941_WLED_REG_SINK,
+				PM8941_WLED_REG_SINK_MASK, all);
+		if (rc)
+			return rc;
+	}
+
+	for (i = 0; i < wled->cfg.num_strings; ++i) {
+		u16 addr = wled->addr + PM8941_WLED_REG_STR_OFFSET * i;
+
+		rc = regmap_update_bits(wled->regmap,
+				addr + PM8941_WLED_REG_STR_MOD_EN_BASE,
+				PM8941_WLED_REG_STR_MOD_MASK,
+				PM8941_WLED_REG_STR_MOD_EN);
+		if (rc)
+			return rc;
+
+		if (wled->cfg.ext_gen) {
+			rc = regmap_update_bits(wled->regmap,
+					addr + PM8941_WLED_REG_STR_MOD_SRC_BASE,
+					PM8941_WLED_REG_STR_MOD_SRC_MASK,
+					PM8941_WLED_REG_STR_MOD_SRC_EXT);
+			if (rc)
+				return rc;
+		}
+
+		rc = regmap_update_bits(wled->regmap,
+				addr + PM8941_WLED_REG_STR_SCALE_BASE,
+				PM8941_WLED_REG_STR_SCALE_MASK,
+				wled->cfg.i_limit);
+		if (rc)
+			return rc;
+
+		rc = regmap_update_bits(wled->regmap,
+				addr + PM8941_WLED_REG_STR_CABC_BASE,
+				PM8941_WLED_REG_STR_CABC_MASK,
+				wled->cfg.cabc_en ?
+					PM8941_WLED_REG_STR_CABC_EN : 0);
+		if (rc)
+			return rc;
+	}
+
+	return 0;
+}
+
+static const struct pm8941_wled_config pm8941_wled_config_defaults = {
+	.i_boost_limit = 3,
+	.i_limit = 20,
+	.ovp = 2,
+	.switch_freq = 5,
+	.num_strings = 0,
+	.cs_out_en = false,
+	.ext_gen = false,
+	.cabc_en = false,
+};
+
+struct pm8941_wled_var_cfg {
+	const u32 *values;
+	u32 (*fn)(u32);
+	int size;
+};
+
+static const u32 pm8941_wled_i_boost_limit_values[] = {
+	105, 385, 525, 805, 980, 1260, 1400, 1680,
+};
+
+static const struct pm8941_wled_var_cfg pm8941_wled_i_boost_limit_cfg = {
+	.values = pm8941_wled_i_boost_limit_values,
+	.size = ARRAY_SIZE(pm8941_wled_i_boost_limit_values),
+};
+
+static const u32 pm8941_wled_ovp_values[] = {
+	35, 32, 29, 27,
+};
+
+static const struct pm8941_wled_var_cfg pm8941_wled_ovp_cfg = {
+	.values = pm8941_wled_ovp_values,
+	.size = ARRAY_SIZE(pm8941_wled_ovp_values),
+};
+
+static u32 pm8941_wled_num_strings_values_fn(u32 idx)
+{
+	return idx + 1;
+}
+
+static const struct pm8941_wled_var_cfg pm8941_wled_num_strings_cfg = {
+	.fn = pm8941_wled_num_strings_values_fn,
+	.size = 3,
+};
+
+static u32 pm8941_wled_switch_freq_values_fn(u32 idx)
+{
+	return 19200 / (2 * (1 + idx));
+}
+
+static const struct pm8941_wled_var_cfg pm8941_wled_switch_freq_cfg = {
+	.fn = pm8941_wled_switch_freq_values_fn,
+	.size = 16,
+};
+
+static const struct pm8941_wled_var_cfg pm8941_wled_i_limit_cfg = {
+	.size = 26,
+};
+
+static u32 pm8941_wled_values(const struct pm8941_wled_var_cfg *cfg, u32 idx)
+{
+	if (idx >= cfg->size)
+		return UINT_MAX;
+	if (cfg->fn)
+		return cfg->fn(idx);
+	if (cfg->values)
+		return cfg->values[idx];
+	return idx;
+}
+
+static int pm8941_wled_configure(struct pm8941_wled *wled, struct device *dev)
+{
+	struct pm8941_wled_config *cfg = &wled->cfg;
+	u32 val;
+	int rc;
+	int i;
+
+	const struct {
+		const char *name;
+		u32 *val_ptr;
+		const struct pm8941_wled_var_cfg *cfg;
+	} u32_opts[] = {
+		{
+			"qcom,current-boost-limit",
+			&cfg->i_boost_limit,
+			.cfg = &pm8941_wled_i_boost_limit_cfg,
+		},
+		{
+			"qcom,current-limit",
+			&cfg->i_limit,
+			.cfg = &pm8941_wled_i_limit_cfg,
+		},
+		{
+			"qcom,ovp",
+			&cfg->ovp,
+			.cfg = &pm8941_wled_ovp_cfg,
+		},
+		{
+			"qcom,switching-freq",
+			&cfg->switch_freq,
+			.cfg = &pm8941_wled_switch_freq_cfg,
+		},
+		{
+			"qcom,num-strings",
+			&cfg->num_strings,
+			.cfg = &pm8941_wled_num_strings_cfg,
+		},
+	};
+	const struct {
+		const char *name;
+		bool *val_ptr;
+	} bool_opts[] = {
+		{ "qcom,cs-out", &cfg->cs_out_en, },
+		{ "qcom,ext-gen", &cfg->ext_gen, },
+		{ "qcom,cabc", &cfg->cabc_en, },
+	};
+
+	rc = of_property_read_u32(dev->of_node, "reg", &val);
+	if (rc || val > 0xffff) {
+		dev_err(dev, "invalid IO resources\n");
+		return rc ? rc : -EINVAL;
+	}
+	wled->addr = val;
+
+	rc = of_property_read_string(dev->of_node, "label", &wled->cdev.name);
+	if (rc)
+		wled->cdev.name = dev->of_node->name;
+
+	wled->cdev.default_trigger = of_get_property(dev->of_node,
+			"linux,default-trigger", NULL);
+
+	*cfg = pm8941_wled_config_defaults;
+	for (i = 0; i < ARRAY_SIZE(u32_opts); ++i) {
+		u32 sel, c;
+		int j, rj;
+
+		rc = of_property_read_u32(dev->of_node, u32_opts[i].name, &val);
+		if (rc) {
+			if (rc != -EINVAL) {
+				dev_err(dev, "error reading '%s'\n",
+						u32_opts[i].name);
+				return rc;
+			}
+			continue;
+		}
+
+		sel = UINT_MAX;
+		rj = -1;
+		c = pm8941_wled_values(u32_opts[i].cfg, 0);
+		for (j = 0; c != UINT_MAX; ++j) {
+			if (c <= val && (sel == UINT_MAX || c >= sel)) {
+				sel = c;
+				rj = j;
+			}
+			c = pm8941_wled_values(u32_opts[i].cfg, j + 1);
+		}
+		if (sel == UINT_MAX) {
+			dev_err(dev, "invalid value for '%s'\n",
+					u32_opts[i].name);
+			return rc;
+		}
+		if (sel != val) {
+			dev_warn(dev, "rounding '%s' down to %u\n",
+					u32_opts[i].name, sel);
+		}
+		dev_dbg(dev, "'%s' = %u\n", u32_opts[i].name, sel);
+		*u32_opts[i].val_ptr = rj;
+	};
+
+	for (i = 0; i < ARRAY_SIZE(bool_opts); ++i) {
+		if (of_property_read_bool(dev->of_node, bool_opts[i].name))
+			*bool_opts[i].val_ptr = true;
+	}
+
+	cfg->num_strings = cfg->num_strings + 1;
+
+	return 0;
+}
+
+static int pm8941_wled_probe(struct platform_device *pdev)
+{
+	struct pm8941_wled *wled;
+	struct regmap *regmap;
+	int rc;
+
+	regmap = dev_get_regmap(pdev->dev.parent, NULL);
+	if (!regmap) {
+		dev_err(&pdev->dev, "Unable to get regmap\n");
+		return -EINVAL;
+	}
+
+	wled = devm_kzalloc(&pdev->dev, sizeof(*wled), GFP_KERNEL);
+	if (!wled)
+		return -ENOMEM;
+
+	wled->regmap = regmap;
+
+	rc = pm8941_wled_configure(wled, &pdev->dev);
+	if (rc)
+		return rc;
+
+	rc = pm8941_wled_setup(wled);
+	if (rc)
+		return rc;
+
+	wled->cdev.brightness_set = pm8941_wled_set_brightness;
+
+	rc = led_classdev_register(&pdev->dev, &wled->cdev);
+	if (rc)
+		return rc;
+
+	platform_set_drvdata(pdev, wled);
+
+	return 0;
+};
+
+static int pm8941_wled_remove(struct platform_device *pdev)
+{
+	struct pm8941_wled *wled;
+
+	wled = platform_get_drvdata(pdev);
+	led_classdev_unregister(&wled->cdev);
+
+	return 0;
+}
+
+static const struct of_device_id pm8941_wled_match_table[] = {
+	{ .compatible = "qcom,pm8941-wled" },
+	{}
+};
+MODULE_DEVICE_TABLE(of, pm8941_wled_match_table);
+
+static struct platform_driver pm8941_wled_driver = {
+	.probe	= pm8941_wled_probe,
+	.remove = pm8941_wled_remove,
+	.driver	= {
+		.name		= "pm8941-wled",
+		.owner		= THIS_MODULE,
+		.of_match_table	= pm8941_wled_match_table,
+	},
+};
+
+module_platform_driver(pm8941_wled_driver);
+
+MODULE_DESCRIPTION("pm8941 wled driver");
+MODULE_LICENSE("GPL v2");
+MODULE_ALIAS("platform:pm8941-wled");
-- 
1.9.1

Re: [PATCH v2 2/2] leds: add Qualcomm PM8941 WLED driver

[Bjorn Andersson]

On Fri, Jan 23, 2015 at 4:54 PM, Bjorn Andersson
<bjorn.andersson@sonymobile.com> wrote:
> From: Courtney Cavin <courtney.cavin@sonymobile.com>
>
> This adds support for the WLED ('White' LED) block on Qualcomm's
> PM8941 PMICs.
>
> Signed-off-by: Courtney Cavin <courtney.cavin@sonymobile.com>
> Signed-off-by: Bjorn Andersson <bjorn.andersson@sonymobile.com>
> ---

Sorry, I missed the change log.

Changed since v1:
* Replace enum blob with defines
* Merged wled_context and wled structs
* Some style updates

Regards,
Bjorn

Re: [PATCH v2 2/2] leds: add Qualcomm PM8941 WLED driver

[Ivan T. Ivanov]

Hi Bjorn,

Just few nitpick comments. 

On Fri, 2015-01-23 at 16:54 -0800, Bjorn Andersson wrote:
> From: Courtney Cavin cavin@sonymobile.com>
> 
> This adds support for the WLED ('White' LED) block on Qualcomm's
> PM8941 PMICs.
> 
> Signed-off-by: Courtney Cavin cavin@sonymobile.com>
> Signed-off-by: Bjorn Andersson andersson@sonymobile.com>
> ---
>  drivers/leds/Kconfig            |   8 +
>  drivers/leds/Makefile           |   1 +
>  drivers/leds/leds-pm8941-wled.c | 459 ++++++++++++++++++++++++++++++++++++++++
>  3 files changed, 468 insertions(+)
>  create mode 100644 drivers/leds/leds-pm8941-wled.c
> 

<snip>

> +
> +#define PM8941_WLED_REG_VAL_BASE               0x40
> +#define  PM8941_WLED_REG_VAL_MAX               0xFFF
> +
> +#define PM8941_WLED_REG_MOD_EN                 0x46
> +#define  PM8941_WLED_REG_MOD_EN_BIT            BIT(7)
> +#define  PM8941_WLED_REG_MOD_EN_MASK           BIT(7)

Is it possible bit definitions to have same indentation like registers offsets?

> 
> +struct pm8941_wled_config {
> +       u32 i_boost_limit;
> +       u32 ovp;
> +       u32 switch_freq;
> +       u32 num_strings;
> +       u32 i_limit;
> +       bool cs_out_en;
> +       bool ext_gen;
> +       bool cabc_en;
> +};
> +

Could this be further squashed to bellow structure?

> +struct pm8941_wled {
> +       struct regmap *regmap;
> +       u16 addr;
> +
> +       struct led_classdev cdev;
> +
> +       struct pm8941_wled_config cfg;
> +};
> +
> 

<snip>

> +static void pm8941_wled_set_brightness(struct led_classdev *cdev,
> +                                                                                       enum 
> led_brightness value)
> +{
> +       if (pm8941_wled_set(cdev, value)) {

pm8941_wled_set() is used only here, could it be merged into this function?
 
> +               dev_err(cdev->dev, "Unable to set brightness\n");
> +               return;
> +       }
> +       cdev->brightness = value;
> +}
> +
> 

Otherwise it looks good. Driver is loaded and device is detected
properly (i have added readings for type and subtype registers).
Do you know where I can measure result from changing brightness 
sysfs entry. I am using 8074 dragonboard?

Thank you,
Ivan

Re: [PATCH v2 2/2] leds: add Qualcomm PM8941 WLED driver

[Bjorn Andersson]

On Thu 29 Jan 04:48 PST 2015, Ivan T. Ivanov wrote:

> 
> Hi Bjorn,
> 
> Just few nitpick comments. 
> 

Thanks.

> On Fri, 2015-01-23 at 16:54 -0800, Bjorn Andersson wrote:
> > From: Courtney Cavin cavin@sonymobile.com>
> > 
> > This adds support for the WLED ('White' LED) block on Qualcomm's
> > PM8941 PMICs.
> > 
> > Signed-off-by: Courtney Cavin cavin@sonymobile.com>
> > Signed-off-by: Bjorn Andersson andersson@sonymobile.com>
> > ---
> >  drivers/leds/Kconfig            |   8 +
> >  drivers/leds/Makefile           |   1 +
> >  drivers/leds/leds-pm8941-wled.c | 459 ++++++++++++++++++++++++++++++++++++++++
> >  3 files changed, 468 insertions(+)
> >  create mode 100644 drivers/leds/leds-pm8941-wled.c
> > 
> 
> <snip>
> 
> > +
> > +#define PM8941_WLED_REG_VAL_BASE               0x40
> > +#define  PM8941_WLED_REG_VAL_MAX               0xFFF
> > +
> > +#define PM8941_WLED_REG_MOD_EN                 0x46
> > +#define  PM8941_WLED_REG_MOD_EN_BIT            BIT(7)
> > +#define  PM8941_WLED_REG_MOD_EN_MASK           BIT(7)
> 
> Is it possible bit definitions to have same indentation like registers offsets?
> 

Well, yes ;)

But I like the separation, so unless Bryan would like me to change it I prefer
to leave it as is.

> > 
> > +struct pm8941_wled_config {
> > +       u32 i_boost_limit;
> > +       u32 ovp;
> > +       u32 switch_freq;
> > +       u32 num_strings;
> > +       u32 i_limit;
> > +       bool cs_out_en;
> > +       bool ext_gen;
> > +       bool cabc_en;
> > +};
> > +
> 
> Could this be further squashed to bellow structure?
> 

It could, but I see that it would simplify things.

> > +struct pm8941_wled {
> > +       struct regmap *regmap;
> > +       u16 addr;
> > +
> > +       struct led_classdev cdev;
> > +
> > +       struct pm8941_wled_config cfg;
> > +};
> > +
> > 
> 
> <snip>
> 
> > +static void pm8941_wled_set_brightness(struct led_classdev *cdev,
> > +                                                                                       enum 
> > led_brightness value)
> > +{
> > +       if (pm8941_wled_set(cdev, value)) {
> 
> pm8941_wled_set() is used only here, could it be merged into this function?
>  

It could, but that would just mean that we move these lines into the tail of
pm8941_wled_set and replace all the returns with a bunch of gotos. So I don't
think it's cleaner.

> > +               dev_err(cdev->dev, "Unable to set brightness\n");
> > +               return;
> > +       }
> > +       cdev->brightness = value;
> > +}
> > +
> > 
> 
> Otherwise it looks good. Driver is loaded and device is detected
> properly (i have added readings for type and subtype registers).
> Do you know where I can measure result from changing brightness 
> sysfs entry. I am using 8074 dragonboard?
> 

I'm not sure how this is exposed on the dragonboard. I've tested it on Xperia
Z1 (honami) and the display lights up nicely.

Regards,
Bjorn

Re: [PATCH v2 1/2] leds: add DT binding for Qualcomm PM8941 WLED block

[Bryan Wu]

On Fri, Jan 23, 2015 at 4:54 PM, Bjorn Andersson
<bjorn.andersson@sonymobile.com> wrote:
> From: Courtney Cavin <courtney.cavin@sonymobile.com>
>
> This adds device tree binding documentation for the WLED ('White' LED)
> block on Qualcomm's PM8941 PMICs.
>
> Signed-off-by: Courtney Cavin <courtney.cavin@sonymobile.com>
> Signed-off-by: Bjorn Andersson <bjorn.andersson@sonymobile.com>

I will merge this into my branch for next 3.20 release.

Thanks,
-Bryan


> ---
>  .../devicetree/bindings/leds/leds-pm8941-wled.txt  | 43 ++++++++++++++++++++++
>  1 file changed, 43 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/leds/leds-pm8941-wled.txt
>
> diff --git a/Documentation/devicetree/bindings/leds/leds-pm8941-wled.txt b/Documentation/devicetree/bindings/leds/leds-pm8941-wled.txt
> new file mode 100644
> index 0000000..a85a964
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/leds/leds-pm8941-wled.txt
> @@ -0,0 +1,43 @@
> +Binding for Qualcomm PM8941 WLED driver
> +
> +Required properties:
> +- compatible: should be "qcom,pm8941-wled"
> +- reg: slave address
> +
> +Optional properties:
> +- label: The label for this led
> +  See Documentation/devicetree/bindings/leds/common.txt
> +- linux,default-trigger: Default trigger assigned to the LED
> +  See Documentation/devicetree/bindings/leds/common.txt
> +- qcom,cs-out: bool; enable current sink output
> +- qcom,cabc: bool; enable content adaptive backlight control
> +- qcom,ext-gen: bool; use externally generated modulator signal to dim
> +- qcom,current-limit: mA; per-string current limit; value from 0 to 25
> +       default: 20mA
> +- qcom,current-boost-limit: mA; boost current limit; one of:
> +       105, 385, 525, 805, 980, 1260, 1400, 1680
> +       default: 805mA
> +- qcom,switching-freq: kHz; switching frequency; one of:
> +       600, 640, 685, 738, 800, 872, 960, 1066, 1200, 1371,
> +       1600, 1920, 2400, 3200, 4800, 9600,
> +       default: 1600kHz
> +- qcom,ovp: V; Over-voltage protection limit; one of:
> +       27, 29, 32, 35
> +       default: 29V
> +- qcom,num-strings: #; number of led strings attached; value from 1 to 3
> +       default: 2
> +
> +Example:
> +
> +pm8941-wled@d800 {
> +       compatible = "qcom,pm8941-wled";
> +       reg = <0xd800>;
> +       label = "backlight";
> +
> +       qcom,cs-out;
> +       qcom,current-limit = <20>;
> +       qcom,current-boost-limit = <805>;
> +       qcom,switching-freq = <1600>;
> +       qcom,ovp = <29>;
> +       qcom,num-strings = <2>;
> +};
> --
> 1.9.1
>

Re: [PATCH v2 2/2] leds: add Qualcomm PM8941 WLED driver

[Bryan Wu]

On Thu, Jan 29, 2015 at 11:07 AM, Bjorn Andersson
<bjorn.andersson@sonymobile.com> wrote:
> On Thu 29 Jan 04:48 PST 2015, Ivan T. Ivanov wrote:
>
>>
>> Hi Bjorn,
>>
>> Just few nitpick comments.
>>
>
> Thanks.
>
>> On Fri, 2015-01-23 at 16:54 -0800, Bjorn Andersson wrote:
>> > From: Courtney Cavin cavin@sonymobile.com>
>> >
>> > This adds support for the WLED ('White' LED) block on Qualcomm's
>> > PM8941 PMICs.
>> >
>> > Signed-off-by: Courtney Cavin cavin@sonymobile.com>
>> > Signed-off-by: Bjorn Andersson andersson@sonymobile.com>


I'm good with this patch and will merge it into my tree.

Thanks,
-Bryan

>> > ---
>> >  drivers/leds/Kconfig            |   8 +
>> >  drivers/leds/Makefile           |   1 +
>> >  drivers/leds/leds-pm8941-wled.c | 459 ++++++++++++++++++++++++++++++++++++++++
>> >  3 files changed, 468 insertions(+)
>> >  create mode 100644 drivers/leds/leds-pm8941-wled.c
>> >
>>
>> <snip>
>>
>> > +
>> > +#define PM8941_WLED_REG_VAL_BASE               0x40
>> > +#define  PM8941_WLED_REG_VAL_MAX               0xFFF
>> > +
>> > +#define PM8941_WLED_REG_MOD_EN                 0x46
>> > +#define  PM8941_WLED_REG_MOD_EN_BIT            BIT(7)
>> > +#define  PM8941_WLED_REG_MOD_EN_MASK           BIT(7)
>>
>> Is it possible bit definitions to have same indentation like registers offsets?
>>
>
> Well, yes ;)
>
> But I like the separation, so unless Bryan would like me to change it I prefer
> to leave it as is.
>
>> >
>> > +struct pm8941_wled_config {
>> > +       u32 i_boost_limit;
>> > +       u32 ovp;
>> > +       u32 switch_freq;
>> > +       u32 num_strings;
>> > +       u32 i_limit;
>> > +       bool cs_out_en;
>> > +       bool ext_gen;
>> > +       bool cabc_en;
>> > +};
>> > +
>>
>> Could this be further squashed to bellow structure?
>>
>
> It could, but I see that it would simplify things.
>
>> > +struct pm8941_wled {
>> > +       struct regmap *regmap;
>> > +       u16 addr;
>> > +
>> > +       struct led_classdev cdev;
>> > +
>> > +       struct pm8941_wled_config cfg;
>> > +};
>> > +
>> >
>>
>> <snip>
>>
>> > +static void pm8941_wled_set_brightness(struct led_classdev *cdev,
>> > +                                                                                       enum
>> > led_brightness value)
>> > +{
>> > +       if (pm8941_wled_set(cdev, value)) {
>>
>> pm8941_wled_set() is used only here, could it be merged into this function?
>>
>
> It could, but that would just mean that we move these lines into the tail of
> pm8941_wled_set and replace all the returns with a bunch of gotos. So I don't
> think it's cleaner.
>
>> > +               dev_err(cdev->dev, "Unable to set brightness\n");
>> > +               return;
>> > +       }
>> > +       cdev->brightness = value;
>> > +}
>> > +
>> >
>>
>> Otherwise it looks good. Driver is loaded and device is detected
>> properly (i have added readings for type and subtype registers).
>> Do you know where I can measure result from changing brightness
>> sysfs entry. I am using 8074 dragonboard?
>>
>
> I'm not sure how this is exposed on the dragonboard. I've tested it on Xperia
> Z1 (honami) and the display lights up nicely.
>
> Regards,
> Bjorn

Re: [PATCH v2 2/2] leds: add Qualcomm PM8941 WLED driver

[Stephen Boyd]

On 01/23/15 16:54, Bjorn Andersson wrote:
> +
> +static int pm8941_wled_configure(struct pm8941_wled *wled, struct device *dev)
> +{
> +	struct pm8941_wled_config *cfg = &wled->cfg;
> +	u32 val;
> +	int rc;
> +	int i;
> +
> +	const struct {
> +		const char *name;
> +		u32 *val_ptr;
> +		const struct pm8941_wled_var_cfg *cfg;
> +	} u32_opts[] = {
> +		{
> +			"qcom,current-boost-limit",
> +			&cfg->i_boost_limit,
> +			.cfg = &pm8941_wled_i_boost_limit_cfg,
> +		},
> +		{
> +			"qcom,current-limit",
> +			&cfg->i_limit,
> +			.cfg = &pm8941_wled_i_limit_cfg,
> +		},
> +		{
> +			"qcom,ovp",
> +			&cfg->ovp,
> +			.cfg = &pm8941_wled_ovp_cfg,
> +		},
> +		{
> +			"qcom,switching-freq",
> +			&cfg->switch_freq,
> +			.cfg = &pm8941_wled_switch_freq_cfg,
> +		},
> +		{
> +			"qcom,num-strings",
> +			&cfg->num_strings,
> +			.cfg = &pm8941_wled_num_strings_cfg,
> +		},
> +	};
> +	const struct {
> +		const char *name;
> +		bool *val_ptr;
> +	} bool_opts[] = {
> +		{ "qcom,cs-out", &cfg->cs_out_en, },
> +		{ "qcom,ext-gen", &cfg->ext_gen, },
> +		{ "qcom,cabc", &cfg->cabc_en, },
> +	};
> +
> +	rc = of_property_read_u32(dev->of_node, "reg", &val);
> +	if (rc || val > 0xffff) {
> +		dev_err(dev, "invalid IO resources\n");
> +		return rc ? rc : -EINVAL;
> +	}
> +	wled->addr = val;
> +
> +	rc = of_property_read_string(dev->of_node, "label", &wled->cdev.name);
> +	if (rc)
> +		wled->cdev.name = dev->of_node->name;
> +
> +	wled->cdev.default_trigger = of_get_property(dev->of_node,
> +			"linux,default-trigger", NULL);
> +
> +	*cfg = pm8941_wled_config_defaults;
> +	for (i = 0; i < ARRAY_SIZE(u32_opts); ++i) {
> +		u32 sel, c;
> +		int j, rj;
> +
> +		rc = of_property_read_u32(dev->of_node, u32_opts[i].name, &val);
> +		if (rc) {
> +			if (rc != -EINVAL) {
> +				dev_err(dev, "error reading '%s'\n",
> +						u32_opts[i].name);
> +				return rc;
> +			}
> +			continue;
> +		}
> +
> +		sel = UINT_MAX;
> +		rj = -1;
> +		c = pm8941_wled_values(u32_opts[i].cfg, 0);
> +		for (j = 0; c != UINT_MAX; ++j) {
> +			if (c <= val && (sel == UINT_MAX || c >= sel)) {
> +				sel = c;
> +				rj = j;
> +			}
> +			c = pm8941_wled_values(u32_opts[i].cfg, j + 1);
> +		}
> +		if (sel == UINT_MAX) {
> +			dev_err(dev, "invalid value for '%s'\n",
> +					u32_opts[i].name);
> +			return rc;

Isn't rc always 0 here? Don't we want to return an error?

Also, I find this code very convoluted given that we loop through a
table and match based on nodes and call function pointers, etc. Why
can't we just have a handful of if statements with of_property_read_u32
in them? That way we don't have to jump through so many hoops, bouncing
all around this file to figure out what's going on. If we did I imagine
we wouldn't have missed out on rc being 0 here.

> +
> +static int pm8941_wled_remove(struct platform_device *pdev)
> +{
> +	struct pm8941_wled *wled;
> +
> +	wled = platform_get_drvdata(pdev);
> +	led_classdev_unregister(&wled->cdev);

Would be nice to have a devm for this one too.

> +
> +	return 0;
> +}
> +
> +static const struct of_device_id pm8941_wled_match_table[] = {
> +	{ .compatible = "qcom,pm8941-wled" },
> +	{}
> +};
> +MODULE_DEVICE_TABLE(of, pm8941_wled_match_table);
> +
> +static struct platform_driver pm8941_wled_driver = {
> +	.probe	= pm8941_wled_probe,
> +	.remove = pm8941_wled_remove,
> +	.driver	= {
> +		.name		= "pm8941-wled",
> +		.owner		= THIS_MODULE,

THIS_MODULE should be removed.

-- 
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
a Linux Foundation Collaborative Project

Re: [PATCH v2 2/2] leds: add Qualcomm PM8941 WLED driver

[Stephen Boyd]

On 01/29/15 04:48, Ivan T. Ivanov wrote:
>
> Otherwise it looks good. Driver is loaded and device is detected
> properly (i have added readings for type and subtype registers).
> Do you know where I can measure result from changing brightness 
> sysfs entry. I am using 8074 dragonboard?

Does the backlight turn on? From what I can tell it controls the
backlight, but it may be that nothings getting displayed so it won't be
noticeable.

-- 
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
a Linux Foundation Collaborative Project

Re: [PATCH v2 2/2] leds: add Qualcomm PM8941 WLED driver

[Stephen Boyd]

On 01/23/15 16:54, Bjorn Andersson wrote:
> +
> +static int pm8941_wled_set(struct led_classdev *cdev,
> +			   enum led_brightness value)
> +{
> +	struct pm8941_wled *wled;
> +	u8 ctrl = 0;
> +	u16 val;
> +	int rc;
> +	int i;
> +
> +	wled = container_of(cdev, struct pm8941_wled, cdev);
> +
> +	if (value != 0)
> +		ctrl = PM8941_WLED_REG_MOD_EN_BIT;
> +
> +	val = value * PM8941_WLED_REG_VAL_MAX / LED_FULL;
> +
> +	rc = regmap_update_bits(wled->regmap,
> +			wled->addr + PM8941_WLED_REG_MOD_EN,
> +			PM8941_WLED_REG_MOD_EN_MASK, ctrl);
> +	if (rc)
> +		return rc;
> +
> +	for (i = 0; i < wled->cfg.num_strings; ++i) {
> +		u8 v[2] = { val & 0xff, (val >> 8) & 0xf };
> +
> +		rc = regmap_bulk_write(wled->regmap,
> +				wled->addr + PM8941_WLED_REG_VAL_BASE + 2 * i,
> +				v, 2);
> +		if (rc)
> +			return rc;
> +	}
> +
> +	rc = regmap_update_bits(wled->regmap,
> +			wled->addr + PM8941_WLED_REG_SYNC,
> +			PM8941_WLED_REG_SYNC_MASK, PM8941_WLED_REG_SYNC_ALL);
> +	if (rc)
> +		return rc;
> +
> +	rc = regmap_update_bits(wled->regmap,
> +			wled->addr + PM8941_WLED_REG_SYNC,
> +			PM8941_WLED_REG_SYNC_MASK, PM8941_WLED_REG_SYNC_CLEAR);
> +	return rc;
> +}

This doesn't seem to do anything for the OVP spike mentioned in this
patch[1]. Do you see that problem on your device? I imagine the PMIC is
the same.

[1]
https://www.codeaurora.org/cgit/quic/la/kernel/msm/commit/?id=fef9e15072562f0f28dc7066dcdd69388df81ed3

-- 
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
a Linux Foundation Collaborative Project

Re: [PATCH v2 2/2] leds: add Qualcomm PM8941 WLED driver

[Ivan T. Ivanov]

On Thu, 2015-02-12 at 20:07 -0800, Stephen Boyd wrote:
> On 01/29/15 04:48, Ivan T. Ivanov wrote:
> > Otherwise it looks good. Driver is loaded and device is detected
> > properly (i have added readings for type and subtype registers).
> > Do you know where I can measure result from changing brightness
> > sysfs entry. I am using 8074 dragonboard?
> 
> Does the backlight turn on? From what I can tell it controls the
> backlight, but it may be that nothings getting displayed so it won't be
> noticeable.
> 

Yes, I can not see visual changes. That is why I have asked where
I could hook the probe and measure current change or wherever.
BL_WLEDx signals go out from J2, but somehow I was unable to 
locate it on the board, will try to look harder :-)

Regards,
Ivan

Re: [PATCH v2 2/2] leds: add Qualcomm PM8941 WLED driver

[Stephen Boyd]

On 02/12/15 20:28, Ivan T. Ivanov wrote:
> On Thu, 2015-02-12 at 20:07 -0800, Stephen Boyd wrote:
>> On 01/29/15 04:48, Ivan T. Ivanov wrote:
>>> Otherwise it looks good. Driver is loaded and device is detected
>>> properly (i have added readings for type and subtype registers).
>>> Do you know where I can measure result from changing brightness
>>> sysfs entry. I am using 8074 dragonboard?
>> Does the backlight turn on? From what I can tell it controls the
>> backlight, but it may be that nothings getting displayed so it won't be
>> noticeable.
>>
> Yes, I can not see visual changes. That is why I have asked where
> I could hook the probe and measure current change or wherever.
> BL_WLEDx signals go out from J2, but somehow I was unable to 
> locate it on the board, will try to look harder :-)
>
>

When the screen is "off" in android I can still turn on the brightness
to max in sysfs and see the screen glow. Hopefully we don't need
something else to make that work.

-- 
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
a Linux Foundation Collaborative Project

Re: [PATCH v2 2/2] leds: add Qualcomm PM8941 WLED driver

[Bryan Wu]

On Thu, Feb 12, 2015 at 8:04 PM, Stephen Boyd <sboyd@codeaurora.org> wrote:
> On 01/23/15 16:54, Bjorn Andersson wrote:

Thanks for the review, Stephen.
Bjorn, could you please update your patch according to Stephen's review.

-Bryan

>> +
>> +static int pm8941_wled_configure(struct pm8941_wled *wled, struct device *dev)
>> +{
>> +     struct pm8941_wled_config *cfg = &wled->cfg;
>> +     u32 val;
>> +     int rc;
>> +     int i;
>> +
>> +     const struct {
>> +             const char *name;
>> +             u32 *val_ptr;
>> +             const struct pm8941_wled_var_cfg *cfg;
>> +     } u32_opts[] = {
>> +             {
>> +                     "qcom,current-boost-limit",
>> +                     &cfg->i_boost_limit,
>> +                     .cfg = &pm8941_wled_i_boost_limit_cfg,
>> +             },
>> +             {
>> +                     "qcom,current-limit",
>> +                     &cfg->i_limit,
>> +                     .cfg = &pm8941_wled_i_limit_cfg,
>> +             },
>> +             {
>> +                     "qcom,ovp",
>> +                     &cfg->ovp,
>> +                     .cfg = &pm8941_wled_ovp_cfg,
>> +             },
>> +             {
>> +                     "qcom,switching-freq",
>> +                     &cfg->switch_freq,
>> +                     .cfg = &pm8941_wled_switch_freq_cfg,
>> +             },
>> +             {
>> +                     "qcom,num-strings",
>> +                     &cfg->num_strings,
>> +                     .cfg = &pm8941_wled_num_strings_cfg,
>> +             },
>> +     };
>> +     const struct {
>> +             const char *name;
>> +             bool *val_ptr;
>> +     } bool_opts[] = {
>> +             { "qcom,cs-out", &cfg->cs_out_en, },
>> +             { "qcom,ext-gen", &cfg->ext_gen, },
>> +             { "qcom,cabc", &cfg->cabc_en, },
>> +     };
>> +
>> +     rc = of_property_read_u32(dev->of_node, "reg", &val);
>> +     if (rc || val > 0xffff) {
>> +             dev_err(dev, "invalid IO resources\n");
>> +             return rc ? rc : -EINVAL;
>> +     }
>> +     wled->addr = val;
>> +
>> +     rc = of_property_read_string(dev->of_node, "label", &wled->cdev.name);
>> +     if (rc)
>> +             wled->cdev.name = dev->of_node->name;
>> +
>> +     wled->cdev.default_trigger = of_get_property(dev->of_node,
>> +                     "linux,default-trigger", NULL);
>> +
>> +     *cfg = pm8941_wled_config_defaults;
>> +     for (i = 0; i < ARRAY_SIZE(u32_opts); ++i) {
>> +             u32 sel, c;
>> +             int j, rj;
>> +
>> +             rc = of_property_read_u32(dev->of_node, u32_opts[i].name, &val);
>> +             if (rc) {
>> +                     if (rc != -EINVAL) {
>> +                             dev_err(dev, "error reading '%s'\n",
>> +                                             u32_opts[i].name);
>> +                             return rc;
>> +                     }
>> +                     continue;
>> +             }
>> +
>> +             sel = UINT_MAX;
>> +             rj = -1;
>> +             c = pm8941_wled_values(u32_opts[i].cfg, 0);
>> +             for (j = 0; c != UINT_MAX; ++j) {
>> +                     if (c <= val && (sel == UINT_MAX || c >= sel)) {
>> +                             sel = c;
>> +                             rj = j;
>> +                     }
>> +                     c = pm8941_wled_values(u32_opts[i].cfg, j + 1);
>> +             }
>> +             if (sel == UINT_MAX) {
>> +                     dev_err(dev, "invalid value for '%s'\n",
>> +                                     u32_opts[i].name);
>> +                     return rc;
>
> Isn't rc always 0 here? Don't we want to return an error?
>
> Also, I find this code very convoluted given that we loop through a
> table and match based on nodes and call function pointers, etc. Why
> can't we just have a handful of if statements with of_property_read_u32
> in them? That way we don't have to jump through so many hoops, bouncing
> all around this file to figure out what's going on. If we did I imagine
> we wouldn't have missed out on rc being 0 here.
>
>> +
>> +static int pm8941_wled_remove(struct platform_device *pdev)
>> +{
>> +     struct pm8941_wled *wled;
>> +
>> +     wled = platform_get_drvdata(pdev);
>> +     led_classdev_unregister(&wled->cdev);
>
> Would be nice to have a devm for this one too.
>
>> +
>> +     return 0;
>> +}
>> +
>> +static const struct of_device_id pm8941_wled_match_table[] = {
>> +     { .compatible = "qcom,pm8941-wled" },
>> +     {}
>> +};
>> +MODULE_DEVICE_TABLE(of, pm8941_wled_match_table);
>> +
>> +static struct platform_driver pm8941_wled_driver = {
>> +     .probe  = pm8941_wled_probe,
>> +     .remove = pm8941_wled_remove,
>> +     .driver = {
>> +             .name           = "pm8941-wled",
>> +             .owner          = THIS_MODULE,
>
> THIS_MODULE should be removed.
>
> --
> Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
> a Linux Foundation Collaborative Project
>

Re: [PATCH v2 2/2] leds: add Qualcomm PM8941 WLED driver

[Bjorn Andersson]

On Tue, Feb 17, 2015 at 2:14 PM, Bryan Wu <cooloney@gmail.com> wrote:
> On Thu, Feb 12, 2015 at 8:04 PM, Stephen Boyd <sboyd@codeaurora.org> wrote:
>> On 01/23/15 16:54, Bjorn Andersson wrote:
>
> Thanks for the review, Stephen.
> Bjorn, could you please update your patch according to Stephen's review.
>

I will do so, do you want me to send a new patch or an incremental
patch with the changes?

Regards,
Bjorn

Re: [PATCH v2 2/2] leds: add Qualcomm PM8941 WLED driver

[Bryan Wu]

On Tue, Feb 17, 2015 at 2:30 PM, Bjorn Andersson <bjorn@kryo.se> wrote:
> On Tue, Feb 17, 2015 at 2:14 PM, Bryan Wu <cooloney@gmail.com> wrote:
>> On Thu, Feb 12, 2015 at 8:04 PM, Stephen Boyd <sboyd@codeaurora.org> wrote:
>>> On 01/23/15 16:54, Bjorn Andersson wrote:
>>
>> Thanks for the review, Stephen.
>> Bjorn, could you please update your patch according to Stephen's review.
>>
>
> I will do so, do you want me to send a new patch or an incremental
> patch with the changes?
>

Please send me a new one.

-Bryan

Re: [PATCH v2 2/2] leds: add Qualcomm PM8941 WLED driver

[Bjorn Andersson]

On Thu, Feb 12, 2015 at 8:26 PM, Stephen Boyd <sboyd@codeaurora.org> wrote:
> On 01/23/15 16:54, Bjorn Andersson wrote:
>> +
>> +static int pm8941_wled_set(struct led_classdev *cdev,
>> +                        enum led_brightness value)
>> +{
>> +     struct pm8941_wled *wled;
>> +     u8 ctrl = 0;
>> +     u16 val;
>> +     int rc;
>> +     int i;
>> +
>> +     wled = container_of(cdev, struct pm8941_wled, cdev);
>> +
>> +     if (value != 0)
>> +             ctrl = PM8941_WLED_REG_MOD_EN_BIT;
>> +
>> +     val = value * PM8941_WLED_REG_VAL_MAX / LED_FULL;
>> +
>> +     rc = regmap_update_bits(wled->regmap,
>> +                     wled->addr + PM8941_WLED_REG_MOD_EN,
>> +                     PM8941_WLED_REG_MOD_EN_MASK, ctrl);
>> +     if (rc)
>> +             return rc;
>> +
>> +     for (i = 0; i < wled->cfg.num_strings; ++i) {
>> +             u8 v[2] = { val & 0xff, (val >> 8) & 0xf };
>> +
>> +             rc = regmap_bulk_write(wled->regmap,
>> +                             wled->addr + PM8941_WLED_REG_VAL_BASE + 2 * i,
>> +                             v, 2);
>> +             if (rc)
>> +                     return rc;
>> +     }
>> +
>> +     rc = regmap_update_bits(wled->regmap,
>> +                     wled->addr + PM8941_WLED_REG_SYNC,
>> +                     PM8941_WLED_REG_SYNC_MASK, PM8941_WLED_REG_SYNC_ALL);
>> +     if (rc)
>> +             return rc;
>> +
>> +     rc = regmap_update_bits(wled->regmap,
>> +                     wled->addr + PM8941_WLED_REG_SYNC,
>> +                     PM8941_WLED_REG_SYNC_MASK, PM8941_WLED_REG_SYNC_CLEAR);
>> +     return rc;
>> +}
>
> This doesn't seem to do anything for the OVP spike mentioned in this
> patch[1]. Do you see that problem on your device? I imagine the PMIC is
> the same.
>
> [1]
> https://www.codeaurora.org/cgit/quic/la/kernel/msm/commit/?id=fef9e15072562f0f28dc7066dcdd69388df81ed3
>

That's odd, I can't find that fix in any officially supported releases
for 8974 - which Courtney used as reference for this driver.

Just to make sure I understand the solution; when disabling the sinks
the over-voltage-protection sometimes triggers, so we should detect
that and reset the ovp configuration?

I presume the side effect would be that the sinks would not give any
output until this is done?

Regards,
Bjorn

Re: [PATCH v2 2/2] leds: add Qualcomm PM8941 WLED driver

[Bjorn Andersson]

On Thu, Feb 12, 2015 at 8:07 PM, Stephen Boyd <sboyd@codeaurora.org> wrote:
> On 01/29/15 04:48, Ivan T. Ivanov wrote:
>>
>> Otherwise it looks good. Driver is loaded and device is detected
>> properly (i have added readings for type and subtype registers).
>> Do you know where I can measure result from changing brightness
>> sysfs entry. I am using 8074 dragonboard?
>
> Does the backlight turn on? From what I can tell it controls the
> backlight, but it may be that nothings getting displayed so it won't be
> noticeable.
>

I have not measured the voltage/current, nor have had something on the
display. But on the various devices I've tested (Xperia Z1 & Xperia Z
Ultra) the backlight is clearly on.

Regards,
Bjorn

Re: [PATCH v2 2/2] leds: add Qualcomm PM8941 WLED driver

[Bjorn Andersson]

On Thu, Feb 12, 2015 at 8:04 PM, Stephen Boyd <sboyd@codeaurora.org> wrote:
[..]
>> +
>> +static int pm8941_wled_remove(struct platform_device *pdev)
>> +{
>> +     struct pm8941_wled *wled;
>> +
>> +     wled = platform_get_drvdata(pdev);
>> +     led_classdev_unregister(&wled->cdev);
>
> Would be nice to have a devm for this one too.
>

I agree, will hack one up.

>> +
>> +     return 0;
>> +}
>> +
>> +static const struct of_device_id pm8941_wled_match_table[] = {
>> +     { .compatible = "qcom,pm8941-wled" },
>> +     {}
>> +};
>> +MODULE_DEVICE_TABLE(of, pm8941_wled_match_table);
>> +
>> +static struct platform_driver pm8941_wled_driver = {
>> +     .probe  = pm8941_wled_probe,
>> +     .remove = pm8941_wled_remove,
>> +     .driver = {
>> +             .name           = "pm8941-wled",
>> +             .owner          = THIS_MODULE,
>
> THIS_MODULE should be removed.
>

Right, we've had this patch in purgatory for too long time... Will update.

Regards,
Bjorn

Re: [PATCH v2 2/2] leds: add Qualcomm PM8941 WLED driver

[Stephen Boyd]

On 02/17/15 15:02, Bjorn Andersson wrote:
> On Thu, Feb 12, 2015 at 8:26 PM, Stephen Boyd <sboyd@codeaurora.org> wrote:
>> On 01/23/15 16:54, Bjorn Andersson wrote:
>>> +
>>> +static int pm8941_wled_set(struct led_classdev *cdev,
>>> +                        enum led_brightness value)
>>> +{
>>> +     struct pm8941_wled *wled;
>>> +     u8 ctrl = 0;
>>> +     u16 val;
>>> +     int rc;
>>> +     int i;
>>> +
>>> +     wled = container_of(cdev, struct pm8941_wled, cdev);
>>> +
>>> +     if (value != 0)
>>> +             ctrl = PM8941_WLED_REG_MOD_EN_BIT;
>>> +
>>> +     val = value * PM8941_WLED_REG_VAL_MAX / LED_FULL;
>>> +
>>> +     rc = regmap_update_bits(wled->regmap,
>>> +                     wled->addr + PM8941_WLED_REG_MOD_EN,
>>> +                     PM8941_WLED_REG_MOD_EN_MASK, ctrl);
>>> +     if (rc)
>>> +             return rc;
>>> +
>>> +     for (i = 0; i < wled->cfg.num_strings; ++i) {
>>> +             u8 v[2] = { val & 0xff, (val >> 8) & 0xf };
>>> +
>>> +             rc = regmap_bulk_write(wled->regmap,
>>> +                             wled->addr + PM8941_WLED_REG_VAL_BASE + 2 * i,
>>> +                             v, 2);
>>> +             if (rc)
>>> +                     return rc;
>>> +     }
>>> +
>>> +     rc = regmap_update_bits(wled->regmap,
>>> +                     wled->addr + PM8941_WLED_REG_SYNC,
>>> +                     PM8941_WLED_REG_SYNC_MASK, PM8941_WLED_REG_SYNC_ALL);
>>> +     if (rc)
>>> +             return rc;
>>> +
>>> +     rc = regmap_update_bits(wled->regmap,
>>> +                     wled->addr + PM8941_WLED_REG_SYNC,
>>> +                     PM8941_WLED_REG_SYNC_MASK, PM8941_WLED_REG_SYNC_CLEAR);
>>> +     return rc;
>>> +}
>> This doesn't seem to do anything for the OVP spike mentioned in this
>> patch[1]. Do you see that problem on your device? I imagine the PMIC is
>> the same.
>>
>> [1]
>> https://www.codeaurora.org/cgit/quic/la/kernel/msm/commit/?id=fef9e15072562f0f28dc7066dcdd69388df81ed3
>>
> That's odd, I can't find that fix in any officially supported releases
> for 8974 - which Courtney used as reference for this driver.
>
> Just to make sure I understand the solution; when disabling the sinks
> the over-voltage-protection sometimes triggers, so we should detect
> that and reset the ovp configuration?
>
> I presume the side effect would be that the sinks would not give any
> output until this is done?
>

It seems that commit is not very good at describing the problem. From
what I can tell, the inductor current may spike when the WLED is turned
off and the switch is stuck on (the circuit is a boost convertor). To
make sure this doesn't happen, we force an OVP so that the switch is
known to be open (i.e. off) and thus can't cause the current spike when
we disable.

-- 
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
a Linux Foundation Collaborative Project

Re: [PATCH v2 2/2] leds: add Qualcomm PM8941 WLED driver

[Bjorn Andersson]

On Tue, Feb 17, 2015 at 6:45 PM, Stephen Boyd <sboyd@codeaurora.org> wrote:
> On 02/17/15 15:02, Bjorn Andersson wrote:
>> On Thu, Feb 12, 2015 at 8:26 PM, Stephen Boyd <sboyd@codeaurora.org> wrote:
[..]
>>> This doesn't seem to do anything for the OVP spike mentioned in this
>>> patch[1]. Do you see that problem on your device? I imagine the PMIC is
>>> the same.
>>>
>>> [1]
>>> https://www.codeaurora.org/cgit/quic/la/kernel/msm/commit/?id=fef9e15072562f0f28dc7066dcdd69388df81ed3
>>>
>> That's odd, I can't find that fix in any officially supported releases
>> for 8974 - which Courtney used as reference for this driver.
>>
>> Just to make sure I understand the solution; when disabling the sinks
>> the over-voltage-protection sometimes triggers, so we should detect
>> that and reset the ovp configuration?
>>
>> I presume the side effect would be that the sinks would not give any
>> output until this is done?
>>
>
> It seems that commit is not very good at describing the problem. From
> what I can tell, the inductor current may spike when the WLED is turned
> off and the switch is stuck on (the circuit is a boost convertor). To
> make sure this doesn't happen, we force an OVP so that the switch is
> known to be open (i.e. off) and thus can't cause the current spike when
> we disable.
>

Apparently that commit is only half of the solution, there's another
commit earlier in history with the same commit message [2].

With both [1] and [2] in place there are a bunch of different
operations going on, so I'm not able to fully map these to your
description - which does make sense.

[2] https://www.codeaurora.org/cgit/quic/la/kernel/msm/commit/?id=b1a2f61113c69d2aba68c583101ac40a05cb2c5b

Regards,
Bjorn