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

[Pavel Machek]

Hi!

> Introducing the LM3692x Dual-String white LED driver.
> 
> Data sheet is located

"located at"? (Twice.)

> http://www.ti.com/lit/ds/snvsa29/snvsa29.pdf
> 
> Signed-off-by: Dan Murphy <dmurphy@xxxxxx>

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

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


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

Otherwise looks good,

Acked-by: Pavel Machek <pavel@xxxxxx>
									Pavel

-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html

Attachment: signature.asc
Description: Digital signature