[PATCH v4 0/3] Add ktd2692 Flash LED driver using LED Flash class

[PATCH v4 0/3] Add ktd2692 Flash LED driver using LED Flash class

[Ingi Kim]

This patch adds ktd2692 Flash LED driver with LED Flash class

Change in v4:
	- Clean up the code
	- Modify binding documentation of ktd2692

Change in v3:
	- Clean up the code
	- Add aux gpio pin to control Flash LED

Change in v2:
	- Introduction of LED Flash class as Jacek's comment
	- Supplement of binding documentation
	- Rename gpio control pin and remove unused pin
	- Add regulator for the Flash LED

Ingi Kim (3):
  of: Add vendor prefix for Kinetic technologies
  leds: ktd2692: add device tree bindings for ktd2692
  leds: Add ktd2692 flash LED driver

 .../devicetree/bindings/leds/leds-ktd2692.txt      |  33 ++
 .../devicetree/bindings/vendor-prefixes.txt        |   1 +
 drivers/leds/Kconfig                               |   9 +
 drivers/leds/Makefile                              |   1 +
 drivers/leds/leds-ktd2692.c                        | 412 +++++++++++++++++++++
 5 files changed, 456 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/leds/leds-ktd2692.txt
 create mode 100644 drivers/leds/leds-ktd2692.c

-- 
2.0.5

[PATCH v4 1/3] of: Add vendor prefix for Kinetic technologies

[Ingi Kim]

This patch adds vendor prefix for Kinetic technologies

Signed-off-by: Ingi Kim <ingi2.kim@samsung.com>
Acked-by: Rob Herring <robh@kernel.org>
---
 Documentation/devicetree/bindings/vendor-prefixes.txt | 1 +
 1 file changed, 1 insertion(+)

diff --git a/Documentation/devicetree/bindings/vendor-prefixes.txt b/Documentation/devicetree/bindings/vendor-prefixes.txt
index 389ca13..de9e126 100644
--- a/Documentation/devicetree/bindings/vendor-prefixes.txt
+++ b/Documentation/devicetree/bindings/vendor-prefixes.txt
@@ -98,6 +98,7 @@ isee	ISEE 2007 S.L.
 isil	Intersil
 karo	Ka-Ro electronics GmbH
 keymile	Keymile GmbH
+kinetic Kinetic Technologies
 lacie	LaCie
 lantiq	Lantiq Semiconductor
 lenovo	Lenovo Group Ltd.
-- 
2.0.5

[PATCH v4 2/3] leds: ktd2692: add device tree bindings for ktd2692

[Ingi Kim]

This patch adds the device tree bindings for ktd2692 flash LEDs.
Add optional properties 'flash-timeout-us' to control flash timeout
and 'vin-supply' for flash-led regulator

Signed-off-by: Ingi Kim <ingi2.kim@samsung.com>
---
 .../devicetree/bindings/leds/leds-ktd2692.txt      | 33 ++++++++++++++++++++++
 1 file changed, 33 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/leds/leds-ktd2692.txt

diff --git a/Documentation/devicetree/bindings/leds/leds-ktd2692.txt b/Documentation/devicetree/bindings/leds/leds-ktd2692.txt
new file mode 100644
index 0000000..f78512f
--- /dev/null
+++ b/Documentation/devicetree/bindings/leds/leds-ktd2692.txt
@@ -0,0 +1,33 @@
+* Kinetic Technologies - KTD2692 Flash LED Driver
+
+KTD2692 is the ideal power solution for high-power flash LEDs.
+It uses ExpressWire single-wire programming for maximum flexibility.
+
+The ExpressWire interface through CTRL pin can control LED on/off and
+enable/disable the IC, Movie(max 1/3 of Flash current) / Flash mode current,
+Flash timeout, LVP(low voltage protection).
+
+Also, When the AUX pin is pulled high while CTRL pin is high,
+LED current will be ramped up to the flash-mode current level.
+
+Required properties:
+	- compatible: "kinetic,ktd2692"
+	- ctrl-gpio, aux-gpio : gpio pins in order control ktd2692 flash led.
+		There is an internal 300kOhm pull-down resistor at each pin
+	- vin-supply : "vin" LED supply (2.7V to 5.5V)
+		See Documentation/devicetree/bindings/regulator/regulator.txt
+
+Optional property:
+	- flash-timeout-us : Maximum flash timeout in microseconds.
+			flash timeout ranges from 0 to 1835000us and
+			default is 1049000us.
+
+Example:
+
+flash-led {
+	compatible = "kinetic,ktd2692";
+	ctrl-gpio = <&gpc0 1 0>;
+	aux-gpio = <&gpc0 2 0>;
+	flash-timeout-us = <1835000>;
+	vin-supply = <&vbat>;
+};
-- 
2.0.5

Re: [PATCH v4 2/3] leds: ktd2692: add device tree bindings for ktd2692

[Varka Bhadram]

On 03/25/2015 07:00 AM, Ingi Kim wrote:
> This patch adds the device tree bindings for ktd2692 flash LEDs.
> Add optional properties 'flash-timeout-us' to control flash timeout
> and 'vin-supply' for flash-led regulator
>
> Signed-off-by: Ingi Kim <ingi2.kim@samsung.com>
> ---
>   .../devicetree/bindings/leds/leds-ktd2692.txt      | 33 ++++++++++++++++++++++
>   1 file changed, 33 insertions(+)
>   create mode 100644 Documentation/devicetree/bindings/leds/leds-ktd2692.txt
>
> diff --git a/Documentation/devicetree/bindings/leds/leds-ktd2692.txt b/Documentation/devicetree/bindings/leds/leds-ktd2692.txt
> new file mode 100644
> index 0000000..f78512f
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/leds/leds-ktd2692.txt
> @@ -0,0 +1,33 @@
> +* Kinetic Technologies - KTD2692 Flash LED Driver
> +
> +KTD2692 is the ideal power solution for high-power flash LEDs.
> +It uses ExpressWire single-wire programming for maximum flexibility.
> +
> +The ExpressWire interface through CTRL pin can control LED on/off and
> +enable/disable the IC, Movie(max 1/3 of Flash current) / Flash mode current,
> +Flash timeout, LVP(low voltage protection).
> +
> +Also, When the AUX pin is pulled high while CTRL pin is high,
> +LED current will be ramped up to the flash-mode current level.
> +
> +Required properties:
> +	- compatible: "kinetic,ktd2692"
> +	- ctrl-gpio, aux-gpio : gpio pins in order control ktd2692 flash led.
> +		There is an internal 300kOhm pull-down resistor at each pin
> +	- vin-supply : "vin" LED supply (2.7V to 5.5V)
> +		See Documentation/devicetree/bindings/regulator/regulator.txt
> +

Above bindings are not that readable to me. Remove the tab spaces before properties.

> +Optional property:
> +	- flash-timeout-us : Maximum flash timeout in microseconds.
> +			flash timeout ranges from 0 to 1835000us and
> +			default is 1049000us.
> +
> +Example:
> +
> +flash-led {
> +	compatible = "kinetic,ktd2692";
> +	ctrl-gpio = <&gpc0 1 0>;
> +	aux-gpio = <&gpc0 2 0>;
> +	flash-timeout-us = <1835000>;
> +	vin-supply = <&vbat>;
> +};


-- 
Varka Bhadram

Re: [PATCH v4 2/3] leds: ktd2692: add device tree bindings for ktd2692

[Ingi Kim]

Hi Varka,

Thanks for the review

On 2015년 03월 25일 12:31, Varka Bhadram wrote:
> On 03/25/2015 07:00 AM, Ingi Kim wrote:
>> This patch adds the device tree bindings for ktd2692 flash LEDs.
>> Add optional properties 'flash-timeout-us' to control flash timeout
>> and 'vin-supply' for flash-led regulator
>>
>> Signed-off-by: Ingi Kim <ingi2.kim@samsung.com>
>> ---
>>   .../devicetree/bindings/leds/leds-ktd2692.txt      | 33 ++++++++++++++++++++++
>>   1 file changed, 33 insertions(+)
>>   create mode 100644 Documentation/devicetree/bindings/leds/leds-ktd2692.txt
>>
>> diff --git a/Documentation/devicetree/bindings/leds/leds-ktd2692.txt b/Documentation/devicetree/bindings/leds/leds-ktd2692.txt
>> new file mode 100644
>> index 0000000..f78512f
>> --- /dev/null
>> +++ b/Documentation/devicetree/bindings/leds/leds-ktd2692.txt
>> @@ -0,0 +1,33 @@
>> +* Kinetic Technologies - KTD2692 Flash LED Driver
>> +
>> +KTD2692 is the ideal power solution for high-power flash LEDs.
>> +It uses ExpressWire single-wire programming for maximum flexibility.
>> +
>> +The ExpressWire interface through CTRL pin can control LED on/off and
>> +enable/disable the IC, Movie(max 1/3 of Flash current) / Flash mode current,
>> +Flash timeout, LVP(low voltage protection).
>> +
>> +Also, When the AUX pin is pulled high while CTRL pin is high,
>> +LED current will be ramped up to the flash-mode current level.
>> +
>> +Required properties:
>> +    - compatible: "kinetic,ktd2692"
>> +    - ctrl-gpio, aux-gpio : gpio pins in order control ktd2692 flash led.
>> +        There is an internal 300kOhm pull-down resistor at each pin
>> +    - vin-supply : "vin" LED supply (2.7V to 5.5V)
>> +        See Documentation/devicetree/bindings/regulator/regulator.txt
>> +
> 
> Above bindings are not that readable to me. Remove the tab spaces before properties.
> 

Isn't it clear?
OK, I'll fix it more

>> +Optional property:
>> +    - flash-timeout-us : Maximum flash timeout in microseconds.
>> +            flash timeout ranges from 0 to 1835000us and
>> +            default is 1049000us.
>> +
>> +Example:
>> +
>> +flash-led {
>> +    compatible = "kinetic,ktd2692";
>> +    ctrl-gpio = <&gpc0 1 0>;
>> +    aux-gpio = <&gpc0 2 0>;
>> +    flash-timeout-us = <1835000>;
>> +    vin-supply = <&vbat>;
>> +};
> 
> 

[PATCH v4 3/3] leds: Add ktd2692 flash LED driver

[Ingi Kim]

This patch adds a driver to support the ktd2692 flash LEDs.
ktd2692 can control flash current by ExpressWire interface.

Signed-off-by: Ingi Kim <ingi2.kim-Sze3O3UU22JBDgjK7y7TUQ@public.gmane.org>
---
 drivers/leds/Kconfig        |   9 +
 drivers/leds/Makefile       |   1 +
 drivers/leds/leds-ktd2692.c | 412 ++++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 422 insertions(+)
 create mode 100644 drivers/leds/leds-ktd2692.c

diff --git a/drivers/leds/Kconfig b/drivers/leds/Kconfig
index 25b320d..9311cfc4 100644
--- a/drivers/leds/Kconfig
+++ b/drivers/leds/Kconfig
@@ -498,6 +498,15 @@ config LEDS_MENF21BMC
 	  This driver can also be built as a module. If so the module
 	  will be called leds-menf21bmc.
 
+config LEDS_KTD2692
+	tristate "Flash LED support for the KTD2692 Driver"
+	depends on LEDS_CLASS_FLASH && GPIOLIB
+	help
+	  This option enables support for the KTD2692 connected through
+	  ExpressWire Interface. Say Y to enabled these.
+	  It depends on LEDS_CLASS_FLASH for using flash led (strobe) and
+	  GPIOLIB for using gpio pin to control Expresswire interface
+
 comment "LED driver for blink(1) USB RGB LED is under Special HID drivers (HID_THINGM)"
 
 config LEDS_BLINKM
diff --git a/drivers/leds/Makefile b/drivers/leds/Makefile
index cbba921..289513b 100644
--- a/drivers/leds/Makefile
+++ b/drivers/leds/Makefile
@@ -58,6 +58,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_KTD2692)		+= leds-ktd2692.o
 
 # LED SPI Drivers
 obj-$(CONFIG_LEDS_DAC124S085)		+= leds-dac124s085.o
diff --git a/drivers/leds/leds-ktd2692.c b/drivers/leds/leds-ktd2692.c
new file mode 100644
index 0000000..c31ec9d
--- /dev/null
+++ b/drivers/leds/leds-ktd2692.c
@@ -0,0 +1,412 @@
+/*
+ * LED driver : leds-ktd2692.c
+ *
+ * Copyright (C) 2015 Samsung Electronics
+ * Ingi Kim <ingi2.kim-Sze3O3UU22JBDgjK7y7TUQ@public.gmane.org>
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License version 2 as
+ * published by the Free Software Foundation.
+ */
+
+#include <linux/delay.h>
+#include <linux/err.h>
+#include <linux/led-class-flash.h>
+#include <linux/module.h>
+#include <linux/mutex.h>
+#include <linux/of_gpio.h>
+#include <linux/platform_device.h>
+#include <linux/regulator/consumer.h>
+#include <linux/workqueue.h>
+
+#define GET_BIT(bit, val)			(((val) >> (bit)) & 0x01)
+
+/* Value related the flash mode */
+#define KTD2692_FLASH_MODE_TIMEOUT_LEVELS	8
+#define KTD2692_FLASH_MODE_TIMEOUT_DISABLE	0
+#define KTD2692_FLASH_MODE_TIMEOUT_DEFAULT_US	1049000
+#define KTD2692_FLASH_MODE_TIMEOUT_MAX_US	1835000
+#define KTD2692_FLASH_MODE_CURR_PERCENT(x)	(((x) * 16) / 100)
+
+/* Macro for getting offset of flash timeout */
+#define GET_TIMEOUT_OFFSET(timeout, step)	((timeout) / (step))
+
+/* Adjust a multiple of brightness */
+#define KTD2692_BRIGHTNESS_RANGE_255_TO_16(x)	(((x) >> 4) & 0x0F)
+#define KTD2692_BRIGHTNESS_RANGE_255_TO_8(x)	(((x) >> 5) & 0x0F)
+#define KTD2692_BRIGHTNESS_RANGE_255_TO_4(x)	(((x) >> 6) & 0x0F)
+
+/* Base register address */
+#define KTD2692_REG_LVP_BASE			0x00
+#define KTD2692_REG_FLASH_TIMEOUT_BASE		0x20
+#define KTD2692_REG_MIN_CURRENT_SET_BASE	0x40
+#define KTD2692_REG_MOVIE_CURRENT_BASE		0x60
+#define KTD2692_REG_FLASH_CURRENT_BASE		0x80
+#define KTD2692_REG_MODE_BASE			0xA0
+
+/* Set bit coding time for expresswire interface */
+#define KTD2692_TIME_RESET_US			700
+#define KTD2692_TIME_DATA_START_TIME_US		10
+#define KTD2692_TIME_HIGH_END_OF_DATA_US	350
+#define KTD2692_TIME_LOW_END_OF_DATA_US		10
+#define KTD2692_TIME_SHORT_BITSET_US		4
+#define KTD2692_TIME_LONG_BITSET_US		12
+
+/* KTD2692 default length of name */
+#define KTD2692_NAME_LENGTH			20
+
+/* KTD2692 default name */
+#define KTD2692_DEFAULT_NAME			"ktd2692"
+
+enum ktd2692_bitset {
+	KTD2692_LOW = 0,
+	KTD2692_HIGH,
+};
+
+/* Movie / Flash Mode Control */
+enum ktd2692_led_mode {
+	KTD2692_MODE_DISABLE = 0,	/* default */
+	KTD2692_MODE_MOVIE,
+	KTD2692_MODE_FLASH,
+};
+
+struct ktd2692_context {
+	/* Related LED Flash class device */
+	struct led_classdev_flash fled_cdev;
+
+	struct mutex lock;
+	struct regulator *regulator;
+	struct work_struct work_brightness_set;
+
+	int aux_gpio;
+	int ctrl_gpio;
+
+	enum ktd2692_led_mode mode;
+	enum led_brightness torch_brightness;
+};
+
+static struct ktd2692_context *fled_cdev_to_led(
+				struct led_classdev_flash *fled_cdev)
+{
+	return container_of(fled_cdev, struct ktd2692_context, fled_cdev);
+}
+
+static void ktd2692_led_regulator_enable(struct ktd2692_context *led)
+{
+	struct led_classdev_flash *fled_cdev = &led->fled_cdev;
+	struct led_classdev *led_cdev = &fled_cdev->led_cdev;
+	int ret;
+
+	if (regulator_is_enabled(led->regulator) > 0)
+		return;
+
+	ret = regulator_enable(led->regulator);
+	if (ret)
+		dev_err(led_cdev->dev, "Failed to enable vin:%d\n", ret);
+}
+
+static void ktd2692_led_regulator_disable(struct ktd2692_context *led)
+{
+	struct led_classdev_flash *fled_cdev = &led->fled_cdev;
+	struct led_classdev *led_cdev = &fled_cdev->led_cdev;
+	int ret;
+
+	if (!regulator_is_enabled(led->regulator))
+		return;
+
+	ret = regulator_disable(led->regulator);
+	if (ret)
+		dev_err(led_cdev->dev, "Failed to disable vin:%d\n", ret);
+}
+
+static void ktd2692_expresswire_start(struct ktd2692_context *led)
+{
+	gpio_set_value(led->ctrl_gpio, KTD2692_HIGH);
+	udelay(KTD2692_TIME_DATA_START_TIME_US);
+}
+
+static void ktd2692_expresswire_reset(struct ktd2692_context *led)
+{
+	gpio_set_value(led->ctrl_gpio, KTD2692_LOW);
+	udelay(KTD2692_TIME_RESET_US);
+}
+
+static void ktd2692_expresswire_end(struct ktd2692_context *led)
+{
+	gpio_set_value(led->ctrl_gpio, KTD2692_LOW);
+	udelay(KTD2692_TIME_LOW_END_OF_DATA_US);
+	gpio_set_value(led->ctrl_gpio, KTD2692_HIGH);
+	udelay(KTD2692_TIME_HIGH_END_OF_DATA_US);
+}
+
+static void ktd2692_expresswire_set_bit(struct ktd2692_context *led, bool bit)
+{
+	if (bit) {
+		gpio_set_value(led->ctrl_gpio, KTD2692_LOW);
+		udelay(KTD2692_TIME_SHORT_BITSET_US);
+		gpio_set_value(led->ctrl_gpio, KTD2692_HIGH);
+		udelay(KTD2692_TIME_LONG_BITSET_US);
+	} else {
+		gpio_set_value(led->ctrl_gpio, KTD2692_LOW);
+		udelay(KTD2692_TIME_LONG_BITSET_US);
+		gpio_set_value(led->ctrl_gpio, KTD2692_HIGH);
+		udelay(KTD2692_TIME_SHORT_BITSET_US);
+	}
+}
+
+static void ktd2692_expresswire_write(struct ktd2692_context *led, u8 value)
+{
+	int i;
+
+	ktd2692_expresswire_start(led);
+	for (i = 7; i >= 0; i--)
+		ktd2692_expresswire_set_bit(led, GET_BIT(i, value));
+	ktd2692_expresswire_end(led);
+}
+
+static void ktd2692_brightness_set(struct ktd2692_context *led,
+					enum led_brightness brightness)
+{
+	mutex_lock(&led->lock);
+
+	if (brightness == LED_OFF) {
+		led->mode = KTD2692_MODE_DISABLE;
+		gpio_set_value(led->aux_gpio, KTD2692_LOW);
+		goto out;
+	}
+
+	ktd2692_expresswire_write(led, KTD2692_REG_MOVIE_CURRENT_BASE |
+				KTD2692_BRIGHTNESS_RANGE_255_TO_8(brightness));
+	led->mode = KTD2692_MODE_MOVIE;
+
+out:
+	ktd2692_expresswire_write(led, led->mode | KTD2692_REG_MODE_BASE);
+	mutex_unlock(&led->lock);
+}
+
+static void ktd2692_brightness_set_work(struct work_struct *work)
+{
+	struct ktd2692_context *led =
+		container_of(work, struct ktd2692_context, work_brightness_set);
+
+	ktd2692_brightness_set(led, led->torch_brightness);
+}
+
+static void ktd2692_led_brightness_set(struct led_classdev *led_cdev,
+					enum led_brightness brightness)
+{
+	struct led_classdev_flash *fled_cdev = lcdev_to_flcdev(led_cdev);
+	struct ktd2692_context *led = fled_cdev_to_led(fled_cdev);
+
+	led->torch_brightness = brightness;
+	schedule_work(&led->work_brightness_set);
+}
+
+static int ktd2692_led_brightness_set_sync(struct led_classdev *led_cdev,
+					enum led_brightness brightness)
+{
+	struct led_classdev_flash *fled_cdev = lcdev_to_flcdev(led_cdev);
+	struct ktd2692_context *led = fled_cdev_to_led(fled_cdev);
+
+	ktd2692_brightness_set(led, brightness);
+
+	return 0;
+}
+
+static int ktd2692_led_flash_strobe_set(struct led_classdev_flash *fled_cdev,
+						bool state)
+{
+	struct ktd2692_context *led = fled_cdev_to_led(fled_cdev);
+	struct led_flash_setting *timeout = &fled_cdev->timeout;
+	u32 flash_tm_reg;
+
+	mutex_lock(&led->lock);
+
+	if (state == 0) {
+		led->mode = KTD2692_MODE_DISABLE;
+		gpio_set_value(led->aux_gpio, KTD2692_LOW);
+		goto done;
+	}
+
+	flash_tm_reg = GET_TIMEOUT_OFFSET(timeout->val, timeout->step);
+	ktd2692_expresswire_write(led, flash_tm_reg
+			| KTD2692_REG_FLASH_TIMEOUT_BASE);
+
+	led->mode = KTD2692_MODE_FLASH;
+	gpio_set_value(led->aux_gpio, KTD2692_HIGH);
+
+done:
+	ktd2692_expresswire_write(led, led->mode | KTD2692_REG_MODE_BASE);
+
+	fled_cdev->led_cdev.brightness = LED_OFF;
+	led->mode = KTD2692_MODE_DISABLE;
+
+	mutex_unlock(&led->lock);
+
+	return 0;
+}
+
+static int ktd2692_led_flash_timeout_set(struct led_classdev_flash *fled_cdev,
+						u32 timeout)
+{
+	return 0;
+}
+
+static void ktd2692_init_flash_timeout(u32 flash_timeout_us,
+					struct led_flash_setting *setting)
+{
+	setting->min = KTD2692_FLASH_MODE_TIMEOUT_DISABLE;
+	setting->max = KTD2692_FLASH_MODE_TIMEOUT_MAX_US;
+	setting->step = KTD2692_FLASH_MODE_TIMEOUT_MAX_US
+			/ (KTD2692_FLASH_MODE_TIMEOUT_LEVELS - 1);
+	setting->val = flash_timeout_us;
+}
+
+static void ktd2692_setup(struct ktd2692_context *led)
+{
+	led->mode = KTD2692_MODE_DISABLE;
+	ktd2692_expresswire_reset(led);
+	gpio_set_value(led->aux_gpio, KTD2692_LOW);
+
+	ktd2692_expresswire_write(led, KTD2692_FLASH_MODE_CURR_PERCENT(45)
+					 | KTD2692_REG_FLASH_CURRENT_BASE);
+}
+
+static int ktd2692_parse_dt(struct ktd2692_context *led, struct device *dev,
+				u32 *flash_timeout_us)
+{
+	struct device_node *np = dev->of_node;
+
+	int ret;
+
+	led->ctrl_gpio = of_get_named_gpio(np, "ctrl-gpio", 0);
+	if (!gpio_is_valid(led->ctrl_gpio)) {
+		dev_err(dev, "no ctrl-gpio property found\n");
+		return -EINVAL;
+	}
+
+	led->aux_gpio = of_get_named_gpio(np, "aux-gpio", 0);
+	if (!gpio_is_valid(led->aux_gpio)) {
+		dev_err(dev, "no aux-gpio property found\n");
+		return -EINVAL;
+	}
+
+	ret = devm_gpio_request_one(dev, led->ctrl_gpio,
+					GPIOF_OPEN_SOURCE, "ctrl-gpio");
+	if (ret) {
+		dev_err(dev, "failed to request ctrl-gpio %d error %d\n",
+				led->ctrl_gpio, ret);
+		return ret;
+	}
+
+	ret = devm_gpio_request_one(dev, led->aux_gpio,
+					GPIOF_OPEN_SOURCE, "aux-gpio");
+	if (ret) {
+		dev_err(dev, "failed to request aux-gpio %d error %d\n",
+				led->aux_gpio, ret);
+		return ret;
+	}
+
+	ret = of_property_read_u32(np, "flash-timeout-us", flash_timeout_us);
+	/* default setting */
+	if (ret)
+		*flash_timeout_us = KTD2692_FLASH_MODE_TIMEOUT_DEFAULT_US;
+
+	return 0;
+}
+
+static const struct led_flash_ops flash_ops = {
+	.strobe_set = ktd2692_led_flash_strobe_set,
+	.timeout_set = ktd2692_led_flash_timeout_set,
+};
+
+static int ktd2692_probe(struct platform_device *pdev)
+{
+	struct ktd2692_context *led;
+	struct led_classdev *led_cdev;
+	struct led_classdev_flash *fled_cdev;
+	struct led_flash_setting flash_timeout;
+	u32 flash_timeout_us;
+	int ret;
+
+	led = devm_kzalloc(&pdev->dev, sizeof(*led), GFP_KERNEL);
+	if (!led)
+		return -ENOMEM;
+
+	if (!pdev->dev.of_node)
+		return -ENXIO;
+
+	fled_cdev = &led->fled_cdev;
+	led_cdev = &fled_cdev->led_cdev;
+
+	ret = ktd2692_parse_dt(led, &pdev->dev, &flash_timeout_us);
+	if (ret)
+		return ret;
+
+	led->regulator = devm_regulator_get(&pdev->dev, "vin");
+	if (IS_ERR(led->regulator)) {
+		dev_err(&pdev->dev, "regulator get failed\n");
+		return PTR_ERR(led->regulator);
+	}
+
+	ktd2692_init_flash_timeout(flash_timeout_us, &flash_timeout);
+
+	fled_cdev->timeout = flash_timeout;
+	fled_cdev->ops = &flash_ops;
+
+	led_cdev->name = KTD2692_DEFAULT_NAME;
+	led_cdev->brightness_set = ktd2692_led_brightness_set;
+	led_cdev->brightness_set_sync = ktd2692_led_brightness_set_sync;
+	led_cdev->flags |= LED_CORE_SUSPENDRESUME;
+	led_cdev->flags |= LED_DEV_CAP_FLASH;
+
+	mutex_init(&led->lock);
+	INIT_WORK(&led->work_brightness_set, ktd2692_brightness_set_work);
+
+	platform_set_drvdata(pdev, led);
+
+	ret = led_classdev_flash_register(&pdev->dev, fled_cdev);
+	if (ret) {
+		dev_err(&pdev->dev, "can't register LED %s\n", led_cdev->name);
+		cancel_work_sync(&led->work_brightness_set);
+		return ret;
+	}
+
+	ktd2692_setup(led);
+	ktd2692_led_regulator_enable(led);
+
+	return 0;
+}
+
+static int ktd2692_remove(struct platform_device *pdev)
+{
+	struct ktd2692_context *led = platform_get_drvdata(pdev);
+
+	ktd2692_led_regulator_disable(led);
+	led_classdev_flash_unregister(&led->fled_cdev);
+	cancel_work_sync(&led->work_brightness_set);
+
+	mutex_destroy(&led->lock);
+
+	return 0;
+}
+
+static const struct of_device_id ktd2692_match[] = {
+	{ .compatible = "kinetic,ktd2692", },
+	{ /* sentinel */ },
+};
+
+static struct platform_driver ktd2692_driver = {
+	.driver = {
+		.name  = "leds-ktd2692",
+		.of_match_table = ktd2692_match,
+	},
+	.probe  = ktd2692_probe,
+	.remove = ktd2692_remove,
+};
+
+module_platform_driver(ktd2692_driver);
+
+MODULE_AUTHOR("Ingi Kim <ingi2.kim-Sze3O3UU22JBDgjK7y7TUQ@public.gmane.org>");
+MODULE_DESCRIPTION("Kinetic KTD2692 LED driver");
+MODULE_LICENSE("GPL v2");
-- 
2.0.5

--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v4 3/3] leds: Add ktd2692 flash LED driver

[Varka Bhadram]

On 03/25/2015 07:00 AM, Ingi Kim wrote:

> This patch adds a driver to support the ktd2692 flash LEDs.
> ktd2692 can control flash current by ExpressWire interface.
>
> Signed-off-by: Ingi Kim <ingi2.kim@samsung.com>
> ---
>   drivers/leds/Kconfig        |   9 +
>   drivers/leds/Makefile       |   1 +
>   drivers/leds/leds-ktd2692.c | 412 ++++++++++++++++++++++++++++++++++++++++++++
>   3 files changed, 422 insertions(+)
>   create mode 100644 drivers/leds/leds-ktd2692.c
>
(...)

> +static int ktd2692_parse_dt(struct ktd2692_context *led, struct device *dev,
> +				u32 *flash_timeout_us)
> +{
> +	struct device_node *np = dev->of_node;
> +

Unnecessary one line space..

> +	int ret;
> +
> +	led->ctrl_gpio = of_get_named_gpio(np, "ctrl-gpio", 0);
> +	if (!gpio_is_valid(led->ctrl_gpio)) {
> +		dev_err(dev, "no ctrl-gpio property found\n");
> +		return -EINVAL;
> +	}
> +
> +	led->aux_gpio = of_get_named_gpio(np, "aux-gpio", 0);
> +	if (!gpio_is_valid(led->aux_gpio)) {
> +		dev_err(dev, "no aux-gpio property found\n");
> +		return -EINVAL;
> +	}
> +
> +	ret = devm_gpio_request_one(dev, led->ctrl_gpio,
> +					GPIOF_OPEN_SOURCE, "ctrl-gpio");
> +	if (ret) {
> +		dev_err(dev, "failed to request ctrl-gpio %d error %d\n",
> +				led->ctrl_gpio, ret);
> +		return ret;
> +	}
> +
> +	ret = devm_gpio_request_one(dev, led->aux_gpio,
> +					GPIOF_OPEN_SOURCE, "aux-gpio");
> +	if (ret) {
> +		dev_err(dev, "failed to request aux-gpio %d error %d\n",
> +				led->aux_gpio, ret);
> +		return ret;
> +	}
> +
> +	ret = of_property_read_u32(np, "flash-timeout-us", flash_timeout_us);
> +	/* default setting */
> +	if (ret)
> +		*flash_timeout_us = KTD2692_FLASH_MODE_TIMEOUT_DEFAULT_US;
> +
> +	return 0;
> +}
> +
> +static const struct led_flash_ops flash_ops = {
> +	.strobe_set = ktd2692_led_flash_strobe_set,
> +	.timeout_set = ktd2692_led_flash_timeout_set,
> +};
> +
> +static int ktd2692_probe(struct platform_device *pdev)
> +{
> +	struct ktd2692_context *led;
> +	struct led_classdev *led_cdev;
> +	struct led_classdev_flash *fled_cdev;
> +	struct led_flash_setting flash_timeout;
> +	u32 flash_timeout_us;
> +	int ret;
> +
> +	led = devm_kzalloc(&pdev->dev, sizeof(*led), GFP_KERNEL);
> +	if (!led)
> +		return -ENOMEM;
> +
> +	if (!pdev->dev.of_node)
> +		return -ENXIO;
> +

Above operation dt related. So if you do that in ktd2692_parse_dt(), it will be good

> +	fled_cdev = &led->fled_cdev;
> +	led_cdev = &fled_cdev->led_cdev;
> +
> +	ret = ktd2692_parse_dt(led, &pdev->dev, &flash_timeout_us);
> +	if (ret)
> +		return ret;
> +
> +	led->regulator = devm_regulator_get(&pdev->dev, "vin");
> +	if (IS_ERR(led->regulator)) {
> +		dev_err(&pdev->dev, "regulator get failed\n");
> +		return PTR_ERR(led->regulator);
> +	}
> +
> +	ktd2692_init_flash_timeout(flash_timeout_us, &flash_timeout);
> +
> +	fled_cdev->timeout = flash_timeout;
> +	fled_cdev->ops = &flash_ops;
> +
> +	led_cdev->name = KTD2692_DEFAULT_NAME;
> +	led_cdev->brightness_set = ktd2692_led_brightness_set;
> +	led_cdev->brightness_set_sync = ktd2692_led_brightness_set_sync;
> +	led_cdev->flags |= LED_CORE_SUSPENDRESUME;
> +	led_cdev->flags |= LED_DEV_CAP_FLASH;
> +
> +	mutex_init(&led->lock);
> +	INIT_WORK(&led->work_brightness_set, ktd2692_brightness_set_work);
> +
> +	platform_set_drvdata(pdev, led);
> +
> +	ret = led_classdev_flash_register(&pdev->dev, fled_cdev);
> +	if (ret) {
> +		dev_err(&pdev->dev, "can't register LED %s\n", led_cdev->name);
> +		cancel_work_sync(&led->work_brightness_set);

Is the above API is correct to use in this place.?

cancel_work_sync — cancel a work and wait for it to finish...

Work is not yet scheduled..?

What about mutex destroy..?

> +		return ret;
> +	}
> +
> +	ktd2692_setup(led);
> +	ktd2692_led_regulator_enable(led);
> +
> +	return 0;
> +}
> +
> +static int ktd2692_remove(struct platform_device *pdev)
> +{
> +	struct ktd2692_context *led = platform_get_drvdata(pdev);
> +
> +	ktd2692_led_regulator_disable(led);
> +	led_classdev_flash_unregister(&led->fled_cdev);
> +	cancel_work_sync(&led->work_brightness_set);
> +
> +	mutex_destroy(&led->lock);
> +
> +	return 0;
> +}
> +
> +static const struct of_device_id ktd2692_match[] = {
> +	{ .compatible = "kinetic,ktd2692", },
> +	{ /* sentinel */ },
> +};
> +
> +static struct platform_driver ktd2692_driver = {
> +	.driver = {
> +		.name  = "leds-ktd2692",
> +		.of_match_table = ktd2692_match,
> +	},
> +	.probe  = ktd2692_probe,
> +	.remove = ktd2692_remove,
> +};
> +
> +module_platform_driver(ktd2692_driver);
> +
> +MODULE_AUTHOR("Ingi Kim <ingi2.kim@samsung.com>");
> +MODULE_DESCRIPTION("Kinetic KTD2692 LED driver");
> +MODULE_LICENSE("GPL v2");


-- 
Varka Bhadram

Re: [PATCH v4 3/3] leds: Add ktd2692 flash LED driver

[Ingi Kim]

Hi Varka,

On 2015년 03월 25일 12:28, Varka Bhadram wrote:
> On 03/25/2015 07:00 AM, Ingi Kim wrote:
> 
>> This patch adds a driver to support the ktd2692 flash LEDs.
>> ktd2692 can control flash current by ExpressWire interface.
>>
>> Signed-off-by: Ingi Kim <ingi2.kim@samsung.com>
>> ---
>>   drivers/leds/Kconfig        |   9 +
>>   drivers/leds/Makefile       |   1 +
>>   drivers/leds/leds-ktd2692.c | 412 ++++++++++++++++++++++++++++++++++++++++++++
>>   3 files changed, 422 insertions(+)
>>   create mode 100644 drivers/leds/leds-ktd2692.c
>>
> (...)
> 
>> +static int ktd2692_parse_dt(struct ktd2692_context *led, struct device *dev,
>> +                u32 *flash_timeout_us)
>> +{
>> +    struct device_node *np = dev->of_node;
>> +
> 
> Unnecessary one line space..
> 

Oh, I missed it! Thanks

>> +    int ret;
>> +
>> +    led->ctrl_gpio = of_get_named_gpio(np, "ctrl-gpio", 0);
>> +    if (!gpio_is_valid(led->ctrl_gpio)) {
>> +        dev_err(dev, "no ctrl-gpio property found\n");
>> +        return -EINVAL;
>> +    }
>> +
>> +    led->aux_gpio = of_get_named_gpio(np, "aux-gpio", 0);
>> +    if (!gpio_is_valid(led->aux_gpio)) {
>> +        dev_err(dev, "no aux-gpio property found\n");
>> +        return -EINVAL;
>> +    }
>> +
>> +    ret = devm_gpio_request_one(dev, led->ctrl_gpio,
>> +                    GPIOF_OPEN_SOURCE, "ctrl-gpio");
>> +    if (ret) {
>> +        dev_err(dev, "failed to request ctrl-gpio %d error %d\n",
>> +                led->ctrl_gpio, ret);
>> +        return ret;
>> +    }
>> +
>> +    ret = devm_gpio_request_one(dev, led->aux_gpio,
>> +                    GPIOF_OPEN_SOURCE, "aux-gpio");
>> +    if (ret) {
>> +        dev_err(dev, "failed to request aux-gpio %d error %d\n",
>> +                led->aux_gpio, ret);
>> +        return ret;
>> +    }
>> +
>> +    ret = of_property_read_u32(np, "flash-timeout-us", flash_timeout_us);
>> +    /* default setting */
>> +    if (ret)
>> +        *flash_timeout_us = KTD2692_FLASH_MODE_TIMEOUT_DEFAULT_US;
>> +
>> +    return 0;
>> +}
>> +
>> +static const struct led_flash_ops flash_ops = {
>> +    .strobe_set = ktd2692_led_flash_strobe_set,
>> +    .timeout_set = ktd2692_led_flash_timeout_set,
>> +};
>> +
>> +static int ktd2692_probe(struct platform_device *pdev)
>> +{
>> +    struct ktd2692_context *led;
>> +    struct led_classdev *led_cdev;
>> +    struct led_classdev_flash *fled_cdev;
>> +    struct led_flash_setting flash_timeout;
>> +    u32 flash_timeout_us;
>> +    int ret;
>> +
>> +    led = devm_kzalloc(&pdev->dev, sizeof(*led), GFP_KERNEL);
>> +    if (!led)
>> +        return -ENOMEM;
>> +
>> +    if (!pdev->dev.of_node)
>> +        return -ENXIO;
>> +
> 
> Above operation dt related. So if you do that in ktd2692_parse_dt(), it will be good
> 

Good point!
I'll do that

>> +    fled_cdev = &led->fled_cdev;
>> +    led_cdev = &fled_cdev->led_cdev;
>> +
>> +    ret = ktd2692_parse_dt(led, &pdev->dev, &flash_timeout_us);
>> +    if (ret)
>> +        return ret;
>> +
>> +    led->regulator = devm_regulator_get(&pdev->dev, "vin");
>> +    if (IS_ERR(led->regulator)) {
>> +        dev_err(&pdev->dev, "regulator get failed\n");
>> +        return PTR_ERR(led->regulator);
>> +    }
>> +
>> +    ktd2692_init_flash_timeout(flash_timeout_us, &flash_timeout);
>> +
>> +    fled_cdev->timeout = flash_timeout;
>> +    fled_cdev->ops = &flash_ops;
>> +
>> +    led_cdev->name = KTD2692_DEFAULT_NAME;
>> +    led_cdev->brightness_set = ktd2692_led_brightness_set;
>> +    led_cdev->brightness_set_sync = ktd2692_led_brightness_set_sync;
>> +    led_cdev->flags |= LED_CORE_SUSPENDRESUME;
>> +    led_cdev->flags |= LED_DEV_CAP_FLASH;
>> +
>> +    mutex_init(&led->lock);
>> +    INIT_WORK(&led->work_brightness_set, ktd2692_brightness_set_work);
>> +
>> +    platform_set_drvdata(pdev, led);
>> +
>> +    ret = led_classdev_flash_register(&pdev->dev, fled_cdev);
>> +    if (ret) {
>> +        dev_err(&pdev->dev, "can't register LED %s\n", led_cdev->name);
>> +        cancel_work_sync(&led->work_brightness_set);
> 
> Is the above API is correct to use in this place.?
> 
> cancel_work_sync — cancel a work and wait for it to finish...
> 
> Work is not yet scheduled..?
> 
> What about mutex destroy..?
> 

Right, I should remove this API.
It seems to be useless in this place

And mutex_destroy() will be added, Thank you!

>> +        return ret;
>> +    }
>> +
>> +    ktd2692_setup(led);
>> +    ktd2692_led_regulator_enable(led);
>> +
>> +    return 0;
>> +}
>> +
>> +static int ktd2692_remove(struct platform_device *pdev)
>> +{
>> +    struct ktd2692_context *led = platform_get_drvdata(pdev);
>> +
>> +    ktd2692_led_regulator_disable(led);
>> +    led_classdev_flash_unregister(&led->fled_cdev);
>> +    cancel_work_sync(&led->work_brightness_set);
>> +
>> +    mutex_destroy(&led->lock);
>> +
>> +    return 0;
>> +}
>> +
>> +static const struct of_device_id ktd2692_match[] = {
>> +    { .compatible = "kinetic,ktd2692", },
>> +    { /* sentinel */ },
>> +};
>> +
>> +static struct platform_driver ktd2692_driver = {
>> +    .driver = {
>> +        .name  = "leds-ktd2692",
>> +        .of_match_table = ktd2692_match,
>> +    },
>> +    .probe  = ktd2692_probe,
>> +    .remove = ktd2692_remove,
>> +};
>> +
>> +module_platform_driver(ktd2692_driver);
>> +
>> +MODULE_AUTHOR("Ingi Kim <ingi2.kim@samsung.com>");
>> +MODULE_DESCRIPTION("Kinetic KTD2692 LED driver");
>> +MODULE_LICENSE("GPL v2");
> 
> 

Re: [PATCH v4 3/3] leds: Add ktd2692 flash LED driver

[Sakari Ailus]

Hi Ingi,

Thank you for the patch.

On Wed, Mar 25, 2015 at 10:30:44AM +0900, Ingi Kim wrote:
> This patch adds a driver to support the ktd2692 flash LEDs.
> ktd2692 can control flash current by ExpressWire interface.
> 
> Signed-off-by: Ingi Kim <ingi2.kim-Sze3O3UU22JBDgjK7y7TUQ@public.gmane.org>
> ---
>  drivers/leds/Kconfig        |   9 +
>  drivers/leds/Makefile       |   1 +
>  drivers/leds/leds-ktd2692.c | 412 ++++++++++++++++++++++++++++++++++++++++++++
>  3 files changed, 422 insertions(+)
>  create mode 100644 drivers/leds/leds-ktd2692.c
> 
> diff --git a/drivers/leds/Kconfig b/drivers/leds/Kconfig
> index 25b320d..9311cfc4 100644
> --- a/drivers/leds/Kconfig
> +++ b/drivers/leds/Kconfig
> @@ -498,6 +498,15 @@ config LEDS_MENF21BMC
>  	  This driver can also be built as a module. If so the module
>  	  will be called leds-menf21bmc.
>  
> +config LEDS_KTD2692
> +	tristate "Flash LED support for the KTD2692 Driver"

Does ktd2692 act as something else as well? If not, how about "KTD2692 LED
flash support"?

> +	depends on LEDS_CLASS_FLASH && GPIOLIB
> +	help
> +	  This option enables support for the KTD2692 connected through
> +	  ExpressWire Interface. Say Y to enabled these.
> +	  It depends on LEDS_CLASS_FLASH for using flash led (strobe) and
> +	  GPIOLIB for using gpio pin to control Expresswire interface

The dependencies are shown by make *config, I would drop them from here.

> +
>  comment "LED driver for blink(1) USB RGB LED is under Special HID drivers (HID_THINGM)"
>  
>  config LEDS_BLINKM
> diff --git a/drivers/leds/Makefile b/drivers/leds/Makefile
> index cbba921..289513b 100644
> --- a/drivers/leds/Makefile
> +++ b/drivers/leds/Makefile
> @@ -58,6 +58,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_KTD2692)		+= leds-ktd2692.o
>  
>  # LED SPI Drivers
>  obj-$(CONFIG_LEDS_DAC124S085)		+= leds-dac124s085.o
> diff --git a/drivers/leds/leds-ktd2692.c b/drivers/leds/leds-ktd2692.c
> new file mode 100644
> index 0000000..c31ec9d
> --- /dev/null
> +++ b/drivers/leds/leds-ktd2692.c
> @@ -0,0 +1,412 @@
> +/*
> + * LED driver : leds-ktd2692.c
> + *
> + * Copyright (C) 2015 Samsung Electronics
> + * Ingi Kim <ingi2.kim-Sze3O3UU22JBDgjK7y7TUQ@public.gmane.org>
> + *
> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License version 2 as
> + * published by the Free Software Foundation.
> + */
> +
> +#include <linux/delay.h>
> +#include <linux/err.h>
> +#include <linux/led-class-flash.h>
> +#include <linux/module.h>
> +#include <linux/mutex.h>
> +#include <linux/of_gpio.h>
> +#include <linux/platform_device.h>
> +#include <linux/regulator/consumer.h>
> +#include <linux/workqueue.h>
> +
> +#define GET_BIT(bit, val)			(((val) >> (bit)) & 0x01)
> +
> +/* Value related the flash mode */
> +#define KTD2692_FLASH_MODE_TIMEOUT_LEVELS	8
> +#define KTD2692_FLASH_MODE_TIMEOUT_DISABLE	0
> +#define KTD2692_FLASH_MODE_TIMEOUT_DEFAULT_US	1049000
> +#define KTD2692_FLASH_MODE_TIMEOUT_MAX_US	1835000
> +#define KTD2692_FLASH_MODE_CURR_PERCENT(x)	(((x) * 16) / 100)
> +
> +/* Macro for getting offset of flash timeout */
> +#define GET_TIMEOUT_OFFSET(timeout, step)	((timeout) / (step))
> +
> +/* Adjust a multiple of brightness */
> +#define KTD2692_BRIGHTNESS_RANGE_255_TO_16(x)	(((x) >> 4) & 0x0F)
> +#define KTD2692_BRIGHTNESS_RANGE_255_TO_8(x)	(((x) >> 5) & 0x0F)
> +#define KTD2692_BRIGHTNESS_RANGE_255_TO_4(x)	(((x) >> 6) & 0x0F)
> +
> +/* Base register address */
> +#define KTD2692_REG_LVP_BASE			0x00
> +#define KTD2692_REG_FLASH_TIMEOUT_BASE		0x20
> +#define KTD2692_REG_MIN_CURRENT_SET_BASE	0x40
> +#define KTD2692_REG_MOVIE_CURRENT_BASE		0x60
> +#define KTD2692_REG_FLASH_CURRENT_BASE		0x80
> +#define KTD2692_REG_MODE_BASE			0xA0
> +
> +/* Set bit coding time for expresswire interface */
> +#define KTD2692_TIME_RESET_US			700
> +#define KTD2692_TIME_DATA_START_TIME_US		10
> +#define KTD2692_TIME_HIGH_END_OF_DATA_US	350
> +#define KTD2692_TIME_LOW_END_OF_DATA_US		10
> +#define KTD2692_TIME_SHORT_BITSET_US		4
> +#define KTD2692_TIME_LONG_BITSET_US		12
> +
> +/* KTD2692 default length of name */
> +#define KTD2692_NAME_LENGTH			20
> +
> +/* KTD2692 default name */
> +#define KTD2692_DEFAULT_NAME			"ktd2692"
> +
> +enum ktd2692_bitset {
> +	KTD2692_LOW = 0,
> +	KTD2692_HIGH,
> +};
> +
> +/* Movie / Flash Mode Control */
> +enum ktd2692_led_mode {
> +	KTD2692_MODE_DISABLE = 0,	/* default */
> +	KTD2692_MODE_MOVIE,
> +	KTD2692_MODE_FLASH,
> +};
> +
> +struct ktd2692_context {
> +	/* Related LED Flash class device */
> +	struct led_classdev_flash fled_cdev;
> +
> +	struct mutex lock;
> +	struct regulator *regulator;
> +	struct work_struct work_brightness_set;
> +
> +	int aux_gpio;
> +	int ctrl_gpio;
> +
> +	enum ktd2692_led_mode mode;
> +	enum led_brightness torch_brightness;
> +};
> +
> +static struct ktd2692_context *fled_cdev_to_led(
> +				struct led_classdev_flash *fled_cdev)
> +{
> +	return container_of(fled_cdev, struct ktd2692_context, fled_cdev);
> +}
> +
> +static void ktd2692_led_regulator_enable(struct ktd2692_context *led)
> +{
> +	struct led_classdev_flash *fled_cdev = &led->fled_cdev;
> +	struct led_classdev *led_cdev = &fled_cdev->led_cdev;
> +	int ret;
> +
> +	if (regulator_is_enabled(led->regulator) > 0)
> +		return;

What's the purpose of this? Why not to just call regulator_enable() instead?

This way you could easily mess up with other users of the same regulator.

> +
> +	ret = regulator_enable(led->regulator);
> +	if (ret)
> +		dev_err(led_cdev->dev, "Failed to enable vin:%d\n", ret);
> +}
> +
> +static void ktd2692_led_regulator_disable(struct ktd2692_context *led)
> +{
> +	struct led_classdev_flash *fled_cdev = &led->fled_cdev;
> +	struct led_classdev *led_cdev = &fled_cdev->led_cdev;
> +	int ret;
> +
> +	if (!regulator_is_enabled(led->regulator))
> +		return;
> +
> +	ret = regulator_disable(led->regulator);
> +	if (ret)
> +		dev_err(led_cdev->dev, "Failed to disable vin:%d\n", ret);
> +}
> +
> +static void ktd2692_expresswire_start(struct ktd2692_context *led)
> +{
> +	gpio_set_value(led->ctrl_gpio, KTD2692_HIGH);
> +	udelay(KTD2692_TIME_DATA_START_TIME_US);
> +}
> +
> +static void ktd2692_expresswire_reset(struct ktd2692_context *led)
> +{
> +	gpio_set_value(led->ctrl_gpio, KTD2692_LOW);
> +	udelay(KTD2692_TIME_RESET_US);
> +}
> +
> +static void ktd2692_expresswire_end(struct ktd2692_context *led)
> +{
> +	gpio_set_value(led->ctrl_gpio, KTD2692_LOW);
> +	udelay(KTD2692_TIME_LOW_END_OF_DATA_US);
> +	gpio_set_value(led->ctrl_gpio, KTD2692_HIGH);
> +	udelay(KTD2692_TIME_HIGH_END_OF_DATA_US);
> +}
> +
> +static void ktd2692_expresswire_set_bit(struct ktd2692_context *led, bool bit)
> +{
> +	if (bit) {
> +		gpio_set_value(led->ctrl_gpio, KTD2692_LOW);
> +		udelay(KTD2692_TIME_SHORT_BITSET_US);
> +		gpio_set_value(led->ctrl_gpio, KTD2692_HIGH);
> +		udelay(KTD2692_TIME_LONG_BITSET_US);
> +	} else {
> +		gpio_set_value(led->ctrl_gpio, KTD2692_LOW);
> +		udelay(KTD2692_TIME_LONG_BITSET_US);
> +		gpio_set_value(led->ctrl_gpio, KTD2692_HIGH);
> +		udelay(KTD2692_TIME_SHORT_BITSET_US);
> +	}
> +}
> +
> +static void ktd2692_expresswire_write(struct ktd2692_context *led, u8 value)
> +{
> +	int i;
> +
> +	ktd2692_expresswire_start(led);
> +	for (i = 7; i >= 0; i--)
> +		ktd2692_expresswire_set_bit(led, GET_BIT(i, value));

I think this would be cleaner as:

value & BIT(i)

Casting an integer as bool will result in true if it's non-zero, so you
don't need to cast.

> +	ktd2692_expresswire_end(led);
> +}
> +
> +static void ktd2692_brightness_set(struct ktd2692_context *led,
> +					enum led_brightness brightness)
> +{
> +	mutex_lock(&led->lock);
> +
> +	if (brightness == LED_OFF) {
> +		led->mode = KTD2692_MODE_DISABLE;
> +		gpio_set_value(led->aux_gpio, KTD2692_LOW);
> +		goto out;
> +	}
> +
> +	ktd2692_expresswire_write(led, KTD2692_REG_MOVIE_CURRENT_BASE |
> +				KTD2692_BRIGHTNESS_RANGE_255_TO_8(brightness));
> +	led->mode = KTD2692_MODE_MOVIE;
> +
> +out:
> +	ktd2692_expresswire_write(led, led->mode | KTD2692_REG_MODE_BASE);
> +	mutex_unlock(&led->lock);
> +}
> +
> +static void ktd2692_brightness_set_work(struct work_struct *work)
> +{
> +	struct ktd2692_context *led =
> +		container_of(work, struct ktd2692_context, work_brightness_set);
> +
> +	ktd2692_brightness_set(led, led->torch_brightness);
> +}
> +
> +static void ktd2692_led_brightness_set(struct led_classdev *led_cdev,
> +					enum led_brightness brightness)
> +{
> +	struct led_classdev_flash *fled_cdev = lcdev_to_flcdev(led_cdev);
> +	struct ktd2692_context *led = fled_cdev_to_led(fled_cdev);
> +
> +	led->torch_brightness = brightness;
> +	schedule_work(&led->work_brightness_set);
> +}
> +
> +static int ktd2692_led_brightness_set_sync(struct led_classdev *led_cdev,
> +					enum led_brightness brightness)
> +{
> +	struct led_classdev_flash *fled_cdev = lcdev_to_flcdev(led_cdev);
> +	struct ktd2692_context *led = fled_cdev_to_led(fled_cdev);
> +
> +	ktd2692_brightness_set(led, brightness);
> +
> +	return 0;
> +}
> +
> +static int ktd2692_led_flash_strobe_set(struct led_classdev_flash *fled_cdev,
> +						bool state)
> +{
> +	struct ktd2692_context *led = fled_cdev_to_led(fled_cdev);
> +	struct led_flash_setting *timeout = &fled_cdev->timeout;
> +	u32 flash_tm_reg;
> +
> +	mutex_lock(&led->lock);
> +
> +	if (state == 0) {
> +		led->mode = KTD2692_MODE_DISABLE;
> +		gpio_set_value(led->aux_gpio, KTD2692_LOW);
> +		goto done;
> +	}
> +
> +	flash_tm_reg = GET_TIMEOUT_OFFSET(timeout->val, timeout->step);
> +	ktd2692_expresswire_write(led, flash_tm_reg
> +			| KTD2692_REG_FLASH_TIMEOUT_BASE);
> +
> +	led->mode = KTD2692_MODE_FLASH;
> +	gpio_set_value(led->aux_gpio, KTD2692_HIGH);
> +
> +done:
> +	ktd2692_expresswire_write(led, led->mode | KTD2692_REG_MODE_BASE);
> +
> +	fled_cdev->led_cdev.brightness = LED_OFF;
> +	led->mode = KTD2692_MODE_DISABLE;
> +
> +	mutex_unlock(&led->lock);
> +
> +	return 0;
> +}
> +
> +static int ktd2692_led_flash_timeout_set(struct led_classdev_flash *fled_cdev,
> +						u32 timeout)
> +{
> +	return 0;
> +}
> +
> +static void ktd2692_init_flash_timeout(u32 flash_timeout_us,
> +					struct led_flash_setting *setting)
> +{
> +	setting->min = KTD2692_FLASH_MODE_TIMEOUT_DISABLE;
> +	setting->max = KTD2692_FLASH_MODE_TIMEOUT_MAX_US;
> +	setting->step = KTD2692_FLASH_MODE_TIMEOUT_MAX_US
> +			/ (KTD2692_FLASH_MODE_TIMEOUT_LEVELS - 1);
> +	setting->val = flash_timeout_us;
> +}
> +
> +static void ktd2692_setup(struct ktd2692_context *led)
> +{
> +	led->mode = KTD2692_MODE_DISABLE;
> +	ktd2692_expresswire_reset(led);
> +	gpio_set_value(led->aux_gpio, KTD2692_LOW);
> +
> +	ktd2692_expresswire_write(led, KTD2692_FLASH_MODE_CURR_PERCENT(45)
> +					 | KTD2692_REG_FLASH_CURRENT_BASE);
> +}
> +
> +static int ktd2692_parse_dt(struct ktd2692_context *led, struct device *dev,
> +				u32 *flash_timeout_us)
> +{
> +	struct device_node *np = dev->of_node;
> +
> +	int ret;
> +
> +	led->ctrl_gpio = of_get_named_gpio(np, "ctrl-gpio", 0);
> +	if (!gpio_is_valid(led->ctrl_gpio)) {
> +		dev_err(dev, "no ctrl-gpio property found\n");
> +		return -EINVAL;
> +	}
> +
> +	led->aux_gpio = of_get_named_gpio(np, "aux-gpio", 0);
> +	if (!gpio_is_valid(led->aux_gpio)) {
> +		dev_err(dev, "no aux-gpio property found\n");
> +		return -EINVAL;
> +	}
> +
> +	ret = devm_gpio_request_one(dev, led->ctrl_gpio,
> +					GPIOF_OPEN_SOURCE, "ctrl-gpio");
> +	if (ret) {
> +		dev_err(dev, "failed to request ctrl-gpio %d error %d\n",
> +				led->ctrl_gpio, ret);
> +		return ret;
> +	}
> +
> +	ret = devm_gpio_request_one(dev, led->aux_gpio,
> +					GPIOF_OPEN_SOURCE, "aux-gpio");
> +	if (ret) {
> +		dev_err(dev, "failed to request aux-gpio %d error %d\n",
> +				led->aux_gpio, ret);
> +		return ret;
> +	}
> +
> +	ret = of_property_read_u32(np, "flash-timeout-us", flash_timeout_us);
> +	/* default setting */
> +	if (ret)
> +		*flash_timeout_us = KTD2692_FLASH_MODE_TIMEOUT_DEFAULT_US;
> +
> +	return 0;
> +}
> +
> +static const struct led_flash_ops flash_ops = {
> +	.strobe_set = ktd2692_led_flash_strobe_set,
> +	.timeout_set = ktd2692_led_flash_timeout_set,
> +};
> +
> +static int ktd2692_probe(struct platform_device *pdev)
> +{
> +	struct ktd2692_context *led;
> +	struct led_classdev *led_cdev;
> +	struct led_classdev_flash *fled_cdev;
> +	struct led_flash_setting flash_timeout;
> +	u32 flash_timeout_us;
> +	int ret;
> +
> +	led = devm_kzalloc(&pdev->dev, sizeof(*led), GFP_KERNEL);
> +	if (!led)
> +		return -ENOMEM;
> +
> +	if (!pdev->dev.of_node)
> +		return -ENXIO;
> +
> +	fled_cdev = &led->fled_cdev;
> +	led_cdev = &fled_cdev->led_cdev;
> +
> +	ret = ktd2692_parse_dt(led, &pdev->dev, &flash_timeout_us);
> +	if (ret)
> +		return ret;
> +
> +	led->regulator = devm_regulator_get(&pdev->dev, "vin");
> +	if (IS_ERR(led->regulator)) {
> +		dev_err(&pdev->dev, "regulator get failed\n");
> +		return PTR_ERR(led->regulator);
> +	}
> +
> +	ktd2692_init_flash_timeout(flash_timeout_us, &flash_timeout);
> +
> +	fled_cdev->timeout = flash_timeout;
> +	fled_cdev->ops = &flash_ops;
> +
> +	led_cdev->name = KTD2692_DEFAULT_NAME;
> +	led_cdev->brightness_set = ktd2692_led_brightness_set;
> +	led_cdev->brightness_set_sync = ktd2692_led_brightness_set_sync;
> +	led_cdev->flags |= LED_CORE_SUSPENDRESUME;
> +	led_cdev->flags |= LED_DEV_CAP_FLASH;

You could unify the above two lines.

> +
> +	mutex_init(&led->lock);
> +	INIT_WORK(&led->work_brightness_set, ktd2692_brightness_set_work);
> +
> +	platform_set_drvdata(pdev, led);
> +
> +	ret = led_classdev_flash_register(&pdev->dev, fled_cdev);
> +	if (ret) {
> +		dev_err(&pdev->dev, "can't register LED %s\n", led_cdev->name);

You should do mutex_destroy() here.

> +		cancel_work_sync(&led->work_brightness_set);
> +		return ret;
> +	}
> +

This is a LED flash device. Do you intend to add support for the V4L2 flash
API as well?

> +	ktd2692_setup(led);
> +	ktd2692_led_regulator_enable(led);

Hmm. I guess the regulator was already enabled, assuming you have tested
this. :-)

> +
> +	return 0;
> +}
> +
> +static int ktd2692_remove(struct platform_device *pdev)
> +{
> +	struct ktd2692_context *led = platform_get_drvdata(pdev);
> +
> +	ktd2692_led_regulator_disable(led);
> +	led_classdev_flash_unregister(&led->fled_cdev);
> +	cancel_work_sync(&led->work_brightness_set);
> +
> +	mutex_destroy(&led->lock);
> +
> +	return 0;
> +}
> +
> +static const struct of_device_id ktd2692_match[] = {
> +	{ .compatible = "kinetic,ktd2692", },
> +	{ /* sentinel */ },
> +};
> +
> +static struct platform_driver ktd2692_driver = {
> +	.driver = {
> +		.name  = "leds-ktd2692",
> +		.of_match_table = ktd2692_match,
> +	},
> +	.probe  = ktd2692_probe,
> +	.remove = ktd2692_remove,
> +};
> +
> +module_platform_driver(ktd2692_driver);
> +
> +MODULE_AUTHOR("Ingi Kim <ingi2.kim-Sze3O3UU22JBDgjK7y7TUQ@public.gmane.org>");
> +MODULE_DESCRIPTION("Kinetic KTD2692 LED driver");
> +MODULE_LICENSE("GPL v2");

-- 
Kind regards,

Sakari Ailus
e-mail: sakari.ailus-X3B1VOXEql0@public.gmane.org	XMPP: sailus-PCDdDYkjdNMDXYZnReoRVg@public.gmane.org
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v4 3/3] leds: Add ktd2692 flash LED driver

[Ingi Kim]

Hi Sakari,

Thanks for the review

On 2015년 03월 25일 22:53, Sakari Ailus wrote:
> Hi Ingi,
> 
> Thank you for the patch.
> 
> On Wed, Mar 25, 2015 at 10:30:44AM +0900, Ingi Kim wrote:
>> This patch adds a driver to support the ktd2692 flash LEDs.
>> ktd2692 can control flash current by ExpressWire interface.
>>
>> Signed-off-by: Ingi Kim <ingi2.kim@samsung.com>
>> ---
>>  drivers/leds/Kconfig        |   9 +
>>  drivers/leds/Makefile       |   1 +
>>  drivers/leds/leds-ktd2692.c | 412 ++++++++++++++++++++++++++++++++++++++++++++
>>  3 files changed, 422 insertions(+)
>>  create mode 100644 drivers/leds/leds-ktd2692.c
>>
>> diff --git a/drivers/leds/Kconfig b/drivers/leds/Kconfig
>> index 25b320d..9311cfc4 100644
>> --- a/drivers/leds/Kconfig
>> +++ b/drivers/leds/Kconfig
>> @@ -498,6 +498,15 @@ config LEDS_MENF21BMC
>>  	  This driver can also be built as a module. If so the module
>>  	  will be called leds-menf21bmc.
>>  
>> +config LEDS_KTD2692
>> +	tristate "Flash LED support for the KTD2692 Driver"
> 
> Does ktd2692 act as something else as well? If not, how about "KTD2692 LED
> flash support"?
> 

Right, KTD2692 driver acts just for flash led.

>> +	depends on LEDS_CLASS_FLASH && GPIOLIB
>> +	help
>> +	  This option enables support for the KTD2692 connected through
>> +	  ExpressWire Interface. Say Y to enabled these.
>> +	  It depends on LEDS_CLASS_FLASH for using flash led (strobe) and
>> +	  GPIOLIB for using gpio pin to control Expresswire interface
> 
> The dependencies are shown by make *config, I would drop them from here.
> 

Did you mean help message about dependencies? or config (LEDS_CLASS_FLASH, GPIOLIB)
if you mean latter, I don't know exactly what you say.
if it wasn't, I should drop superfluous help message

>> +
>>  comment "LED driver for blink(1) USB RGB LED is under Special HID drivers (HID_THINGM)"
>>  
>>  config LEDS_BLINKM
>> diff --git a/drivers/leds/Makefile b/drivers/leds/Makefile
>> index cbba921..289513b 100644
>> --- a/drivers/leds/Makefile
>> +++ b/drivers/leds/Makefile
>> @@ -58,6 +58,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_KTD2692)		+= leds-ktd2692.o
>>  
>>  # LED SPI Drivers
>>  obj-$(CONFIG_LEDS_DAC124S085)		+= leds-dac124s085.o
>> diff --git a/drivers/leds/leds-ktd2692.c b/drivers/leds/leds-ktd2692.c
>> new file mode 100644
>> index 0000000..c31ec9d
>> --- /dev/null
>> +++ b/drivers/leds/leds-ktd2692.c
>> @@ -0,0 +1,412 @@
>> +/*
>> + * LED driver : leds-ktd2692.c
>> + *
>> + * Copyright (C) 2015 Samsung Electronics
>> + * Ingi Kim <ingi2.kim@samsung.com>
>> + *
>> + * This program is free software; you can redistribute it and/or modify
>> + * it under the terms of the GNU General Public License version 2 as
>> + * published by the Free Software Foundation.
>> + */
>> +
>> +#include <linux/delay.h>
>> +#include <linux/err.h>
>> +#include <linux/led-class-flash.h>
>> +#include <linux/module.h>
>> +#include <linux/mutex.h>
>> +#include <linux/of_gpio.h>
>> +#include <linux/platform_device.h>
>> +#include <linux/regulator/consumer.h>
>> +#include <linux/workqueue.h>
>> +
>> +#define GET_BIT(bit, val)			(((val) >> (bit)) & 0x01)
>> +
>> +/* Value related the flash mode */
>> +#define KTD2692_FLASH_MODE_TIMEOUT_LEVELS	8
>> +#define KTD2692_FLASH_MODE_TIMEOUT_DISABLE	0
>> +#define KTD2692_FLASH_MODE_TIMEOUT_DEFAULT_US	1049000
>> +#define KTD2692_FLASH_MODE_TIMEOUT_MAX_US	1835000
>> +#define KTD2692_FLASH_MODE_CURR_PERCENT(x)	(((x) * 16) / 100)
>> +
>> +/* Macro for getting offset of flash timeout */
>> +#define GET_TIMEOUT_OFFSET(timeout, step)	((timeout) / (step))
>> +
>> +/* Adjust a multiple of brightness */
>> +#define KTD2692_BRIGHTNESS_RANGE_255_TO_16(x)	(((x) >> 4) & 0x0F)
>> +#define KTD2692_BRIGHTNESS_RANGE_255_TO_8(x)	(((x) >> 5) & 0x0F)
>> +#define KTD2692_BRIGHTNESS_RANGE_255_TO_4(x)	(((x) >> 6) & 0x0F)
>> +
>> +/* Base register address */
>> +#define KTD2692_REG_LVP_BASE			0x00
>> +#define KTD2692_REG_FLASH_TIMEOUT_BASE		0x20
>> +#define KTD2692_REG_MIN_CURRENT_SET_BASE	0x40
>> +#define KTD2692_REG_MOVIE_CURRENT_BASE		0x60
>> +#define KTD2692_REG_FLASH_CURRENT_BASE		0x80
>> +#define KTD2692_REG_MODE_BASE			0xA0
>> +
>> +/* Set bit coding time for expresswire interface */
>> +#define KTD2692_TIME_RESET_US			700
>> +#define KTD2692_TIME_DATA_START_TIME_US		10
>> +#define KTD2692_TIME_HIGH_END_OF_DATA_US	350
>> +#define KTD2692_TIME_LOW_END_OF_DATA_US		10
>> +#define KTD2692_TIME_SHORT_BITSET_US		4
>> +#define KTD2692_TIME_LONG_BITSET_US		12
>> +
>> +/* KTD2692 default length of name */
>> +#define KTD2692_NAME_LENGTH			20
>> +
>> +/* KTD2692 default name */
>> +#define KTD2692_DEFAULT_NAME			"ktd2692"
>> +
>> +enum ktd2692_bitset {
>> +	KTD2692_LOW = 0,
>> +	KTD2692_HIGH,
>> +};
>> +
>> +/* Movie / Flash Mode Control */
>> +enum ktd2692_led_mode {
>> +	KTD2692_MODE_DISABLE = 0,	/* default */
>> +	KTD2692_MODE_MOVIE,
>> +	KTD2692_MODE_FLASH,
>> +};
>> +
>> +struct ktd2692_context {
>> +	/* Related LED Flash class device */
>> +	struct led_classdev_flash fled_cdev;
>> +
>> +	struct mutex lock;
>> +	struct regulator *regulator;
>> +	struct work_struct work_brightness_set;
>> +
>> +	int aux_gpio;
>> +	int ctrl_gpio;
>> +
>> +	enum ktd2692_led_mode mode;
>> +	enum led_brightness torch_brightness;
>> +};
>> +
>> +static struct ktd2692_context *fled_cdev_to_led(
>> +				struct led_classdev_flash *fled_cdev)
>> +{
>> +	return container_of(fled_cdev, struct ktd2692_context, fled_cdev);
>> +}
>> +
>> +static void ktd2692_led_regulator_enable(struct ktd2692_context *led)
>> +{
>> +	struct led_classdev_flash *fled_cdev = &led->fled_cdev;
>> +	struct led_classdev *led_cdev = &fled_cdev->led_cdev;
>> +	int ret;
>> +
>> +	if (regulator_is_enabled(led->regulator) > 0)
>> +		return;
> 
> What's the purpose of this? Why not to just call regulator_enable() instead?
> 
> This way you could easily mess up with other users of the same regulator.
> 

I want to harmonize all APIs in here...

but I'd better simplify code if it would make a mess

>> +
>> +	ret = regulator_enable(led->regulator);
>> +	if (ret)
>> +		dev_err(led_cdev->dev, "Failed to enable vin:%d\n", ret);
>> +}
>> +
>> +static void ktd2692_led_regulator_disable(struct ktd2692_context *led)
>> +{
>> +	struct led_classdev_flash *fled_cdev = &led->fled_cdev;
>> +	struct led_classdev *led_cdev = &fled_cdev->led_cdev;
>> +	int ret;
>> +
>> +	if (!regulator_is_enabled(led->regulator))
>> +		return;
>> +
>> +	ret = regulator_disable(led->regulator);
>> +	if (ret)
>> +		dev_err(led_cdev->dev, "Failed to disable vin:%d\n", ret);
>> +}
>> +
>> +static void ktd2692_expresswire_start(struct ktd2692_context *led)
>> +{
>> +	gpio_set_value(led->ctrl_gpio, KTD2692_HIGH);
>> +	udelay(KTD2692_TIME_DATA_START_TIME_US);
>> +}
>> +
>> +static void ktd2692_expresswire_reset(struct ktd2692_context *led)
>> +{
>> +	gpio_set_value(led->ctrl_gpio, KTD2692_LOW);
>> +	udelay(KTD2692_TIME_RESET_US);
>> +}
>> +
>> +static void ktd2692_expresswire_end(struct ktd2692_context *led)
>> +{
>> +	gpio_set_value(led->ctrl_gpio, KTD2692_LOW);
>> +	udelay(KTD2692_TIME_LOW_END_OF_DATA_US);
>> +	gpio_set_value(led->ctrl_gpio, KTD2692_HIGH);
>> +	udelay(KTD2692_TIME_HIGH_END_OF_DATA_US);
>> +}
>> +
>> +static void ktd2692_expresswire_set_bit(struct ktd2692_context *led, bool bit)
>> +{
>> +	if (bit) {
>> +		gpio_set_value(led->ctrl_gpio, KTD2692_LOW);
>> +		udelay(KTD2692_TIME_SHORT_BITSET_US);
>> +		gpio_set_value(led->ctrl_gpio, KTD2692_HIGH);
>> +		udelay(KTD2692_TIME_LONG_BITSET_US);
>> +	} else {
>> +		gpio_set_value(led->ctrl_gpio, KTD2692_LOW);
>> +		udelay(KTD2692_TIME_LONG_BITSET_US);
>> +		gpio_set_value(led->ctrl_gpio, KTD2692_HIGH);
>> +		udelay(KTD2692_TIME_SHORT_BITSET_US);
>> +	}
>> +}
>> +
>> +static void ktd2692_expresswire_write(struct ktd2692_context *led, u8 value)
>> +{
>> +	int i;
>> +
>> +	ktd2692_expresswire_start(led);
>> +	for (i = 7; i >= 0; i--)
>> +		ktd2692_expresswire_set_bit(led, GET_BIT(i, value));
> 
> I think this would be cleaner as:
> 
> value & BIT(i)
> 
> Casting an integer as bool will result in true if it's non-zero, so you
> don't need to cast.
> 

Thanks for good suggestion
I'll try

>> +	ktd2692_expresswire_end(led);
>> +}
>> +
>> +static void ktd2692_brightness_set(struct ktd2692_context *led,
>> +					enum led_brightness brightness)
>> +{
>> +	mutex_lock(&led->lock);
>> +
>> +	if (brightness == LED_OFF) {
>> +		led->mode = KTD2692_MODE_DISABLE;
>> +		gpio_set_value(led->aux_gpio, KTD2692_LOW);
>> +		goto out;
>> +	}
>> +
>> +	ktd2692_expresswire_write(led, KTD2692_REG_MOVIE_CURRENT_BASE |
>> +				KTD2692_BRIGHTNESS_RANGE_255_TO_8(brightness));
>> +	led->mode = KTD2692_MODE_MOVIE;
>> +
>> +out:
>> +	ktd2692_expresswire_write(led, led->mode | KTD2692_REG_MODE_BASE);
>> +	mutex_unlock(&led->lock);
>> +}
>> +
>> +static void ktd2692_brightness_set_work(struct work_struct *work)
>> +{
>> +	struct ktd2692_context *led =
>> +		container_of(work, struct ktd2692_context, work_brightness_set);
>> +
>> +	ktd2692_brightness_set(led, led->torch_brightness);
>> +}
>> +
>> +static void ktd2692_led_brightness_set(struct led_classdev *led_cdev,
>> +					enum led_brightness brightness)
>> +{
>> +	struct led_classdev_flash *fled_cdev = lcdev_to_flcdev(led_cdev);
>> +	struct ktd2692_context *led = fled_cdev_to_led(fled_cdev);
>> +
>> +	led->torch_brightness = brightness;
>> +	schedule_work(&led->work_brightness_set);
>> +}
>> +
>> +static int ktd2692_led_brightness_set_sync(struct led_classdev *led_cdev,
>> +					enum led_brightness brightness)
>> +{
>> +	struct led_classdev_flash *fled_cdev = lcdev_to_flcdev(led_cdev);
>> +	struct ktd2692_context *led = fled_cdev_to_led(fled_cdev);
>> +
>> +	ktd2692_brightness_set(led, brightness);
>> +
>> +	return 0;
>> +}
>> +
>> +static int ktd2692_led_flash_strobe_set(struct led_classdev_flash *fled_cdev,
>> +						bool state)
>> +{
>> +	struct ktd2692_context *led = fled_cdev_to_led(fled_cdev);
>> +	struct led_flash_setting *timeout = &fled_cdev->timeout;
>> +	u32 flash_tm_reg;
>> +
>> +	mutex_lock(&led->lock);
>> +
>> +	if (state == 0) {
>> +		led->mode = KTD2692_MODE_DISABLE;
>> +		gpio_set_value(led->aux_gpio, KTD2692_LOW);
>> +		goto done;
>> +	}
>> +
>> +	flash_tm_reg = GET_TIMEOUT_OFFSET(timeout->val, timeout->step);
>> +	ktd2692_expresswire_write(led, flash_tm_reg
>> +			| KTD2692_REG_FLASH_TIMEOUT_BASE);
>> +
>> +	led->mode = KTD2692_MODE_FLASH;
>> +	gpio_set_value(led->aux_gpio, KTD2692_HIGH);
>> +
>> +done:
>> +	ktd2692_expresswire_write(led, led->mode | KTD2692_REG_MODE_BASE);
>> +
>> +	fled_cdev->led_cdev.brightness = LED_OFF;
>> +	led->mode = KTD2692_MODE_DISABLE;
>> +
>> +	mutex_unlock(&led->lock);
>> +
>> +	return 0;
>> +}
>> +
>> +static int ktd2692_led_flash_timeout_set(struct led_classdev_flash *fled_cdev,
>> +						u32 timeout)
>> +{
>> +	return 0;
>> +}
>> +
>> +static void ktd2692_init_flash_timeout(u32 flash_timeout_us,
>> +					struct led_flash_setting *setting)
>> +{
>> +	setting->min = KTD2692_FLASH_MODE_TIMEOUT_DISABLE;
>> +	setting->max = KTD2692_FLASH_MODE_TIMEOUT_MAX_US;
>> +	setting->step = KTD2692_FLASH_MODE_TIMEOUT_MAX_US
>> +			/ (KTD2692_FLASH_MODE_TIMEOUT_LEVELS - 1);
>> +	setting->val = flash_timeout_us;
>> +}
>> +
>> +static void ktd2692_setup(struct ktd2692_context *led)
>> +{
>> +	led->mode = KTD2692_MODE_DISABLE;
>> +	ktd2692_expresswire_reset(led);
>> +	gpio_set_value(led->aux_gpio, KTD2692_LOW);
>> +
>> +	ktd2692_expresswire_write(led, KTD2692_FLASH_MODE_CURR_PERCENT(45)
>> +					 | KTD2692_REG_FLASH_CURRENT_BASE);
>> +}
>> +
>> +static int ktd2692_parse_dt(struct ktd2692_context *led, struct device *dev,
>> +				u32 *flash_timeout_us)
>> +{
>> +	struct device_node *np = dev->of_node;
>> +
>> +	int ret;
>> +
>> +	led->ctrl_gpio = of_get_named_gpio(np, "ctrl-gpio", 0);
>> +	if (!gpio_is_valid(led->ctrl_gpio)) {
>> +		dev_err(dev, "no ctrl-gpio property found\n");
>> +		return -EINVAL;
>> +	}
>> +
>> +	led->aux_gpio = of_get_named_gpio(np, "aux-gpio", 0);
>> +	if (!gpio_is_valid(led->aux_gpio)) {
>> +		dev_err(dev, "no aux-gpio property found\n");
>> +		return -EINVAL;
>> +	}
>> +
>> +	ret = devm_gpio_request_one(dev, led->ctrl_gpio,
>> +					GPIOF_OPEN_SOURCE, "ctrl-gpio");
>> +	if (ret) {
>> +		dev_err(dev, "failed to request ctrl-gpio %d error %d\n",
>> +				led->ctrl_gpio, ret);
>> +		return ret;
>> +	}
>> +
>> +	ret = devm_gpio_request_one(dev, led->aux_gpio,
>> +					GPIOF_OPEN_SOURCE, "aux-gpio");
>> +	if (ret) {
>> +		dev_err(dev, "failed to request aux-gpio %d error %d\n",
>> +				led->aux_gpio, ret);
>> +		return ret;
>> +	}
>> +
>> +	ret = of_property_read_u32(np, "flash-timeout-us", flash_timeout_us);
>> +	/* default setting */
>> +	if (ret)
>> +		*flash_timeout_us = KTD2692_FLASH_MODE_TIMEOUT_DEFAULT_US;
>> +
>> +	return 0;
>> +}
>> +
>> +static const struct led_flash_ops flash_ops = {
>> +	.strobe_set = ktd2692_led_flash_strobe_set,
>> +	.timeout_set = ktd2692_led_flash_timeout_set,
>> +};
>> +
>> +static int ktd2692_probe(struct platform_device *pdev)
>> +{
>> +	struct ktd2692_context *led;
>> +	struct led_classdev *led_cdev;
>> +	struct led_classdev_flash *fled_cdev;
>> +	struct led_flash_setting flash_timeout;
>> +	u32 flash_timeout_us;
>> +	int ret;
>> +
>> +	led = devm_kzalloc(&pdev->dev, sizeof(*led), GFP_KERNEL);
>> +	if (!led)
>> +		return -ENOMEM;
>> +
>> +	if (!pdev->dev.of_node)
>> +		return -ENXIO;
>> +
>> +	fled_cdev = &led->fled_cdev;
>> +	led_cdev = &fled_cdev->led_cdev;
>> +
>> +	ret = ktd2692_parse_dt(led, &pdev->dev, &flash_timeout_us);
>> +	if (ret)
>> +		return ret;
>> +
>> +	led->regulator = devm_regulator_get(&pdev->dev, "vin");
>> +	if (IS_ERR(led->regulator)) {
>> +		dev_err(&pdev->dev, "regulator get failed\n");
>> +		return PTR_ERR(led->regulator);
>> +	}
>> +
>> +	ktd2692_init_flash_timeout(flash_timeout_us, &flash_timeout);
>> +
>> +	fled_cdev->timeout = flash_timeout;
>> +	fled_cdev->ops = &flash_ops;
>> +
>> +	led_cdev->name = KTD2692_DEFAULT_NAME;
>> +	led_cdev->brightness_set = ktd2692_led_brightness_set;
>> +	led_cdev->brightness_set_sync = ktd2692_led_brightness_set_sync;
>> +	led_cdev->flags |= LED_CORE_SUSPENDRESUME;
>> +	led_cdev->flags |= LED_DEV_CAP_FLASH;
> 
> You could unify the above two lines.
> 

Is it better to unify code than separate?

>> +
>> +	mutex_init(&led->lock);
>> +	INIT_WORK(&led->work_brightness_set, ktd2692_brightness_set_work);
>> +
>> +	platform_set_drvdata(pdev, led);
>> +
>> +	ret = led_classdev_flash_register(&pdev->dev, fled_cdev);
>> +	if (ret) {
>> +		dev_err(&pdev->dev, "can't register LED %s\n", led_cdev->name);
> 
> You should do mutex_destroy() here.
> 

Right. I should do
Thanks

>> +		cancel_work_sync(&led->work_brightness_set);
>> +		return ret;
>> +	}
>> +
> 
> This is a LED flash device. Do you intend to add support for the V4L2 flash
> API as well?
> 

I hope :-)
maybe I would support extend version later

>> +	ktd2692_setup(led);
>> +	ktd2692_led_regulator_enable(led);
> 
> Hmm. I guess the regulator was already enabled, assuming you have tested
> this. :-)
> 

Oh, I'll check.
Isn't it necessary to use this API if the regulator was enabled?

>> +
>> +	return 0;
>> +}
>> +
>> +static int ktd2692_remove(struct platform_device *pdev)
>> +{
>> +	struct ktd2692_context *led = platform_get_drvdata(pdev);
>> +
>> +	ktd2692_led_regulator_disable(led);
>> +	led_classdev_flash_unregister(&led->fled_cdev);
>> +	cancel_work_sync(&led->work_brightness_set);
>> +
>> +	mutex_destroy(&led->lock);
>> +
>> +	return 0;
>> +}
>> +
>> +static const struct of_device_id ktd2692_match[] = {
>> +	{ .compatible = "kinetic,ktd2692", },
>> +	{ /* sentinel */ },
>> +};
>> +
>> +static struct platform_driver ktd2692_driver = {
>> +	.driver = {
>> +		.name  = "leds-ktd2692",
>> +		.of_match_table = ktd2692_match,
>> +	},
>> +	.probe  = ktd2692_probe,
>> +	.remove = ktd2692_remove,
>> +};
>> +
>> +module_platform_driver(ktd2692_driver);
>> +
>> +MODULE_AUTHOR("Ingi Kim <ingi2.kim@samsung.com>");
>> +MODULE_DESCRIPTION("Kinetic KTD2692 LED driver");
>> +MODULE_LICENSE("GPL v2");
> 

Re: [PATCH v4 3/3] leds: Add ktd2692 flash LED driver

[Sakari Ailus]

Hi Ingi,

On Thu, Mar 26, 2015 at 01:56:39PM +0900, Ingi Kim wrote:
...
> >> +	depends on LEDS_CLASS_FLASH && GPIOLIB
> >> +	help
> >> +	  This option enables support for the KTD2692 connected through
> >> +	  ExpressWire Interface. Say Y to enabled these.
> >> +	  It depends on LEDS_CLASS_FLASH for using flash led (strobe) and
> >> +	  GPIOLIB for using gpio pin to control Expresswire interface
> > 
> > The dependencies are shown by make *config, I would drop them from here.
> > 
> 
> Did you mean help message about dependencies? or config (LEDS_CLASS_FLASH, GPIOLIB)
> if you mean latter, I don't know exactly what you say.
> if it wasn't, I should drop superfluous help message

Just the dependencies in the help message, please. The rest of the help
message is fine IMO.

> >> +
> >>  comment "LED driver for blink(1) USB RGB LED is under Special HID drivers (HID_THINGM)"
> >>  
> >>  config LEDS_BLINKM

...

> >> +static void ktd2692_led_regulator_enable(struct ktd2692_context *led)
> >> +{
> >> +	struct led_classdev_flash *fled_cdev = &led->fled_cdev;
> >> +	struct led_classdev *led_cdev = &fled_cdev->led_cdev;
> >> +	int ret;
> >> +
> >> +	if (regulator_is_enabled(led->regulator) > 0)
> >> +		return;
> > 
> > What's the purpose of this? Why not to just call regulator_enable() instead?
> > 
> > This way you could easily mess up with other users of the same regulator.
> > 
> 
> I want to harmonize all APIs in here...
> 
> but I'd better simplify code if it would make a mess

What is now possible:

1. driver x: regulator_enable(), which turns on the regulator

2. ktd2692 sees the regulator is enabled and will not increase the use count

3. ktd2692: regulator_disable()

4. driver x still needs needs the regulator, but it was disabled by ktd2692

So, you must call regulator_enable() and regulator_disable()
unconditionally. I'd put this where ktd2692_led_regulator_enable() and
ktd2692_led_regulator_disable() are called now.

> 
> >> +
> >> +	ret = regulator_enable(led->regulator);
> >> +	if (ret)
> >> +		dev_err(led_cdev->dev, "Failed to enable vin:%d\n", ret);
> >> +}
> >> +
> >> +static void ktd2692_led_regulator_disable(struct ktd2692_context *led)
> >> +{
> >> +	struct led_classdev_flash *fled_cdev = &led->fled_cdev;
> >> +	struct led_classdev *led_cdev = &fled_cdev->led_cdev;
> >> +	int ret;
> >> +
> >> +	if (!regulator_is_enabled(led->regulator))
> >> +		return;
> >> +
> >> +	ret = regulator_disable(led->regulator);
> >> +	if (ret)
> >> +		dev_err(led_cdev->dev, "Failed to disable vin:%d\n", ret);
> >> +}

...

> >> +static int ktd2692_probe(struct platform_device *pdev)
> >> +{
> >> +	struct ktd2692_context *led;
> >> +	struct led_classdev *led_cdev;
> >> +	struct led_classdev_flash *fled_cdev;
> >> +	struct led_flash_setting flash_timeout;
> >> +	u32 flash_timeout_us;
> >> +	int ret;
> >> +
> >> +	led = devm_kzalloc(&pdev->dev, sizeof(*led), GFP_KERNEL);
> >> +	if (!led)
> >> +		return -ENOMEM;
> >> +
> >> +	if (!pdev->dev.of_node)
> >> +		return -ENXIO;
> >> +
> >> +	fled_cdev = &led->fled_cdev;
> >> +	led_cdev = &fled_cdev->led_cdev;
> >> +
> >> +	ret = ktd2692_parse_dt(led, &pdev->dev, &flash_timeout_us);
> >> +	if (ret)
> >> +		return ret;
> >> +
> >> +	led->regulator = devm_regulator_get(&pdev->dev, "vin");
> >> +	if (IS_ERR(led->regulator)) {
> >> +		dev_err(&pdev->dev, "regulator get failed\n");
> >> +		return PTR_ERR(led->regulator);
> >> +	}
> >> +
> >> +	ktd2692_init_flash_timeout(flash_timeout_us, &flash_timeout);
> >> +
> >> +	fled_cdev->timeout = flash_timeout;
> >> +	fled_cdev->ops = &flash_ops;
> >> +
> >> +	led_cdev->name = KTD2692_DEFAULT_NAME;
> >> +	led_cdev->brightness_set = ktd2692_led_brightness_set;
> >> +	led_cdev->brightness_set_sync = ktd2692_led_brightness_set_sync;
> >> +	led_cdev->flags |= LED_CORE_SUSPENDRESUME;
> >> +	led_cdev->flags |= LED_DEV_CAP_FLASH;
> > 
> > You could unify the above two lines.
> > 
> 
> Is it better to unify code than separate?

I'd think so.

> 
> >> +
> >> +	mutex_init(&led->lock);
> >> +	INIT_WORK(&led->work_brightness_set, ktd2692_brightness_set_work);
> >> +
> >> +	platform_set_drvdata(pdev, led);
> >> +
> >> +	ret = led_classdev_flash_register(&pdev->dev, fled_cdev);
> >> +	if (ret) {
> >> +		dev_err(&pdev->dev, "can't register LED %s\n", led_cdev->name);
> > 
> > You should do mutex_destroy() here.
> > 
> 
> Right. I should do
> Thanks
> 
> >> +		cancel_work_sync(&led->work_brightness_set);
> >> +		return ret;
> >> +	}
> >> +
> > 
> > This is a LED flash device. Do you intend to add support for the V4L2 flash
> > API as well?
> > 
> 
> I hope :-)
> maybe I would support extend version later

Another patch later is fine IMO.

> 
> >> +	ktd2692_setup(led);
> >> +	ktd2692_led_regulator_enable(led);
> > 
> > Hmm. I guess the regulator was already enabled, assuming you have tested
> > this. :-)
> > 
> 
> Oh, I'll check.
> Isn't it necessary to use this API if the regulator was enabled?

It is. Because you use the ktd2692 chip before enabling the regulator, the
regulator must have been enabled previously by something else than ktd2692
driver.

-- 
Regards,

Sakari Ailus
e-mail: sakari.ailus@iki.fi	XMPP: sailus@retiisi.org.uk