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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <abe5f2cc-9216-6f0b-8063-8a0079557ecf@ti.com>
Date:   Wed, 15 Nov 2017 15:25:42 -0600
From:   Dan Murphy <dmurphy@...com>
To:     Pavel Machek <pavel@....cz>
CC:     <robh+dt@...nel.org>, <mark.rutland@....com>, <rpurdie@...ys.net>,
        <jacek.anaszewski@...il.com>, <devicetree@...r.kernel.org>,
        <linux-kernel@...r.kernel.org>, <linux-leds@...r.kernel.org>
Subject: Re: [PATCH v3 2/2] leds: lm3692x: Introduce LM3692x dual string
 driver

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@...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@....cz>
> 									Pavel
> 


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