| Message ID | 20230322160926.948687-7-dan.scally@ideasonboard.com |
|---|---|
| State | New |
| Headers | show |
| Series | Add WLED support to TPS68470 LED driver | expand |
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@ideasonboard.com> > --- > 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@redhat.com> > */ > > +#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
On Wed 2023-03-22 16:09:24, 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. Wow. No! If hardware does not wart you to burn the LED, it is not okay to just work around that. These are not designed for continuous operation. > diff --git a/drivers/leds/leds-tps68470.c b/drivers/leds/leds-tps68470.c > index 44df175d25de..abcd3494b1a8 100644 Fun sha1 hash ;-). > @@ -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. > + */ We don't want the LED to overheat. That takes precedence. Find out what are the maximum limits for on time at various current levels. LED framework should be used for torch mode, with current set such that unlimited operation is safe. V4L2 should be used for flash mode. BR, Pavel
Hi Pavel, thanks for taking a look On 23/03/2023 11:22, Pavel Machek wrote: > On Wed 2023-03-22 16:09:24, 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. > Wow. No! > > If hardware does not wart you to burn the LED, it is not okay to just > work around that. These are not designed for continuous operation. > >> diff --git a/drivers/leds/leds-tps68470.c b/drivers/leds/leds-tps68470.c >> index 44df175d25de..abcd3494b1a8 100644 > Fun sha1 hash ;-). heh yeh > >> @@ -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. >> + */ > We don't want the LED to overheat. That takes precedence. > > Find out what are the maximum limits for on time at various current > levels. LED framework should be used for torch mode, with current set > such that unlimited operation is safe. V4L2 should be used for flash > mode. I did it this way because this is how the IC operates on my device whilst it's booted to Windows...but I suppose given they don't expose the LED outside of their Hello auth thing they can guarantee it's not being lit for too long - I confess that hadn't occurred to me. Anyway; I'll update this to re-trigger if the IC is in torch mode within the timeout (which the datasheet explicitly says you can do in torch mode; the current is much more heavily limited in that mode) and in the flash mode to update the brightness setting to 0 once the timeout expires so it reflects the actual state of the LED. Does that sound ok? > > BR, > Pavel
Hi, On 3/23/23 17:25, Dan Scally wrote: > Hi Pavel, thanks for taking a look > > On 23/03/2023 11:22, Pavel Machek wrote: >> On Wed 2023-03-22 16:09:24, 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. >> Wow. No! >> >> If hardware does not wart you to burn the LED, it is not okay to just >> work around that. These are not designed for continuous operation. >> >>> diff --git a/drivers/leds/leds-tps68470.c b/drivers/leds/leds-tps68470.c >>> index 44df175d25de..abcd3494b1a8 100644 >> Fun sha1 hash ;-). > > > heh yeh > >> >>> @@ -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. >>> + */ >> We don't want the LED to overheat. That takes precedence. >> >> Find out what are the maximum limits for on time at various current >> levels. LED framework should be used for torch mode, with current set >> such that unlimited operation is safe. V4L2 should be used for flash >> mode. > > > I did it this way because this is how the IC operates on my device whilst it's booted to Windows...but I suppose given they don't expose the LED outside of their Hello auth thing they can guarantee it's not being lit for too long - I confess that hadn't occurred to me. Anyway; I'll update this to re-trigger if the IC is in torch mode within the timeout (which the datasheet explicitly says you can do in torch mode; the current is much more heavily limited in that mode) and in the flash mode to update the brightness setting to 0 once the timeout expires so it reflects the actual state of the LED. Does that sound ok? That sounds reasonable to me. Regards, Hans
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@redhat.com> */ +#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; }; 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 */
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@ideasonboard.com> --- drivers/leds/leds-tps68470.c | 102 ++++++++++++++++++++++++++++++++++- include/linux/mfd/tps68470.h | 12 +++++ 2 files changed, 113 insertions(+), 1 deletion(-)