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

From: Dan Murphy
Date: Tue May 15 2018 - 18:08:48 EST


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