| Message ID | 20180515154352.20263-1-dmurphy@ti.com |
|---|---|
| State | Not Applicable, archived |
| Headers | show |
| Series | [v6,1/2] dt: bindings: lm3601x: Introduce the lm3601x driver | expand |
Hi Dan, Thanks for the update. On 05/15/2018 05:43 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> > --- > > 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 | 47 +++++++++++++++++++ > 1 file changed, 47 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..27930a89e9a5 > --- /dev/null > +++ b/Documentation/devicetree/bindings/leds/leds-lm3601x.txt > @@ -0,0 +1,47 @@ > +* 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 > + - led-sources: 0 - Indicates a IR mode > + 1 - Indicates a Torch (white LED) mode You don't need led-sources at all. Please use reg as in the previous version. led-sources is useful for describing more complex arrangements. Here reg will do. > + > +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 Please give also a step in the format like below (taken from max77693): Valid values: 62500 - 1000000, step by 62500 (rounded down) > + > +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 = <376000>; > + flash-max-microamp = <1500000>; > + flash-max-timeout-us = <1600000>; > + led-sources = <1>; > + }; > +} > + > +For more product information please see the links below: > +http://www.ti.com/product/LM36010 > +http://www.ti.com/product/LM36011 >
Dan, Thanks for the update. Please refer to my comments below. On 05/15/2018 05:43 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> > --- > > 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 | 595 ++++++++++++++++++++++++++++++++++++ > 3 files changed, 605 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..fa87da5d5159 > --- /dev/null > +++ b/drivers/leds/leds-lm3601x.c > @@ -0,0 +1,595 @@ > +// 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_IR 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_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 > + * @led_node: DT device node for the led > + * @cdev_torch: led class device pointer for the torch > + * @cdev_ir: led class device pointer for infrared > + * @fled_cdev: flash led class device pointer > + * @led_name: LED label for the Torch or IR LED > + * @strobe: LED label for the strobe > + * @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 > + * @led_mode: The mode to enable either IR or Torch > + */ > +struct lm3601x_led { > + struct mutex lock; > + struct regmap *regmap; > + struct i2c_client *client; > + > + struct device_node *led_node; > + > + struct led_classdev cdev_torch; > + struct led_classdev cdev_ir; > + > + struct led_classdev_flash fled_cdev; > + > + char led_name[LED_MAX_NAME_SIZE]; > + char strobe[LED_MAX_NAME_SIZE]; > + > + unsigned int last_flag; > + unsigned int strobe_timeout; > + > + u32 torch_current_max; > + u32 strobe_current_max; > + u32 max_strobe_timeout; > + > + int led_mode; > +}; > + > +static const struct lm3601x_max_timeouts strobe_timeouts[] = { > + { 40000, 0x00 }, > + { 80000, 0x01 }, > + { 120000, 0x02 }, > + { 160000, 0x03 }, > + { 200000, 0x04 }, > + { 240000, 0x05 }, > + { 280000, 0x06 }, > + { 320000, 0x07 }, > + { 360000, 0x08 }, > + { 400000, 0x09 }, > + { 600000, 0x0a }, > + { 800000, 0x0b }, > + { 1000000, 0x0c }, > + { 1200000, 0x0d }, > + { 1400000, 0x0e }, > + { 1600000, 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 = 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 lm3601x_led *led = > + container_of(cdev, struct lm3601x_led, cdev_torch); > + u8 brightness_val; > + int ret, led_mode_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/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, > + led_mode_val, > + 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 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); > + > + 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 = { > + .strobe_set = lm3601x_strobe_set, > + .strobe_get = lm3601x_strobe_get, > + .timeout_set = lm3601x_strobe_timeout_set, > + .fault_get = lm3601x_strobe_fault_get, You're still missing flash_brightness_set here. > +}; > + > +static int lm3601x_register_leds(struct lm3601x_led *led) > +{ > + struct led_classdev_flash *fled_cdev; > + struct led_classdev *led_cdev; > + int err = -ENODEV; > + > + led->cdev_torch.name = led->led_name; > + led->cdev_torch.max_brightness = LED_FULL; This should be led->torch_current_max converted to brightness levels. Please compare how leds-max77693 and leds-as3645a do that. > + led->cdev_torch.brightness_set_blocking = lm3601x_brightness_set; > + err = devm_led_classdev_register(&led->client->dev, > + &led->cdev_torch); > + if (err < 0) > + return err; You shouldn't register two LED class devices for a single DT child node, and honestly I don't know why you would have to do it. > + > + 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_FUL > + 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); Please look into the implementation of led_classdev_flash_register() and compare other LED flash class drivers. What prevents you from applying the same approach? To put it concisely: LED flash class wrapper was designed to allow for operating on a single LED in two modes: torch and flash. Torch brightness is governed by brightness file and flash brightness is governed by flash_brightness file. The difference between the two modes is that torch mode is activated by writing value greater than 0 to the brightness file and flash mode is activated by writing 1 to the strobe_set file. In other words flash_brightness overwrites the (torch) brightness for the time equal to the flash_timeout or until 0 is written to the strobe_set file. > + 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; > + char *mode_name; > + int ret = -ENODEV; > + > + for_each_available_child_of_node(node, child_node) { > + led->led_node = of_node_get(child_node); > + if (!led->led_node) { > + dev_err(&led->client->dev, > + "No LED Child node\n"); > + > + goto out_err; > + } > + > + ret = of_property_read_u32(led->led_node, "led-sources", > + &led->led_mode); > + if (ret) { > + dev_err(&led->client->dev, > + "led-sources 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"); > + > + goto out_err; > + > + } > + } > + > + if (led->led_mode == LM3601X_LED_TORCH) { > + ret = of_property_read_string(led->led_node, "label", &name); > + if (!ret) > + snprintf(led->led_name, sizeof(led->led_name), > + "%s:%s", led->led_node->name, name); First segment of a LED class device name should be "devicename". > + else > + snprintf(led->led_name, sizeof(led->led_name), > + "%s:torch", led->led_node->name); > + > + ret = of_property_read_u32(led->led_node, "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; > + } > + > + mode_name = "torch"; > + > + } else if (led->led_mode == LM3601X_LED_IR) { > + ret = of_property_read_string(led->led_node, "label", &name); > + if (!ret) > + snprintf(led->led_name, sizeof(led->led_name), > + "%s:%s", led->led_node->name, name); > + else > + snprintf(led->led_name, sizeof(led->led_name), > + "%s::infrared", led->led_node->name); Ditto. > + > + mode_name = "ir"; > + > + } else { > + dev_warn(&led->client->dev, > + "No LED mode is selected exiting probe\n"); > + > + goto out_err; > + } > + > + /* Flash mode is available in IR or Torch mode so read the DT */ > + snprintf(led->strobe, sizeof(led->strobe), > + "%s:%s:strobe", led->led_node->name, mode_name); strobe is not a proper LED function name (it is not a noun). Please use "flash" instead. > + > + ret = of_property_read_u32(led->led_node, > + "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 = of_property_read_u32(led->led_node, > + "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; > + } > + > + lm3601x_init_flash_timeout(led); > + > +out_err: > + of_node_put(led->led_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); You need also mutex_destroy(&led->lock) here. > + > + 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"); >
Jacek On 05/15/2018 04:13 PM, Jacek Anaszewski wrote: > Hi Dan, > > Thanks for the update. > > On 05/15/2018 05:43 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> >> --- >> >> 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 | 47 +++++++++++++++++++ >> 1 file changed, 47 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..27930a89e9a5 >> --- /dev/null >> +++ b/Documentation/devicetree/bindings/leds/leds-lm3601x.txt >> @@ -0,0 +1,47 @@ >> +* 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 >> + - led-sources: 0 - Indicates a IR mode >> + 1 - Indicates a Torch (white LED) mode > > You don't need led-sources at all. Please use reg as in > the previous version. led-sources is useful for describing > more complex arrangements. Here reg will do. OK. Thought we would keep consistent with the max IC. > >> + >> +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 > > Please give also a step in the format like below > (taken from max77693): > > Valid values: 62500 - 1000000, step by 62500 (rounded down) I would do this but the step is not linear. The step is 40ms from 40 -> 400. after that the step goes to 200ms from 400->1600. same with the current ranges. Dan > > >> + >> +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 = <376000>; >> + flash-max-microamp = <1500000>; >> + flash-max-timeout-us = <1600000>; >> + led-sources = <1>; >> + }; >> +} >> + >> +For more product information please see the links below: >> +http://www.ti.com/product/LM36010 >> +http://www.ti.com/product/LM36011 >> >
Jacek On 05/15/2018 04:24 PM, Jacek Anaszewski wrote: > Dan, > > Thanks for the update. Please refer to my comments below. > > On 05/15/2018 05:43 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> >> --- >> >> 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 | 595 ++++++++++++++++++++++++++++++++++++ >> 3 files changed, 605 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..fa87da5d5159 >> --- /dev/null >> +++ b/drivers/leds/leds-lm3601x.c >> @@ -0,0 +1,595 @@ >> +// 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_IR 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_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 >> + * @led_node: DT device node for the led >> + * @cdev_torch: led class device pointer for the torch >> + * @cdev_ir: led class device pointer for infrared >> + * @fled_cdev: flash led class device pointer >> + * @led_name: LED label for the Torch or IR LED >> + * @strobe: LED label for the strobe >> + * @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 >> + * @led_mode: The mode to enable either IR or Torch >> + */ >> +struct lm3601x_led { >> + struct mutex lock; >> + struct regmap *regmap; >> + struct i2c_client *client; >> + >> + struct device_node *led_node; >> + >> + struct led_classdev cdev_torch; >> + struct led_classdev cdev_ir; >> + >> + struct led_classdev_flash fled_cdev; >> + >> + char led_name[LED_MAX_NAME_SIZE]; >> + char strobe[LED_MAX_NAME_SIZE]; >> + >> + unsigned int last_flag; >> + unsigned int strobe_timeout; >> + >> + u32 torch_current_max; >> + u32 strobe_current_max; >> + u32 max_strobe_timeout; >> + >> + int led_mode; >> +}; >> + >> +static const struct lm3601x_max_timeouts strobe_timeouts[] = { >> + { 40000, 0x00 }, >> + { 80000, 0x01 }, >> + { 120000, 0x02 }, >> + { 160000, 0x03 }, >> + { 200000, 0x04 }, >> + { 240000, 0x05 }, >> + { 280000, 0x06 }, >> + { 320000, 0x07 }, >> + { 360000, 0x08 }, >> + { 400000, 0x09 }, >> + { 600000, 0x0a }, >> + { 800000, 0x0b }, >> + { 1000000, 0x0c }, >> + { 1200000, 0x0d }, >> + { 1400000, 0x0e }, >> + { 1600000, 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 = 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 lm3601x_led *led = >> + container_of(cdev, struct lm3601x_led, cdev_torch); >> + u8 brightness_val; >> + int ret, led_mode_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/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, >> + led_mode_val, >> + 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 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); >> + >> + 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 = { >> + .strobe_set = lm3601x_strobe_set, >> + .strobe_get = lm3601x_strobe_get, >> + .timeout_set = lm3601x_strobe_timeout_set, >> + .fault_get = lm3601x_strobe_fault_get, > > You're still missing flash_brightness_set here. OK not sure how this is working then. > >> +}; >> + >> +static int lm3601x_register_leds(struct lm3601x_led *led) >> +{ >> + struct led_classdev_flash *fled_cdev; >> + struct led_classdev *led_cdev; >> + int err = -ENODEV; >> + >> + led->cdev_torch.name = led->led_name; >> + led->cdev_torch.max_brightness = LED_FULL; > > This should be led->torch_current_max converted to brightness levels. > Please compare how leds-max77693 and leds-as3645a do that. I will look into it. > >> + led->cdev_torch.brightness_set_blocking = lm3601x_brightness_set; >> + err = devm_led_classdev_register(&led->client->dev, >> + &led->cdev_torch); >> + if (err < 0) >> + return err; > > You shouldn't register two LED class devices for a single DT child > node, and honestly I don't know why you would have to do it. See below > >> + >> + 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_FUL >> + 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); > > Please look into the implementation of led_classdev_flash_register() > and compare other LED flash class drivers. What prevents you from > applying the same approach? I did actually follow the same approach here as the as3645a driver. I used this driver since I have a data sheet and can read the code. This is a copy, paste, slight modify from that as3645a driver as3645a_led_class_setup function. I used the devm register function above as a variation. I will have to dig further into it. > > To put it concisely: LED flash class wrapper was designed to allow > for operating on a single LED in two modes: torch and flash. > Torch brightness is governed by brightness file and flash brightness > is governed by flash_brightness file. The difference between the two > modes is that torch mode is activated by writing value greater than 0 > to the brightness file and flash mode is activated by writing 1 to > the strobe_set file. > > In other words flash_brightness overwrites the (torch) brightness > for the time equal to the flash_timeout or until 0 is written > to the strobe_set file. I will look at it again > >> + 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; >> + char *mode_name; >> + int ret = -ENODEV; >> + >> + for_each_available_child_of_node(node, child_node) { >> + led->led_node = of_node_get(child_node); >> + if (!led->led_node) { >> + dev_err(&led->client->dev, >> + "No LED Child node\n"); >> + >> + goto out_err; >> + } >> + >> + ret = of_property_read_u32(led->led_node, "led-sources", >> + &led->led_mode); >> + if (ret) { >> + dev_err(&led->client->dev, >> + "led-sources 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"); >> + >> + goto out_err; >> + >> + } >> + } >> + >> + if (led->led_mode == LM3601X_LED_TORCH) { >> + ret = of_property_read_string(led->led_node, "label", &name); >> + if (!ret) >> + snprintf(led->led_name, sizeof(led->led_name), >> + "%s:%s", led->led_node->name, name); > > First segment of a LED class device name should be "devicename". Hmm. I remember this conversation from my other drivers. I just picked the wrong dt node name. I will fix it. > >> + else >> + snprintf(led->led_name, sizeof(led->led_name), >> + "%s:torch", led->led_node->name); >> + >> + ret = of_property_read_u32(led->led_node, "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; >> + } >> + >> + mode_name = "torch"; >> + >> + } else if (led->led_mode == LM3601X_LED_IR) { >> + ret = of_property_read_string(led->led_node, "label", &name); >> + if (!ret) >> + snprintf(led->led_name, sizeof(led->led_name), >> + "%s:%s", led->led_node->name, name); >> + else >> + snprintf(led->led_name, sizeof(led->led_name), >> + "%s::infrared", led->led_node->name); > > Ditto. Ack > >> + >> + mode_name = "ir"; >> + >> + } else { >> + dev_warn(&led->client->dev, >> + "No LED mode is selected exiting probe\n"); >> + >> + goto out_err; >> + } >> + >> + /* Flash mode is available in IR or Torch mode so read the DT */ >> + snprintf(led->strobe, sizeof(led->strobe), >> + "%s:%s:strobe", led->led_node->name, mode_name); > > strobe is not a proper LED function name (it is not a noun). > Please use "flash" instead. Strobe can be used as a noun to define an electronic camera flash. But this is really a US English definition. I can change it to flash > >> + >> + ret = of_property_read_u32(led->led_node, >> + "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 = of_property_read_u32(led->led_node, >> + "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; >> + } >> + >> + lm3601x_init_flash_timeout(led); >> + >> +out_err: >> + of_node_put(led->led_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); > > You need also mutex_destroy(&led->lock) here. OK. > >> + >> + 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"); >> >
On Tue, May 15, 2018 at 6:43 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 > + depends on LEDS_CLASS && I2C && OF What is OF specific in this driver? > +#define LM3601X_STRB_LVL_TRIG ~BIT(3) > +#define LM36010_BOOST_LIMIT_19 ~BIT(5) > +#define LM36010_BOOST_FREQ_2MHZ ~BIT(6) > +#define LM36010_BOOST_MODE_NORM ~BIT(7) These looks rather strange. Shouldn't be just 0:s? > +struct lm3601x_led { > + struct mutex lock; > + struct regmap *regmap; > + struct i2c_client *client; > + struct device_node *led_node; Why do you need this? > +}; > +static const struct lm3601x_max_timeouts strobe_timeouts[] = { > + { 40000, 0x00 }, > + { 80000, 0x01 }, > + { 120000, 0x02 }, > + { 160000, 0x03 }, > + { 200000, 0x04 }, > + { 240000, 0x05 }, > + { 280000, 0x06 }, > + { 320000, 0x07 }, > + { 360000, 0x08 }, > + { 400000, 0x09 }, > + { 600000, 0x0a }, > + { 800000, 0x0b }, > + { 1000000, 0x0c }, > + { 1200000, 0x0d }, > + { 1400000, 0x0e }, > + { 1600000, 0x0f }, Huh?! strobe_timeout = (x + 1) * 40 * MSECS_IN_SEC; > +}; > + 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; Better to put lines here in reversed xmas tree order (longest first). > + reg_count = ARRAY_SIZE(strobe_timeouts); > + for (i = 0; i < reg_count; i++) { > + if (led->strobe_timeout > strobe_timeouts[i].timeout) > + continue; binary search? Check lib/bsearch.c (IIRC). But! See above the formula. > + brightness_val = (brightness/2); Spaces. > +static int lm3601x_register_leds(struct lm3601x_led *led) > +{ > + int err = -ENODEV; Useless assignment. > + err = led_classdev_flash_register(&led->client->dev, > + fled_cdev); > + > + return err; This is return led_...(); > +} > + ret = of_property_read_string(led->led_node, "label", &name); device_property_...(); > + if (!ret) if (ret) sounds more natural. And better just to split > + snprintf(led->led_name, sizeof(led->led_name), > + "%s:%s", led->led_node->name, name); > + else > + snprintf(led->led_name, sizeof(led->led_name), > + "%s:torch", led->led_node->name); const char *tmp; ret = device_property_read_...(&tmp); if (ret) tmp = ... sprintf(...); > + led = devm_kzalloc(&client->dev, > + sizeof(struct lm3601x_led), GFP_KERNEL); sizeof(*led) and one line in the result > +static const struct i2c_device_id lm3601x_id[] = { > + { "LM36010", CHIP_LM36010 }, > + { "LM36011", CHIP_LM36011 }, > + { }, Terminators better w/o comma. > +}; > +MODULE_DEVICE_TABLE(i2c, lm3601x_id); > + > +static const struct of_device_id of_lm3601x_leds_match[] = { > + { .compatible = "ti,lm36010", }, > + { .compatible = "ti,lm36011", }, > + {}, Ditto. > +}; > +MODULE_DEVICE_TABLE(of, of_lm3601x_leds_match);
Andy Thanks for the review On 05/15/2018 04:56 PM, Andy Shevchenko wrote: > On Tue, May 15, 2018 at 6:43 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 > >> + depends on LEDS_CLASS && I2C && OF > > What is OF specific in this driver? as3645a_led_class_setup has a "of" dependency > > >> +#define LM3601X_STRB_LVL_TRIG ~BIT(3) > >> +#define LM36010_BOOST_LIMIT_19 ~BIT(5) > >> +#define LM36010_BOOST_FREQ_2MHZ ~BIT(6) > >> +#define LM36010_BOOST_MODE_NORM ~BIT(7) > > These looks rather strange. Shouldn't be just 0:s? I don't actually use these so I can just remove them > >> +struct lm3601x_led { >> + struct mutex lock; >> + struct regmap *regmap; >> + struct i2c_client *client; > >> + struct device_node *led_node; > > Why do you need this? I don't but I can store it locally. I was using the nodes in the register functions in previous versions. > >> +}; > >> +static const struct lm3601x_max_timeouts strobe_timeouts[] = { >> + { 40000, 0x00 }, >> + { 80000, 0x01 }, >> + { 120000, 0x02 }, >> + { 160000, 0x03 }, >> + { 200000, 0x04 }, >> + { 240000, 0x05 }, >> + { 280000, 0x06 }, >> + { 320000, 0x07 }, >> + { 360000, 0x08 }, >> + { 400000, 0x09 }, >> + { 600000, 0x0a }, >> + { 800000, 0x0b }, >> + { 1000000, 0x0c }, >> + { 1200000, 0x0d }, >> + { 1400000, 0x0e }, >> + { 1600000, 0x0f }, > > Huh?! Please give comments that actually mean something other wise I will opt to ignore them. > > strobe_timeout = (x + 1) * 40 * MSECS_IN_SEC; Not sure what equation you are trying to point out here. But if you are trying to apply a timeout step you cannot do this with this part. As pointed out in the DT doc the timeout step is not linear. > >> +}; > >> + 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; > > Better to put lines here in reversed xmas tree order (longest first). I can do that, again this is a preference. > >> + reg_count = ARRAY_SIZE(strobe_timeouts); >> + for (i = 0; i < reg_count; i++) { >> + if (led->strobe_timeout > strobe_timeouts[i].timeout) >> + continue; > > binary search? > > Check lib/bsearch.c (IIRC). > > But! See above the formula. > >> + brightness_val = (brightness/2); > > Spaces. Not sure what this means checkpatch was clean > >> +static int lm3601x_register_leds(struct lm3601x_led *led) >> +{ > > >> + int err = -ENODEV; > > Useless assignment. This is an artifact from prior versions I can remove it > >> + err = led_classdev_flash_register(&led->client->dev, >> + fled_cdev); >> + >> + return err; > > This is return led_...(); That is a preference. It does not have to be that way. > >> +} > >> + ret = of_property_read_string(led->led_node, "label", &name); > > device_property_...(); It can be if the maintainer is requesting this. Is the trend to move to these functions? Most drivers use the "of" calls. > >> + if (!ret) > > if (ret) sounds more natural. And better just to split > >> + snprintf(led->led_name, sizeof(led->led_name), >> + "%s:%s", led->led_node->name, name); >> + else >> + snprintf(led->led_name, sizeof(led->led_name), >> + "%s:torch", led->led_node->name); > > const char *tmp; > > ret = device_property_read_...(&tmp); > if (ret) > tmp = ... > sprintf(...); > >> + led = devm_kzalloc(&client->dev, >> + sizeof(struct lm3601x_led), GFP_KERNEL); > > sizeof(*led) and one line in the result > >> +static const struct i2c_device_id lm3601x_id[] = { >> + { "LM36010", CHIP_LM36010 }, >> + { "LM36011", CHIP_LM36011 }, >> + { }, > > Terminators better w/o comma. Looking at other drivers adding comma's on the sentinel is accepted. See the as3645a driver > >> +}; >> +MODULE_DEVICE_TABLE(i2c, lm3601x_id); >> + >> +static const struct of_device_id of_lm3601x_leds_match[] = { >> + { .compatible = "ti,lm36010", }, >> + { .compatible = "ti,lm36011", }, >> + {}, > > Ditto. See above > >> +}; >> +MODULE_DEVICE_TABLE(of, of_lm3601x_leds_match); > > > >
On Wed, May 16, 2018 at 1:08 AM, Dan Murphy <dmurphy@ti.com> wrote: > On 05/15/2018 04:56 PM, Andy Shevchenko wrote: >> On Tue, May 15, 2018 at 6:43 PM, Dan Murphy <dmurphy@ti.com> wrote: >>> + depends on LEDS_CLASS && I2C && OF >> >> What is OF specific in this driver? > > as3645a_led_class_setup has a "of" dependency So what? Is it called from this driver or? >>> +static const struct lm3601x_max_timeouts strobe_timeouts[] = { >>> + { 40000, 0x00 }, >>> + { 80000, 0x01 }, >>> + { 120000, 0x02 }, >>> + { 160000, 0x03 }, >>> + { 200000, 0x04 }, >>> + { 240000, 0x05 }, >>> + { 280000, 0x06 }, >>> + { 320000, 0x07 }, >>> + { 360000, 0x08 }, >>> + { 400000, 0x09 }, >>> + { 600000, 0x0a }, >>> + { 800000, 0x0b }, >>> + { 1000000, 0x0c }, >>> + { 1200000, 0x0d }, >>> + { 1400000, 0x0e }, >>> + { 1600000, 0x0f }, >> >> Huh?! > > Please give comments that actually mean something other wise I will opt to ignore them. I did below. >> strobe_timeout = (x + 1) * 40 * MSECS_IN_SEC; > > Not sure what equation you are trying to point out here. But if you are trying to apply > a timeout step you cannot do this with this part. As pointed out in the DT doc the timeout > step is not linear. Yeah, I know people are more than often too lazy to think. if (x < 9) strobe_timeout = (x + 1) * 40 * MSECS_IN_SEC; else strobe_timeout = (400 + (x - 9) * 200) * MSECS_IN_SEC; >>> + brightness_val = (brightness/2); >> >> Spaces. > > Not sure what this means checkpatch was clean Even besides missed whispaces it has redundant parens. checkpatch is not a silver bullet to get your code clean and nice. >> This is return led_...(); > > That is a preference. It does not have to be that way. What do you mean? We do not appreciate +LOCs for no (or even nagative!) benefit. >>> + ret = of_property_read_string(led->led_node, "label", &name); >> >> device_property_...(); > > It can be if the maintainer is requesting this. Jacek, if you need rationale behind this comment it's here: the driver has nothing DT specific and getting rid of OF centric programming allows to reuse the driver on non-DT platforms w/o touching a source code. > Is the trend to move to these functions? See above. > Most drivers use the "of" calls. So what? >>> + if (!ret) >> >> if (ret) sounds more natural. And better just to split >> >>> + snprintf(led->led_name, sizeof(led->led_name), >>> + "%s:%s", led->led_node->name, name); >>> + else >>> + snprintf(led->led_name, sizeof(led->led_name), >>> + "%s:torch", led->led_node->name); >> >> const char *tmp; >> >> ret = device_property_read_...(&tmp); >> if (ret) >> tmp = ... >> sprintf(...); No comments on this? >>> + led = devm_kzalloc(&client->dev, >>> + sizeof(struct lm3601x_led), GFP_KERNEL); >> >> sizeof(*led) and one line in the result And this? >>> + { }, >> >> Terminators better w/o comma. > > Looking at other drivers adding comma's on the sentinel is accepted. See the as3645a driver So what? Terminator at compile time even better. >>> + {}, >> >> Ditto. > > See above See above.
On Wed, May 16, 2018 at 1:24 AM, Andy Shevchenko <andy.shevchenko@gmail.com> wrote: > On Wed, May 16, 2018 at 1:08 AM, Dan Murphy <dmurphy@ti.com> wrote: >> On 05/15/2018 04:56 PM, Andy Shevchenko wrote: >>>> +static const struct lm3601x_max_timeouts strobe_timeouts[] = { >>>> + { 40000, 0x00 }, >>>> + { 80000, 0x01 }, >>>> + { 120000, 0x02 }, >>>> + { 160000, 0x03 }, >>>> + { 200000, 0x04 }, >>>> + { 240000, 0x05 }, >>>> + { 280000, 0x06 }, >>>> + { 320000, 0x07 }, >>>> + { 360000, 0x08 }, >>>> + { 400000, 0x09 }, >>>> + { 600000, 0x0a }, >>>> + { 800000, 0x0b }, >>>> + { 1000000, 0x0c }, >>>> + { 1200000, 0x0d }, >>>> + { 1400000, 0x0e }, >>>> + { 1600000, 0x0f }, >>> strobe_timeout = (x + 1) * 40 * MSECS_IN_SEC; >> >> Not sure what equation you are trying to point out here. But if you are trying to apply >> a timeout step you cannot do this with this part. As pointed out in the DT doc the timeout >> step is not linear. > > Yeah, I know people are more than often too lazy to think. > > if (x < 9) > strobe_timeout = (x + 1) * 40 * MSECS_IN_SEC; > else > strobe_timeout = (400 + (x - 9) * 200) * MSECS_IN_SEC; Even just (x - 7) * 200 * ...
Dan, On 05/15/2018 11:29 PM, Dan Murphy wrote: > Jacek > > On 05/15/2018 04:13 PM, Jacek Anaszewski wrote: >> Hi Dan, >> >> Thanks for the update. >> >> On 05/15/2018 05:43 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> >>> --- >>> >>> 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 | 47 +++++++++++++++++++ >>> 1 file changed, 47 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..27930a89e9a5 >>> --- /dev/null >>> +++ b/Documentation/devicetree/bindings/leds/leds-lm3601x.txt >>> @@ -0,0 +1,47 @@ >>> +* 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 >>> + - led-sources: 0 - Indicates a IR mode >>> + 1 - Indicates a Torch (white LED) mode >> >> You don't need led-sources at all. Please use reg as in >> the previous version. led-sources is useful for describing >> more complex arrangements. Here reg will do. > > OK. Thought we would keep consistent with the max IC. > >> >>> + >>> +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 >> >> Please give also a step in the format like below >> (taken from max77693): >> >> Valid values: 62500 - 1000000, step by 62500 (rounded down) > > I would do this but the step is not linear. The step is 40ms from 40 -> 400. > after that the step goes to 200ms from 400->1600. > > same with the current ranges. If you're going to apply Andy's formula in the driver, then you can present it also here. Anyway, please avoid using non-standard "->" symbol in favor of "to" if you're using "from". Or even better I propose to list allowed values - there are not so many of them.
On 05/16/2018 03:10 PM, Jacek Anaszewski wrote: > Dan, > > On 05/15/2018 11:29 PM, Dan Murphy wrote: >> Jacek >> >> On 05/15/2018 04:13 PM, Jacek Anaszewski wrote: >>> Hi Dan, >>> >>> Thanks for the update. >>> >>> On 05/15/2018 05:43 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> >>>> --- >>>> >>>> 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 | 47 +++++++++++++++++++ >>>> 1 file changed, 47 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..27930a89e9a5 >>>> --- /dev/null >>>> +++ b/Documentation/devicetree/bindings/leds/leds-lm3601x.txt >>>> @@ -0,0 +1,47 @@ >>>> +* 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 >>>> + - led-sources: 0 - Indicates a IR mode >>>> + 1 - Indicates a Torch (white LED) mode >>> >>> You don't need led-sources at all. Please use reg as in >>> the previous version. led-sources is useful for describing >>> more complex arrangements. Here reg will do. >> >> OK. Thought we would keep consistent with the max IC. >> >>> >>>> + >>>> +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 >>> >>> Please give also a step in the format like below >>> (taken from max77693): >>> >>> Valid values: 62500 - 1000000, step by 62500 (rounded down) >> >> I would do this but the step is not linear. The step is 40ms from 40 -> 400. >> after that the step goes to 200ms from 400->1600. >> >> same with the current ranges. > > If you're going to apply Andy's formula in the driver, then you can > present it also here. Anyway, please avoid using non-standard "->" > symbol in favor of "to" if you're using "from". > OK I will. I have not decided if I am going to apply the formula or not. I am still trying to process all the comments and determine what is preference and what is beneficial. > Or even better I propose to list allowed values - there are not > so many of them. > I will figure that out based on what I do with the driver. Been busy doing internal work today Dan
Dan, On 05/15/2018 11:50 PM, Dan Murphy wrote: > Jacek > > On 05/15/2018 04:24 PM, Jacek Anaszewski wrote: >> Dan, >> >> Thanks for the update. Please refer to my comments below. >> >> On 05/15/2018 05:43 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> >>> --- >>> >>> 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 | 595 ++++++++++++++++++++++++++++++++++++ >>> 3 files changed, 605 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..fa87da5d5159 >>> --- /dev/null >>> +++ b/drivers/leds/leds-lm3601x.c >>> @@ -0,0 +1,595 @@ >>> +// 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_IR 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_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 >>> + * @led_node: DT device node for the led >>> + * @cdev_torch: led class device pointer for the torch >>> + * @cdev_ir: led class device pointer for infrared >>> + * @fled_cdev: flash led class device pointer >>> + * @led_name: LED label for the Torch or IR LED >>> + * @strobe: LED label for the strobe >>> + * @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 >>> + * @led_mode: The mode to enable either IR or Torch >>> + */ >>> +struct lm3601x_led { >>> + struct mutex lock; >>> + struct regmap *regmap; >>> + struct i2c_client *client; >>> + >>> + struct device_node *led_node; >>> + >>> + struct led_classdev cdev_torch; >>> + struct led_classdev cdev_ir; >>> + >>> + struct led_classdev_flash fled_cdev; >>> + >>> + char led_name[LED_MAX_NAME_SIZE]; >>> + char strobe[LED_MAX_NAME_SIZE]; >>> + >>> + unsigned int last_flag; >>> + unsigned int strobe_timeout; >>> + >>> + u32 torch_current_max; >>> + u32 strobe_current_max; >>> + u32 max_strobe_timeout; >>> + >>> + int led_mode; >>> +}; >>> + >>> +static const struct lm3601x_max_timeouts strobe_timeouts[] = { >>> + { 40000, 0x00 }, >>> + { 80000, 0x01 }, >>> + { 120000, 0x02 }, >>> + { 160000, 0x03 }, >>> + { 200000, 0x04 }, >>> + { 240000, 0x05 }, >>> + { 280000, 0x06 }, >>> + { 320000, 0x07 }, >>> + { 360000, 0x08 }, >>> + { 400000, 0x09 }, >>> + { 600000, 0x0a }, >>> + { 800000, 0x0b }, >>> + { 1000000, 0x0c }, >>> + { 1200000, 0x0d }, >>> + { 1400000, 0x0e }, >>> + { 1600000, 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 = 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 lm3601x_led *led = >>> + container_of(cdev, struct lm3601x_led, cdev_torch); >>> + u8 brightness_val; >>> + int ret, led_mode_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/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, >>> + led_mode_val, >>> + 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 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); >>> + >>> + 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 = { >>> + .strobe_set = lm3601x_strobe_set, >>> + .strobe_get = lm3601x_strobe_get, >>> + .timeout_set = lm3601x_strobe_timeout_set, >>> + .fault_get = lm3601x_strobe_fault_get, >> >> You're still missing flash_brightness_set here. > > OK not sure how this is working then. flash_brightness_set sets the brightness that is to be applied upon execution of strobe_set. > >> >>> +}; >>> + >>> +static int lm3601x_register_leds(struct lm3601x_led *led) >>> +{ >>> + struct led_classdev_flash *fled_cdev; >>> + struct led_classdev *led_cdev; >>> + int err = -ENODEV; >>> + >>> + led->cdev_torch.name = led->led_name; >>> + led->cdev_torch.max_brightness = LED_FULL; >> >> This should be led->torch_current_max converted to brightness levels. >> Please compare how leds-max77693 and leds-as3645a do that. > > I will look into it. > >> >>> + led->cdev_torch.brightness_set_blocking = lm3601x_brightness_set; >>> + err = devm_led_classdev_register(&led->client->dev, >>> + &led->cdev_torch); >>> + if (err < 0) >>> + return err; >> >> You shouldn't register two LED class devices for a single DT child >> node, and honestly I don't know why you would have to do it. > > See below > >> >>> + >>> + 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_FUL >>> + 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); >> >> Please look into the implementation of led_classdev_flash_register() >> and compare other LED flash class drivers. What prevents you from >> applying the same approach? > > I did actually follow the same approach here as the as3645a driver. > I used this driver since I have a data sheet and can read the code. You might have been misled by the indicator LED which is indeed a separate one, but it doesn't have a counterpart in your driver. But see the snippets from that driver: This is for setting indicator brightness: ------------------------------------------ iled_cdev->brightness_set_blocking = as3645a_set_indicator_brightness; This is for setting torch brightness: ------------------------------------- fled_cdev->brightness_set_blocking = as3645a_set_assist_brightness; This is for setting flash brightness: ------------------------------------- static const struct led_flash_ops as3645a_led_flash_ops = { .flash_brightness_set = as3645a_set_flash_brightness, > This is a copy, paste, slight modify from that as3645a driver as3645a_led_class_setup function. > > I used the devm register function above as a variation. > > I will have to dig further into it. > >> >> To put it concisely: LED flash class wrapper was designed to allow >> for operating on a single LED in two modes: torch and flash. >> Torch brightness is governed by brightness file and flash brightness >> is governed by flash_brightness file. The difference between the two >> modes is that torch mode is activated by writing value greater than 0 >> to the brightness file and flash mode is activated by writing 1 to >> the strobe_set file. >> >> In other words flash_brightness overwrites the (torch) brightness >> for the time equal to the flash_timeout or until 0 is written >> to the strobe_set file. > > I will look at it again > >> >>> + 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; >>> + char *mode_name; >>> + int ret = -ENODEV; >>> + >>> + for_each_available_child_of_node(node, child_node) { >>> + led->led_node = of_node_get(child_node); >>> + if (!led->led_node) { >>> + dev_err(&led->client->dev, >>> + "No LED Child node\n"); >>> + >>> + goto out_err; >>> + } >>> + >>> + ret = of_property_read_u32(led->led_node, "led-sources", >>> + &led->led_mode); >>> + if (ret) { >>> + dev_err(&led->client->dev, >>> + "led-sources 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"); >>> + >>> + goto out_err; >>> + >>> + } >>> + } >>> + >>> + if (led->led_mode == LM3601X_LED_TORCH) { >>> + ret = of_property_read_string(led->led_node, "label", &name); >>> + if (!ret) >>> + snprintf(led->led_name, sizeof(led->led_name), >>> + "%s:%s", led->led_node->name, name); >> >> First segment of a LED class device name should be "devicename". > > Hmm. I remember this conversation from my other drivers. > I just picked the wrong dt node name. > > I will fix it. > >> >>> + else >>> + snprintf(led->led_name, sizeof(led->led_name), >>> + "%s:torch", led->led_node->name); >>> + >>> + ret = of_property_read_u32(led->led_node, "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; >>> + } >>> + >>> + mode_name = "torch"; >>> + >>> + } else if (led->led_mode == LM3601X_LED_IR) { >>> + ret = of_property_read_string(led->led_node, "label", &name); >>> + if (!ret) >>> + snprintf(led->led_name, sizeof(led->led_name), >>> + "%s:%s", led->led_node->name, name); >>> + else >>> + snprintf(led->led_name, sizeof(led->led_name), >>> + "%s::infrared", led->led_node->name); >> >> Ditto. > > Ack > >> >>> + >>> + mode_name = "ir"; >>> + >>> + } else { >>> + dev_warn(&led->client->dev, >>> + "No LED mode is selected exiting probe\n"); >>> + >>> + goto out_err; >>> + } >>> + >>> + /* Flash mode is available in IR or Torch mode so read the DT */ >>> + snprintf(led->strobe, sizeof(led->strobe), >>> + "%s:%s:strobe", led->led_node->name, mode_name); >> >> strobe is not a proper LED function name (it is not a noun). >> Please use "flash" instead. > > Strobe can be used as a noun to define an electronic camera flash. But this is > really a US English definition. I've checked in Cambridge Dictionary and I was in fact wrong - it can be either a verb or a noun. > I can change it to flash We're using in few places the expression "to strobe the flash", so let's use "flash" consistently to describe the LED function. >>> + >>> + ret = of_property_read_u32(led->led_node, >>> + "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 = of_property_read_u32(led->led_node, >>> + "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; >>> + } >>> + >>> + lm3601x_init_flash_timeout(led); >>> + >>> +out_err: >>> + of_node_put(led->led_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); >> >> You need also mutex_destroy(&led->lock) here. > > OK. > >> >>> + >>> + 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"); >>> >> > >
Andy The first response is to Jacek the rest are addressed to you On 05/15/2018 05:24 PM, Andy Shevchenko wrote: > On Wed, May 16, 2018 at 1:08 AM, Dan Murphy <dmurphy@ti.com> wrote: >> On 05/15/2018 04:56 PM, Andy Shevchenko wrote: >>> On Tue, May 15, 2018 at 6:43 PM, Dan Murphy <dmurphy@ti.com> wrote: > >>>> + depends on LEDS_CLASS && I2C && OF >>> >>> What is OF specific in this driver? >> >> as3645a_led_class_setup has a "of" dependency > > So what? Is it called from this driver or? > Jacek Do you have a preference about the use of "of" calls over the device_property calls? I know Andy is pushing this for IoT and gave a good presentation on it last year on common mistakes. Not sure this is a "common mistake" but more of a overall Linux compatibility issue between x86 and ARM using two different configuration methods. Since these lighting drivers are agnostic to the processor maybe we should adopt that philosophy. I have no problem being the example driver on that. > >>>> +static const struct lm3601x_max_timeouts strobe_timeouts[] = { >>>> + { 40000, 0x00 }, >>>> + { 80000, 0x01 }, >>>> + { 120000, 0x02 }, >>>> + { 160000, 0x03 }, >>>> + { 200000, 0x04 }, >>>> + { 240000, 0x05 }, >>>> + { 280000, 0x06 }, >>>> + { 320000, 0x07 }, >>>> + { 360000, 0x08 }, >>>> + { 400000, 0x09 }, >>>> + { 600000, 0x0a }, >>>> + { 800000, 0x0b }, >>>> + { 1000000, 0x0c }, >>>> + { 1200000, 0x0d }, >>>> + { 1400000, 0x0e }, >>>> + { 1600000, 0x0f }, >>> >>> Huh?! >> >> Please give comments that actually mean something other wise I will opt to ignore them. > > I did below. Sort of. "So what?" and "Huh" are not really a technical comments. I can tell from your LVEE presentation that you have a passion for making the kernel better. That is awesome but if we try to make the code perfect everytime no code will ever get merged. And we are human and we will miss coding issues and make mistakes. > >>> strobe_timeout = (x + 1) * 40 * MSECS_IN_SEC; >> >> Not sure what equation you are trying to point out here. But if you are trying to apply >> a timeout step you cannot do this with this part. As pointed out in the DT doc the timeout >> step is not linear. > > Yeah, I know people are more than often too lazy to think. Well this is why I put the table in. It is not laziness on my part as I try to add clarity for our newbies or productization engineers, translation/lookup tables are not as bad as you think. I am fully aware that this bloats the data section of the image and has negative impacts on IoT small memory foot print devices. > > if (x < 9) > strobe_timeout = (x + 1) * 40 * MSECS_IN_SEC; > else > strobe_timeout = (400 + (x - 9) * 200) * MSECS_IN_SEC; > Not sure if this will produce the register value I am looking for. I have to run through the algorithim. The idea is to take in the timeout and get the reg value to program. If it yields the expected output, or with a modified equation, then I can pull it in. >>>> + brightness_val = (brightness/2); >>> >>> Spaces. >> >> Not sure what this means checkpatch was clean > > Even besides missed whispaces it has redundant parens. > > checkpatch is not a silver bullet to get your code clean and nice. > True but it is the tool many maintainers use (sometimes) to ensure clean and nice. I am still not see the extra spaces here but I do see the unneeded parens. >>> This is return led_...(); >> >> That is a preference. It does not have to be that way. > > What do you mean? We do not appreciate +LOCs for no (or even nagative!) benefit. > Again for non-IoT cases this would be fine but if we are striving to meet IoT size requirements I do understand your concern. >>>> + ret = of_property_read_string(led->led_node, "label", &name); >>> >>> device_property_...(); >> >> It can be if the maintainer is requesting this. > > Jacek, if you need rationale behind this comment it's here: the driver > has nothing DT specific and getting rid of OF centric programming > allows to reuse the driver on non-DT platforms w/o touching a source > code. > See below >> Is the trend to move to these functions? > > See above. See below > >> Most drivers use the "of" calls. > > So what? > I am deferring this to Jacek. Because this should be an overall coding change for all LED drivers that are new. Like I pointed out I don't mind adding/changing the code as long as all LED drivers will have that same mandate moving forward. If the maintainers agree we can get this driver to be the example for the use of device_property calls. > >>>> + if (!ret) >>> >>> if (ret) sounds more natural. And better just to split OK >>> >>>> + snprintf(led->led_name, sizeof(led->led_name), >>>> + "%s:%s", led->led_node->name, name); >>>> + else >>>> + snprintf(led->led_name, sizeof(led->led_name), >>>> + "%s:torch", led->led_node->name); >>> >>> const char *tmp; >>> >>> ret = device_property_read_...(&tmp); >>> if (ret) >>> tmp = ... >>> sprintf(...); > > No comments on this? No. This would depend on the disposition of the device_property calls. If we go that way then yes this will probably change. But we need to have an if..else as the label property is option in the DT documentation and we have to make a label if one does not exist. > >>>> + led = devm_kzalloc(&client->dev, >>>> + sizeof(struct lm3601x_led), GFP_KERNEL); >>> >>> sizeof(*led) and one line in the result > > And this? Either one should yield the same allocation. So I can change it. > > >>>> + { }, >>> >>> Terminators better w/o comma. >> >> Looking at other drivers adding comma's on the sentinel is accepted. See the as3645a driver > > So what? Not a compile issue here. I have been asked by maintainers/reviewers to add the comma on the sentinnel not just struct definition but also for enums. This is a maintainer decision I defer to them for guidance. > > Terminator at compile time even better. > >>>> + {}, >>> >>> Ditto. >> >> See above > > See above. > See above
Hi Andy and Dan, On 05/16/2018 12:24 AM, Andy Shevchenko wrote: > On Wed, May 16, 2018 at 1:08 AM, Dan Murphy <dmurphy@ti.com> wrote: >> On 05/15/2018 04:56 PM, Andy Shevchenko wrote: >>> On Tue, May 15, 2018 at 6:43 PM, Dan Murphy <dmurphy@ti.com> wrote: > >>>> + depends on LEDS_CLASS && I2C && OF >>> >>> What is OF specific in this driver? >> >> as3645a_led_class_setup has a "of" dependency > > So what? Is it called from this driver or? > > >>>> +static const struct lm3601x_max_timeouts strobe_timeouts[] = { >>>> + { 40000, 0x00 }, >>>> + { 80000, 0x01 }, >>>> + { 120000, 0x02 }, >>>> + { 160000, 0x03 }, >>>> + { 200000, 0x04 }, >>>> + { 240000, 0x05 }, >>>> + { 280000, 0x06 }, >>>> + { 320000, 0x07 }, >>>> + { 360000, 0x08 }, >>>> + { 400000, 0x09 }, >>>> + { 600000, 0x0a }, >>>> + { 800000, 0x0b }, >>>> + { 1000000, 0x0c }, >>>> + { 1200000, 0x0d }, >>>> + { 1400000, 0x0e }, >>>> + { 1600000, 0x0f }, >>> >>> Huh?! >> >> Please give comments that actually mean something other wise I will opt to ignore them. > > I did below. > >>> strobe_timeout = (x + 1) * 40 * MSECS_IN_SEC; >> >> Not sure what equation you are trying to point out here. But if you are trying to apply >> a timeout step you cannot do this with this part. As pointed out in the DT doc the timeout >> step is not linear. > > Yeah, I know people are more than often too lazy to think. > > if (x < 9) > strobe_timeout = (x + 1) * 40 * MSECS_IN_SEC; > else > strobe_timeout = (400 + (x - 9) * 200) * MSECS_IN_SEC; > I like the idea. >>>> + brightness_val = (brightness/2); >>> >>> Spaces. >> >> Not sure what this means checkpatch was clean > > Even besides missed whispaces it has redundant parens. > > checkpatch is not a silver bullet to get your code clean and nice. > >>> This is return led_...(); >> >> That is a preference. It does not have to be that way. I missed that. Dan, please follow Andy's advise. > > What do you mean? We do not appreciate +LOCs for no (or even nagative!) benefit. > >>>> + ret = of_property_read_string(led->led_node, "label", &name); >>> >>> device_property_...(); >> >> It can be if the maintainer is requesting this. > > Jacek, if you need rationale behind this comment it's here: the driver > has nothing DT specific and getting rid of OF centric programming > allows to reuse the driver on non-DT platforms w/o touching a source > code. It has an added value, so yes, let's use it as a standard approach from now on. >> Is the trend to move to these functions? > > See above. > >> Most drivers use the "of" calls. > > So what? > > >>>> + if (!ret) >>> >>> if (ret) sounds more natural. And better just to split >>> >>>> + snprintf(led->led_name, sizeof(led->led_name), >>>> + "%s:%s", led->led_node->name, name); >>>> + else >>>> + snprintf(led->led_name, sizeof(led->led_name), >>>> + "%s:torch", led->led_node->name); >>> >>> const char *tmp; >>> >>> ret = device_property_read_...(&tmp); >>> if (ret) >>> tmp = ... >>> sprintf(...); We're no longer taking devicename section of a LED class device name from DT, so it will look differently anyway. > No comments on this? > >>>> + led = devm_kzalloc(&client->dev, >>>> + sizeof(struct lm3601x_led), GFP_KERNEL); >>> >>> sizeof(*led) and one line in the result > > And this? Ack. > >>>> + { }, >>> >>> Terminators better w/o comma. >> >> Looking at other drivers adding comma's on the sentinel is accepted. See the as3645a driver > > So what? > > Terminator at compile time even better. > >>>> + {}, >>> >>> Ditto. >> >> See above > > See above. >
Jacek and Andy On 05/16/2018 04:02 PM, Jacek Anaszewski wrote: > Hi Andy and Dan, > I will make all the changes then. I don't want to go through and ack each one. Thanks for the guidance and the reviews. It will take a couple days to find all the comments and get this all fixed up. Dan > On 05/16/2018 12:24 AM, Andy Shevchenko wrote: >> On Wed, May 16, 2018 at 1:08 AM, Dan Murphy <dmurphy@ti.com> wrote: >>> On 05/15/2018 04:56 PM, Andy Shevchenko wrote: >>>> On Tue, May 15, 2018 at 6:43 PM, Dan Murphy <dmurphy@ti.com> wrote: >> >>>>> + depends on LEDS_CLASS && I2C && OF >>>> >>>> What is OF specific in this driver? >>> >>> as3645a_led_class_setup has a "of" dependency >> >> So what? Is it called from this driver or? >> >> >>>>> +static const struct lm3601x_max_timeouts strobe_timeouts[] = { >>>>> + { 40000, 0x00 }, >>>>> + { 80000, 0x01 }, >>>>> + { 120000, 0x02 }, >>>>> + { 160000, 0x03 }, >>>>> + { 200000, 0x04 }, >>>>> + { 240000, 0x05 }, >>>>> + { 280000, 0x06 }, >>>>> + { 320000, 0x07 }, >>>>> + { 360000, 0x08 }, >>>>> + { 400000, 0x09 }, >>>>> + { 600000, 0x0a }, >>>>> + { 800000, 0x0b }, >>>>> + { 1000000, 0x0c }, >>>>> + { 1200000, 0x0d }, >>>>> + { 1400000, 0x0e }, >>>>> + { 1600000, 0x0f }, >>>> >>>> Huh?! >>> >>> Please give comments that actually mean something other wise I will opt to ignore them. >> >> I did below. >> >>>> strobe_timeout = (x + 1) * 40 * MSECS_IN_SEC; >>> >>> Not sure what equation you are trying to point out here. But if you are trying to apply >>> a timeout step you cannot do this with this part. As pointed out in the DT doc the timeout >>> step is not linear. >> >> Yeah, I know people are more than often too lazy to think. >> >> if (x < 9) >> strobe_timeout = (x + 1) * 40 * MSECS_IN_SEC; >> else >> strobe_timeout = (400 + (x - 9) * 200) * MSECS_IN_SEC; >> > > I like the idea. > >>>>> + brightness_val = (brightness/2); >>>> >>>> Spaces. >>> >>> Not sure what this means checkpatch was clean >> >> Even besides missed whispaces it has redundant parens. >> >> checkpatch is not a silver bullet to get your code clean and nice. >> >>>> This is return led_...(); >>> >>> That is a preference. It does not have to be that way. > > I missed that. Dan, please follow Andy's advise. > >> >> What do you mean? We do not appreciate +LOCs for no (or even nagative!) benefit. >> >>>>> + ret = of_property_read_string(led->led_node, "label", &name); >>>> >>>> device_property_...(); >>> >>> It can be if the maintainer is requesting this. >> >> Jacek, if you need rationale behind this comment it's here: the driver >> has nothing DT specific and getting rid of OF centric programming >> allows to reuse the driver on non-DT platforms w/o touching a source >> code. > > It has an added value, so yes, let's use it as a standard approach > from now on. > >>> Is the trend to move to these functions? >> >> See above. >> >>> Most drivers use the "of" calls. >> >> So what? >> >> >>>>> + if (!ret) >>>> >>>> if (ret) sounds more natural. And better just to split >>>> >>>>> + snprintf(led->led_name, sizeof(led->led_name), >>>>> + "%s:%s", led->led_node->name, name); >>>>> + else >>>>> + snprintf(led->led_name, sizeof(led->led_name), >>>>> + "%s:torch", led->led_node->name); >>>> >>>> const char *tmp; >>>> >>>> ret = device_property_read_...(&tmp); >>>> if (ret) >>>> tmp = ... >>>> sprintf(...); > > We're no longer taking devicename section of a LED class device name > from DT, so it will look differently anyway. > >> No comments on this? >> >>>>> + led = devm_kzalloc(&client->dev, >>>>> + sizeof(struct lm3601x_led), GFP_KERNEL); >>>> >>>> sizeof(*led) and one line in the result >> >> And this? > > Ack. > >> >>>>> + { }, >>>> >>>> Terminators better w/o comma. >>> >>> Looking at other drivers adding comma's on the sentinel is accepted. See the as3645a driver >> >> So what? >> >> Terminator at compile time even better. >> >>>>> + {}, >>>> >>>> Ditto. >>> >>> See above >> >> See above. >> >
Jacek and Andy On 05/16/2018 04:13 PM, Dan Murphy wrote: > Jacek and Andy > > On 05/16/2018 04:02 PM, Jacek Anaszewski wrote: >> Hi Andy and Dan, >> > > I will make all the changes then. I don't want to go through and ack each one. > Let me clarify. I will make all the change Jacek has guided on. There is still a terminator comma vs no comma comment that needs disposition at the end of this file. Dan > Thanks for the guidance and the reviews. > > It will take a couple days to find all the comments and get this all fixed up. > > Dan > >> On 05/16/2018 12:24 AM, Andy Shevchenko wrote: >>> On Wed, May 16, 2018 at 1:08 AM, Dan Murphy <dmurphy@ti.com> wrote: >>>> On 05/15/2018 04:56 PM, Andy Shevchenko wrote: >>>>> On Tue, May 15, 2018 at 6:43 PM, Dan Murphy <dmurphy@ti.com> wrote: >>> >>>>>> + depends on LEDS_CLASS && I2C && OF >>>>> >>>>> What is OF specific in this driver? >>>> >>>> as3645a_led_class_setup has a "of" dependency >>> >>> So what? Is it called from this driver or? >>> >>> >>>>>> +static const struct lm3601x_max_timeouts strobe_timeouts[] = { >>>>>> + { 40000, 0x00 }, >>>>>> + { 80000, 0x01 }, >>>>>> + { 120000, 0x02 }, >>>>>> + { 160000, 0x03 }, >>>>>> + { 200000, 0x04 }, >>>>>> + { 240000, 0x05 }, >>>>>> + { 280000, 0x06 }, >>>>>> + { 320000, 0x07 }, >>>>>> + { 360000, 0x08 }, >>>>>> + { 400000, 0x09 }, >>>>>> + { 600000, 0x0a }, >>>>>> + { 800000, 0x0b }, >>>>>> + { 1000000, 0x0c }, >>>>>> + { 1200000, 0x0d }, >>>>>> + { 1400000, 0x0e }, >>>>>> + { 1600000, 0x0f }, >>>>> >>>>> Huh?! >>>> >>>> Please give comments that actually mean something other wise I will opt to ignore them. >>> >>> I did below. >>> >>>>> strobe_timeout = (x + 1) * 40 * MSECS_IN_SEC; >>>> >>>> Not sure what equation you are trying to point out here. But if you are trying to apply >>>> a timeout step you cannot do this with this part. As pointed out in the DT doc the timeout >>>> step is not linear. >>> >>> Yeah, I know people are more than often too lazy to think. >>> >>> if (x < 9) >>> strobe_timeout = (x + 1) * 40 * MSECS_IN_SEC; >>> else >>> strobe_timeout = (400 + (x - 9) * 200) * MSECS_IN_SEC; >>> >> >> I like the idea. >> >>>>>> + brightness_val = (brightness/2); >>>>> >>>>> Spaces. >>>> >>>> Not sure what this means checkpatch was clean >>> >>> Even besides missed whispaces it has redundant parens. >>> >>> checkpatch is not a silver bullet to get your code clean and nice. >>> >>>>> This is return led_...(); >>>> >>>> That is a preference. It does not have to be that way. >> >> I missed that. Dan, please follow Andy's advise. >> >>> >>> What do you mean? We do not appreciate +LOCs for no (or even nagative!) benefit. >>> >>>>>> + ret = of_property_read_string(led->led_node, "label", &name); >>>>> >>>>> device_property_...(); >>>> >>>> It can be if the maintainer is requesting this. >>> >>> Jacek, if you need rationale behind this comment it's here: the driver >>> has nothing DT specific and getting rid of OF centric programming >>> allows to reuse the driver on non-DT platforms w/o touching a source >>> code. >> >> It has an added value, so yes, let's use it as a standard approach >> from now on. >> >>>> Is the trend to move to these functions? >>> >>> See above. >>> >>>> Most drivers use the "of" calls. >>> >>> So what? >>> >>> >>>>>> + if (!ret) >>>>> >>>>> if (ret) sounds more natural. And better just to split >>>>> >>>>>> + snprintf(led->led_name, sizeof(led->led_name), >>>>>> + "%s:%s", led->led_node->name, name); >>>>>> + else >>>>>> + snprintf(led->led_name, sizeof(led->led_name), >>>>>> + "%s:torch", led->led_node->name); >>>>> >>>>> const char *tmp; >>>>> >>>>> ret = device_property_read_...(&tmp); >>>>> if (ret) >>>>> tmp = ... >>>>> sprintf(...); >> >> We're no longer taking devicename section of a LED class device name >> from DT, so it will look differently anyway. >> >>> No comments on this? >>> >>>>>> + led = devm_kzalloc(&client->dev, >>>>>> + sizeof(struct lm3601x_led), GFP_KERNEL); >>>>> >>>>> sizeof(*led) and one line in the result >>> >>> And this? >> >> Ack. >> >>> >>>>>> + { }, >>>>> >>>>> Terminators better w/o comma. >>>> >>>> Looking at other drivers adding comma's on the sentinel is accepted. See the as3645a driver >>> >>> So what? >>> >>> Terminator at compile time even better. >>> >>>>>> + {}, >>>>> >>>>> Ditto. >>>> >>>> See above >>> >>> See above. >>> >> > >
Dan, On 05/16/2018 11:17 PM, Dan Murphy wrote: > Jacek and Andy > > On 05/16/2018 04:13 PM, Dan Murphy wrote: >> Jacek and Andy >> >> On 05/16/2018 04:02 PM, Jacek Anaszewski wrote: >>> Hi Andy and Dan, >>> >> >> I will make all the changes then. I don't want to go through and ack each one. >> > > Let me clarify. I will make all the change Jacek has guided on. There is still a > terminator comma vs no comma comment that needs disposition at the end of this file. No comma option seems better in this case. >> Thanks for the guidance and the reviews. >> >> It will take a couple days to find all the comments and get this all fixed up. >> >> Dan >> >>> On 05/16/2018 12:24 AM, Andy Shevchenko wrote: >>>> On Wed, May 16, 2018 at 1:08 AM, Dan Murphy <dmurphy@ti.com> wrote: >>>>> On 05/15/2018 04:56 PM, Andy Shevchenko wrote: >>>>>> On Tue, May 15, 2018 at 6:43 PM, Dan Murphy <dmurphy@ti.com> wrote: >>>> >>>>>>> + depends on LEDS_CLASS && I2C && OF >>>>>> >>>>>> What is OF specific in this driver? >>>>> >>>>> as3645a_led_class_setup has a "of" dependency >>>> >>>> So what? Is it called from this driver or? >>>> >>>> >>>>>>> +static const struct lm3601x_max_timeouts strobe_timeouts[] = { >>>>>>> + { 40000, 0x00 }, >>>>>>> + { 80000, 0x01 }, >>>>>>> + { 120000, 0x02 }, >>>>>>> + { 160000, 0x03 }, >>>>>>> + { 200000, 0x04 }, >>>>>>> + { 240000, 0x05 }, >>>>>>> + { 280000, 0x06 }, >>>>>>> + { 320000, 0x07 }, >>>>>>> + { 360000, 0x08 }, >>>>>>> + { 400000, 0x09 }, >>>>>>> + { 600000, 0x0a }, >>>>>>> + { 800000, 0x0b }, >>>>>>> + { 1000000, 0x0c }, >>>>>>> + { 1200000, 0x0d }, >>>>>>> + { 1400000, 0x0e }, >>>>>>> + { 1600000, 0x0f }, >>>>>> >>>>>> Huh?! >>>>> >>>>> Please give comments that actually mean something other wise I will opt to ignore them. >>>> >>>> I did below. >>>> >>>>>> strobe_timeout = (x + 1) * 40 * MSECS_IN_SEC; >>>>> >>>>> Not sure what equation you are trying to point out here. But if you are trying to apply >>>>> a timeout step you cannot do this with this part. As pointed out in the DT doc the timeout >>>>> step is not linear. >>>> >>>> Yeah, I know people are more than often too lazy to think. >>>> >>>> if (x < 9) >>>> strobe_timeout = (x + 1) * 40 * MSECS_IN_SEC; >>>> else >>>> strobe_timeout = (400 + (x - 9) * 200) * MSECS_IN_SEC; >>>> >>> >>> I like the idea. >>> >>>>>>> + brightness_val = (brightness/2); >>>>>> >>>>>> Spaces. >>>>> >>>>> Not sure what this means checkpatch was clean >>>> >>>> Even besides missed whispaces it has redundant parens. >>>> >>>> checkpatch is not a silver bullet to get your code clean and nice. >>>> >>>>>> This is return led_...(); >>>>> >>>>> That is a preference. It does not have to be that way. >>> >>> I missed that. Dan, please follow Andy's advise. >>> >>>> >>>> What do you mean? We do not appreciate +LOCs for no (or even nagative!) benefit. >>>> >>>>>>> + ret = of_property_read_string(led->led_node, "label", &name); >>>>>> >>>>>> device_property_...(); >>>>> >>>>> It can be if the maintainer is requesting this. >>>> >>>> Jacek, if you need rationale behind this comment it's here: the driver >>>> has nothing DT specific and getting rid of OF centric programming >>>> allows to reuse the driver on non-DT platforms w/o touching a source >>>> code. >>> >>> It has an added value, so yes, let's use it as a standard approach >>> from now on. >>> >>>>> Is the trend to move to these functions? >>>> >>>> See above. >>>> >>>>> Most drivers use the "of" calls. >>>> >>>> So what? >>>> >>>> >>>>>>> + if (!ret) >>>>>> >>>>>> if (ret) sounds more natural. And better just to split >>>>>> >>>>>>> + snprintf(led->led_name, sizeof(led->led_name), >>>>>>> + "%s:%s", led->led_node->name, name); >>>>>>> + else >>>>>>> + snprintf(led->led_name, sizeof(led->led_name), >>>>>>> + "%s:torch", led->led_node->name); >>>>>> >>>>>> const char *tmp; >>>>>> >>>>>> ret = device_property_read_...(&tmp); >>>>>> if (ret) >>>>>> tmp = ... >>>>>> sprintf(...); >>> >>> We're no longer taking devicename section of a LED class device name >>> from DT, so it will look differently anyway. >>> >>>> No comments on this? >>>> >>>>>>> + led = devm_kzalloc(&client->dev, >>>>>>> + sizeof(struct lm3601x_led), GFP_KERNEL); >>>>>> >>>>>> sizeof(*led) and one line in the result >>>> >>>> And this? >>> >>> Ack. >>> >>>> >>>>>>> + { }, >>>>>> >>>>>> Terminators better w/o comma. >>>>> >>>>> Looking at other drivers adding comma's on the sentinel is accepted. See the as3645a driver >>>> >>>> So what? >>>> >>>> Terminator at compile time even better. >>>> >>>>>>> + {}, >>>>>> >>>>>> Ditto. >>>>> >>>>> See above >>>> >>>> See above. >>>> >>> >> >> > >
On Wed, May 16, 2018 at 11:43 PM, Dan Murphy <dmurphy@ti.com> wrote: > On 05/15/2018 05:24 PM, Andy Shevchenko wrote: >> On Wed, May 16, 2018 at 1:08 AM, Dan Murphy <dmurphy@ti.com> wrote: >>> On 05/15/2018 04:56 PM, Andy Shevchenko wrote: >>>> On Tue, May 15, 2018 at 6:43 PM, Dan Murphy <dmurphy@ti.com> wrote: >>>>> +static const struct lm3601x_max_timeouts strobe_timeouts[] = { >>>>> + { 40000, 0x00 }, >>>>> + { 80000, 0x01 }, >>>>> + { 120000, 0x02 }, >>>>> + { 160000, 0x03 }, >>>>> + { 200000, 0x04 }, >>>>> + { 240000, 0x05 }, >>>>> + { 280000, 0x06 }, >>>>> + { 320000, 0x07 }, >>>>> + { 360000, 0x08 }, >>>>> + { 400000, 0x09 }, >>>>> + { 600000, 0x0a }, >>>>> + { 800000, 0x0b }, >>>>> + { 1000000, 0x0c }, >>>>> + { 1200000, 0x0d }, >>>>> + { 1400000, 0x0e }, >>>>> + { 1600000, 0x0f }, >>>> >>>> Huh?! >>> >>> Please give comments that actually mean something other wise I will opt to ignore them. >> >> I did below. > > Sort of. "So what?" and "Huh" are not really a technical comments. True. > I can tell from your LVEE presentation that > you have a passion for making the kernel better. That is awesome but if we try to make the code perfect everytime no code will > ever get merged. True. > And we are human and we will miss coding issues and make mistakes. True. While all above is true, I still don't get why we have lookup tables whenever we can use simple calculations. >>>> strobe_timeout = (x + 1) * 40 * MSECS_IN_SEC; >>> >>> Not sure what equation you are trying to point out here. But if you are trying to apply >>> a timeout step you cannot do this with this part. As pointed out in the DT doc the timeout >>> step is not linear. >> >> Yeah, I know people are more than often too lazy to think. > > Well this is why I put the table in. It is not laziness on my part as I try to add clarity for > our newbies or productization engineers, translation/lookup tables are not as bad as you think. Sometimes. > I am fully aware that this bloats the data section of the image and has negative impacts > on IoT small memory foot print devices. > >> >> if (x < 9) >> strobe_timeout = (x + 1) * 40 * MSECS_IN_SEC; >> else >> strobe_timeout = (400 + (x - 9) * 200) * MSECS_IN_SEC; >> > > Not sure if this will produce the register value I am looking for. I have to run through the > algorithim. The idea is to take in the timeout and get the reg value to program. > > If it yields the expected output, or with a modified equation, then I can pull it in. So, you need a reversed calculus in this case. u8 regval; /* To make it in range... */ timeout = clamp_val(timeout, 40000, 1600000); /* ...other possibility to issue an error */ if (timeout >= 400000) regval = timeout / 200000 + 0x07; else regval = timeout / 40000 - 0x01; In any case, please double check it gives a correct values. >>>>> + brightness_val = (brightness/2); > I am still not see the extra spaces here. Exactly, spaces are missed. >> What do you mean? We do not appreciate +LOCs for no (or even nagative!) benefit. > Again for non-IoT cases this would be fine but if we are striving to meet IoT size > requirements I do understand your concern. It has nothing with IoT in this case. The pattern ret = foo(); if (ret) return ret; return 0; is a too way verbose variant of ret foo(); though I can imagine some rare cases where it might make sense. OTOH the variant ret = foo(); return ret; pointless at all. >>>>> + ret = of_property_read_string(led->led_node, "label", &name); >>>> device_property_...(); >> Jacek, if you need rationale behind this comment it's here: the driver >> has nothing DT specific and getting rid of OF centric programming >> allows to reuse the driver on non-DT platforms w/o touching a source >> code. > I am deferring this to Jacek. Because this should be an overall coding change for all LED > drivers that are new. > Like I pointed out I don't mind adding/changing the code as long as > all LED drivers will have that same mandate moving forward. Whenever we might get driver working on different platforms for free, better to do it that way. The entire exersice about unified device properties API is to give drivers an agnostic way for getting their resources / properties. > If the maintainers agree we can get this driver to be the example for the use of device_property calls. That's fair. I dunno why Sakari made as3645a not in this way in the first place. Will talk to him. >>>>> + snprintf(led->led_name, sizeof(led->led_name), >>>>> + "%s:%s", led->led_node->name, name); >>>>> + else >>>>> + snprintf(led->led_name, sizeof(led->led_name), >>>>> + "%s:torch", led->led_node->name); >>>> >>>> const char *tmp; >>>> >>>> ret = device_property_read_...(&tmp); >>>> if (ret) >>>> tmp = ... >>>> sprintf(...); >> >> No comments on this? > > No. This would depend on the disposition of the device_property calls. > If we go that way then yes this will probably change. But we need to have an > if..else as the label property is option in the DT documentation and we have to > make a label if one does not exist. The example is not related to device_property_ vs of_property_. The idea is to figure out the label and use it in one place. Though, I have no preferences of it.
On Thu, May 17, 2018 at 1:11 AM, Andy Shevchenko <andy.shevchenko@gmail.com> wrote: > On Wed, May 16, 2018 at 11:43 PM, Dan Murphy <dmurphy@ti.com> wrote: >> On 05/15/2018 05:24 PM, Andy Shevchenko wrote: >>> On Wed, May 16, 2018 at 1:08 AM, Dan Murphy <dmurphy@ti.com> wrote: >>>> On 05/15/2018 04:56 PM, Andy Shevchenko wrote: >>>>>> +static const struct lm3601x_max_timeouts strobe_timeouts[] = { >>>>>> + { 40000, 0x00 }, >>>>>> + { 80000, 0x01 }, >>>>>> + { 120000, 0x02 }, >>>>>> + { 160000, 0x03 }, >>>>>> + { 200000, 0x04 }, >>>>>> + { 240000, 0x05 }, >>>>>> + { 280000, 0x06 }, >>>>>> + { 320000, 0x07 }, >>>>>> + { 360000, 0x08 }, >>>>>> + { 400000, 0x09 }, >>>>>> + { 600000, 0x0a }, >>>>>> + { 800000, 0x0b }, >>>>>> + { 1000000, 0x0c }, >>>>>> + { 1200000, 0x0d }, >>>>>> + { 1400000, 0x0e }, >>>>>> + { 1600000, 0x0f }, >>> if (x < 9) >>> strobe_timeout = (x + 1) * 40 * MSECS_IN_SEC; >>> else >>> strobe_timeout = (x - 7) * 200 * MSECS_IN_SEC; >> >> Not sure if this will produce the register value I am looking for. I have to run through the >> algorithim. The idea is to take in the timeout and get the reg value to program. >> >> If it yields the expected output, or with a modified equation, then I can pull it in. > > So, you need a reversed calculus in this case. > > u8 regval; > > /* To make it in range... */ > timeout = clamp_val(timeout, 40000, 1600000); > /* ...other possibility to issue an error */ > > if (timeout >= 400000) > regval = timeout / 200000 + 0x07; > else > regval = timeout / 40000 - 0x01; Forgot to mention that there are also three approaches (considering division here): - round up - round down - round to closest > > In any case, please double check it gives a correct values.
Jacek On 05/16/2018 04:17 PM, Dan Murphy wrote: <snip> >>>> >>>> >>>>>>> + if (!ret) >>>>>> >>>>>> if (ret) sounds more natural. And better just to split >>>>>> >>>>>>> + snprintf(led->led_name, sizeof(led->led_name), >>>>>>> + "%s:%s", led->led_node->name, name); >>>>>>> + else >>>>>>> + snprintf(led->led_name, sizeof(led->led_name), >>>>>>> + "%s:torch", led->led_node->name); >>>>>> >>>>>> const char *tmp; >>>>>> >>>>>> ret = device_property_read_...(&tmp); >>>>>> if (ret) >>>>>> tmp = ... >>>>>> sprintf(...); >>> >>> We're no longer taking devicename section of a LED class device name >>> from DT, so it will look differently anyway. >>> So in adding the device_property code I think we again are reaching the LED label issue. In ARM with DT we would take the parent device node name and append it to the label if the optional label property was not available. In migrating to the device_property APIs we don't or can't depend on that parent node anymore. So for the case where the label property does not exist should we use a hard coded name or should we try to use the name from a device_id table. This is how we did this for the leds-lp8860 driver. If the label did not exist we used the i2c_device_id table and pulled the string from there. Thoughts? <snip>
Dan, On 05/17/2018 04:34 PM, Dan Murphy wrote: > Jacek > > On 05/16/2018 04:17 PM, Dan Murphy wrote: > <snip> > >>>>> >>>>> >>>>>>>> + if (!ret) >>>>>>> >>>>>>> if (ret) sounds more natural. And better just to split >>>>>>> >>>>>>>> + snprintf(led->led_name, sizeof(led->led_name), >>>>>>>> + "%s:%s", led->led_node->name, name); >>>>>>>> + else >>>>>>>> + snprintf(led->led_name, sizeof(led->led_name), >>>>>>>> + "%s:torch", led->led_node->name); >>>>>>> >>>>>>> const char *tmp; >>>>>>> >>>>>>> ret = device_property_read_...(&tmp); >>>>>>> if (ret) >>>>>>> tmp = ... >>>>>>> sprintf(...); >>>> >>>> We're no longer taking devicename section of a LED class device name >>>> from DT, so it will look differently anyway. >>>> > > So in adding the device_property code I think we again are reaching the LED label issue. > In ARM with DT we would take the parent device node name and append it to the label AFAIR this approach (parent DT node name used for devicename ) was incidentally applied in leds-as3645a.c. Soon after that we started to use led-controller for parent DT name, according to Rob's request. In the most of LED class drivers this is a child DT node which is used for devicename, in case label is absent. > if the optional label property was not available. In migrating to the device_property > APIs we don't or can't depend on that parent node anymore. > > So for the case where the label property does not exist should we use a hard coded name > or should we try to use the name from a device_id table. > > This is how we did this for the leds-lp8860 driver. If the label did not exist we used the > i2c_device_id table and pulled the string from there. i2c_device_id can't be applied as a generic pattern, but only for I2C hooked devices. Nonetheless since it allows to save few lines of code in case of drivers supporting a family of chips we can use it. We are going to get rid of a devicename section from LED class device name soon anyway, since it is redundant.
On Tue, May 15, 2018 at 10:43:51AM -0500, 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> > --- > > 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 | 47 +++++++++++++++++++ > 1 file changed, 47 insertions(+) > create mode 100644 Documentation/devicetree/bindings/leds/leds-lm3601x.txt Reviewed-by: Rob Herring <robh@kernel.org> -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/Documentation/devicetree/bindings/leds/leds-lm3601x.txt b/Documentation/devicetree/bindings/leds/leds-lm3601x.txt new file mode 100644 index 000000000000..27930a89e9a5 --- /dev/null +++ b/Documentation/devicetree/bindings/leds/leds-lm3601x.txt @@ -0,0 +1,47 @@ +* 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 + - led-sources: 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 = <0>; + label = "white:torch"; + led-max-microamp = <376000>; + flash-max-microamp = <1500000>; + flash-max-timeout-us = <1600000>; + led-sources = <1>; + }; +} + +For more product information please see the links below: +http://www.ti.com/product/LM36010 +http://www.ti.com/product/LM36011
Introduce the device tree bindings for the lm3601x family of LED torch, flash and IR drivers. Signed-off-by: Dan Murphy <dmurphy@ti.com> --- 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 | 47 +++++++++++++++++++ 1 file changed, 47 insertions(+) create mode 100644 Documentation/devicetree/bindings/leds/leds-lm3601x.txt