lkml.org 
[lkml]   [2018]   [May]   [15]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    From
    Date
    SubjectRe: [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

    \
     
     \ /
      Last update: 2018-05-15 23:58    [W:3.678 / U:0.036 seconds]
    ©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site