Re: [PATCH 5/5] leds: lm3697: Introduce the lm3697 driver

[Dan Murphy <dmurphy@ti.com>]

Pavel

On 4/13/19 3:06 PM, Pavel Machek wrote:
> On Mon 2019-03-25 09:24:03, Dan Murphy wrote:
>> Introduce the lm3697 LED driver for
>> backlighting and display.
>>
>> Datasheet location:
>> http://www.ti.com/lit/ds/symlink/lm3697.pdf
>>
>> Signed-off-by: Dan Murphy <dmurphy@ti.com>
>> ---
>>  drivers/leds/Kconfig       |   8 +-
>>  drivers/leds/Makefile      |   1 +
>>  drivers/leds/leds-lm3697.c | 401 +++++++++++++++++++++++++++++++++++++
>>  3 files changed, 409 insertions(+), 1 deletion(-)
>>  create mode 100644 drivers/leds/leds-lm3697.c
>>
>> diff --git a/drivers/leds/Kconfig b/drivers/leds/Kconfig
>> index 735009e73414..688bb9a6f275 100644
>> --- a/drivers/leds/Kconfig
>> +++ b/drivers/leds/Kconfig
>> @@ -776,9 +776,15 @@ config LEDS_NIC78BX
>>  	  To compile this driver as a module, choose M here: the module
>>  	  will be called leds-nic78bx.
>>  
>> +config LEDS_LM3697
>> +	tristate "LED driver for LM3697"
>> +	depends on LEDS_TI_LMU_COMMON
>> +	help
>> +	  Say Y to enable the LM3697 LED driver for TI LMU devices.
>> +	  This supports the LED device LM3697.
>> +
>>  config LEDS_TI_LMU_COMMON
>>  	tristate "LED driver for TI LMU"
>> -	depends on REGMAP
>>  	help
>>            Say Y to enable the LED driver for TI LMU devices.
>>            This supports common features between the TI LM3532, LM3631, LM3632,
> 
> Is deleting "depends on REGMAP" intentional? AFAICT you are using it.
> 

Thanks for pointing that out.  I don't know how that was in there.

> Plus we'd normally expect "COMMON" first and then specific driver. Not
> sure if Kconfig can handle it out-of-order...
> 
> 

OK.  Should I rename the ti_lmu file to leds-common-ti-lmu?

This keeps the naming convention the same in the leds directory as well.

FYI I will not add your acked-by on the LMU patch that introduced the code unless you approve.
Since you found issues with the kconfig

Refererence
https://lore.kernel.org/patchwork/patch/1054500/

>> +static int lm3697_init(struct lm3697 *priv)
>> +{
>> +	struct lm3697_led *led;
>> +	int i, ret;
>> +
>> +	if (priv->enable_gpio) {
>> +		gpiod_direction_output(priv->enable_gpio, 1);
>> +	} else {
>> +		ret = regmap_write(priv->regmap, LM3697_RESET, LM3697_SW_RESET);
>> +		if (ret) {
>> +			dev_err(&priv->client->dev, "Cannot reset the device\n");
>> +			goto out;
>> +		}
>> +	}
>> +
>> +	ret = regmap_write(priv->regmap, LM3697_CTRL_ENABLE, 0x0);
>> +	if (ret) {
>> +		dev_err(&priv->client->dev, "Cannot write ctrl enable\n");
>> +		goto out;
>> +	}
>> +
>> +	ret = regmap_write(priv->regmap, LM3697_OUTPUT_CONFIG, priv->bank_cfg);
>> +	if (ret)
>> +		dev_err(&priv->client->dev, "Cannot write OUTPUT config\n");
> 
> Missing goto out?

Ack

> 
>> +	for (i = 0; i < LM3697_MAX_CONTROL_BANKS; i++) {
>> +		led = &priv->leds[i];
>> +		ret = ti_lmu_common_set_ramp(&led->lmu_data);
>> +		if (ret)
>> +			dev_err(&priv->client->dev, "Setting the ramp rate failed\n");
>> +	}
>> +out:
>> +	return ret;
>> +}
> 									Pavel
>