Re: [PATCH v3 2/2] leds: lm3692x: Introduce LM3692x dual string driver

[Dan Murphy <dmurphy@ti.com>]

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