| 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: <20240620160348.GO3029315@google.com>
Date: Thu, 20 Jun 2024 17:03:48 +0100
From: Lee Jones <lee@...nel.org>
To: Christian Marangi <ansuelsmth@...il.com>
Cc: Pavel Machek <pavel@....cz>, Rob Herring <robh@...nel.org>,
Krzysztof Kozlowski <krzysztof.kozlowski+dt@...aro.org>,
Conor Dooley <conor+dt@...nel.org>,
Jacek Anaszewski <jacek.anaszewski@...il.com>,
linux-leds@...r.kernel.org, devicetree@...r.kernel.org,
linux-kernel@...r.kernel.org
Subject: Re: [PATCH v6 20/20] leds: leds-lp5569: Add support for Texas
Instruments LP5569
On Sun, 16 Jun 2024, Christian Marangi wrote:
> Add support for Texas Instruments LP5569 LED driver.
>
> Texas Instruments LP5569 is 9 channels chip with programmable engines.
>
> It almost a copy of LP5523 with fundamental changes to regs order and
> regs content.
>
> Has difference in how the clock is handled and doesn't support detecting
> clock time automatically, different handling for selftest and different
> scheme for the status regs.
>
> LED chip supports ENGINE and MUX to group LED and run precompiled code
> with magic values to run patterns. This is loaded via the sysfs entry
> and it's passed as a string of ASCII HEX char.
>
> One some devices using this LED Controller (a NBG7815 Router) it was
> found loading big precompiled pattern with up to 96 bytes of code. To
> have support for this "extended" scenario, hardcode each engine to
> support 4 pages of precompiled pattern (128 bytes of code) and 1 page
> for each MUX. This gives plenty of space for any kind precompiled
> pattern keeping simple logic for page handling of each engine and mux.
>
> Signed-off-by: Christian Marangi <ansuelsmth@...il.com>
> ---
> drivers/leds/Kconfig | 16 +-
> drivers/leds/Makefile | 1 +
> drivers/leds/leds-lp5569.c | 542 +++++++++++++++++++++++++++++++++++++
> 3 files changed, 556 insertions(+), 3 deletions(-)
> create mode 100644 drivers/leds/leds-lp5569.c
Pretty good, just a couple of tiny nits.
[...]
> +static ssize_t lp5569_led_short_test(struct lp55xx_led *led, char *buf)
> +{
> + struct lp55xx_chip *chip = led->chip;
> + struct lp55xx_platform_data *pdata = chip->pdata;
> + bool leds_fault[LP5569_MAX_LEDS];
> + struct lp55xx_led *led_tmp = led;
> + int i, ret, pos = 0;
> + u8 status;
> +
> + /* Set in STANDBY state */
> + ret = lp55xx_write(chip, LP5569_REG_ENABLE, 0);
> + if (ret)
> + goto exit;
> +
> + /* Wait 1ms for device to enter STANDBY state */
> + usleep_range(1000, 2000);
> +
> + /* Set Charge Pump to 1x */
> + ret = lp55xx_update_bits(chip, LP5569_REG_MISC,
> + FIELD_PREP(LP5569_CP_MODE_MASK, LP55XX_CP_BYPASS),
> + LP5569_CP_MODE_MASK);
> + if (ret)
> + goto exit;
> +
> + /* Enable LED and set to 100% brightness and current to 100% (25.5mA) */
> + for (i = 0; i < pdata->num_channels; i++) {
> + ret = lp55xx_write(chip, LP5569_REG_LED_PWM_BASE + led_tmp->chan_nr,
> + LED_FULL);
> + if (ret)
> + goto exit;
> +
> + ret = lp55xx_write(chip, LP5569_REG_LED_CURRENT_BASE + led_tmp->chan_nr,
> + LED_FULL);
> + if (ret)
> + goto exit;
> +
> + led_tmp++;
> + }
> +
> + /* Put Device in NORMAL state */
> + ret = lp55xx_write(chip, LP5569_REG_ENABLE, LP5569_ENABLE);
> + if (ret)
> + goto exit;
> +
> + /* Wait 500 us for device to enter NORMAL state */
> + usleep_range(500, 750);
> +
> + /* Enable LED Shorted Test */
> + ret = lp55xx_update_bits(chip, LP5569_REG_MISC2, LP5569_LED_OPEN_TEST,
> + LP5569_LED_SHORT_TEST);
> + if (ret)
> + goto exit;
> +
> + /* Wait 500 us for device to fill status regs */
> + usleep_range(500, 750);
> +
> + /* Parse status led fault 1 regs */
> + ret = lp55xx_read(chip, LP5569_REG_LED_FAULT1, &status);
> + if (ret < 0)
> + goto exit;
> +
> + for (i = 0; i < 8; i++)
> + leds_fault[i] = !!LEDn_STATUS_FAULT(i, status);
> +
> + /* Parse status led fault 2 regs */
> + ret = lp55xx_read(chip, LP5569_REG_LED_FAULT2, &status);
> + if (ret < 0)
> + goto exit;
> +
> + for (i = 0; i < 1; i++)
> + leds_fault[i + 8] = !!LEDn_STATUS_FAULT(i, status);
> +
> + /* Report LED fault */
> + led_tmp = led;
> + for (i = 0; i < pdata->num_channels; i++) {
> + if (leds_fault[led_tmp->chan_nr])
> + pos += sprintf(buf + pos, "LED %d SHORTED FAIL\n",
> + led_tmp->chan_nr);
> +
> + led_tmp++;
> + }
> +
> + ret = pos;
> +
> +exit:
> + /* Disable LED Shorted Test */
> + lp55xx_update_bits(chip, LP5569_REG_MISC2, LP5569_LED_SHORT_TEST,
> + 0);
Nit: This line break seems unnecessary.
> +
> + led_tmp = led;
> + for (i = 0; i < pdata->num_channels; i++) {
> + lp55xx_write(chip, LP5569_REG_LED_PWM_BASE + led_tmp->chan_nr,
> + 0);
Nit: This too.
> +
> + led_tmp++;
> + }
> +
> + return ret;
> +}
> +
> +static ssize_t lp5569_selftest(struct device *dev,
> + struct device_attribute *attr,
> + char *buf)
> +{
> + struct lp55xx_led *led = i2c_get_clientdata(to_i2c_client(dev));
> + struct lp55xx_chip *chip = led->chip;
> + int i, pos = 0;
> +
> + mutex_lock(&chip->lock);
> +
> + /* Test LED Open */
> + pos = lp5569_led_open_test(led, buf);
> + if (pos < 0)
> + goto fail;
> +
> + /* Test LED Shorted */
> + pos = lp5569_led_short_test(led, buf);
> + if (pos < 0)
> + goto fail;
> +
> + for (i = 0; i < chip->pdata->num_channels; i++) {
> + /* Restore current */
> + lp55xx_write(chip, LP5569_REG_LED_CURRENT_BASE + led->chan_nr,
> + led->led_current);
> +
> + /* Restore brightness */
> + lp55xx_write(chip, LP5569_REG_LED_PWM_BASE + led->chan_nr,
> + led->brightness);
> + led++;
> + }
> +
> + if (pos == 0)
> + pos = sprintf(buf, "OK\n");
> + goto release_lock;
> +fail:
> + pos = sprintf(buf, "FAIL\n");
> +
> +release_lock:
> + mutex_unlock(&chip->lock);
> +
> + return pos;
> +}
> +
> +LP55XX_DEV_ATTR_ENGINE_MODE(1);
> +LP55XX_DEV_ATTR_ENGINE_MODE(2);
> +LP55XX_DEV_ATTR_ENGINE_MODE(3);
> +LP55XX_DEV_ATTR_ENGINE_LEDS(1);
> +LP55XX_DEV_ATTR_ENGINE_LEDS(2);
> +LP55XX_DEV_ATTR_ENGINE_LEDS(3);
> +LP55XX_DEV_ATTR_ENGINE_LOAD(1);
> +LP55XX_DEV_ATTR_ENGINE_LOAD(2);
> +LP55XX_DEV_ATTR_ENGINE_LOAD(3);
> +static LP55XX_DEV_ATTR_RO(selftest, lp5569_selftest);
> +LP55XX_DEV_ATTR_MASTER_FADER(1);
> +LP55XX_DEV_ATTR_MASTER_FADER(2);
> +LP55XX_DEV_ATTR_MASTER_FADER(3);
> +static LP55XX_DEV_ATTR_RW(master_fader_leds, lp55xx_show_master_fader_leds,
> + lp55xx_store_master_fader_leds);
> +
> +static struct attribute *lp5569_attributes[] = {
> + &dev_attr_engine1_mode.attr,
> + &dev_attr_engine2_mode.attr,
> + &dev_attr_engine3_mode.attr,
> + &dev_attr_engine1_load.attr,
> + &dev_attr_engine2_load.attr,
> + &dev_attr_engine3_load.attr,
> + &dev_attr_engine1_leds.attr,
> + &dev_attr_engine2_leds.attr,
> + &dev_attr_engine3_leds.attr,
> + &dev_attr_selftest.attr,
> + &dev_attr_master_fader1.attr,
> + &dev_attr_master_fader2.attr,
> + &dev_attr_master_fader3.attr,
> + &dev_attr_master_fader_leds.attr,
> + NULL,
> +};
> +
> +static const struct attribute_group lp5569_group = {
> + .attrs = lp5569_attributes,
> +};
> +
> +/* Chip specific configurations */
> +static struct lp55xx_device_config lp5569_cfg = {
> + .reg_op_mode = {
> + .addr = LP5569_REG_OP_MODE,
> + .shift = LP5569_MODE_ENG_SHIFT,
> + },
> + .reg_exec = {
> + .addr = LP5569_REG_EXEC_CTRL,
> + .shift = LP5569_EXEC_ENG_SHIFT,
> + },
> + .reset = {
> + .addr = LP5569_REG_RESET,
> + .val = LP5569_RESET,
> + },
> + .enable = {
> + .addr = LP5569_REG_ENABLE,
> + .val = LP5569_ENABLE,
> + },
> + .prog_mem_base = {
> + .addr = LP5569_REG_PROG_MEM,
> + },
> + .reg_led_pwm_base = {
> + .addr = LP5569_REG_LED_PWM_BASE,
> + },
> + .reg_led_current_base = {
> + .addr = LP5569_REG_LED_CURRENT_BASE,
> + },
> + .pages_per_engine = LP5569_PAGES_PER_ENGINE,
> + .max_channel = LP5569_MAX_LEDS,
> + .post_init_device = lp5569_post_init_device,
> + .brightness_fn = lp55xx_led_brightness,
> + .multicolor_brightness_fn = lp55xx_multicolor_brightness,
> + .set_led_current = lp55xx_set_led_current,
> + .firmware_cb = lp55xx_firmware_loaded_cb,
> + .run_engine = lp5569_run_engine,
> + .dev_attr_group = &lp5569_group,
> +};
> +
> +static const struct i2c_device_id lp5569_id[] = {
> + { "lp5569", .driver_data = (kernel_ulong_t)&lp5569_cfg, },
> + { }
> +};
> +
> +MODULE_DEVICE_TABLE(i2c, lp5569_id);
> +
> +static const struct of_device_id of_lp5569_leds_match[] = {
> + { .compatible = "ti,lp5569", .data = &lp5569_cfg, },
> + {},
> +};
> +
> +MODULE_DEVICE_TABLE(of, of_lp5569_leds_match);
> +
> +static struct i2c_driver lp5569_driver = {
> + .driver = {
> + .name = "lp5569x",
> + .of_match_table = of_lp5569_leds_match,
> + },
> + .probe = lp55xx_probe,
> + .remove = lp55xx_remove,
> + .id_table = lp5569_id,
> +};
> +
> +module_i2c_driver(lp5569_driver);
> +
> +MODULE_AUTHOR("Christian Marangi <ansuelsmth@...il.com>");
> +MODULE_DESCRIPTION("LP5569 LED engine");
> +MODULE_LICENSE("GPL");
> --
> 2.43.0
>
--
Lee Jones [李琼斯]
Powered by blists - more mailing lists