* [PATCH v7 1/2] dt: bindings: lm3601x: Introduce the lm3601x driver @ 2018-05-21 18:09 Dan Murphy 2018-05-21 18:09 ` [PATCH v7 2/2] leds: lm3601x: Introduce the lm3601x LED driver Dan Murphy 0 siblings, 1 reply; 8+ messages in thread From: Dan Murphy @ 2018-05-21 18:09 UTC (permalink / raw) To: robh+dt, mark.rutland, jacek.anaszewski, andy.shevchenko Cc: devicetree, linux-kernel, linux-leds, Dan Murphy Introduce the device tree bindings for the lm3601x family of LED torch, flash and IR drivers. Reviewed-by: Rob Herring <robh@kernel.org> Signed-off-by: Dan Murphy <dmurphy@ti.com> --- v7 - Removed led-sources in favor of reg, fixed malformatted ranges - https://patchwork.kernel.org/patch/10401435/ v6 - Removed multiple led child nodes, fixed example to display micro ranges for corresponding child nodes and added led-sources to define the current driver - https://patchwork.kernel.org/patch/10392121/ 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 | 45 +++++++++++++++++++ 1 file changed, 45 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..a88b2c41e75b --- /dev/null +++ b/Documentation/devicetree/bindings/leds/leds-lm3601x.txt @@ -0,0 +1,45 @@ +* 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 IR mode + 1 - Indicates a Torch (white LED) mode + +Required properties for flash LED child nodes: + See Documentation/devicetree/bindings/leds/common.txt + - flash-max-microamp : Range from 11mA - 1.5A + - flash-max-timeout-us : Range from 40ms - 1600ms + - led-max-microamp : Range from 2.4mA - 376mA + +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 = <1>; + label = "white:torch"; + led-max-microamp = <376000>; + flash-max-microamp = <1500000>; + flash-max-timeout-us = <1600000>; + }; +} + +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] 8+ messages in thread
* [PATCH v7 2/2] leds: lm3601x: Introduce the lm3601x LED driver 2018-05-21 18:09 [PATCH v7 1/2] dt: bindings: lm3601x: Introduce the lm3601x driver Dan Murphy @ 2018-05-21 18:09 ` Dan Murphy 2018-05-21 20:44 ` Andy Shevchenko 2018-05-21 21:09 ` Jacek Anaszewski 0 siblings, 2 replies; 8+ messages in thread From: Dan Murphy @ 2018-05-21 18:09 UTC (permalink / raw) To: robh+dt, mark.rutland, jacek.anaszewski, andy.shevchenko 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> --- v7 - Numerous fixes to many to list. Highlights are moved from OF APIs to device_property APIs, updated LED registration, timeout to reg algorithim, and fixed label generation - https://patchwork.kernel.org/patch/10401437/ v6 - This driver has been heavily modified from v5. There is no longer reading of individual child nodes. There are too many changes to list here see - https://patchwork.kernel.org/patch/10392123/ 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 | 501 ++++++++++++++++++++++++++++++++++++ 3 files changed, 511 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..7c71cb11d474 --- /dev/null +++ b/drivers/leds/leds-lm3601x.c @@ -0,0 +1,501 @@ +// 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_IR 0x0 +#define LM3601X_LED_TORCH 0x1 + +/* 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_EDGE_TRIG BIT(3) +#define LM3601X_IVFM_EN BIT(4) + +#define LM36010_BOOST_LIMIT_28 BIT(5) +#define LM36010_BOOST_FREQ_4MHZ BIT(6) +#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_MAX_TORCH_I_UA 376000 +#define LM3601X_MIN_TORCH_I_UA 2400 +#define LM3601X_TORCH_REG_DIV 3000 + +#define LM3601X_MAX_STROBE_I_UA 1500000 +#define LM3601X_MIN_STROBE_I_UA 11000 +#define LM3601X_STROBE_REG_DIV 11800 + +#define LM3601X_TIMEOUT_MASK 0x1e +#define LM3601X_ENABLE_MASK 0x03 + +#define LM3601X_LOWER_STEP_US 40000 +#define LM3601X_UPPER_STEP_US 200000 +#define LM3601X_MIN_TIMEOUT_US 40000 +#define LM3601X_MAX_TIMEOUT_US 1600000 +#define LM3601X_TIMEOUT_XOVER 400000 + +enum lm3601x_type { + CHIP_LM36010 = 0, + CHIP_LM36011, +}; + +/** + * struct lm3601x_led - + * @fled_cdev: flash led class device pointer + * @client: Pointer to the I2C client + * @regmap: Devices register map + * @lock: Lock for reading/writing the device + * @led_name: LED label for the Torch or IR LED + * @strobe_timeout: the timeout for the strobe + * @last_flag: last known flags register value + * @torch_current_max: maximum current for the torch + * @strobe_current_max: maximum current for the strobe + * @max_strobe_timeout: maximum timeout for the strobe + * @led_mode: The mode to enable either IR or Torch + */ +struct lm3601x_led { + struct led_classdev_flash fled_cdev; + struct i2c_client *client; + struct regmap *regmap; + struct mutex lock; + + char led_name[LED_MAX_NAME_SIZE]; + + unsigned int strobe_timeout; + unsigned int last_flag; + + u32 torch_current_max; + u32 strobe_current_max; + u32 max_strobe_timeout; + + u32 led_mode; +}; + +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 = 0; + + if (flags_val & LM36010_OVP_FAULT) + led->last_flag |= LED_FAULT_OVER_VOLTAGE; + + if (flags_val & (LM3601X_THERM_SHUTDOWN | LM3601X_THERM_CURR)) + led->last_flag |= LED_FAULT_OVER_TEMPERATURE; + + if (flags_val & LM3601X_SHORT_FAULT) + led->last_flag |= LED_FAULT_SHORT_CIRCUIT; + + if (flags_val & LM36010_CURR_LIMIT) + led->last_flag |= LED_FAULT_OVER_CURRENT; + + if (flags_val & LM3601X_UVLO_FAULT) + led->last_flag |= LED_FAULT_UNDER_VOLTAGE; + + if (flags_val & LM3601X_IVFM_TRIP) + led->last_flag |= LED_FAULT_INPUT_VOLTAGE; + + if (flags_val & LM3601X_THERM_SHUTDOWN) + led->last_flag |= LED_FAULT_LED_OVER_TEMPERATURE; + + return led->last_flag; +} + +static int lm3601x_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, led_mode_val; + u8 brightness_val; + + mutex_lock(&led->lock); + + ret = lm3601x_read_faults(led); + if (ret < 0) + goto out; + + if (led->led_mode == LM3601X_LED_TORCH) + led_mode_val = LM3601X_MODE_TORCH; + else + led_mode_val = LM3601X_MODE_IR_DRV; + + if (brightness == LED_OFF) { + ret = regmap_update_bits(led->regmap, LM3601X_ENABLE_REG, + led_mode_val, LED_OFF); + goto out; + } + + if (brightness == LED_ON) + brightness_val = LED_ON; + else + brightness_val = brightness / LM3601X_TORCH_REG_DIV; + + 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_IR_DRV, + led_mode_val); +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 timeout_reg_val = 0; + int current_timeout; + int ret; + + mutex_lock(&led->lock); + + ret = regmap_read(led->regmap, LM3601X_CFG_REG, ¤t_timeout); + if (ret < 0) + goto out; + + if (led->strobe_timeout >= LM3601X_TIMEOUT_XOVER) + timeout_reg_val = led->strobe_timeout / LM3601X_UPPER_STEP_US + 0x07; + else + timeout_reg_val = led->strobe_timeout / LM3601X_LOWER_STEP_US - 0x01; + + 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_TORCH | LM3601X_MODE_IR_DRV, + 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_flash *fled_cdev, + u32 brightness) +{ + 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 / LM3601X_STROBE_REG_DIV; + + 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); + + 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_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 = { + .flash_brightness_set = lm3601x_strobe_brightness_set, + .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 *led_cdev; + struct led_flash_setting *setting; + + led->fled_cdev.ops = &strobe_ops; + + setting = &led->fled_cdev.timeout; + setting->min = LM3601X_MIN_TIMEOUT_US; + setting->max = led->max_strobe_timeout; + setting->step = LM3601X_LOWER_STEP_US; + setting->val = led->max_strobe_timeout; + + setting = &led->fled_cdev.brightness; + setting->min = LM3601X_MIN_STROBE_I_UA; + setting->max = LM3601X_MAX_STROBE_I_UA; + setting->step = LM3601X_TORCH_REG_DIV; + setting->val = led->strobe_current_max; + + led_cdev = &led->fled_cdev.led_cdev; + led_cdev->name = led->led_name; + led_cdev->brightness_set_blocking = lm3601x_brightness_set; + led_cdev->max_brightness = LM3601X_MAX_TORCH_I_UA; + led_cdev->flags |= LED_DEV_CAP_FLASH; + + return led_classdev_flash_register(&led->client->dev, &led->fled_cdev); +} + +static int lm3601x_parse_node(struct lm3601x_led *led, + struct device_node *node) +{ + struct fwnode_handle *child = NULL; + int ret = -ENODEV; + const char *name; + + child = device_get_next_child_node(&led->client->dev, child); + if (!child) { + dev_err(&led->client->dev, "No LED Child node\n"); + return ret; + } + + ret = fwnode_property_read_u32(child, "reg", &led->led_mode); + if (ret) { + dev_err(&led->client->dev, "reg DT property missing\n"); + goto out_err; + } + + if (led->led_mode > LM3601X_LED_TORCH || + led->led_mode < LM3601X_LED_IR) { + dev_warn(&led->client->dev, "Invalid led mode requested\n"); + ret = -EINVAL; + goto out_err; + } + + ret = fwnode_property_read_string(child, "label", &name); + if (ret) { + if (led->led_mode == LM3601X_LED_TORCH) + name = "torch"; + else + name = "infrared"; + } + + snprintf(led->led_name, sizeof(led->led_name), + "%s:%s", node->name, name); + + ret = fwnode_property_read_u32(child, "led-max-microamp", + &led->torch_current_max); + if (ret < 0) { + dev_warn(&led->client->dev, + "led-max-microamp DT property missing\n"); + goto out_err; + } + + ret = fwnode_property_read_u32(child, "flash-max-microamp", + &led->strobe_current_max); + if (ret < 0) { + dev_warn(&led->client->dev, + "flash-max-microamp DT property missing\n"); + goto out_err; + } + + ret = fwnode_property_read_u32(child, "flash-max-timeout-us", + &led->max_strobe_timeout); + if (ret < 0) { + dev_warn(&led->client->dev, + "flash-max-timeout-us DT property missing\n"); + goto out_err; + } + +out_err: + fwnode_handle_put(child); + 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(*led), GFP_KERNEL); + if (!led) + return -ENOMEM; + + led->client = client; + i2c_set_clientdata(client, led); + + err = lm3601x_parse_node(led, client->dev.of_node); + if (err < 0) + return -ENODEV; + + 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); + + 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] 8+ messages in thread
* Re: [PATCH v7 2/2] leds: lm3601x: Introduce the lm3601x LED driver 2018-05-21 18:09 ` [PATCH v7 2/2] leds: lm3601x: Introduce the lm3601x LED driver Dan Murphy @ 2018-05-21 20:44 ` Andy Shevchenko 2018-05-21 21:32 ` Dan Murphy 2018-05-21 21:09 ` Jacek Anaszewski 1 sibling, 1 reply; 8+ messages in thread From: Andy Shevchenko @ 2018-05-21 20:44 UTC (permalink / raw) To: Dan Murphy Cc: Rob Herring, Mark Rutland, Jacek Anaszewski, devicetree, Linux Kernel Mailing List, Linux LED Subsystem On Mon, May 21, 2018 at 9:09 PM, Dan Murphy <dmurphy@ti.com> 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 Thanks for an update. My comments below. > +config LEDS_LM3601X > + tristate "LED support for LM3601x Chips" > + depends on LEDS_CLASS && I2C && OF Now OF here is superfluous. > + depends on LEDS_CLASS_FLASH > + select REGMAP_I2C > + help > + This option enables support for the TI LM3601x family > + of flash, torch and indicator classes. > +#define LM3601X_TIMEOUT_MASK 0x1e > +#define LM3601X_ENABLE_MASK 0x03 I dunno if GENMASK() will be better here. > +#define LM3601X_LOWER_STEP_US 40000 > +#define LM3601X_UPPER_STEP_US 200000 > +#define LM3601X_MIN_TIMEOUT_US 40000 > +#define LM3601X_MAX_TIMEOUT_US 1600000 > +#define LM3601X_TIMEOUT_XOVER 400000 Missed unit? > + ret = regmap_update_bits(led->regmap, LM3601X_ENABLE_REG, > + LM3601X_MODE_TORCH | LM3601X_MODE_IR_DRV, > + led_mode_val); > + ret = regmap_update_bits(led->regmap, LM3601X_ENABLE_REG, > + LM3601X_MODE_TORCH | LM3601X_MODE_IR_DRV, > + LM3601X_MODE_STROBE); Perhaps #define ..._MODE_TORCH_WITH_IR (..._TOCRH | ..._IR_DRV) ? > +static int lm3601x_probe(struct i2c_client *client, > + const struct i2c_device_id *id) > +{ > + struct lm3601x_led *led; > + int err; > + err = lm3601x_parse_node(led, client->dev.of_node); > + if (err < 0) > + return -ENODEV; Shouldn't be return err; ? > + err = lm3601x_register_leds(led); > + > + return err; I will leave this to maintainers since you seems have strong objections against more or less standard pattern, i.e. return lm3601x_register_leds(); > +} -- With Best Regards, Andy Shevchenko ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH v7 2/2] leds: lm3601x: Introduce the lm3601x LED driver 2018-05-21 20:44 ` Andy Shevchenko @ 2018-05-21 21:32 ` Dan Murphy 0 siblings, 0 replies; 8+ messages in thread From: Dan Murphy @ 2018-05-21 21:32 UTC (permalink / raw) To: Andy Shevchenko Cc: Rob Herring, Mark Rutland, Jacek Anaszewski, devicetree, Linux Kernel Mailing List, Linux LED Subsystem Andy Thanks for the review. I was pretty sure I missed somethings. On 05/21/2018 03:44 PM, Andy Shevchenko wrote: > On Mon, May 21, 2018 at 9:09 PM, Dan Murphy <dmurphy@ti.com> 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 > > Thanks for an update. My comments below. > >> +config LEDS_LM3601X >> + tristate "LED support for LM3601x Chips" >> + depends on LEDS_CLASS && I2C && OF > > Now OF here is superfluous. This is the first one I missed. ACK > >> + depends on LEDS_CLASS_FLASH >> + select REGMAP_I2C >> + help >> + This option enables support for the TI LM3601x family >> + of flash, torch and indicator classes. > >> +#define LM3601X_TIMEOUT_MASK 0x1e >> +#define LM3601X_ENABLE_MASK 0x03 > > I dunno if GENMASK() will be better here. I could probably make it more readable as well. I am thinking #define LM3601X_ENABLE_MASK (LM3601X_MODE_TORCH | LM3601X_MODE_IR_DRV) > >> +#define LM3601X_LOWER_STEP_US 40000 >> +#define LM3601X_UPPER_STEP_US 200000 >> +#define LM3601X_MIN_TIMEOUT_US 40000 >> +#define LM3601X_MAX_TIMEOUT_US 1600000 > >> +#define LM3601X_TIMEOUT_XOVER 400000 > > Missed unit? Ack > >> + ret = regmap_update_bits(led->regmap, LM3601X_ENABLE_REG, >> + LM3601X_MODE_TORCH | LM3601X_MODE_IR_DRV, >> + led_mode_val); > >> + ret = regmap_update_bits(led->regmap, LM3601X_ENABLE_REG, >> + LM3601X_MODE_TORCH | LM3601X_MODE_IR_DRV, >> + LM3601X_MODE_STROBE); > > Perhaps > #define ..._MODE_TORCH_WITH_IR (..._TOCRH | ..._IR_DRV) > ? > This was defined already I just did not use the #define but I will probably replace this with the LM3601X_ENABLE_MASK #define >> +static int lm3601x_probe(struct i2c_client *client, >> + const struct i2c_device_id *id) >> +{ >> + struct lm3601x_led *led; >> + int err; > >> + err = lm3601x_parse_node(led, client->dev.of_node); >> + if (err < 0) > >> + return -ENODEV; > > Shouldn't be > > return err; Ack. But I might just get rid of err altogether when I make the change below to return the status of the register. > > ? > >> + err = lm3601x_register_leds(led); >> + >> + return err; > > I will leave this to maintainers since you seems have strong > objections against more or less standard pattern, i.e. > > return lm3601x_register_leds(); ACk. I was going to change that but I was concerned about loosing err. But I can change it. > >> +} > -- ------------------ Dan Murphy ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH v7 2/2] leds: lm3601x: Introduce the lm3601x LED driver 2018-05-21 18:09 ` [PATCH v7 2/2] leds: lm3601x: Introduce the lm3601x LED driver Dan Murphy 2018-05-21 20:44 ` Andy Shevchenko @ 2018-05-21 21:09 ` Jacek Anaszewski 2018-05-21 21:44 ` Dan Murphy 1 sibling, 1 reply; 8+ messages in thread From: Jacek Anaszewski @ 2018-05-21 21:09 UTC (permalink / raw) To: Dan Murphy, robh+dt, mark.rutland, andy.shevchenko Cc: devicetree, linux-kernel, linux-leds Hi Dan, Thank you for the update. On 05/21/2018 08:09 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> > --- > > v7 - Numerous fixes to many to list. Highlights are moved from OF APIs to device_property > APIs, updated LED registration, timeout to reg algorithim, and fixed label generation - > https://patchwork.kernel.org/patch/10401437/ > > v6 - This driver has been heavily modified from v5. There is no longer reading > of individual child nodes. There are too many changes to list here see - > https://patchwork.kernel.org/patch/10392123/ > 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 | 501 ++++++++++++++++++++++++++++++++++++ > 3 files changed, 511 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..7c71cb11d474 > --- /dev/null > +++ b/drivers/leds/leds-lm3601x.c > @@ -0,0 +1,501 @@ > +// 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_IR 0x0 > +#define LM3601X_LED_TORCH 0x1 > + > +/* 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_EDGE_TRIG BIT(3) > +#define LM3601X_IVFM_EN BIT(4) > + > +#define LM36010_BOOST_LIMIT_28 BIT(5) > +#define LM36010_BOOST_FREQ_4MHZ BIT(6) > +#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_MAX_TORCH_I_UA 376000 > +#define LM3601X_MIN_TORCH_I_UA 2400 > +#define LM3601X_TORCH_REG_DIV 3000 > + > +#define LM3601X_MAX_STROBE_I_UA 1500000 > +#define LM3601X_MIN_STROBE_I_UA 11000 > +#define LM3601X_STROBE_REG_DIV 11800 > + > +#define LM3601X_TIMEOUT_MASK 0x1e > +#define LM3601X_ENABLE_MASK 0x03 > + > +#define LM3601X_LOWER_STEP_US 40000 > +#define LM3601X_UPPER_STEP_US 200000 > +#define LM3601X_MIN_TIMEOUT_US 40000 > +#define LM3601X_MAX_TIMEOUT_US 1600000 > +#define LM3601X_TIMEOUT_XOVER 400000 > + > +enum lm3601x_type { > + CHIP_LM36010 = 0, > + CHIP_LM36011, > +}; > + > +/** > + * struct lm3601x_led - > + * @fled_cdev: flash led class device pointer s/led/LED/ > + * @client: Pointer to the I2C client > + * @regmap: Devices register map > + * @lock: Lock for reading/writing the device > + * @led_name: LED label for the Torch or IR LED > + * @strobe_timeout: the timeout for the strobe > + * @last_flag: last known flags register value > + * @torch_current_max: maximum current for the torch > + * @strobe_current_max: maximum current for the strobe > + * @max_strobe_timeout: maximum timeout for the strobe > + * @led_mode: The mode to enable either IR or Torch > + */ > +struct lm3601x_led { > + struct led_classdev_flash fled_cdev; > + struct i2c_client *client; > + struct regmap *regmap; > + struct mutex lock; > + > + char led_name[LED_MAX_NAME_SIZE]; > + > + unsigned int strobe_timeout; > + unsigned int last_flag; > + > + u32 torch_current_max; > + u32 strobe_current_max; > + u32 max_strobe_timeout; > + > + u32 led_mode; > +}; > + > +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 = 0; > + > + if (flags_val & LM36010_OVP_FAULT) > + led->last_flag |= LED_FAULT_OVER_VOLTAGE; > + > + if (flags_val & (LM3601X_THERM_SHUTDOWN | LM3601X_THERM_CURR)) > + led->last_flag |= LED_FAULT_OVER_TEMPERATURE; > + > + if (flags_val & LM3601X_SHORT_FAULT) > + led->last_flag |= LED_FAULT_SHORT_CIRCUIT; > + > + if (flags_val & LM36010_CURR_LIMIT) > + led->last_flag |= LED_FAULT_OVER_CURRENT; > + > + if (flags_val & LM3601X_UVLO_FAULT) > + led->last_flag |= LED_FAULT_UNDER_VOLTAGE; > + > + if (flags_val & LM3601X_IVFM_TRIP) > + led->last_flag |= LED_FAULT_INPUT_VOLTAGE; > + > + if (flags_val & LM3601X_THERM_SHUTDOWN) > + led->last_flag |= LED_FAULT_LED_OVER_TEMPERATURE; > + > + return led->last_flag; > +} > + > +static int lm3601x_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, led_mode_val; > + u8 brightness_val; > + > + mutex_lock(&led->lock); > + > + ret = lm3601x_read_faults(led); > + if (ret < 0) > + goto out; > + > + if (led->led_mode == LM3601X_LED_TORCH) > + led_mode_val = LM3601X_MODE_TORCH; > + else > + led_mode_val = LM3601X_MODE_IR_DRV; > + > + if (brightness == LED_OFF) { > + ret = regmap_update_bits(led->regmap, LM3601X_ENABLE_REG, > + led_mode_val, LED_OFF); > + goto out; > + } > + > + if (brightness == LED_ON) > + brightness_val = LED_ON; Use of LED_ON enum can be a bit misleading here, It was designed for LEDs that can be only turned on/off. You can use DIV_ROUND_UP to achieve the same in a more clear way. > + else > + brightness_val = brightness / LM3601X_TORCH_REG_DIV; > + > + 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_IR_DRV, > + led_mode_val); > +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 timeout_reg_val = 0; > + int current_timeout; > + int ret; > + > + mutex_lock(&led->lock); > + > + ret = regmap_read(led->regmap, LM3601X_CFG_REG, ¤t_timeout); > + if (ret < 0) > + goto out; > + > + if (led->strobe_timeout >= LM3601X_TIMEOUT_XOVER) > + timeout_reg_val = led->strobe_timeout / LM3601X_UPPER_STEP_US + 0x07; > + else > + timeout_reg_val = led->strobe_timeout / LM3601X_LOWER_STEP_US - 0x01; > + > + 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_TORCH | LM3601X_MODE_IR_DRV, > + 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_flash *fled_cdev, > + u32 brightness) > +{ > + 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; Ditto. > + else > + brightness_val = brightness / LM3601X_STROBE_REG_DIV; > + > + 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); > + > + 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_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 = { > + .flash_brightness_set = lm3601x_strobe_brightness_set, s/strobe/flash/ > + .strobe_set = lm3601x_strobe_set, > + .strobe_get = lm3601x_strobe_get, > + .timeout_set = lm3601x_strobe_timeout_set, Ditto. > + .fault_get = lm3601x_strobe_fault_get, Ditto. > +}; > + > +static int lm3601x_register_leds(struct lm3601x_led *led) > +{ > + struct led_classdev *led_cdev; > + struct led_flash_setting *setting; > + > + led->fled_cdev.ops = &strobe_ops; > + > + setting = &led->fled_cdev.timeout; > + setting->min = LM3601X_MIN_TIMEOUT_US; > + setting->max = led->max_strobe_timeout; > + setting->step = LM3601X_LOWER_STEP_US; > + setting->val = led->max_strobe_timeout; > + > + setting = &led->fled_cdev.brightness; > + setting->min = LM3601X_MIN_STROBE_I_UA; > + setting->max = LM3601X_MAX_STROBE_I_UA; > + setting->step = LM3601X_TORCH_REG_DIV; > + setting->val = led->strobe_current_max; > + > + led_cdev = &led->fled_cdev.led_cdev; > + led_cdev->name = led->led_name; > + led_cdev->brightness_set_blocking = lm3601x_brightness_set; > + led_cdev->max_brightness = LM3601X_MAX_TORCH_I_UA; LED class brightness is expressed in levels, so: led_cdev->max_brightness = DIV_ROUND_UP(LM3601X_MAX_TORCH_I_UA, LM3601X_TORCH_REG_DIV) > + led_cdev->flags |= LED_DEV_CAP_FLASH; > + > + return led_classdev_flash_register(&led->client->dev, &led->fled_cdev); > +} > + > +static int lm3601x_parse_node(struct lm3601x_led *led, > + struct device_node *node) > +{ > + struct fwnode_handle *child = NULL; > + int ret = -ENODEV; > + const char *name; > + > + child = device_get_next_child_node(&led->client->dev, child); > + if (!child) { > + dev_err(&led->client->dev, "No LED Child node\n"); > + return ret; > + } > + > + ret = fwnode_property_read_u32(child, "reg", &led->led_mode); > + if (ret) { > + dev_err(&led->client->dev, "reg DT property missing\n"); > + goto out_err; > + } > + > + if (led->led_mode > LM3601X_LED_TORCH || > + led->led_mode < LM3601X_LED_IR) { > + dev_warn(&led->client->dev, "Invalid led mode requested\n"); > + ret = -EINVAL; > + goto out_err; > + } > + > + ret = fwnode_property_read_string(child, "label", &name); > + if (ret) { > + if (led->led_mode == LM3601X_LED_TORCH) > + name = "torch"; > + else > + name = "infrared"; > + } > + > + snprintf(led->led_name, sizeof(led->led_name), > + "%s:%s", node->name, name); Reading once again my recent explanation regarding this I realized that I didn't provide clear conclusion, which is: we no longer use child node name for LED class device name if label is absent. (apart from that - you're using parent DT node now, i.e. led-controller). Please follow what was done for drivers/leds/leds-cr0014114.c. > + ret = fwnode_property_read_u32(child, "led-max-microamp", > + &led->torch_current_max); > + if (ret < 0) { > + dev_warn(&led->client->dev, > + "led-max-microamp DT property missing\n"); > + goto out_err; > + } > + > + ret = fwnode_property_read_u32(child, "flash-max-microamp", > + &led->strobe_current_max); > + if (ret < 0) { > + dev_warn(&led->client->dev, > + "flash-max-microamp DT property missing\n"); > + goto out_err; > + } > + > + ret = fwnode_property_read_u32(child, "flash-max-timeout-us", > + &led->max_strobe_timeout); > + if (ret < 0) { > + dev_warn(&led->client->dev, > + "flash-max-timeout-us DT property missing\n"); > + goto out_err; > + } > + > +out_err: > + fwnode_handle_put(child); > + 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(*led), GFP_KERNEL); > + if (!led) > + return -ENOMEM; > + > + led->client = client; > + i2c_set_clientdata(client, led); > + > + err = lm3601x_parse_node(led, client->dev.of_node); > + if (err < 0) > + return -ENODEV; > + > + 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); > + > + err = lm3601x_register_leds(led); return 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); > + mutex_destroy() is missing. > + 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] 8+ messages in thread
* Re: [PATCH v7 2/2] leds: lm3601x: Introduce the lm3601x LED driver 2018-05-21 21:09 ` Jacek Anaszewski @ 2018-05-21 21:44 ` Dan Murphy 2018-05-21 23:05 ` Andy Shevchenko 0 siblings, 1 reply; 8+ messages in thread From: Dan Murphy @ 2018-05-21 21:44 UTC (permalink / raw) To: Jacek Anaszewski, robh+dt, mark.rutland, andy.shevchenko Cc: devicetree, linux-kernel, linux-leds Jacek Thanks for the review On 05/21/2018 04:09 PM, Jacek Anaszewski wrote: > Hi Dan, > > Thank you for the update. > > On 05/21/2018 08:09 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> >> --- >> >> v7 - Numerous fixes to many to list. Highlights are moved from OF APIs to device_property >> APIs, updated LED registration, timeout to reg algorithim, and fixed label generation - >> https://patchwork.kernel.org/patch/10401437/ >> >> v6 - This driver has been heavily modified from v5. There is no longer reading >> of individual child nodes. There are too many changes to list here see - >> https://patchwork.kernel.org/patch/10392123/ >> 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 | 501 ++++++++++++++++++++++++++++++++++++ >> 3 files changed, 511 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..7c71cb11d474 >> --- /dev/null >> +++ b/drivers/leds/leds-lm3601x.c >> @@ -0,0 +1,501 @@ >> +// 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_IR 0x0 >> +#define LM3601X_LED_TORCH 0x1 >> + >> +/* 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_EDGE_TRIG BIT(3) >> +#define LM3601X_IVFM_EN BIT(4) >> + >> +#define LM36010_BOOST_LIMIT_28 BIT(5) >> +#define LM36010_BOOST_FREQ_4MHZ BIT(6) >> +#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_MAX_TORCH_I_UA 376000 >> +#define LM3601X_MIN_TORCH_I_UA 2400 >> +#define LM3601X_TORCH_REG_DIV 3000 >> + >> +#define LM3601X_MAX_STROBE_I_UA 1500000 >> +#define LM3601X_MIN_STROBE_I_UA 11000 >> +#define LM3601X_STROBE_REG_DIV 11800 >> + >> +#define LM3601X_TIMEOUT_MASK 0x1e >> +#define LM3601X_ENABLE_MASK 0x03 >> + >> +#define LM3601X_LOWER_STEP_US 40000 >> +#define LM3601X_UPPER_STEP_US 200000 >> +#define LM3601X_MIN_TIMEOUT_US 40000 >> +#define LM3601X_MAX_TIMEOUT_US 1600000 >> +#define LM3601X_TIMEOUT_XOVER 400000 >> + >> +enum lm3601x_type { >> + CHIP_LM36010 = 0, >> + CHIP_LM36011, >> +}; >> + >> +/** >> + * struct lm3601x_led - >> + * @fled_cdev: flash led class device pointer > > s/led/LED/ Ack > >> + * @client: Pointer to the I2C client >> + * @regmap: Devices register map >> + * @lock: Lock for reading/writing the device >> + * @led_name: LED label for the Torch or IR LED >> + * @strobe_timeout: the timeout for the strobe >> + * @last_flag: last known flags register value >> + * @torch_current_max: maximum current for the torch >> + * @strobe_current_max: maximum current for the strobe >> + * @max_strobe_timeout: maximum timeout for the strobe >> + * @led_mode: The mode to enable either IR or Torch >> + */ >> +struct lm3601x_led { >> + struct led_classdev_flash fled_cdev; >> + struct i2c_client *client; >> + struct regmap *regmap; >> + struct mutex lock; >> + >> + char led_name[LED_MAX_NAME_SIZE]; >> + >> + unsigned int strobe_timeout; >> + unsigned int last_flag; >> + >> + u32 torch_current_max; >> + u32 strobe_current_max; >> + u32 max_strobe_timeout; >> + >> + u32 led_mode; >> +}; >> + >> +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 = 0; >> + >> + if (flags_val & LM36010_OVP_FAULT) >> + led->last_flag |= LED_FAULT_OVER_VOLTAGE; >> + >> + if (flags_val & (LM3601X_THERM_SHUTDOWN | LM3601X_THERM_CURR)) >> + led->last_flag |= LED_FAULT_OVER_TEMPERATURE; >> + >> + if (flags_val & LM3601X_SHORT_FAULT) >> + led->last_flag |= LED_FAULT_SHORT_CIRCUIT; >> + >> + if (flags_val & LM36010_CURR_LIMIT) >> + led->last_flag |= LED_FAULT_OVER_CURRENT; >> + >> + if (flags_val & LM3601X_UVLO_FAULT) >> + led->last_flag |= LED_FAULT_UNDER_VOLTAGE; >> + >> + if (flags_val & LM3601X_IVFM_TRIP) >> + led->last_flag |= LED_FAULT_INPUT_VOLTAGE; >> + >> + if (flags_val & LM3601X_THERM_SHUTDOWN) >> + led->last_flag |= LED_FAULT_LED_OVER_TEMPERATURE; >> + >> + return led->last_flag; >> +} >> + >> +static int lm3601x_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, led_mode_val; >> + u8 brightness_val; >> + >> + mutex_lock(&led->lock); >> + >> + ret = lm3601x_read_faults(led); >> + if (ret < 0) >> + goto out; >> + >> + if (led->led_mode == LM3601X_LED_TORCH) >> + led_mode_val = LM3601X_MODE_TORCH; >> + else >> + led_mode_val = LM3601X_MODE_IR_DRV; >> + >> + if (brightness == LED_OFF) { >> + ret = regmap_update_bits(led->regmap, LM3601X_ENABLE_REG, >> + led_mode_val, LED_OFF); >> + goto out; >> + } >> + >> + if (brightness == LED_ON) >> + brightness_val = LED_ON; > > Use of LED_ON enum can be a bit misleading here, > It was designed for LEDs that can be only turned on/off. > > You can use DIV_ROUND_UP to achieve the same in a more clear way. ACK > >> + else >> + brightness_val = brightness / LM3601X_TORCH_REG_DIV; >> + >> + 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_IR_DRV, >> + led_mode_val); >> +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 timeout_reg_val = 0; >> + int current_timeout; >> + int ret; >> + >> + mutex_lock(&led->lock); >> + >> + ret = regmap_read(led->regmap, LM3601X_CFG_REG, ¤t_timeout); >> + if (ret < 0) >> + goto out; >> + >> + if (led->strobe_timeout >= LM3601X_TIMEOUT_XOVER) >> + timeout_reg_val = led->strobe_timeout / LM3601X_UPPER_STEP_US + 0x07; >> + else >> + timeout_reg_val = led->strobe_timeout / LM3601X_LOWER_STEP_US - 0x01; >> + >> + 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_TORCH | LM3601X_MODE_IR_DRV, >> + 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_flash *fled_cdev, >> + u32 brightness) >> +{ >> + 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; > > Ditto. ACK > >> + else >> + brightness_val = brightness / LM3601X_STROBE_REG_DIV; >> + >> + 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); >> + >> + 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_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 = { >> + .flash_brightness_set = lm3601x_strobe_brightness_set, > > s/strobe/flash/ ACK > >> + .strobe_set = lm3601x_strobe_set, >> + .strobe_get = lm3601x_strobe_get, >> + .timeout_set = lm3601x_strobe_timeout_set, > > Ditto. ACK > >> + .fault_get = lm3601x_strobe_fault_get, > > Ditto. ACK > >> +}; >> + >> +static int lm3601x_register_leds(struct lm3601x_led *led) >> +{ >> + struct led_classdev *led_cdev; >> + struct led_flash_setting *setting; >> + >> + led->fled_cdev.ops = &strobe_ops; >> + >> + setting = &led->fled_cdev.timeout; >> + setting->min = LM3601X_MIN_TIMEOUT_US; >> + setting->max = led->max_strobe_timeout; >> + setting->step = LM3601X_LOWER_STEP_US; >> + setting->val = led->max_strobe_timeout; >> + >> + setting = &led->fled_cdev.brightness; >> + setting->min = LM3601X_MIN_STROBE_I_UA; >> + setting->max = LM3601X_MAX_STROBE_I_UA; >> + setting->step = LM3601X_TORCH_REG_DIV; >> + setting->val = led->strobe_current_max; >> + >> + led_cdev = &led->fled_cdev.led_cdev; >> + led_cdev->name = led->led_name; >> + led_cdev->brightness_set_blocking = lm3601x_brightness_set; >> + led_cdev->max_brightness = LM3601X_MAX_TORCH_I_UA; > > LED class brightness is expressed in levels, so: > > led_cdev->max_brightness = DIV_ROUND_UP(LM3601X_MAX_TORCH_I_UA, LM3601X_TORCH_REG_DIV) ACK > >> + led_cdev->flags |= LED_DEV_CAP_FLASH; >> + >> + return led_classdev_flash_register(&led->client->dev, &led->fled_cdev); >> +} >> + >> +static int lm3601x_parse_node(struct lm3601x_led *led, >> + struct device_node *node) >> +{ >> + struct fwnode_handle *child = NULL; >> + int ret = -ENODEV; >> + const char *name; >> + >> + child = device_get_next_child_node(&led->client->dev, child); >> + if (!child) { >> + dev_err(&led->client->dev, "No LED Child node\n"); >> + return ret; >> + } >> + >> + ret = fwnode_property_read_u32(child, "reg", &led->led_mode); >> + if (ret) { >> + dev_err(&led->client->dev, "reg DT property missing\n"); >> + goto out_err; >> + } >> + >> + if (led->led_mode > LM3601X_LED_TORCH || >> + led->led_mode < LM3601X_LED_IR) { >> + dev_warn(&led->client->dev, "Invalid led mode requested\n"); >> + ret = -EINVAL; >> + goto out_err; >> + } >> + >> + ret = fwnode_property_read_string(child, "label", &name); >> + if (ret) { >> + if (led->led_mode == LM3601X_LED_TORCH) >> + name = "torch"; >> + else >> + name = "infrared"; >> + } >> + >> + snprintf(led->led_name, sizeof(led->led_name), >> + "%s:%s", node->name, name); > > Reading once again my recent explanation regarding this I realized > that I didn't provide clear conclusion, which is: we no longer > use child node name for LED class device name if label is absent. > (apart from that - you're using parent DT node now, i.e. > led-controller). > > Please follow what was done for drivers/leds/leds-cr0014114.c. Hmmm. If this is calling dev->of_node->name to store the name will this work in non-DT configurations? I have not dug to deeply into the fwnode code to find out how the nodes get populated. So my question may not even be valid. > >> + ret = fwnode_property_read_u32(child, "led-max-microamp", >> + &led->torch_current_max); >> + if (ret < 0) { >> + dev_warn(&led->client->dev, >> + "led-max-microamp DT property missing\n"); >> + goto out_err; >> + } >> + >> + ret = fwnode_property_read_u32(child, "flash-max-microamp", >> + &led->strobe_current_max); >> + if (ret < 0) { >> + dev_warn(&led->client->dev, >> + "flash-max-microamp DT property missing\n"); >> + goto out_err; >> + } >> + >> + ret = fwnode_property_read_u32(child, "flash-max-timeout-us", >> + &led->max_strobe_timeout); >> + if (ret < 0) { >> + dev_warn(&led->client->dev, >> + "flash-max-timeout-us DT property missing\n"); >> + goto out_err; >> + } >> + >> +out_err: >> + fwnode_handle_put(child); >> + 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(*led), GFP_KERNEL); >> + if (!led) >> + return -ENOMEM; >> + >> + led->client = client; >> + i2c_set_clientdata(client, led); >> + >> + err = lm3601x_parse_node(led, client->dev.of_node); >> + if (err < 0) >> + return -ENODEV; >> + >> + 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); >> + >> + err = lm3601x_register_leds(led); > > return lm3601x_register_leds(led) ACK. I will end up dumping the err variable altogether > >> + >> + 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); >> + > > mutex_destroy() is missing. > >> + 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] 8+ messages in thread
* Re: [PATCH v7 2/2] leds: lm3601x: Introduce the lm3601x LED driver 2018-05-21 21:44 ` Dan Murphy @ 2018-05-21 23:05 ` Andy Shevchenko 2018-05-22 12:17 ` Dan Murphy 0 siblings, 1 reply; 8+ messages in thread From: Andy Shevchenko @ 2018-05-21 23:05 UTC (permalink / raw) To: Dan Murphy Cc: Jacek Anaszewski, Rob Herring, Mark Rutland, devicetree, Linux Kernel Mailing List, Linux LED Subsystem On Tue, May 22, 2018 at 12:44 AM, Dan Murphy <dmurphy@ti.com> wrote: >>> + child = device_get_next_child_node(&led->client->dev, child); >>> + if (!child) { >>> + dev_err(&led->client->dev, "No LED Child node\n"); >>> + return ret; >>> + } >>> + >>> + ret = fwnode_property_read_u32(child, "reg", &led->led_mode); >>> + if (ret) { >>> + dev_err(&led->client->dev, "reg DT property missing\n"); >>> + goto out_err; >>> + } >>> + >>> + if (led->led_mode > LM3601X_LED_TORCH || >>> + led->led_mode < LM3601X_LED_IR) { >>> + dev_warn(&led->client->dev, "Invalid led mode requested\n"); >>> + ret = -EINVAL; >>> + goto out_err; >>> + } >>> + >>> + ret = fwnode_property_read_string(child, "label", &name); >>> + if (ret) { >>> + if (led->led_mode == LM3601X_LED_TORCH) >>> + name = "torch"; >>> + else >>> + name = "infrared"; >>> + } >>> + >>> + snprintf(led->led_name, sizeof(led->led_name), >>> + "%s:%s", node->name, name); >> >> Reading once again my recent explanation regarding this I realized >> that I didn't provide clear conclusion, which is: we no longer >> use child node name for LED class device name if label is absent. >> (apart from that - you're using parent DT node now, i.e. >> led-controller). >> >> Please follow what was done for drivers/leds/leds-cr0014114.c. > > Hmmm. If this is calling dev->of_node->name to store the name will this > work in non-DT configurations? I didn't found this kind of use in linux-next, perhaps I missed something? In the driver Jacek referred to I found, though, use of of_node, which at some point should be changed to fwnode. For now you can fill that if you want to using something like this (IIRC it should work): if (is_of_node(fwnode)) ...->of_node = to_of_node(...); > I have not dug to deeply into the fwnode code to find out how the nodes > get populated. So my question may not even be valid. -- With Best Regards, Andy Shevchenko ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH v7 2/2] leds: lm3601x: Introduce the lm3601x LED driver 2018-05-21 23:05 ` Andy Shevchenko @ 2018-05-22 12:17 ` Dan Murphy 0 siblings, 0 replies; 8+ messages in thread From: Dan Murphy @ 2018-05-22 12:17 UTC (permalink / raw) To: Andy Shevchenko Cc: Jacek Anaszewski, Rob Herring, Mark Rutland, devicetree, Linux Kernel Mailing List, Linux LED Subsystem Andy On 05/21/2018 06:05 PM, Andy Shevchenko wrote: > On Tue, May 22, 2018 at 12:44 AM, Dan Murphy <dmurphy@ti.com> wrote: > > >>>> + child = device_get_next_child_node(&led->client->dev, child); >>>> + if (!child) { >>>> + dev_err(&led->client->dev, "No LED Child node\n"); >>>> + return ret; >>>> + } >>>> + >>>> + ret = fwnode_property_read_u32(child, "reg", &led->led_mode); >>>> + if (ret) { >>>> + dev_err(&led->client->dev, "reg DT property missing\n"); >>>> + goto out_err; >>>> + } >>>> + >>>> + if (led->led_mode > LM3601X_LED_TORCH || >>>> + led->led_mode < LM3601X_LED_IR) { >>>> + dev_warn(&led->client->dev, "Invalid led mode requested\n"); >>>> + ret = -EINVAL; >>>> + goto out_err; >>>> + } >>>> + >>>> + ret = fwnode_property_read_string(child, "label", &name); >>>> + if (ret) { >>>> + if (led->led_mode == LM3601X_LED_TORCH) >>>> + name = "torch"; >>>> + else >>>> + name = "infrared"; >>>> + } >>>> + >>>> + snprintf(led->led_name, sizeof(led->led_name), >>>> + "%s:%s", node->name, name); >>> >>> Reading once again my recent explanation regarding this I realized >>> that I didn't provide clear conclusion, which is: we no longer >>> use child node name for LED class device name if label is absent. >>> (apart from that - you're using parent DT node now, i.e. >>> led-controller). >>> >>> Please follow what was done for drivers/leds/leds-cr0014114.c. >> >> Hmmm. If this is calling dev->of_node->name to store the name will this >> work in non-DT configurations? > > I didn't found this kind of use in linux-next, perhaps I missed something? > Linux next has a different version then what I found doing a search for the driver. I will change the code to match what is in linux-next. Dan > In the driver Jacek referred to I found, though, use of of_node, which > at some point should be changed to fwnode. > > For now you can fill that if you want to using something like this > (IIRC it should work): > > if (is_of_node(fwnode)) > ...->of_node = to_of_node(...); > >> I have not dug to deeply into the fwnode code to find out how the nodes >> get populated. So my question may not even be valid. > -- ------------------ Dan Murphy ^ permalink raw reply [flat|nested] 8+ messages in thread
end of thread, other threads:[~2018-05-22 12:17 UTC | newest] Thread overview: 8+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2018-05-21 18:09 [PATCH v7 1/2] dt: bindings: lm3601x: Introduce the lm3601x driver Dan Murphy 2018-05-21 18:09 ` [PATCH v7 2/2] leds: lm3601x: Introduce the lm3601x LED driver Dan Murphy 2018-05-21 20:44 ` Andy Shevchenko 2018-05-21 21:32 ` Dan Murphy 2018-05-21 21:09 ` Jacek Anaszewski 2018-05-21 21:44 ` Dan Murphy 2018-05-21 23:05 ` Andy Shevchenko 2018-05-22 12:17 ` Dan Murphy
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).