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@xxxxxx> 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); > > > > -- ------------------ Dan Murphy -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
