| 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
| ||
|
Message-ID: <Y8qdX7QIQntPWuuA@google.com>
Date: Fri, 20 Jan 2023 13:55:43 +0000
From: Lee Jones <lee@...nel.org>
To: Maarten Zanders <maarten.zanders@...d.be>
Cc: Pavel Machek <pavel@....cz>, krzysztof.kozlowski@...aro.org,
linux-leds@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH v2 2/2] leds: lp55xx: configure internal charge pump
On Tue, 10 Jan 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@...d.be>
> ---
> drivers/leds/leds-lp5521.c | 12 ++++++------
> drivers/leds/leds-lp5523.c | 18 +++++++++++++-----
> drivers/leds/leds-lp55xx-common.c | 22 ++++++++++++++++++++++
> drivers/leds/leds-lp8501.c | 8 ++++++--
> include/linux/platform_data/leds-lp55xx.h | 9 +++++++++
> 5 files changed, 56 insertions(+), 13 deletions(-)
>
> diff --git a/drivers/leds/leds-lp5521.c b/drivers/leds/leds-lp5521.c
> index 19478d9c19a7..ee1c72cffdab 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..a690a484fd7b 100644
> --- a/drivers/leds/leds-lp55xx-common.c
> +++ b/drivers/leds/leds-lp55xx-common.c
> @@ -653,6 +653,13 @@ static int lp55xx_parse_logical_led(struct device_node *np,
> return ret;
> }
>
> +static const char * const charge_pump_modes[] = {
> + [LP55XX_CP_OFF] = "off",
> + [LP55XX_CP_BYPASS] = "bypass",
> + [LP55XX_CP_BOOST] = "boost",
> + [LP55XX_CP_AUTO] = "auto",
> +};
> +
> struct lp55xx_platform_data *lp55xx_of_populate_pdata(struct device *dev,
> struct device_node *np,
> struct lp55xx_chip *chip)
> @@ -661,6 +668,8 @@ struct lp55xx_platform_data *lp55xx_of_populate_pdata(struct device *dev,
> struct lp55xx_platform_data *pdata;
> struct lp55xx_led_config *cfg;
> int num_channels;
> + enum lp55xx_charge_pump_mode cp_mode;
> + const char *pm;
> int i = 0;
> int ret;
>
> @@ -691,6 +700,19 @@ struct lp55xx_platform_data *lp55xx_of_populate_pdata(struct device *dev,
> i++;
> }
>
> + pdata->charge_pump_mode = LP55XX_CP_AUTO;
> + ret = of_property_read_string(np, "ti,charge-pump-mode", &pm);
> + if (!ret) {
> + for (cp_mode = LP55XX_CP_OFF;
> + cp_mode < ARRAY_SIZE(charge_pump_modes);
> + cp_mode++) {
> + if (!strcasecmp(pm, charge_pump_modes[cp_mode])) {
> + pdata->charge_pump_mode = cp_mode;
> + break;
> + }
> + }
> + }
A little over-engineered, no?
Why not make the property a numerical value, then simply:
ret = of_property_read_u32(np, "ti,charge-pump-mode", &pdata->charge_pump_mode);
if (ret)
data->charge_pump_mode = LP55XX_CP_AUTO;
Elevates the requirement for the crumby indexed array of strings above.
Remainder looks sane enough.
--
Lee Jones [李琼斯]
Powered by blists - more mailing lists