* [PATCH] leds-bcm6328: support second hw blinking interval @ 2020-04-24 12:46 Álvaro Fernández Rojas 2020-04-24 13:32 ` [PATCH v2] " Álvaro Fernández Rojas 0 siblings, 1 reply; 14+ messages in thread From: Álvaro Fernández Rojas @ 2020-04-24 12:46 UTC (permalink / raw) To: linux-leds, jacek.anaszewski, jonas.gorski, rpurdie, pavel Cc: Álvaro Fernández Rojas Add support for both configurable HW blinking intervals. Signed-off-by: Álvaro Fernández Rojas <noltari@gmail.com> --- drivers/leds/leds-bcm6328.c | 54 ++++++++++++++++++++++++++----------- 1 file changed, 38 insertions(+), 16 deletions(-) diff --git a/drivers/leds/leds-bcm6328.c b/drivers/leds/leds-bcm6328.c index 42e1b7598c3a..beb1aaff68f6 100644 --- a/drivers/leds/leds-bcm6328.c +++ b/drivers/leds/leds-bcm6328.c @@ -24,12 +24,16 @@ #define BCM6328_LED_MAX_COUNT 24 #define BCM6328_LED_DEF_DELAY 500 +#define BCM6328_LED_INTERVAL_NUM 2 #define BCM6328_LED_INTERVAL_MS 20 #define BCM6328_LED_INTV_MASK 0x3f -#define BCM6328_LED_FAST_INTV_SHIFT 6 -#define BCM6328_LED_FAST_INTV_MASK (BCM6328_LED_INTV_MASK << \ - BCM6328_LED_FAST_INTV_SHIFT) +#define BCM6328_LED_INTV1_SHIFT 0 +#define BCM6328_LED_INTV1_MASK (BCM6328_LED_INTV_MASK << \ + BCM6328_LED_INTV1_SHIFT) +#define BCM6328_LED_INTV2_SHIFT 6 +#define BCM6328_LED_INTV2_MASK (BCM6328_LED_INTV_MASK << \ + BCM6328_LED_INTV2_SHIFT) #define BCM6328_SERIAL_LED_EN BIT(12) #define BCM6328_SERIAL_LED_MUX BIT(13) #define BCM6328_SERIAL_LED_CLK_NPOL BIT(14) @@ -45,8 +49,8 @@ #define BCM6328_LED_MODE_MASK 3 #define BCM6328_LED_MODE_ON 0 -#define BCM6328_LED_MODE_FAST 1 -#define BCM6328_LED_MODE_BLINK 2 +#define BCM6328_LED_MODE_INTV1 1 +#define BCM6328_LED_MODE_INTV2 2 #define BCM6328_LED_MODE_OFF 3 #define BCM6328_LED_SHIFT(X) ((X) << 1) @@ -127,7 +131,8 @@ static void bcm6328_led_set(struct led_classdev *led_cdev, unsigned long flags; spin_lock_irqsave(led->lock, flags); - *(led->blink_leds) &= ~BIT(led->pin); + led->blink_leds[0] &= ~BIT(led->pin); + led->blink_leds[1] &= ~BIT(led->pin); if ((led->active_low && value == LED_OFF) || (!led->active_low && value != LED_OFF)) bcm6328_led_mode(led, BCM6328_LED_MODE_ON); @@ -176,20 +181,35 @@ static int bcm6328_blink_set(struct led_classdev *led_cdev, } spin_lock_irqsave(led->lock, flags); - if (*(led->blink_leds) == 0 || - *(led->blink_leds) == BIT(led->pin) || - *(led->blink_delay) == delay) { + if (led->blink_leds[0] == 0 || + led->blink_leds[0] == BIT(led->pin) || + led->blink_delay[0] == delay) { unsigned long val; - *(led->blink_leds) |= BIT(led->pin); - *(led->blink_delay) = delay; + led->blink_leds[0] |= BIT(led->pin); + led->blink_delay[0] = delay; val = bcm6328_led_read(led->mem + BCM6328_REG_INIT); - val &= ~BCM6328_LED_FAST_INTV_MASK; - val |= (delay << BCM6328_LED_FAST_INTV_SHIFT); + val &= ~BCM6328_LED_INTV1_MASK; + val |= (delay << BCM6328_LED_INTV1_SHIFT); bcm6328_led_write(led->mem + BCM6328_REG_INIT, val); - bcm6328_led_mode(led, BCM6328_LED_MODE_BLINK); + bcm6328_led_mode(led, BCM6328_LED_MODE_INTV1); + rc = 0; + } else if (led->blink_leds[1] == 0 || + led->blink_leds[1] == BIT(led->pin) || + led->blink_delay[1] == delay) { + unsigned long val; + + led->blink_leds[1] |= BIT(led->pin); + led->blink_delay[1] = delay; + + val = bcm6328_led_read(led->mem + BCM6328_REG_INIT); + val &= ~BCM6328_LED_INTV2_MASK; + val |= (delay << BCM6328_LED_INTV2_SHIFT); + bcm6328_led_write(led->mem + BCM6328_REG_INIT, val); + + bcm6328_led_mode(led, BCM6328_LED_MODE_INTV2); rc = 0; } else { dev_dbg(led_cdev->dev, @@ -358,11 +378,13 @@ static int bcm6328_leds_probe(struct platform_device *pdev) if (!lock) return -ENOMEM; - blink_leds = devm_kzalloc(dev, sizeof(*blink_leds), GFP_KERNEL); + blink_leds = devm_kcalloc(dev, BCM6328_LED_INTERVAL_NUM, + sizeof(*blink_leds), GFP_KERNEL); if (!blink_leds) return -ENOMEM; - blink_delay = devm_kzalloc(dev, sizeof(*blink_delay), GFP_KERNEL); + blink_delay = devm_kcalloc(dev, BCM6328_LED_INTERVAL_NUM, + sizeof(*blink_delay), GFP_KERNEL); if (!blink_delay) return -ENOMEM; -- 2.20.1 ^ permalink raw reply related [flat|nested] 14+ messages in thread
* [PATCH v2] leds-bcm6328: support second hw blinking interval 2020-04-24 12:46 [PATCH] leds-bcm6328: support second hw blinking interval Álvaro Fernández Rojas @ 2020-04-24 13:32 ` Álvaro Fernández Rojas 2020-04-25 21:14 ` Pavel Machek 2020-05-12 10:01 ` [PATCH v3] " Álvaro Fernández Rojas 0 siblings, 2 replies; 14+ messages in thread From: Álvaro Fernández Rojas @ 2020-04-24 13:32 UTC (permalink / raw) To: linux-leds, jacek.anaszewski, jonas.gorski, rpurdie, pavel Cc: Álvaro Fernández Rojas Add support for both configurable HW blinking intervals. Signed-off-by: Álvaro Fernández Rojas <noltari@gmail.com> --- v2: remove LED from the other interval drivers/leds/leds-bcm6328.c | 56 ++++++++++++++++++++++++++----------- 1 file changed, 40 insertions(+), 16 deletions(-) diff --git a/drivers/leds/leds-bcm6328.c b/drivers/leds/leds-bcm6328.c index 42e1b7598c3a..a5a57a8d2a1c 100644 --- a/drivers/leds/leds-bcm6328.c +++ b/drivers/leds/leds-bcm6328.c @@ -24,12 +24,16 @@ #define BCM6328_LED_MAX_COUNT 24 #define BCM6328_LED_DEF_DELAY 500 +#define BCM6328_LED_INTERVAL_NUM 2 #define BCM6328_LED_INTERVAL_MS 20 #define BCM6328_LED_INTV_MASK 0x3f -#define BCM6328_LED_FAST_INTV_SHIFT 6 -#define BCM6328_LED_FAST_INTV_MASK (BCM6328_LED_INTV_MASK << \ - BCM6328_LED_FAST_INTV_SHIFT) +#define BCM6328_LED_INTV1_SHIFT 0 +#define BCM6328_LED_INTV1_MASK (BCM6328_LED_INTV_MASK << \ + BCM6328_LED_INTV1_SHIFT) +#define BCM6328_LED_INTV2_SHIFT 6 +#define BCM6328_LED_INTV2_MASK (BCM6328_LED_INTV_MASK << \ + BCM6328_LED_INTV2_SHIFT) #define BCM6328_SERIAL_LED_EN BIT(12) #define BCM6328_SERIAL_LED_MUX BIT(13) #define BCM6328_SERIAL_LED_CLK_NPOL BIT(14) @@ -45,8 +49,8 @@ #define BCM6328_LED_MODE_MASK 3 #define BCM6328_LED_MODE_ON 0 -#define BCM6328_LED_MODE_FAST 1 -#define BCM6328_LED_MODE_BLINK 2 +#define BCM6328_LED_MODE_INTV1 1 +#define BCM6328_LED_MODE_INTV2 2 #define BCM6328_LED_MODE_OFF 3 #define BCM6328_LED_SHIFT(X) ((X) << 1) @@ -127,7 +131,8 @@ static void bcm6328_led_set(struct led_classdev *led_cdev, unsigned long flags; spin_lock_irqsave(led->lock, flags); - *(led->blink_leds) &= ~BIT(led->pin); + led->blink_leds[0] &= ~BIT(led->pin); + led->blink_leds[1] &= ~BIT(led->pin); if ((led->active_low && value == LED_OFF) || (!led->active_low && value != LED_OFF)) bcm6328_led_mode(led, BCM6328_LED_MODE_ON); @@ -176,20 +181,37 @@ static int bcm6328_blink_set(struct led_classdev *led_cdev, } spin_lock_irqsave(led->lock, flags); - if (*(led->blink_leds) == 0 || - *(led->blink_leds) == BIT(led->pin) || - *(led->blink_delay) == delay) { + if (led->blink_leds[0] == 0 || + led->blink_leds[0] == BIT(led->pin) || + led->blink_delay[0] == delay) { unsigned long val; - *(led->blink_leds) |= BIT(led->pin); - *(led->blink_delay) = delay; + led->blink_leds[0] |= BIT(led->pin); + led->blink_leds[1] &= ~BIT(led->pin); + led->blink_delay[0] = delay; val = bcm6328_led_read(led->mem + BCM6328_REG_INIT); - val &= ~BCM6328_LED_FAST_INTV_MASK; - val |= (delay << BCM6328_LED_FAST_INTV_SHIFT); + val &= ~BCM6328_LED_INTV1_MASK; + val |= (delay << BCM6328_LED_INTV1_SHIFT); bcm6328_led_write(led->mem + BCM6328_REG_INIT, val); - bcm6328_led_mode(led, BCM6328_LED_MODE_BLINK); + bcm6328_led_mode(led, BCM6328_LED_MODE_INTV1); + rc = 0; + } else if (led->blink_leds[1] == 0 || + led->blink_leds[1] == BIT(led->pin) || + led->blink_delay[1] == delay) { + unsigned long val; + + led->blink_leds[0] &= ~BIT(led->pin); + led->blink_leds[1] |= BIT(led->pin); + led->blink_delay[1] = delay; + + val = bcm6328_led_read(led->mem + BCM6328_REG_INIT); + val &= ~BCM6328_LED_INTV2_MASK; + val |= (delay << BCM6328_LED_INTV2_SHIFT); + bcm6328_led_write(led->mem + BCM6328_REG_INIT, val); + + bcm6328_led_mode(led, BCM6328_LED_MODE_INTV2); rc = 0; } else { dev_dbg(led_cdev->dev, @@ -358,11 +380,13 @@ static int bcm6328_leds_probe(struct platform_device *pdev) if (!lock) return -ENOMEM; - blink_leds = devm_kzalloc(dev, sizeof(*blink_leds), GFP_KERNEL); + blink_leds = devm_kcalloc(dev, BCM6328_LED_INTERVAL_NUM, + sizeof(*blink_leds), GFP_KERNEL); if (!blink_leds) return -ENOMEM; - blink_delay = devm_kzalloc(dev, sizeof(*blink_delay), GFP_KERNEL); + blink_delay = devm_kcalloc(dev, BCM6328_LED_INTERVAL_NUM, + sizeof(*blink_delay), GFP_KERNEL); if (!blink_delay) return -ENOMEM; -- 2.20.1 ^ permalink raw reply related [flat|nested] 14+ messages in thread
* Re: [PATCH v2] leds-bcm6328: support second hw blinking interval 2020-04-24 13:32 ` [PATCH v2] " Álvaro Fernández Rojas @ 2020-04-25 21:14 ` Pavel Machek 2020-04-26 8:39 ` Álvaro Fernández Rojas 2020-05-12 10:01 ` [PATCH v3] " Álvaro Fernández Rojas 1 sibling, 1 reply; 14+ messages in thread From: Pavel Machek @ 2020-04-25 21:14 UTC (permalink / raw) To: Álvaro Fernández Rojas Cc: linux-leds, jacek.anaszewski, jonas.gorski, rpurdie [-- Attachment #1: Type: text/plain, Size: 4627 bytes --] Hi! > Add support for both configurable HW blinking intervals. The code could use ... better documentation and better changelog. What is the current blinking interval and what other interval does this add? Best regards, Pavel > Signed-off-by: Álvaro Fernández Rojas <noltari@gmail.com> > --- > v2: remove LED from the other interval > > drivers/leds/leds-bcm6328.c | 56 ++++++++++++++++++++++++++----------- > 1 file changed, 40 insertions(+), 16 deletions(-) > > diff --git a/drivers/leds/leds-bcm6328.c b/drivers/leds/leds-bcm6328.c > index 42e1b7598c3a..a5a57a8d2a1c 100644 > --- a/drivers/leds/leds-bcm6328.c > +++ b/drivers/leds/leds-bcm6328.c > @@ -24,12 +24,16 @@ > > #define BCM6328_LED_MAX_COUNT 24 > #define BCM6328_LED_DEF_DELAY 500 > +#define BCM6328_LED_INTERVAL_NUM 2 > #define BCM6328_LED_INTERVAL_MS 20 > > #define BCM6328_LED_INTV_MASK 0x3f > -#define BCM6328_LED_FAST_INTV_SHIFT 6 > -#define BCM6328_LED_FAST_INTV_MASK (BCM6328_LED_INTV_MASK << \ > - BCM6328_LED_FAST_INTV_SHIFT) > +#define BCM6328_LED_INTV1_SHIFT 0 > +#define BCM6328_LED_INTV1_MASK (BCM6328_LED_INTV_MASK << \ > + BCM6328_LED_INTV1_SHIFT) > +#define BCM6328_LED_INTV2_SHIFT 6 > +#define BCM6328_LED_INTV2_MASK (BCM6328_LED_INTV_MASK << \ > + BCM6328_LED_INTV2_SHIFT) > #define BCM6328_SERIAL_LED_EN BIT(12) > #define BCM6328_SERIAL_LED_MUX BIT(13) > #define BCM6328_SERIAL_LED_CLK_NPOL BIT(14) > @@ -45,8 +49,8 @@ > > #define BCM6328_LED_MODE_MASK 3 > #define BCM6328_LED_MODE_ON 0 > -#define BCM6328_LED_MODE_FAST 1 > -#define BCM6328_LED_MODE_BLINK 2 > +#define BCM6328_LED_MODE_INTV1 1 > +#define BCM6328_LED_MODE_INTV2 2 > #define BCM6328_LED_MODE_OFF 3 > #define BCM6328_LED_SHIFT(X) ((X) << 1) > > @@ -127,7 +131,8 @@ static void bcm6328_led_set(struct led_classdev *led_cdev, > unsigned long flags; > > spin_lock_irqsave(led->lock, flags); > - *(led->blink_leds) &= ~BIT(led->pin); > + led->blink_leds[0] &= ~BIT(led->pin); > + led->blink_leds[1] &= ~BIT(led->pin); > if ((led->active_low && value == LED_OFF) || > (!led->active_low && value != LED_OFF)) > bcm6328_led_mode(led, BCM6328_LED_MODE_ON); > @@ -176,20 +181,37 @@ static int bcm6328_blink_set(struct led_classdev *led_cdev, > } > > spin_lock_irqsave(led->lock, flags); > - if (*(led->blink_leds) == 0 || > - *(led->blink_leds) == BIT(led->pin) || > - *(led->blink_delay) == delay) { > + if (led->blink_leds[0] == 0 || > + led->blink_leds[0] == BIT(led->pin) || > + led->blink_delay[0] == delay) { > unsigned long val; > > - *(led->blink_leds) |= BIT(led->pin); > - *(led->blink_delay) = delay; > + led->blink_leds[0] |= BIT(led->pin); > + led->blink_leds[1] &= ~BIT(led->pin); > + led->blink_delay[0] = delay; > > val = bcm6328_led_read(led->mem + BCM6328_REG_INIT); > - val &= ~BCM6328_LED_FAST_INTV_MASK; > - val |= (delay << BCM6328_LED_FAST_INTV_SHIFT); > + val &= ~BCM6328_LED_INTV1_MASK; > + val |= (delay << BCM6328_LED_INTV1_SHIFT); > bcm6328_led_write(led->mem + BCM6328_REG_INIT, val); > > - bcm6328_led_mode(led, BCM6328_LED_MODE_BLINK); > + bcm6328_led_mode(led, BCM6328_LED_MODE_INTV1); > + rc = 0; > + } else if (led->blink_leds[1] == 0 || > + led->blink_leds[1] == BIT(led->pin) || > + led->blink_delay[1] == delay) { > + unsigned long val; > + > + led->blink_leds[0] &= ~BIT(led->pin); > + led->blink_leds[1] |= BIT(led->pin); > + led->blink_delay[1] = delay; > + > + val = bcm6328_led_read(led->mem + BCM6328_REG_INIT); > + val &= ~BCM6328_LED_INTV2_MASK; > + val |= (delay << BCM6328_LED_INTV2_SHIFT); > + bcm6328_led_write(led->mem + BCM6328_REG_INIT, val); > + > + bcm6328_led_mode(led, BCM6328_LED_MODE_INTV2); > rc = 0; > } else { > dev_dbg(led_cdev->dev, > @@ -358,11 +380,13 @@ static int bcm6328_leds_probe(struct platform_device *pdev) > if (!lock) > return -ENOMEM; > > - blink_leds = devm_kzalloc(dev, sizeof(*blink_leds), GFP_KERNEL); > + blink_leds = devm_kcalloc(dev, BCM6328_LED_INTERVAL_NUM, > + sizeof(*blink_leds), GFP_KERNEL); > if (!blink_leds) > return -ENOMEM; > > - blink_delay = devm_kzalloc(dev, sizeof(*blink_delay), GFP_KERNEL); > + blink_delay = devm_kcalloc(dev, BCM6328_LED_INTERVAL_NUM, > + sizeof(*blink_delay), GFP_KERNEL); > if (!blink_delay) > return -ENOMEM; > -- (english) http://www.livejournal.com/~pavelmachek (cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html [-- Attachment #2: Digital signature --] [-- Type: application/pgp-signature, Size: 181 bytes --] ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH v2] leds-bcm6328: support second hw blinking interval 2020-04-25 21:14 ` Pavel Machek @ 2020-04-26 8:39 ` Álvaro Fernández Rojas 0 siblings, 0 replies; 14+ messages in thread From: Álvaro Fernández Rojas @ 2020-04-26 8:39 UTC (permalink / raw) To: Pavel Machek; +Cc: linux-leds, jacek.anaszewski, jonas.gorski, rpurdie Hi Pavel, Two different HW blinking intervals can be configured in the controller, but we were only using one of them until now. These blinking intervals are labeled as FAST_INTV and SLOW_INTV in the sources provided by Broadcom: https://github.com/jameshilliard/gfiber-gflt100/blob/master/shared/opensource/include/bcm963xx/63268_map_part.h#L1077 However, there are no fast/slow blinking intervals, since the configurable delay range is exactly the same for both of them. Therefore, a LED can have 4 states (https://github.com/jameshilliard/gfiber-gflt100/blob/master/shared/opensource/include/bcm963xx/63268_map_part.h#L1085): Off, Blinking Interval 1, Blinking Interval 2 and On. P.S: sorry for the previous emails, but my email client wasn't properly configured... (It should be now :$) Best regards, Álvaro. El 25/04/2020 a las 23:14, Pavel Machek escribió: > Hi! > >> Add support for both configurable HW blinking intervals. > > The code could use ... better documentation and better changelog. What > is the current blinking interval and what other interval does this > add? > > Best regards, > Pavel > >> Signed-off-by: Álvaro Fernández Rojas <noltari@gmail.com> >> --- >> v2: remove LED from the other interval >> >> drivers/leds/leds-bcm6328.c | 56 ++++++++++++++++++++++++++----------- >> 1 file changed, 40 insertions(+), 16 deletions(-) >> >> diff --git a/drivers/leds/leds-bcm6328.c b/drivers/leds/leds-bcm6328.c >> index 42e1b7598c3a..a5a57a8d2a1c 100644 >> --- a/drivers/leds/leds-bcm6328.c >> +++ b/drivers/leds/leds-bcm6328.c >> @@ -24,12 +24,16 @@ >> >> #define BCM6328_LED_MAX_COUNT 24 >> #define BCM6328_LED_DEF_DELAY 500 >> +#define BCM6328_LED_INTERVAL_NUM 2 >> #define BCM6328_LED_INTERVAL_MS 20 >> >> #define BCM6328_LED_INTV_MASK 0x3f >> -#define BCM6328_LED_FAST_INTV_SHIFT 6 >> -#define BCM6328_LED_FAST_INTV_MASK (BCM6328_LED_INTV_MASK << \ >> - BCM6328_LED_FAST_INTV_SHIFT) >> +#define BCM6328_LED_INTV1_SHIFT 0 >> +#define BCM6328_LED_INTV1_MASK (BCM6328_LED_INTV_MASK << \ >> + BCM6328_LED_INTV1_SHIFT) >> +#define BCM6328_LED_INTV2_SHIFT 6 >> +#define BCM6328_LED_INTV2_MASK (BCM6328_LED_INTV_MASK << \ >> + BCM6328_LED_INTV2_SHIFT) >> #define BCM6328_SERIAL_LED_EN BIT(12) >> #define BCM6328_SERIAL_LED_MUX BIT(13) >> #define BCM6328_SERIAL_LED_CLK_NPOL BIT(14) >> @@ -45,8 +49,8 @@ >> >> #define BCM6328_LED_MODE_MASK 3 >> #define BCM6328_LED_MODE_ON 0 >> -#define BCM6328_LED_MODE_FAST 1 >> -#define BCM6328_LED_MODE_BLINK 2 >> +#define BCM6328_LED_MODE_INTV1 1 >> +#define BCM6328_LED_MODE_INTV2 2 >> #define BCM6328_LED_MODE_OFF 3 >> #define BCM6328_LED_SHIFT(X) ((X) << 1) >> >> @@ -127,7 +131,8 @@ static void bcm6328_led_set(struct led_classdev *led_cdev, >> unsigned long flags; >> >> spin_lock_irqsave(led->lock, flags); >> - *(led->blink_leds) &= ~BIT(led->pin); >> + led->blink_leds[0] &= ~BIT(led->pin); >> + led->blink_leds[1] &= ~BIT(led->pin); >> if ((led->active_low && value == LED_OFF) || >> (!led->active_low && value != LED_OFF)) >> bcm6328_led_mode(led, BCM6328_LED_MODE_ON); >> @@ -176,20 +181,37 @@ static int bcm6328_blink_set(struct led_classdev *led_cdev, >> } >> >> spin_lock_irqsave(led->lock, flags); >> - if (*(led->blink_leds) == 0 || >> - *(led->blink_leds) == BIT(led->pin) || >> - *(led->blink_delay) == delay) { >> + if (led->blink_leds[0] == 0 || >> + led->blink_leds[0] == BIT(led->pin) || >> + led->blink_delay[0] == delay) { >> unsigned long val; >> >> - *(led->blink_leds) |= BIT(led->pin); >> - *(led->blink_delay) = delay; >> + led->blink_leds[0] |= BIT(led->pin); >> + led->blink_leds[1] &= ~BIT(led->pin); >> + led->blink_delay[0] = delay; >> >> val = bcm6328_led_read(led->mem + BCM6328_REG_INIT); >> - val &= ~BCM6328_LED_FAST_INTV_MASK; >> - val |= (delay << BCM6328_LED_FAST_INTV_SHIFT); >> + val &= ~BCM6328_LED_INTV1_MASK; >> + val |= (delay << BCM6328_LED_INTV1_SHIFT); >> bcm6328_led_write(led->mem + BCM6328_REG_INIT, val); >> >> - bcm6328_led_mode(led, BCM6328_LED_MODE_BLINK); >> + bcm6328_led_mode(led, BCM6328_LED_MODE_INTV1); >> + rc = 0; >> + } else if (led->blink_leds[1] == 0 || >> + led->blink_leds[1] == BIT(led->pin) || >> + led->blink_delay[1] == delay) { >> + unsigned long val; >> + >> + led->blink_leds[0] &= ~BIT(led->pin); >> + led->blink_leds[1] |= BIT(led->pin); >> + led->blink_delay[1] = delay; >> + >> + val = bcm6328_led_read(led->mem + BCM6328_REG_INIT); >> + val &= ~BCM6328_LED_INTV2_MASK; >> + val |= (delay << BCM6328_LED_INTV2_SHIFT); >> + bcm6328_led_write(led->mem + BCM6328_REG_INIT, val); >> + >> + bcm6328_led_mode(led, BCM6328_LED_MODE_INTV2); >> rc = 0; >> } else { >> dev_dbg(led_cdev->dev, >> @@ -358,11 +380,13 @@ static int bcm6328_leds_probe(struct platform_device *pdev) >> if (!lock) >> return -ENOMEM; >> >> - blink_leds = devm_kzalloc(dev, sizeof(*blink_leds), GFP_KERNEL); >> + blink_leds = devm_kcalloc(dev, BCM6328_LED_INTERVAL_NUM, >> + sizeof(*blink_leds), GFP_KERNEL); >> if (!blink_leds) >> return -ENOMEM; >> >> - blink_delay = devm_kzalloc(dev, sizeof(*blink_delay), GFP_KERNEL); >> + blink_delay = devm_kcalloc(dev, BCM6328_LED_INTERVAL_NUM, >> + sizeof(*blink_delay), GFP_KERNEL); >> if (!blink_delay) >> return -ENOMEM; >> > ^ permalink raw reply [flat|nested] 14+ messages in thread
* [PATCH v3] leds-bcm6328: support second hw blinking interval 2020-04-24 13:32 ` [PATCH v2] " Álvaro Fernández Rojas 2020-04-25 21:14 ` Pavel Machek @ 2020-05-12 10:01 ` Álvaro Fernández Rojas 2020-06-04 13:24 ` Pavel Machek 2020-06-04 13:48 ` [PATCH v4] " Álvaro Fernández Rojas 1 sibling, 2 replies; 14+ messages in thread From: Álvaro Fernández Rojas @ 2020-05-12 10:01 UTC (permalink / raw) To: linux-leds, jacek.anaszewski, jonas.gorski, rpurdie, pavel Cc: Álvaro Fernández Rojas Add support for both configurable HW blinking intervals. Signed-off-by: Álvaro Fernández Rojas <noltari@gmail.com> --- v3: add code documentation v2: remove LED from the other interval drivers/leds/leds-bcm6328.c | 83 ++++++++++++++++++++++++++++++------- 1 file changed, 67 insertions(+), 16 deletions(-) diff --git a/drivers/leds/leds-bcm6328.c b/drivers/leds/leds-bcm6328.c index 42e1b7598c3a..050a77591f0b 100644 --- a/drivers/leds/leds-bcm6328.c +++ b/drivers/leds/leds-bcm6328.c @@ -24,12 +24,16 @@ #define BCM6328_LED_MAX_COUNT 24 #define BCM6328_LED_DEF_DELAY 500 +#define BCM6328_LED_INTERVAL_NUM 2 #define BCM6328_LED_INTERVAL_MS 20 #define BCM6328_LED_INTV_MASK 0x3f -#define BCM6328_LED_FAST_INTV_SHIFT 6 -#define BCM6328_LED_FAST_INTV_MASK (BCM6328_LED_INTV_MASK << \ - BCM6328_LED_FAST_INTV_SHIFT) +#define BCM6328_LED_INTV1_SHIFT 0 +#define BCM6328_LED_INTV1_MASK (BCM6328_LED_INTV_MASK << \ + BCM6328_LED_INTV1_SHIFT) +#define BCM6328_LED_INTV2_SHIFT 6 +#define BCM6328_LED_INTV2_MASK (BCM6328_LED_INTV_MASK << \ + BCM6328_LED_INTV2_SHIFT) #define BCM6328_SERIAL_LED_EN BIT(12) #define BCM6328_SERIAL_LED_MUX BIT(13) #define BCM6328_SERIAL_LED_CLK_NPOL BIT(14) @@ -45,8 +49,8 @@ #define BCM6328_LED_MODE_MASK 3 #define BCM6328_LED_MODE_ON 0 -#define BCM6328_LED_MODE_FAST 1 -#define BCM6328_LED_MODE_BLINK 2 +#define BCM6328_LED_MODE_INTV1 1 +#define BCM6328_LED_MODE_INTV2 2 #define BCM6328_LED_MODE_OFF 3 #define BCM6328_LED_SHIFT(X) ((X) << 1) @@ -127,12 +131,18 @@ static void bcm6328_led_set(struct led_classdev *led_cdev, unsigned long flags; spin_lock_irqsave(led->lock, flags); - *(led->blink_leds) &= ~BIT(led->pin); + + /* Remove LED from cached HW blinking intervals */ + led->blink_leds[0] &= ~BIT(led->pin); + led->blink_leds[1] &= ~BIT(led->pin); + + /* Set LED on/off */ if ((led->active_low && value == LED_OFF) || (!led->active_low && value != LED_OFF)) bcm6328_led_mode(led, BCM6328_LED_MODE_ON); else bcm6328_led_mode(led, BCM6328_LED_MODE_OFF); + spin_unlock_irqrestore(led->lock, flags); } @@ -176,20 +186,59 @@ static int bcm6328_blink_set(struct led_classdev *led_cdev, } spin_lock_irqsave(led->lock, flags); - if (*(led->blink_leds) == 0 || - *(led->blink_leds) == BIT(led->pin) || - *(led->blink_delay) == delay) { + /* + * Check if any of the two configurable HW blinking intervals is + * available: + * 1. No LEDs assigned to the HW blinking interval. + * 2. LEDs with the same delay assigned. + */ + if (led->blink_leds[0] == 0 || + led->blink_leds[0] == BIT(led->pin) || + led->blink_delay[0] == delay) { + unsigned long val; + + /* Add LED to the first HW blinking interval cache */ + led->blink_leds[0] |= BIT(led->pin); + + /* Remove LED from the second HW blinking interval cache */ + led->blink_leds[1] &= ~BIT(led->pin); + + /* Cache the LED in the first HW blinking interval delay */ + led->blink_delay[0] = delay; + + /* Update the delay for the first HW blinking interval */ + val = bcm6328_led_read(led->mem + BCM6328_REG_INIT); + val &= ~BCM6328_LED_INTV1_MASK; + val |= (delay << BCM6328_LED_INTV1_SHIFT); + bcm6328_led_write(led->mem + BCM6328_REG_INIT, val); + + /* Set the LED to first HW blinking interval */ + bcm6328_led_mode(led, BCM6328_LED_MODE_INTV1); + + rc = 0; + } else if (led->blink_leds[1] == 0 || + led->blink_leds[1] == BIT(led->pin) || + led->blink_delay[1] == delay) { unsigned long val; - *(led->blink_leds) |= BIT(led->pin); - *(led->blink_delay) = delay; + /* Remove LED from the first HW blinking interval */ + led->blink_leds[0] &= ~BIT(led->pin); + + /* Add LED to the second HW blinking interval */ + led->blink_leds[1] |= BIT(led->pin); + /* Cache the LED in the first HW blinking interval delay */ + led->blink_delay[1] = delay; + + /* Update the delay for the second HW blinking interval */ val = bcm6328_led_read(led->mem + BCM6328_REG_INIT); - val &= ~BCM6328_LED_FAST_INTV_MASK; - val |= (delay << BCM6328_LED_FAST_INTV_SHIFT); + val &= ~BCM6328_LED_INTV2_MASK; + val |= (delay << BCM6328_LED_INTV2_SHIFT); bcm6328_led_write(led->mem + BCM6328_REG_INIT, val); - bcm6328_led_mode(led, BCM6328_LED_MODE_BLINK); + /* Set the LED to second HW blinking interval */ + bcm6328_led_mode(led, BCM6328_LED_MODE_INTV2); + rc = 0; } else { dev_dbg(led_cdev->dev, @@ -358,11 +407,13 @@ static int bcm6328_leds_probe(struct platform_device *pdev) if (!lock) return -ENOMEM; - blink_leds = devm_kzalloc(dev, sizeof(*blink_leds), GFP_KERNEL); + blink_leds = devm_kcalloc(dev, BCM6328_LED_INTERVAL_NUM, + sizeof(*blink_leds), GFP_KERNEL); if (!blink_leds) return -ENOMEM; - blink_delay = devm_kzalloc(dev, sizeof(*blink_delay), GFP_KERNEL); + blink_delay = devm_kcalloc(dev, BCM6328_LED_INTERVAL_NUM, + sizeof(*blink_delay), GFP_KERNEL); if (!blink_delay) return -ENOMEM; -- 2.26.2 ^ permalink raw reply related [flat|nested] 14+ messages in thread
* Re: [PATCH v3] leds-bcm6328: support second hw blinking interval 2020-05-12 10:01 ` [PATCH v3] " Álvaro Fernández Rojas @ 2020-06-04 13:24 ` Pavel Machek 2020-06-04 13:35 ` Álvaro Fernández Rojas 2020-06-04 13:48 ` [PATCH v4] " Álvaro Fernández Rojas 1 sibling, 1 reply; 14+ messages in thread From: Pavel Machek @ 2020-06-04 13:24 UTC (permalink / raw) To: Álvaro Fernández Rojas Cc: linux-leds, jacek.anaszewski, jonas.gorski, rpurdie [-- Attachment #1: Type: text/plain, Size: 5857 bytes --] On Tue 2020-05-12 12:01:36, Álvaro Fernández Rojas wrote: > Add support for both configurable HW blinking intervals. > The code could use ... better documentation and better changelog. What is the current blinking interval and what other interval does this add? Best regards, Pavel > Signed-off-by: Álvaro Fernández Rojas <noltari@gmail.com> > --- > v3: add code documentation > v2: remove LED from the other interval > > drivers/leds/leds-bcm6328.c | 83 ++++++++++++++++++++++++++++++------- > 1 file changed, 67 insertions(+), 16 deletions(-) > > diff --git a/drivers/leds/leds-bcm6328.c b/drivers/leds/leds-bcm6328.c > index 42e1b7598c3a..050a77591f0b 100644 > --- a/drivers/leds/leds-bcm6328.c > +++ b/drivers/leds/leds-bcm6328.c > @@ -24,12 +24,16 @@ > > #define BCM6328_LED_MAX_COUNT 24 > #define BCM6328_LED_DEF_DELAY 500 > +#define BCM6328_LED_INTERVAL_NUM 2 > #define BCM6328_LED_INTERVAL_MS 20 > > #define BCM6328_LED_INTV_MASK 0x3f > -#define BCM6328_LED_FAST_INTV_SHIFT 6 > -#define BCM6328_LED_FAST_INTV_MASK (BCM6328_LED_INTV_MASK << \ > - BCM6328_LED_FAST_INTV_SHIFT) > +#define BCM6328_LED_INTV1_SHIFT 0 > +#define BCM6328_LED_INTV1_MASK (BCM6328_LED_INTV_MASK << \ > + BCM6328_LED_INTV1_SHIFT) > +#define BCM6328_LED_INTV2_SHIFT 6 > +#define BCM6328_LED_INTV2_MASK (BCM6328_LED_INTV_MASK << \ > + BCM6328_LED_INTV2_SHIFT) > #define BCM6328_SERIAL_LED_EN BIT(12) > #define BCM6328_SERIAL_LED_MUX BIT(13) > #define BCM6328_SERIAL_LED_CLK_NPOL BIT(14) > @@ -45,8 +49,8 @@ > > #define BCM6328_LED_MODE_MASK 3 > #define BCM6328_LED_MODE_ON 0 > -#define BCM6328_LED_MODE_FAST 1 > -#define BCM6328_LED_MODE_BLINK 2 > +#define BCM6328_LED_MODE_INTV1 1 > +#define BCM6328_LED_MODE_INTV2 2 > #define BCM6328_LED_MODE_OFF 3 > #define BCM6328_LED_SHIFT(X) ((X) << 1) > > @@ -127,12 +131,18 @@ static void bcm6328_led_set(struct led_classdev *led_cdev, > unsigned long flags; > > spin_lock_irqsave(led->lock, flags); > - *(led->blink_leds) &= ~BIT(led->pin); > + > + /* Remove LED from cached HW blinking intervals */ > + led->blink_leds[0] &= ~BIT(led->pin); > + led->blink_leds[1] &= ~BIT(led->pin); > + > + /* Set LED on/off */ > if ((led->active_low && value == LED_OFF) || > (!led->active_low && value != LED_OFF)) > bcm6328_led_mode(led, BCM6328_LED_MODE_ON); > else > bcm6328_led_mode(led, BCM6328_LED_MODE_OFF); > + > spin_unlock_irqrestore(led->lock, flags); > } > > @@ -176,20 +186,59 @@ static int bcm6328_blink_set(struct led_classdev *led_cdev, > } > > spin_lock_irqsave(led->lock, flags); > - if (*(led->blink_leds) == 0 || > - *(led->blink_leds) == BIT(led->pin) || > - *(led->blink_delay) == delay) { > + /* > + * Check if any of the two configurable HW blinking intervals is > + * available: > + * 1. No LEDs assigned to the HW blinking interval. > + * 2. LEDs with the same delay assigned. > + */ > + if (led->blink_leds[0] == 0 || > + led->blink_leds[0] == BIT(led->pin) || > + led->blink_delay[0] == delay) { > + unsigned long val; > + > + /* Add LED to the first HW blinking interval cache */ > + led->blink_leds[0] |= BIT(led->pin); > + > + /* Remove LED from the second HW blinking interval cache */ > + led->blink_leds[1] &= ~BIT(led->pin); > + > + /* Cache the LED in the first HW blinking interval delay */ > + led->blink_delay[0] = delay; > + > + /* Update the delay for the first HW blinking interval */ > + val = bcm6328_led_read(led->mem + BCM6328_REG_INIT); > + val &= ~BCM6328_LED_INTV1_MASK; > + val |= (delay << BCM6328_LED_INTV1_SHIFT); > + bcm6328_led_write(led->mem + BCM6328_REG_INIT, val); > + > + /* Set the LED to first HW blinking interval */ > + bcm6328_led_mode(led, BCM6328_LED_MODE_INTV1); > + > + rc = 0; > + } else if (led->blink_leds[1] == 0 || > + led->blink_leds[1] == BIT(led->pin) || > + led->blink_delay[1] == delay) { > unsigned long val; > > - *(led->blink_leds) |= BIT(led->pin); > - *(led->blink_delay) = delay; > + /* Remove LED from the first HW blinking interval */ > + led->blink_leds[0] &= ~BIT(led->pin); > + > + /* Add LED to the second HW blinking interval */ > + led->blink_leds[1] |= BIT(led->pin); > > + /* Cache the LED in the first HW blinking interval delay */ > + led->blink_delay[1] = delay; > + > + /* Update the delay for the second HW blinking interval */ > val = bcm6328_led_read(led->mem + BCM6328_REG_INIT); > - val &= ~BCM6328_LED_FAST_INTV_MASK; > - val |= (delay << BCM6328_LED_FAST_INTV_SHIFT); > + val &= ~BCM6328_LED_INTV2_MASK; > + val |= (delay << BCM6328_LED_INTV2_SHIFT); > bcm6328_led_write(led->mem + BCM6328_REG_INIT, val); > > - bcm6328_led_mode(led, BCM6328_LED_MODE_BLINK); > + /* Set the LED to second HW blinking interval */ > + bcm6328_led_mode(led, BCM6328_LED_MODE_INTV2); > + > rc = 0; > } else { > dev_dbg(led_cdev->dev, > @@ -358,11 +407,13 @@ static int bcm6328_leds_probe(struct platform_device *pdev) > if (!lock) > return -ENOMEM; > > - blink_leds = devm_kzalloc(dev, sizeof(*blink_leds), GFP_KERNEL); > + blink_leds = devm_kcalloc(dev, BCM6328_LED_INTERVAL_NUM, > + sizeof(*blink_leds), GFP_KERNEL); > if (!blink_leds) > return -ENOMEM; > > - blink_delay = devm_kzalloc(dev, sizeof(*blink_delay), GFP_KERNEL); > + blink_delay = devm_kcalloc(dev, BCM6328_LED_INTERVAL_NUM, > + sizeof(*blink_delay), GFP_KERNEL); > if (!blink_delay) > return -ENOMEM; > > -- > 2.26.2 -- (english) http://www.livejournal.com/~pavelmachek (cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html [-- Attachment #2: signature.asc --] [-- Type: application/pgp-signature, Size: 195 bytes --] ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH v3] leds-bcm6328: support second hw blinking interval 2020-06-04 13:24 ` Pavel Machek @ 2020-06-04 13:35 ` Álvaro Fernández Rojas 2020-06-04 13:40 ` Pavel Machek 0 siblings, 1 reply; 14+ messages in thread From: Álvaro Fernández Rojas @ 2020-06-04 13:35 UTC (permalink / raw) To: Pavel Machek; +Cc: linux-leds, jacek.anaszewski, Jonas Gorski, rpurdie HI Pavel, Right now, only one hw blinking interval is supported, but up to two hw blinking intervals can be configured in the controller. This patch just adds support for the second hw blinking interval. BTW, I will send v4 which should clarify that each LED can be configured in 4 modes: 0: On 1: HW Blinking (Interval 1) 2: HW Blinking (Interval 2) 3: Off Until now we were only using 3 modes: 0: On 1: HW Blinking (Interval 1) 3: Off Best regards, Álvaro. > El 4 jun 2020, a las 15:24, Pavel Machek <pavel@ucw.cz> escribió: > > On Tue 2020-05-12 12:01:36, Álvaro Fernández Rojas wrote: >> Add support for both configurable HW blinking intervals. >> > > The code could use ... better documentation and better changelog. What > is the current blinking interval and what other interval does this > add? > > Best regards, > Pavel > >> Signed-off-by: Álvaro Fernández Rojas <noltari@gmail.com> >> --- >> v3: add code documentation >> v2: remove LED from the other interval >> >> drivers/leds/leds-bcm6328.c | 83 ++++++++++++++++++++++++++++++------- >> 1 file changed, 67 insertions(+), 16 deletions(-) >> >> diff --git a/drivers/leds/leds-bcm6328.c b/drivers/leds/leds-bcm6328.c >> index 42e1b7598c3a..050a77591f0b 100644 >> --- a/drivers/leds/leds-bcm6328.c >> +++ b/drivers/leds/leds-bcm6328.c >> @@ -24,12 +24,16 @@ >> >> #define BCM6328_LED_MAX_COUNT 24 >> #define BCM6328_LED_DEF_DELAY 500 >> +#define BCM6328_LED_INTERVAL_NUM 2 >> #define BCM6328_LED_INTERVAL_MS 20 >> >> #define BCM6328_LED_INTV_MASK 0x3f >> -#define BCM6328_LED_FAST_INTV_SHIFT 6 >> -#define BCM6328_LED_FAST_INTV_MASK (BCM6328_LED_INTV_MASK << \ >> - BCM6328_LED_FAST_INTV_SHIFT) >> +#define BCM6328_LED_INTV1_SHIFT 0 >> +#define BCM6328_LED_INTV1_MASK (BCM6328_LED_INTV_MASK << \ >> + BCM6328_LED_INTV1_SHIFT) >> +#define BCM6328_LED_INTV2_SHIFT 6 >> +#define BCM6328_LED_INTV2_MASK (BCM6328_LED_INTV_MASK << \ >> + BCM6328_LED_INTV2_SHIFT) >> #define BCM6328_SERIAL_LED_EN BIT(12) >> #define BCM6328_SERIAL_LED_MUX BIT(13) >> #define BCM6328_SERIAL_LED_CLK_NPOL BIT(14) >> @@ -45,8 +49,8 @@ >> >> #define BCM6328_LED_MODE_MASK 3 >> #define BCM6328_LED_MODE_ON 0 >> -#define BCM6328_LED_MODE_FAST 1 >> -#define BCM6328_LED_MODE_BLINK 2 >> +#define BCM6328_LED_MODE_INTV1 1 >> +#define BCM6328_LED_MODE_INTV2 2 >> #define BCM6328_LED_MODE_OFF 3 >> #define BCM6328_LED_SHIFT(X) ((X) << 1) >> >> @@ -127,12 +131,18 @@ static void bcm6328_led_set(struct led_classdev *led_cdev, >> unsigned long flags; >> >> spin_lock_irqsave(led->lock, flags); >> - *(led->blink_leds) &= ~BIT(led->pin); >> + >> + /* Remove LED from cached HW blinking intervals */ >> + led->blink_leds[0] &= ~BIT(led->pin); >> + led->blink_leds[1] &= ~BIT(led->pin); >> + >> + /* Set LED on/off */ >> if ((led->active_low && value == LED_OFF) || >> (!led->active_low && value != LED_OFF)) >> bcm6328_led_mode(led, BCM6328_LED_MODE_ON); >> else >> bcm6328_led_mode(led, BCM6328_LED_MODE_OFF); >> + >> spin_unlock_irqrestore(led->lock, flags); >> } >> >> @@ -176,20 +186,59 @@ static int bcm6328_blink_set(struct led_classdev *led_cdev, >> } >> >> spin_lock_irqsave(led->lock, flags); >> - if (*(led->blink_leds) == 0 || >> - *(led->blink_leds) == BIT(led->pin) || >> - *(led->blink_delay) == delay) { >> + /* >> + * Check if any of the two configurable HW blinking intervals is >> + * available: >> + * 1. No LEDs assigned to the HW blinking interval. >> + * 2. LEDs with the same delay assigned. >> + */ >> + if (led->blink_leds[0] == 0 || >> + led->blink_leds[0] == BIT(led->pin) || >> + led->blink_delay[0] == delay) { >> + unsigned long val; >> + >> + /* Add LED to the first HW blinking interval cache */ >> + led->blink_leds[0] |= BIT(led->pin); >> + >> + /* Remove LED from the second HW blinking interval cache */ >> + led->blink_leds[1] &= ~BIT(led->pin); >> + >> + /* Cache the LED in the first HW blinking interval delay */ >> + led->blink_delay[0] = delay; >> + >> + /* Update the delay for the first HW blinking interval */ >> + val = bcm6328_led_read(led->mem + BCM6328_REG_INIT); >> + val &= ~BCM6328_LED_INTV1_MASK; >> + val |= (delay << BCM6328_LED_INTV1_SHIFT); >> + bcm6328_led_write(led->mem + BCM6328_REG_INIT, val); >> + >> + /* Set the LED to first HW blinking interval */ >> + bcm6328_led_mode(led, BCM6328_LED_MODE_INTV1); >> + >> + rc = 0; >> + } else if (led->blink_leds[1] == 0 || >> + led->blink_leds[1] == BIT(led->pin) || >> + led->blink_delay[1] == delay) { >> unsigned long val; >> >> - *(led->blink_leds) |= BIT(led->pin); >> - *(led->blink_delay) = delay; >> + /* Remove LED from the first HW blinking interval */ >> + led->blink_leds[0] &= ~BIT(led->pin); >> + >> + /* Add LED to the second HW blinking interval */ >> + led->blink_leds[1] |= BIT(led->pin); >> >> + /* Cache the LED in the first HW blinking interval delay */ >> + led->blink_delay[1] = delay; >> + >> + /* Update the delay for the second HW blinking interval */ >> val = bcm6328_led_read(led->mem + BCM6328_REG_INIT); >> - val &= ~BCM6328_LED_FAST_INTV_MASK; >> - val |= (delay << BCM6328_LED_FAST_INTV_SHIFT); >> + val &= ~BCM6328_LED_INTV2_MASK; >> + val |= (delay << BCM6328_LED_INTV2_SHIFT); >> bcm6328_led_write(led->mem + BCM6328_REG_INIT, val); >> >> - bcm6328_led_mode(led, BCM6328_LED_MODE_BLINK); >> + /* Set the LED to second HW blinking interval */ >> + bcm6328_led_mode(led, BCM6328_LED_MODE_INTV2); >> + >> rc = 0; >> } else { >> dev_dbg(led_cdev->dev, >> @@ -358,11 +407,13 @@ static int bcm6328_leds_probe(struct platform_device *pdev) >> if (!lock) >> return -ENOMEM; >> >> - blink_leds = devm_kzalloc(dev, sizeof(*blink_leds), GFP_KERNEL); >> + blink_leds = devm_kcalloc(dev, BCM6328_LED_INTERVAL_NUM, >> + sizeof(*blink_leds), GFP_KERNEL); >> if (!blink_leds) >> return -ENOMEM; >> >> - blink_delay = devm_kzalloc(dev, sizeof(*blink_delay), GFP_KERNEL); >> + blink_delay = devm_kcalloc(dev, BCM6328_LED_INTERVAL_NUM, >> + sizeof(*blink_delay), GFP_KERNEL); >> if (!blink_delay) >> return -ENOMEM; >> >> -- >> 2.26.2 > > -- > (english) http://www.livejournal.com/~pavelmachek > (cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH v3] leds-bcm6328: support second hw blinking interval 2020-06-04 13:35 ` Álvaro Fernández Rojas @ 2020-06-04 13:40 ` Pavel Machek 2020-06-04 13:43 ` Álvaro Fernández Rojas 0 siblings, 1 reply; 14+ messages in thread From: Pavel Machek @ 2020-06-04 13:40 UTC (permalink / raw) To: Álvaro Fernández Rojas Cc: linux-leds, jacek.anaszewski, Jonas Gorski, rpurdie [-- Attachment #1: Type: text/plain, Size: 7211 bytes --] On Thu 2020-06-04 15:35:06, Álvaro Fernández Rojas wrote: > HI Pavel, > > Right now, only one hw blinking interval is supported, but up to two hw blinking intervals can be configured in the controller. > This patch just adds support for the second hw blinking interval. > > BTW, I will send v4 which should clarify that each LED can be configured in 4 modes: > 0: On > 1: HW Blinking (Interval 1) > 2: HW Blinking (Interval 2) > 3: Off > > Until now we were only using 3 modes: > 0: On > 1: HW Blinking (Interval 1) > 3: Off So the issue is .... there are like 5 LEDs and they can be on or off or blinking with at most 2 different intervals? Pavel > > El 4 jun 2020, a las 15:24, Pavel Machek <pavel@ucw.cz> escribió: > > > > On Tue 2020-05-12 12:01:36, Álvaro Fernández Rojas wrote: > >> Add support for both configurable HW blinking intervals. > >> > > > > The code could use ... better documentation and better changelog. What > > is the current blinking interval and what other interval does this > > add? > > > > Best regards, > > Pavel > > > >> Signed-off-by: Álvaro Fernández Rojas <noltari@gmail.com> > >> --- > >> v3: add code documentation > >> v2: remove LED from the other interval > >> > >> drivers/leds/leds-bcm6328.c | 83 ++++++++++++++++++++++++++++++------- > >> 1 file changed, 67 insertions(+), 16 deletions(-) > >> > >> diff --git a/drivers/leds/leds-bcm6328.c b/drivers/leds/leds-bcm6328.c > >> index 42e1b7598c3a..050a77591f0b 100644 > >> --- a/drivers/leds/leds-bcm6328.c > >> +++ b/drivers/leds/leds-bcm6328.c > >> @@ -24,12 +24,16 @@ > >> > >> #define BCM6328_LED_MAX_COUNT 24 > >> #define BCM6328_LED_DEF_DELAY 500 > >> +#define BCM6328_LED_INTERVAL_NUM 2 > >> #define BCM6328_LED_INTERVAL_MS 20 > >> > >> #define BCM6328_LED_INTV_MASK 0x3f > >> -#define BCM6328_LED_FAST_INTV_SHIFT 6 > >> -#define BCM6328_LED_FAST_INTV_MASK (BCM6328_LED_INTV_MASK << \ > >> - BCM6328_LED_FAST_INTV_SHIFT) > >> +#define BCM6328_LED_INTV1_SHIFT 0 > >> +#define BCM6328_LED_INTV1_MASK (BCM6328_LED_INTV_MASK << \ > >> + BCM6328_LED_INTV1_SHIFT) > >> +#define BCM6328_LED_INTV2_SHIFT 6 > >> +#define BCM6328_LED_INTV2_MASK (BCM6328_LED_INTV_MASK << \ > >> + BCM6328_LED_INTV2_SHIFT) > >> #define BCM6328_SERIAL_LED_EN BIT(12) > >> #define BCM6328_SERIAL_LED_MUX BIT(13) > >> #define BCM6328_SERIAL_LED_CLK_NPOL BIT(14) > >> @@ -45,8 +49,8 @@ > >> > >> #define BCM6328_LED_MODE_MASK 3 > >> #define BCM6328_LED_MODE_ON 0 > >> -#define BCM6328_LED_MODE_FAST 1 > >> -#define BCM6328_LED_MODE_BLINK 2 > >> +#define BCM6328_LED_MODE_INTV1 1 > >> +#define BCM6328_LED_MODE_INTV2 2 > >> #define BCM6328_LED_MODE_OFF 3 > >> #define BCM6328_LED_SHIFT(X) ((X) << 1) > >> > >> @@ -127,12 +131,18 @@ static void bcm6328_led_set(struct led_classdev *led_cdev, > >> unsigned long flags; > >> > >> spin_lock_irqsave(led->lock, flags); > >> - *(led->blink_leds) &= ~BIT(led->pin); > >> + > >> + /* Remove LED from cached HW blinking intervals */ > >> + led->blink_leds[0] &= ~BIT(led->pin); > >> + led->blink_leds[1] &= ~BIT(led->pin); > >> + > >> + /* Set LED on/off */ > >> if ((led->active_low && value == LED_OFF) || > >> (!led->active_low && value != LED_OFF)) > >> bcm6328_led_mode(led, BCM6328_LED_MODE_ON); > >> else > >> bcm6328_led_mode(led, BCM6328_LED_MODE_OFF); > >> + > >> spin_unlock_irqrestore(led->lock, flags); > >> } > >> > >> @@ -176,20 +186,59 @@ static int bcm6328_blink_set(struct led_classdev *led_cdev, > >> } > >> > >> spin_lock_irqsave(led->lock, flags); > >> - if (*(led->blink_leds) == 0 || > >> - *(led->blink_leds) == BIT(led->pin) || > >> - *(led->blink_delay) == delay) { > >> + /* > >> + * Check if any of the two configurable HW blinking intervals is > >> + * available: > >> + * 1. No LEDs assigned to the HW blinking interval. > >> + * 2. LEDs with the same delay assigned. > >> + */ > >> + if (led->blink_leds[0] == 0 || > >> + led->blink_leds[0] == BIT(led->pin) || > >> + led->blink_delay[0] == delay) { > >> + unsigned long val; > >> + > >> + /* Add LED to the first HW blinking interval cache */ > >> + led->blink_leds[0] |= BIT(led->pin); > >> + > >> + /* Remove LED from the second HW blinking interval cache */ > >> + led->blink_leds[1] &= ~BIT(led->pin); > >> + > >> + /* Cache the LED in the first HW blinking interval delay */ > >> + led->blink_delay[0] = delay; > >> + > >> + /* Update the delay for the first HW blinking interval */ > >> + val = bcm6328_led_read(led->mem + BCM6328_REG_INIT); > >> + val &= ~BCM6328_LED_INTV1_MASK; > >> + val |= (delay << BCM6328_LED_INTV1_SHIFT); > >> + bcm6328_led_write(led->mem + BCM6328_REG_INIT, val); > >> + > >> + /* Set the LED to first HW blinking interval */ > >> + bcm6328_led_mode(led, BCM6328_LED_MODE_INTV1); > >> + > >> + rc = 0; > >> + } else if (led->blink_leds[1] == 0 || > >> + led->blink_leds[1] == BIT(led->pin) || > >> + led->blink_delay[1] == delay) { > >> unsigned long val; > >> > >> - *(led->blink_leds) |= BIT(led->pin); > >> - *(led->blink_delay) = delay; > >> + /* Remove LED from the first HW blinking interval */ > >> + led->blink_leds[0] &= ~BIT(led->pin); > >> + > >> + /* Add LED to the second HW blinking interval */ > >> + led->blink_leds[1] |= BIT(led->pin); > >> > >> + /* Cache the LED in the first HW blinking interval delay */ > >> + led->blink_delay[1] = delay; > >> + > >> + /* Update the delay for the second HW blinking interval */ > >> val = bcm6328_led_read(led->mem + BCM6328_REG_INIT); > >> - val &= ~BCM6328_LED_FAST_INTV_MASK; > >> - val |= (delay << BCM6328_LED_FAST_INTV_SHIFT); > >> + val &= ~BCM6328_LED_INTV2_MASK; > >> + val |= (delay << BCM6328_LED_INTV2_SHIFT); > >> bcm6328_led_write(led->mem + BCM6328_REG_INIT, val); > >> > >> - bcm6328_led_mode(led, BCM6328_LED_MODE_BLINK); > >> + /* Set the LED to second HW blinking interval */ > >> + bcm6328_led_mode(led, BCM6328_LED_MODE_INTV2); > >> + > >> rc = 0; > >> } else { > >> dev_dbg(led_cdev->dev, > >> @@ -358,11 +407,13 @@ static int bcm6328_leds_probe(struct platform_device *pdev) > >> if (!lock) > >> return -ENOMEM; > >> > >> - blink_leds = devm_kzalloc(dev, sizeof(*blink_leds), GFP_KERNEL); > >> + blink_leds = devm_kcalloc(dev, BCM6328_LED_INTERVAL_NUM, > >> + sizeof(*blink_leds), GFP_KERNEL); > >> if (!blink_leds) > >> return -ENOMEM; > >> > >> - blink_delay = devm_kzalloc(dev, sizeof(*blink_delay), GFP_KERNEL); > >> + blink_delay = devm_kcalloc(dev, BCM6328_LED_INTERVAL_NUM, > >> + sizeof(*blink_delay), GFP_KERNEL); > >> if (!blink_delay) > >> return -ENOMEM; > >> > >> -- > >> 2.26.2 > > > > -- > > (english) http://www.livejournal.com/~pavelmachek > > (cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html -- (english) http://www.livejournal.com/~pavelmachek (cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html [-- Attachment #2: signature.asc --] [-- Type: application/pgp-signature, Size: 195 bytes --] ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH v3] leds-bcm6328: support second hw blinking interval 2020-06-04 13:40 ` Pavel Machek @ 2020-06-04 13:43 ` Álvaro Fernández Rojas 2020-06-04 13:53 ` Pavel Machek 0 siblings, 1 reply; 14+ messages in thread From: Álvaro Fernández Rojas @ 2020-06-04 13:43 UTC (permalink / raw) To: Pavel Machek; +Cc: linux-leds, jacek.anaszewski, Jonas Gorski, rpurdie Hi Pavel, > El 4 jun 2020, a las 15:40, Pavel Machek <pavel@ucw.cz> escribió: > > On Thu 2020-06-04 15:35:06, Álvaro Fernández Rojas wrote: >> HI Pavel, >> >> Right now, only one hw blinking interval is supported, but up to two hw blinking intervals can be configured in the controller. >> This patch just adds support for the second hw blinking interval. >> >> BTW, I will send v4 which should clarify that each LED can be configured in 4 modes: >> 0: On >> 1: HW Blinking (Interval 1) >> 2: HW Blinking (Interval 2) >> 3: Off >> >> Until now we were only using 3 modes: >> 0: On >> 1: HW Blinking (Interval 1) >> 3: Off > > So the issue is .... there are like 5 LEDs and they can be on or off > or blinking with at most 2 different intervals? > Pavel Yes, the controller supports two different blinking delays (intervals), and LEDs can either be on, off or assigned to one of the two blinking delays. However, the current upstream controller only supports one blinking delay. This patch just adds support for both blinking delays instead of just one. > >>> El 4 jun 2020, a las 15:24, Pavel Machek <pavel@ucw.cz> escribió: >>> >>> On Tue 2020-05-12 12:01:36, Álvaro Fernández Rojas wrote: >>>> Add support for both configurable HW blinking intervals. >>>> >>> >>> The code could use ... better documentation and better changelog. What >>> is the current blinking interval and what other interval does this >>> add? >>> >>> Best regards, >>> Pavel >>> >>>> Signed-off-by: Álvaro Fernández Rojas <noltari@gmail.com> >>>> --- >>>> v3: add code documentation >>>> v2: remove LED from the other interval >>>> >>>> drivers/leds/leds-bcm6328.c | 83 ++++++++++++++++++++++++++++++------- >>>> 1 file changed, 67 insertions(+), 16 deletions(-) >>>> >>>> diff --git a/drivers/leds/leds-bcm6328.c b/drivers/leds/leds-bcm6328.c >>>> index 42e1b7598c3a..050a77591f0b 100644 >>>> --- a/drivers/leds/leds-bcm6328.c >>>> +++ b/drivers/leds/leds-bcm6328.c >>>> @@ -24,12 +24,16 @@ >>>> >>>> #define BCM6328_LED_MAX_COUNT 24 >>>> #define BCM6328_LED_DEF_DELAY 500 >>>> +#define BCM6328_LED_INTERVAL_NUM 2 >>>> #define BCM6328_LED_INTERVAL_MS 20 >>>> >>>> #define BCM6328_LED_INTV_MASK 0x3f >>>> -#define BCM6328_LED_FAST_INTV_SHIFT 6 >>>> -#define BCM6328_LED_FAST_INTV_MASK (BCM6328_LED_INTV_MASK << \ >>>> - BCM6328_LED_FAST_INTV_SHIFT) >>>> +#define BCM6328_LED_INTV1_SHIFT 0 >>>> +#define BCM6328_LED_INTV1_MASK (BCM6328_LED_INTV_MASK << \ >>>> + BCM6328_LED_INTV1_SHIFT) >>>> +#define BCM6328_LED_INTV2_SHIFT 6 >>>> +#define BCM6328_LED_INTV2_MASK (BCM6328_LED_INTV_MASK << \ >>>> + BCM6328_LED_INTV2_SHIFT) >>>> #define BCM6328_SERIAL_LED_EN BIT(12) >>>> #define BCM6328_SERIAL_LED_MUX BIT(13) >>>> #define BCM6328_SERIAL_LED_CLK_NPOL BIT(14) >>>> @@ -45,8 +49,8 @@ >>>> >>>> #define BCM6328_LED_MODE_MASK 3 >>>> #define BCM6328_LED_MODE_ON 0 >>>> -#define BCM6328_LED_MODE_FAST 1 >>>> -#define BCM6328_LED_MODE_BLINK 2 >>>> +#define BCM6328_LED_MODE_INTV1 1 >>>> +#define BCM6328_LED_MODE_INTV2 2 >>>> #define BCM6328_LED_MODE_OFF 3 >>>> #define BCM6328_LED_SHIFT(X) ((X) << 1) >>>> >>>> @@ -127,12 +131,18 @@ static void bcm6328_led_set(struct led_classdev *led_cdev, >>>> unsigned long flags; >>>> >>>> spin_lock_irqsave(led->lock, flags); >>>> - *(led->blink_leds) &= ~BIT(led->pin); >>>> + >>>> + /* Remove LED from cached HW blinking intervals */ >>>> + led->blink_leds[0] &= ~BIT(led->pin); >>>> + led->blink_leds[1] &= ~BIT(led->pin); >>>> + >>>> + /* Set LED on/off */ >>>> if ((led->active_low && value == LED_OFF) || >>>> (!led->active_low && value != LED_OFF)) >>>> bcm6328_led_mode(led, BCM6328_LED_MODE_ON); >>>> else >>>> bcm6328_led_mode(led, BCM6328_LED_MODE_OFF); >>>> + >>>> spin_unlock_irqrestore(led->lock, flags); >>>> } >>>> >>>> @@ -176,20 +186,59 @@ static int bcm6328_blink_set(struct led_classdev *led_cdev, >>>> } >>>> >>>> spin_lock_irqsave(led->lock, flags); >>>> - if (*(led->blink_leds) == 0 || >>>> - *(led->blink_leds) == BIT(led->pin) || >>>> - *(led->blink_delay) == delay) { >>>> + /* >>>> + * Check if any of the two configurable HW blinking intervals is >>>> + * available: >>>> + * 1. No LEDs assigned to the HW blinking interval. >>>> + * 2. LEDs with the same delay assigned. >>>> + */ >>>> + if (led->blink_leds[0] == 0 || >>>> + led->blink_leds[0] == BIT(led->pin) || >>>> + led->blink_delay[0] == delay) { >>>> + unsigned long val; >>>> + >>>> + /* Add LED to the first HW blinking interval cache */ >>>> + led->blink_leds[0] |= BIT(led->pin); >>>> + >>>> + /* Remove LED from the second HW blinking interval cache */ >>>> + led->blink_leds[1] &= ~BIT(led->pin); >>>> + >>>> + /* Cache the LED in the first HW blinking interval delay */ >>>> + led->blink_delay[0] = delay; >>>> + >>>> + /* Update the delay for the first HW blinking interval */ >>>> + val = bcm6328_led_read(led->mem + BCM6328_REG_INIT); >>>> + val &= ~BCM6328_LED_INTV1_MASK; >>>> + val |= (delay << BCM6328_LED_INTV1_SHIFT); >>>> + bcm6328_led_write(led->mem + BCM6328_REG_INIT, val); >>>> + >>>> + /* Set the LED to first HW blinking interval */ >>>> + bcm6328_led_mode(led, BCM6328_LED_MODE_INTV1); >>>> + >>>> + rc = 0; >>>> + } else if (led->blink_leds[1] == 0 || >>>> + led->blink_leds[1] == BIT(led->pin) || >>>> + led->blink_delay[1] == delay) { >>>> unsigned long val; >>>> >>>> - *(led->blink_leds) |= BIT(led->pin); >>>> - *(led->blink_delay) = delay; >>>> + /* Remove LED from the first HW blinking interval */ >>>> + led->blink_leds[0] &= ~BIT(led->pin); >>>> + >>>> + /* Add LED to the second HW blinking interval */ >>>> + led->blink_leds[1] |= BIT(led->pin); >>>> >>>> + /* Cache the LED in the first HW blinking interval delay */ >>>> + led->blink_delay[1] = delay; >>>> + >>>> + /* Update the delay for the second HW blinking interval */ >>>> val = bcm6328_led_read(led->mem + BCM6328_REG_INIT); >>>> - val &= ~BCM6328_LED_FAST_INTV_MASK; >>>> - val |= (delay << BCM6328_LED_FAST_INTV_SHIFT); >>>> + val &= ~BCM6328_LED_INTV2_MASK; >>>> + val |= (delay << BCM6328_LED_INTV2_SHIFT); >>>> bcm6328_led_write(led->mem + BCM6328_REG_INIT, val); >>>> >>>> - bcm6328_led_mode(led, BCM6328_LED_MODE_BLINK); >>>> + /* Set the LED to second HW blinking interval */ >>>> + bcm6328_led_mode(led, BCM6328_LED_MODE_INTV2); >>>> + >>>> rc = 0; >>>> } else { >>>> dev_dbg(led_cdev->dev, >>>> @@ -358,11 +407,13 @@ static int bcm6328_leds_probe(struct platform_device *pdev) >>>> if (!lock) >>>> return -ENOMEM; >>>> >>>> - blink_leds = devm_kzalloc(dev, sizeof(*blink_leds), GFP_KERNEL); >>>> + blink_leds = devm_kcalloc(dev, BCM6328_LED_INTERVAL_NUM, >>>> + sizeof(*blink_leds), GFP_KERNEL); >>>> if (!blink_leds) >>>> return -ENOMEM; >>>> >>>> - blink_delay = devm_kzalloc(dev, sizeof(*blink_delay), GFP_KERNEL); >>>> + blink_delay = devm_kcalloc(dev, BCM6328_LED_INTERVAL_NUM, >>>> + sizeof(*blink_delay), GFP_KERNEL); >>>> if (!blink_delay) >>>> return -ENOMEM; >>>> >>>> -- >>>> 2.26.2 >>> >>> -- >>> (english) http://www.livejournal.com/~pavelmachek >>> (cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html > > -- > (english) http://www.livejournal.com/~pavelmachek > (cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH v3] leds-bcm6328: support second hw blinking interval 2020-06-04 13:43 ` Álvaro Fernández Rojas @ 2020-06-04 13:53 ` Pavel Machek 2020-06-04 14:00 ` Álvaro Fernández Rojas 0 siblings, 1 reply; 14+ messages in thread From: Pavel Machek @ 2020-06-04 13:53 UTC (permalink / raw) To: Álvaro Fernández Rojas Cc: linux-leds, jacek.anaszewski, Jonas Gorski, rpurdie [-- Attachment #1: Type: text/plain, Size: 1446 bytes --] On Thu 2020-06-04 15:43:35, Álvaro Fernández Rojas wrote: > Hi Pavel, > > > El 4 jun 2020, a las 15:40, Pavel Machek <pavel@ucw.cz> escribió: > > > > On Thu 2020-06-04 15:35:06, Álvaro Fernández Rojas wrote: > >> HI Pavel, > >> > >> Right now, only one hw blinking interval is supported, but up to two hw blinking intervals can be configured in the controller. > >> This patch just adds support for the second hw blinking interval. > >> > >> BTW, I will send v4 which should clarify that each LED can be configured in 4 modes: > >> 0: On > >> 1: HW Blinking (Interval 1) > >> 2: HW Blinking (Interval 2) > >> 3: Off > >> > >> Until now we were only using 3 modes: > >> 0: On > >> 1: HW Blinking (Interval 1) > >> 3: Off > > > > So the issue is .... there are like 5 LEDs and they can be on or off > > or blinking with at most 2 different intervals? > > Pavel > > Yes, the controller supports two different blinking delays (intervals), and LEDs can either be on, off or assigned to one of the two blinking delays. > However, the current upstream controller only supports one blinking delay. This patch just adds support for both blinking delays instead of just one. > Ok. Please put this kind of summary in comment somewhere. Best regards, Pavel -- (english) http://www.livejournal.com/~pavelmachek (cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html [-- Attachment #2: signature.asc --] [-- Type: application/pgp-signature, Size: 195 bytes --] ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH v3] leds-bcm6328: support second hw blinking interval 2020-06-04 13:53 ` Pavel Machek @ 2020-06-04 14:00 ` Álvaro Fernández Rojas 0 siblings, 0 replies; 14+ messages in thread From: Álvaro Fernández Rojas @ 2020-06-04 14:00 UTC (permalink / raw) To: Pavel Machek; +Cc: linux-leds, jacek.anaszewski, Jonas Gorski, rpurdie Sorry, I had just sent v4 without it. However, I’ve now sent v5 with the summary. Best regards, Álvaro. > El 4 jun 2020, a las 15:53, Pavel Machek <pavel@ucw.cz> escribió: > > On Thu 2020-06-04 15:43:35, Álvaro Fernández Rojas wrote: >> Hi Pavel, >> >>> El 4 jun 2020, a las 15:40, Pavel Machek <pavel@ucw.cz> escribió: >>> >>> On Thu 2020-06-04 15:35:06, Álvaro Fernández Rojas wrote: >>>> HI Pavel, >>>> >>>> Right now, only one hw blinking interval is supported, but up to two hw blinking intervals can be configured in the controller. >>>> This patch just adds support for the second hw blinking interval. >>>> >>>> BTW, I will send v4 which should clarify that each LED can be configured in 4 modes: >>>> 0: On >>>> 1: HW Blinking (Interval 1) >>>> 2: HW Blinking (Interval 2) >>>> 3: Off >>>> >>>> Until now we were only using 3 modes: >>>> 0: On >>>> 1: HW Blinking (Interval 1) >>>> 3: Off >>> >>> So the issue is .... there are like 5 LEDs and they can be on or off >>> or blinking with at most 2 different intervals? >>> Pavel >> >> Yes, the controller supports two different blinking delays (intervals), and LEDs can either be on, off or assigned to one of the two blinking delays. >> However, the current upstream controller only supports one blinking delay. This patch just adds support for both blinking delays instead of just one. >> > > Ok. Please put this kind of summary in comment somewhere. > > Best regards, > Pavel > > -- > (english) http://www.livejournal.com/~pavelmachek > (cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html ^ permalink raw reply [flat|nested] 14+ messages in thread
* [PATCH v4] leds-bcm6328: support second hw blinking interval 2020-05-12 10:01 ` [PATCH v3] " Álvaro Fernández Rojas 2020-06-04 13:24 ` Pavel Machek @ 2020-06-04 13:48 ` Álvaro Fernández Rojas 2020-06-04 13:59 ` [PATCH v5] " Álvaro Fernández Rojas 1 sibling, 1 reply; 14+ messages in thread From: Álvaro Fernández Rojas @ 2020-06-04 13:48 UTC (permalink / raw) To: linux-kernel, linux-leds, jacek.anaszewski, jonas.gorski, rpurdie, pavel Cc: Álvaro Fernández Rojas Add support for both configurable HW blinking intervals. Signed-off-by: Álvaro Fernández Rojas <noltari@gmail.com> --- v4: improve code documentation and use blink instead of intervals, which should make the code much more clear to understand v3: add code documentation v2: remove LED from the other blinking interval drivers/leds/leds-bcm6328.c | 97 ++++++++++++++++++++++++++++--------- 1 file changed, 75 insertions(+), 22 deletions(-) diff --git a/drivers/leds/leds-bcm6328.c b/drivers/leds/leds-bcm6328.c index 42e1b7598c3a..bad7efb75112 100644 --- a/drivers/leds/leds-bcm6328.c +++ b/drivers/leds/leds-bcm6328.c @@ -24,12 +24,17 @@ #define BCM6328_LED_MAX_COUNT 24 #define BCM6328_LED_DEF_DELAY 500 -#define BCM6328_LED_INTERVAL_MS 20 -#define BCM6328_LED_INTV_MASK 0x3f -#define BCM6328_LED_FAST_INTV_SHIFT 6 -#define BCM6328_LED_FAST_INTV_MASK (BCM6328_LED_INTV_MASK << \ - BCM6328_LED_FAST_INTV_SHIFT) +#define BCM6328_LED_BLINK_DELAYS 2 +#define BCM6328_LED_BLINK_MS 20 + +#define BCM6328_LED_BLINK_MASK 0x3f +#define BCM6328_LED_BLINK1_SHIFT 0 +#define BCM6328_LED_BLINK1_MASK (BCM6328_LED_BLINK_MASK << \ + BCM6328_LED_BLINK1_SHIFT) +#define BCM6328_LED_BLINK2_SHIFT 6 +#define BCM6328_LED_BLINK2_MASK (BCM6328_LED_BLINK_MASK << \ + BCM6328_LED_BLINK2_SHIFT) #define BCM6328_SERIAL_LED_EN BIT(12) #define BCM6328_SERIAL_LED_MUX BIT(13) #define BCM6328_SERIAL_LED_CLK_NPOL BIT(14) @@ -45,8 +50,8 @@ #define BCM6328_LED_MODE_MASK 3 #define BCM6328_LED_MODE_ON 0 -#define BCM6328_LED_MODE_FAST 1 -#define BCM6328_LED_MODE_BLINK 2 +#define BCM6328_LED_MODE_BLINK1 1 +#define BCM6328_LED_MODE_BLINK2 2 #define BCM6328_LED_MODE_OFF 3 #define BCM6328_LED_SHIFT(X) ((X) << 1) @@ -127,12 +132,18 @@ static void bcm6328_led_set(struct led_classdev *led_cdev, unsigned long flags; spin_lock_irqsave(led->lock, flags); - *(led->blink_leds) &= ~BIT(led->pin); + + /* Remove LED from cached HW blinking intervals */ + led->blink_leds[0] &= ~BIT(led->pin); + led->blink_leds[1] &= ~BIT(led->pin); + + /* Set LED on/off */ if ((led->active_low && value == LED_OFF) || (!led->active_low && value != LED_OFF)) bcm6328_led_mode(led, BCM6328_LED_MODE_ON); else bcm6328_led_mode(led, BCM6328_LED_MODE_OFF); + spin_unlock_irqrestore(led->lock, flags); } @@ -140,8 +151,8 @@ static unsigned long bcm6328_blink_delay(unsigned long delay) { unsigned long bcm6328_delay; - bcm6328_delay = delay + BCM6328_LED_INTERVAL_MS / 2; - bcm6328_delay = bcm6328_delay / BCM6328_LED_INTERVAL_MS; + bcm6328_delay = delay + BCM6328_LED_BLINK_MS / 2; + bcm6328_delay = bcm6328_delay / BCM6328_LED_BLINK_MS; if (bcm6328_delay == 0) bcm6328_delay = 1; @@ -168,28 +179,68 @@ static int bcm6328_blink_set(struct led_classdev *led_cdev, return -EINVAL; } - if (delay > BCM6328_LED_INTV_MASK) { + if (delay > BCM6328_LED_BLINK_MASK) { dev_dbg(led_cdev->dev, "fallback to soft blinking (delay > %ums)\n", - BCM6328_LED_INTV_MASK * BCM6328_LED_INTERVAL_MS); + BCM6328_LED_BLINK_MASK * BCM6328_LED_BLINK_MS); return -EINVAL; } spin_lock_irqsave(led->lock, flags); - if (*(led->blink_leds) == 0 || - *(led->blink_leds) == BIT(led->pin) || - *(led->blink_delay) == delay) { + /* + * Check if any of the two configurable HW blinking intervals is + * available: + * 1. No LEDs assigned to the HW blinking interval. + * 2. Only this LED is assigned to the HW blinking interval. + * 3. LEDs with the same delay assigned. + */ + if (led->blink_leds[0] == 0 || + led->blink_leds[0] == BIT(led->pin) || + led->blink_delay[0] == delay) { unsigned long val; - *(led->blink_leds) |= BIT(led->pin); - *(led->blink_delay) = delay; + /* Add LED to the first HW blinking interval cache */ + led->blink_leds[0] |= BIT(led->pin); + + /* Remove LED from the second HW blinking interval cache */ + led->blink_leds[1] &= ~BIT(led->pin); + /* Cache first HW blinking interval delay */ + led->blink_delay[0] = delay; + + /* Update the delay for the first HW blinking interval */ val = bcm6328_led_read(led->mem + BCM6328_REG_INIT); - val &= ~BCM6328_LED_FAST_INTV_MASK; - val |= (delay << BCM6328_LED_FAST_INTV_SHIFT); + val &= ~BCM6328_LED_BLINK1_MASK; + val |= (delay << BCM6328_LED_BLINK1_SHIFT); bcm6328_led_write(led->mem + BCM6328_REG_INIT, val); - bcm6328_led_mode(led, BCM6328_LED_MODE_BLINK); + /* Set the LED to first HW blinking interval */ + bcm6328_led_mode(led, BCM6328_LED_MODE_BLINK1); + + rc = 0; + } else if (led->blink_leds[1] == 0 || + led->blink_leds[1] == BIT(led->pin) || + led->blink_delay[1] == delay) { + unsigned long val; + + /* Remove LED from the first HW blinking interval */ + led->blink_leds[0] &= ~BIT(led->pin); + + /* Add LED to the second HW blinking interval */ + led->blink_leds[1] |= BIT(led->pin); + + /* Cache second HW blinking interval delay */ + led->blink_delay[1] = delay; + + /* Update the delay for the second HW blinking interval */ + val = bcm6328_led_read(led->mem + BCM6328_REG_INIT); + val &= ~BCM6328_LED_BLINK2_MASK; + val |= (delay << BCM6328_LED_BLINK2_SHIFT); + bcm6328_led_write(led->mem + BCM6328_REG_INIT, val); + + /* Set the LED to second HW blinking interval */ + bcm6328_led_mode(led, BCM6328_LED_MODE_BLINK2); + rc = 0; } else { dev_dbg(led_cdev->dev, @@ -358,11 +409,13 @@ static int bcm6328_leds_probe(struct platform_device *pdev) if (!lock) return -ENOMEM; - blink_leds = devm_kzalloc(dev, sizeof(*blink_leds), GFP_KERNEL); + blink_leds = devm_kcalloc(dev, BCM6328_LED_BLINK_DELAYS, + sizeof(*blink_leds), GFP_KERNEL); if (!blink_leds) return -ENOMEM; - blink_delay = devm_kzalloc(dev, sizeof(*blink_delay), GFP_KERNEL); + blink_delay = devm_kcalloc(dev, BCM6328_LED_BLINK_DELAYS, + sizeof(*blink_delay), GFP_KERNEL); if (!blink_delay) return -ENOMEM; -- 2.26.2 ^ permalink raw reply related [flat|nested] 14+ messages in thread
* [PATCH v5] leds-bcm6328: support second hw blinking interval 2020-06-04 13:48 ` [PATCH v4] " Álvaro Fernández Rojas @ 2020-06-04 13:59 ` Álvaro Fernández Rojas 2020-06-04 14:01 ` Pavel Machek 0 siblings, 1 reply; 14+ messages in thread From: Álvaro Fernández Rojas @ 2020-06-04 13:59 UTC (permalink / raw) To: linux-kernel, linux-leds, jacek.anaszewski, jonas.gorski, rpurdie, pavel Cc: Álvaro Fernández Rojas Right now the driver uses only 3 LED modes: 0: On 1: HW Blinking (Interval 1) 3: Off However, the controller supports a second HW blinking interval, which results in 4 possible LED modes: 0: On 1: HW Blinking (Interval 1) 2: HW Blinking (Interval 2) 3: Off Signed-off-by: Álvaro Fernández Rojas <noltari@gmail.com> --- v5: add commit description including LED modes clarification v4: improve code documentation and use blink instead of intervals, which should make the code much more clear to understand v3: add code documentation v2: remove LED from the other blinking interval drivers/leds/leds-bcm6328.c | 97 ++++++++++++++++++++++++++++--------- 1 file changed, 75 insertions(+), 22 deletions(-) diff --git a/drivers/leds/leds-bcm6328.c b/drivers/leds/leds-bcm6328.c index 42e1b7598c3a..bad7efb75112 100644 --- a/drivers/leds/leds-bcm6328.c +++ b/drivers/leds/leds-bcm6328.c @@ -24,12 +24,17 @@ #define BCM6328_LED_MAX_COUNT 24 #define BCM6328_LED_DEF_DELAY 500 -#define BCM6328_LED_INTERVAL_MS 20 -#define BCM6328_LED_INTV_MASK 0x3f -#define BCM6328_LED_FAST_INTV_SHIFT 6 -#define BCM6328_LED_FAST_INTV_MASK (BCM6328_LED_INTV_MASK << \ - BCM6328_LED_FAST_INTV_SHIFT) +#define BCM6328_LED_BLINK_DELAYS 2 +#define BCM6328_LED_BLINK_MS 20 + +#define BCM6328_LED_BLINK_MASK 0x3f +#define BCM6328_LED_BLINK1_SHIFT 0 +#define BCM6328_LED_BLINK1_MASK (BCM6328_LED_BLINK_MASK << \ + BCM6328_LED_BLINK1_SHIFT) +#define BCM6328_LED_BLINK2_SHIFT 6 +#define BCM6328_LED_BLINK2_MASK (BCM6328_LED_BLINK_MASK << \ + BCM6328_LED_BLINK2_SHIFT) #define BCM6328_SERIAL_LED_EN BIT(12) #define BCM6328_SERIAL_LED_MUX BIT(13) #define BCM6328_SERIAL_LED_CLK_NPOL BIT(14) @@ -45,8 +50,8 @@ #define BCM6328_LED_MODE_MASK 3 #define BCM6328_LED_MODE_ON 0 -#define BCM6328_LED_MODE_FAST 1 -#define BCM6328_LED_MODE_BLINK 2 +#define BCM6328_LED_MODE_BLINK1 1 +#define BCM6328_LED_MODE_BLINK2 2 #define BCM6328_LED_MODE_OFF 3 #define BCM6328_LED_SHIFT(X) ((X) << 1) @@ -127,12 +132,18 @@ static void bcm6328_led_set(struct led_classdev *led_cdev, unsigned long flags; spin_lock_irqsave(led->lock, flags); - *(led->blink_leds) &= ~BIT(led->pin); + + /* Remove LED from cached HW blinking intervals */ + led->blink_leds[0] &= ~BIT(led->pin); + led->blink_leds[1] &= ~BIT(led->pin); + + /* Set LED on/off */ if ((led->active_low && value == LED_OFF) || (!led->active_low && value != LED_OFF)) bcm6328_led_mode(led, BCM6328_LED_MODE_ON); else bcm6328_led_mode(led, BCM6328_LED_MODE_OFF); + spin_unlock_irqrestore(led->lock, flags); } @@ -140,8 +151,8 @@ static unsigned long bcm6328_blink_delay(unsigned long delay) { unsigned long bcm6328_delay; - bcm6328_delay = delay + BCM6328_LED_INTERVAL_MS / 2; - bcm6328_delay = bcm6328_delay / BCM6328_LED_INTERVAL_MS; + bcm6328_delay = delay + BCM6328_LED_BLINK_MS / 2; + bcm6328_delay = bcm6328_delay / BCM6328_LED_BLINK_MS; if (bcm6328_delay == 0) bcm6328_delay = 1; @@ -168,28 +179,68 @@ static int bcm6328_blink_set(struct led_classdev *led_cdev, return -EINVAL; } - if (delay > BCM6328_LED_INTV_MASK) { + if (delay > BCM6328_LED_BLINK_MASK) { dev_dbg(led_cdev->dev, "fallback to soft blinking (delay > %ums)\n", - BCM6328_LED_INTV_MASK * BCM6328_LED_INTERVAL_MS); + BCM6328_LED_BLINK_MASK * BCM6328_LED_BLINK_MS); return -EINVAL; } spin_lock_irqsave(led->lock, flags); - if (*(led->blink_leds) == 0 || - *(led->blink_leds) == BIT(led->pin) || - *(led->blink_delay) == delay) { + /* + * Check if any of the two configurable HW blinking intervals is + * available: + * 1. No LEDs assigned to the HW blinking interval. + * 2. Only this LED is assigned to the HW blinking interval. + * 3. LEDs with the same delay assigned. + */ + if (led->blink_leds[0] == 0 || + led->blink_leds[0] == BIT(led->pin) || + led->blink_delay[0] == delay) { unsigned long val; - *(led->blink_leds) |= BIT(led->pin); - *(led->blink_delay) = delay; + /* Add LED to the first HW blinking interval cache */ + led->blink_leds[0] |= BIT(led->pin); + + /* Remove LED from the second HW blinking interval cache */ + led->blink_leds[1] &= ~BIT(led->pin); + /* Cache first HW blinking interval delay */ + led->blink_delay[0] = delay; + + /* Update the delay for the first HW blinking interval */ val = bcm6328_led_read(led->mem + BCM6328_REG_INIT); - val &= ~BCM6328_LED_FAST_INTV_MASK; - val |= (delay << BCM6328_LED_FAST_INTV_SHIFT); + val &= ~BCM6328_LED_BLINK1_MASK; + val |= (delay << BCM6328_LED_BLINK1_SHIFT); bcm6328_led_write(led->mem + BCM6328_REG_INIT, val); - bcm6328_led_mode(led, BCM6328_LED_MODE_BLINK); + /* Set the LED to first HW blinking interval */ + bcm6328_led_mode(led, BCM6328_LED_MODE_BLINK1); + + rc = 0; + } else if (led->blink_leds[1] == 0 || + led->blink_leds[1] == BIT(led->pin) || + led->blink_delay[1] == delay) { + unsigned long val; + + /* Remove LED from the first HW blinking interval */ + led->blink_leds[0] &= ~BIT(led->pin); + + /* Add LED to the second HW blinking interval */ + led->blink_leds[1] |= BIT(led->pin); + + /* Cache second HW blinking interval delay */ + led->blink_delay[1] = delay; + + /* Update the delay for the second HW blinking interval */ + val = bcm6328_led_read(led->mem + BCM6328_REG_INIT); + val &= ~BCM6328_LED_BLINK2_MASK; + val |= (delay << BCM6328_LED_BLINK2_SHIFT); + bcm6328_led_write(led->mem + BCM6328_REG_INIT, val); + + /* Set the LED to second HW blinking interval */ + bcm6328_led_mode(led, BCM6328_LED_MODE_BLINK2); + rc = 0; } else { dev_dbg(led_cdev->dev, @@ -358,11 +409,13 @@ static int bcm6328_leds_probe(struct platform_device *pdev) if (!lock) return -ENOMEM; - blink_leds = devm_kzalloc(dev, sizeof(*blink_leds), GFP_KERNEL); + blink_leds = devm_kcalloc(dev, BCM6328_LED_BLINK_DELAYS, + sizeof(*blink_leds), GFP_KERNEL); if (!blink_leds) return -ENOMEM; - blink_delay = devm_kzalloc(dev, sizeof(*blink_delay), GFP_KERNEL); + blink_delay = devm_kcalloc(dev, BCM6328_LED_BLINK_DELAYS, + sizeof(*blink_delay), GFP_KERNEL); if (!blink_delay) return -ENOMEM; -- 2.26.2 ^ permalink raw reply related [flat|nested] 14+ messages in thread
* Re: [PATCH v5] leds-bcm6328: support second hw blinking interval 2020-06-04 13:59 ` [PATCH v5] " Álvaro Fernández Rojas @ 2020-06-04 14:01 ` Pavel Machek 0 siblings, 0 replies; 14+ messages in thread From: Pavel Machek @ 2020-06-04 14:01 UTC (permalink / raw) To: Álvaro Fernández Rojas Cc: linux-kernel, linux-leds, jacek.anaszewski, jonas.gorski, rpurdie [-- Attachment #1: Type: text/plain, Size: 616 bytes --] On Thu 2020-06-04 15:59:05, Álvaro Fernández Rojas wrote: > Right now the driver uses only 3 LED modes: > 0: On > 1: HW Blinking (Interval 1) > 3: Off > > However, the controller supports a second HW blinking interval, which results > in 4 possible LED modes: > 0: On > 1: HW Blinking (Interval 1) > 2: HW Blinking (Interval 2) > 3: Off > Signed-off-by: Álvaro Fernández Rojas <noltari@gmail.com> Thanks, applied. Best regards, Pavel -- (english) http://www.livejournal.com/~pavelmachek (cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html [-- Attachment #2: signature.asc --] [-- Type: application/pgp-signature, Size: 195 bytes --] ^ permalink raw reply [flat|nested] 14+ messages in thread
end of thread, other threads:[~2020-06-04 14:01 UTC | newest] Thread overview: 14+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2020-04-24 12:46 [PATCH] leds-bcm6328: support second hw blinking interval Álvaro Fernández Rojas 2020-04-24 13:32 ` [PATCH v2] " Álvaro Fernández Rojas 2020-04-25 21:14 ` Pavel Machek 2020-04-26 8:39 ` Álvaro Fernández Rojas 2020-05-12 10:01 ` [PATCH v3] " Álvaro Fernández Rojas 2020-06-04 13:24 ` Pavel Machek 2020-06-04 13:35 ` Álvaro Fernández Rojas 2020-06-04 13:40 ` Pavel Machek 2020-06-04 13:43 ` Álvaro Fernández Rojas 2020-06-04 13:53 ` Pavel Machek 2020-06-04 14:00 ` Álvaro Fernández Rojas 2020-06-04 13:48 ` [PATCH v4] " Álvaro Fernández Rojas 2020-06-04 13:59 ` [PATCH v5] " Álvaro Fernández Rojas 2020-06-04 14:01 ` Pavel Machek
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).