Hi! > Introduce the lm3697 LED driver for > backlighting and display. > > Datasheet location: > http://www.ti.com/lit/ds/symlink/lm3697.pdf > > Signed-off-by: Dan Murphy <dmur...@ti.com>
> +
> +#define LM3697_HVLED1_2_3_A 0
> +#define LM3697_HVLED1_B_HVLED2_3_A 1
> +#define LM3697_HVLED2_B_HVLED1_3_A 2
> +#define LM3697_HVLED1_2_B_HVLED3_A 3
> +#define LM3697_HVLED3_B_HVLED1_2_A 4
> +#define LM3697_HVLED1_3_B_HVLED2_A 5
> +#define LM3697_HVLED1_A_HVLED2_3_B 6
> +#define LM3697_HVLED1_2_3_B 7
That's rather long and verbose way to describe a bitmap, right?
> +static const struct regmap_config lm3697_regmap_config = {
> + .reg_bits = 8,
> + .val_bits = 8,
> +
> + .max_register = LM3697_CTRL_ENABLE,
> + .reg_defaults = lm3697_reg_defs,
> + .num_reg_defaults = ARRAY_SIZE(lm3697_reg_defs),
> + .cache_type = REGCACHE_RBTREE,
> +};
Is rbtree good idea? You don't have that many registers.
> +static int lm3697_init(struct lm3697 *priv)
> +{
> + int ret;
> +
....
> + regmap_write(priv->regmap, LM3697_RESET, LM3697_SW_RESET);
No error checking required here?
> + if (priv->control_bank_config < LM3697_HVLED1_2_3_A ||
> + priv->control_bank_config > LM3697_HVLED1_2_3_B) {
> + dev_err(&priv->client->dev, "Control bank configuration is out
> of range\n");
> + ret = -EINVAL;
> + goto out;
> + }
> +
> + device_for_each_child_node(priv->dev, child) {
> + led = &priv->leds[i];
> +
> + ret = fwnode_property_read_u32(child, "reg",
> &led->control_bank);
> + if (ret) {
> + dev_err(&priv->client->dev, "reg DT property
> missing\n");
> + goto child_out;
> + }
> +
> + fwnode_property_read_string(child, "linux,default-trigger",
> + &led->led_dev.default_trigger);
> +
> + ret = fwnode_property_read_string(child, "label", &name);
> + if (ret)
> + snprintf(led->label, sizeof(led->label),
> + "%s::", priv->client->name);
> + else
> + snprintf(led->label, sizeof(led->label),
> + "%s:%s", priv->client->name, name);
> +
> +
> + led->priv = priv;
> + led->led_dev.name = led->label;
> + led->led_dev.brightness_set_blocking = lm3697_brightness_set;
> +
> + ret = devm_led_classdev_register(priv->dev, &led->led_dev);
> + if (ret) {
> + dev_err(&priv->client->dev, "led register err: %d\n",
> ret);
> + goto child_out;
> + }
> +
> + if (priv->control_bank_config == LM3697_HVLED1_2_3_A ||
> + priv->control_bank_config == LM3697_HVLED1_2_3_B)
> + goto child_out;
This checks if we have just one bank, I see it. Should it also check
the led actually uses the correct bank?
> + i++;
> + fwnode_handle_put(child);
> + }
> +
> +child_out:
> + fwnode_handle_put(child);
Is not the fwnode_handle_put() done twice for non-error case?
> + ret = lm3697_init(led);
> + if (ret)
> + return ret;
> +
> + return ret;
> +}
The if is not needed here.
> +static int lm3697_remove(struct i2c_client *client)
> +{
> + struct lm3697 *led = i2c_get_clientdata(client);
> + int ret;
> +
> + ret = regmap_update_bits(led->regmap, LM3697_CTRL_ENABLE,
> + LM3697_CTRL_A_B_EN, 0);
> + if (ret) {
> + dev_err(&led->client->dev, "Failed to disable regulator\n");
> + return ret;
Misleading, this does nothing with regulators.
Pavel
--
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures)
http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html
signature.asc
Description: Digital signature
