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

[PATCH 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.8.2.2

[PATCH 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 | 466 ++++++++++++++++++++++++++++++++++++++++
 3 files changed, 475 insertions(+)
 create mode 100644 drivers/leds/leds-pm8941-wled.c

diff --git a/drivers/leds/Kconfig b/drivers/leds/Kconfig
index a210338..9091cf4 100644
--- a/drivers/leds/Kconfig
+++ b/drivers/leds/Kconfig
@@ -505,6 +505,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
+	depends on 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 a2b1647..5474afc 100644
--- a/drivers/leds/Makefile
+++ b/drivers/leds/Makefile
@@ -56,6 +56,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..aa06d0b
--- /dev/null
+++ b/drivers/leds/leds-pm8941-wled.c
@@ -0,0 +1,466 @@
+/* Copyright (c) 2013, 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/leds.h>
+#include <linux/module.h>
+#include <linux/of.h>
+#include <linux/of_device.h>
+#include <linux/regmap.h>
+
+enum pm8941_wled_reg {
+	PM8941_WLED_REG_VAL_BASE		= 0x40,
+#define PM8941_WLED_REG_VAL_MAX			0xFFF
+
+	PM8941_WLED_REG_MOD_EN			= 0x46,
+#define PM8941_WLED_REG_MOD_EN_BIT		BIT(7)
+#define PM8941_WLED_REG_MOD_EN_MASK		BIT(7)
+
+	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
+
+	PM8941_WLED_REG_FREQ			= 0x4c,
+#define PM8941_WLED_REG_FREQ_MASK		0x0f
+
+	PM8941_WLED_REG_OVP			= 0x4d,
+#define PM8941_WLED_REG_OVP_MASK		0x03
+
+	PM8941_WLED_REG_BOOST			= 0x4e,
+#define PM8941_WLED_REG_BOOST_MASK		0x07
+
+	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
+
+	PM8941_WLED_REG_STR_MOD_EN_BASE		= 0x60,
+#define PM8941_WLED_REG_STR_MOD_EN		BIT(7)
+#define PM8941_WLED_REG_STR_MOD_MASK		BIT(7)
+
+	PM8941_WLED_REG_STR_SCALE_BASE		= 0x62,
+#define PM8941_WLED_REG_STR_SCALE_MASK		0x1f
+
+	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
+
+	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 led_classdev cdev;
+	struct pm8941_wled_config cfg;
+	u16 addr;
+};
+
+struct pm8941_wled_context {
+	struct pm8941_wled wled;
+	struct regmap *regmap;
+};
+
+static int pm8941_wled_set(struct led_classdev *cdev,
+			   enum led_brightness value)
+{
+	struct pm8941_wled_context *ctx;
+	struct pm8941_wled *wled;
+	u8 ctrl = 0;
+	u16 val;
+	int rc;
+	int i;
+
+	wled = container_of(cdev, struct pm8941_wled, cdev);
+	ctx = container_of(wled, struct pm8941_wled_context, wled);
+
+	if (value != 0)
+		ctrl = PM8941_WLED_REG_MOD_EN_BIT;
+
+	val = (value * PM8941_WLED_REG_VAL_MAX) / LED_FULL;
+
+	rc = regmap_update_bits(ctx->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(ctx->regmap,
+				wled->addr + PM8941_WLED_REG_VAL_BASE + 2 * i,
+				v, 2);
+		if (rc)
+			return rc;
+	}
+
+	rc = regmap_update_bits(ctx->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(ctx->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)
+{
+	struct pm8941_wled_context *ctx;
+	int rc;
+	int i;
+
+	ctx = container_of(wled, struct pm8941_wled_context, wled);
+	rc = regmap_update_bits(ctx->regmap,
+			wled->addr + PM8941_WLED_REG_OVP,
+			PM8941_WLED_REG_OVP_MASK, wled->cfg.ovp);
+	if (rc)
+		return rc;
+
+	rc = regmap_update_bits(ctx->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(ctx->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(ctx->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(ctx->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(ctx->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(ctx->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(ctx->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, },
+	};
+
+	if (dev->of_node == NULL) {
+		dev_err(dev, "no OF node!\n");
+		return -EINVAL;
+	}
+
+	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_context *ctx;
+	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;
+	}
+
+	ctx = devm_kzalloc(&pdev->dev, sizeof(*ctx), GFP_KERNEL);
+	if (ctx == NULL)
+		return -ENOMEM;
+
+	ctx->regmap = regmap;
+
+	rc = pm8941_wled_configure(&ctx->wled, &pdev->dev);
+	if (rc)
+		return rc;
+
+	rc = pm8941_wled_setup(&ctx->wled);
+	if (rc)
+		return rc;
+
+	ctx->wled.cdev.brightness_set = pm8941_wled_set_brightness;
+
+	rc = led_classdev_register(&pdev->dev, &ctx->wled.cdev);
+	if (rc)
+		return rc;
+
+	platform_set_drvdata(pdev, ctx);
+	dev_info(&pdev->dev, "registered led \"%s\"\n", ctx->wled.cdev.name);
+
+	return 0;
+};
+
+static int pm8941_wled_remove(struct platform_device *pdev)
+{
+	struct pm8941_wled_context *ctx;
+
+	ctx = platform_get_drvdata(pdev);
+	led_classdev_unregister(&ctx->wled.cdev);
+
+	return 0;
+}
+
+static const struct of_device_id pm8941_wled_match_table[] = {
+	{ .compatible = "qcom,pm8941-wled" },
+	{}
+};
+
+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.8.2.2

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

[Bryan Wu]

On Mon, Dec 8, 2014 at 4:22 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.
>

Looks good to me, I need device tree reviewer's Ack to merge it.

Thanks,
-Bryan

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

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

[Bryan Wu]

On Mon, Dec 8, 2014 at 4:22 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>
> ---
>  drivers/leds/Kconfig            |   8 +
>  drivers/leds/Makefile           |   1 +
>  drivers/leds/leds-pm8941-wled.c | 466 ++++++++++++++++++++++++++++++++++++++++
>  3 files changed, 475 insertions(+)
>  create mode 100644 drivers/leds/leds-pm8941-wled.c
>
> diff --git a/drivers/leds/Kconfig b/drivers/leds/Kconfig
> index a210338..9091cf4 100644
> --- a/drivers/leds/Kconfig
> +++ b/drivers/leds/Kconfig
> @@ -505,6 +505,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
> +       depends on REGMAP
Here should be "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 a2b1647..5474afc 100644
> --- a/drivers/leds/Makefile
> +++ b/drivers/leds/Makefile
> @@ -56,6 +56,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..aa06d0b
> --- /dev/null
> +++ b/drivers/leds/leds-pm8941-wled.c
> @@ -0,0 +1,466 @@
> +/* Copyright (c) 2013, 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/leds.h>
> +#include <linux/module.h>
> +#include <linux/of.h>
> +#include <linux/of_device.h>
> +#include <linux/regmap.h>
> +
> +enum pm8941_wled_reg {
> +       PM8941_WLED_REG_VAL_BASE                = 0x40,
> +#define PM8941_WLED_REG_VAL_MAX                        0xFFF
> +
> +       PM8941_WLED_REG_MOD_EN                  = 0x46,
> +#define PM8941_WLED_REG_MOD_EN_BIT             BIT(7)
> +#define PM8941_WLED_REG_MOD_EN_MASK            BIT(7)
> +
> +       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
> +
> +       PM8941_WLED_REG_FREQ                    = 0x4c,
> +#define PM8941_WLED_REG_FREQ_MASK              0x0f
> +
> +       PM8941_WLED_REG_OVP                     = 0x4d,
> +#define PM8941_WLED_REG_OVP_MASK               0x03
> +
> +       PM8941_WLED_REG_BOOST                   = 0x4e,
> +#define PM8941_WLED_REG_BOOST_MASK             0x07
> +
> +       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
> +
> +       PM8941_WLED_REG_STR_MOD_EN_BASE         = 0x60,
> +#define PM8941_WLED_REG_STR_MOD_EN             BIT(7)
> +#define PM8941_WLED_REG_STR_MOD_MASK           BIT(7)
> +
> +       PM8941_WLED_REG_STR_SCALE_BASE          = 0x62,
> +#define PM8941_WLED_REG_STR_SCALE_MASK         0x1f
> +
> +       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
> +
> +       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 led_classdev cdev;
> +       struct pm8941_wled_config cfg;
> +       u16 addr;
> +};
> +
> +struct pm8941_wled_context {
> +       struct pm8941_wled wled;
> +       struct regmap *regmap;
> +};
> +

I think you can just add *regmap into pm8941_wled struct and
pm8941_wled_context is redundant to me.

> +static int pm8941_wled_set(struct led_classdev *cdev,
> +                          enum led_brightness value)
> +{
> +       struct pm8941_wled_context *ctx;
> +       struct pm8941_wled *wled;
> +       u8 ctrl = 0;
> +       u16 val;
> +       int rc;
> +       int i;
> +
> +       wled = container_of(cdev, struct pm8941_wled, cdev);
> +       ctx = container_of(wled, struct pm8941_wled_context, wled);
> +
> +       if (value != 0)
> +               ctrl = PM8941_WLED_REG_MOD_EN_BIT;
> +
> +       val = (value * PM8941_WLED_REG_VAL_MAX) / LED_FULL;
> +
> +       rc = regmap_update_bits(ctx->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(ctx->regmap,
> +                               wled->addr + PM8941_WLED_REG_VAL_BASE + 2 * i,
> +                               v, 2);
> +               if (rc)
> +                       return rc;
> +       }
> +
> +       rc = regmap_update_bits(ctx->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(ctx->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)
> +{
> +       struct pm8941_wled_context *ctx;
> +       int rc;
> +       int i;
> +
> +       ctx = container_of(wled, struct pm8941_wled_context, wled);
> +       rc = regmap_update_bits(ctx->regmap,
> +                       wled->addr + PM8941_WLED_REG_OVP,
> +                       PM8941_WLED_REG_OVP_MASK, wled->cfg.ovp);
> +       if (rc)
> +               return rc;
> +
> +       rc = regmap_update_bits(ctx->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(ctx->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(ctx->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(ctx->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(ctx->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(ctx->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(ctx->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, },
> +       };
> +
> +       if (dev->of_node == NULL) {
> +               dev_err(dev, "no OF node!\n");
> +               return -EINVAL;
> +       }
> +
> +       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_context *ctx;
> +       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;
> +       }
> +

Is this regmap mmio registers or i2c registers? Will the regmap
operation sleep?

> +       ctx = devm_kzalloc(&pdev->dev, sizeof(*ctx), GFP_KERNEL);
> +       if (ctx == NULL)
> +               return -ENOMEM;
> +
> +       ctx->regmap = regmap;
> +
> +       rc = pm8941_wled_configure(&ctx->wled, &pdev->dev);
> +       if (rc)
> +               return rc;
> +
> +       rc = pm8941_wled_setup(&ctx->wled);
> +       if (rc)
> +               return rc;
> +
> +       ctx->wled.cdev.brightness_set = pm8941_wled_set_brightness;
> +
> +       rc = led_classdev_register(&pdev->dev, &ctx->wled.cdev);
> +       if (rc)
> +               return rc;
> +
> +       platform_set_drvdata(pdev, ctx);
> +       dev_info(&pdev->dev, "registered led \"%s\"\n", ctx->wled.cdev.name);
> +
> +       return 0;
> +};
> +
> +static int pm8941_wled_remove(struct platform_device *pdev)
> +{
> +       struct pm8941_wled_context *ctx;
> +
> +       ctx = platform_get_drvdata(pdev);
> +       led_classdev_unregister(&ctx->wled.cdev);
> +
> +       return 0;
> +}
> +
> +static const struct of_device_id pm8941_wled_match_table[] = {
> +       { .compatible = "qcom,pm8941-wled" },
> +       {}
> +};
> +
> +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");
No space after ":"

> --
> 1.8.2.2
>

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

[Bryan Wu]

On Tue, Dec 9, 2014 at 5:19 PM, Bryan Wu <cooloney@gmail.com> wrote:
> On Mon, Dec 8, 2014 at 4:22 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.
>>
>
> Looks good to me, I need device tree reviewer's Ack to merge it.
>

Hi Rob, can you help to review it?

-Bryan

> Thanks,
> -Bryan
>
>> 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.8.2.2
>>

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

[Stephen Boyd]

On 12/08/2014 04:22 PM, Bjorn Andersson wrote:
> diff --git a/drivers/leds/leds-pm8941-wled.c b/drivers/leds/leds-pm8941-wled.c
> new file mode 100644
> index 0000000..aa06d0b
> --- /dev/null
> +++ b/drivers/leds/leds-pm8941-wled.c
> @@ -0,0 +1,466 @@
> +/* Copyright (c) 2013, 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/leds.h>
> +#include <linux/module.h>
> +#include <linux/of.h>
> +#include <linux/of_device.h>
> +#include <linux/regmap.h>

Missing <linux/kernel.h> for container_of() macro.

> +
> +enum pm8941_wled_reg {
> +	PM8941_WLED_REG_VAL_BASE		= 0x40,
> +#define PM8941_WLED_REG_VAL_MAX			0xFFF
> +
> +	PM8941_WLED_REG_MOD_EN			= 0x46,
> +#define PM8941_WLED_REG_MOD_EN_BIT		BIT(7)
> +#define PM8941_WLED_REG_MOD_EN_MASK		BIT(7)
> +
> +	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
> +
> +	PM8941_WLED_REG_FREQ			= 0x4c,
> +#define PM8941_WLED_REG_FREQ_MASK		0x0f
> +
> +	PM8941_WLED_REG_OVP			= 0x4d,
> +#define PM8941_WLED_REG_OVP_MASK		0x03
> +
> +	PM8941_WLED_REG_BOOST			= 0x4e,
> +#define PM8941_WLED_REG_BOOST_MASK		0x07
> +
> +	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
> +
> +	PM8941_WLED_REG_STR_MOD_EN_BASE		= 0x60,
> +#define PM8941_WLED_REG_STR_MOD_EN		BIT(7)
> +#define PM8941_WLED_REG_STR_MOD_MASK		BIT(7)
> +
> +	PM8941_WLED_REG_STR_SCALE_BASE		= 0x62,
> +#define PM8941_WLED_REG_STR_SCALE_MASK		0x1f
> +
> +	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
> +
> +	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)
> +};

I don't see any value in this enum. It's never used in a switch
statement so why not just have #defines for the register offsets. It
would make reading this a lot easier too.


> +
> +static int pm8941_wled_set(struct led_classdev *cdev,
> +			   enum led_brightness value)
> +{
> +	struct pm8941_wled_context *ctx;
> +	struct pm8941_wled *wled;
> +	u8 ctrl = 0;
> +	u16 val;
> +	int rc;
> +	int i;
> +
> +	wled = container_of(cdev, struct pm8941_wled, cdev);
> +	ctx = container_of(wled, struct pm8941_wled_context, wled);
> +
> +	if (value != 0)
> +		ctrl = PM8941_WLED_REG_MOD_EN_BIT;
> +
> +	val = (value * PM8941_WLED_REG_VAL_MAX) / LED_FULL;

Unnecessary parens here.

> +
> +	rc = regmap_update_bits(ctx->regmap,
>
> +
> +static int pm8941_wled_configure(struct pm8941_wled *wled, struct device *dev)
> +{
[...]
> +
> +	if (dev->of_node == NULL) {
> +		dev_err(dev, "no OF node!\n");
> +		return -EINVAL;
> +	}

Seems like an unnecessary check. Everything's DT so far.

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

Do we really want to do all this complicated rounding and checking?
Can't we just assume the DT is correct?

> +		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_context *ctx;
> +	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;
> +	}
> +
> +	ctx = devm_kzalloc(&pdev->dev, sizeof(*ctx), GFP_KERNEL);
> +	if (ctx == NULL)

Nitpick only because it doesn't match the !regmap style above. Please
use the !ctx style here as well.

> +		return -ENOMEM;
> +
> +	ctx->regmap = regmap;
> +
> +	rc = pm8941_wled_configure(&ctx->wled, &pdev->dev);
> +	if (rc)
> +		return rc;
> +
> +	rc = pm8941_wled_setup(&ctx->wled);
> +	if (rc)
> +		return rc;
> +
> +	ctx->wled.cdev.brightness_set = pm8941_wled_set_brightness;
> +
> +	rc = led_classdev_register(&pdev->dev, &ctx->wled.cdev);
> +	if (rc)
> +		return rc;
> +
> +	platform_set_drvdata(pdev, ctx);
> +	dev_info(&pdev->dev, "registered led \"%s\"\n", ctx->wled.cdev.name);

Please don't spam the log with "I'm alive!" messages.

> +
> +	return 0;
> +};
> +
> +static int pm8941_wled_remove(struct platform_device *pdev)
> +{
> +	struct pm8941_wled_context *ctx;
> +
> +	ctx = platform_get_drvdata(pdev);
> +	led_classdev_unregister(&ctx->wled.cdev);
> +
> +	return 0;
> +}
> +
> +static const struct of_device_id pm8941_wled_match_table[] = {
> +	{ .compatible = "qcom,pm8941-wled" },
> +	{}
> +};

MODULE_DEVICE_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");

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

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

[Stephen Boyd]

On 12/08/2014 04:22 PM, Bjorn Andersson wrote:

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

It would be nice if the units were part of the property name, i.e.
qcom,ovp-V, qcom,current-boost-limit-mA, etc. That way the units are
known without having to refer to the documentation.

> +- qcom,num-strings: #; number of led strings attached; value from 1 to 3
> +	default: 2

I wonder if qcom,#strings is more appropriate?

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


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

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

[Bjorn]

On Tue 16 Dec 17:15 PST 2014, Stephen Boyd wrote:

> On 12/08/2014 04:22 PM, Bjorn Andersson wrote:
> 
[..]
> > +- 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,ovp: V; Over-voltage protection limit; one of:
> > +	27, 29, 32, 35
> > +	default: 29V
> 
> It would be nice if the units were part of the property name, i.e.
> qcom,ovp-V, qcom,current-boost-limit-mA, etc. That way the units are
> known without having to refer to the documentation.
> 

That might be useful, on the other hand one probably need to look inte bindings
docs to figure out valid values anyways.

> > +- qcom,num-strings: #; number of led strings attached; value from 1 to 3
> > +	default: 2
> 
> I wonder if qcom,#strings is more appropriate?
> 

Checking the other docs indicate that it's far more common to use num- than #
here, but I haven't found anything written about it - it's more common to name
this just "qcom,strings" than "qcom,#strings" at least...

Regards,
Bjorn

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

[Bjorn]

On Tue 16 Dec 16:54 PST 2014, Bryan Wu wrote:

> On Mon, Dec 8, 2014 at 4:22 PM, Bjorn Andersson

[..]

> > diff --git a/drivers/leds/Kconfig b/drivers/leds/Kconfig

[..]

> > +config LEDS_PM8941_WLED
> > +       tristate "LED support for the Qualcomm PM8941 WLED block"
> > +       depends on LEDS_CLASS
> > +       depends on REGMAP
> Here should be "select REGMAP"
> 

Thanks
> > +       help
> > +         This option enables support for the 'White' LED block
> > +         on Qualcomm PM8941 PMICs.
> > +

[..]

> > diff --git a/drivers/leds/leds-pm8941-wled.c b/drivers/leds/leds-pm8941-wled.c

[..]

> > +
> > +struct pm8941_wled_context {
> > +       struct pm8941_wled wled;
> > +       struct regmap *regmap;
> > +};
> > +
> 
> I think you can just add *regmap into pm8941_wled struct and
> pm8941_wled_context is redundant to me.
> 

Looks like a development leftover, I will fold it in.

[..]

> > +
> > +static int pm8941_wled_probe(struct platform_device *pdev)
> > +{
> > +       struct pm8941_wled_context *ctx;
> > +       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;
> > +       }
> > +
> 
> Is this regmap mmio registers or i2c registers? Will the regmap
> operation sleep?
> 

It's a spmi regmap, in it's current configuration it's marked as "fast" i.e. it
will not sleep.

I discussed this with Courtney before sending the patch out and as this is used
for backlight we don't see a reason calling set_brightness() in atomic context.
If this changes I would send out a follow up patch.

> > +       ctx = devm_kzalloc(&pdev->dev, sizeof(*ctx), GFP_KERNEL);
> > +       if (ctx == NULL)
> > +               return -ENOMEM;
> > +
> > +       ctx->regmap = regmap;
> > +
> > +       rc = pm8941_wled_configure(&ctx->wled, &pdev->dev);
> > +       if (rc)
> > +               return rc;
> > +
> > +       rc = pm8941_wled_setup(&ctx->wled);
> > +       if (rc)
> > +               return rc;
> > +
> > +       ctx->wled.cdev.brightness_set = pm8941_wled_set_brightness;
> > +
> > +       rc = led_classdev_register(&pdev->dev, &ctx->wled.cdev);
> > +       if (rc)
> > +               return rc;
> > +
> > +       platform_set_drvdata(pdev, ctx);
> > +       dev_info(&pdev->dev, "registered led \"%s\"\n", ctx->wled.cdev.name);
> > +
> > +       return 0;
> > +};
> > +

[..]

> > +MODULE_DESCRIPTION("pm8941 wled driver");
> > +MODULE_LICENSE("GPL v2");
> > +MODULE_ALIAS("platform: pm8941-wled");
> No space after ":"
> 

Oops.

Regards,
Bjorn

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

[Bjorn]

On Tue 16 Dec 17:15 PST 2014, Stephen Boyd wrote:

> On 12/08/2014 04:22 PM, Bjorn Andersson wrote:
> > diff --git a/drivers/leds/leds-pm8941-wled.c b/drivers/leds/leds-pm8941-wled.c

[..]

> > +#include <linux/leds.h>
> > +#include <linux/module.h>
> > +#include <linux/of.h>
> > +#include <linux/of_device.h>
> > +#include <linux/regmap.h>
> 
> Missing <linux/kernel.h> for container_of() macro.
> 

Thanks

> > +
> > +enum pm8941_wled_reg {
> > +	PM8941_WLED_REG_VAL_BASE		= 0x40,
> > +#define PM8941_WLED_REG_VAL_MAX			0xFFF
> > +
> > +	PM8941_WLED_REG_MOD_EN			= 0x46,
> > +#define PM8941_WLED_REG_MOD_EN_BIT		BIT(7)
> > +#define PM8941_WLED_REG_MOD_EN_MASK		BIT(7)
> > +
> > +	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
> > +
> > +	PM8941_WLED_REG_FREQ			= 0x4c,
> > +#define PM8941_WLED_REG_FREQ_MASK		0x0f
> > +
> > +	PM8941_WLED_REG_OVP			= 0x4d,
> > +#define PM8941_WLED_REG_OVP_MASK		0x03
> > +
> > +	PM8941_WLED_REG_BOOST			= 0x4e,
> > +#define PM8941_WLED_REG_BOOST_MASK		0x07
> > +
> > +	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
> > +
> > +	PM8941_WLED_REG_STR_MOD_EN_BASE		= 0x60,
> > +#define PM8941_WLED_REG_STR_MOD_EN		BIT(7)
> > +#define PM8941_WLED_REG_STR_MOD_MASK		BIT(7)
> > +
> > +	PM8941_WLED_REG_STR_SCALE_BASE		= 0x62,
> > +#define PM8941_WLED_REG_STR_SCALE_MASK		0x1f
> > +
> > +	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
> > +
> > +	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)
> > +};
> 
> I don't see any value in this enum. It's never used in a switch
> statement so why not just have #defines for the register offsets. It
> would make reading this a lot easier too.
> 

Make sense, I'll rework it.

> > +
> > +static int pm8941_wled_set(struct led_classdev *cdev,
> > +			   enum led_brightness value)
> > +{
> > +	struct pm8941_wled_context *ctx;
> > +	struct pm8941_wled *wled;
> > +	u8 ctrl = 0;
> > +	u16 val;
> > +	int rc;
> > +	int i;
> > +
> > +	wled = container_of(cdev, struct pm8941_wled, cdev);
> > +	ctx = container_of(wled, struct pm8941_wled_context, wled);
> > +
> > +	if (value != 0)
> > +		ctrl = PM8941_WLED_REG_MOD_EN_BIT;
> > +
> > +	val = (value * PM8941_WLED_REG_VAL_MAX) / LED_FULL;
> 
> Unnecessary parens here.
> 

Agreed

> > +
> > +	rc = regmap_update_bits(ctx->regmap,
> >
> > +
> > +static int pm8941_wled_configure(struct pm8941_wled *wled, struct device *dev)
> > +{
> [...]
> > +
> > +	if (dev->of_node == NULL) {
> > +		dev_err(dev, "no OF node!\n");
> > +		return -EINVAL;
> > +	}
> 
> Seems like an unnecessary check. Everything's DT so far.
> 

Agreed.

> > +
> > +	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);
> > +		}
> 
> Do we really want to do all this complicated rounding and checking?
> Can't we just assume the DT is correct?
> 

We could remove the printout about rounding, but the logic turns human-readable
values into register values (indices), so we need something for that.

> > +		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_context *ctx;
> > +	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;
> > +	}
> > +
> > +	ctx = devm_kzalloc(&pdev->dev, sizeof(*ctx), GFP_KERNEL);
> > +	if (ctx == NULL)
> 
> Nitpick only because it doesn't match the !regmap style above. Please
> use the !ctx style here as well.
> 

Agreed

> > +		return -ENOMEM;
> > +
> > +	ctx->regmap = regmap;
> > +
> > +	rc = pm8941_wled_configure(&ctx->wled, &pdev->dev);
> > +	if (rc)
> > +		return rc;
> > +
> > +	rc = pm8941_wled_setup(&ctx->wled);
> > +	if (rc)
> > +		return rc;
> > +
> > +	ctx->wled.cdev.brightness_set = pm8941_wled_set_brightness;
> > +
> > +	rc = led_classdev_register(&pdev->dev, &ctx->wled.cdev);
> > +	if (rc)
> > +		return rc;
> > +
> > +	platform_set_drvdata(pdev, ctx);
> > +	dev_info(&pdev->dev, "registered led \"%s\"\n", ctx->wled.cdev.name);
> 
> Please don't spam the log with "I'm alive!" messages.
> 

Ok

> > +
> > +	return 0;
> > +};
> > +

[..]

> > +
> > +static const struct of_device_id pm8941_wled_match_table[] = {
> > +	{ .compatible = "qcom,pm8941-wled" },
> > +	{}
> > +};
> 
> MODULE_DEVICE_TABLE?
> 

Indeed

Thanks for the review Stephen.

Regards,
Bjorn

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

[Ivan T. Ivanov]

On Thu, 2014-12-18 at 15:43 -0800, Bjorn wrote:
> On Tue 16 Dec 16:54 PST 2014, Bryan Wu wrote:
> 
> > On Mon, Dec 8, 2014 at 4:22 PM, Bjorn Andersson
> 
> [..]
> 
> > > diff --git a/drivers/leds/Kconfig b/drivers/leds/Kconfig
> 
> [..]
> 
> > > +config LEDS_PM8941_WLED
> > > +       tristate "LED support for the Qualcomm PM8941 WLED block"
> > > +       depends on LEDS_CLASS
> > > +       depends on REGMAP
> > Here should be "select REGMAP"
> > 
> 
> Thanks
> > > 


If not mistaken the correct way was:

        depends on SPMI
        select REGMAP_SPMI

or
        depends on MFD_SPMI_PMIC

Regards,
Ivan

https://patchwork.kernel.org/patch/4566591/