* [PATCH v5 1/2] dt: bindings: lm3601x: Introduce the lm3601x driver @ 2018-05-10 17:47 ` Dan Murphy 0 siblings, 0 replies; 33+ messages in thread From: Dan Murphy @ 2018-05-10 17:47 UTC (permalink / raw) To: robh+dt, mark.rutland, jacek.anaszewski, pavel, afd Cc: devicetree, linux-kernel, linux-leds, Dan Murphy Introduce the device tree bindings for the lm3601x family of LED torch, flash and IR drivers. Signed-off-by: Dan Murphy <dmurphy@ti.com> --- v5 - No changes - https://patchwork.kernel.org/patch/10391743/ v4 - Added " " around "=", changed strobe to flash on label, removed "support and register" comment and change ir lable to ir:torch - See v2 patchworks for comments v3 - Removed wildcard compatible - https://patchwork.kernel.org/patch/10386241/ v2 - No changes - https://patchwork.kernel.org/patch/10384587/ .../devicetree/bindings/leds/leds-lm3601x.txt | 50 +++++++++++++++++++ 1 file changed, 50 insertions(+) create mode 100644 Documentation/devicetree/bindings/leds/leds-lm3601x.txt diff --git a/Documentation/devicetree/bindings/leds/leds-lm3601x.txt b/Documentation/devicetree/bindings/leds/leds-lm3601x.txt new file mode 100644 index 000000000000..697e5e3a1d4c --- /dev/null +++ b/Documentation/devicetree/bindings/leds/leds-lm3601x.txt @@ -0,0 +1,50 @@ +* Texas Instruments - lm3601x Single-LED Flash Driver + +The LM3601X are ultra-small LED flash drivers that +provide a high level of adjustability. + +Required properties: + - compatible : Can be one of the following + "ti,lm36010" + "ti,lm36011" + - reg : I2C slave address + - #address-cells : 1 + - #size-cells : 0 + +Required child properties: + - reg : 0 - Indicates a torch interface + 1 - Indicates a flash interface + 2 - Indicates an infrared interface + +Optional child properties: + - label : see Documentation/devicetree/bindings/leds/common.txt + +Example: +led-controller@64 { + compatible = "ti,lm36010"; + #address-cells = <1>; + #size-cells = <0>; + reg = <0x64>; + + led@0 { + reg = <0>; + label = "white:torch"; + led-max-microamp = <10000>; + }; + + led@1 { + reg = <1>; + label = "white:flash"; + flash-max-microamp = <10000>; + flash-max-timeout-us = <800>; + }; + + led@2 { + reg = <2>; + label = "ir:torch"; + }; +} + +For more product information please see the links below: +http://www.ti.com/product/LM36010 +http://www.ti.com/product/LM36011 -- 2.17.0.582.gccdcbd54c ^ permalink raw reply related [flat|nested] 33+ messages in thread
* [PATCH v5 1/2] dt: bindings: lm3601x: Introduce the lm3601x driver @ 2018-05-10 17:47 ` Dan Murphy 0 siblings, 0 replies; 33+ messages in thread From: Dan Murphy @ 2018-05-10 17:47 UTC (permalink / raw) To: robh+dt, mark.rutland, jacek.anaszewski, pavel, afd Cc: devicetree, linux-kernel, linux-leds, Dan Murphy Introduce the device tree bindings for the lm3601x family of LED torch, flash and IR drivers. Signed-off-by: Dan Murphy <dmurphy@ti.com> --- v5 - No changes - https://patchwork.kernel.org/patch/10391743/ v4 - Added " " around "=", changed strobe to flash on label, removed "support and register" comment and change ir lable to ir:torch - See v2 patchworks for comments v3 - Removed wildcard compatible - https://patchwork.kernel.org/patch/10386241/ v2 - No changes - https://patchwork.kernel.org/patch/10384587/ .../devicetree/bindings/leds/leds-lm3601x.txt | 50 +++++++++++++++++++ 1 file changed, 50 insertions(+) create mode 100644 Documentation/devicetree/bindings/leds/leds-lm3601x.txt diff --git a/Documentation/devicetree/bindings/leds/leds-lm3601x.txt b/Documentation/devicetree/bindings/leds/leds-lm3601x.txt new file mode 100644 index 000000000000..697e5e3a1d4c --- /dev/null +++ b/Documentation/devicetree/bindings/leds/leds-lm3601x.txt @@ -0,0 +1,50 @@ +* Texas Instruments - lm3601x Single-LED Flash Driver + +The LM3601X are ultra-small LED flash drivers that +provide a high level of adjustability. + +Required properties: + - compatible : Can be one of the following + "ti,lm36010" + "ti,lm36011" + - reg : I2C slave address + - #address-cells : 1 + - #size-cells : 0 + +Required child properties: + - reg : 0 - Indicates a torch interface + 1 - Indicates a flash interface + 2 - Indicates an infrared interface + +Optional child properties: + - label : see Documentation/devicetree/bindings/leds/common.txt + +Example: +led-controller@64 { + compatible = "ti,lm36010"; + #address-cells = <1>; + #size-cells = <0>; + reg = <0x64>; + + led@0 { + reg = <0>; + label = "white:torch"; + led-max-microamp = <10000>; + }; + + led@1 { + reg = <1>; + label = "white:flash"; + flash-max-microamp = <10000>; + flash-max-timeout-us = <800>; + }; + + led@2 { + reg = <2>; + label = "ir:torch"; + }; +} + +For more product information please see the links below: +http://www.ti.com/product/LM36010 +http://www.ti.com/product/LM36011 -- 2.17.0.582.gccdcbd54c ^ permalink raw reply related [flat|nested] 33+ messages in thread
* [PATCH v5 2/2] leds: lm3601x: Introduce the lm3601x LED driver 2018-05-10 17:47 ` Dan Murphy @ 2018-05-10 17:47 ` Dan Murphy -1 siblings, 0 replies; 33+ messages in thread From: Dan Murphy @ 2018-05-10 17:47 UTC (permalink / raw) To: robh+dt, mark.rutland, jacek.anaszewski, pavel, afd Cc: devicetree, linux-kernel, linux-leds, Dan Murphy Introduce the family of LED devices that can drive a torch, strobe or IR LED. The LED driver can be configured with a strobe timer to execute a strobe flash. The IR LED brightness is controlled via the torch brightness register. The data sheet for each the LM36010 and LM36011 LED drivers can be found here: http://www.ti.com/product/LM36010 http://www.ti.com/product/LM36011 Signed-off-by: Dan Murphy <dmurphy@ti.com> --- v5 - Fixed magic numbers, change reg cache type, added of_put_node to release the dt node ref, and I did not change the remove function to leave the LED in its state on driver removal - https://patchwork.kernel.org/patch/10391741/ v4 - Fixed Cocci issue using ARRAY_SIZE - https://patchwork.kernel.org/patch/10389259/ v3 - removed wildcard dt compatible, fixed copyright, fixed struct doc, removed RO registers from default, added regmap volatile for FLAGS_REG, updated regmap cache type, fixed unlock and extra semi colon in strobe_set, removed unnecessary out label in led register and fixed checking of the ret in brightness_set - https://patchwork.kernel.org/patch/10386243/ v2 - Fixed kbuild issue and removed unused cdev_strobe - https://patchwork.kernel.org/patch/10384585/ drivers/leds/Kconfig | 9 + drivers/leds/Makefile | 1 + drivers/leds/leds-lm3601x.c | 622 ++++++++++++++++++++++++++++++++++++ 3 files changed, 632 insertions(+) create mode 100644 drivers/leds/leds-lm3601x.c diff --git a/drivers/leds/Kconfig b/drivers/leds/Kconfig index 2c896c0e69e1..50ae536f343f 100644 --- a/drivers/leds/Kconfig +++ b/drivers/leds/Kconfig @@ -145,6 +145,15 @@ config LEDS_LM3692X This option enables support for the TI LM3692x family of white LED string drivers used for backlighting. +config LEDS_LM3601X + tristate "LED support for LM3601x Chips" + depends on LEDS_CLASS && I2C && OF + depends on LEDS_CLASS_FLASH + select REGMAP_I2C + help + This option enables support for the TI LM3601x family + of flash, torch and indicator classes. + config LEDS_LOCOMO tristate "LED Support for Locomo device" depends on LEDS_CLASS diff --git a/drivers/leds/Makefile b/drivers/leds/Makefile index 91eca81cae82..b79807fe1b67 100644 --- a/drivers/leds/Makefile +++ b/drivers/leds/Makefile @@ -76,6 +76,7 @@ obj-$(CONFIG_LEDS_MLXREG) += leds-mlxreg.o obj-$(CONFIG_LEDS_NIC78BX) += leds-nic78bx.o obj-$(CONFIG_LEDS_MT6323) += leds-mt6323.o obj-$(CONFIG_LEDS_LM3692X) += leds-lm3692x.o +obj-$(CONFIG_LEDS_LM3601X) += leds-lm3601x.o # LED SPI Drivers obj-$(CONFIG_LEDS_DAC124S085) += leds-dac124s085.o diff --git a/drivers/leds/leds-lm3601x.c b/drivers/leds/leds-lm3601x.c new file mode 100644 index 000000000000..3c00886fbc7a --- /dev/null +++ b/drivers/leds/leds-lm3601x.c @@ -0,0 +1,622 @@ +// SPDX-License-Identifier: GPL-2.0 +/* + * Flash and torch driver for Texas Instruments LM3601X LED + * Flash driver chip family + * Copyright (C) 2018 Texas Instruments Incorporated - http://www.ti.com/ + */ + +#include <linux/delay.h> +#include <linux/i2c.h> +#include <linux/leds.h> +#include <linux/led-class-flash.h> +#include <linux/module.h> +#include <linux/regmap.h> +#include <linux/slab.h> +#include <uapi/linux/uleds.h> + +#define LM3601X_LED_TORCH 0x0 +#define LM3601X_LED_STROBE 0x1 +#define LM3601X_LED_IR 0x2 + +/* Registers */ +#define LM3601X_ENABLE_REG 0x01 +#define LM3601X_CFG_REG 0x02 +#define LM3601X_LED_FLASH_REG 0x03 +#define LM3601X_LED_TORCH_REG 0x04 +#define LM3601X_FLAGS_REG 0x05 +#define LM3601X_DEV_ID_REG 0x06 + +#define LM3601X_SW_RESET BIT(7) + +/* Enable Mode bits */ +#define LM3601X_MODE_STANDBY 0x00 +#define LM3601X_MODE_IR_DRV BIT(0) +#define LM3601X_MODE_TORCH BIT(1) +#define LM3601X_MODE_STROBE (BIT(0) | BIT(1)) +#define LM3601X_STRB_EN BIT(2) +#define LM3601X_STRB_LVL_TRIG ~BIT(3) +#define LM3601X_STRB_EDGE_TRIG BIT(3) +#define LM3601X_IVFM_EN BIT(4) + +#define LM36010_BOOST_LIMIT_19 ~BIT(5) +#define LM36010_BOOST_LIMIT_28 BIT(5) +#define LM36010_BOOST_FREQ_2MHZ ~BIT(6) +#define LM36010_BOOST_FREQ_4MHZ BIT(6) +#define LM36010_BOOST_MODE_NORM ~BIT(7) +#define LM36010_BOOST_MODE_PASS BIT(7) + +/* Flag Mask */ +#define LM3601X_FLASH_TIME_OUT BIT(0) +#define LM3601X_UVLO_FAULT BIT(1) +#define LM3601X_THERM_SHUTDOWN BIT(2) +#define LM3601X_THERM_CURR BIT(3) +#define LM36010_CURR_LIMIT BIT(4) +#define LM3601X_SHORT_FAULT BIT(5) +#define LM3601X_IVFM_TRIP BIT(6) +#define LM36010_OVP_FAULT BIT(7) + +#define LM3601X_MIN_TORCH_I_UA 2400 +#define LM3601X_MIN_STROBE_I_MA 11 + +#define LM3601X_TIMEOUT_MASK 0x1e +#define LM3601X_ENABLE_MASK 0x03 + +enum lm3601x_type { + CHIP_LM36010 = 0, + CHIP_LM36011, +}; + +/** + * struct lm3601x_max_timeouts - + * @timeout: timeout value in ms + * @regval: the value of the register to write + */ +struct lm3601x_max_timeouts { + int timeout; + int reg_val; +}; + +/** + * struct lm3601x_led - + * @lock: Lock for reading/writing the device + * @regmap: Devices register map + * @client: Pointer to the I2C client + * @cdev_torch: led class device pointer for the torch + * @cdev_ir: led class device pointer for infrared + * @fled_cdev: flash led class device pointer + * @strobe_node: DT device node for the strobe + * @torch: LED label for the torch + * @strobe: LED label for the strobe + * @ir: LED label for the infrared + * @last_flag: last known flags register value + * @strobe_timeout: the timeout for the strobe + * @torch_current_max: maximum current for the torch + * @strobe_current_max: maximum current for the strobe + * @max_strobe_timeout: maximum timeout for the strobe + */ +struct lm3601x_led { + struct mutex lock; + struct regmap *regmap; + struct i2c_client *client; + + struct led_classdev cdev_torch; + struct led_classdev cdev_ir; + + struct led_classdev_flash fled_cdev; + + struct device_node *strobe_node; + struct device_node *torch_node; + struct device_node *ir_node; + + char torch[LED_MAX_NAME_SIZE]; + char strobe[LED_MAX_NAME_SIZE]; + char ir[LED_MAX_NAME_SIZE]; + + unsigned int last_flag; + unsigned int strobe_timeout; + + u32 torch_current_max; + u32 strobe_current_max; + u32 max_strobe_timeout; +}; + +static const struct lm3601x_max_timeouts strobe_timeouts[] = { + { 40, 0x00 }, + { 80, 0x01 }, + { 120, 0x02 }, + { 160, 0x03 }, + { 200, 0x04 }, + { 240, 0x05 }, + { 280, 0x06 }, + { 320, 0x07 }, + { 360, 0x08 }, + { 400, 0x09 }, + { 600, 0x0a }, + { 800, 0x0b }, + { 1000, 0x0c }, + { 1200, 0x0d }, + { 1400, 0x0e }, + { 1600, 0x0f }, +}; + +static const struct reg_default lm3601x_regmap_defs[] = { + { LM3601X_ENABLE_REG, 0x20 }, + { LM3601X_CFG_REG, 0x15 }, + { LM3601X_LED_FLASH_REG, 0x00 }, + { LM3601X_LED_TORCH_REG, 0x00 }, +}; + +static bool lm3601x_volatile_reg(struct device *dev, unsigned int reg) +{ + switch (reg) { + case LM3601X_FLAGS_REG: + return true; + default: + return false; + } +} + +static const struct regmap_config lm3601x_regmap = { + .reg_bits = 8, + .val_bits = 8, + + .max_register = LM3601X_DEV_ID_REG, + .reg_defaults = lm3601x_regmap_defs, + .num_reg_defaults = ARRAY_SIZE(lm3601x_regmap_defs), + .cache_type = REGCACHE_RBTREE, + .volatile_reg = lm3601x_volatile_reg, +}; + +static struct lm3601x_led *fled_cdev_to_led( + struct led_classdev_flash *fled_cdev) +{ + return container_of(fled_cdev, struct lm3601x_led, fled_cdev); +} + +static int lm3601x_read_faults(struct lm3601x_led *led) +{ + int flags_val; + int ret; + + ret = regmap_read(led->regmap, LM3601X_FLAGS_REG, &flags_val); + if (ret < 0) + return -EIO; + + led->last_flag = flags_val; + + return led->last_flag; +} + +static int lm3601x_torch_brightness_set(struct led_classdev *cdev, + enum led_brightness brightness) +{ + struct lm3601x_led *led = + container_of(cdev, struct lm3601x_led, cdev_torch); + int ret; + u8 brightness_val; + + mutex_lock(&led->lock); + + ret = lm3601x_read_faults(led); + if (ret < 0) + goto out; + + if (brightness == LED_OFF) { + ret = regmap_update_bits(led->regmap, LM3601X_ENABLE_REG, + LM3601X_MODE_TORCH, LED_OFF); + goto out; + } + + if (brightness == LED_ON) + brightness_val = LED_ON; + else + brightness_val = (brightness/2); + + ret = regmap_write(led->regmap, LM3601X_LED_TORCH_REG, brightness_val); + if (ret < 0) + goto out; + + ret = regmap_update_bits(led->regmap, LM3601X_ENABLE_REG, + LM3601X_MODE_TORCH, + LM3601X_MODE_TORCH); + +out: + mutex_unlock(&led->lock); + return ret; +} + +static int lm3601x_strobe_set(struct led_classdev_flash *fled_cdev, + bool state) +{ + struct lm3601x_led *led = fled_cdev_to_led(fled_cdev); + int ret; + int current_timeout; + int reg_count; + int i; + int timeout_reg_val = 0; + + mutex_lock(&led->lock); + + ret = regmap_read(led->regmap, LM3601X_CFG_REG, ¤t_timeout); + if (ret < 0) + goto out; + + reg_count = ARRAY_SIZE(strobe_timeouts); + for (i = 0; i < reg_count; i++) { + if (led->strobe_timeout > strobe_timeouts[i].timeout) + continue; + + if (led->strobe_timeout <= strobe_timeouts[i].timeout) { + timeout_reg_val = (strobe_timeouts[i].reg_val << 1); + break; + } + + ret = -EINVAL; + goto out; + } + + if (led->strobe_timeout != current_timeout) + ret = regmap_update_bits(led->regmap, LM3601X_CFG_REG, + LM3601X_TIMEOUT_MASK, timeout_reg_val); + + if (state) + ret = regmap_update_bits(led->regmap, LM3601X_ENABLE_REG, + LM3601X_MODE_STROBE, + LM3601X_MODE_STROBE); + else + ret = regmap_update_bits(led->regmap, LM3601X_ENABLE_REG, + LM3601X_MODE_STROBE, LED_OFF); + + ret = lm3601x_read_faults(led); +out: + mutex_unlock(&led->lock); + return ret; +} + +static int lm3601x_strobe_brightness_set(struct led_classdev *cdev, + enum led_brightness brightness) +{ + struct led_classdev_flash *fled_cdev = lcdev_to_flcdev(cdev); + struct lm3601x_led *led = fled_cdev_to_led(fled_cdev); + int ret; + u8 brightness_val; + + mutex_lock(&led->lock); + ret = lm3601x_read_faults(led); + if (ret < 0) + goto out; + + if (brightness == LED_OFF) { + ret = regmap_update_bits(led->regmap, LM3601X_ENABLE_REG, + LM3601X_MODE_STROBE, LED_OFF); + goto out; + } + + if (brightness == LED_ON) + brightness_val = LED_ON; + else + brightness_val = (brightness/2); + + ret = regmap_write(led->regmap, LM3601X_LED_FLASH_REG, brightness_val); + +out: + mutex_unlock(&led->lock); + return ret; +} + +static int lm3601x_strobe_timeout_set(struct led_classdev_flash *fled_cdev, + u32 timeout) +{ + struct lm3601x_led *led = fled_cdev_to_led(fled_cdev); + int ret = 0; + + mutex_lock(&led->lock); + + if (timeout > led->max_strobe_timeout) + ret = -EINVAL; + else + led->strobe_timeout = timeout; + + mutex_unlock(&led->lock); + + return ret; +} + +static int lm3601x_strobe_get(struct led_classdev_flash *fled_cdev, + bool *state) +{ + struct lm3601x_led *led = fled_cdev_to_led(fled_cdev); + int ret; + int strobe_state; + + mutex_lock(&led->lock); + + ret = regmap_read(led->regmap, LM3601X_ENABLE_REG, &strobe_state); + if (ret < 0) + goto out; + + *state = strobe_state & LM3601X_MODE_STROBE; + +out: + mutex_unlock(&led->lock); + return ret; +} + +static int lm3601x_ir_brightness_set(struct led_classdev *cdev, + enum led_brightness brightness) +{ + struct lm3601x_led *led = + container_of(cdev, struct lm3601x_led, cdev_ir); + int ret; + u8 brightness_val; + + mutex_lock(&led->lock); + ret = lm3601x_read_faults(led); + if (ret < 0) + goto out; + + if (brightness == LED_OFF) { + ret = regmap_update_bits(led->regmap, LM3601X_ENABLE_REG, + LM3601X_LED_IR, + LED_OFF); + goto out; + } + + if (brightness == LED_ON) + brightness_val = LED_ON; + else + brightness_val = (brightness/2); + + ret = regmap_write(led->regmap, LM3601X_LED_FLASH_REG, brightness_val); + if (ret < 0) + goto out; + + ret = regmap_update_bits(led->regmap, LM3601X_ENABLE_REG, + LM3601X_MODE_IR_DRV, + LM3601X_MODE_IR_DRV); + +out: + mutex_unlock(&led->lock); + return ret; +} + +static int lm3601x_strobe_fault_get(struct led_classdev_flash *fled_cdev, + u32 *fault) +{ + struct lm3601x_led *led = fled_cdev_to_led(fled_cdev); + + lm3601x_read_faults(led); + + *fault = led->last_flag; + + return 0; +} + +static const struct led_flash_ops strobe_ops = { + .strobe_set = lm3601x_strobe_set, + .strobe_get = lm3601x_strobe_get, + .timeout_set = lm3601x_strobe_timeout_set, + .fault_get = lm3601x_strobe_fault_get, +}; + +static int lm3601x_register_leds(struct lm3601x_led *led) +{ + struct led_classdev_flash *fled_cdev; + struct led_classdev *led_cdev; + int err = -ENODEV; + + if (led->torch_node) { + led->cdev_torch.name = led->torch; + led->cdev_torch.max_brightness = LED_FULL; + led->cdev_torch.brightness_set_blocking = lm3601x_torch_brightness_set; + err = devm_led_classdev_register(&led->client->dev, + &led->cdev_torch); + if (err < 0) + return err; + } + + if (led->strobe_node) { + fled_cdev = &led->fled_cdev; + fled_cdev->ops = &strobe_ops; + + led_cdev = &fled_cdev->led_cdev; + led_cdev->name = led->strobe; + led_cdev->max_brightness = LED_FULL; + led_cdev->brightness_set_blocking = lm3601x_strobe_brightness_set; + led_cdev->flags |= LED_DEV_CAP_FLASH; + + err = led_classdev_flash_register(&led->client->dev, + fled_cdev); + if (err < 0) + return err; + } + + if (led->ir_node) { + led->cdev_ir.name = led->ir; + led->cdev_ir.max_brightness = LED_FULL; + led->cdev_ir.brightness_set_blocking = + lm3601x_ir_brightness_set; + + err = devm_led_classdev_register(&led->client->dev, + &led->cdev_ir); + } + + return err; +} + +static void lm3601x_init_flash_timeout(struct lm3601x_led *led) +{ + struct led_flash_setting *setting; + + setting = &led->fled_cdev.timeout; + setting->min = strobe_timeouts[0].timeout; + setting->max = led->max_strobe_timeout; + setting->step = 40; + setting->val = led->max_strobe_timeout; +} + +static int lm3601x_parse_node(struct lm3601x_led *led, + struct device_node *node) +{ + struct device_node *child_node; + const char *name; + int ret = -ENODEV; + + for_each_available_child_of_node(node, child_node) { + u32 id = 0; + + of_property_read_u32(child_node, "reg", &id); + + switch (id) { + case LM3601X_LED_TORCH: + led->torch_node = of_node_get(child_node); + break; + case LM3601X_LED_STROBE: + led->strobe_node = of_node_get(child_node); + break; + case LM3601X_LED_IR: + led->ir_node = of_node_get(child_node); + break; + default: + dev_warn(&led->client->dev, + "unknown LED %u encountered, ignoring\n", id); + break; + } + } + + if (led->torch_node) { + ret = of_property_read_string(led->torch_node, "label", &name); + if (!ret) + snprintf(led->torch, sizeof(led->torch), + "%s:%s", led->torch_node->name, name); + else + snprintf(led->torch, sizeof(led->torch), + "%s:torch", led->torch_node->name); + ret = of_property_read_u32(led->torch_node, "led-max-microamp", + &led->torch_current_max); + if (ret < 0) { + led->torch_current_max = LM3601X_MIN_TORCH_I_UA; + dev_warn(&led->client->dev, + "led-max-microamp DT property missing\n"); + } + + of_node_put(led->torch_node); + } + + if (led->strobe_node) { + ret = of_property_read_string(led->strobe_node, "label", &name); + if (!ret) + snprintf(led->strobe, sizeof(led->strobe), + "%s:%s", led->strobe_node->name, name); + else + snprintf(led->strobe, sizeof(led->strobe), + "%s::strobe", led->strobe_node->name); + + ret = of_property_read_u32(led->strobe_node, + "flash-max-microamp", + &led->strobe_current_max); + if (ret < 0) { + led->strobe_current_max = LM3601X_MIN_STROBE_I_MA; + dev_warn(&led->client->dev, + "flash-max-microamp DT property missing\n"); + } + + ret = of_property_read_u32(led->strobe_node, + "flash-max-timeout-us", + &led->max_strobe_timeout); + if (ret < 0) { + led->max_strobe_timeout = strobe_timeouts[0].reg_val; + dev_warn(&led->client->dev, + "flash-max-timeout-us DT property missing\n"); + } + + lm3601x_init_flash_timeout(led); + + of_node_put(led->strobe_node); + } + + if (led->ir_node) { + ret = of_property_read_string(led->ir_node, "label", &name); + if (!ret) + snprintf(led->ir, sizeof(led->ir), + "%s:%s", led->ir_node->name, name); + else + snprintf(led->ir, sizeof(led->ir), + "%s::infrared", led->ir_node->name); + + of_node_put(led->ir_node); + } + + return ret; +} + +static int lm3601x_probe(struct i2c_client *client, + const struct i2c_device_id *id) +{ + struct lm3601x_led *led; + int err; + + led = devm_kzalloc(&client->dev, + sizeof(struct lm3601x_led), GFP_KERNEL); + if (!led) + return -ENOMEM; + + err = lm3601x_parse_node(led, client->dev.of_node); + if (err < 0) + return -ENODEV; + + led->client = client; + led->regmap = devm_regmap_init_i2c(client, &lm3601x_regmap); + if (IS_ERR(led->regmap)) { + err = PTR_ERR(led->regmap); + dev_err(&client->dev, + "Failed to allocate register map: %d\n", err); + return err; + } + + mutex_init(&led->lock); + i2c_set_clientdata(client, led); + err = lm3601x_register_leds(led); + + return err; +} + +static int lm3601x_remove(struct i2c_client *client) +{ + struct lm3601x_led *led = i2c_get_clientdata(client); + + regmap_update_bits(led->regmap, LM3601X_ENABLE_REG, + LM3601X_ENABLE_MASK, + LM3601X_MODE_STANDBY); + + return 0; +} + +static const struct i2c_device_id lm3601x_id[] = { + { "LM36010", CHIP_LM36010 }, + { "LM36011", CHIP_LM36011 }, + { }, +}; +MODULE_DEVICE_TABLE(i2c, lm3601x_id); + +static const struct of_device_id of_lm3601x_leds_match[] = { + { .compatible = "ti,lm36010", }, + { .compatible = "ti,lm36011", }, + {}, +}; +MODULE_DEVICE_TABLE(of, of_lm3601x_leds_match); + +static struct i2c_driver lm3601x_i2c_driver = { + .driver = { + .name = "lm3601x", + .of_match_table = of_lm3601x_leds_match, + }, + .probe = lm3601x_probe, + .remove = lm3601x_remove, + .id_table = lm3601x_id, +}; +module_i2c_driver(lm3601x_i2c_driver); + +MODULE_DESCRIPTION("Texas Instruments Flash Lighting driver for LM3601X"); +MODULE_AUTHOR("Dan Murphy <dmurphy@ti.com>"); +MODULE_LICENSE("GPL v2"); -- 2.17.0.582.gccdcbd54c ^ permalink raw reply related [flat|nested] 33+ messages in thread
* [PATCH v5 2/2] leds: lm3601x: Introduce the lm3601x LED driver @ 2018-05-10 17:47 ` Dan Murphy 0 siblings, 0 replies; 33+ messages in thread From: Dan Murphy @ 2018-05-10 17:47 UTC (permalink / raw) To: robh+dt, mark.rutland, jacek.anaszewski, pavel, afd Cc: devicetree, linux-kernel, linux-leds, Dan Murphy Introduce the family of LED devices that can drive a torch, strobe or IR LED. The LED driver can be configured with a strobe timer to execute a strobe flash. The IR LED brightness is controlled via the torch brightness register. The data sheet for each the LM36010 and LM36011 LED drivers can be found here: http://www.ti.com/product/LM36010 http://www.ti.com/product/LM36011 Signed-off-by: Dan Murphy <dmurphy@ti.com> --- v5 - Fixed magic numbers, change reg cache type, added of_put_node to release the dt node ref, and I did not change the remove function to leave the LED in its state on driver removal - https://patchwork.kernel.org/patch/10391741/ v4 - Fixed Cocci issue using ARRAY_SIZE - https://patchwork.kernel.org/patch/10389259/ v3 - removed wildcard dt compatible, fixed copyright, fixed struct doc, removed RO registers from default, added regmap volatile for FLAGS_REG, updated regmap cache type, fixed unlock and extra semi colon in strobe_set, removed unnecessary out label in led register and fixed checking of the ret in brightness_set - https://patchwork.kernel.org/patch/10386243/ v2 - Fixed kbuild issue and removed unused cdev_strobe - https://patchwork.kernel.org/patch/10384585/ drivers/leds/Kconfig | 9 + drivers/leds/Makefile | 1 + drivers/leds/leds-lm3601x.c | 622 ++++++++++++++++++++++++++++++++++++ 3 files changed, 632 insertions(+) create mode 100644 drivers/leds/leds-lm3601x.c diff --git a/drivers/leds/Kconfig b/drivers/leds/Kconfig index 2c896c0e69e1..50ae536f343f 100644 --- a/drivers/leds/Kconfig +++ b/drivers/leds/Kconfig @@ -145,6 +145,15 @@ config LEDS_LM3692X This option enables support for the TI LM3692x family of white LED string drivers used for backlighting. +config LEDS_LM3601X + tristate "LED support for LM3601x Chips" + depends on LEDS_CLASS && I2C && OF + depends on LEDS_CLASS_FLASH + select REGMAP_I2C + help + This option enables support for the TI LM3601x family + of flash, torch and indicator classes. + config LEDS_LOCOMO tristate "LED Support for Locomo device" depends on LEDS_CLASS diff --git a/drivers/leds/Makefile b/drivers/leds/Makefile index 91eca81cae82..b79807fe1b67 100644 --- a/drivers/leds/Makefile +++ b/drivers/leds/Makefile @@ -76,6 +76,7 @@ obj-$(CONFIG_LEDS_MLXREG) += leds-mlxreg.o obj-$(CONFIG_LEDS_NIC78BX) += leds-nic78bx.o obj-$(CONFIG_LEDS_MT6323) += leds-mt6323.o obj-$(CONFIG_LEDS_LM3692X) += leds-lm3692x.o +obj-$(CONFIG_LEDS_LM3601X) += leds-lm3601x.o # LED SPI Drivers obj-$(CONFIG_LEDS_DAC124S085) += leds-dac124s085.o diff --git a/drivers/leds/leds-lm3601x.c b/drivers/leds/leds-lm3601x.c new file mode 100644 index 000000000000..3c00886fbc7a --- /dev/null +++ b/drivers/leds/leds-lm3601x.c @@ -0,0 +1,622 @@ +// SPDX-License-Identifier: GPL-2.0 +/* + * Flash and torch driver for Texas Instruments LM3601X LED + * Flash driver chip family + * Copyright (C) 2018 Texas Instruments Incorporated - http://www.ti.com/ + */ + +#include <linux/delay.h> +#include <linux/i2c.h> +#include <linux/leds.h> +#include <linux/led-class-flash.h> +#include <linux/module.h> +#include <linux/regmap.h> +#include <linux/slab.h> +#include <uapi/linux/uleds.h> + +#define LM3601X_LED_TORCH 0x0 +#define LM3601X_LED_STROBE 0x1 +#define LM3601X_LED_IR 0x2 + +/* Registers */ +#define LM3601X_ENABLE_REG 0x01 +#define LM3601X_CFG_REG 0x02 +#define LM3601X_LED_FLASH_REG 0x03 +#define LM3601X_LED_TORCH_REG 0x04 +#define LM3601X_FLAGS_REG 0x05 +#define LM3601X_DEV_ID_REG 0x06 + +#define LM3601X_SW_RESET BIT(7) + +/* Enable Mode bits */ +#define LM3601X_MODE_STANDBY 0x00 +#define LM3601X_MODE_IR_DRV BIT(0) +#define LM3601X_MODE_TORCH BIT(1) +#define LM3601X_MODE_STROBE (BIT(0) | BIT(1)) +#define LM3601X_STRB_EN BIT(2) +#define LM3601X_STRB_LVL_TRIG ~BIT(3) +#define LM3601X_STRB_EDGE_TRIG BIT(3) +#define LM3601X_IVFM_EN BIT(4) + +#define LM36010_BOOST_LIMIT_19 ~BIT(5) +#define LM36010_BOOST_LIMIT_28 BIT(5) +#define LM36010_BOOST_FREQ_2MHZ ~BIT(6) +#define LM36010_BOOST_FREQ_4MHZ BIT(6) +#define LM36010_BOOST_MODE_NORM ~BIT(7) +#define LM36010_BOOST_MODE_PASS BIT(7) + +/* Flag Mask */ +#define LM3601X_FLASH_TIME_OUT BIT(0) +#define LM3601X_UVLO_FAULT BIT(1) +#define LM3601X_THERM_SHUTDOWN BIT(2) +#define LM3601X_THERM_CURR BIT(3) +#define LM36010_CURR_LIMIT BIT(4) +#define LM3601X_SHORT_FAULT BIT(5) +#define LM3601X_IVFM_TRIP BIT(6) +#define LM36010_OVP_FAULT BIT(7) + +#define LM3601X_MIN_TORCH_I_UA 2400 +#define LM3601X_MIN_STROBE_I_MA 11 + +#define LM3601X_TIMEOUT_MASK 0x1e +#define LM3601X_ENABLE_MASK 0x03 + +enum lm3601x_type { + CHIP_LM36010 = 0, + CHIP_LM36011, +}; + +/** + * struct lm3601x_max_timeouts - + * @timeout: timeout value in ms + * @regval: the value of the register to write + */ +struct lm3601x_max_timeouts { + int timeout; + int reg_val; +}; + +/** + * struct lm3601x_led - + * @lock: Lock for reading/writing the device + * @regmap: Devices register map + * @client: Pointer to the I2C client + * @cdev_torch: led class device pointer for the torch + * @cdev_ir: led class device pointer for infrared + * @fled_cdev: flash led class device pointer + * @strobe_node: DT device node for the strobe + * @torch: LED label for the torch + * @strobe: LED label for the strobe + * @ir: LED label for the infrared + * @last_flag: last known flags register value + * @strobe_timeout: the timeout for the strobe + * @torch_current_max: maximum current for the torch + * @strobe_current_max: maximum current for the strobe + * @max_strobe_timeout: maximum timeout for the strobe + */ +struct lm3601x_led { + struct mutex lock; + struct regmap *regmap; + struct i2c_client *client; + + struct led_classdev cdev_torch; + struct led_classdev cdev_ir; + + struct led_classdev_flash fled_cdev; + + struct device_node *strobe_node; + struct device_node *torch_node; + struct device_node *ir_node; + + char torch[LED_MAX_NAME_SIZE]; + char strobe[LED_MAX_NAME_SIZE]; + char ir[LED_MAX_NAME_SIZE]; + + unsigned int last_flag; + unsigned int strobe_timeout; + + u32 torch_current_max; + u32 strobe_current_max; + u32 max_strobe_timeout; +}; + +static const struct lm3601x_max_timeouts strobe_timeouts[] = { + { 40, 0x00 }, + { 80, 0x01 }, + { 120, 0x02 }, + { 160, 0x03 }, + { 200, 0x04 }, + { 240, 0x05 }, + { 280, 0x06 }, + { 320, 0x07 }, + { 360, 0x08 }, + { 400, 0x09 }, + { 600, 0x0a }, + { 800, 0x0b }, + { 1000, 0x0c }, + { 1200, 0x0d }, + { 1400, 0x0e }, + { 1600, 0x0f }, +}; + +static const struct reg_default lm3601x_regmap_defs[] = { + { LM3601X_ENABLE_REG, 0x20 }, + { LM3601X_CFG_REG, 0x15 }, + { LM3601X_LED_FLASH_REG, 0x00 }, + { LM3601X_LED_TORCH_REG, 0x00 }, +}; + +static bool lm3601x_volatile_reg(struct device *dev, unsigned int reg) +{ + switch (reg) { + case LM3601X_FLAGS_REG: + return true; + default: + return false; + } +} + +static const struct regmap_config lm3601x_regmap = { + .reg_bits = 8, + .val_bits = 8, + + .max_register = LM3601X_DEV_ID_REG, + .reg_defaults = lm3601x_regmap_defs, + .num_reg_defaults = ARRAY_SIZE(lm3601x_regmap_defs), + .cache_type = REGCACHE_RBTREE, + .volatile_reg = lm3601x_volatile_reg, +}; + +static struct lm3601x_led *fled_cdev_to_led( + struct led_classdev_flash *fled_cdev) +{ + return container_of(fled_cdev, struct lm3601x_led, fled_cdev); +} + +static int lm3601x_read_faults(struct lm3601x_led *led) +{ + int flags_val; + int ret; + + ret = regmap_read(led->regmap, LM3601X_FLAGS_REG, &flags_val); + if (ret < 0) + return -EIO; + + led->last_flag = flags_val; + + return led->last_flag; +} + +static int lm3601x_torch_brightness_set(struct led_classdev *cdev, + enum led_brightness brightness) +{ + struct lm3601x_led *led = + container_of(cdev, struct lm3601x_led, cdev_torch); + int ret; + u8 brightness_val; + + mutex_lock(&led->lock); + + ret = lm3601x_read_faults(led); + if (ret < 0) + goto out; + + if (brightness == LED_OFF) { + ret = regmap_update_bits(led->regmap, LM3601X_ENABLE_REG, + LM3601X_MODE_TORCH, LED_OFF); + goto out; + } + + if (brightness == LED_ON) + brightness_val = LED_ON; + else + brightness_val = (brightness/2); + + ret = regmap_write(led->regmap, LM3601X_LED_TORCH_REG, brightness_val); + if (ret < 0) + goto out; + + ret = regmap_update_bits(led->regmap, LM3601X_ENABLE_REG, + LM3601X_MODE_TORCH, + LM3601X_MODE_TORCH); + +out: + mutex_unlock(&led->lock); + return ret; +} + +static int lm3601x_strobe_set(struct led_classdev_flash *fled_cdev, + bool state) +{ + struct lm3601x_led *led = fled_cdev_to_led(fled_cdev); + int ret; + int current_timeout; + int reg_count; + int i; + int timeout_reg_val = 0; + + mutex_lock(&led->lock); + + ret = regmap_read(led->regmap, LM3601X_CFG_REG, ¤t_timeout); + if (ret < 0) + goto out; + + reg_count = ARRAY_SIZE(strobe_timeouts); + for (i = 0; i < reg_count; i++) { + if (led->strobe_timeout > strobe_timeouts[i].timeout) + continue; + + if (led->strobe_timeout <= strobe_timeouts[i].timeout) { + timeout_reg_val = (strobe_timeouts[i].reg_val << 1); + break; + } + + ret = -EINVAL; + goto out; + } + + if (led->strobe_timeout != current_timeout) + ret = regmap_update_bits(led->regmap, LM3601X_CFG_REG, + LM3601X_TIMEOUT_MASK, timeout_reg_val); + + if (state) + ret = regmap_update_bits(led->regmap, LM3601X_ENABLE_REG, + LM3601X_MODE_STROBE, + LM3601X_MODE_STROBE); + else + ret = regmap_update_bits(led->regmap, LM3601X_ENABLE_REG, + LM3601X_MODE_STROBE, LED_OFF); + + ret = lm3601x_read_faults(led); +out: + mutex_unlock(&led->lock); + return ret; +} + +static int lm3601x_strobe_brightness_set(struct led_classdev *cdev, + enum led_brightness brightness) +{ + struct led_classdev_flash *fled_cdev = lcdev_to_flcdev(cdev); + struct lm3601x_led *led = fled_cdev_to_led(fled_cdev); + int ret; + u8 brightness_val; + + mutex_lock(&led->lock); + ret = lm3601x_read_faults(led); + if (ret < 0) + goto out; + + if (brightness == LED_OFF) { + ret = regmap_update_bits(led->regmap, LM3601X_ENABLE_REG, + LM3601X_MODE_STROBE, LED_OFF); + goto out; + } + + if (brightness == LED_ON) + brightness_val = LED_ON; + else + brightness_val = (brightness/2); + + ret = regmap_write(led->regmap, LM3601X_LED_FLASH_REG, brightness_val); + +out: + mutex_unlock(&led->lock); + return ret; +} + +static int lm3601x_strobe_timeout_set(struct led_classdev_flash *fled_cdev, + u32 timeout) +{ + struct lm3601x_led *led = fled_cdev_to_led(fled_cdev); + int ret = 0; + + mutex_lock(&led->lock); + + if (timeout > led->max_strobe_timeout) + ret = -EINVAL; + else + led->strobe_timeout = timeout; + + mutex_unlock(&led->lock); + + return ret; +} + +static int lm3601x_strobe_get(struct led_classdev_flash *fled_cdev, + bool *state) +{ + struct lm3601x_led *led = fled_cdev_to_led(fled_cdev); + int ret; + int strobe_state; + + mutex_lock(&led->lock); + + ret = regmap_read(led->regmap, LM3601X_ENABLE_REG, &strobe_state); + if (ret < 0) + goto out; + + *state = strobe_state & LM3601X_MODE_STROBE; + +out: + mutex_unlock(&led->lock); + return ret; +} + +static int lm3601x_ir_brightness_set(struct led_classdev *cdev, + enum led_brightness brightness) +{ + struct lm3601x_led *led = + container_of(cdev, struct lm3601x_led, cdev_ir); + int ret; + u8 brightness_val; + + mutex_lock(&led->lock); + ret = lm3601x_read_faults(led); + if (ret < 0) + goto out; + + if (brightness == LED_OFF) { + ret = regmap_update_bits(led->regmap, LM3601X_ENABLE_REG, + LM3601X_LED_IR, + LED_OFF); + goto out; + } + + if (brightness == LED_ON) + brightness_val = LED_ON; + else + brightness_val = (brightness/2); + + ret = regmap_write(led->regmap, LM3601X_LED_FLASH_REG, brightness_val); + if (ret < 0) + goto out; + + ret = regmap_update_bits(led->regmap, LM3601X_ENABLE_REG, + LM3601X_MODE_IR_DRV, + LM3601X_MODE_IR_DRV); + +out: + mutex_unlock(&led->lock); + return ret; +} + +static int lm3601x_strobe_fault_get(struct led_classdev_flash *fled_cdev, + u32 *fault) +{ + struct lm3601x_led *led = fled_cdev_to_led(fled_cdev); + + lm3601x_read_faults(led); + + *fault = led->last_flag; + + return 0; +} + +static const struct led_flash_ops strobe_ops = { + .strobe_set = lm3601x_strobe_set, + .strobe_get = lm3601x_strobe_get, + .timeout_set = lm3601x_strobe_timeout_set, + .fault_get = lm3601x_strobe_fault_get, +}; + +static int lm3601x_register_leds(struct lm3601x_led *led) +{ + struct led_classdev_flash *fled_cdev; + struct led_classdev *led_cdev; + int err = -ENODEV; + + if (led->torch_node) { + led->cdev_torch.name = led->torch; + led->cdev_torch.max_brightness = LED_FULL; + led->cdev_torch.brightness_set_blocking = lm3601x_torch_brightness_set; + err = devm_led_classdev_register(&led->client->dev, + &led->cdev_torch); + if (err < 0) + return err; + } + + if (led->strobe_node) { + fled_cdev = &led->fled_cdev; + fled_cdev->ops = &strobe_ops; + + led_cdev = &fled_cdev->led_cdev; + led_cdev->name = led->strobe; + led_cdev->max_brightness = LED_FULL; + led_cdev->brightness_set_blocking = lm3601x_strobe_brightness_set; + led_cdev->flags |= LED_DEV_CAP_FLASH; + + err = led_classdev_flash_register(&led->client->dev, + fled_cdev); + if (err < 0) + return err; + } + + if (led->ir_node) { + led->cdev_ir.name = led->ir; + led->cdev_ir.max_brightness = LED_FULL; + led->cdev_ir.brightness_set_blocking = + lm3601x_ir_brightness_set; + + err = devm_led_classdev_register(&led->client->dev, + &led->cdev_ir); + } + + return err; +} + +static void lm3601x_init_flash_timeout(struct lm3601x_led *led) +{ + struct led_flash_setting *setting; + + setting = &led->fled_cdev.timeout; + setting->min = strobe_timeouts[0].timeout; + setting->max = led->max_strobe_timeout; + setting->step = 40; + setting->val = led->max_strobe_timeout; +} + +static int lm3601x_parse_node(struct lm3601x_led *led, + struct device_node *node) +{ + struct device_node *child_node; + const char *name; + int ret = -ENODEV; + + for_each_available_child_of_node(node, child_node) { + u32 id = 0; + + of_property_read_u32(child_node, "reg", &id); + + switch (id) { + case LM3601X_LED_TORCH: + led->torch_node = of_node_get(child_node); + break; + case LM3601X_LED_STROBE: + led->strobe_node = of_node_get(child_node); + break; + case LM3601X_LED_IR: + led->ir_node = of_node_get(child_node); + break; + default: + dev_warn(&led->client->dev, + "unknown LED %u encountered, ignoring\n", id); + break; + } + } + + if (led->torch_node) { + ret = of_property_read_string(led->torch_node, "label", &name); + if (!ret) + snprintf(led->torch, sizeof(led->torch), + "%s:%s", led->torch_node->name, name); + else + snprintf(led->torch, sizeof(led->torch), + "%s:torch", led->torch_node->name); + ret = of_property_read_u32(led->torch_node, "led-max-microamp", + &led->torch_current_max); + if (ret < 0) { + led->torch_current_max = LM3601X_MIN_TORCH_I_UA; + dev_warn(&led->client->dev, + "led-max-microamp DT property missing\n"); + } + + of_node_put(led->torch_node); + } + + if (led->strobe_node) { + ret = of_property_read_string(led->strobe_node, "label", &name); + if (!ret) + snprintf(led->strobe, sizeof(led->strobe), + "%s:%s", led->strobe_node->name, name); + else + snprintf(led->strobe, sizeof(led->strobe), + "%s::strobe", led->strobe_node->name); + + ret = of_property_read_u32(led->strobe_node, + "flash-max-microamp", + &led->strobe_current_max); + if (ret < 0) { + led->strobe_current_max = LM3601X_MIN_STROBE_I_MA; + dev_warn(&led->client->dev, + "flash-max-microamp DT property missing\n"); + } + + ret = of_property_read_u32(led->strobe_node, + "flash-max-timeout-us", + &led->max_strobe_timeout); + if (ret < 0) { + led->max_strobe_timeout = strobe_timeouts[0].reg_val; + dev_warn(&led->client->dev, + "flash-max-timeout-us DT property missing\n"); + } + + lm3601x_init_flash_timeout(led); + + of_node_put(led->strobe_node); + } + + if (led->ir_node) { + ret = of_property_read_string(led->ir_node, "label", &name); + if (!ret) + snprintf(led->ir, sizeof(led->ir), + "%s:%s", led->ir_node->name, name); + else + snprintf(led->ir, sizeof(led->ir), + "%s::infrared", led->ir_node->name); + + of_node_put(led->ir_node); + } + + return ret; +} + +static int lm3601x_probe(struct i2c_client *client, + const struct i2c_device_id *id) +{ + struct lm3601x_led *led; + int err; + + led = devm_kzalloc(&client->dev, + sizeof(struct lm3601x_led), GFP_KERNEL); + if (!led) + return -ENOMEM; + + err = lm3601x_parse_node(led, client->dev.of_node); + if (err < 0) + return -ENODEV; + + led->client = client; + led->regmap = devm_regmap_init_i2c(client, &lm3601x_regmap); + if (IS_ERR(led->regmap)) { + err = PTR_ERR(led->regmap); + dev_err(&client->dev, + "Failed to allocate register map: %d\n", err); + return err; + } + + mutex_init(&led->lock); + i2c_set_clientdata(client, led); + err = lm3601x_register_leds(led); + + return err; +} + +static int lm3601x_remove(struct i2c_client *client) +{ + struct lm3601x_led *led = i2c_get_clientdata(client); + + regmap_update_bits(led->regmap, LM3601X_ENABLE_REG, + LM3601X_ENABLE_MASK, + LM3601X_MODE_STANDBY); + + return 0; +} + +static const struct i2c_device_id lm3601x_id[] = { + { "LM36010", CHIP_LM36010 }, + { "LM36011", CHIP_LM36011 }, + { }, +}; +MODULE_DEVICE_TABLE(i2c, lm3601x_id); + +static const struct of_device_id of_lm3601x_leds_match[] = { + { .compatible = "ti,lm36010", }, + { .compatible = "ti,lm36011", }, + {}, +}; +MODULE_DEVICE_TABLE(of, of_lm3601x_leds_match); + +static struct i2c_driver lm3601x_i2c_driver = { + .driver = { + .name = "lm3601x", + .of_match_table = of_lm3601x_leds_match, + }, + .probe = lm3601x_probe, + .remove = lm3601x_remove, + .id_table = lm3601x_id, +}; +module_i2c_driver(lm3601x_i2c_driver); + +MODULE_DESCRIPTION("Texas Instruments Flash Lighting driver for LM3601X"); +MODULE_AUTHOR("Dan Murphy <dmurphy@ti.com>"); +MODULE_LICENSE("GPL v2"); -- 2.17.0.582.gccdcbd54c ^ permalink raw reply related [flat|nested] 33+ messages in thread
* Re: [PATCH v5 2/2] leds: lm3601x: Introduce the lm3601x LED driver 2018-05-10 17:47 ` Dan Murphy (?) @ 2018-05-10 20:48 ` Jacek Anaszewski 2018-05-11 11:56 ` Dan Murphy -1 siblings, 1 reply; 33+ messages in thread From: Jacek Anaszewski @ 2018-05-10 20:48 UTC (permalink / raw) To: Dan Murphy, robh+dt, mark.rutland, pavel, afd Cc: devicetree, linux-kernel, linux-leds Hi Dan, Thank you for the patch. On 05/10/2018 07:47 PM, Dan Murphy wrote: > Introduce the family of LED devices that can > drive a torch, strobe or IR LED. > > The LED driver can be configured with a strobe > timer to execute a strobe flash. The IR LED > brightness is controlled via the torch brightness > register. > > The data sheet for each the LM36010 and LM36011 > LED drivers can be found here: > http://www.ti.com/product/LM36010 > http://www.ti.com/product/LM36011 > > Signed-off-by: Dan Murphy <dmurphy@ti.com> > --- > > v5 - Fixed magic numbers, change reg cache type, added of_put_node to release > the dt node ref, and I did not change the remove function to leave the LED in its > state on driver removal - https://patchwork.kernel.org/patch/10391741/ > > v4 - Fixed Cocci issue using ARRAY_SIZE - https://patchwork.kernel.org/patch/10389259/ > v3 - removed wildcard dt compatible, fixed copyright, fixed struct doc, removed > RO registers from default, added regmap volatile for FLAGS_REG, updated regmap cache type, > fixed unlock and extra semi colon in strobe_set, removed unnecessary out label > in led register and fixed checking of the ret in brightness_set - https://patchwork.kernel.org/patch/10386243/ > v2 - Fixed kbuild issue and removed unused cdev_strobe - https://patchwork.kernel.org/patch/10384585/ > > > drivers/leds/Kconfig | 9 + > drivers/leds/Makefile | 1 + > drivers/leds/leds-lm3601x.c | 622 ++++++++++++++++++++++++++++++++++++ > 3 files changed, 632 insertions(+) > create mode 100644 drivers/leds/leds-lm3601x.c > > diff --git a/drivers/leds/Kconfig b/drivers/leds/Kconfig > index 2c896c0e69e1..50ae536f343f 100644 > --- a/drivers/leds/Kconfig > +++ b/drivers/leds/Kconfig > @@ -145,6 +145,15 @@ config LEDS_LM3692X > This option enables support for the TI LM3692x family > of white LED string drivers used for backlighting. > > +config LEDS_LM3601X > + tristate "LED support for LM3601x Chips" > + depends on LEDS_CLASS && I2C && OF > + depends on LEDS_CLASS_FLASH > + select REGMAP_I2C > + help > + This option enables support for the TI LM3601x family > + of flash, torch and indicator classes. > + > config LEDS_LOCOMO > tristate "LED Support for Locomo device" > depends on LEDS_CLASS > diff --git a/drivers/leds/Makefile b/drivers/leds/Makefile > index 91eca81cae82..b79807fe1b67 100644 > --- a/drivers/leds/Makefile > +++ b/drivers/leds/Makefile > @@ -76,6 +76,7 @@ obj-$(CONFIG_LEDS_MLXREG) += leds-mlxreg.o > obj-$(CONFIG_LEDS_NIC78BX) += leds-nic78bx.o > obj-$(CONFIG_LEDS_MT6323) += leds-mt6323.o > obj-$(CONFIG_LEDS_LM3692X) += leds-lm3692x.o > +obj-$(CONFIG_LEDS_LM3601X) += leds-lm3601x.o > > # LED SPI Drivers > obj-$(CONFIG_LEDS_DAC124S085) += leds-dac124s085.o > diff --git a/drivers/leds/leds-lm3601x.c b/drivers/leds/leds-lm3601x.c > new file mode 100644 > index 000000000000..3c00886fbc7a > --- /dev/null > +++ b/drivers/leds/leds-lm3601x.c > @@ -0,0 +1,622 @@ > +// SPDX-License-Identifier: GPL-2.0 > +/* > + * Flash and torch driver for Texas Instruments LM3601X LED > + * Flash driver chip family > + * Copyright (C) 2018 Texas Instruments Incorporated - http://www.ti.com/ > + */ Please use uniform "//" comment style here. > +#include <linux/delay.h> > +#include <linux/i2c.h> > +#include <linux/leds.h> > +#include <linux/led-class-flash.h> > +#include <linux/module.h> > +#include <linux/regmap.h> > +#include <linux/slab.h> > +#include <uapi/linux/uleds.h> > + > +#define LM3601X_LED_TORCH 0x0 > +#define LM3601X_LED_STROBE 0x1 > +#define LM3601X_LED_IR 0x2 > + > +/* Registers */ > +#define LM3601X_ENABLE_REG 0x01 > +#define LM3601X_CFG_REG 0x02 > +#define LM3601X_LED_FLASH_REG 0x03 > +#define LM3601X_LED_TORCH_REG 0x04 > +#define LM3601X_FLAGS_REG 0x05 > +#define LM3601X_DEV_ID_REG 0x06 > + > +#define LM3601X_SW_RESET BIT(7) > + > +/* Enable Mode bits */ > +#define LM3601X_MODE_STANDBY 0x00 > +#define LM3601X_MODE_IR_DRV BIT(0) > +#define LM3601X_MODE_TORCH BIT(1) > +#define LM3601X_MODE_STROBE (BIT(0) | BIT(1)) > +#define LM3601X_STRB_EN BIT(2) > +#define LM3601X_STRB_LVL_TRIG ~BIT(3) > +#define LM3601X_STRB_EDGE_TRIG BIT(3) > +#define LM3601X_IVFM_EN BIT(4) > + > +#define LM36010_BOOST_LIMIT_19 ~BIT(5) > +#define LM36010_BOOST_LIMIT_28 BIT(5) > +#define LM36010_BOOST_FREQ_2MHZ ~BIT(6) > +#define LM36010_BOOST_FREQ_4MHZ BIT(6) > +#define LM36010_BOOST_MODE_NORM ~BIT(7) > +#define LM36010_BOOST_MODE_PASS BIT(7) > + > +/* Flag Mask */ > +#define LM3601X_FLASH_TIME_OUT BIT(0) > +#define LM3601X_UVLO_FAULT BIT(1) > +#define LM3601X_THERM_SHUTDOWN BIT(2) > +#define LM3601X_THERM_CURR BIT(3) > +#define LM36010_CURR_LIMIT BIT(4) > +#define LM3601X_SHORT_FAULT BIT(5) > +#define LM3601X_IVFM_TRIP BIT(6) > +#define LM36010_OVP_FAULT BIT(7) > + > +#define LM3601X_MIN_TORCH_I_UA 2400 > +#define LM3601X_MIN_STROBE_I_MA 11 > + > +#define LM3601X_TIMEOUT_MASK 0x1e > +#define LM3601X_ENABLE_MASK 0x03 > + > +enum lm3601x_type { > + CHIP_LM36010 = 0, > + CHIP_LM36011, > +}; > + > +/** > + * struct lm3601x_max_timeouts - > + * @timeout: timeout value in ms > + * @regval: the value of the register to write > + */ > +struct lm3601x_max_timeouts { > + int timeout; > + int reg_val; > +}; > + > +/** > + * struct lm3601x_led - > + * @lock: Lock for reading/writing the device > + * @regmap: Devices register map > + * @client: Pointer to the I2C client > + * @cdev_torch: led class device pointer for the torch > + * @cdev_ir: led class device pointer for infrared > + * @fled_cdev: flash led class device pointer > + * @strobe_node: DT device node for the strobe > + * @torch: LED label for the torch > + * @strobe: LED label for the strobe > + * @ir: LED label for the infrared > + * @last_flag: last known flags register value > + * @strobe_timeout: the timeout for the strobe > + * @torch_current_max: maximum current for the torch > + * @strobe_current_max: maximum current for the strobe > + * @max_strobe_timeout: maximum timeout for the strobe > + */ > +struct lm3601x_led { > + struct mutex lock; > + struct regmap *regmap; > + struct i2c_client *client; > + > + struct led_classdev cdev_torch; > + struct led_classdev cdev_ir; > + > + struct led_classdev_flash fled_cdev; > + > + struct device_node *strobe_node; > + struct device_node *torch_node; > + struct device_node *ir_node; > + > + char torch[LED_MAX_NAME_SIZE]; > + char strobe[LED_MAX_NAME_SIZE]; > + char ir[LED_MAX_NAME_SIZE]; > + > + unsigned int last_flag; > + unsigned int strobe_timeout; > + > + u32 torch_current_max; > + u32 strobe_current_max; > + u32 max_strobe_timeout; > +}; > + > +static const struct lm3601x_max_timeouts strobe_timeouts[] = { > + { 40, 0x00 }, > + { 80, 0x01 }, > + { 120, 0x02 }, > + { 160, 0x03 }, > + { 200, 0x04 }, > + { 240, 0x05 }, > + { 280, 0x06 }, > + { 320, 0x07 }, > + { 360, 0x08 }, > + { 400, 0x09 }, > + { 600, 0x0a }, > + { 800, 0x0b }, > + { 1000, 0x0c }, > + { 1200, 0x0d }, > + { 1400, 0x0e }, > + { 1600, 0x0f }, > +}; > + > +static const struct reg_default lm3601x_regmap_defs[] = { > + { LM3601X_ENABLE_REG, 0x20 }, > + { LM3601X_CFG_REG, 0x15 }, > + { LM3601X_LED_FLASH_REG, 0x00 }, > + { LM3601X_LED_TORCH_REG, 0x00 }, > +}; > + > +static bool lm3601x_volatile_reg(struct device *dev, unsigned int reg) > +{ > + switch (reg) { > + case LM3601X_FLAGS_REG: > + return true; > + default: > + return false; > + } > +} > + > +static const struct regmap_config lm3601x_regmap = { > + .reg_bits = 8, > + .val_bits = 8, > + > + .max_register = LM3601X_DEV_ID_REG, > + .reg_defaults = lm3601x_regmap_defs, > + .num_reg_defaults = ARRAY_SIZE(lm3601x_regmap_defs), > + .cache_type = REGCACHE_RBTREE, > + .volatile_reg = lm3601x_volatile_reg, > +}; > + > +static struct lm3601x_led *fled_cdev_to_led( > + struct led_classdev_flash *fled_cdev) > +{ > + return container_of(fled_cdev, struct lm3601x_led, fled_cdev); > +} > + > +static int lm3601x_read_faults(struct lm3601x_led *led) > +{ > + int flags_val; > + int ret; > + > + ret = regmap_read(led->regmap, LM3601X_FLAGS_REG, &flags_val); > + if (ret < 0) > + return -EIO; > + > + led->last_flag = flags_val; > + > + return led->last_flag; > +} > + > +static int lm3601x_torch_brightness_set(struct led_classdev *cdev, > + enum led_brightness brightness) > +{ > + struct lm3601x_led *led = > + container_of(cdev, struct lm3601x_led, cdev_torch); > + int ret; > + u8 brightness_val; > + > + mutex_lock(&led->lock); > + > + ret = lm3601x_read_faults(led); > + if (ret < 0) > + goto out; > + > + if (brightness == LED_OFF) { > + ret = regmap_update_bits(led->regmap, LM3601X_ENABLE_REG, > + LM3601X_MODE_TORCH, LED_OFF); > + goto out; > + } > + > + if (brightness == LED_ON) > + brightness_val = LED_ON; > + else > + brightness_val = (brightness/2); > + > + ret = regmap_write(led->regmap, LM3601X_LED_TORCH_REG, brightness_val); > + if (ret < 0) > + goto out; > + > + ret = regmap_update_bits(led->regmap, LM3601X_ENABLE_REG, > + LM3601X_MODE_TORCH, > + LM3601X_MODE_TORCH); > + > +out: > + mutex_unlock(&led->lock); > + return ret; > +} > + > +static int lm3601x_strobe_set(struct led_classdev_flash *fled_cdev, > + bool state) > +{ > + struct lm3601x_led *led = fled_cdev_to_led(fled_cdev); > + int ret; > + int current_timeout; > + int reg_count; > + int i; > + int timeout_reg_val = 0; > + > + mutex_lock(&led->lock); > + > + ret = regmap_read(led->regmap, LM3601X_CFG_REG, ¤t_timeout); > + if (ret < 0) > + goto out; > + > + reg_count = ARRAY_SIZE(strobe_timeouts); > + for (i = 0; i < reg_count; i++) { > + if (led->strobe_timeout > strobe_timeouts[i].timeout) > + continue; > + > + if (led->strobe_timeout <= strobe_timeouts[i].timeout) { > + timeout_reg_val = (strobe_timeouts[i].reg_val << 1); > + break; > + } > + > + ret = -EINVAL; > + goto out; > + } > + > + if (led->strobe_timeout != current_timeout) > + ret = regmap_update_bits(led->regmap, LM3601X_CFG_REG, > + LM3601X_TIMEOUT_MASK, timeout_reg_val); > + > + if (state) > + ret = regmap_update_bits(led->regmap, LM3601X_ENABLE_REG, > + LM3601X_MODE_STROBE, > + LM3601X_MODE_STROBE); > + else > + ret = regmap_update_bits(led->regmap, LM3601X_ENABLE_REG, > + LM3601X_MODE_STROBE, LED_OFF); > + > + ret = lm3601x_read_faults(led); > +out: > + mutex_unlock(&led->lock); > + return ret; > +} > + > +static int lm3601x_strobe_brightness_set(struct led_classdev *cdev, > + enum led_brightness brightness) > +{ > + struct led_classdev_flash *fled_cdev = lcdev_to_flcdev(cdev); > + struct lm3601x_led *led = fled_cdev_to_led(fled_cdev); > + int ret; > + u8 brightness_val; > + > + mutex_lock(&led->lock); > + ret = lm3601x_read_faults(led); > + if (ret < 0) > + goto out; > + > + if (brightness == LED_OFF) { > + ret = regmap_update_bits(led->regmap, LM3601X_ENABLE_REG, > + LM3601X_MODE_STROBE, LED_OFF); > + goto out; > + } > + > + if (brightness == LED_ON) > + brightness_val = LED_ON; > + else > + brightness_val = (brightness/2); > + > + ret = regmap_write(led->regmap, LM3601X_LED_FLASH_REG, brightness_val); > + > +out: > + mutex_unlock(&led->lock); > + return ret; > +} > + > +static int lm3601x_strobe_timeout_set(struct led_classdev_flash *fled_cdev, > + u32 timeout) > +{ > + struct lm3601x_led *led = fled_cdev_to_led(fled_cdev); > + int ret = 0; > + > + mutex_lock(&led->lock); > + > + if (timeout > led->max_strobe_timeout) > + ret = -EINVAL; LED flash core takes care of clamping timeout setting according to the defined constraints. > + else > + led->strobe_timeout = timeout; > + > + mutex_unlock(&led->lock); > + > + return ret; > +} > + > +static int lm3601x_strobe_get(struct led_classdev_flash *fled_cdev, > + bool *state) > +{ > + struct lm3601x_led *led = fled_cdev_to_led(fled_cdev); > + int ret; > + int strobe_state; > + > + mutex_lock(&led->lock); > + > + ret = regmap_read(led->regmap, LM3601X_ENABLE_REG, &strobe_state); > + if (ret < 0) > + goto out; > + > + *state = strobe_state & LM3601X_MODE_STROBE; > + > +out: > + mutex_unlock(&led->lock); > + return ret; > +} > + > +static int lm3601x_ir_brightness_set(struct led_classdev *cdev, > + enum led_brightness brightness) > +{ > + struct lm3601x_led *led = > + container_of(cdev, struct lm3601x_led, cdev_ir); > + int ret; > + u8 brightness_val; > + > + mutex_lock(&led->lock); > + ret = lm3601x_read_faults(led); > + if (ret < 0) > + goto out; > + > + if (brightness == LED_OFF) { > + ret = regmap_update_bits(led->regmap, LM3601X_ENABLE_REG, > + LM3601X_LED_IR, > + LED_OFF); > + goto out; > + } > + > + if (brightness == LED_ON) > + brightness_val = LED_ON; > + else > + brightness_val = (brightness/2); > + > + ret = regmap_write(led->regmap, LM3601X_LED_FLASH_REG, brightness_val); > + if (ret < 0) > + goto out; > + > + ret = regmap_update_bits(led->regmap, LM3601X_ENABLE_REG, > + LM3601X_MODE_IR_DRV, > + LM3601X_MODE_IR_DRV); > + > +out: > + mutex_unlock(&led->lock); > + return ret; > +} > + > +static int lm3601x_strobe_fault_get(struct led_classdev_flash *fled_cdev, > + u32 *fault) Does the device report only strobe related faults? > +{ > + struct lm3601x_led *led = fled_cdev_to_led(fled_cdev); > + > + lm3601x_read_faults(led); > + > + *fault = led->last_flag; You should translate device specific faults to LED_FAULT* flags. Please compare e.g. drivers/leds/leds-max77693.c or driver/leds/leds-as3645a.c. > + > + return 0; > +} > + > +static const struct led_flash_ops strobe_ops = { > + .strobe_set = lm3601x_strobe_set, > + .strobe_get = lm3601x_strobe_get, > + .timeout_set = lm3601x_strobe_timeout_set, > + .fault_get = lm3601x_strobe_fault_get, > +}; > + > +static int lm3601x_register_leds(struct lm3601x_led *led) > +{ > + struct led_classdev_flash *fled_cdev; > + struct led_classdev *led_cdev; > + int err = -ENODEV; > + > + if (led->torch_node) { > + led->cdev_torch.name = led->torch; > + led->cdev_torch.max_brightness = LED_FULL; > + led->cdev_torch.brightness_set_blocking = lm3601x_torch_brightness_set; > + err = devm_led_classdev_register(&led->client->dev, > + &led->cdev_torch); > + if (err < 0) > + return err; > + } > + > + if (led->strobe_node) { > + fled_cdev = &led->fled_cdev; > + fled_cdev->ops = &strobe_ops; > + > + led_cdev = &fled_cdev->led_cdev; > + led_cdev->name = led->strobe; > + led_cdev->max_brightness = LED_FULL; > + led_cdev->brightness_set_blocking = lm3601x_strobe_brightness_set; > + led_cdev->flags |= LED_DEV_CAP_FLASH; > + > + err = led_classdev_flash_register(&led->client->dev, > + fled_cdev); > + if (err < 0) > + return err; > + } > + > + if (led->ir_node) { > + led->cdev_ir.name = led->ir; > + led->cdev_ir.max_brightness = LED_FULL; > + led->cdev_ir.brightness_set_blocking = > + lm3601x_ir_brightness_set; > + > + err = devm_led_classdev_register(&led->client->dev, > + &led->cdev_ir); > + } > + > + return err; > +} > + > +static void lm3601x_init_flash_timeout(struct lm3601x_led *led) > +{ > + struct led_flash_setting *setting; > + > + setting = &led->fled_cdev.timeout; > + setting->min = strobe_timeouts[0].timeout; > + setting->max = led->max_strobe_timeout; > + setting->step = 40; > + setting->val = led->max_strobe_timeout; > +} > + > +static int lm3601x_parse_node(struct lm3601x_led *led, > + struct device_node *node) > +{ > + struct device_node *child_node; > + const char *name; > + int ret = -ENODEV; > + > + for_each_available_child_of_node(node, child_node) { > + u32 id = 0; > + > + of_property_read_u32(child_node, "reg", &id); > + > + switch (id) { > + case LM3601X_LED_TORCH: > + led->torch_node = of_node_get(child_node); > + break; > + case LM3601X_LED_STROBE: > + led->strobe_node = of_node_get(child_node); > + break; > + case LM3601X_LED_IR: > + led->ir_node = of_node_get(child_node); > + break; > + default: > + dev_warn(&led->client->dev, > + "unknown LED %u encountered, ignoring\n", id); > + break; > + } > + } > + > + if (led->torch_node) { > + ret = of_property_read_string(led->torch_node, "label", &name); > + if (!ret) > + snprintf(led->torch, sizeof(led->torch), > + "%s:%s", led->torch_node->name, name); > + else > + snprintf(led->torch, sizeof(led->torch), > + "%s:torch", led->torch_node->name); > + ret = of_property_read_u32(led->torch_node, "led-max-microamp", > + &led->torch_current_max); > + if (ret < 0) { > + led->torch_current_max = LM3601X_MIN_TORCH_I_UA; > + dev_warn(&led->client->dev, > + "led-max-microamp DT property missing\n"); > + } > + > + of_node_put(led->torch_node); > + } > + > + if (led->strobe_node) { > + ret = of_property_read_string(led->strobe_node, "label", &name); > + if (!ret) > + snprintf(led->strobe, sizeof(led->strobe), > + "%s:%s", led->strobe_node->name, name); > + else > + snprintf(led->strobe, sizeof(led->strobe), > + "%s::strobe", led->strobe_node->name); > + > + ret = of_property_read_u32(led->strobe_node, > + "flash-max-microamp", > + &led->strobe_current_max); > + if (ret < 0) { > + led->strobe_current_max = LM3601X_MIN_STROBE_I_MA; > + dev_warn(&led->client->dev, > + "flash-max-microamp DT property missing\n"); > + } > + > + ret = of_property_read_u32(led->strobe_node, > + "flash-max-timeout-us", > + &led->max_strobe_timeout); > + if (ret < 0) { > + led->max_strobe_timeout = strobe_timeouts[0].reg_val; > + dev_warn(&led->client->dev, > + "flash-max-timeout-us DT property missing\n"); > + } Common LED bindings state that flash-max-microamp and flash-max-timeout-us properties are mandatory. > + > + lm3601x_init_flash_timeout(led); > + > + of_node_put(led->strobe_node); > + } > + > + if (led->ir_node) { > + ret = of_property_read_string(led->ir_node, "label", &name); > + if (!ret) > + snprintf(led->ir, sizeof(led->ir), > + "%s:%s", led->ir_node->name, name); > + else > + snprintf(led->ir, sizeof(led->ir), > + "%s::infrared", led->ir_node->name); > + > + of_node_put(led->ir_node); > + } > + > + return ret; > +} > + > +static int lm3601x_probe(struct i2c_client *client, > + const struct i2c_device_id *id) > +{ > + struct lm3601x_led *led; > + int err; > + > + led = devm_kzalloc(&client->dev, > + sizeof(struct lm3601x_led), GFP_KERNEL); > + if (!led) > + return -ENOMEM; > + > + err = lm3601x_parse_node(led, client->dev.of_node); > + if (err < 0) > + return -ENODEV; > + > + led->client = client; > + led->regmap = devm_regmap_init_i2c(client, &lm3601x_regmap); > + if (IS_ERR(led->regmap)) { > + err = PTR_ERR(led->regmap); > + dev_err(&client->dev, > + "Failed to allocate register map: %d\n", err); > + return err; > + } > + > + mutex_init(&led->lock); > + i2c_set_clientdata(client, led); > + err = lm3601x_register_leds(led); > + > + return err; > +} > + > +static int lm3601x_remove(struct i2c_client *client) > +{ > + struct lm3601x_led *led = i2c_get_clientdata(client); > + > + regmap_update_bits(led->regmap, LM3601X_ENABLE_REG, > + LM3601X_ENABLE_MASK, > + LM3601X_MODE_STANDBY); > + > + return 0; > +} > + > +static const struct i2c_device_id lm3601x_id[] = { > + { "LM36010", CHIP_LM36010 }, > + { "LM36011", CHIP_LM36011 }, > + { }, > +}; > +MODULE_DEVICE_TABLE(i2c, lm3601x_id); > + > +static const struct of_device_id of_lm3601x_leds_match[] = { > + { .compatible = "ti,lm36010", }, > + { .compatible = "ti,lm36011", }, > + {}, > +}; > +MODULE_DEVICE_TABLE(of, of_lm3601x_leds_match); > + > +static struct i2c_driver lm3601x_i2c_driver = { > + .driver = { > + .name = "lm3601x", > + .of_match_table = of_lm3601x_leds_match, > + }, > + .probe = lm3601x_probe, > + .remove = lm3601x_remove, > + .id_table = lm3601x_id, > +}; > +module_i2c_driver(lm3601x_i2c_driver); > + > +MODULE_DESCRIPTION("Texas Instruments Flash Lighting driver for LM3601X"); > +MODULE_AUTHOR("Dan Murphy <dmurphy@ti.com>"); > +MODULE_LICENSE("GPL v2"); > -- Best regards, Jacek Anaszewski ^ permalink raw reply [flat|nested] 33+ messages in thread
* Re: [PATCH v5 2/2] leds: lm3601x: Introduce the lm3601x LED driver 2018-05-10 20:48 ` Jacek Anaszewski @ 2018-05-11 11:56 ` Dan Murphy 0 siblings, 0 replies; 33+ messages in thread From: Dan Murphy @ 2018-05-11 11:56 UTC (permalink / raw) To: Jacek Anaszewski, robh+dt, mark.rutland, pavel, afd Cc: devicetree, linux-kernel, linux-leds Jacek Thanks for the review On 05/10/2018 03:48 PM, Jacek Anaszewski wrote: > Hi Dan, > > Thank you for the patch. > > On 05/10/2018 07:47 PM, Dan Murphy wrote: >> Introduce the family of LED devices that can >> drive a torch, strobe or IR LED. >> >> The LED driver can be configured with a strobe >> timer to execute a strobe flash. The IR LED >> brightness is controlled via the torch brightness >> register. >> >> The data sheet for each the LM36010 and LM36011 >> LED drivers can be found here: >> http://www.ti.com/product/LM36010 >> http://www.ti.com/product/LM36011 >> >> Signed-off-by: Dan Murphy <dmurphy@ti.com> >> --- >> >> v5 - Fixed magic numbers, change reg cache type, added of_put_node to release >> the dt node ref, and I did not change the remove function to leave the LED in its >> state on driver removal - https://patchwork.kernel.org/patch/10391741/ >> >> v4 - Fixed Cocci issue using ARRAY_SIZE - https://patchwork.kernel.org/patch/10389259/ >> v3 - removed wildcard dt compatible, fixed copyright, fixed struct doc, removed >> RO registers from default, added regmap volatile for FLAGS_REG, updated regmap cache type, >> fixed unlock and extra semi colon in strobe_set, removed unnecessary out label >> in led register and fixed checking of the ret in brightness_set - https://patchwork.kernel.org/patch/10386243/ >> v2 - Fixed kbuild issue and removed unused cdev_strobe - https://patchwork.kernel.org/patch/10384585/ >> >> >> drivers/leds/Kconfig | 9 + >> drivers/leds/Makefile | 1 + >> drivers/leds/leds-lm3601x.c | 622 ++++++++++++++++++++++++++++++++++++ >> 3 files changed, 632 insertions(+) >> create mode 100644 drivers/leds/leds-lm3601x.c >> >> diff --git a/drivers/leds/Kconfig b/drivers/leds/Kconfig >> index 2c896c0e69e1..50ae536f343f 100644 >> --- a/drivers/leds/Kconfig >> +++ b/drivers/leds/Kconfig >> @@ -145,6 +145,15 @@ config LEDS_LM3692X >> This option enables support for the TI LM3692x family >> of white LED string drivers used for backlighting. >> +config LEDS_LM3601X >> + tristate "LED support for LM3601x Chips" >> + depends on LEDS_CLASS && I2C && OF >> + depends on LEDS_CLASS_FLASH >> + select REGMAP_I2C >> + help >> + This option enables support for the TI LM3601x family >> + of flash, torch and indicator classes. >> + >> config LEDS_LOCOMO >> tristate "LED Support for Locomo device" >> depends on LEDS_CLASS >> diff --git a/drivers/leds/Makefile b/drivers/leds/Makefile >> index 91eca81cae82..b79807fe1b67 100644 >> --- a/drivers/leds/Makefile >> +++ b/drivers/leds/Makefile >> @@ -76,6 +76,7 @@ obj-$(CONFIG_LEDS_MLXREG) += leds-mlxreg.o >> obj-$(CONFIG_LEDS_NIC78BX) += leds-nic78bx.o >> obj-$(CONFIG_LEDS_MT6323) += leds-mt6323.o >> obj-$(CONFIG_LEDS_LM3692X) += leds-lm3692x.o >> +obj-$(CONFIG_LEDS_LM3601X) += leds-lm3601x.o >> # LED SPI Drivers >> obj-$(CONFIG_LEDS_DAC124S085) += leds-dac124s085.o >> diff --git a/drivers/leds/leds-lm3601x.c b/drivers/leds/leds-lm3601x.c >> new file mode 100644 >> index 000000000000..3c00886fbc7a >> --- /dev/null >> +++ b/drivers/leds/leds-lm3601x.c >> @@ -0,0 +1,622 @@ >> +// SPDX-License-Identifier: GPL-2.0 >> +/* >> + * Flash and torch driver for Texas Instruments LM3601X LED >> + * Flash driver chip family >> + * Copyright (C) 2018 Texas Instruments Incorporated - http://www.ti.com/ >> + */ > > Please use uniform "//" comment style here. Is this a LEDs only requirement because I checked a few other files and this is how the license and header is presented. Unless I missed some new change to source code headers. And the documentation does not dictate any change to this. > >> +#include <linux/delay.h> >> +#include <linux/i2c.h> >> +#include <linux/leds.h> >> +#include <linux/led-class-flash.h> >> +#include <linux/module.h> >> +#include <linux/regmap.h> >> +#include <linux/slab.h> >> +#include <uapi/linux/uleds.h> >> + >> +#define LM3601X_LED_TORCH 0x0 >> +#define LM3601X_LED_STROBE 0x1 >> +#define LM3601X_LED_IR 0x2 >> + >> +/* Registers */ >> +#define LM3601X_ENABLE_REG 0x01 >> +#define LM3601X_CFG_REG 0x02 >> +#define LM3601X_LED_FLASH_REG 0x03 >> +#define LM3601X_LED_TORCH_REG 0x04 >> +#define LM3601X_FLAGS_REG 0x05 >> +#define LM3601X_DEV_ID_REG 0x06 >> + >> +#define LM3601X_SW_RESET BIT(7) >> + >> +/* Enable Mode bits */ >> +#define LM3601X_MODE_STANDBY 0x00 >> +#define LM3601X_MODE_IR_DRV BIT(0) >> +#define LM3601X_MODE_TORCH BIT(1) >> +#define LM3601X_MODE_STROBE (BIT(0) | BIT(1)) >> +#define LM3601X_STRB_EN BIT(2) >> +#define LM3601X_STRB_LVL_TRIG ~BIT(3) >> +#define LM3601X_STRB_EDGE_TRIG BIT(3) >> +#define LM3601X_IVFM_EN BIT(4) >> + >> +#define LM36010_BOOST_LIMIT_19 ~BIT(5) >> +#define LM36010_BOOST_LIMIT_28 BIT(5) >> +#define LM36010_BOOST_FREQ_2MHZ ~BIT(6) >> +#define LM36010_BOOST_FREQ_4MHZ BIT(6) >> +#define LM36010_BOOST_MODE_NORM ~BIT(7) >> +#define LM36010_BOOST_MODE_PASS BIT(7) >> + >> +/* Flag Mask */ >> +#define LM3601X_FLASH_TIME_OUT BIT(0) >> +#define LM3601X_UVLO_FAULT BIT(1) >> +#define LM3601X_THERM_SHUTDOWN BIT(2) >> +#define LM3601X_THERM_CURR BIT(3) >> +#define LM36010_CURR_LIMIT BIT(4) >> +#define LM3601X_SHORT_FAULT BIT(5) >> +#define LM3601X_IVFM_TRIP BIT(6) >> +#define LM36010_OVP_FAULT BIT(7) >> + >> +#define LM3601X_MIN_TORCH_I_UA 2400 >> +#define LM3601X_MIN_STROBE_I_MA 11 >> + >> +#define LM3601X_TIMEOUT_MASK 0x1e >> +#define LM3601X_ENABLE_MASK 0x03 >> + >> +enum lm3601x_type { >> + CHIP_LM36010 = 0, >> + CHIP_LM36011, >> +}; >> + >> +/** >> + * struct lm3601x_max_timeouts - >> + * @timeout: timeout value in ms >> + * @regval: the value of the register to write >> + */ >> +struct lm3601x_max_timeouts { >> + int timeout; >> + int reg_val; >> +}; >> + >> +/** >> + * struct lm3601x_led - >> + * @lock: Lock for reading/writing the device >> + * @regmap: Devices register map >> + * @client: Pointer to the I2C client >> + * @cdev_torch: led class device pointer for the torch >> + * @cdev_ir: led class device pointer for infrared >> + * @fled_cdev: flash led class device pointer >> + * @strobe_node: DT device node for the strobe >> + * @torch: LED label for the torch >> + * @strobe: LED label for the strobe >> + * @ir: LED label for the infrared >> + * @last_flag: last known flags register value >> + * @strobe_timeout: the timeout for the strobe >> + * @torch_current_max: maximum current for the torch >> + * @strobe_current_max: maximum current for the strobe >> + * @max_strobe_timeout: maximum timeout for the strobe >> + */ >> +struct lm3601x_led { >> + struct mutex lock; >> + struct regmap *regmap; >> + struct i2c_client *client; >> + >> + struct led_classdev cdev_torch; >> + struct led_classdev cdev_ir; >> + >> + struct led_classdev_flash fled_cdev; >> + >> + struct device_node *strobe_node; >> + struct device_node *torch_node; >> + struct device_node *ir_node; >> + >> + char torch[LED_MAX_NAME_SIZE]; >> + char strobe[LED_MAX_NAME_SIZE]; >> + char ir[LED_MAX_NAME_SIZE]; >> + >> + unsigned int last_flag; >> + unsigned int strobe_timeout; >> + >> + u32 torch_current_max; >> + u32 strobe_current_max; >> + u32 max_strobe_timeout; >> +}; >> + >> +static const struct lm3601x_max_timeouts strobe_timeouts[] = { >> + { 40, 0x00 }, >> + { 80, 0x01 }, >> + { 120, 0x02 }, >> + { 160, 0x03 }, >> + { 200, 0x04 }, >> + { 240, 0x05 }, >> + { 280, 0x06 }, >> + { 320, 0x07 }, >> + { 360, 0x08 }, >> + { 400, 0x09 }, >> + { 600, 0x0a }, >> + { 800, 0x0b }, >> + { 1000, 0x0c }, >> + { 1200, 0x0d }, >> + { 1400, 0x0e }, >> + { 1600, 0x0f }, >> +}; >> + >> +static const struct reg_default lm3601x_regmap_defs[] = { >> + { LM3601X_ENABLE_REG, 0x20 }, >> + { LM3601X_CFG_REG, 0x15 }, >> + { LM3601X_LED_FLASH_REG, 0x00 }, >> + { LM3601X_LED_TORCH_REG, 0x00 }, >> +}; >> + >> +static bool lm3601x_volatile_reg(struct device *dev, unsigned int reg) >> +{ >> + switch (reg) { >> + case LM3601X_FLAGS_REG: >> + return true; >> + default: >> + return false; >> + } >> +} >> + >> +static const struct regmap_config lm3601x_regmap = { >> + .reg_bits = 8, >> + .val_bits = 8, >> + >> + .max_register = LM3601X_DEV_ID_REG, >> + .reg_defaults = lm3601x_regmap_defs, >> + .num_reg_defaults = ARRAY_SIZE(lm3601x_regmap_defs), >> + .cache_type = REGCACHE_RBTREE, >> + .volatile_reg = lm3601x_volatile_reg, >> +}; >> + >> +static struct lm3601x_led *fled_cdev_to_led( >> + struct led_classdev_flash *fled_cdev) >> +{ >> + return container_of(fled_cdev, struct lm3601x_led, fled_cdev); >> +} >> + >> +static int lm3601x_read_faults(struct lm3601x_led *led) >> +{ >> + int flags_val; >> + int ret; >> + >> + ret = regmap_read(led->regmap, LM3601X_FLAGS_REG, &flags_val); >> + if (ret < 0) >> + return -EIO; >> + >> + led->last_flag = flags_val; >> + >> + return led->last_flag; >> +} >> + >> +static int lm3601x_torch_brightness_set(struct led_classdev *cdev, >> + enum led_brightness brightness) >> +{ >> + struct lm3601x_led *led = >> + container_of(cdev, struct lm3601x_led, cdev_torch); >> + int ret; >> + u8 brightness_val; >> + >> + mutex_lock(&led->lock); >> + >> + ret = lm3601x_read_faults(led); >> + if (ret < 0) >> + goto out; >> + >> + if (brightness == LED_OFF) { >> + ret = regmap_update_bits(led->regmap, LM3601X_ENABLE_REG, >> + LM3601X_MODE_TORCH, LED_OFF); >> + goto out; >> + } >> + >> + if (brightness == LED_ON) >> + brightness_val = LED_ON; >> + else >> + brightness_val = (brightness/2); >> + >> + ret = regmap_write(led->regmap, LM3601X_LED_TORCH_REG, brightness_val); >> + if (ret < 0) >> + goto out; >> + >> + ret = regmap_update_bits(led->regmap, LM3601X_ENABLE_REG, >> + LM3601X_MODE_TORCH, >> + LM3601X_MODE_TORCH); >> + >> +out: >> + mutex_unlock(&led->lock); >> + return ret; >> +} >> + >> +static int lm3601x_strobe_set(struct led_classdev_flash *fled_cdev, >> + bool state) >> +{ >> + struct lm3601x_led *led = fled_cdev_to_led(fled_cdev); >> + int ret; >> + int current_timeout; >> + int reg_count; >> + int i; >> + int timeout_reg_val = 0; >> + >> + mutex_lock(&led->lock); >> + >> + ret = regmap_read(led->regmap, LM3601X_CFG_REG, ¤t_timeout); >> + if (ret < 0) >> + goto out; >> + >> + reg_count = ARRAY_SIZE(strobe_timeouts); >> + for (i = 0; i < reg_count; i++) { >> + if (led->strobe_timeout > strobe_timeouts[i].timeout) >> + continue; >> + >> + if (led->strobe_timeout <= strobe_timeouts[i].timeout) { >> + timeout_reg_val = (strobe_timeouts[i].reg_val << 1); >> + break; >> + } >> + >> + ret = -EINVAL; >> + goto out; >> + } >> + >> + if (led->strobe_timeout != current_timeout) >> + ret = regmap_update_bits(led->regmap, LM3601X_CFG_REG, >> + LM3601X_TIMEOUT_MASK, timeout_reg_val); >> + >> + if (state) >> + ret = regmap_update_bits(led->regmap, LM3601X_ENABLE_REG, >> + LM3601X_MODE_STROBE, >> + LM3601X_MODE_STROBE); >> + else >> + ret = regmap_update_bits(led->regmap, LM3601X_ENABLE_REG, >> + LM3601X_MODE_STROBE, LED_OFF); >> + >> + ret = lm3601x_read_faults(led); >> +out: >> + mutex_unlock(&led->lock); >> + return ret; >> +} >> + >> +static int lm3601x_strobe_brightness_set(struct led_classdev *cdev, >> + enum led_brightness brightness) >> +{ >> + struct led_classdev_flash *fled_cdev = lcdev_to_flcdev(cdev); >> + struct lm3601x_led *led = fled_cdev_to_led(fled_cdev); >> + int ret; >> + u8 brightness_val; >> + >> + mutex_lock(&led->lock); >> + ret = lm3601x_read_faults(led); >> + if (ret < 0) >> + goto out; >> + >> + if (brightness == LED_OFF) { >> + ret = regmap_update_bits(led->regmap, LM3601X_ENABLE_REG, >> + LM3601X_MODE_STROBE, LED_OFF); >> + goto out; >> + } >> + >> + if (brightness == LED_ON) >> + brightness_val = LED_ON; >> + else >> + brightness_val = (brightness/2); >> + >> + ret = regmap_write(led->regmap, LM3601X_LED_FLASH_REG, brightness_val); >> + >> +out: >> + mutex_unlock(&led->lock); >> + return ret; >> +} >> + >> +static int lm3601x_strobe_timeout_set(struct led_classdev_flash *fled_cdev, >> + u32 timeout) >> +{ >> + struct lm3601x_led *led = fled_cdev_to_led(fled_cdev); >> + int ret = 0; >> + >> + mutex_lock(&led->lock); >> + >> + if (timeout > led->max_strobe_timeout) >> + ret = -EINVAL; > > LED flash core takes care of clamping timeout setting > according to the defined constraints. OK > >> + else >> + led->strobe_timeout = timeout; >> + >> + mutex_unlock(&led->lock); >> + >> + return ret; >> +} >> + >> +static int lm3601x_strobe_get(struct led_classdev_flash *fled_cdev, >> + bool *state) >> +{ >> + struct lm3601x_led *led = fled_cdev_to_led(fled_cdev); >> + int ret; >> + int strobe_state; >> + >> + mutex_lock(&led->lock); >> + >> + ret = regmap_read(led->regmap, LM3601X_ENABLE_REG, &strobe_state); >> + if (ret < 0) >> + goto out; >> + >> + *state = strobe_state & LM3601X_MODE_STROBE; >> + >> +out: >> + mutex_unlock(&led->lock); >> + return ret; >> +} >> + >> +static int lm3601x_ir_brightness_set(struct led_classdev *cdev, >> + enum led_brightness brightness) >> +{ >> + struct lm3601x_led *led = >> + container_of(cdev, struct lm3601x_led, cdev_ir); >> + int ret; >> + u8 brightness_val; >> + >> + mutex_lock(&led->lock); >> + ret = lm3601x_read_faults(led); >> + if (ret < 0) >> + goto out; >> + >> + if (brightness == LED_OFF) { >> + ret = regmap_update_bits(led->regmap, LM3601X_ENABLE_REG, >> + LM3601X_LED_IR, >> + LED_OFF); >> + goto out; >> + } >> + >> + if (brightness == LED_ON) >> + brightness_val = LED_ON; >> + else >> + brightness_val = (brightness/2); >> + >> + ret = regmap_write(led->regmap, LM3601X_LED_FLASH_REG, brightness_val); >> + if (ret < 0) >> + goto out; >> + >> + ret = regmap_update_bits(led->regmap, LM3601X_ENABLE_REG, >> + LM3601X_MODE_IR_DRV, >> + LM3601X_MODE_IR_DRV); >> + >> +out: >> + mutex_unlock(&led->lock); >> + return ret; >> +} >> + >> +static int lm3601x_strobe_fault_get(struct led_classdev_flash *fled_cdev, >> + u32 *fault) > > Does the device report only strobe related faults? No. It has under/over voltage and thermal. > >> +{ >> + struct lm3601x_led *led = fled_cdev_to_led(fled_cdev); >> + >> + lm3601x_read_faults(led); >> + >> + *fault = led->last_flag; > > You should translate device specific faults to LED_FAULT* flags. > Please compare e.g. drivers/leds/leds-max77693.c or > driver/leds/leds-as3645a.c. Hmm. This code is in the max77693.c. I think the translation should be in the read_faults function in this file and not here. I will add that translation above > >> + >> + return 0; >> +} >> + >> +static const struct led_flash_ops strobe_ops = { >> + .strobe_set = lm3601x_strobe_set, >> + .strobe_get = lm3601x_strobe_get, >> + .timeout_set = lm3601x_strobe_timeout_set, >> + .fault_get = lm3601x_strobe_fault_get, >> +}; >> + >> +static int lm3601x_register_leds(struct lm3601x_led *led) >> +{ >> + struct led_classdev_flash *fled_cdev; >> + struct led_classdev *led_cdev; >> + int err = -ENODEV; >> + >> + if (led->torch_node) { >> + led->cdev_torch.name = led->torch; >> + led->cdev_torch.max_brightness = LED_FULL; >> + led->cdev_torch.brightness_set_blocking = lm3601x_torch_brightness_set; >> + err = devm_led_classdev_register(&led->client->dev, >> + &led->cdev_torch); >> + if (err < 0) >> + return err; >> + } >> + >> + if (led->strobe_node) { >> + fled_cdev = &led->fled_cdev; >> + fled_cdev->ops = &strobe_ops; >> + >> + led_cdev = &fled_cdev->led_cdev; >> + led_cdev->name = led->strobe; >> + led_cdev->max_brightness = LED_FULL; >> + led_cdev->brightness_set_blocking = lm3601x_strobe_brightness_set; >> + led_cdev->flags |= LED_DEV_CAP_FLASH; >> + >> + err = led_classdev_flash_register(&led->client->dev, >> + fled_cdev); >> + if (err < 0) >> + return err; >> + } >> + >> + if (led->ir_node) { >> + led->cdev_ir.name = led->ir; >> + led->cdev_ir.max_brightness = LED_FULL; >> + led->cdev_ir.brightness_set_blocking = >> + lm3601x_ir_brightness_set; >> + >> + err = devm_led_classdev_register(&led->client->dev, >> + &led->cdev_ir); >> + } >> + >> + return err; >> +} >> + >> +static void lm3601x_init_flash_timeout(struct lm3601x_led *led) >> +{ >> + struct led_flash_setting *setting; >> + >> + setting = &led->fled_cdev.timeout; >> + setting->min = strobe_timeouts[0].timeout; >> + setting->max = led->max_strobe_timeout; >> + setting->step = 40; >> + setting->val = led->max_strobe_timeout; >> +} >> + >> +static int lm3601x_parse_node(struct lm3601x_led *led, >> + struct device_node *node) >> +{ >> + struct device_node *child_node; >> + const char *name; >> + int ret = -ENODEV; >> + >> + for_each_available_child_of_node(node, child_node) { >> + u32 id = 0; >> + >> + of_property_read_u32(child_node, "reg", &id); >> + >> + switch (id) { >> + case LM3601X_LED_TORCH: >> + led->torch_node = of_node_get(child_node); >> + break; >> + case LM3601X_LED_STROBE: >> + led->strobe_node = of_node_get(child_node); >> + break; >> + case LM3601X_LED_IR: >> + led->ir_node = of_node_get(child_node); >> + break; >> + default: >> + dev_warn(&led->client->dev, >> + "unknown LED %u encountered, ignoring\n", id); >> + break; >> + } >> + } >> + >> + if (led->torch_node) { >> + ret = of_property_read_string(led->torch_node, "label", &name); >> + if (!ret) >> + snprintf(led->torch, sizeof(led->torch), >> + "%s:%s", led->torch_node->name, name); >> + else >> + snprintf(led->torch, sizeof(led->torch), >> + "%s:torch", led->torch_node->name); >> + ret = of_property_read_u32(led->torch_node, "led-max-microamp", >> + &led->torch_current_max); >> + if (ret < 0) { >> + led->torch_current_max = LM3601X_MIN_TORCH_I_UA; >> + dev_warn(&led->client->dev, >> + "led-max-microamp DT property missing\n"); >> + } >> + >> + of_node_put(led->torch_node); >> + } >> + >> + if (led->strobe_node) { >> + ret = of_property_read_string(led->strobe_node, "label", &name); >> + if (!ret) >> + snprintf(led->strobe, sizeof(led->strobe), >> + "%s:%s", led->strobe_node->name, name); >> + else >> + snprintf(led->strobe, sizeof(led->strobe), >> + "%s::strobe", led->strobe_node->name); >> + >> + ret = of_property_read_u32(led->strobe_node, >> + "flash-max-microamp", >> + &led->strobe_current_max); >> + if (ret < 0) { >> + led->strobe_current_max = LM3601X_MIN_STROBE_I_MA; >> + dev_warn(&led->client->dev, >> + "flash-max-microamp DT property missing\n"); >> + } >> + >> + ret = of_property_read_u32(led->strobe_node, >> + "flash-max-timeout-us", >> + &led->max_strobe_timeout); >> + if (ret < 0) { >> + led->max_strobe_timeout = strobe_timeouts[0].reg_val; >> + dev_warn(&led->client->dev, >> + "flash-max-timeout-us DT property missing\n"); >> + } > > Common LED bindings state that flash-max-microamp and > flash-max-timeout-us properties are mandatory. OK. > >> + >> + lm3601x_init_flash_timeout(led); >> + >> + of_node_put(led->strobe_node); >> + } >> + >> + if (led->ir_node) { >> + ret = of_property_read_string(led->ir_node, "label", &name); >> + if (!ret) >> + snprintf(led->ir, sizeof(led->ir), >> + "%s:%s", led->ir_node->name, name); >> + else >> + snprintf(led->ir, sizeof(led->ir), >> + "%s::infrared", led->ir_node->name); >> + >> + of_node_put(led->ir_node); >> + } >> + >> + return ret; >> +} >> + >> +static int lm3601x_probe(struct i2c_client *client, >> + const struct i2c_device_id *id) >> +{ >> + struct lm3601x_led *led; >> + int err; >> + >> + led = devm_kzalloc(&client->dev, >> + sizeof(struct lm3601x_led), GFP_KERNEL); >> + if (!led) >> + return -ENOMEM; >> + >> + err = lm3601x_parse_node(led, client->dev.of_node); >> + if (err < 0) >> + return -ENODEV; >> + >> + led->client = client; >> + led->regmap = devm_regmap_init_i2c(client, &lm3601x_regmap); >> + if (IS_ERR(led->regmap)) { >> + err = PTR_ERR(led->regmap); >> + dev_err(&client->dev, >> + "Failed to allocate register map: %d\n", err); >> + return err; >> + } >> + >> + mutex_init(&led->lock); >> + i2c_set_clientdata(client, led); >> + err = lm3601x_register_leds(led); >> + >> + return err; >> +} >> + >> +static int lm3601x_remove(struct i2c_client *client) >> +{ >> + struct lm3601x_led *led = i2c_get_clientdata(client); >> + >> + regmap_update_bits(led->regmap, LM3601X_ENABLE_REG, >> + LM3601X_ENABLE_MASK, >> + LM3601X_MODE_STANDBY); >> + >> + return 0; >> +} >> + >> +static const struct i2c_device_id lm3601x_id[] = { >> + { "LM36010", CHIP_LM36010 }, >> + { "LM36011", CHIP_LM36011 }, >> + { }, >> +}; >> +MODULE_DEVICE_TABLE(i2c, lm3601x_id); >> + >> +static const struct of_device_id of_lm3601x_leds_match[] = { >> + { .compatible = "ti,lm36010", }, >> + { .compatible = "ti,lm36011", }, >> + {}, >> +}; >> +MODULE_DEVICE_TABLE(of, of_lm3601x_leds_match); >> + >> +static struct i2c_driver lm3601x_i2c_driver = { >> + .driver = { >> + .name = "lm3601x", >> + .of_match_table = of_lm3601x_leds_match, >> + }, >> + .probe = lm3601x_probe, >> + .remove = lm3601x_remove, >> + .id_table = lm3601x_id, >> +}; >> +module_i2c_driver(lm3601x_i2c_driver); >> + >> +MODULE_DESCRIPTION("Texas Instruments Flash Lighting driver for LM3601X"); >> +MODULE_AUTHOR("Dan Murphy <dmurphy@ti.com>"); >> +MODULE_LICENSE("GPL v2"); >> > -- ------------------ Dan Murphy ^ permalink raw reply [flat|nested] 33+ messages in thread
* Re: [PATCH v5 2/2] leds: lm3601x: Introduce the lm3601x LED driver @ 2018-05-11 11:56 ` Dan Murphy 0 siblings, 0 replies; 33+ messages in thread From: Dan Murphy @ 2018-05-11 11:56 UTC (permalink / raw) To: Jacek Anaszewski, robh+dt, mark.rutland, pavel, afd Cc: devicetree, linux-kernel, linux-leds Jacek Thanks for the review On 05/10/2018 03:48 PM, Jacek Anaszewski wrote: > Hi Dan, > > Thank you for the patch. > > On 05/10/2018 07:47 PM, Dan Murphy wrote: >> Introduce the family of LED devices that can >> drive a torch, strobe or IR LED. >> >> The LED driver can be configured with a strobe >> timer to execute a strobe flash. The IR LED >> brightness is controlled via the torch brightness >> register. >> >> The data sheet for each the LM36010 and LM36011 >> LED drivers can be found here: >> http://www.ti.com/product/LM36010 >> http://www.ti.com/product/LM36011 >> >> Signed-off-by: Dan Murphy <dmurphy@ti.com> >> --- >> >> v5 - Fixed magic numbers, change reg cache type, added of_put_node to release >> the dt node ref, and I did not change the remove function to leave the LED in its >> state on driver removal - https://patchwork.kernel.org/patch/10391741/ >> >> v4 - Fixed Cocci issue using ARRAY_SIZE - https://patchwork.kernel.org/patch/10389259/ >> v3 - removed wildcard dt compatible, fixed copyright, fixed struct doc, removed >> RO registers from default, added regmap volatile for FLAGS_REG, updated regmap cache type, >> fixed unlock and extra semi colon in strobe_set, removed unnecessary out label >> in led register and fixed checking of the ret in brightness_set - https://patchwork.kernel.org/patch/10386243/ >> v2 - Fixed kbuild issue and removed unused cdev_strobe - https://patchwork.kernel.org/patch/10384585/ >> >> >> drivers/leds/Kconfig | 9 + >> drivers/leds/Makefile | 1 + >> drivers/leds/leds-lm3601x.c | 622 ++++++++++++++++++++++++++++++++++++ >> 3 files changed, 632 insertions(+) >> create mode 100644 drivers/leds/leds-lm3601x.c >> >> diff --git a/drivers/leds/Kconfig b/drivers/leds/Kconfig >> index 2c896c0e69e1..50ae536f343f 100644 >> --- a/drivers/leds/Kconfig >> +++ b/drivers/leds/Kconfig >> @@ -145,6 +145,15 @@ config LEDS_LM3692X >> This option enables support for the TI LM3692x family >> of white LED string drivers used for backlighting. >> +config LEDS_LM3601X >> + tristate "LED support for LM3601x Chips" >> + depends on LEDS_CLASS && I2C && OF >> + depends on LEDS_CLASS_FLASH >> + select REGMAP_I2C >> + help >> + This option enables support for the TI LM3601x family >> + of flash, torch and indicator classes. >> + >> config LEDS_LOCOMO >> tristate "LED Support for Locomo device" >> depends on LEDS_CLASS >> diff --git a/drivers/leds/Makefile b/drivers/leds/Makefile >> index 91eca81cae82..b79807fe1b67 100644 >> --- a/drivers/leds/Makefile >> +++ b/drivers/leds/Makefile >> @@ -76,6 +76,7 @@ obj-$(CONFIG_LEDS_MLXREG) += leds-mlxreg.o >> obj-$(CONFIG_LEDS_NIC78BX) += leds-nic78bx.o >> obj-$(CONFIG_LEDS_MT6323) += leds-mt6323.o >> obj-$(CONFIG_LEDS_LM3692X) += leds-lm3692x.o >> +obj-$(CONFIG_LEDS_LM3601X) += leds-lm3601x.o >> # LED SPI Drivers >> obj-$(CONFIG_LEDS_DAC124S085) += leds-dac124s085.o >> diff --git a/drivers/leds/leds-lm3601x.c b/drivers/leds/leds-lm3601x.c >> new file mode 100644 >> index 000000000000..3c00886fbc7a >> --- /dev/null >> +++ b/drivers/leds/leds-lm3601x.c >> @@ -0,0 +1,622 @@ >> +// SPDX-License-Identifier: GPL-2.0 >> +/* >> + * Flash and torch driver for Texas Instruments LM3601X LED >> + * Flash driver chip family >> + * Copyright (C) 2018 Texas Instruments Incorporated - http://www.ti.com/ >> + */ > > Please use uniform "//" comment style here. Is this a LEDs only requirement because I checked a few other files and this is how the license and header is presented. Unless I missed some new change to source code headers. And the documentation does not dictate any change to this. > >> +#include <linux/delay.h> >> +#include <linux/i2c.h> >> +#include <linux/leds.h> >> +#include <linux/led-class-flash.h> >> +#include <linux/module.h> >> +#include <linux/regmap.h> >> +#include <linux/slab.h> >> +#include <uapi/linux/uleds.h> >> + >> +#define LM3601X_LED_TORCH 0x0 >> +#define LM3601X_LED_STROBE 0x1 >> +#define LM3601X_LED_IR 0x2 >> + >> +/* Registers */ >> +#define LM3601X_ENABLE_REG 0x01 >> +#define LM3601X_CFG_REG 0x02 >> +#define LM3601X_LED_FLASH_REG 0x03 >> +#define LM3601X_LED_TORCH_REG 0x04 >> +#define LM3601X_FLAGS_REG 0x05 >> +#define LM3601X_DEV_ID_REG 0x06 >> + >> +#define LM3601X_SW_RESET BIT(7) >> + >> +/* Enable Mode bits */ >> +#define LM3601X_MODE_STANDBY 0x00 >> +#define LM3601X_MODE_IR_DRV BIT(0) >> +#define LM3601X_MODE_TORCH BIT(1) >> +#define LM3601X_MODE_STROBE (BIT(0) | BIT(1)) >> +#define LM3601X_STRB_EN BIT(2) >> +#define LM3601X_STRB_LVL_TRIG ~BIT(3) >> +#define LM3601X_STRB_EDGE_TRIG BIT(3) >> +#define LM3601X_IVFM_EN BIT(4) >> + >> +#define LM36010_BOOST_LIMIT_19 ~BIT(5) >> +#define LM36010_BOOST_LIMIT_28 BIT(5) >> +#define LM36010_BOOST_FREQ_2MHZ ~BIT(6) >> +#define LM36010_BOOST_FREQ_4MHZ BIT(6) >> +#define LM36010_BOOST_MODE_NORM ~BIT(7) >> +#define LM36010_BOOST_MODE_PASS BIT(7) >> + >> +/* Flag Mask */ >> +#define LM3601X_FLASH_TIME_OUT BIT(0) >> +#define LM3601X_UVLO_FAULT BIT(1) >> +#define LM3601X_THERM_SHUTDOWN BIT(2) >> +#define LM3601X_THERM_CURR BIT(3) >> +#define LM36010_CURR_LIMIT BIT(4) >> +#define LM3601X_SHORT_FAULT BIT(5) >> +#define LM3601X_IVFM_TRIP BIT(6) >> +#define LM36010_OVP_FAULT BIT(7) >> + >> +#define LM3601X_MIN_TORCH_I_UA 2400 >> +#define LM3601X_MIN_STROBE_I_MA 11 >> + >> +#define LM3601X_TIMEOUT_MASK 0x1e >> +#define LM3601X_ENABLE_MASK 0x03 >> + >> +enum lm3601x_type { >> + CHIP_LM36010 = 0, >> + CHIP_LM36011, >> +}; >> + >> +/** >> + * struct lm3601x_max_timeouts - >> + * @timeout: timeout value in ms >> + * @regval: the value of the register to write >> + */ >> +struct lm3601x_max_timeouts { >> + int timeout; >> + int reg_val; >> +}; >> + >> +/** >> + * struct lm3601x_led - >> + * @lock: Lock for reading/writing the device >> + * @regmap: Devices register map >> + * @client: Pointer to the I2C client >> + * @cdev_torch: led class device pointer for the torch >> + * @cdev_ir: led class device pointer for infrared >> + * @fled_cdev: flash led class device pointer >> + * @strobe_node: DT device node for the strobe >> + * @torch: LED label for the torch >> + * @strobe: LED label for the strobe >> + * @ir: LED label for the infrared >> + * @last_flag: last known flags register value >> + * @strobe_timeout: the timeout for the strobe >> + * @torch_current_max: maximum current for the torch >> + * @strobe_current_max: maximum current for the strobe >> + * @max_strobe_timeout: maximum timeout for the strobe >> + */ >> +struct lm3601x_led { >> + struct mutex lock; >> + struct regmap *regmap; >> + struct i2c_client *client; >> + >> + struct led_classdev cdev_torch; >> + struct led_classdev cdev_ir; >> + >> + struct led_classdev_flash fled_cdev; >> + >> + struct device_node *strobe_node; >> + struct device_node *torch_node; >> + struct device_node *ir_node; >> + >> + char torch[LED_MAX_NAME_SIZE]; >> + char strobe[LED_MAX_NAME_SIZE]; >> + char ir[LED_MAX_NAME_SIZE]; >> + >> + unsigned int last_flag; >> + unsigned int strobe_timeout; >> + >> + u32 torch_current_max; >> + u32 strobe_current_max; >> + u32 max_strobe_timeout; >> +}; >> + >> +static const struct lm3601x_max_timeouts strobe_timeouts[] = { >> + { 40, 0x00 }, >> + { 80, 0x01 }, >> + { 120, 0x02 }, >> + { 160, 0x03 }, >> + { 200, 0x04 }, >> + { 240, 0x05 }, >> + { 280, 0x06 }, >> + { 320, 0x07 }, >> + { 360, 0x08 }, >> + { 400, 0x09 }, >> + { 600, 0x0a }, >> + { 800, 0x0b }, >> + { 1000, 0x0c }, >> + { 1200, 0x0d }, >> + { 1400, 0x0e }, >> + { 1600, 0x0f }, >> +}; >> + >> +static const struct reg_default lm3601x_regmap_defs[] = { >> + { LM3601X_ENABLE_REG, 0x20 }, >> + { LM3601X_CFG_REG, 0x15 }, >> + { LM3601X_LED_FLASH_REG, 0x00 }, >> + { LM3601X_LED_TORCH_REG, 0x00 }, >> +}; >> + >> +static bool lm3601x_volatile_reg(struct device *dev, unsigned int reg) >> +{ >> + switch (reg) { >> + case LM3601X_FLAGS_REG: >> + return true; >> + default: >> + return false; >> + } >> +} >> + >> +static const struct regmap_config lm3601x_regmap = { >> + .reg_bits = 8, >> + .val_bits = 8, >> + >> + .max_register = LM3601X_DEV_ID_REG, >> + .reg_defaults = lm3601x_regmap_defs, >> + .num_reg_defaults = ARRAY_SIZE(lm3601x_regmap_defs), >> + .cache_type = REGCACHE_RBTREE, >> + .volatile_reg = lm3601x_volatile_reg, >> +}; >> + >> +static struct lm3601x_led *fled_cdev_to_led( >> + struct led_classdev_flash *fled_cdev) >> +{ >> + return container_of(fled_cdev, struct lm3601x_led, fled_cdev); >> +} >> + >> +static int lm3601x_read_faults(struct lm3601x_led *led) >> +{ >> + int flags_val; >> + int ret; >> + >> + ret = regmap_read(led->regmap, LM3601X_FLAGS_REG, &flags_val); >> + if (ret < 0) >> + return -EIO; >> + >> + led->last_flag = flags_val; >> + >> + return led->last_flag; >> +} >> + >> +static int lm3601x_torch_brightness_set(struct led_classdev *cdev, >> + enum led_brightness brightness) >> +{ >> + struct lm3601x_led *led = >> + container_of(cdev, struct lm3601x_led, cdev_torch); >> + int ret; >> + u8 brightness_val; >> + >> + mutex_lock(&led->lock); >> + >> + ret = lm3601x_read_faults(led); >> + if (ret < 0) >> + goto out; >> + >> + if (brightness == LED_OFF) { >> + ret = regmap_update_bits(led->regmap, LM3601X_ENABLE_REG, >> + LM3601X_MODE_TORCH, LED_OFF); >> + goto out; >> + } >> + >> + if (brightness == LED_ON) >> + brightness_val = LED_ON; >> + else >> + brightness_val = (brightness/2); >> + >> + ret = regmap_write(led->regmap, LM3601X_LED_TORCH_REG, brightness_val); >> + if (ret < 0) >> + goto out; >> + >> + ret = regmap_update_bits(led->regmap, LM3601X_ENABLE_REG, >> + LM3601X_MODE_TORCH, >> + LM3601X_MODE_TORCH); >> + >> +out: >> + mutex_unlock(&led->lock); >> + return ret; >> +} >> + >> +static int lm3601x_strobe_set(struct led_classdev_flash *fled_cdev, >> + bool state) >> +{ >> + struct lm3601x_led *led = fled_cdev_to_led(fled_cdev); >> + int ret; >> + int current_timeout; >> + int reg_count; >> + int i; >> + int timeout_reg_val = 0; >> + >> + mutex_lock(&led->lock); >> + >> + ret = regmap_read(led->regmap, LM3601X_CFG_REG, ¤t_timeout); >> + if (ret < 0) >> + goto out; >> + >> + reg_count = ARRAY_SIZE(strobe_timeouts); >> + for (i = 0; i < reg_count; i++) { >> + if (led->strobe_timeout > strobe_timeouts[i].timeout) >> + continue; >> + >> + if (led->strobe_timeout <= strobe_timeouts[i].timeout) { >> + timeout_reg_val = (strobe_timeouts[i].reg_val << 1); >> + break; >> + } >> + >> + ret = -EINVAL; >> + goto out; >> + } >> + >> + if (led->strobe_timeout != current_timeout) >> + ret = regmap_update_bits(led->regmap, LM3601X_CFG_REG, >> + LM3601X_TIMEOUT_MASK, timeout_reg_val); >> + >> + if (state) >> + ret = regmap_update_bits(led->regmap, LM3601X_ENABLE_REG, >> + LM3601X_MODE_STROBE, >> + LM3601X_MODE_STROBE); >> + else >> + ret = regmap_update_bits(led->regmap, LM3601X_ENABLE_REG, >> + LM3601X_MODE_STROBE, LED_OFF); >> + >> + ret = lm3601x_read_faults(led); >> +out: >> + mutex_unlock(&led->lock); >> + return ret; >> +} >> + >> +static int lm3601x_strobe_brightness_set(struct led_classdev *cdev, >> + enum led_brightness brightness) >> +{ >> + struct led_classdev_flash *fled_cdev = lcdev_to_flcdev(cdev); >> + struct lm3601x_led *led = fled_cdev_to_led(fled_cdev); >> + int ret; >> + u8 brightness_val; >> + >> + mutex_lock(&led->lock); >> + ret = lm3601x_read_faults(led); >> + if (ret < 0) >> + goto out; >> + >> + if (brightness == LED_OFF) { >> + ret = regmap_update_bits(led->regmap, LM3601X_ENABLE_REG, >> + LM3601X_MODE_STROBE, LED_OFF); >> + goto out; >> + } >> + >> + if (brightness == LED_ON) >> + brightness_val = LED_ON; >> + else >> + brightness_val = (brightness/2); >> + >> + ret = regmap_write(led->regmap, LM3601X_LED_FLASH_REG, brightness_val); >> + >> +out: >> + mutex_unlock(&led->lock); >> + return ret; >> +} >> + >> +static int lm3601x_strobe_timeout_set(struct led_classdev_flash *fled_cdev, >> + u32 timeout) >> +{ >> + struct lm3601x_led *led = fled_cdev_to_led(fled_cdev); >> + int ret = 0; >> + >> + mutex_lock(&led->lock); >> + >> + if (timeout > led->max_strobe_timeout) >> + ret = -EINVAL; > > LED flash core takes care of clamping timeout setting > according to the defined constraints. OK > >> + else >> + led->strobe_timeout = timeout; >> + >> + mutex_unlock(&led->lock); >> + >> + return ret; >> +} >> + >> +static int lm3601x_strobe_get(struct led_classdev_flash *fled_cdev, >> + bool *state) >> +{ >> + struct lm3601x_led *led = fled_cdev_to_led(fled_cdev); >> + int ret; >> + int strobe_state; >> + >> + mutex_lock(&led->lock); >> + >> + ret = regmap_read(led->regmap, LM3601X_ENABLE_REG, &strobe_state); >> + if (ret < 0) >> + goto out; >> + >> + *state = strobe_state & LM3601X_MODE_STROBE; >> + >> +out: >> + mutex_unlock(&led->lock); >> + return ret; >> +} >> + >> +static int lm3601x_ir_brightness_set(struct led_classdev *cdev, >> + enum led_brightness brightness) >> +{ >> + struct lm3601x_led *led = >> + container_of(cdev, struct lm3601x_led, cdev_ir); >> + int ret; >> + u8 brightness_val; >> + >> + mutex_lock(&led->lock); >> + ret = lm3601x_read_faults(led); >> + if (ret < 0) >> + goto out; >> + >> + if (brightness == LED_OFF) { >> + ret = regmap_update_bits(led->regmap, LM3601X_ENABLE_REG, >> + LM3601X_LED_IR, >> + LED_OFF); >> + goto out; >> + } >> + >> + if (brightness == LED_ON) >> + brightness_val = LED_ON; >> + else >> + brightness_val = (brightness/2); >> + >> + ret = regmap_write(led->regmap, LM3601X_LED_FLASH_REG, brightness_val); >> + if (ret < 0) >> + goto out; >> + >> + ret = regmap_update_bits(led->regmap, LM3601X_ENABLE_REG, >> + LM3601X_MODE_IR_DRV, >> + LM3601X_MODE_IR_DRV); >> + >> +out: >> + mutex_unlock(&led->lock); >> + return ret; >> +} >> + >> +static int lm3601x_strobe_fault_get(struct led_classdev_flash *fled_cdev, >> + u32 *fault) > > Does the device report only strobe related faults? No. It has under/over voltage and thermal. > >> +{ >> + struct lm3601x_led *led = fled_cdev_to_led(fled_cdev); >> + >> + lm3601x_read_faults(led); >> + >> + *fault = led->last_flag; > > You should translate device specific faults to LED_FAULT* flags. > Please compare e.g. drivers/leds/leds-max77693.c or > driver/leds/leds-as3645a.c. Hmm. This code is in the max77693.c. I think the translation should be in the read_faults function in this file and not here. I will add that translation above > >> + >> + return 0; >> +} >> + >> +static const struct led_flash_ops strobe_ops = { >> + .strobe_set = lm3601x_strobe_set, >> + .strobe_get = lm3601x_strobe_get, >> + .timeout_set = lm3601x_strobe_timeout_set, >> + .fault_get = lm3601x_strobe_fault_get, >> +}; >> + >> +static int lm3601x_register_leds(struct lm3601x_led *led) >> +{ >> + struct led_classdev_flash *fled_cdev; >> + struct led_classdev *led_cdev; >> + int err = -ENODEV; >> + >> + if (led->torch_node) { >> + led->cdev_torch.name = led->torch; >> + led->cdev_torch.max_brightness = LED_FULL; >> + led->cdev_torch.brightness_set_blocking = lm3601x_torch_brightness_set; >> + err = devm_led_classdev_register(&led->client->dev, >> + &led->cdev_torch); >> + if (err < 0) >> + return err; >> + } >> + >> + if (led->strobe_node) { >> + fled_cdev = &led->fled_cdev; >> + fled_cdev->ops = &strobe_ops; >> + >> + led_cdev = &fled_cdev->led_cdev; >> + led_cdev->name = led->strobe; >> + led_cdev->max_brightness = LED_FULL; >> + led_cdev->brightness_set_blocking = lm3601x_strobe_brightness_set; >> + led_cdev->flags |= LED_DEV_CAP_FLASH; >> + >> + err = led_classdev_flash_register(&led->client->dev, >> + fled_cdev); >> + if (err < 0) >> + return err; >> + } >> + >> + if (led->ir_node) { >> + led->cdev_ir.name = led->ir; >> + led->cdev_ir.max_brightness = LED_FULL; >> + led->cdev_ir.brightness_set_blocking = >> + lm3601x_ir_brightness_set; >> + >> + err = devm_led_classdev_register(&led->client->dev, >> + &led->cdev_ir); >> + } >> + >> + return err; >> +} >> + >> +static void lm3601x_init_flash_timeout(struct lm3601x_led *led) >> +{ >> + struct led_flash_setting *setting; >> + >> + setting = &led->fled_cdev.timeout; >> + setting->min = strobe_timeouts[0].timeout; >> + setting->max = led->max_strobe_timeout; >> + setting->step = 40; >> + setting->val = led->max_strobe_timeout; >> +} >> + >> +static int lm3601x_parse_node(struct lm3601x_led *led, >> + struct device_node *node) >> +{ >> + struct device_node *child_node; >> + const char *name; >> + int ret = -ENODEV; >> + >> + for_each_available_child_of_node(node, child_node) { >> + u32 id = 0; >> + >> + of_property_read_u32(child_node, "reg", &id); >> + >> + switch (id) { >> + case LM3601X_LED_TORCH: >> + led->torch_node = of_node_get(child_node); >> + break; >> + case LM3601X_LED_STROBE: >> + led->strobe_node = of_node_get(child_node); >> + break; >> + case LM3601X_LED_IR: >> + led->ir_node = of_node_get(child_node); >> + break; >> + default: >> + dev_warn(&led->client->dev, >> + "unknown LED %u encountered, ignoring\n", id); >> + break; >> + } >> + } >> + >> + if (led->torch_node) { >> + ret = of_property_read_string(led->torch_node, "label", &name); >> + if (!ret) >> + snprintf(led->torch, sizeof(led->torch), >> + "%s:%s", led->torch_node->name, name); >> + else >> + snprintf(led->torch, sizeof(led->torch), >> + "%s:torch", led->torch_node->name); >> + ret = of_property_read_u32(led->torch_node, "led-max-microamp", >> + &led->torch_current_max); >> + if (ret < 0) { >> + led->torch_current_max = LM3601X_MIN_TORCH_I_UA; >> + dev_warn(&led->client->dev, >> + "led-max-microamp DT property missing\n"); >> + } >> + >> + of_node_put(led->torch_node); >> + } >> + >> + if (led->strobe_node) { >> + ret = of_property_read_string(led->strobe_node, "label", &name); >> + if (!ret) >> + snprintf(led->strobe, sizeof(led->strobe), >> + "%s:%s", led->strobe_node->name, name); >> + else >> + snprintf(led->strobe, sizeof(led->strobe), >> + "%s::strobe", led->strobe_node->name); >> + >> + ret = of_property_read_u32(led->strobe_node, >> + "flash-max-microamp", >> + &led->strobe_current_max); >> + if (ret < 0) { >> + led->strobe_current_max = LM3601X_MIN_STROBE_I_MA; >> + dev_warn(&led->client->dev, >> + "flash-max-microamp DT property missing\n"); >> + } >> + >> + ret = of_property_read_u32(led->strobe_node, >> + "flash-max-timeout-us", >> + &led->max_strobe_timeout); >> + if (ret < 0) { >> + led->max_strobe_timeout = strobe_timeouts[0].reg_val; >> + dev_warn(&led->client->dev, >> + "flash-max-timeout-us DT property missing\n"); >> + } > > Common LED bindings state that flash-max-microamp and > flash-max-timeout-us properties are mandatory. OK. > >> + >> + lm3601x_init_flash_timeout(led); >> + >> + of_node_put(led->strobe_node); >> + } >> + >> + if (led->ir_node) { >> + ret = of_property_read_string(led->ir_node, "label", &name); >> + if (!ret) >> + snprintf(led->ir, sizeof(led->ir), >> + "%s:%s", led->ir_node->name, name); >> + else >> + snprintf(led->ir, sizeof(led->ir), >> + "%s::infrared", led->ir_node->name); >> + >> + of_node_put(led->ir_node); >> + } >> + >> + return ret; >> +} >> + >> +static int lm3601x_probe(struct i2c_client *client, >> + const struct i2c_device_id *id) >> +{ >> + struct lm3601x_led *led; >> + int err; >> + >> + led = devm_kzalloc(&client->dev, >> + sizeof(struct lm3601x_led), GFP_KERNEL); >> + if (!led) >> + return -ENOMEM; >> + >> + err = lm3601x_parse_node(led, client->dev.of_node); >> + if (err < 0) >> + return -ENODEV; >> + >> + led->client = client; >> + led->regmap = devm_regmap_init_i2c(client, &lm3601x_regmap); >> + if (IS_ERR(led->regmap)) { >> + err = PTR_ERR(led->regmap); >> + dev_err(&client->dev, >> + "Failed to allocate register map: %d\n", err); >> + return err; >> + } >> + >> + mutex_init(&led->lock); >> + i2c_set_clientdata(client, led); >> + err = lm3601x_register_leds(led); >> + >> + return err; >> +} >> + >> +static int lm3601x_remove(struct i2c_client *client) >> +{ >> + struct lm3601x_led *led = i2c_get_clientdata(client); >> + >> + regmap_update_bits(led->regmap, LM3601X_ENABLE_REG, >> + LM3601X_ENABLE_MASK, >> + LM3601X_MODE_STANDBY); >> + >> + return 0; >> +} >> + >> +static const struct i2c_device_id lm3601x_id[] = { >> + { "LM36010", CHIP_LM36010 }, >> + { "LM36011", CHIP_LM36011 }, >> + { }, >> +}; >> +MODULE_DEVICE_TABLE(i2c, lm3601x_id); >> + >> +static const struct of_device_id of_lm3601x_leds_match[] = { >> + { .compatible = "ti,lm36010", }, >> + { .compatible = "ti,lm36011", }, >> + {}, >> +}; >> +MODULE_DEVICE_TABLE(of, of_lm3601x_leds_match); >> + >> +static struct i2c_driver lm3601x_i2c_driver = { >> + .driver = { >> + .name = "lm3601x", >> + .of_match_table = of_lm3601x_leds_match, >> + }, >> + .probe = lm3601x_probe, >> + .remove = lm3601x_remove, >> + .id_table = lm3601x_id, >> +}; >> +module_i2c_driver(lm3601x_i2c_driver); >> + >> +MODULE_DESCRIPTION("Texas Instruments Flash Lighting driver for LM3601X"); >> +MODULE_AUTHOR("Dan Murphy <dmurphy@ti.com>"); >> +MODULE_LICENSE("GPL v2"); >> > -- ------------------ Dan Murphy ^ permalink raw reply [flat|nested] 33+ messages in thread
* Re: [PATCH v5 2/2] leds: lm3601x: Introduce the lm3601x LED driver 2018-05-11 11:56 ` Dan Murphy (?) @ 2018-05-11 20:26 ` Jacek Anaszewski -1 siblings, 0 replies; 33+ messages in thread From: Jacek Anaszewski @ 2018-05-11 20:26 UTC (permalink / raw) To: Dan Murphy, robh+dt, mark.rutland, pavel, afd Cc: devicetree, linux-kernel, linux-leds Hi Dan, On 05/11/2018 01:56 PM, Dan Murphy wrote: > Jacek > > Thanks for the review > > On 05/10/2018 03:48 PM, Jacek Anaszewski wrote: >> Hi Dan, >> >> Thank you for the patch. >> >> On 05/10/2018 07:47 PM, Dan Murphy wrote: >>> Introduce the family of LED devices that can >>> drive a torch, strobe or IR LED. >>> >>> The LED driver can be configured with a strobe >>> timer to execute a strobe flash. The IR LED >>> brightness is controlled via the torch brightness >>> register. >>> >>> The data sheet for each the LM36010 and LM36011 >>> LED drivers can be found here: >>> http://www.ti.com/product/LM36010 >>> http://www.ti.com/product/LM36011 >>> >>> Signed-off-by: Dan Murphy <dmurphy@ti.com> >>> --- >>> >>> v5 - Fixed magic numbers, change reg cache type, added of_put_node to release >>> the dt node ref, and I did not change the remove function to leave the LED in its >>> state on driver removal - https://patchwork.kernel.org/patch/10391741/ >>> >>> v4 - Fixed Cocci issue using ARRAY_SIZE - https://patchwork.kernel.org/patch/10389259/ >>> v3 - removed wildcard dt compatible, fixed copyright, fixed struct doc, removed >>> RO registers from default, added regmap volatile for FLAGS_REG, updated regmap cache type, >>> fixed unlock and extra semi colon in strobe_set, removed unnecessary out label >>> in led register and fixed checking of the ret in brightness_set - https://patchwork.kernel.org/patch/10386243/ >>> v2 - Fixed kbuild issue and removed unused cdev_strobe - https://patchwork.kernel.org/patch/10384585/ >>> >>> >>> drivers/leds/Kconfig | 9 + >>> drivers/leds/Makefile | 1 + >>> drivers/leds/leds-lm3601x.c | 622 ++++++++++++++++++++++++++++++++++++ >>> 3 files changed, 632 insertions(+) >>> create mode 100644 drivers/leds/leds-lm3601x.c >>> >>> diff --git a/drivers/leds/Kconfig b/drivers/leds/Kconfig >>> index 2c896c0e69e1..50ae536f343f 100644 >>> --- a/drivers/leds/Kconfig >>> +++ b/drivers/leds/Kconfig >>> @@ -145,6 +145,15 @@ config LEDS_LM3692X >>> This option enables support for the TI LM3692x family >>> of white LED string drivers used for backlighting. >>> +config LEDS_LM3601X >>> + tristate "LED support for LM3601x Chips" >>> + depends on LEDS_CLASS && I2C && OF >>> + depends on LEDS_CLASS_FLASH >>> + select REGMAP_I2C >>> + help >>> + This option enables support for the TI LM3601x family >>> + of flash, torch and indicator classes. >>> + >>> config LEDS_LOCOMO >>> tristate "LED Support for Locomo device" >>> depends on LEDS_CLASS >>> diff --git a/drivers/leds/Makefile b/drivers/leds/Makefile >>> index 91eca81cae82..b79807fe1b67 100644 >>> --- a/drivers/leds/Makefile >>> +++ b/drivers/leds/Makefile >>> @@ -76,6 +76,7 @@ obj-$(CONFIG_LEDS_MLXREG) += leds-mlxreg.o >>> obj-$(CONFIG_LEDS_NIC78BX) += leds-nic78bx.o >>> obj-$(CONFIG_LEDS_MT6323) += leds-mt6323.o >>> obj-$(CONFIG_LEDS_LM3692X) += leds-lm3692x.o >>> +obj-$(CONFIG_LEDS_LM3601X) += leds-lm3601x.o >>> # LED SPI Drivers >>> obj-$(CONFIG_LEDS_DAC124S085) += leds-dac124s085.o >>> diff --git a/drivers/leds/leds-lm3601x.c b/drivers/leds/leds-lm3601x.c >>> new file mode 100644 >>> index 000000000000..3c00886fbc7a >>> --- /dev/null >>> +++ b/drivers/leds/leds-lm3601x.c >>> @@ -0,0 +1,622 @@ >>> +// SPDX-License-Identifier: GPL-2.0 >>> +/* >>> + * Flash and torch driver for Texas Instruments LM3601X LED >>> + * Flash driver chip family >>> + * Copyright (C) 2018 Texas Instruments Incorporated - http://www.ti.com/ >>> + */ >> >> Please use uniform "//" comment style here. > > Is this a LEDs only requirement because I checked a few other files and this is > how the license and header is presented. > > Unless I missed some new change to source code headers. And the documentation > does not dictate any change to this. I started to require this since mixed comment styles look just awkward to me. Moreover Linus also suggested it in [0]: "And yes, feel free to replace block comments with // while at it." Also grep shows the same approach at least in the sound subsystem :-) >> >>> +#include <linux/delay.h> >>> +#include <linux/i2c.h> >>> +#include <linux/leds.h> >>> +#include <linux/led-class-flash.h> >>> +#include <linux/module.h> >>> +#include <linux/regmap.h> >>> +#include <linux/slab.h> >>> +#include <uapi/linux/uleds.h> >>> + >>> +#define LM3601X_LED_TORCH 0x0 >>> +#define LM3601X_LED_STROBE 0x1 >>> +#define LM3601X_LED_IR 0x2 >>> + >>> +/* Registers */ >>> +#define LM3601X_ENABLE_REG 0x01 >>> +#define LM3601X_CFG_REG 0x02 >>> +#define LM3601X_LED_FLASH_REG 0x03 >>> +#define LM3601X_LED_TORCH_REG 0x04 >>> +#define LM3601X_FLAGS_REG 0x05 >>> +#define LM3601X_DEV_ID_REG 0x06 >>> + >>> +#define LM3601X_SW_RESET BIT(7) >>> + >>> +/* Enable Mode bits */ >>> +#define LM3601X_MODE_STANDBY 0x00 >>> +#define LM3601X_MODE_IR_DRV BIT(0) >>> +#define LM3601X_MODE_TORCH BIT(1) >>> +#define LM3601X_MODE_STROBE (BIT(0) | BIT(1)) >>> +#define LM3601X_STRB_EN BIT(2) >>> +#define LM3601X_STRB_LVL_TRIG ~BIT(3) >>> +#define LM3601X_STRB_EDGE_TRIG BIT(3) >>> +#define LM3601X_IVFM_EN BIT(4) >>> + >>> +#define LM36010_BOOST_LIMIT_19 ~BIT(5) >>> +#define LM36010_BOOST_LIMIT_28 BIT(5) >>> +#define LM36010_BOOST_FREQ_2MHZ ~BIT(6) >>> +#define LM36010_BOOST_FREQ_4MHZ BIT(6) >>> +#define LM36010_BOOST_MODE_NORM ~BIT(7) >>> +#define LM36010_BOOST_MODE_PASS BIT(7) >>> + >>> +/* Flag Mask */ >>> +#define LM3601X_FLASH_TIME_OUT BIT(0) >>> +#define LM3601X_UVLO_FAULT BIT(1) >>> +#define LM3601X_THERM_SHUTDOWN BIT(2) >>> +#define LM3601X_THERM_CURR BIT(3) >>> +#define LM36010_CURR_LIMIT BIT(4) >>> +#define LM3601X_SHORT_FAULT BIT(5) >>> +#define LM3601X_IVFM_TRIP BIT(6) >>> +#define LM36010_OVP_FAULT BIT(7) >>> + >>> +#define LM3601X_MIN_TORCH_I_UA 2400 >>> +#define LM3601X_MIN_STROBE_I_MA 11 >>> + >>> +#define LM3601X_TIMEOUT_MASK 0x1e >>> +#define LM3601X_ENABLE_MASK 0x03 >>> + >>> +enum lm3601x_type { >>> + CHIP_LM36010 = 0, >>> + CHIP_LM36011, >>> +}; >>> + >>> +/** >>> + * struct lm3601x_max_timeouts - >>> + * @timeout: timeout value in ms >>> + * @regval: the value of the register to write >>> + */ >>> +struct lm3601x_max_timeouts { >>> + int timeout; >>> + int reg_val; >>> +}; >>> + >>> +/** >>> + * struct lm3601x_led - >>> + * @lock: Lock for reading/writing the device >>> + * @regmap: Devices register map >>> + * @client: Pointer to the I2C client >>> + * @cdev_torch: led class device pointer for the torch >>> + * @cdev_ir: led class device pointer for infrared >>> + * @fled_cdev: flash led class device pointer >>> + * @strobe_node: DT device node for the strobe >>> + * @torch: LED label for the torch >>> + * @strobe: LED label for the strobe >>> + * @ir: LED label for the infrared >>> + * @last_flag: last known flags register value >>> + * @strobe_timeout: the timeout for the strobe >>> + * @torch_current_max: maximum current for the torch >>> + * @strobe_current_max: maximum current for the strobe >>> + * @max_strobe_timeout: maximum timeout for the strobe >>> + */ >>> +struct lm3601x_led { >>> + struct mutex lock; >>> + struct regmap *regmap; >>> + struct i2c_client *client; >>> + >>> + struct led_classdev cdev_torch; >>> + struct led_classdev cdev_ir; >>> + >>> + struct led_classdev_flash fled_cdev; >>> + >>> + struct device_node *strobe_node; >>> + struct device_node *torch_node; >>> + struct device_node *ir_node; >>> + >>> + char torch[LED_MAX_NAME_SIZE]; >>> + char strobe[LED_MAX_NAME_SIZE]; >>> + char ir[LED_MAX_NAME_SIZE]; >>> + >>> + unsigned int last_flag; >>> + unsigned int strobe_timeout; >>> + >>> + u32 torch_current_max; >>> + u32 strobe_current_max; >>> + u32 max_strobe_timeout; >>> +}; >>> + >>> +static const struct lm3601x_max_timeouts strobe_timeouts[] = { >>> + { 40, 0x00 }, >>> + { 80, 0x01 }, >>> + { 120, 0x02 }, >>> + { 160, 0x03 }, >>> + { 200, 0x04 }, >>> + { 240, 0x05 }, >>> + { 280, 0x06 }, >>> + { 320, 0x07 }, >>> + { 360, 0x08 }, >>> + { 400, 0x09 }, >>> + { 600, 0x0a }, >>> + { 800, 0x0b }, >>> + { 1000, 0x0c }, >>> + { 1200, 0x0d }, >>> + { 1400, 0x0e }, >>> + { 1600, 0x0f }, >>> +}; >>> + >>> +static const struct reg_default lm3601x_regmap_defs[] = { >>> + { LM3601X_ENABLE_REG, 0x20 }, >>> + { LM3601X_CFG_REG, 0x15 }, >>> + { LM3601X_LED_FLASH_REG, 0x00 }, >>> + { LM3601X_LED_TORCH_REG, 0x00 }, >>> +}; >>> + >>> +static bool lm3601x_volatile_reg(struct device *dev, unsigned int reg) >>> +{ >>> + switch (reg) { >>> + case LM3601X_FLAGS_REG: >>> + return true; >>> + default: >>> + return false; >>> + } >>> +} >>> + >>> +static const struct regmap_config lm3601x_regmap = { >>> + .reg_bits = 8, >>> + .val_bits = 8, >>> + >>> + .max_register = LM3601X_DEV_ID_REG, >>> + .reg_defaults = lm3601x_regmap_defs, >>> + .num_reg_defaults = ARRAY_SIZE(lm3601x_regmap_defs), >>> + .cache_type = REGCACHE_RBTREE, >>> + .volatile_reg = lm3601x_volatile_reg, >>> +}; >>> + >>> +static struct lm3601x_led *fled_cdev_to_led( >>> + struct led_classdev_flash *fled_cdev) >>> +{ >>> + return container_of(fled_cdev, struct lm3601x_led, fled_cdev); >>> +} >>> + >>> +static int lm3601x_read_faults(struct lm3601x_led *led) >>> +{ >>> + int flags_val; >>> + int ret; >>> + >>> + ret = regmap_read(led->regmap, LM3601X_FLAGS_REG, &flags_val); >>> + if (ret < 0) >>> + return -EIO; >>> + >>> + led->last_flag = flags_val; >>> + >>> + return led->last_flag; >>> +} >>> + >>> +static int lm3601x_torch_brightness_set(struct led_classdev *cdev, >>> + enum led_brightness brightness) >>> +{ >>> + struct lm3601x_led *led = >>> + container_of(cdev, struct lm3601x_led, cdev_torch); >>> + int ret; >>> + u8 brightness_val; >>> + >>> + mutex_lock(&led->lock); >>> + >>> + ret = lm3601x_read_faults(led); >>> + if (ret < 0) >>> + goto out; >>> + >>> + if (brightness == LED_OFF) { >>> + ret = regmap_update_bits(led->regmap, LM3601X_ENABLE_REG, >>> + LM3601X_MODE_TORCH, LED_OFF); >>> + goto out; >>> + } >>> + >>> + if (brightness == LED_ON) >>> + brightness_val = LED_ON; >>> + else >>> + brightness_val = (brightness/2); >>> + >>> + ret = regmap_write(led->regmap, LM3601X_LED_TORCH_REG, brightness_val); >>> + if (ret < 0) >>> + goto out; >>> + >>> + ret = regmap_update_bits(led->regmap, LM3601X_ENABLE_REG, >>> + LM3601X_MODE_TORCH, >>> + LM3601X_MODE_TORCH); >>> + >>> +out: >>> + mutex_unlock(&led->lock); >>> + return ret; >>> +} >>> + >>> +static int lm3601x_strobe_set(struct led_classdev_flash *fled_cdev, >>> + bool state) >>> +{ >>> + struct lm3601x_led *led = fled_cdev_to_led(fled_cdev); >>> + int ret; >>> + int current_timeout; >>> + int reg_count; >>> + int i; >>> + int timeout_reg_val = 0; >>> + >>> + mutex_lock(&led->lock); >>> + >>> + ret = regmap_read(led->regmap, LM3601X_CFG_REG, ¤t_timeout); >>> + if (ret < 0) >>> + goto out; >>> + >>> + reg_count = ARRAY_SIZE(strobe_timeouts); >>> + for (i = 0; i < reg_count; i++) { >>> + if (led->strobe_timeout > strobe_timeouts[i].timeout) >>> + continue; >>> + >>> + if (led->strobe_timeout <= strobe_timeouts[i].timeout) { >>> + timeout_reg_val = (strobe_timeouts[i].reg_val << 1); >>> + break; >>> + } >>> + >>> + ret = -EINVAL; >>> + goto out; >>> + } >>> + >>> + if (led->strobe_timeout != current_timeout) >>> + ret = regmap_update_bits(led->regmap, LM3601X_CFG_REG, >>> + LM3601X_TIMEOUT_MASK, timeout_reg_val); >>> + >>> + if (state) >>> + ret = regmap_update_bits(led->regmap, LM3601X_ENABLE_REG, >>> + LM3601X_MODE_STROBE, >>> + LM3601X_MODE_STROBE); >>> + else >>> + ret = regmap_update_bits(led->regmap, LM3601X_ENABLE_REG, >>> + LM3601X_MODE_STROBE, LED_OFF); >>> + >>> + ret = lm3601x_read_faults(led); >>> +out: >>> + mutex_unlock(&led->lock); >>> + return ret; >>> +} >>> + >>> +static int lm3601x_strobe_brightness_set(struct led_classdev *cdev, >>> + enum led_brightness brightness) >>> +{ >>> + struct led_classdev_flash *fled_cdev = lcdev_to_flcdev(cdev); >>> + struct lm3601x_led *led = fled_cdev_to_led(fled_cdev); >>> + int ret; >>> + u8 brightness_val; >>> + >>> + mutex_lock(&led->lock); >>> + ret = lm3601x_read_faults(led); >>> + if (ret < 0) >>> + goto out; >>> + >>> + if (brightness == LED_OFF) { >>> + ret = regmap_update_bits(led->regmap, LM3601X_ENABLE_REG, >>> + LM3601X_MODE_STROBE, LED_OFF); >>> + goto out; >>> + } >>> + >>> + if (brightness == LED_ON) >>> + brightness_val = LED_ON; >>> + else >>> + brightness_val = (brightness/2); >>> + >>> + ret = regmap_write(led->regmap, LM3601X_LED_FLASH_REG, brightness_val); >>> + >>> +out: >>> + mutex_unlock(&led->lock); >>> + return ret; >>> +} >>> + >>> +static int lm3601x_strobe_timeout_set(struct led_classdev_flash *fled_cdev, >>> + u32 timeout) >>> +{ >>> + struct lm3601x_led *led = fled_cdev_to_led(fled_cdev); >>> + int ret = 0; >>> + >>> + mutex_lock(&led->lock); >>> + >>> + if (timeout > led->max_strobe_timeout) >>> + ret = -EINVAL; >> >> LED flash core takes care of clamping timeout setting >> according to the defined constraints. > > OK > >> >>> + else >>> + led->strobe_timeout = timeout; >>> + >>> + mutex_unlock(&led->lock); >>> + >>> + return ret; >>> +} >>> + >>> +static int lm3601x_strobe_get(struct led_classdev_flash *fled_cdev, >>> + bool *state) >>> +{ >>> + struct lm3601x_led *led = fled_cdev_to_led(fled_cdev); >>> + int ret; >>> + int strobe_state; >>> + >>> + mutex_lock(&led->lock); >>> + >>> + ret = regmap_read(led->regmap, LM3601X_ENABLE_REG, &strobe_state); >>> + if (ret < 0) >>> + goto out; >>> + >>> + *state = strobe_state & LM3601X_MODE_STROBE; >>> + >>> +out: >>> + mutex_unlock(&led->lock); >>> + return ret; >>> +} >>> + >>> +static int lm3601x_ir_brightness_set(struct led_classdev *cdev, >>> + enum led_brightness brightness) >>> +{ >>> + struct lm3601x_led *led = >>> + container_of(cdev, struct lm3601x_led, cdev_ir); >>> + int ret; >>> + u8 brightness_val; >>> + >>> + mutex_lock(&led->lock); >>> + ret = lm3601x_read_faults(led); >>> + if (ret < 0) >>> + goto out; >>> + >>> + if (brightness == LED_OFF) { >>> + ret = regmap_update_bits(led->regmap, LM3601X_ENABLE_REG, >>> + LM3601X_LED_IR, >>> + LED_OFF); >>> + goto out; >>> + } >>> + >>> + if (brightness == LED_ON) >>> + brightness_val = LED_ON; >>> + else >>> + brightness_val = (brightness/2); >>> + >>> + ret = regmap_write(led->regmap, LM3601X_LED_FLASH_REG, brightness_val); >>> + if (ret < 0) >>> + goto out; >>> + >>> + ret = regmap_update_bits(led->regmap, LM3601X_ENABLE_REG, >>> + LM3601X_MODE_IR_DRV, >>> + LM3601X_MODE_IR_DRV); >>> + >>> +out: >>> + mutex_unlock(&led->lock); >>> + return ret; >>> +} >>> + >>> +static int lm3601x_strobe_fault_get(struct led_classdev_flash *fled_cdev, >>> + u32 *fault) >> >> Does the device report only strobe related faults? > > No. It has under/over voltage and thermal. I'd remove "strobe" from the function name then. >>> +{ >>> + struct lm3601x_led *led = fled_cdev_to_led(fled_cdev); >>> + >>> + lm3601x_read_faults(led); >>> + >>> + *fault = led->last_flag; >> >> You should translate device specific faults to LED_FAULT* flags. >> Please compare e.g. drivers/leds/leds-max77693.c or >> driver/leds/leds-as3645a.c. > > Hmm. This code is in the max77693.c. I think the translation should be in the > read_faults function in this file and not here. Correct. > I will add that translation above > >> >>> + >>> + return 0; >>> +} >>> + >>> +static const struct led_flash_ops strobe_ops = { >>> + .strobe_set = lm3601x_strobe_set, >>> + .strobe_get = lm3601x_strobe_get, >>> + .timeout_set = lm3601x_strobe_timeout_set, >>> + .fault_get = lm3601x_strobe_fault_get, >>> +}; >>> + >>> +static int lm3601x_register_leds(struct lm3601x_led *led) >>> +{ >>> + struct led_classdev_flash *fled_cdev; >>> + struct led_classdev *led_cdev; >>> + int err = -ENODEV; >>> + >>> + if (led->torch_node) { >>> + led->cdev_torch.name = led->torch; >>> + led->cdev_torch.max_brightness = LED_FULL; >>> + led->cdev_torch.brightness_set_blocking = lm3601x_torch_brightness_set; >>> + err = devm_led_classdev_register(&led->client->dev, >>> + &led->cdev_torch); >>> + if (err < 0) >>> + return err; >>> + } >>> + >>> + if (led->strobe_node) { >>> + fled_cdev = &led->fled_cdev; >>> + fled_cdev->ops = &strobe_ops; >>> + >>> + led_cdev = &fled_cdev->led_cdev; >>> + led_cdev->name = led->strobe; >>> + led_cdev->max_brightness = LED_FULL; >>> + led_cdev->brightness_set_blocking = lm3601x_strobe_brightness_set; >>> + led_cdev->flags |= LED_DEV_CAP_FLASH; >>> + >>> + err = led_classdev_flash_register(&led->client->dev, >>> + fled_cdev); >>> + if (err < 0) >>> + return err; >>> + } >>> + >>> + if (led->ir_node) { >>> + led->cdev_ir.name = led->ir; >>> + led->cdev_ir.max_brightness = LED_FULL; >>> + led->cdev_ir.brightness_set_blocking = >>> + lm3601x_ir_brightness_set; >>> + >>> + err = devm_led_classdev_register(&led->client->dev, >>> + &led->cdev_ir); >>> + } >>> + >>> + return err; >>> +} >>> + >>> +static void lm3601x_init_flash_timeout(struct lm3601x_led *led) >>> +{ >>> + struct led_flash_setting *setting; >>> + >>> + setting = &led->fled_cdev.timeout; >>> + setting->min = strobe_timeouts[0].timeout; >>> + setting->max = led->max_strobe_timeout; >>> + setting->step = 40; >>> + setting->val = led->max_strobe_timeout; >>> +} >>> + >>> +static int lm3601x_parse_node(struct lm3601x_led *led, >>> + struct device_node *node) >>> +{ >>> + struct device_node *child_node; >>> + const char *name; >>> + int ret = -ENODEV; >>> + >>> + for_each_available_child_of_node(node, child_node) { >>> + u32 id = 0; >>> + >>> + of_property_read_u32(child_node, "reg", &id); >>> + >>> + switch (id) { >>> + case LM3601X_LED_TORCH: >>> + led->torch_node = of_node_get(child_node); >>> + break; >>> + case LM3601X_LED_STROBE: >>> + led->strobe_node = of_node_get(child_node); >>> + break; >>> + case LM3601X_LED_IR: >>> + led->ir_node = of_node_get(child_node); >>> + break; >>> + default: >>> + dev_warn(&led->client->dev, >>> + "unknown LED %u encountered, ignoring\n", id); >>> + break; >>> + } >>> + } >>> + >>> + if (led->torch_node) { >>> + ret = of_property_read_string(led->torch_node, "label", &name); >>> + if (!ret) >>> + snprintf(led->torch, sizeof(led->torch), >>> + "%s:%s", led->torch_node->name, name); >>> + else >>> + snprintf(led->torch, sizeof(led->torch), >>> + "%s:torch", led->torch_node->name); >>> + ret = of_property_read_u32(led->torch_node, "led-max-microamp", >>> + &led->torch_current_max); >>> + if (ret < 0) { >>> + led->torch_current_max = LM3601X_MIN_TORCH_I_UA; >>> + dev_warn(&led->client->dev, >>> + "led-max-microamp DT property missing\n"); >>> + } >>> + >>> + of_node_put(led->torch_node); >>> + } >>> + >>> + if (led->strobe_node) { >>> + ret = of_property_read_string(led->strobe_node, "label", &name); >>> + if (!ret) >>> + snprintf(led->strobe, sizeof(led->strobe), >>> + "%s:%s", led->strobe_node->name, name); >>> + else >>> + snprintf(led->strobe, sizeof(led->strobe), >>> + "%s::strobe", led->strobe_node->name); >>> + >>> + ret = of_property_read_u32(led->strobe_node, >>> + "flash-max-microamp", >>> + &led->strobe_current_max); >>> + if (ret < 0) { >>> + led->strobe_current_max = LM3601X_MIN_STROBE_I_MA; >>> + dev_warn(&led->client->dev, >>> + "flash-max-microamp DT property missing\n"); >>> + } >>> + >>> + ret = of_property_read_u32(led->strobe_node, >>> + "flash-max-timeout-us", >>> + &led->max_strobe_timeout); >>> + if (ret < 0) { >>> + led->max_strobe_timeout = strobe_timeouts[0].reg_val; >>> + dev_warn(&led->client->dev, >>> + "flash-max-timeout-us DT property missing\n"); >>> + } >> >> Common LED bindings state that flash-max-microamp and >> flash-max-timeout-us properties are mandatory. > > OK. > >> >>> + >>> + lm3601x_init_flash_timeout(led); >>> + >>> + of_node_put(led->strobe_node); >>> + } >>> + >>> + if (led->ir_node) { >>> + ret = of_property_read_string(led->ir_node, "label", &name); >>> + if (!ret) >>> + snprintf(led->ir, sizeof(led->ir), >>> + "%s:%s", led->ir_node->name, name); >>> + else >>> + snprintf(led->ir, sizeof(led->ir), >>> + "%s::infrared", led->ir_node->name); >>> + >>> + of_node_put(led->ir_node); >>> + } >>> + >>> + return ret; >>> +} >>> + >>> +static int lm3601x_probe(struct i2c_client *client, >>> + const struct i2c_device_id *id) >>> +{ >>> + struct lm3601x_led *led; >>> + int err; >>> + >>> + led = devm_kzalloc(&client->dev, >>> + sizeof(struct lm3601x_led), GFP_KERNEL); >>> + if (!led) >>> + return -ENOMEM; >>> + >>> + err = lm3601x_parse_node(led, client->dev.of_node); >>> + if (err < 0) >>> + return -ENODEV; >>> + >>> + led->client = client; >>> + led->regmap = devm_regmap_init_i2c(client, &lm3601x_regmap); >>> + if (IS_ERR(led->regmap)) { >>> + err = PTR_ERR(led->regmap); >>> + dev_err(&client->dev, >>> + "Failed to allocate register map: %d\n", err); >>> + return err; >>> + } >>> + >>> + mutex_init(&led->lock); >>> + i2c_set_clientdata(client, led); >>> + err = lm3601x_register_leds(led); >>> + >>> + return err; >>> +} >>> + >>> +static int lm3601x_remove(struct i2c_client *client) >>> +{ >>> + struct lm3601x_led *led = i2c_get_clientdata(client); >>> + >>> + regmap_update_bits(led->regmap, LM3601X_ENABLE_REG, >>> + LM3601X_ENABLE_MASK, >>> + LM3601X_MODE_STANDBY); >>> + >>> + return 0; >>> +} >>> + >>> +static const struct i2c_device_id lm3601x_id[] = { >>> + { "LM36010", CHIP_LM36010 }, >>> + { "LM36011", CHIP_LM36011 }, >>> + { }, >>> +}; >>> +MODULE_DEVICE_TABLE(i2c, lm3601x_id); >>> + >>> +static const struct of_device_id of_lm3601x_leds_match[] = { >>> + { .compatible = "ti,lm36010", }, >>> + { .compatible = "ti,lm36011", }, >>> + {}, >>> +}; >>> +MODULE_DEVICE_TABLE(of, of_lm3601x_leds_match); >>> + >>> +static struct i2c_driver lm3601x_i2c_driver = { >>> + .driver = { >>> + .name = "lm3601x", >>> + .of_match_table = of_lm3601x_leds_match, >>> + }, >>> + .probe = lm3601x_probe, >>> + .remove = lm3601x_remove, >>> + .id_table = lm3601x_id, >>> +}; >>> +module_i2c_driver(lm3601x_i2c_driver); >>> + >>> +MODULE_DESCRIPTION("Texas Instruments Flash Lighting driver for LM3601X"); >>> +MODULE_AUTHOR("Dan Murphy <dmurphy@ti.com>"); >>> +MODULE_LICENSE("GPL v2"); >>> >> > > [0] https://lkml.org/lkml/2017/11/2/715 -- Best regards, Jacek Anaszewski ^ permalink raw reply [flat|nested] 33+ messages in thread
* Re: [PATCH v5 2/2] leds: lm3601x: Introduce the lm3601x LED driver 2018-05-11 11:56 ` Dan Murphy @ 2018-05-14 19:40 ` Dan Murphy -1 siblings, 0 replies; 33+ messages in thread From: Dan Murphy @ 2018-05-14 19:40 UTC (permalink / raw) To: Jacek Anaszewski, robh+dt, mark.rutland, pavel, afd Cc: devicetree, linux-kernel, linux-leds Jacek On 05/11/2018 06:56 AM, Dan Murphy wrote: <snip> >>> + } >>> + >>> + if (led->strobe_node) { >>> + ret = of_property_read_string(led->strobe_node, "label", &name); >>> + if (!ret) >>> + snprintf(led->strobe, sizeof(led->strobe), >>> + "%s:%s", led->strobe_node->name, name); >>> + else >>> + snprintf(led->strobe, sizeof(led->strobe), >>> + "%s::strobe", led->strobe_node->name); >>> + >>> + ret = of_property_read_u32(led->strobe_node, >>> + "flash-max-microamp", >>> + &led->strobe_current_max); >>> + if (ret < 0) { >>> + led->strobe_current_max = LM3601X_MIN_STROBE_I_MA; >>> + dev_warn(&led->client->dev, >>> + "flash-max-microamp DT property missing\n"); >>> + } >>> + >>> + ret = of_property_read_u32(led->strobe_node, >>> + "flash-max-timeout-us", >>> + &led->max_strobe_timeout); >>> + if (ret < 0) { >>> + led->max_strobe_timeout = strobe_timeouts[0].reg_val; >>> + dev_warn(&led->client->dev, >>> + "flash-max-timeout-us DT property missing\n"); >>> + } >> >> Common LED bindings state that flash-max-microamp and >> flash-max-timeout-us properties are mandatory. > > OK. OK I looked at the max776973 driver and well if the flash-max-microamp and flash-max-timeout-us nodes are missing it sets a default value for each if the node is not present. So should we remove this code from the Max77693 driver too and fail probe as being asked in this driver? Dan > >> >>> + >>> + lm3601x_init_flash_timeout(led); <snip> >>> >> > > -- ------------------ Dan Murphy ^ permalink raw reply [flat|nested] 33+ messages in thread
* Re: [PATCH v5 2/2] leds: lm3601x: Introduce the lm3601x LED driver @ 2018-05-14 19:40 ` Dan Murphy 0 siblings, 0 replies; 33+ messages in thread From: Dan Murphy @ 2018-05-14 19:40 UTC (permalink / raw) To: Jacek Anaszewski, robh+dt, mark.rutland, pavel, afd Cc: devicetree, linux-kernel, linux-leds Jacek On 05/11/2018 06:56 AM, Dan Murphy wrote: <snip> >>> + } >>> + >>> + if (led->strobe_node) { >>> + ret = of_property_read_string(led->strobe_node, "label", &name); >>> + if (!ret) >>> + snprintf(led->strobe, sizeof(led->strobe), >>> + "%s:%s", led->strobe_node->name, name); >>> + else >>> + snprintf(led->strobe, sizeof(led->strobe), >>> + "%s::strobe", led->strobe_node->name); >>> + >>> + ret = of_property_read_u32(led->strobe_node, >>> + "flash-max-microamp", >>> + &led->strobe_current_max); >>> + if (ret < 0) { >>> + led->strobe_current_max = LM3601X_MIN_STROBE_I_MA; >>> + dev_warn(&led->client->dev, >>> + "flash-max-microamp DT property missing\n"); >>> + } >>> + >>> + ret = of_property_read_u32(led->strobe_node, >>> + "flash-max-timeout-us", >>> + &led->max_strobe_timeout); >>> + if (ret < 0) { >>> + led->max_strobe_timeout = strobe_timeouts[0].reg_val; >>> + dev_warn(&led->client->dev, >>> + "flash-max-timeout-us DT property missing\n"); >>> + } >> >> Common LED bindings state that flash-max-microamp and >> flash-max-timeout-us properties are mandatory. > > OK. OK I looked at the max776973 driver and well if the flash-max-microamp and flash-max-timeout-us nodes are missing it sets a default value for each if the node is not present. So should we remove this code from the Max77693 driver too and fail probe as being asked in this driver? Dan > >> >>> + >>> + lm3601x_init_flash_timeout(led); <snip> >>> >> > > -- ------------------ Dan Murphy ^ permalink raw reply [flat|nested] 33+ messages in thread
* Re: [PATCH v5 2/2] leds: lm3601x: Introduce the lm3601x LED driver 2018-05-14 19:40 ` Dan Murphy (?) @ 2018-05-14 19:49 ` Andy Shevchenko -1 siblings, 0 replies; 33+ messages in thread From: Andy Shevchenko @ 2018-05-14 19:49 UTC (permalink / raw) To: Dan Murphy Cc: Jacek Anaszewski, Rob Herring, Mark Rutland, Pavel Machek, Andrew F. Davis, devicetree, Linux Kernel Mailing List, Linux LED Subsystem On Mon, May 14, 2018 at 10:40 PM, Dan Murphy <dmurphy@ti.com> wrote: > On 05/11/2018 06:56 AM, Dan Murphy wrote: >>>> + ret = of_property_read_string(led->strobe_node, "label", &name); >>>> + ret = of_property_read_u32(led->strobe_node, >>>> + ret = of_property_read_u32(led->strobe_node, >>> Common LED bindings state that flash-max-microamp and >>> flash-max-timeout-us properties are mandatory. >> >> OK. > > OK I looked at the max776973 driver and well if the flash-max-microamp and > flash-max-timeout-us nodes are missing it sets a default value for each if the > node is not present. > > So should we remove this code from the Max77693 driver too and fail probe as being asked > in this driver? I would also add that using device_property_*() API is much better then using OF specific one. It will help IoT / DIY entusiasts use them on non-DT platforms. -- With Best Regards, Andy Shevchenko ^ permalink raw reply [flat|nested] 33+ messages in thread
* Re: [PATCH v5 2/2] leds: lm3601x: Introduce the lm3601x LED driver 2018-05-14 19:40 ` Dan Murphy (?) (?) @ 2018-05-14 20:05 ` Pavel Machek 2018-05-14 20:13 ` Dan Murphy -1 siblings, 1 reply; 33+ messages in thread From: Pavel Machek @ 2018-05-14 20:05 UTC (permalink / raw) To: Dan Murphy Cc: Jacek Anaszewski, robh+dt, mark.rutland, afd, devicetree, linux-kernel, linux-leds [-- Attachment #1: Type: text/plain, Size: 775 bytes --] Hi! > > OK. > > OK I looked at the max776973 driver and well if the flash-max-microamp and > flash-max-timeout-us nodes are missing it sets a default value for each if the > node is not present. > > So should we remove this code from the Max77693 driver too and fail probe as being asked > in this driver? Well, modifying driver without access to the hardware is tricky :-(. If it does something stupid (like using other than minimum values for the flash-max-microamp/flash-max-timeout-us), it needs to be fixed. And maybe comment "don't copy this, its wrong" would be in order... Best regards, Pavel -- (english) http://www.livejournal.com/~pavelmachek (cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html [-- Attachment #2: Digital signature --] [-- Type: application/pgp-signature, Size: 181 bytes --] ^ permalink raw reply [flat|nested] 33+ messages in thread
* Re: [PATCH v5 2/2] leds: lm3601x: Introduce the lm3601x LED driver 2018-05-14 20:05 ` Pavel Machek @ 2018-05-14 20:13 ` Dan Murphy 0 siblings, 0 replies; 33+ messages in thread From: Dan Murphy @ 2018-05-14 20:13 UTC (permalink / raw) To: Pavel Machek Cc: Jacek Anaszewski, robh+dt, mark.rutland, afd, devicetree, linux-kernel, linux-leds Pavel On 05/14/2018 03:05 PM, Pavel Machek wrote: > Hi! > >>> OK. >> >> OK I looked at the max776973 driver and well if the flash-max-microamp and >> flash-max-timeout-us nodes are missing it sets a default value for each if the >> node is not present. >> >> So should we remove this code from the Max77693 driver too and fail probe as being asked >> in this driver? > > Well, modifying driver without access to the hardware is tricky > :-(. If it does something stupid (like using other than minimum values > for the flash-max-microamp/flash-max-timeout-us), it needs to be > fixed. Well we should be able to test the probe/parse dt node and reject the probe if the node is not present. That can be done without HW the setup is done pretty early in the probe without even attempting to communicate with the hardware. Dan > > And maybe comment "don't copy this, its wrong" would be in order... > > Best regards, > Pavel > -- ------------------ Dan Murphy ^ permalink raw reply [flat|nested] 33+ messages in thread
* Re: [PATCH v5 2/2] leds: lm3601x: Introduce the lm3601x LED driver @ 2018-05-14 20:13 ` Dan Murphy 0 siblings, 0 replies; 33+ messages in thread From: Dan Murphy @ 2018-05-14 20:13 UTC (permalink / raw) To: Pavel Machek Cc: Jacek Anaszewski, robh+dt, mark.rutland, afd, devicetree, linux-kernel, linux-leds Pavel On 05/14/2018 03:05 PM, Pavel Machek wrote: > Hi! > >>> OK. >> >> OK I looked at the max776973 driver and well if the flash-max-microamp and >> flash-max-timeout-us nodes are missing it sets a default value for each if the >> node is not present. >> >> So should we remove this code from the Max77693 driver too and fail probe as being asked >> in this driver? > > Well, modifying driver without access to the hardware is tricky > :-(. If it does something stupid (like using other than minimum values > for the flash-max-microamp/flash-max-timeout-us), it needs to be > fixed. Well we should be able to test the probe/parse dt node and reject the probe if the node is not present. That can be done without HW the setup is done pretty early in the probe without even attempting to communicate with the hardware. Dan > > And maybe comment "don't copy this, its wrong" would be in order... > > Best regards, > Pavel > -- ------------------ Dan Murphy ^ permalink raw reply [flat|nested] 33+ messages in thread
* Re: [PATCH v5 2/2] leds: lm3601x: Introduce the lm3601x LED driver 2018-05-14 20:13 ` Dan Murphy (?) @ 2018-05-14 20:16 ` Pavel Machek 2018-05-14 20:31 ` Dan Murphy -1 siblings, 1 reply; 33+ messages in thread From: Pavel Machek @ 2018-05-14 20:16 UTC (permalink / raw) To: Dan Murphy Cc: Jacek Anaszewski, robh+dt, mark.rutland, afd, devicetree, linux-kernel, linux-leds [-- Attachment #1: Type: text/plain, Size: 1244 bytes --] On Mon 2018-05-14 15:13:34, Dan Murphy wrote: > Pavel > > On 05/14/2018 03:05 PM, Pavel Machek wrote: > > Hi! > > > >>> OK. > >> > >> OK I looked at the max776973 driver and well if the flash-max-microamp and > >> flash-max-timeout-us nodes are missing it sets a default value for each if the > >> node is not present. > >> > >> So should we remove this code from the Max77693 driver too and fail probe as being asked > >> in this driver? > > > > Well, modifying driver without access to the hardware is tricky > > :-(. If it does something stupid (like using other than minimum values > > for the flash-max-microamp/flash-max-timeout-us), it needs to be > > fixed. > > Well we should be able to test the probe/parse dt node and reject the probe if the node is not > present. That can be done without HW the setup is done pretty early in the probe without even attempting to communicate > with the hardware. > Well, yes, you can do that. But if someone is actively using board with DT w/o flash-max-microamp, you'll make them unhappy. Best regards, Pavel -- (english) http://www.livejournal.com/~pavelmachek (cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html [-- Attachment #2: Digital signature --] [-- Type: application/pgp-signature, Size: 181 bytes --] ^ permalink raw reply [flat|nested] 33+ messages in thread
* Re: [PATCH v5 2/2] leds: lm3601x: Introduce the lm3601x LED driver 2018-05-14 20:16 ` Pavel Machek @ 2018-05-14 20:31 ` Dan Murphy 0 siblings, 0 replies; 33+ messages in thread From: Dan Murphy @ 2018-05-14 20:31 UTC (permalink / raw) To: Pavel Machek Cc: Jacek Anaszewski, robh+dt, mark.rutland, afd, devicetree, linux-kernel, linux-leds On 05/14/2018 03:16 PM, Pavel Machek wrote: > On Mon 2018-05-14 15:13:34, Dan Murphy wrote: >> Pavel >> >> On 05/14/2018 03:05 PM, Pavel Machek wrote: >>> Hi! >>> >>>>> OK. >>>> >>>> OK I looked at the max776973 driver and well if the flash-max-microamp and >>>> flash-max-timeout-us nodes are missing it sets a default value for each if the >>>> node is not present. >>>> >>>> So should we remove this code from the Max77693 driver too and fail probe as being asked >>>> in this driver? >>> >>> Well, modifying driver without access to the hardware is tricky >>> :-(. If it does something stupid (like using other than minimum values >>> for the flash-max-microamp/flash-max-timeout-us), it needs to be >>> fixed. >> >> Well we should be able to test the probe/parse dt node and reject the probe if the node is not >> present. That can be done without HW the setup is done pretty early in the probe without even attempting to communicate >> with the hardware. >> > > Well, yes, you can do that. > > But if someone is actively using board with DT w/o flash-max-microamp, > you'll make them unhappy. > I grepped the dt directories and I did not find any upstream dt files configuring the max77693 with a flash led source. Unless Jacek can point out an upstream file I may have over looked. Unhappiness will come from not upstreaming your code ;) Dan > Best regards, > Pavel > -- ------------------ Dan Murphy ^ permalink raw reply [flat|nested] 33+ messages in thread
* Re: [PATCH v5 2/2] leds: lm3601x: Introduce the lm3601x LED driver @ 2018-05-14 20:31 ` Dan Murphy 0 siblings, 0 replies; 33+ messages in thread From: Dan Murphy @ 2018-05-14 20:31 UTC (permalink / raw) To: Pavel Machek Cc: Jacek Anaszewski, robh+dt, mark.rutland, afd, devicetree, linux-kernel, linux-leds On 05/14/2018 03:16 PM, Pavel Machek wrote: > On Mon 2018-05-14 15:13:34, Dan Murphy wrote: >> Pavel >> >> On 05/14/2018 03:05 PM, Pavel Machek wrote: >>> Hi! >>> >>>>> OK. >>>> >>>> OK I looked at the max776973 driver and well if the flash-max-microamp and >>>> flash-max-timeout-us nodes are missing it sets a default value for each if the >>>> node is not present. >>>> >>>> So should we remove this code from the Max77693 driver too and fail probe as being asked >>>> in this driver? >>> >>> Well, modifying driver without access to the hardware is tricky >>> :-(. If it does something stupid (like using other than minimum values >>> for the flash-max-microamp/flash-max-timeout-us), it needs to be >>> fixed. >> >> Well we should be able to test the probe/parse dt node and reject the probe if the node is not >> present. That can be done without HW the setup is done pretty early in the probe without even attempting to communicate >> with the hardware. >> > > Well, yes, you can do that. > > But if someone is actively using board with DT w/o flash-max-microamp, > you'll make them unhappy. > I grepped the dt directories and I did not find any upstream dt files configuring the max77693 with a flash led source. Unless Jacek can point out an upstream file I may have over looked. Unhappiness will come from not upstreaming your code ;) Dan > Best regards, > Pavel > -- ------------------ Dan Murphy ^ permalink raw reply [flat|nested] 33+ messages in thread
* Re: [PATCH v5 2/2] leds: lm3601x: Introduce the lm3601x LED driver 2018-05-14 19:40 ` Dan Murphy ` (2 preceding siblings ...) (?) @ 2018-05-14 20:05 ` Jacek Anaszewski 2018-05-14 20:09 ` Dan Murphy -1 siblings, 1 reply; 33+ messages in thread From: Jacek Anaszewski @ 2018-05-14 20:05 UTC (permalink / raw) To: Dan Murphy, robh+dt, mark.rutland, pavel, afd Cc: devicetree, linux-kernel, linux-leds Hi Dan, On 05/14/2018 09:40 PM, Dan Murphy wrote: > Jacek > > On 05/11/2018 06:56 AM, Dan Murphy wrote: > <snip> > >>>> + } >>>> + >>>> + if (led->strobe_node) { >>>> + ret = of_property_read_string(led->strobe_node, "label", &name); >>>> + if (!ret) >>>> + snprintf(led->strobe, sizeof(led->strobe), >>>> + "%s:%s", led->strobe_node->name, name); >>>> + else >>>> + snprintf(led->strobe, sizeof(led->strobe), >>>> + "%s::strobe", led->strobe_node->name); >>>> + >>>> + ret = of_property_read_u32(led->strobe_node, >>>> + "flash-max-microamp", >>>> + &led->strobe_current_max); >>>> + if (ret < 0) { >>>> + led->strobe_current_max = LM3601X_MIN_STROBE_I_MA; >>>> + dev_warn(&led->client->dev, >>>> + "flash-max-microamp DT property missing\n"); >>>> + } >>>> + >>>> + ret = of_property_read_u32(led->strobe_node, >>>> + "flash-max-timeout-us", >>>> + &led->max_strobe_timeout); >>>> + if (ret < 0) { >>>> + led->max_strobe_timeout = strobe_timeouts[0].reg_val; >>>> + dev_warn(&led->client->dev, >>>> + "flash-max-timeout-us DT property missing\n"); >>>> + } >>> >>> Common LED bindings state that flash-max-microamp and >>> flash-max-timeout-us properties are mandatory. >> >> OK. > > OK I looked at the max776973 driver and well if the flash-max-microamp and > flash-max-timeout-us nodes are missing it sets a default value for each if the > node is not present. Ah, yes, this driver was being introduced as the first LED flash class driver and we were being iteratively adjusting LED common bindings according to the new findings, so some details could have been left out of sync. > So should we remove this code from the Max77693 driver too and fail probe as being asked > in this driver? Yes, that would match what the bindings require. -- Best regards, Jacek Anaszewski ^ permalink raw reply [flat|nested] 33+ messages in thread
* Re: [PATCH v5 2/2] leds: lm3601x: Introduce the lm3601x LED driver 2018-05-14 20:05 ` Jacek Anaszewski @ 2018-05-14 20:09 ` Dan Murphy 0 siblings, 0 replies; 33+ messages in thread From: Dan Murphy @ 2018-05-14 20:09 UTC (permalink / raw) To: Jacek Anaszewski, robh+dt, mark.rutland, pavel, afd Cc: devicetree, linux-kernel, linux-leds On 05/14/2018 03:05 PM, Jacek Anaszewski wrote: > Hi Dan, > > On 05/14/2018 09:40 PM, Dan Murphy wrote: >> Jacek >> >> On 05/11/2018 06:56 AM, Dan Murphy wrote: >> <snip> >> >>>>> + } >>>>> + >>>>> + if (led->strobe_node) { >>>>> + ret = of_property_read_string(led->strobe_node, "label", &name); >>>>> + if (!ret) >>>>> + snprintf(led->strobe, sizeof(led->strobe), >>>>> + "%s:%s", led->strobe_node->name, name); >>>>> + else >>>>> + snprintf(led->strobe, sizeof(led->strobe), >>>>> + "%s::strobe", led->strobe_node->name); >>>>> + >>>>> + ret = of_property_read_u32(led->strobe_node, >>>>> + "flash-max-microamp", >>>>> + &led->strobe_current_max); >>>>> + if (ret < 0) { >>>>> + led->strobe_current_max = LM3601X_MIN_STROBE_I_MA; >>>>> + dev_warn(&led->client->dev, >>>>> + "flash-max-microamp DT property missing\n"); >>>>> + } >>>>> + >>>>> + ret = of_property_read_u32(led->strobe_node, >>>>> + "flash-max-timeout-us", >>>>> + &led->max_strobe_timeout); >>>>> + if (ret < 0) { >>>>> + led->max_strobe_timeout = strobe_timeouts[0].reg_val; >>>>> + dev_warn(&led->client->dev, >>>>> + "flash-max-timeout-us DT property missing\n"); >>>>> + } >>>> >>>> Common LED bindings state that flash-max-microamp and >>>> flash-max-timeout-us properties are mandatory. >>> >>> OK. >> >> OK I looked at the max776973 driver and well if the flash-max-microamp and >> flash-max-timeout-us nodes are missing it sets a default value for each if the >> node is not present. > > Ah, yes, this driver was being introduced as the first LED flash class driver and we were being iteratively adjusting LED common bindings > according to the new findings, so some details could have been left > out of sync. > >> So should we remove this code from the Max77693 driver too and fail probe as being asked >> in this driver? > > Yes, that would match what the bindings require. Did you want me to remove it and submit? I don't have a board to verify but I can definitely test out the probe and parse dt functionality. Don't need HW for that. Dan > -- ------------------ Dan Murphy ^ permalink raw reply [flat|nested] 33+ messages in thread
* Re: [PATCH v5 2/2] leds: lm3601x: Introduce the lm3601x LED driver @ 2018-05-14 20:09 ` Dan Murphy 0 siblings, 0 replies; 33+ messages in thread From: Dan Murphy @ 2018-05-14 20:09 UTC (permalink / raw) To: Jacek Anaszewski, robh+dt, mark.rutland, pavel, afd Cc: devicetree, linux-kernel, linux-leds On 05/14/2018 03:05 PM, Jacek Anaszewski wrote: > Hi Dan, > > On 05/14/2018 09:40 PM, Dan Murphy wrote: >> Jacek >> >> On 05/11/2018 06:56 AM, Dan Murphy wrote: >> <snip> >> >>>>> + } >>>>> + >>>>> + if (led->strobe_node) { >>>>> + ret = of_property_read_string(led->strobe_node, "label", &name); >>>>> + if (!ret) >>>>> + snprintf(led->strobe, sizeof(led->strobe), >>>>> + "%s:%s", led->strobe_node->name, name); >>>>> + else >>>>> + snprintf(led->strobe, sizeof(led->strobe), >>>>> + "%s::strobe", led->strobe_node->name); >>>>> + >>>>> + ret = of_property_read_u32(led->strobe_node, >>>>> + "flash-max-microamp", >>>>> + &led->strobe_current_max); >>>>> + if (ret < 0) { >>>>> + led->strobe_current_max = LM3601X_MIN_STROBE_I_MA; >>>>> + dev_warn(&led->client->dev, >>>>> + "flash-max-microamp DT property missing\n"); >>>>> + } >>>>> + >>>>> + ret = of_property_read_u32(led->strobe_node, >>>>> + "flash-max-timeout-us", >>>>> + &led->max_strobe_timeout); >>>>> + if (ret < 0) { >>>>> + led->max_strobe_timeout = strobe_timeouts[0].reg_val; >>>>> + dev_warn(&led->client->dev, >>>>> + "flash-max-timeout-us DT property missing\n"); >>>>> + } >>>> >>>> Common LED bindings state that flash-max-microamp and >>>> flash-max-timeout-us properties are mandatory. >>> >>> OK. >> >> OK I looked at the max776973 driver and well if the flash-max-microamp and >> flash-max-timeout-us nodes are missing it sets a default value for each if the >> node is not present. > > Ah, yes, this driver was being introduced as the first LED flash class driver and we were being iteratively adjusting LED common bindings > according to the new findings, so some details could have been left > out of sync. > >> So should we remove this code from the Max77693 driver too and fail probe as being asked >> in this driver? > > Yes, that would match what the bindings require. Did you want me to remove it and submit? I don't have a board to verify but I can definitely test out the probe and parse dt functionality. Don't need HW for that. Dan > -- ------------------ Dan Murphy ^ permalink raw reply [flat|nested] 33+ messages in thread
* Re: [PATCH v5 1/2] dt: bindings: lm3601x: Introduce the lm3601x driver 2018-05-10 17:47 ` Dan Murphy (?) (?) @ 2018-05-10 18:54 ` Pavel Machek 2018-05-10 19:06 ` Dan Murphy -1 siblings, 1 reply; 33+ messages in thread From: Pavel Machek @ 2018-05-10 18:54 UTC (permalink / raw) To: Dan Murphy Cc: robh+dt, mark.rutland, jacek.anaszewski, afd, devicetree, linux-kernel, linux-leds [-- Attachment #1: Type: text/plain, Size: 1198 bytes --] Hi! > Introduce the device tree bindings for the lm3601x > family of LED torch, flash and IR drivers. > > Signed-off-by: Dan Murphy <dmurphy@ti.com> Better, thanks. > +++ b/Documentation/devicetree/bindings/leds/leds-lm3601x.txt > @@ -0,0 +1,50 @@ > +* Texas Instruments - lm3601x Single-LED Flash Driver Ok, so is it single-LED driver, or can it driver ir & white LEDs at the same time? > +Example: > +led-controller@64 { > + compatible = "ti,lm36010"; > + #address-cells = <1>; > + #size-cells = <0>; > + reg = <0x64>; > + > + led@0 { > + reg = <0>; > + label = "white:torch"; > + led-max-microamp = <10000>; > + }; > + > + led@1 { > + reg = <1>; > + label = "white:flash"; > + flash-max-microamp = <10000>; > + flash-max-timeout-us = <800>; > + }; Is this realistic config? I'd expect flash to use more power than torch, and would expect longer timeout than 0.8msec. Also.. if this is physically one white LED, it should not be spread over reg = <0> and reg = <1>... Best regards, Pavel -- (english) http://www.livejournal.com/~pavelmachek (cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html [-- Attachment #2: Digital signature --] [-- Type: application/pgp-signature, Size: 181 bytes --] ^ permalink raw reply [flat|nested] 33+ messages in thread
* Re: [PATCH v5 1/2] dt: bindings: lm3601x: Introduce the lm3601x driver 2018-05-10 18:54 ` [PATCH v5 1/2] dt: bindings: lm3601x: Introduce the lm3601x driver Pavel Machek @ 2018-05-10 19:06 ` Dan Murphy 0 siblings, 0 replies; 33+ messages in thread From: Dan Murphy @ 2018-05-10 19:06 UTC (permalink / raw) To: Pavel Machek Cc: robh+dt, mark.rutland, jacek.anaszewski, afd, devicetree, linux-kernel, linux-leds Pavel On 05/10/2018 01:54 PM, Pavel Machek wrote: > Hi! > >> Introduce the device tree bindings for the lm3601x >> family of LED torch, flash and IR drivers. >> >> Signed-off-by: Dan Murphy <dmurphy@ti.com> > > Better, thanks. >> +++ b/Documentation/devicetree/bindings/leds/leds-lm3601x.txt >> @@ -0,0 +1,50 @@ >> +* Texas Instruments - lm3601x Single-LED Flash Driver > > Ok, so is it single-LED driver, or can it driver ir & white LEDs at > the same time? It is a single LED driver. It can drive a Torch white LED or IR LED indefinitely or if the driver is programmed to strobe mode the driver will drive the configured LED for the flash timeout specified. Basically a flash and a flash light. IR or White LED. > >> +Example: >> +led-controller@64 { >> + compatible = "ti,lm36010"; >> + #address-cells = <1>; >> + #size-cells = <0>; >> + reg = <0x64>; >> + >> + led@0 { >> + reg = <0>; >> + label = "white:torch"; >> + led-max-microamp = <10000>; >> + }; >> + >> + led@1 { >> + reg = <1>; >> + label = "white:flash"; >> + flash-max-microamp = <10000>; >> + flash-max-timeout-us = <800>; >> + }; > > Is this realistic config? I'd expect flash to use more power than > torch, and would expect longer timeout than 0.8msec. Timeout in the spreadsheet is ms I will update this example and code Current in the spreadsheet is mA I will update this example and code > > Also.. if this is physically one white LED, it should not be > spread over reg = <0> and reg = <1>... If the torch LED and strobe LED are the same LED how do I expose them both to the user. It is up to consumer to configure the required interfaces they want to expose to the filesystem. Maybe they don't want the strobe feature and just want LED on/off and switch between IR and White. The IR is driven via a different output pin. Dan > > Best regards, > Pavel > -- ------------------ Dan Murphy ^ permalink raw reply [flat|nested] 33+ messages in thread
* Re: [PATCH v5 1/2] dt: bindings: lm3601x: Introduce the lm3601x driver @ 2018-05-10 19:06 ` Dan Murphy 0 siblings, 0 replies; 33+ messages in thread From: Dan Murphy @ 2018-05-10 19:06 UTC (permalink / raw) To: Pavel Machek Cc: robh+dt, mark.rutland, jacek.anaszewski, afd, devicetree, linux-kernel, linux-leds Pavel On 05/10/2018 01:54 PM, Pavel Machek wrote: > Hi! > >> Introduce the device tree bindings for the lm3601x >> family of LED torch, flash and IR drivers. >> >> Signed-off-by: Dan Murphy <dmurphy@ti.com> > > Better, thanks. >> +++ b/Documentation/devicetree/bindings/leds/leds-lm3601x.txt >> @@ -0,0 +1,50 @@ >> +* Texas Instruments - lm3601x Single-LED Flash Driver > > Ok, so is it single-LED driver, or can it driver ir & white LEDs at > the same time? It is a single LED driver. It can drive a Torch white LED or IR LED indefinitely or if the driver is programmed to strobe mode the driver will drive the configured LED for the flash timeout specified. Basically a flash and a flash light. IR or White LED. > >> +Example: >> +led-controller@64 { >> + compatible = "ti,lm36010"; >> + #address-cells = <1>; >> + #size-cells = <0>; >> + reg = <0x64>; >> + >> + led@0 { >> + reg = <0>; >> + label = "white:torch"; >> + led-max-microamp = <10000>; >> + }; >> + >> + led@1 { >> + reg = <1>; >> + label = "white:flash"; >> + flash-max-microamp = <10000>; >> + flash-max-timeout-us = <800>; >> + }; > > Is this realistic config? I'd expect flash to use more power than > torch, and would expect longer timeout than 0.8msec. Timeout in the spreadsheet is ms I will update this example and code Current in the spreadsheet is mA I will update this example and code > > Also.. if this is physically one white LED, it should not be > spread over reg = <0> and reg = <1>... If the torch LED and strobe LED are the same LED how do I expose them both to the user. It is up to consumer to configure the required interfaces they want to expose to the filesystem. Maybe they don't want the strobe feature and just want LED on/off and switch between IR and White. The IR is driven via a different output pin. Dan > > Best regards, > Pavel > -- ------------------ Dan Murphy ^ permalink raw reply [flat|nested] 33+ messages in thread
* Re: [PATCH v5 1/2] dt: bindings: lm3601x: Introduce the lm3601x driver 2018-05-10 19:06 ` Dan Murphy @ 2018-05-10 19:10 ` Dan Murphy -1 siblings, 0 replies; 33+ messages in thread From: Dan Murphy @ 2018-05-10 19:10 UTC (permalink / raw) To: Pavel Machek Cc: robh+dt, mark.rutland, jacek.anaszewski, afd, devicetree, linux-kernel, linux-leds On 05/10/2018 02:06 PM, Dan Murphy wrote: > Pavel > > On 05/10/2018 01:54 PM, Pavel Machek wrote: >> Hi! >> >>> Introduce the device tree bindings for the lm3601x >>> family of LED torch, flash and IR drivers. >>> >>> Signed-off-by: Dan Murphy <dmurphy@ti.com> >> >> Better, thanks. >>> +++ b/Documentation/devicetree/bindings/leds/leds-lm3601x.txt >>> @@ -0,0 +1,50 @@ >>> +* Texas Instruments - lm3601x Single-LED Flash Driver >> >> Ok, so is it single-LED driver, or can it driver ir & white LEDs at >> the same time? > > It is a single LED driver. It can drive a Torch white LED or IR LED indefinitely or if the driver > is programmed to strobe mode the driver will drive the configured LED for the flash timeout specified. > > Basically a flash and a flash light. IR or White LED. > > >> >>> +Example: >>> +led-controller@64 { >>> + compatible = "ti,lm36010"; >>> + #address-cells = <1>; >>> + #size-cells = <0>; >>> + reg = <0x64>; >>> + >>> + led@0 { >>> + reg = <0>; >>> + label = "white:torch"; >>> + led-max-microamp = <10000>; >>> + }; >>> + >>> + led@1 { >>> + reg = <1>; >>> + label = "white:flash"; >>> + flash-max-microamp = <10000>; >>> + flash-max-timeout-us = <800>; >>> + }; >> >> Is this realistic config? I'd expect flash to use more power than >> torch, and would expect longer timeout than 0.8msec. > > Timeout in the spreadsheet is ms I will update this example and code > Current in the spreadsheet is mA I will update this example and code > >> >> Also.. if this is physically one white LED, it should not be >> spread over reg = <0> and reg = <1>... > > If the torch LED and strobe LED are the same LED how do I expose them both to the user. > It is up to consumer to configure the required interfaces they want to expose to the filesystem. > > Maybe they don't want the strobe feature and just want LED on/off and switch between IR and White. > > The IR is driven via a different output pin. Correction. There is only one LED drive pin. The mode selected determines how the device will drive the LED. So you may have one device to drive a Torch and another device to drive the LED. Strobe is available in either case but not always required if strobe is not desired by the customer hence why I have a separate DT node for it. Dan > > Dan > >> >> Best regards, >> Pavel >> > > -- ------------------ Dan Murphy ^ permalink raw reply [flat|nested] 33+ messages in thread
* Re: [PATCH v5 1/2] dt: bindings: lm3601x: Introduce the lm3601x driver @ 2018-05-10 19:10 ` Dan Murphy 0 siblings, 0 replies; 33+ messages in thread From: Dan Murphy @ 2018-05-10 19:10 UTC (permalink / raw) To: Pavel Machek Cc: robh+dt, mark.rutland, jacek.anaszewski, afd, devicetree, linux-kernel, linux-leds On 05/10/2018 02:06 PM, Dan Murphy wrote: > Pavel > > On 05/10/2018 01:54 PM, Pavel Machek wrote: >> Hi! >> >>> Introduce the device tree bindings for the lm3601x >>> family of LED torch, flash and IR drivers. >>> >>> Signed-off-by: Dan Murphy <dmurphy@ti.com> >> >> Better, thanks. >>> +++ b/Documentation/devicetree/bindings/leds/leds-lm3601x.txt >>> @@ -0,0 +1,50 @@ >>> +* Texas Instruments - lm3601x Single-LED Flash Driver >> >> Ok, so is it single-LED driver, or can it driver ir & white LEDs at >> the same time? > > It is a single LED driver. It can drive a Torch white LED or IR LED indefinitely or if the driver > is programmed to strobe mode the driver will drive the configured LED for the flash timeout specified. > > Basically a flash and a flash light. IR or White LED. > > >> >>> +Example: >>> +led-controller@64 { >>> + compatible = "ti,lm36010"; >>> + #address-cells = <1>; >>> + #size-cells = <0>; >>> + reg = <0x64>; >>> + >>> + led@0 { >>> + reg = <0>; >>> + label = "white:torch"; >>> + led-max-microamp = <10000>; >>> + }; >>> + >>> + led@1 { >>> + reg = <1>; >>> + label = "white:flash"; >>> + flash-max-microamp = <10000>; >>> + flash-max-timeout-us = <800>; >>> + }; >> >> Is this realistic config? I'd expect flash to use more power than >> torch, and would expect longer timeout than 0.8msec. > > Timeout in the spreadsheet is ms I will update this example and code > Current in the spreadsheet is mA I will update this example and code > >> >> Also.. if this is physically one white LED, it should not be >> spread over reg = <0> and reg = <1>... > > If the torch LED and strobe LED are the same LED how do I expose them both to the user. > It is up to consumer to configure the required interfaces they want to expose to the filesystem. > > Maybe they don't want the strobe feature and just want LED on/off and switch between IR and White. > > The IR is driven via a different output pin. Correction. There is only one LED drive pin. The mode selected determines how the device will drive the LED. So you may have one device to drive a Torch and another device to drive the LED. Strobe is available in either case but not always required if strobe is not desired by the customer hence why I have a separate DT node for it. Dan > > Dan > >> >> Best regards, >> Pavel >> > > -- ------------------ Dan Murphy ^ permalink raw reply [flat|nested] 33+ messages in thread
* Re: [PATCH v5 1/2] dt: bindings: lm3601x: Introduce the lm3601x driver 2018-05-10 19:10 ` Dan Murphy (?) @ 2018-05-10 20:17 ` Jacek Anaszewski 2018-05-11 12:12 ` Dan Murphy -1 siblings, 1 reply; 33+ messages in thread From: Jacek Anaszewski @ 2018-05-10 20:17 UTC (permalink / raw) To: Dan Murphy, Pavel Machek Cc: robh+dt, mark.rutland, afd, devicetree, linux-kernel, linux-leds Hi Dan, On 05/10/2018 09:10 PM, Dan Murphy wrote: > On 05/10/2018 02:06 PM, Dan Murphy wrote: >> Pavel >> >> On 05/10/2018 01:54 PM, Pavel Machek wrote: >>> Hi! >>> >>>> Introduce the device tree bindings for the lm3601x >>>> family of LED torch, flash and IR drivers. >>>> >>>> Signed-off-by: Dan Murphy <dmurphy@ti.com> >>> >>> Better, thanks. >>>> +++ b/Documentation/devicetree/bindings/leds/leds-lm3601x.txt >>>> @@ -0,0 +1,50 @@ >>>> +* Texas Instruments - lm3601x Single-LED Flash Driver >>> >>> Ok, so is it single-LED driver, or can it driver ir & white LEDs at >>> the same time? >> >> It is a single LED driver. It can drive a Torch white LED or IR LED indefinitely or if the driver >> is programmed to strobe mode the driver will drive the configured LED for the flash timeout specified. >> >> Basically a flash and a flash light. IR or White LED. >> >> >>> >>>> +Example: >>>> +led-controller@64 { >>>> + compatible = "ti,lm36010"; >>>> + #address-cells = <1>; >>>> + #size-cells = <0>; >>>> + reg = <0x64>; >>>> + >>>> + led@0 { >>>> + reg = <0>; >>>> + label = "white:torch"; >>>> + led-max-microamp = <10000>; >>>> + }; >>>> + >>>> + led@1 { >>>> + reg = <1>; >>>> + label = "white:flash"; >>>> + flash-max-microamp = <10000>; >>>> + flash-max-timeout-us = <800>; >>>> + }; >>> >>> Is this realistic config? I'd expect flash to use more power than >>> torch, and would expect longer timeout than 0.8msec. >> >> Timeout in the spreadsheet is ms I will update this example and code >> Current in the spreadsheet is mA I will update this example and code >> >>> >>> Also.. if this is physically one white LED, it should not be >>> spread over reg = <0> and reg = <1>... >> >> If the torch LED and strobe LED are the same LED how do I expose them both to the user. >> It is up to consumer to configure the required interfaces they want to expose to the filesystem. LED flash class interface is prepared for it. led_clasdev_flash_register() internally calls led_classdev_register(), so there is brightness file available for torch related operations. >> Maybe they don't want the strobe feature and just want LED on/off and switch between IR and White. >> >> The IR is driven via a different output pin. > > Correction. There is only one LED drive pin. The mode selected determines how the device will drive the > LED. So you may have one device to drive a Torch and another device to drive the LED. But only one type of LED can be soldered on the board at a time, right? If yes, then this is clearly a DT related configuration, and only one child DT node should be defined for given board. > Strobe is available in either case but not always required if strobe is not desired by the customer hence > why I have a separate DT node for it. > > Dan > >> >> Dan >> >>> >>> Best regards, >>> Pavel >>> >> >> > > -- Best regards, Jacek Anaszewski ^ permalink raw reply [flat|nested] 33+ messages in thread
* Re: [PATCH v5 1/2] dt: bindings: lm3601x: Introduce the lm3601x driver 2018-05-10 20:17 ` Jacek Anaszewski @ 2018-05-11 12:12 ` Dan Murphy 0 siblings, 0 replies; 33+ messages in thread From: Dan Murphy @ 2018-05-11 12:12 UTC (permalink / raw) To: Jacek Anaszewski, Pavel Machek Cc: robh+dt, mark.rutland, afd, devicetree, linux-kernel, linux-leds Jacek Thanks for the review On 05/10/2018 03:17 PM, Jacek Anaszewski wrote: > Hi Dan, > > On 05/10/2018 09:10 PM, Dan Murphy wrote: >> On 05/10/2018 02:06 PM, Dan Murphy wrote: >>> Pavel >>> >>> On 05/10/2018 01:54 PM, Pavel Machek wrote: >>>> Hi! >>>> >>>>> Introduce the device tree bindings for the lm3601x >>>>> family of LED torch, flash and IR drivers. >>>>> >>>>> Signed-off-by: Dan Murphy <dmurphy@ti.com> >>>> >>>> Better, thanks. >>>>> +++ b/Documentation/devicetree/bindings/leds/leds-lm3601x.txt >>>>> @@ -0,0 +1,50 @@ >>>>> +* Texas Instruments - lm3601x Single-LED Flash Driver >>>> >>>> Ok, so is it single-LED driver, or can it driver ir & white LEDs at >>>> the same time? >>> >>> It is a single LED driver. It can drive a Torch white LED or IR LED indefinitely or if the driver >>> is programmed to strobe mode the driver will drive the configured LED for the flash timeout specified. >>> >>> Basically a flash and a flash light. IR or White LED. >>> >>> >>>> >>>>> +Example: >>>>> +led-controller@64 { >>>>> + compatible = "ti,lm36010"; >>>>> + #address-cells = <1>; >>>>> + #size-cells = <0>; >>>>> + reg = <0x64>; >>>>> + >>>>> + led@0 { >>>>> + reg = <0>; >>>>> + label = "white:torch"; >>>>> + led-max-microamp = <10000>; >>>>> + }; >>>>> + >>>>> + led@1 { >>>>> + reg = <1>; >>>>> + label = "white:flash"; >>>>> + flash-max-microamp = <10000>; >>>>> + flash-max-timeout-us = <800>; >>>>> + }; >>>> >>>> Is this realistic config? I'd expect flash to use more power than >>>> torch, and would expect longer timeout than 0.8msec. >>> >>> Timeout in the spreadsheet is ms I will update this example and code >>> Current in the spreadsheet is mA I will update this example and code >>> >>>> >>>> Also.. if this is physically one white LED, it should not be >>>> spread over reg = <0> and reg = <1>... >>> >>> If the torch LED and strobe LED are the same LED how do I expose them both to the user. >>> It is up to consumer to configure the required interfaces they want to expose to the filesystem. > > LED flash class interface is prepared for it. > led_clasdev_flash_register() internally calls led_classdev_register(), > so there is brightness file available for torch related operations. Yes the flash class may handle this and expose the brightness node but the HW has two different registers to set the corresponding brightness so one set brightness node does not work. There is no way to differentiate between the strobe brightness (register 4) and the torch brightness (register 3). When the enable register is written the driver will read the corresponding register and set the current for the LED. This is why I separated out the strobe from the torch into 2 different LED nodes. I cannot seem to find the data sheet on line for the max77693 so I cannot verify that the device only has a single brightness register for both torch and strobe. > >>> Maybe they don't want the strobe feature and just want LED on/off and switch between IR and White. >>> >>> The IR is driven via a different output pin. >> >> Correction. There is only one LED drive pin. The mode selected determines how the device will drive the >> LED. So you may have one device to drive a Torch and another device to drive the LED. > > But only one type of LED can be soldered on the board at a time, right? > If yes, then this is clearly a DT related configuration, and only > one child DT node should be defined for given board. But see above. There is not a clean way to expose the torch/ir and strobe separately. Dan > >> Strobe is available in either case but not always required if strobe is not desired by the customer hence >> why I have a separate DT node for it. >> >> Dan >> >>> >>> Dan >>> >>>> >>>> Best regards, >>>> Pavel >>>> >>> >>> >> >> > -- ------------------ Dan Murphy ^ permalink raw reply [flat|nested] 33+ messages in thread
* Re: [PATCH v5 1/2] dt: bindings: lm3601x: Introduce the lm3601x driver @ 2018-05-11 12:12 ` Dan Murphy 0 siblings, 0 replies; 33+ messages in thread From: Dan Murphy @ 2018-05-11 12:12 UTC (permalink / raw) To: Jacek Anaszewski, Pavel Machek Cc: robh+dt, mark.rutland, afd, devicetree, linux-kernel, linux-leds Jacek Thanks for the review On 05/10/2018 03:17 PM, Jacek Anaszewski wrote: > Hi Dan, > > On 05/10/2018 09:10 PM, Dan Murphy wrote: >> On 05/10/2018 02:06 PM, Dan Murphy wrote: >>> Pavel >>> >>> On 05/10/2018 01:54 PM, Pavel Machek wrote: >>>> Hi! >>>> >>>>> Introduce the device tree bindings for the lm3601x >>>>> family of LED torch, flash and IR drivers. >>>>> >>>>> Signed-off-by: Dan Murphy <dmurphy@ti.com> >>>> >>>> Better, thanks. >>>>> +++ b/Documentation/devicetree/bindings/leds/leds-lm3601x.txt >>>>> @@ -0,0 +1,50 @@ >>>>> +* Texas Instruments - lm3601x Single-LED Flash Driver >>>> >>>> Ok, so is it single-LED driver, or can it driver ir & white LEDs at >>>> the same time? >>> >>> It is a single LED driver. It can drive a Torch white LED or IR LED indefinitely or if the driver >>> is programmed to strobe mode the driver will drive the configured LED for the flash timeout specified. >>> >>> Basically a flash and a flash light. IR or White LED. >>> >>> >>>> >>>>> +Example: >>>>> +led-controller@64 { >>>>> + compatible = "ti,lm36010"; >>>>> + #address-cells = <1>; >>>>> + #size-cells = <0>; >>>>> + reg = <0x64>; >>>>> + >>>>> + led@0 { >>>>> + reg = <0>; >>>>> + label = "white:torch"; >>>>> + led-max-microamp = <10000>; >>>>> + }; >>>>> + >>>>> + led@1 { >>>>> + reg = <1>; >>>>> + label = "white:flash"; >>>>> + flash-max-microamp = <10000>; >>>>> + flash-max-timeout-us = <800>; >>>>> + }; >>>> >>>> Is this realistic config? I'd expect flash to use more power than >>>> torch, and would expect longer timeout than 0.8msec. >>> >>> Timeout in the spreadsheet is ms I will update this example and code >>> Current in the spreadsheet is mA I will update this example and code >>> >>>> >>>> Also.. if this is physically one white LED, it should not be >>>> spread over reg = <0> and reg = <1>... >>> >>> If the torch LED and strobe LED are the same LED how do I expose them both to the user. >>> It is up to consumer to configure the required interfaces they want to expose to the filesystem. > > LED flash class interface is prepared for it. > led_clasdev_flash_register() internally calls led_classdev_register(), > so there is brightness file available for torch related operations. Yes the flash class may handle this and expose the brightness node but the HW has two different registers to set the corresponding brightness so one set brightness node does not work. There is no way to differentiate between the strobe brightness (register 4) and the torch brightness (register 3). When the enable register is written the driver will read the corresponding register and set the current for the LED. This is why I separated out the strobe from the torch into 2 different LED nodes. I cannot seem to find the data sheet on line for the max77693 so I cannot verify that the device only has a single brightness register for both torch and strobe. > >>> Maybe they don't want the strobe feature and just want LED on/off and switch between IR and White. >>> >>> The IR is driven via a different output pin. >> >> Correction. There is only one LED drive pin. The mode selected determines how the device will drive the >> LED. So you may have one device to drive a Torch and another device to drive the LED. > > But only one type of LED can be soldered on the board at a time, right? > If yes, then this is clearly a DT related configuration, and only > one child DT node should be defined for given board. But see above. There is not a clean way to expose the torch/ir and strobe separately. Dan > >> Strobe is available in either case but not always required if strobe is not desired by the customer hence >> why I have a separate DT node for it. >> >> Dan >> >>> >>> Dan >>> >>>> >>>> Best regards, >>>> Pavel >>>> >>> >>> >> >> > -- ------------------ Dan Murphy ^ permalink raw reply [flat|nested] 33+ messages in thread
* Re: [PATCH v5 1/2] dt: bindings: lm3601x: Introduce the lm3601x driver 2018-05-11 12:12 ` Dan Murphy (?) @ 2018-05-11 20:27 ` Jacek Anaszewski 2018-05-14 20:07 ` Dan Murphy -1 siblings, 1 reply; 33+ messages in thread From: Jacek Anaszewski @ 2018-05-11 20:27 UTC (permalink / raw) To: Dan Murphy, Pavel Machek Cc: robh+dt, mark.rutland, afd, devicetree, linux-kernel, linux-leds Dan, On 05/11/2018 02:12 PM, Dan Murphy wrote: > Jacek > > Thanks for the review > > On 05/10/2018 03:17 PM, Jacek Anaszewski wrote: >> Hi Dan, >> >> On 05/10/2018 09:10 PM, Dan Murphy wrote: >>> On 05/10/2018 02:06 PM, Dan Murphy wrote: >>>> Pavel >>>> >>>> On 05/10/2018 01:54 PM, Pavel Machek wrote: >>>>> Hi! >>>>> >>>>>> Introduce the device tree bindings for the lm3601x >>>>>> family of LED torch, flash and IR drivers. >>>>>> >>>>>> Signed-off-by: Dan Murphy <dmurphy@ti.com> >>>>> >>>>> Better, thanks. >>>>>> +++ b/Documentation/devicetree/bindings/leds/leds-lm3601x.txt >>>>>> @@ -0,0 +1,50 @@ >>>>>> +* Texas Instruments - lm3601x Single-LED Flash Driver >>>>> >>>>> Ok, so is it single-LED driver, or can it driver ir & white LEDs at >>>>> the same time? >>>> >>>> It is a single LED driver. It can drive a Torch white LED or IR LED indefinitely or if the driver >>>> is programmed to strobe mode the driver will drive the configured LED for the flash timeout specified. >>>> >>>> Basically a flash and a flash light. IR or White LED. >>>> >>>> >>>>> >>>>>> +Example: >>>>>> +led-controller@64 { >>>>>> + compatible = "ti,lm36010"; >>>>>> + #address-cells = <1>; >>>>>> + #size-cells = <0>; >>>>>> + reg = <0x64>; >>>>>> + >>>>>> + led@0 { >>>>>> + reg = <0>; >>>>>> + label = "white:torch"; >>>>>> + led-max-microamp = <10000>; >>>>>> + }; >>>>>> + >>>>>> + led@1 { >>>>>> + reg = <1>; >>>>>> + label = "white:flash"; >>>>>> + flash-max-microamp = <10000>; >>>>>> + flash-max-timeout-us = <800>; >>>>>> + }; >>>>> >>>>> Is this realistic config? I'd expect flash to use more power than >>>>> torch, and would expect longer timeout than 0.8msec. >>>> >>>> Timeout in the spreadsheet is ms I will update this example and code >>>> Current in the spreadsheet is mA I will update this example and code >>>> >>>>> >>>>> Also.. if this is physically one white LED, it should not be >>>>> spread over reg = <0> and reg = <1>... >>>> >>>> If the torch LED and strobe LED are the same LED how do I expose them both to the user. >>>> It is up to consumer to configure the required interfaces they want to expose to the filesystem. >> >> LED flash class interface is prepared for it. >> led_clasdev_flash_register() internally calls led_classdev_register(), >> so there is brightness file available for torch related operations. > > Yes the flash class may handle this and expose the brightness node but the HW has two different registers to set the > corresponding brightness so one set brightness node does not work. > There is no way to differentiate between the strobe brightness (register 4) and the torch brightness (register 3). Hmm? There is brightness file (with brightness_set{_blocking} op) from LED class and flash_brightness file (with flash_brightness_set op) from LED class flash. I've only now realized that you're not using flash_brightness_set op for the "strobe_node" in your driver. I assume that you overlooked it. Please don't introduce "strobe_brightness" naming convention - we already have "flash_brightness" for that. > When the enable register is written the driver will read the corresponding register and set > the current for the LED. > > This is why I separated out the strobe from the torch into 2 different LED nodes. > > I cannot seem to find the data sheet on line for the max77693 so I cannot verify that the device only has > a single brightness register for both torch and strobe. It wasn't openly available at least at the time when I had an access to it. But - please look at the functions max77693_set_torch_current() and max77693_set_flash_current(). The device has separate registers for torch and flash brightnesses. >>>> Maybe they don't want the strobe feature and just want LED on/off and switch between IR and White. >>>> >>>> The IR is driven via a different output pin. >>> >>> Correction. There is only one LED drive pin. The mode selected determines how the device will drive the >>> LED. So you may have one device to drive a Torch and another device to drive the LED. >> >> But only one type of LED can be soldered on the board at a time, right? >> If yes, then this is clearly a DT related configuration, and only >> one child DT node should be defined for given board. > > But see above. There is not a clean way to expose the torch/ir and strobe separately. > > Dan > > >> >>> Strobe is available in either case but not always required if strobe is not desired by the customer hence >>> why I have a separate DT node for it. >>> >>> Dan >>> >>>> >>>> Dan >>>> >>>>> >>>>> Best regards, >>>>> Pavel >>>>> >>>> >>>> >>> >>> >> > > -- Best regards, Jacek Anaszewski ^ permalink raw reply [flat|nested] 33+ messages in thread
* Re: [PATCH v5 1/2] dt: bindings: lm3601x: Introduce the lm3601x driver 2018-05-11 20:27 ` Jacek Anaszewski @ 2018-05-14 20:07 ` Dan Murphy 0 siblings, 0 replies; 33+ messages in thread From: Dan Murphy @ 2018-05-14 20:07 UTC (permalink / raw) To: Jacek Anaszewski, Pavel Machek Cc: robh+dt, mark.rutland, afd, devicetree, linux-kernel, linux-leds Jacek On 05/11/2018 03:27 PM, Jacek Anaszewski wrote: > Dan, > > On 05/11/2018 02:12 PM, Dan Murphy wrote: >> Jacek >> >> Thanks for the review >> >> On 05/10/2018 03:17 PM, Jacek Anaszewski wrote: >>> Hi Dan, >>> >>> On 05/10/2018 09:10 PM, Dan Murphy wrote: >>>> On 05/10/2018 02:06 PM, Dan Murphy wrote: >>>>> Pavel >>>>> >>>>> On 05/10/2018 01:54 PM, Pavel Machek wrote: >>>>>> Hi! >>>>>> >>>>>>> Introduce the device tree bindings for the lm3601x >>>>>>> family of LED torch, flash and IR drivers. >>>>>>> >>>>>>> Signed-off-by: Dan Murphy <dmurphy@ti.com> >>>>>> >>>>>> Better, thanks. >>>>>>> +++ b/Documentation/devicetree/bindings/leds/leds-lm3601x.txt >>>>>>> @@ -0,0 +1,50 @@ >>>>>>> +* Texas Instruments - lm3601x Single-LED Flash Driver >>>>>> >>>>>> Ok, so is it single-LED driver, or can it driver ir & white LEDs at >>>>>> the same time? >>>>> >>>>> It is a single LED driver. It can drive a Torch white LED or IR LED indefinitely or if the driver >>>>> is programmed to strobe mode the driver will drive the configured LED for the flash timeout specified. >>>>> >>>>> Basically a flash and a flash light. IR or White LED. >>>>> >>>>> >>>>>> >>>>>>> +Example: >>>>>>> +led-controller@64 { >>>>>>> + compatible = "ti,lm36010"; >>>>>>> + #address-cells = <1>; >>>>>>> + #size-cells = <0>; >>>>>>> + reg = <0x64>; >>>>>>> + >>>>>>> + led@0 { >>>>>>> + reg = <0>; >>>>>>> + label = "white:torch"; >>>>>>> + led-max-microamp = <10000>; >>>>>>> + }; >>>>>>> + >>>>>>> + led@1 { >>>>>>> + reg = <1>; >>>>>>> + label = "white:flash"; >>>>>>> + flash-max-microamp = <10000>; >>>>>>> + flash-max-timeout-us = <800>; >>>>>>> + }; >>>>>> >>>>>> Is this realistic config? I'd expect flash to use more power than >>>>>> torch, and would expect longer timeout than 0.8msec. >>>>> >>>>> Timeout in the spreadsheet is ms I will update this example and code >>>>> Current in the spreadsheet is mA I will update this example and code >>>>> >>>>>> >>>>>> Also.. if this is physically one white LED, it should not be >>>>>> spread over reg = <0> and reg = <1>... >>>>> >>>>> If the torch LED and strobe LED are the same LED how do I expose them both to the user. >>>>> It is up to consumer to configure the required interfaces they want to expose to the filesystem. >>> >>> LED flash class interface is prepared for it. >>> led_clasdev_flash_register() internally calls led_classdev_register(), >>> so there is brightness file available for torch related operations. >> >> Yes the flash class may handle this and expose the brightness node but the HW has two different registers to set the >> corresponding brightness so one set brightness node does not work. >> There is no way to differentiate between the strobe brightness (register 4) and the torch brightness (register 3). > > Hmm? There is brightness file (with brightness_set{_blocking} op) from > LED class and flash_brightness file (with flash_brightness_set op) from > LED class flash. > > I've only now realized that you're not using flash_brightness_set op for > the "strobe_node" in your driver. I assume that you overlooked it. > Please don't introduce "strobe_brightness" naming convention - we > already have "flash_brightness" for that. > >> When the enable register is written the driver will read the corresponding register and set >> the current for the LED. >> >> This is why I separated out the strobe from the torch into 2 different LED nodes. >> >> I cannot seem to find the data sheet on line for the max77693 so I cannot verify that the device only has >> a single brightness register for both torch and strobe. > > It wasn't openly available at least at the time when I had an access > to it. > > But - please look at the functions max77693_set_torch_current() and > max77693_set_flash_current(). The device has separate registers > for torch and flash brightnesses. I have looked at these functions and the parse dt function that dictates if there is 1 fled or 2 fled. I am having trouble associating what these mean because the led-sources is being used to define how many LEDs. In the common.txt it says "- led-sources : List of device current outputs the LED is connected to. The outputs are identified by the numbers that must be defined in the LED device binding documentation." I cannot find the max77693 dt documentation or even a dt file that defines what the led-sources could be defined as. In addition, per the common.txt, the led sources should define the current outputs the LED is connected to. Is one to assume that it is referring to the physical current pin connection or the devices internal current routing? When I first referenced this driver as template driver it was misleading to say fled1 and fled2 as I took it to mean, and I still do, that the driver supports 2 LED connections and not a single output pin with different internal current routing. Without the data sheet or the dt documentation it makes understanding a little difficult. I will fix it up how ever you would like it to be but I am thinking we need to fix up the max77693 as well so we can have 2 reference drivers. It is very difficult to derive the driver without a public version of the data sheet. Dan > >>>>> Maybe they don't want the strobe feature and just want LED on/off and switch between IR and White. >>>>> >>>>> The IR is driven via a different output pin. >>>> >>>> Correction. There is only one LED drive pin. The mode selected determines how the device will drive the >>>> LED. So you may have one device to drive a Torch and another device to drive the LED. >>> >>> But only one type of LED can be soldered on the board at a time, right? >>> If yes, then this is clearly a DT related configuration, and only >>> one child DT node should be defined for given board. >> >> But see above. There is not a clean way to expose the torch/ir and strobe separately. >> >> Dan >> >> >>> >>>> Strobe is available in either case but not always required if strobe is not desired by the customer hence >>>> why I have a separate DT node for it. >>>> >>>> Dan >>>> >>>>> >>>>> Dan >>>>> >>>>>> >>>>>> Best regards, >>>>>> Pavel >>>>>> >>>>> >>>>> >>>> >>>> >>> >> >> > -- ------------------ Dan Murphy ^ permalink raw reply [flat|nested] 33+ messages in thread
* Re: [PATCH v5 1/2] dt: bindings: lm3601x: Introduce the lm3601x driver @ 2018-05-14 20:07 ` Dan Murphy 0 siblings, 0 replies; 33+ messages in thread From: Dan Murphy @ 2018-05-14 20:07 UTC (permalink / raw) To: Jacek Anaszewski, Pavel Machek Cc: robh+dt, mark.rutland, afd, devicetree, linux-kernel, linux-leds Jacek On 05/11/2018 03:27 PM, Jacek Anaszewski wrote: > Dan, > > On 05/11/2018 02:12 PM, Dan Murphy wrote: >> Jacek >> >> Thanks for the review >> >> On 05/10/2018 03:17 PM, Jacek Anaszewski wrote: >>> Hi Dan, >>> >>> On 05/10/2018 09:10 PM, Dan Murphy wrote: >>>> On 05/10/2018 02:06 PM, Dan Murphy wrote: >>>>> Pavel >>>>> >>>>> On 05/10/2018 01:54 PM, Pavel Machek wrote: >>>>>> Hi! >>>>>> >>>>>>> Introduce the device tree bindings for the lm3601x >>>>>>> family of LED torch, flash and IR drivers. >>>>>>> >>>>>>> Signed-off-by: Dan Murphy <dmurphy@ti.com> >>>>>> >>>>>> Better, thanks. >>>>>>> +++ b/Documentation/devicetree/bindings/leds/leds-lm3601x.txt >>>>>>> @@ -0,0 +1,50 @@ >>>>>>> +* Texas Instruments - lm3601x Single-LED Flash Driver >>>>>> >>>>>> Ok, so is it single-LED driver, or can it driver ir & white LEDs at >>>>>> the same time? >>>>> >>>>> It is a single LED driver. It can drive a Torch white LED or IR LED indefinitely or if the driver >>>>> is programmed to strobe mode the driver will drive the configured LED for the flash timeout specified. >>>>> >>>>> Basically a flash and a flash light. IR or White LED. >>>>> >>>>> >>>>>> >>>>>>> +Example: >>>>>>> +led-controller@64 { >>>>>>> + compatible = "ti,lm36010"; >>>>>>> + #address-cells = <1>; >>>>>>> + #size-cells = <0>; >>>>>>> + reg = <0x64>; >>>>>>> + >>>>>>> + led@0 { >>>>>>> + reg = <0>; >>>>>>> + label = "white:torch"; >>>>>>> + led-max-microamp = <10000>; >>>>>>> + }; >>>>>>> + >>>>>>> + led@1 { >>>>>>> + reg = <1>; >>>>>>> + label = "white:flash"; >>>>>>> + flash-max-microamp = <10000>; >>>>>>> + flash-max-timeout-us = <800>; >>>>>>> + }; >>>>>> >>>>>> Is this realistic config? I'd expect flash to use more power than >>>>>> torch, and would expect longer timeout than 0.8msec. >>>>> >>>>> Timeout in the spreadsheet is ms I will update this example and code >>>>> Current in the spreadsheet is mA I will update this example and code >>>>> >>>>>> >>>>>> Also.. if this is physically one white LED, it should not be >>>>>> spread over reg = <0> and reg = <1>... >>>>> >>>>> If the torch LED and strobe LED are the same LED how do I expose them both to the user. >>>>> It is up to consumer to configure the required interfaces they want to expose to the filesystem. >>> >>> LED flash class interface is prepared for it. >>> led_clasdev_flash_register() internally calls led_classdev_register(), >>> so there is brightness file available for torch related operations. >> >> Yes the flash class may handle this and expose the brightness node but the HW has two different registers to set the >> corresponding brightness so one set brightness node does not work. >> There is no way to differentiate between the strobe brightness (register 4) and the torch brightness (register 3). > > Hmm? There is brightness file (with brightness_set{_blocking} op) from > LED class and flash_brightness file (with flash_brightness_set op) from > LED class flash. > > I've only now realized that you're not using flash_brightness_set op for > the "strobe_node" in your driver. I assume that you overlooked it. > Please don't introduce "strobe_brightness" naming convention - we > already have "flash_brightness" for that. > >> When the enable register is written the driver will read the corresponding register and set >> the current for the LED. >> >> This is why I separated out the strobe from the torch into 2 different LED nodes. >> >> I cannot seem to find the data sheet on line for the max77693 so I cannot verify that the device only has >> a single brightness register for both torch and strobe. > > It wasn't openly available at least at the time when I had an access > to it. > > But - please look at the functions max77693_set_torch_current() and > max77693_set_flash_current(). The device has separate registers > for torch and flash brightnesses. I have looked at these functions and the parse dt function that dictates if there is 1 fled or 2 fled. I am having trouble associating what these mean because the led-sources is being used to define how many LEDs. In the common.txt it says "- led-sources : List of device current outputs the LED is connected to. The outputs are identified by the numbers that must be defined in the LED device binding documentation." I cannot find the max77693 dt documentation or even a dt file that defines what the led-sources could be defined as. In addition, per the common.txt, the led sources should define the current outputs the LED is connected to. Is one to assume that it is referring to the physical current pin connection or the devices internal current routing? When I first referenced this driver as template driver it was misleading to say fled1 and fled2 as I took it to mean, and I still do, that the driver supports 2 LED connections and not a single output pin with different internal current routing. Without the data sheet or the dt documentation it makes understanding a little difficult. I will fix it up how ever you would like it to be but I am thinking we need to fix up the max77693 as well so we can have 2 reference drivers. It is very difficult to derive the driver without a public version of the data sheet. Dan > >>>>> Maybe they don't want the strobe feature and just want LED on/off and switch between IR and White. >>>>> >>>>> The IR is driven via a different output pin. >>>> >>>> Correction. There is only one LED drive pin. The mode selected determines how the device will drive the >>>> LED. So you may have one device to drive a Torch and another device to drive the LED. >>> >>> But only one type of LED can be soldered on the board at a time, right? >>> If yes, then this is clearly a DT related configuration, and only >>> one child DT node should be defined for given board. >> >> But see above. There is not a clean way to expose the torch/ir and strobe separately. >> >> Dan >> >> >>> >>>> Strobe is available in either case but not always required if strobe is not desired by the customer hence >>>> why I have a separate DT node for it. >>>> >>>> Dan >>>> >>>>> >>>>> Dan >>>>> >>>>>> >>>>>> Best regards, >>>>>> Pavel >>>>>> >>>>> >>>>> >>>> >>>> >>> >> >> > -- ------------------ Dan Murphy ^ permalink raw reply [flat|nested] 33+ messages in thread
* Re: [PATCH v5 1/2] dt: bindings: lm3601x: Introduce the lm3601x driver 2018-05-14 20:07 ` Dan Murphy (?) @ 2018-05-14 20:48 ` Jacek Anaszewski -1 siblings, 0 replies; 33+ messages in thread From: Jacek Anaszewski @ 2018-05-14 20:48 UTC (permalink / raw) To: Dan Murphy, Pavel Machek Cc: robh+dt, mark.rutland, afd, devicetree, linux-kernel, linux-leds Dan, On 05/14/2018 10:07 PM, Dan Murphy wrote: > Jacek > > On 05/11/2018 03:27 PM, Jacek Anaszewski wrote: >> Dan, >> >> On 05/11/2018 02:12 PM, Dan Murphy wrote: >>> Jacek >>> >>> Thanks for the review >>> >>> On 05/10/2018 03:17 PM, Jacek Anaszewski wrote: >>>> Hi Dan, >>>> >>>> On 05/10/2018 09:10 PM, Dan Murphy wrote: >>>>> On 05/10/2018 02:06 PM, Dan Murphy wrote: >>>>>> Pavel >>>>>> >>>>>> On 05/10/2018 01:54 PM, Pavel Machek wrote: >>>>>>> Hi! >>>>>>> >>>>>>>> Introduce the device tree bindings for the lm3601x >>>>>>>> family of LED torch, flash and IR drivers. >>>>>>>> >>>>>>>> Signed-off-by: Dan Murphy <dmurphy@ti.com> >>>>>>> >>>>>>> Better, thanks. >>>>>>>> +++ b/Documentation/devicetree/bindings/leds/leds-lm3601x.txt >>>>>>>> @@ -0,0 +1,50 @@ >>>>>>>> +* Texas Instruments - lm3601x Single-LED Flash Driver >>>>>>> >>>>>>> Ok, so is it single-LED driver, or can it driver ir & white LEDs at >>>>>>> the same time? >>>>>> >>>>>> It is a single LED driver. It can drive a Torch white LED or IR LED indefinitely or if the driver >>>>>> is programmed to strobe mode the driver will drive the configured LED for the flash timeout specified. >>>>>> >>>>>> Basically a flash and a flash light. IR or White LED. >>>>>> >>>>>> >>>>>>> >>>>>>>> +Example: >>>>>>>> +led-controller@64 { >>>>>>>> + compatible = "ti,lm36010"; >>>>>>>> + #address-cells = <1>; >>>>>>>> + #size-cells = <0>; >>>>>>>> + reg = <0x64>; >>>>>>>> + >>>>>>>> + led@0 { >>>>>>>> + reg = <0>; >>>>>>>> + label = "white:torch"; >>>>>>>> + led-max-microamp = <10000>; >>>>>>>> + }; >>>>>>>> + >>>>>>>> + led@1 { >>>>>>>> + reg = <1>; >>>>>>>> + label = "white:flash"; >>>>>>>> + flash-max-microamp = <10000>; >>>>>>>> + flash-max-timeout-us = <800>; >>>>>>>> + }; >>>>>>> >>>>>>> Is this realistic config? I'd expect flash to use more power than >>>>>>> torch, and would expect longer timeout than 0.8msec. >>>>>> >>>>>> Timeout in the spreadsheet is ms I will update this example and code >>>>>> Current in the spreadsheet is mA I will update this example and code >>>>>> >>>>>>> >>>>>>> Also.. if this is physically one white LED, it should not be >>>>>>> spread over reg = <0> and reg = <1>... >>>>>> >>>>>> If the torch LED and strobe LED are the same LED how do I expose them both to the user. >>>>>> It is up to consumer to configure the required interfaces they want to expose to the filesystem. >>>> >>>> LED flash class interface is prepared for it. >>>> led_clasdev_flash_register() internally calls led_classdev_register(), >>>> so there is brightness file available for torch related operations. >>> >>> Yes the flash class may handle this and expose the brightness node but the HW has two different registers to set the >>> corresponding brightness so one set brightness node does not work. >>> There is no way to differentiate between the strobe brightness (register 4) and the torch brightness (register 3). >> >> Hmm? There is brightness file (with brightness_set{_blocking} op) from >> LED class and flash_brightness file (with flash_brightness_set op) from >> LED class flash. >> >> I've only now realized that you're not using flash_brightness_set op for >> the "strobe_node" in your driver. I assume that you overlooked it. >> Please don't introduce "strobe_brightness" naming convention - we >> already have "flash_brightness" for that. >> >>> When the enable register is written the driver will read the corresponding register and set >>> the current for the LED. >>> >>> This is why I separated out the strobe from the torch into 2 different LED nodes. >>> >>> I cannot seem to find the data sheet on line for the max77693 so I cannot verify that the device only has >>> a single brightness register for both torch and strobe. >> >> It wasn't openly available at least at the time when I had an access >> to it. >> >> But - please look at the functions max77693_set_torch_current() and >> max77693_set_flash_current(). The device has separate registers >> for torch and flash brightnesses. > > > I have looked at these functions and the parse dt function that dictates if there is 1 fled or 2 fled. > I am having trouble associating what these mean because the led-sources is being used to define how many LEDs. > > In the common.txt it says > "- led-sources : List of device current outputs the LED is connected to. The > outputs are identified by the numbers that must be defined > in the LED device binding documentation." > > I cannot find the max77693 dt documentation or even a dt file that defines what the led-sources could be defined as. > In addition, per the common.txt, the led sources should define the current outputs the LED is connected to. You can find more details in Documentation/devicetree/bindings/mfd/max77693.txt. Generally the led-sources property was introduced to solve this particular case when there are two IOUTS that can have connected two LEDs or a single LED which allows to feed it with greater current. > > Is one to assume that it is referring to the physical current pin connection or the devices internal current routing? > > When I first referenced this driver as template driver it was misleading to say fled1 and fled2 as I took it to mean, and I still do, that the > driver supports 2 LED connections and not a single output pin with different internal current routing. > > Without the data sheet or the dt documentation it makes understanding a little difficult. There are more straightforward LED flash class drivers, e.g. drivers/leds/leds-aat1290, you can refer to. > I will fix it up how ever you would like it to be but I am thinking we need to fix up the max77693 as well so we can have 2 reference > drivers. There is also drivers/leds/leds-as3645a.c and one greybus driver. So we have in fact four LED flash class drivers in the tree :-) > It is very difficult to derive the driver without a public version of the data sheet. I agree, but mfd documentation I pointed should be enough to increase your comprehension of the subject, I hope. If not, then we can extend it basing on your remarks and my memory. >>>>>> Maybe they don't want the strobe feature and just want LED on/off and switch between IR and White. >>>>>> >>>>>> The IR is driven via a different output pin. >>>>> >>>>> Correction. There is only one LED drive pin. The mode selected determines how the device will drive the >>>>> LED. So you may have one device to drive a Torch and another device to drive the LED. >>>> >>>> But only one type of LED can be soldered on the board at a time, right? >>>> If yes, then this is clearly a DT related configuration, and only >>>> one child DT node should be defined for given board. >>> >>> But see above. There is not a clean way to expose the torch/ir and strobe separately. >>> >>> Dan >>> >>> >>>> >>>>> Strobe is available in either case but not always required if strobe is not desired by the customer hence >>>>> why I have a separate DT node for it. >>>>> >>>>> Dan >>>>> >>>>>> >>>>>> Dan >>>>>> >>>>>>> >>>>>>> Best regards, >>>>>>> Pavel >>>>>>> >>>>>> >>>>>> >>>>> >>>>> >>>> >>> >>> >> > > -- Best regards, Jacek Anaszewski ^ permalink raw reply [flat|nested] 33+ messages in thread
* Re: [PATCH v5 1/2] dt: bindings: lm3601x: Introduce the lm3601x driver 2018-05-10 17:47 ` Dan Murphy ` (2 preceding siblings ...) (?) @ 2018-05-10 20:50 ` Jacek Anaszewski -1 siblings, 0 replies; 33+ messages in thread From: Jacek Anaszewski @ 2018-05-10 20:50 UTC (permalink / raw) To: Dan Murphy, robh+dt, mark.rutland, pavel, afd Cc: devicetree, linux-kernel, linux-leds On 05/10/2018 07:47 PM, Dan Murphy wrote: > Introduce the device tree bindings for the lm3601x > family of LED torch, flash and IR drivers. > > Signed-off-by: Dan Murphy <dmurphy@ti.com> > --- > > v5 - No changes - https://patchwork.kernel.org/patch/10391743/ > > v4 - Added " " around "=", changed strobe to flash on label, removed "support and > register" comment and change ir lable to ir:torch - See v2 patchworks for comments > v3 - Removed wildcard compatible - https://patchwork.kernel.org/patch/10386241/ > v2 - No changes - https://patchwork.kernel.org/patch/10384587/ > > .../devicetree/bindings/leds/leds-lm3601x.txt | 50 +++++++++++++++++++ > 1 file changed, 50 insertions(+) > create mode 100644 Documentation/devicetree/bindings/leds/leds-lm3601x.txt > > diff --git a/Documentation/devicetree/bindings/leds/leds-lm3601x.txt b/Documentation/devicetree/bindings/leds/leds-lm3601x.txt > new file mode 100644 > index 000000000000..697e5e3a1d4c > --- /dev/null > +++ b/Documentation/devicetree/bindings/leds/leds-lm3601x.txt > @@ -0,0 +1,50 @@ > +* Texas Instruments - lm3601x Single-LED Flash Driver > + > +The LM3601X are ultra-small LED flash drivers that > +provide a high level of adjustability. > + > +Required properties: > + - compatible : Can be one of the following > + "ti,lm36010" > + "ti,lm36011" > + - reg : I2C slave address > + - #address-cells : 1 > + - #size-cells : 0 > + > +Required child properties: > + - reg : 0 - Indicates a torch interface > + 1 - Indicates a flash interface > + 2 - Indicates an infrared interface You need here also: - flash-max-microamp : see Documentation/devicetree/bindin /leds/common.txt - flash-max-timeout-us : see Documentation/devicetree/bindings/leds/common.txt > + > +Optional child properties: > + - label : see Documentation/devicetree/bindings/leds/common.txt > + > +Example: > +led-controller@64 { > + compatible = "ti,lm36010"; > + #address-cells = <1>; > + #size-cells = <0>; > + reg = <0x64>; > + > + led@0 { > + reg = <0>; > + label = "white:torch"; > + led-max-microamp = <10000>; > + }; > + > + led@1 { > + reg = <1>; > + label = "white:flash"; > + flash-max-microamp = <10000>; > + flash-max-timeout-us = <800>; > + }; > + > + led@2 { > + reg = <2>; > + label = "ir:torch"; > + }; > +} > + > +For more product information please see the links below: > +http://www.ti.com/product/LM36010 > +http://www.ti.com/product/LM36011 > -- Best regards, Jacek Anaszewski ^ permalink raw reply [flat|nested] 33+ messages in thread
end of thread, other threads:[~2018-05-14 20:48 UTC | newest] Thread overview: 33+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2018-05-10 17:47 [PATCH v5 1/2] dt: bindings: lm3601x: Introduce the lm3601x driver Dan Murphy 2018-05-10 17:47 ` Dan Murphy 2018-05-10 17:47 ` [PATCH v5 2/2] leds: lm3601x: Introduce the lm3601x LED driver Dan Murphy 2018-05-10 17:47 ` Dan Murphy 2018-05-10 20:48 ` Jacek Anaszewski 2018-05-11 11:56 ` Dan Murphy 2018-05-11 11:56 ` Dan Murphy 2018-05-11 20:26 ` Jacek Anaszewski 2018-05-14 19:40 ` Dan Murphy 2018-05-14 19:40 ` Dan Murphy 2018-05-14 19:49 ` Andy Shevchenko 2018-05-14 20:05 ` Pavel Machek 2018-05-14 20:13 ` Dan Murphy 2018-05-14 20:13 ` Dan Murphy 2018-05-14 20:16 ` Pavel Machek 2018-05-14 20:31 ` Dan Murphy 2018-05-14 20:31 ` Dan Murphy 2018-05-14 20:05 ` Jacek Anaszewski 2018-05-14 20:09 ` Dan Murphy 2018-05-14 20:09 ` Dan Murphy 2018-05-10 18:54 ` [PATCH v5 1/2] dt: bindings: lm3601x: Introduce the lm3601x driver Pavel Machek 2018-05-10 19:06 ` Dan Murphy 2018-05-10 19:06 ` Dan Murphy 2018-05-10 19:10 ` Dan Murphy 2018-05-10 19:10 ` Dan Murphy 2018-05-10 20:17 ` Jacek Anaszewski 2018-05-11 12:12 ` Dan Murphy 2018-05-11 12:12 ` Dan Murphy 2018-05-11 20:27 ` Jacek Anaszewski 2018-05-14 20:07 ` Dan Murphy 2018-05-14 20:07 ` Dan Murphy 2018-05-14 20:48 ` Jacek Anaszewski 2018-05-10 20:50 ` Jacek Anaszewski
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.