lkml.org 
[lkml]   [2017]   [Nov]   [15]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
SubjectRe: [PATCH v3 2/2] leds: lm3692x: Introduce LM3692x dual string driver
From
Date
Pavel

On 11/15/2017 03:12 PM, Pavel Machek wrote:
> Hi!
>
>> Introducing the LM3692x Dual-String white LED driver.
>>
>> Data sheet is located
>
> "located at"? (Twice.)

Actually it is 3x since I call it out in the dt binding as well.

So what to eliminate?

>
>> http://www.ti.com/lit/ds/snvsa29/snvsa29.pdf
>>
>> Signed-off-by: Dan Murphy <dmurphy@ti.com>
>
>> +config LEDS_LM3692X
>> + tristate "LED support for LM3692x Chips"
>> + depends on LEDS_CLASS && I2C
>> + select REGMAP_I2C
>> + help
>> + This option enables support for the TI LM3692x family
>> + of LED drivers.
>
> "If unsure ..., module will be named..."
>
> Might want to say this is for backlight LEDs here.

AcK

>
>> +static int lm3692x_fault_check(struct lm3692x_led *led)
>> +{
>> + int ret, fault;
>> + unsigned int read_buf;
>> +
>> + ret = regmap_read(led->regmap, LM3692X_FAULT_FLAGS, &read_buf);
>> + if (ret)
>> + return ret;
>> +
>> + fault = read_buf;
>> +
>> + if (fault)
>> + dev_err(&led->client->dev, "Detected a fault 0x%X\n",
>> + fault);
>> +
>> + return ret;
>> +}
>
> Get rid of "fault" variable?
>
> Does fault need to be propagated to the caller?

I should probably set ret to fail if I see a fault or as you suggest propagate the fault

>
>
>> +static int lm3692x_init(struct lm3692x_led *led)
>> +{
>> + int ret;
>> +
>> + if (led->regulator) {
>> + ret = regulator_enable(led->regulator);
>> + if (ret)
>> + dev_err(&led->client->dev,
>> + "Failed to enable regulator\n");
>> + goto out;
>> + }
>> +
>> + if (led->enable_gpio)
>> + gpiod_direction_output(led->enable_gpio, 1);
>> +
>> + ret = lm3692x_fault_check(led);
>> + if (ret) {
>> + dev_err(&led->client->dev, "Cannot read/clear faults\n");
>> + goto out;
>> + }
>> +
>> + ret = regmap_write(led->regmap, LM3692X_BRT_CTRL, 0x00);
>> + if (ret) {
>> + dev_err(&led->client->dev, "Fail writing BRT CTRL\n");
>> + goto out;
>> + }
>
> How often are those fails reached? Maybe regmap wrapper that would
> print "reading/writing register XY failed" would be enough?

Or maybe in the out label I can just dev_err once saying initializing the device failed.
These are really only called at probe time. Its the only time init is called.

Dan

>
> Otherwise looks good,
>
> Acked-by: Pavel Machek <pavel@ucw.cz>
> Pavel
>


--
------------------
Dan Murphy

\
 
 \ /
  Last update: 2017-11-15 22:27    [W:0.106 / U:0.020 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site