Messages in this thread | | | Subject | Re: [PATCH v3 2/2] leds: lm3692x: Introduce LM3692x dual string driver | From | Dan Murphy <> | Date | Wed, 15 Nov 2017 15:25:42 -0600 |
| |
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
| |