[PATCH v5 0/4] leds: add leds-mt6323 support on MT7623 SoC

[PATCH v5 0/4] leds: add leds-mt6323 support on MT7623 SoC

[sean.wang]

From: Sean Wang <sean.wang@mediatek.com>

MT7623 SoC uses MT6323 PMIC as the default power supply
which has LED function insides. The patchset introduces
the LED support for MT6323 with on, off and hardware
dimmed and blinked and it should work on other similar
SoCs if also using MT6323.

Changes since v1:
- fixed typo in the comments
- sorted include directives alphabetically
- applied all register definitions with MT6323 prefix
- removed the redundant structure declaration
- fixed coding style defined in kernel doc format consistently
- added error handling into all the occurrences where regmap APIs
  are used
- removed loudly debug message
- made magic constant into meaningful macro
- added missing mutex_destroy when module removed called
- updated module license with GPL
- fixed sparse warnings

Changes since v2:
- fixed the typo in devicetree Documentation
- cleaned up the style and explanation in the comments
- applied all macro/struct/function definitions with MT6323 prefix
- put the logic about the setup from device tree into function for simplicity

Changes since v3:
- changed the variable current_brightness into the proper type
- removed the redundant member led_num from mt6323_leds structure
- collected the brightness setting into mt6323_led_hw_brightness() call
- added brightness_get() support into the core layer
- fixed zero duty cycle handling due to hardware doesn't support
- fixed duty cycle calculation mapping to the hardware register
- added the brightness as the input in mt6323_led_hw_on() call
- enhanced error handling in mt6323_led_set_dt_default() call
- fixed potential issue which caused accessing unallocated memory if excessive
  the led controller nodes are defined
- fixed redundant memory allocation in mt6323_leds structure
- added reg property for defining the led controller for which the child
  node is predestined

Changes since v4:
- enhanced the example in leds-mt6323.txt for patch 1/4 with adding the missing
  #address-cell and #size-cell, correcting sub-node naming with led@0 style,
  removing the unrelated setup for LED and moving the description for the node
  properties into the right place
- enhanced the documentation in mt6397.txt for patch 2/4
- changed the subject prefixes into "dt-bindings: ..." for the device tree commits
- fixed that missing of_node_put() in the error path called from mt6323_led_probe()
- fixed that incorrect duty_cycle calculation mapping into the hardware register in
case duty ratio is lower than 1/64 rather than 0

Sean Wang (4):
  dt-bindings: leds: Add document bindings for leds-mt6323
  dt-bindings: mfd: Add LED subnode binding for MT6323 PMIC
  leds: Add LED support for MT6323 PMIC
  mfd: mt6397: Add MT6323 LED support into MT6397 driver

 .../devicetree/bindings/leds/leds-mt6323.txt       |  63 +++
 Documentation/devicetree/bindings/mfd/mt6397.txt   |   5 +
 drivers/leds/Kconfig                               |   8 +
 drivers/leds/Makefile                              |   1 +
 drivers/leds/leds-mt6323.c                         | 502 +++++++++++++++++++++
 drivers/mfd/mt6397-core.c                          |   4 +
 6 files changed, 583 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/leds/leds-mt6323.txt
 create mode 100644 drivers/leds/leds-mt6323.c

-- 
1.9.1

[PATCH v5 1/4] dt-bindings: leds: Add document bindings for leds-mt6323

[sean.wang]

From: Sean Wang <sean.wang@mediatek.com>

This patch adds documentation for devicetree bindings
for LED support on MT6323 PMIC

Signed-off-by: Sean Wang <sean.wang@mediatek.com>
---
 .../devicetree/bindings/leds/leds-mt6323.txt       | 63 ++++++++++++++++++++++
 1 file changed, 63 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/leds/leds-mt6323.txt

diff --git a/Documentation/devicetree/bindings/leds/leds-mt6323.txt b/Documentation/devicetree/bindings/leds/leds-mt6323.txt
new file mode 100644
index 0000000..135c441
--- /dev/null
+++ b/Documentation/devicetree/bindings/leds/leds-mt6323.txt
@@ -0,0 +1,63 @@
+Device Tree Bindings for LED support on MT6323 PMIC
+
+MT6323 LED controller is subfunction provided by
+MT6323 PMIC, so the LED controllers are defined as
+the subnode of the function node provided by MT6323
+PMIC controller that is being defined as one kind of
+Muti-Function Device (MFD) using shared bus called
+PMIC wrapper for each subfunction to access remote
+MT6323 PMIC hardware.
+
+For MT6323 MFD bindings see:
+Documentation/devicetree/bindings/mfd/mt6397.txt
+For MediaTek PMIC wrapper bindings see:
+Documentation/devicetree/bindings/soc/mediatek/pwrap.txt
+
+There's sub-node for the LED controller that describes
+the initial behavior for each LED physically and currently
+only four LED sub-nodes can be supported.
+
+Required properties:
+- compatible : Must be "mediatek,mt6323-led"
+- address-cells : Must be 1
+- size-cells : Must be 0
+
+LED sub-node properties:
+- reg : LED channel number (0..3)
+- label : (optional)
+  see Documentation/devicetree/bindings/leds/common.txt
+- linux,default-trigger : (optional)
+  see Documentation/devicetree/bindings/leds/common.txt
+- default-state: (optional) The initial state of the LED
+  see Documentation/devicetree/bindings/leds/common.txt
+
+Example:
+
+	pmic: mt6323 {
+		compatible = "mediatek,mt6323";
+
+		...
+
+		mt6323led: leds {
+			compatible = "mediatek,mt6323-led";
+			#address-cells = <1>;
+			#size-cells = <0>;
+
+			led@0 {
+				reg = <0>;
+				label = "LED0";
+				linux,default-trigger = "timer";
+				default-state = "on";
+			};
+			led@1 {
+				reg = <1>;
+				label = "LED1";
+				default-state = "off";
+			};
+			led@2 {
+				reg = <2>;
+				label = "LED2";
+				default-state = "on";
+			};
+		};
+	};
-- 
1.9.1

[PATCH v5 2/4] dt-bindings: mfd: Add LED subnode binding for MT6323 PMIC

[sean.wang]

From: Sean Wang <sean.wang@mediatek.com>

This patch adds documentation for devicetree bindings
for LED support as the subnode of MT6323 PMIC

Signed-off-by: Sean Wang <sean.wang@mediatek.com>
---
 Documentation/devicetree/bindings/mfd/mt6397.txt | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/Documentation/devicetree/bindings/mfd/mt6397.txt b/Documentation/devicetree/bindings/mfd/mt6397.txt
index 949c85f..522a3bb 100644
--- a/Documentation/devicetree/bindings/mfd/mt6397.txt
+++ b/Documentation/devicetree/bindings/mfd/mt6397.txt
@@ -6,6 +6,7 @@ MT6397/MT6323 is a multifunction device with the following sub modules:
 - Audio codec
 - GPIO
 - Clock
+- LED
 
 It is interfaced to host controller using SPI interface by a proprietary hardware
 called PMIC wrapper or pwrap. MT6397/MT6323 MFD is a child device of pwrap.
@@ -34,6 +35,10 @@ Optional subnodes:
 - clk
 	Required properties:
 		- compatible: "mediatek,mt6397-clk"
+- led
+	Required properties:
+		- compatible: "mediatek,mt6323-led"
+	see Documentation/devicetree/bindings/leds/leds-mt6323.txt
 
 Example:
 	pwrap: pwrap@1000f000 {
-- 
1.9.1

[PATCH v5 3/4] leds: Add LED support for MT6323 PMIC

[sean.wang]

From: Sean Wang <sean.wang@mediatek.com>

MT6323 PMIC is a multi-function device that includes
LED function. It allows attaching up to 4 LEDs which
can either be on, off or dimmed and/or blinked with
the controller.

Signed-off-by: Sean Wang <sean.wang@mediatek.com>
---
 drivers/leds/Kconfig       |   8 +
 drivers/leds/Makefile      |   1 +
 drivers/leds/leds-mt6323.c | 502 +++++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 511 insertions(+)
 create mode 100644 drivers/leds/leds-mt6323.c

diff --git a/drivers/leds/Kconfig b/drivers/leds/Kconfig
index c621cbb..30095fc 100644
--- a/drivers/leds/Kconfig
+++ b/drivers/leds/Kconfig
@@ -117,6 +117,14 @@ config LEDS_MIKROTIK_RB532
 	  This option enables support for the so called "User LED" of
 	  Mikrotik's Routerboard 532.
 
+config LEDS_MT6323
+	tristate "LED Support for Mediatek MT6323 PMIC"
+	depends on LEDS_CLASS
+	depends on MFD_MT6397
+	help
+	  This option enables support for on-chip LED drivers found on
+	  Mediatek MT6323 PMIC.
+
 config LEDS_S3C24XX
 	tristate "LED Support for Samsung S3C24XX GPIO LEDs"
 	depends on LEDS_CLASS
diff --git a/drivers/leds/Makefile b/drivers/leds/Makefile
index 6b82737..4feb332 100644
--- a/drivers/leds/Makefile
+++ b/drivers/leds/Makefile
@@ -72,6 +72,7 @@ obj-$(CONFIG_LEDS_IS31FL32XX)		+= leds-is31fl32xx.o
 obj-$(CONFIG_LEDS_PM8058)		+= leds-pm8058.o
 obj-$(CONFIG_LEDS_MLXCPLD)		+= leds-mlxcpld.o
 obj-$(CONFIG_LEDS_NIC78BX)		+= leds-nic78bx.o
+obj-$(CONFIG_LEDS_MT6323)		+= leds-mt6323.o
 
 # LED SPI Drivers
 obj-$(CONFIG_LEDS_DAC124S085)		+= leds-dac124s085.o
diff --git a/drivers/leds/leds-mt6323.c b/drivers/leds/leds-mt6323.c
new file mode 100644
index 0000000..48081bc
--- /dev/null
+++ b/drivers/leds/leds-mt6323.c
@@ -0,0 +1,502 @@
+/*
+ * LED driver for Mediatek MT6323 PMIC
+ *
+ * Copyright (C) 2017 Sean Wang <sean.wang@mediatek.com>
+ *
+ * This program is free software; you can redistribute it and/or
+ * modify it under the terms of the GNU General Public License as
+ * published by the Free Software Foundation; either version 2 of
+ * the License, or (at your option) any later version.
+ *
+ * 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/mfd/mt6323/registers.h>
+#include <linux/mfd/mt6397/core.h>
+#include <linux/module.h>
+#include <linux/of.h>
+#include <linux/platform_device.h>
+#include <linux/regmap.h>
+
+/*
+ * Register field for MT6323_TOP_CKPDN0 to enable
+ * 32K clock common for LED device.
+ */
+#define MT6323_RG_DRV_32K_CK_PDN	BIT(11)
+#define MT6323_RG_DRV_32K_CK_PDN_MASK	BIT(11)
+
+/*
+ * Register field for MT6323_TOP_CKPDN2 to enable
+ * individual clock for LED device.
+ */
+#define MT6323_RG_ISINK_CK_PDN(i)	BIT(i)
+#define MT6323_RG_ISINK_CK_PDN_MASK(i)	BIT(i)
+
+/*
+ * Register field for MT6323_TOP_CKCON1 to select
+ * clock source.
+ */
+#define MT6323_RG_ISINK_CK_SEL_MASK(i)	(BIT(10) << (i))
+
+/*
+ * Register for MT6323_ISINK_CON0 to setup the
+ * duty cycle of the blink.
+ */
+#define MT6323_ISINK_CON0(i)		(MT6323_ISINK0_CON0 + 0x8 * (i))
+#define MT6323_ISINK_DIM_DUTY_MASK	(0x1f << 8)
+#define MT6323_ISINK_DIM_DUTY(i)	(((i) << 8) & \
+					MT6323_ISINK_DIM_DUTY_MASK)
+
+/* Register to setup the period of the blink. */
+#define MT6323_ISINK_CON1(i)		(MT6323_ISINK0_CON1 + 0x8 * (i))
+#define MT6323_ISINK_DIM_FSEL_MASK	(0xffff)
+#define MT6323_ISINK_DIM_FSEL(i)	((i) & MT6323_ISINK_DIM_FSEL_MASK)
+
+/* Register to control the brightness. */
+#define MT6323_ISINK_CON2(i)		(MT6323_ISINK0_CON2 + 0x8 * (i))
+#define MT6323_ISINK_CH_STEP_SHIFT	12
+#define MT6323_ISINK_CH_STEP_MASK	(0x7 << 12)
+#define MT6323_ISINK_CH_STEP(i)		(((i) << 12) & \
+					MT6323_ISINK_CH_STEP_MASK)
+#define MT6323_ISINK_SFSTR0_TC_MASK	(0x3 << 1)
+#define MT6323_ISINK_SFSTR0_TC(i)	(((i) << 1) & \
+					MT6323_ISINK_SFSTR0_TC_MASK)
+#define MT6323_ISINK_SFSTR0_EN_MASK	BIT(0)
+#define MT6323_ISINK_SFSTR0_EN		BIT(0)
+
+/* Register to LED channel enablement. */
+#define MT6323_ISINK_CH_EN_MASK(i)	BIT(i)
+#define MT6323_ISINK_CH_EN(i)		BIT(i)
+
+#define MT6323_MAX_PERIOD		10000
+#define MT6323_MAX_LEDS			4
+#define MT6323_MAX_BRIGHTNESS		6
+#define MT6323_UNIT_DUTY		3125
+#define MT6323_CAL_HW_DUTY(o, p)	DIV_ROUND_CLOSEST((o) * 100000ul,\
+					(p) * MT6323_UNIT_DUTY)
+
+struct mt6323_leds;
+
+/**
+ * struct mt6323_led - state container for the LED device
+ * @id:			the identifier in MT6323 LED device
+ * @parent:		the pointer to MT6323 LED controller
+ * @cdev:		LED class device for this LED device
+ * @current_brightness: current state of the LED device
+ */
+struct mt6323_led {
+	int			id;
+	struct mt6323_leds	*parent;
+	struct led_classdev	cdev;
+	enum led_brightness	current_brightness;
+};
+
+/**
+ * struct mt6323_leds -	state container for holding LED controller
+ *			of the driver
+ * @dev:		the device pointer
+ * @hw:			the underlying hardware providing shared
+ *			bus for the register operations
+ * @lock:		the lock among process context
+ * @led:		the array that contains the state of individual
+ *			LED device
+ */
+struct mt6323_leds {
+	struct device		*dev;
+	struct mt6397_chip	*hw;
+	/* protect among process context */
+	struct mutex		lock;
+	struct mt6323_led	*led[MT6323_MAX_LEDS];
+};
+
+static int mt6323_led_hw_brightness(struct led_classdev *cdev,
+				    enum led_brightness brightness)
+{
+	struct mt6323_led *led = container_of(cdev, struct mt6323_led, cdev);
+	struct mt6323_leds *leds = led->parent;
+	struct regmap *regmap = leds->hw->regmap;
+	u32 con2_mask = 0, con2_val = 0;
+	int ret;
+
+	/*
+	 * Setup current output for the corresponding
+	 * brightness level.
+	 */
+	con2_mask |= MT6323_ISINK_CH_STEP_MASK |
+		     MT6323_ISINK_SFSTR0_TC_MASK |
+		     MT6323_ISINK_SFSTR0_EN_MASK;
+	con2_val |=  MT6323_ISINK_CH_STEP(brightness - 1) |
+		     MT6323_ISINK_SFSTR0_TC(2) |
+		     MT6323_ISINK_SFSTR0_EN;
+
+	ret = regmap_update_bits(regmap, MT6323_ISINK_CON2(led->id),
+				 con2_mask, con2_val);
+	return ret;
+}
+
+static int mt6323_led_hw_off(struct led_classdev *cdev)
+{
+	struct mt6323_led *led = container_of(cdev, struct mt6323_led, cdev);
+	struct mt6323_leds *leds = led->parent;
+	struct regmap *regmap = leds->hw->regmap;
+	unsigned int status;
+	int ret;
+
+	status = MT6323_ISINK_CH_EN(led->id);
+	ret = regmap_update_bits(regmap, MT6323_ISINK_EN_CTRL,
+				 MT6323_ISINK_CH_EN_MASK(led->id), ~status);
+	if (ret < 0)
+		return ret;
+
+	usleep_range(100, 300);
+	ret = regmap_update_bits(regmap, MT6323_TOP_CKPDN2,
+				 MT6323_RG_ISINK_CK_PDN_MASK(led->id),
+				 MT6323_RG_ISINK_CK_PDN(led->id));
+	if (ret < 0)
+		return ret;
+
+	return 0;
+}
+
+static enum led_brightness
+mt6323_get_led_hw_brightness(struct led_classdev *cdev)
+{
+	struct mt6323_led *led = container_of(cdev, struct mt6323_led, cdev);
+	struct mt6323_leds *leds = led->parent;
+	struct regmap *regmap = leds->hw->regmap;
+	unsigned int status;
+	int ret;
+
+	ret = regmap_read(regmap, MT6323_TOP_CKPDN2, &status);
+	if (ret < 0)
+		return ret;
+
+	if (status & MT6323_RG_ISINK_CK_PDN_MASK(led->id))
+		return 0;
+
+	ret = regmap_read(regmap, MT6323_ISINK_EN_CTRL, &status);
+	if (ret < 0)
+		return ret;
+
+	if (!(status & MT6323_ISINK_CH_EN(led->id)))
+		return 0;
+
+	ret = regmap_read(regmap, MT6323_ISINK_CON2(led->id), &status);
+	if (ret < 0)
+		return ret;
+
+	return  ((status & MT6323_ISINK_CH_STEP_MASK)
+		  >> MT6323_ISINK_CH_STEP_SHIFT) + 1;
+}
+
+static int mt6323_led_hw_on(struct led_classdev *cdev,
+			    enum led_brightness brightness)
+{
+	struct mt6323_led *led = container_of(cdev, struct mt6323_led, cdev);
+	struct mt6323_leds *leds = led->parent;
+	struct regmap *regmap = leds->hw->regmap;
+	unsigned int status;
+	int ret;
+
+	/*
+	 * Setup required clock source, enable the corresponding
+	 * clock and channel and let work with continuous blink as
+	 * the default.
+	 */
+	ret = regmap_update_bits(regmap, MT6323_TOP_CKCON1,
+				 MT6323_RG_ISINK_CK_SEL_MASK(led->id), 0);
+	if (ret < 0)
+		return ret;
+
+	status = MT6323_RG_ISINK_CK_PDN(led->id);
+	ret = regmap_update_bits(regmap, MT6323_TOP_CKPDN2,
+				 MT6323_RG_ISINK_CK_PDN_MASK(led->id),
+				 ~status);
+	if (ret < 0)
+		return ret;
+
+	usleep_range(100, 300);
+
+	ret = regmap_update_bits(regmap, MT6323_ISINK_EN_CTRL,
+				 MT6323_ISINK_CH_EN_MASK(led->id),
+				 MT6323_ISINK_CH_EN(led->id));
+	if (ret < 0)
+		return ret;
+
+	ret = mt6323_led_hw_brightness(cdev, brightness);
+	if (ret < 0)
+		return ret;
+
+	ret = regmap_update_bits(regmap, MT6323_ISINK_CON0(led->id),
+				 MT6323_ISINK_DIM_DUTY_MASK,
+				 MT6323_ISINK_DIM_DUTY(31));
+	if (ret < 0)
+		return ret;
+
+	ret = regmap_update_bits(regmap, MT6323_ISINK_CON1(led->id),
+				 MT6323_ISINK_DIM_FSEL_MASK,
+				 MT6323_ISINK_DIM_FSEL(1000));
+	if (ret < 0)
+		return ret;
+
+	return 0;
+}
+
+static int mt6323_led_set_blink(struct led_classdev *cdev,
+				unsigned long *delay_on,
+				unsigned long *delay_off)
+{
+	struct mt6323_led *led = container_of(cdev, struct mt6323_led, cdev);
+	struct mt6323_leds *leds = led->parent;
+	struct regmap *regmap = leds->hw->regmap;
+	unsigned long period;
+	u8 duty_hw;
+	int ret;
+
+	/*
+	 * Units are in ms, if over the hardware able
+	 * to support, fallback into software blink
+	 */
+	period = *delay_on + *delay_off;
+
+	if (period > MT6323_MAX_PERIOD)
+		return -EINVAL;
+
+	/*
+	 * LED subsystem requires a default user
+	 * friendly blink pattern for the LED so using
+	 * 1Hz duty cycle 50% here if without specific
+	 * value delay_on and delay off being assigned.
+	 */
+	if (!*delay_on && !*delay_off) {
+		*delay_on = 500;
+		*delay_off = 500;
+	}
+
+	/*
+	 * Calculate duty_hw based on the percentage of period during
+	 * which the led is ON.
+	 */
+	duty_hw = MT6323_CAL_HW_DUTY(*delay_on, period);
+
+	/* hardware doesn't support zero duty cycle. */
+	if (!duty_hw)
+		return -EINVAL;
+
+	mutex_lock(&leds->lock);
+	/*
+	 * Set max_brightness as the software blink behavior
+	 * when no blink brightness.
+	 */
+	if (!led->current_brightness) {
+		ret = mt6323_led_hw_on(cdev, cdev->max_brightness);
+		if (ret < 0)
+			goto out;
+		led->current_brightness = cdev->max_brightness;
+	}
+
+	ret = regmap_update_bits(regmap, MT6323_ISINK_CON0(led->id),
+				 MT6323_ISINK_DIM_DUTY_MASK,
+				 MT6323_ISINK_DIM_DUTY(duty_hw - 1));
+	if (ret < 0)
+		goto out;
+
+	ret = regmap_update_bits(regmap, MT6323_ISINK_CON1(led->id),
+				 MT6323_ISINK_DIM_FSEL_MASK,
+				 MT6323_ISINK_DIM_FSEL(period - 1));
+out:
+	mutex_unlock(&leds->lock);
+
+	return ret;
+}
+
+static int mt6323_led_set_brightness(struct led_classdev *cdev,
+				     enum led_brightness brightness)
+{
+	struct mt6323_led *led = container_of(cdev, struct mt6323_led, cdev);
+	struct mt6323_leds *leds = led->parent;
+	int ret;
+
+	mutex_lock(&leds->lock);
+
+	if (!led->current_brightness && brightness) {
+		ret = mt6323_led_hw_on(cdev, brightness);
+		if (ret < 0)
+			goto out;
+	} else if (brightness) {
+		ret = mt6323_led_hw_brightness(cdev, brightness);
+		if (ret < 0)
+			goto out;
+	} else {
+		ret = mt6323_led_hw_off(cdev);
+		if (ret < 0)
+			goto out;
+	}
+
+	led->current_brightness = brightness;
+out:
+	mutex_unlock(&leds->lock);
+
+	return ret;
+}
+
+static int mt6323_led_set_dt_default(struct led_classdev *cdev,
+				     struct device_node *np)
+{
+	struct mt6323_led *led = container_of(cdev, struct mt6323_led, cdev);
+	const char *state;
+	int ret = 0;
+
+	led->cdev.name = of_get_property(np, "label", NULL) ? : np->name;
+	led->cdev.default_trigger = of_get_property(np,
+						    "linux,default-trigger",
+						    NULL);
+
+	state = of_get_property(np, "default-state", NULL);
+	if (state) {
+		if (!strcmp(state, "keep")) {
+			ret = mt6323_get_led_hw_brightness(cdev);
+			if (ret < 0)
+				return ret;
+			led->current_brightness = ret;
+			ret = 0;
+		} else if (!strcmp(state, "on")) {
+			ret =
+			mt6323_led_set_brightness(cdev, cdev->max_brightness);
+		} else  {
+			ret = mt6323_led_set_brightness(cdev, LED_OFF);
+		}
+	}
+
+	return ret;
+}
+
+static int mt6323_led_probe(struct platform_device *pdev)
+{
+	struct device *dev = &pdev->dev;
+	struct device_node *np = pdev->dev.of_node;
+	struct device_node *child;
+	struct mt6397_chip *hw = dev_get_drvdata(pdev->dev.parent);
+	struct mt6323_leds *leds;
+	struct mt6323_led *led;
+	int ret;
+	unsigned int status;
+	u32 reg;
+
+	leds = devm_kzalloc(dev, sizeof(*leds), GFP_KERNEL);
+	if (!leds)
+		return -ENOMEM;
+
+	platform_set_drvdata(pdev, leds);
+	leds->dev = dev;
+
+	/*
+	 * leds->hw points to the underlying bus for the register
+	 * controlled.
+	 */
+	leds->hw = hw;
+	mutex_init(&leds->lock);
+
+	status = MT6323_RG_DRV_32K_CK_PDN;
+	ret = regmap_update_bits(leds->hw->regmap, MT6323_TOP_CKPDN0,
+				 MT6323_RG_DRV_32K_CK_PDN_MASK, ~status);
+	if (ret < 0) {
+		dev_err(leds->dev,
+			"Failed to update MT6323_TOP_CKPDN0 Register\n");
+		return ret;
+	}
+
+	for_each_available_child_of_node(np, child) {
+		ret = of_property_read_u32(child, "reg", &reg);
+		if (ret) {
+			dev_err(dev, "Failed to read led 'reg' property\n");
+			goto put_child_node;
+		}
+
+		if (reg < 0 || reg > MT6323_MAX_LEDS || leds->led[reg]) {
+			dev_err(dev, "Invalid led reg %u\n", reg);
+			ret = -EINVAL;
+			goto put_child_node;
+		}
+
+		led = devm_kzalloc(dev, sizeof(*led), GFP_KERNEL);
+		if (!led) {
+			ret = -ENOMEM;
+			goto put_child_node;
+		}
+
+		leds->led[reg] = led;
+		leds->led[reg]->id = reg;
+		leds->led[reg]->cdev.max_brightness = MT6323_MAX_BRIGHTNESS;
+		leds->led[reg]->cdev.brightness_set_blocking =
+					mt6323_led_set_brightness;
+		leds->led[reg]->cdev.blink_set = mt6323_led_set_blink;
+		leds->led[reg]->cdev.brightness_get =
+					mt6323_get_led_hw_brightness;
+		leds->led[reg]->parent = leds;
+
+		ret = mt6323_led_set_dt_default(&leds->led[reg]->cdev, child);
+		if (ret < 0) {
+			dev_err(leds->dev,
+				"Failed to LED set default from devicetree\n");
+			goto put_child_node;
+		}
+
+		ret = devm_led_classdev_register(dev, &leds->led[reg]->cdev);
+		if (ret) {
+			dev_err(&pdev->dev, "Failed to register LED: %d\n",
+				ret);
+			goto put_child_node;
+		}
+		leds->led[reg]->cdev.dev->of_node = child;
+	}
+
+	return 0;
+
+put_child_node:
+	of_node_put(child);
+	return ret;
+}
+
+static int mt6323_led_remove(struct platform_device *pdev)
+{
+	struct mt6323_leds *leds = platform_get_drvdata(pdev);
+	int i;
+
+	/* Turn the LEDs off on driver removal. */
+	for (i = 0 ; leds->led[i] ; i++)
+		mt6323_led_hw_off(&leds->led[i]->cdev);
+
+	regmap_update_bits(leds->hw->regmap, MT6323_TOP_CKPDN0,
+			   MT6323_RG_DRV_32K_CK_PDN_MASK,
+			   MT6323_RG_DRV_32K_CK_PDN);
+
+	mutex_destroy(&leds->lock);
+
+	return 0;
+}
+
+static const struct of_device_id mt6323_led_dt_match[] = {
+	{ .compatible = "mediatek,mt6323-led" },
+	{},
+};
+MODULE_DEVICE_TABLE(of, mt6323_led_dt_match);
+
+static struct platform_driver mt6323_led_driver = {
+	.probe		= mt6323_led_probe,
+	.remove		= mt6323_led_remove,
+	.driver		= {
+		.name	= "mt6323-led",
+		.of_match_table = mt6323_led_dt_match,
+	},
+};
+
+module_platform_driver(mt6323_led_driver);
+
+MODULE_DESCRIPTION("LED driver for Mediatek MT6323 PMIC");
+MODULE_AUTHOR("Sean Wang <sean.wang@mediatek.com>");
+MODULE_LICENSE("GPL");
-- 
1.9.1

[PATCH v5 4/4] mfd: mt6397: Add MT6323 LED support into MT6397 driver

[sean.wang]

From: Sean Wang <sean.wang@mediatek.com>

Add compatible string as "mt6323-led" that will make
the OF core spawn child devices for the LED subnode
of that MT6323 MFD device.

Signed-off-by: Sean Wang <sean.wang@mediatek.com>
---
 drivers/mfd/mt6397-core.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/drivers/mfd/mt6397-core.c b/drivers/mfd/mt6397-core.c
index e14d8b0..8e601c8 100644
--- a/drivers/mfd/mt6397-core.c
+++ b/drivers/mfd/mt6397-core.c
@@ -48,6 +48,10 @@
 		.name = "mt6323-regulator",
 		.of_compatible = "mediatek,mt6323-regulator"
 	},
+	{
+		.name = "mt6323-led",
+		.of_compatible = "mediatek,mt6323-led"
+	},
 };
 
 static const struct mfd_cell mt6397_devs[] = {
-- 
1.9.1

Re: [PATCH v5 1/4] dt-bindings: leds: Add document bindings for leds-mt6323

[Rob Herring]

On Fri, Mar 03, 2017 at 09:56:22PM +0800, sean.wang@mediatek.com wrote:
> From: Sean Wang <sean.wang@mediatek.com>
> 
> This patch adds documentation for devicetree bindings
> for LED support on MT6323 PMIC
> 
> Signed-off-by: Sean Wang <sean.wang@mediatek.com>
> ---
>  .../devicetree/bindings/leds/leds-mt6323.txt       | 63 ++++++++++++++++++++++
>  1 file changed, 63 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/leds/leds-mt6323.txt
> 
> diff --git a/Documentation/devicetree/bindings/leds/leds-mt6323.txt b/Documentation/devicetree/bindings/leds/leds-mt6323.txt
> new file mode 100644
> index 0000000..135c441
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/leds/leds-mt6323.txt
> @@ -0,0 +1,63 @@
> +Device Tree Bindings for LED support on MT6323 PMIC
> +
> +MT6323 LED controller is subfunction provided by
> +MT6323 PMIC, so the LED controllers are defined as
> +the subnode of the function node provided by MT6323
> +PMIC controller that is being defined as one kind of
> +Muti-Function Device (MFD) using shared bus called
> +PMIC wrapper for each subfunction to access remote
> +MT6323 PMIC hardware.

Your line wrap is kind of short. Fix you line wrap to 72-80 char.

> +
> +For MT6323 MFD bindings see:
> +Documentation/devicetree/bindings/mfd/mt6397.txt
> +For MediaTek PMIC wrapper bindings see:
> +Documentation/devicetree/bindings/soc/mediatek/pwrap.txt
> +
> +There's sub-node for the LED controller that describes
> +the initial behavior for each LED physically and currently
> +only four LED sub-nodes can be supported.
> +
> +Required properties:
> +- compatible : Must be "mediatek,mt6323-led"
> +- address-cells : Must be 1
> +- size-cells : Must be 0
> +
> +LED sub-node properties:
> +- reg : LED channel number (0..3)
> +- label : (optional)
> +  see Documentation/devicetree/bindings/leds/common.txt
> +- linux,default-trigger : (optional)
> +  see Documentation/devicetree/bindings/leds/common.txt
> +- default-state: (optional) The initial state of the LED
> +  see Documentation/devicetree/bindings/leds/common.txt

Group these by required and optional. You can drop "The initial state of 
the LED".

> +
> +Example:
> +
> +	pmic: mt6323 {

mt6323: pmic {

> +		compatible = "mediatek,mt6323";
> +
> +		...
> +
> +		mt6323led: leds {
> +			compatible = "mediatek,mt6323-led";
> +			#address-cells = <1>;
> +			#size-cells = <0>;
> +
> +			led@0 {
> +				reg = <0>;
> +				label = "LED0";
> +				linux,default-trigger = "timer";
> +				default-state = "on";
> +			};
> +			led@1 {
> +				reg = <1>;
> +				label = "LED1";
> +				default-state = "off";
> +			};
> +			led@2 {
> +				reg = <2>;
> +				label = "LED2";
> +				default-state = "on";
> +			};
> +		};
> +	};
> -- 
> 1.9.1
> 

Re: [PATCH v5 2/4] dt-bindings: mfd: Add LED subnode binding for MT6323 PMIC

[Rob Herring]

On Fri, Mar 03, 2017 at 09:56:23PM +0800, sean.wang@mediatek.com wrote:
> From: Sean Wang <sean.wang@mediatek.com>
> 
> This patch adds documentation for devicetree bindings
> for LED support as the subnode of MT6323 PMIC
> 
> Signed-off-by: Sean Wang <sean.wang@mediatek.com>
> ---
>  Documentation/devicetree/bindings/mfd/mt6397.txt | 5 +++++
>  1 file changed, 5 insertions(+)

Acked-by: Rob Herring <robh@kernel.org>

Re: [PATCH v5 2/4] dt-bindings: mfd: Add LED subnode binding for MT6323 PMIC

[Matthias Brugger]

On 03/03/17 14:56, sean.wang@mediatek.com wrote:
> From: Sean Wang <sean.wang@mediatek.com>
>
> This patch adds documentation for devicetree bindings
> for LED support as the subnode of MT6323 PMIC
>
> Signed-off-by: Sean Wang <sean.wang@mediatek.com>
> ---
>  Documentation/devicetree/bindings/mfd/mt6397.txt | 5 +++++
>  1 file changed, 5 insertions(+)
>
> diff --git a/Documentation/devicetree/bindings/mfd/mt6397.txt b/Documentation/devicetree/bindings/mfd/mt6397.txt
> index 949c85f..522a3bb 100644
> --- a/Documentation/devicetree/bindings/mfd/mt6397.txt
> +++ b/Documentation/devicetree/bindings/mfd/mt6397.txt
> @@ -6,6 +6,7 @@ MT6397/MT6323 is a multifunction device with the following sub modules:
>  - Audio codec
>  - GPIO
>  - Clock
> +- LED
>
>  It is interfaced to host controller using SPI interface by a proprietary hardware
>  called PMIC wrapper or pwrap. MT6397/MT6323 MFD is a child device of pwrap.
> @@ -34,6 +35,10 @@ Optional subnodes:
>  - clk
>  	Required properties:
>  		- compatible: "mediatek,mt6397-clk"
> +- led
> +	Required properties:
> +		- compatible: "mediatek,mt6323-led"
> +	see Documentation/devicetree/bindings/leds/leds-mt6323.txt

Part of this patch is already part of linux-next:
05daab3 ("Documentation: devicetree: Add LED subnode binding for MT6323 
PMIC")

Regards,
Matthias

Re: [PATCH v5 2/4] dt-bindings: mfd: Add LED subnode binding for MT6323 PMIC

[Jacek Anaszewski]

Hi Matthias,

On 03/13/2017 01:00 PM, Matthias Brugger wrote:
> 
> 
> On 03/03/17 14:56, sean.wang@mediatek.com wrote:
>> From: Sean Wang <sean.wang@mediatek.com>
>>
>> This patch adds documentation for devicetree bindings
>> for LED support as the subnode of MT6323 PMIC
>>
>> Signed-off-by: Sean Wang <sean.wang@mediatek.com>
>> ---
>>  Documentation/devicetree/bindings/mfd/mt6397.txt | 5 +++++
>>  1 file changed, 5 insertions(+)
>>
>> diff --git a/Documentation/devicetree/bindings/mfd/mt6397.txt
>> b/Documentation/devicetree/bindings/mfd/mt6397.txt
>> index 949c85f..522a3bb 100644
>> --- a/Documentation/devicetree/bindings/mfd/mt6397.txt
>> +++ b/Documentation/devicetree/bindings/mfd/mt6397.txt
>> @@ -6,6 +6,7 @@ MT6397/MT6323 is a multifunction device with the
>> following sub modules:
>>  - Audio codec
>>  - GPIO
>>  - Clock
>> +- LED
>>
>>  It is interfaced to host controller using SPI interface by a
>> proprietary hardware
>>  called PMIC wrapper or pwrap. MT6397/MT6323 MFD is a child device of
>> pwrap.
>> @@ -34,6 +35,10 @@ Optional subnodes:
>>  - clk
>>      Required properties:
>>          - compatible: "mediatek,mt6397-clk"
>> +- led
>> +    Required properties:
>> +        - compatible: "mediatek,mt6323-led"
>> +    see Documentation/devicetree/bindings/leds/leds-mt6323.txt
> 
> Part of this patch is already part of linux-next:
> 05daab3 ("Documentation: devicetree: Add LED subnode binding for MT6323
> PMIC")

Yes, it's been merged via mfd tree. It doesn't break anything besides
that it points to non-existent file. Hopefully we will manage to merge
the LED part before merge window, otherwise the mfd part will have to
be dropped.

-- 
Best regards,
Jacek Anaszewski

Re: [PATCH v5 2/4] dt-bindings: mfd: Add LED subnode binding for MT6323 PMIC

[Sean Wang]

On Mon, 2017-03-13 at 13:00 +0100, Matthias Brugger wrote:
> 
> On 03/03/17 14:56, sean.wang@mediatek.com wrote:
> > From: Sean Wang <sean.wang@mediatek.com>
> >
> > This patch adds documentation for devicetree bindings
> > for LED support as the subnode of MT6323 PMIC
> >
> > Signed-off-by: Sean Wang <sean.wang@mediatek.com>
> > ---
> >  Documentation/devicetree/bindings/mfd/mt6397.txt | 5 +++++
> >  1 file changed, 5 insertions(+)
> >
> > diff --git a/Documentation/devicetree/bindings/mfd/mt6397.txt b/Documentation/devicetree/bindings/mfd/mt6397.txt
> > index 949c85f..522a3bb 100644
> > --- a/Documentation/devicetree/bindings/mfd/mt6397.txt
> > +++ b/Documentation/devicetree/bindings/mfd/mt6397.txt
> > @@ -6,6 +6,7 @@ MT6397/MT6323 is a multifunction device with the following sub modules:
> >  - Audio codec
> >  - GPIO
> >  - Clock
> > +- LED
> >
> >  It is interfaced to host controller using SPI interface by a proprietary hardware
> >  called PMIC wrapper or pwrap. MT6397/MT6323 MFD is a child device of pwrap.
> > @@ -34,6 +35,10 @@ Optional subnodes:
> >  - clk
> >  	Required properties:
> >  		- compatible: "mediatek,mt6397-clk"
> > +- led
> > +	Required properties:
> > +		- compatible: "mediatek,mt6323-led"
> > +	see Documentation/devicetree/bindings/leds/leds-mt6323.txt
> 
> Part of this patch is already part of linux-next:
> 05daab3 ("Documentation: devicetree: Add LED subnode binding for MT6323 
> PMIC")
> 
Hi Matthias,

There is still small differences from the merged patch. 

I will submit the series for the next version based on newer kernel and

split the already merged ones out. Thanks for your kindly remainder.

	Sean
> Regards,
> Matthias

Re: [PATCH v5 4/4] mfd: mt6397: Add MT6323 LED support into MT6397 driver

[Lee Jones]

On Fri, 03 Mar 2017, sean.wang@mediatek.com wrote:

> From: Sean Wang <sean.wang@mediatek.com>
> 
> Add compatible string as "mt6323-led" that will make
> the OF core spawn child devices for the LED subnode
> of that MT6323 MFD device.
> 
> Signed-off-by: Sean Wang <sean.wang@mediatek.com>
> ---
>  drivers/mfd/mt6397-core.c | 4 ++++
>  1 file changed, 4 insertions(+)

For my own reference:
  Acked-for-MFD-by: Lee Jones <lee.jones@linaro.org>

> diff --git a/drivers/mfd/mt6397-core.c b/drivers/mfd/mt6397-core.c
> index e14d8b0..8e601c8 100644
> --- a/drivers/mfd/mt6397-core.c
> +++ b/drivers/mfd/mt6397-core.c
> @@ -48,6 +48,10 @@
>  		.name = "mt6323-regulator",
>  		.of_compatible = "mediatek,mt6323-regulator"
>  	},
> +	{

Put this on the line above.

> +		.name = "mt6323-led",
> +		.of_compatible = "mediatek,mt6323-led"
> +	},
>  };
>  
>  static const struct mfd_cell mt6397_devs[] = {

-- 
Lee Jones
Linaro STMicroelectronics Landing Team Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog