Messages in this thread | | | From | Andy Shevchenko <> | Date | Wed, 16 May 2018 00:56:58 +0300 | Subject | Re: [PATCH v6 2/2] leds: lm3601x: Introduce the lm3601x LED driver |
| |
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);
-- With Best Regards, Andy Shevchenko
| |