Re: [PATCH v6 2/2] leds: lp55xx: configure internal charge pump

[Lee Jones]

On Fri, 31 Mar 2023, Maarten Zanders wrote:

> The LP55xx range of devices have an internal charge pump which
> can (automatically) increase the output voltage towards the
> LED's, boosting the output voltage to 4.5V.
>
> Implement this option from the devicetree. When the setting
> is not present it will operate in automatic mode as before.
>
> Tested on LP55231. Datasheet analysis shows that LP5521, LP5523
> and LP8501 are identical in topology and are modified in the
> same way.
>
> Signed-off-by: Maarten Zanders <maarten.zanders@xxxxxxx>
> ---
>
> Notes:
>     v1: implement as bool to disable charge pump
>     v2: rewrite to use string configuration, supporting all modes
>     v3: simplification by replacing string from DTS by constant
>     v4: added notes
>     v5: property type to u32
>     v6: cleanup parsing of DT paramter

Sorry Maarten, just a couple of small nits.

>  drivers/leds/leds-lp5521.c                | 12 ++++++------
>  drivers/leds/leds-lp5523.c                | 18 +++++++++++++-----
>  drivers/leds/leds-lp55xx-common.c         | 14 ++++++++++++++
>  drivers/leds/leds-lp8501.c                |  8 ++++++--
>  include/linux/platform_data/leds-lp55xx.h |  3 +++
>  5 files changed, 39 insertions(+), 13 deletions(-)
>
> diff --git a/drivers/leds/leds-lp5521.c b/drivers/leds/leds-lp5521.c
> index 19478d9c19a7..76c6b81afb38 100644
> --- a/drivers/leds/leds-lp5521.c
> +++ b/drivers/leds/leds-lp5521.c
> @@ -58,14 +58,11 @@
>  /* CONFIG register */
>  #define LP5521_PWM_HF			0x40	/* PWM: 0 = 256Hz, 1 = 558Hz */
>  #define LP5521_PWRSAVE_EN		0x20	/* 1 = Power save mode */
> -#define LP5521_CP_MODE_OFF		0	/* Charge pump (CP) off */
> -#define LP5521_CP_MODE_BYPASS		8	/* CP forced to bypass mode */
> -#define LP5521_CP_MODE_1X5		0x10	/* CP forced to 1.5x mode */
> -#define LP5521_CP_MODE_AUTO		0x18	/* Automatic mode selection */
> +#define LP5521_CP_MODE_MASK		0x18	/* Charge pump mode */
> +#define LP5521_CP_MODE_SHIFT		3
>  #define LP5521_R_TO_BATT		0x04	/* R out: 0 = CP, 1 = Vbat */
>  #define LP5521_CLK_INT			0x01	/* Internal clock */
> -#define LP5521_DEFAULT_CFG		\
> -	(LP5521_PWM_HF | LP5521_PWRSAVE_EN | LP5521_CP_MODE_AUTO)
> +#define LP5521_DEFAULT_CFG		(LP5521_PWM_HF | LP5521_PWRSAVE_EN)
>
>  /* Status */
>  #define LP5521_EXT_CLK_USED		0x08
> @@ -310,6 +307,9 @@ static int lp5521_post_init_device(struct lp55xx_chip *chip)
>  	if (!lp55xx_is_extclk_used(chip))
>  		val |= LP5521_CLK_INT;
>
> +	val |= (chip->pdata->charge_pump_mode << LP5521_CP_MODE_SHIFT) &
> +		LP5521_CP_MODE_MASK;
> +

This fits on one line, no?  < 100-chars?

>  	ret = lp55xx_write(chip, LP5521_REG_CONFIG, val);
>  	if (ret)
>  		return ret;
> diff --git a/drivers/leds/leds-lp5523.c b/drivers/leds/leds-lp5523.c
> index e08e3de1428d..b5d10d4252e6 100644
> --- a/drivers/leds/leds-lp5523.c
> +++ b/drivers/leds/leds-lp5523.c
> @@ -57,8 +57,12 @@
>  #define LP5523_AUTO_INC			0x40
>  #define LP5523_PWR_SAVE			0x20
>  #define LP5523_PWM_PWR_SAVE		0x04
> -#define LP5523_CP_AUTO			0x18
> +#define LP5523_CP_MODE_MASK		0x18
> +#define LP5523_CP_MODE_SHIFT		3
>  #define LP5523_AUTO_CLK			0x02
> +#define LP5523_DEFAULT_CONFIG	\
> +	(LP5523_AUTO_INC | LP5523_PWR_SAVE |\

Space after the | please.

Perhaps even tab the '\'s out to align.

Or perhaps the below line can go on the one above.

> +	 LP5523_AUTO_CLK | LP5523_PWM_PWR_SAVE)
>
>  #define LP5523_EN_LEDTEST		0x80
>  #define LP5523_LEDTEST_DONE		0x80
> @@ -125,6 +129,7 @@ static void lp5523_set_led_current(struct lp55xx_led *led, u8 led_current)
>  static int lp5523_post_init_device(struct lp55xx_chip *chip)
>  {
>  	int ret;
> +	int val;
>
>  	ret = lp55xx_write(chip, LP5523_REG_ENABLE, LP5523_ENABLE);
>  	if (ret)
> @@ -133,10 +138,13 @@ static int lp5523_post_init_device(struct lp55xx_chip *chip)
>  	/* Chip startup time is 500 us, 1 - 2 ms gives some margin */
>  	usleep_range(1000, 2000);
>
> -	ret = lp55xx_write(chip, LP5523_REG_CONFIG,
> -			    LP5523_AUTO_INC | LP5523_PWR_SAVE |
> -			    LP5523_CP_AUTO | LP5523_AUTO_CLK |
> -			    LP5523_PWM_PWR_SAVE);
> +	val = LP5523_DEFAULT_CONFIG;
> +
> +	val |= (chip->pdata->charge_pump_mode << LP5523_CP_MODE_SHIFT) &
> +	       LP5523_CP_MODE_MASK;

One line.

> +
> +	ret = lp55xx_write(chip, LP5523_REG_CONFIG, val);
> +
>  	if (ret)
>  		return ret;
>
> diff --git a/drivers/leds/leds-lp55xx-common.c b/drivers/leds/leds-lp55xx-common.c
> index c1940964067a..5a02c4a4ec98 100644
> --- a/drivers/leds/leds-lp55xx-common.c
> +++ b/drivers/leds/leds-lp55xx-common.c
> @@ -19,6 +19,8 @@
>  #include <linux/slab.h>
>  #include <linux/gpio/consumer.h>
>
> +#include <dt-bindings/leds/leds-lp55xx.h>

This can be part of the main block.

> +
>  #include "leds-lp55xx-common.h"
>
>  /* External clock rate */
> @@ -691,6 +693,16 @@ struct lp55xx_platform_data *lp55xx_of_populate_pdata(struct device *dev,
>  		i++;
>  	}
>
> +	if (of_property_read_u32(np, "ti,charge-pump-mode",
> +				 &pdata->charge_pump_mode))

One line?

> +		pdata->charge_pump_mode = LP55XX_CP_AUTO;
> +
> +	if (pdata->charge_pump_mode > LP55XX_CP_AUTO) {
> +		dev_err(dev, "invalid charge pump mode %d\n",
> +			pdata->charge_pump_mode);

As above.

> +		return ERR_PTR(-EINVAL);
> +	}
> +
>  	of_property_read_string(np, "label", &pdata->label);
>  	of_property_read_u8(np, "clock-mode", &pdata->clock_mode);
>
> diff --git a/drivers/leds/leds-lp8501.c b/drivers/leds/leds-lp8501.c
> index ae11a02c0ab2..f0e70e116919 100644
> --- a/drivers/leds/leds-lp8501.c
> +++ b/drivers/leds/leds-lp8501.c
> @@ -53,10 +53,11 @@
>  #define LP8501_PWM_PSAVE		BIT(7)
>  #define LP8501_AUTO_INC			BIT(6)
>  #define LP8501_PWR_SAVE			BIT(5)
> -#define LP8501_CP_AUTO			0x18
> +#define LP8501_CP_MODE_MASK		0x18
> +#define LP8501_CP_MODE_SHIFT		3
>  #define LP8501_INT_CLK			BIT(0)
>  #define LP8501_DEFAULT_CFG	\
> -	(LP8501_PWM_PSAVE | LP8501_AUTO_INC | LP8501_PWR_SAVE | LP8501_CP_AUTO)
> +	(LP8501_PWM_PSAVE | LP8501_AUTO_INC | LP8501_PWR_SAVE)
>
>  #define LP8501_REG_RESET		0x3D
>  #define LP8501_RESET			0xFF
> @@ -102,6 +103,9 @@ static int lp8501_post_init_device(struct lp55xx_chip *chip)
>  	if (chip->pdata->clock_mode != LP55XX_CLOCK_EXT)
>  		val |= LP8501_INT_CLK;
>
> +	val |= (chip->pdata->charge_pump_mode << LP8501_CP_MODE_SHIFT) &
> +	       LP8501_CP_MODE_MASK;
> +

As above.

>  	ret = lp55xx_write(chip, LP8501_REG_CONFIG, val);
>  	if (ret)
>  		return ret;
> diff --git a/include/linux/platform_data/leds-lp55xx.h b/include/linux/platform_data/leds-lp55xx.h
> index 3441064713a3..3cc8db0b12b5 100644
> --- a/include/linux/platform_data/leds-lp55xx.h
> +++ b/include/linux/platform_data/leds-lp55xx.h
> @@ -73,6 +73,9 @@ struct lp55xx_platform_data {
>  	/* Clock configuration */
>  	u8 clock_mode;
>
> +	/* Charge pump mode */
> +	u32 charge_pump_mode;

That's a lot of data.  Does it need to be u32?

> +
>  	/* optional enable GPIO */
>  	struct gpio_desc *enable_gpiod;
>
> --
> 2.37.3
>

--
Lee Jones [李琼斯]