Hi,
On 3/22/23 17:09, Daniel Scally wrote:
> The TPS68470 PMIC provides a third LED driver in addition to the two
> indicator LEDs. Add support for the WLED. To ensure the LED is active
> for as long as the kernel instructs it to be we need to re-trigger it
> periodically to avoid the IC's internal timeouts.
>
> Signed-off-by: Daniel Scally <dan.scally@xxxxxxxxxxxxxxxx>
> ---
> drivers/leds/leds-tps68470.c | 102 ++++++++++++++++++++++++++++++++++-
> include/linux/mfd/tps68470.h | 12 +++++
> 2 files changed, 113 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/leds/leds-tps68470.c b/drivers/leds/leds-tps68470.c
> index 44df175d25de..abcd3494b1a8 100644
> --- a/drivers/leds/leds-tps68470.c
> +++ b/drivers/leds/leds-tps68470.c
> @@ -8,6 +8,7 @@
> * Kate Hsuan <hpa@xxxxxxxxxx>
> */
>
> +#include <linux/clk.h>
> #include <linux/leds.h>
> #include <linux/mfd/tps68470.h>
> #include <linux/module.h>
> @@ -15,7 +16,10 @@
> #include <linux/platform_device.h>
> #include <linux/property.h>
> #include <linux/regmap.h>
> +#include <linux/workqueue.h>
>
> +#define work_to_led(work) \
> + container_of(work, struct tps68470_led, keepalive_work)
>
> #define lcdev_to_led(led_cdev) \
> container_of(led_cdev, struct tps68470_led, lcdev)
> @@ -26,20 +30,25 @@
> enum tps68470_led_ids {
> TPS68470_ILED_A,
> TPS68470_ILED_B,
> + TPS68470_WLED,
> TPS68470_NUM_LEDS
> };
>
> static const char *tps68470_led_names[] = {
> [TPS68470_ILED_A] = "tps68470-iled_a",
> [TPS68470_ILED_B] = "tps68470-iled_b",
> + [TPS68470_WLED] = "tps68470-wled",
> };
>
> struct tps68470_led {
> unsigned int led_id;
> struct led_classdev lcdev;
> + enum led_brightness state;
> + struct work_struct keepalive_work;
The way this is used does not look very periodical, it seems to just run as fast as it can?
IMHO this should use a delayed-work item and then queue that to run at a certain interval.
> };
>
> struct tps68470_device {
> + struct clk *clk;
> struct device *dev;
> struct regmap *regmap;
> struct tps68470_led leds[TPS68470_NUM_LEDS];
> @@ -52,11 +61,33 @@ enum ctrlb_current {
> CTRLB_16MA = 3,
> };
>
> +/*
> + * The WLED can operate in different modes, including a Flash and Torch mode. In
> + * each mode there's a timeout which ranges from a matter of milliseconds to up
> + * to 13 seconds. We don't want that timeout to apply though because the LED
> + * should be lit until we say that it should no longer be lit, re-trigger the
> + * LED periodically to keep it alive.
> + */
> +static void tps68470_wled_keepalive_work(struct work_struct *work)
> +{
> + struct tps68470_device *tps68470;
> + struct tps68470_led *led;
> +
> + led = work_to_led(work);
> + tps68470 = led_to_tps68470(led, led->led_id);
> +
> + regmap_update_bits_async(tps68470->regmap, TPS68470_REG_WLEDCTL,
> + TPS68470_WLED_CTL_MASK, TPS68470_WLED_CTL_MASK);
> + schedule_work(&led->keepalive_work);
> +}
> +
> static int tps68470_brightness_set(struct led_classdev *led_cdev, enum led_brightness brightness)
> {
> struct tps68470_led *led = lcdev_to_led(led_cdev);
> struct tps68470_device *tps68470 = led_to_tps68470(led, led->led_id);
> struct regmap *regmap = tps68470->regmap;
> + const char *errmsg;
> + int ret;
>
> switch (led->led_id) {
> case TPS68470_ILED_A:
> @@ -65,8 +96,59 @@ static int tps68470_brightness_set(struct led_classdev *led_cdev, enum led_brigh
> case TPS68470_ILED_B:
> return regmap_update_bits(regmap, TPS68470_REG_ILEDCTL, TPS68470_ILEDCTL_ENB,
> brightness ? TPS68470_ILEDCTL_ENB : 0);
> + case TPS68470_WLED:
> + /*
> + * LED core does not prevent re-setting brightness to its current
> + * value; we need to do so here to avoid unbalanced calls to clk
> + * enable/disable.
> + */
> + if (led->state == brightness)
> + return 0;
> +
> + if (brightness) {
> + schedule_work(&led->keepalive_work);
> +
> + ret = clk_prepare_enable(tps68470->clk);
> + if (ret) {
> + errmsg = "failed to start clock\n";
> + goto err_cancel_work;
> + }
> + } else {
> + cancel_work_sync(&led->keepalive_work);
> + clk_disable_unprepare(tps68470->clk);
> + }
> +
> + ret = regmap_update_bits(tps68470->regmap, TPS68470_REG_WLEDCTL,
> + TPS68470_WLED_EN_MASK,
> + brightness ? TPS68470_WLED_EN_MASK :
> + ~TPS68470_WLED_EN_MASK);
> + if (ret) {
> + errmsg = "failed to set WLED EN\n";
> + goto err_disable_clk;
> + }
> +
> + ret = regmap_update_bits(tps68470->regmap, TPS68470_REG_WLEDCTL,
> + TPS68470_WLED_CTL_MASK,
> + brightness ? TPS68470_WLED_CTL_MASK :
> + ~TPS68470_WLED_CTL_MASK);
> + if (ret) {
> + errmsg = "failed to set WLED START\n";
> + goto err_disable_clk;
> + }
> +
> + led->state = brightness;
> + break;
> + default:
> + return dev_err_probe(led_cdev->dev, -EINVAL, "invalid LED ID\n");
> }
> - return -EINVAL;
> +
> + return ret;
> +
> +err_disable_clk:
> + clk_disable_unprepare(tps68470->clk);
> +err_cancel_work:
> + cancel_work_sync(&led->keepalive_work);
> + return dev_err_probe(tps68470->dev, ret, errmsg);
> }
>
> static enum led_brightness tps68470_brightness_get(struct led_classdev *led_cdev)
> @@ -88,6 +170,14 @@ static enum led_brightness tps68470_brightness_get(struct led_classdev *led_cdev
> value &= led->led_id == TPS68470_ILED_A ? TPS68470_ILEDCTL_ENA :
> TPS68470_ILEDCTL_ENB;
> break;
> + case TPS68470_WLED:
> + ret = regmap_read(regmap, TPS68470_REG_WLEDCTL, &value);
> + if (ret)
> + return dev_err_probe(led_cdev->dev, ret,
> + "failed to read LED status\n");
> +
> + value &= TPS68470_WLED_CTL_MASK;
> + break;
> default:
> return dev_err_probe(led_cdev->dev, -EINVAL, "invalid LED ID\n");
> }
> @@ -177,6 +267,11 @@ static int tps68470_leds_probe(struct platform_device *pdev)
> tps68470->dev = &pdev->dev;
> tps68470->regmap = dev_get_drvdata(pdev->dev.parent);
>
> + tps68470->clk = devm_clk_get(tps68470->dev, NULL);
> + if (IS_ERR(tps68470->clk))
> + return dev_err_probe(tps68470->dev, PTR_ERR(tps68470->clk),
> + "failed to get clock\n");
> +
> for (i = 0; i < TPS68470_NUM_LEDS; i++) {
> led = &tps68470->leds[i];
> lcdev = &led->lcdev;
> @@ -206,6 +301,11 @@ static int tps68470_leds_probe(struct platform_device *pdev)
> if (ret)
> goto err_exit;
> }
> +
> + if (led->led_id == TPS68470_WLED) {
> + INIT_WORK(&led->keepalive_work,
> + tps68470_wled_keepalive_work);
> + }
> }
>
> ret = tps68470_leds_init(tps68470);
> diff --git a/include/linux/mfd/tps68470.h b/include/linux/mfd/tps68470.h
> index 2d2abb25b944..103ff730e028 100644
> --- a/include/linux/mfd/tps68470.h
> +++ b/include/linux/mfd/tps68470.h
> @@ -35,6 +35,11 @@
> #define TPS68470_REG_GPDI 0x26
> #define TPS68470_REG_GPDO 0x27
> #define TPS68470_REG_ILEDCTL 0x28
> +#define TPS68470_REG_WLEDMAXF 0x2F
> +#define TPS68470_REG_WLEDTO 0x30
> +#define TPS68470_REG_WLEDC1 0x34
> +#define TPS68470_REG_WLEDC2 0x35
> +#define TPS68470_REG_WLEDCTL 0x36
> #define TPS68470_REG_VCMVAL 0x3C
> #define TPS68470_REG_VAUX1VAL 0x3D
> #define TPS68470_REG_VAUX2VAL 0x3E
> @@ -98,5 +103,12 @@
> #define TPS68470_ILEDCTL_ENA BIT(2)
> #define TPS68470_ILEDCTL_ENB BIT(6)
> #define TPS68470_ILEDCTL_CTRLB GENMASK(5, 4)
> +#define TPS68470_WLEDMAXF_MAX_CUR_MASK GENMASK(4, 0)
> +#define TPS68470_WLEDC_ILED_MASK GENMASK(4, 0)
> +#define TPS68470_WLED_MODE_MASK GENMASK(1, 0)
> +#define TPS68470_WLED_EN_MASK BIT(2)
> +#define TPS68470_WLED_DISLED1 BIT(3)
> +#define TPS68470_WLED_DISLED2 BIT(4)
> +#define TPS68470_WLED_CTL_MASK BIT(5)
>
> #endif /* __LINUX_MFD_TPS68470_H */
Otherwise this LGTM.
Regards,
Hans
