| Message ID | 20230331114610.48111-3-maarten.zanders@mind.be |
|---|---|
| State | Superseded |
| Headers | show |
| Series | leds: lp55xx: configure internal charge pump | expand |
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@mind.be> > --- > > 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 [李琼斯]
> Sorry Maarten, just a couple of small nits. No need to be sorry. > This fits on one line, no? < 100-chars? I thought the line length was meant to be <80 chars according to: https://www.kernel.org/doc/html/latest/process/coding-style.html#breaking-long-lines-and-strings Happy to adapt if this shouldn't be taken as strict as I tried to do. Either way I'll post an update with a cleanup. > >> --- 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? No, it luckily doesn't get that big. This is to avoid an intermediate variable & casting as the DT parameter has to be 32bit (which I learned from an earlier comment). It was changed from u8 in v5 of the patch. Thanks for the review, Maarten
On Thu, 06 Apr 2023, Maarten Zanders wrote: > > > Sorry Maarten, just a couple of small nits. > > No need to be sorry. > > > > This fits on one line, no? < 100-chars? > > I thought the line length was meant to be <80 chars according to: > > https://www.kernel.org/doc/html/latest/process/coding-style.html#breaking-long-lines-and-strings > > Happy to adapt if this shouldn't be taken as strict as I tried to do. Either > way I'll post an update with a cleanup. checkpatch.pl now checks for >100-chars. > > > --- 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? > > No, it luckily doesn't get that big. This is to avoid an intermediate > variable & casting as the DT parameter has to be 32bit (which I learned from > an earlier comment). It was changed from u8 in v5 of the patch. Fair enough. -- Lee Jones [李琼斯]
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; + 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 |\ + 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; + + 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> + #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)) + 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); + 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; + 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; + /* optional enable GPIO */ struct gpio_desc *enable_gpiod;
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@mind.be> --- 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 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(-)