Re: [PATCH v6 2/2] leds: lm3601x: Introduce the lm3601x LED driver

[Andy Shevchenko]

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?


> +#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