Re: [PATCH 1/2] leds: bcm6328: improve blink support

[Jacek Anaszewski <j.anaszewski@samsung.com>]

Hi Alvaro,

Thanks for the patch. Applied with modifications mentioned below.
Please always use scripts/checkpatch.pl before patch submission,
it allows to detect this kind of problems.

On 12/16/2015 09:13 PM, Álvaro Fernández Rojas wrote:
> BCM6328 controller has a margin of 20ms per blink step, which means that we
> can only set it to 20, 40, 60 ... 1260 ms (0x3f * 20ms).
> However, when checking if delay_on == delay_off, we were not considering the
> case when the user had set delay_on=20 and delay_off=21, since this will
> cause the driver to fallback to software blinking.
> This update fixes this issue and improves blink steps by rounding them
> in a more sensible way. Now 30-49ms is rounded to 40 ms, and previous behaviour
> implied 40-59ms being rounded to 40 ms.

Reformatted commit message so that it didn't break 75 characters line
length limit.

>
> Signed-off-by: Álvaro Fernández Rojas <noltari@gmail.com>
> ---
>   drivers/leds/leds-bcm6328.c | 18 +++++++++++++-----
>   1 file changed, 13 insertions(+), 5 deletions(-)
>
> diff --git a/drivers/leds/leds-bcm6328.c b/drivers/leds/leds-bcm6328.c
> index 47f7c9f..0d4d274 100644
> --- a/drivers/leds/leds-bcm6328.c
> +++ b/drivers/leds/leds-bcm6328.c
> @@ -140,6 +140,16 @@ static void bcm6328_led_set(struct led_classdev *led_cdev,
>   	spin_unlock_irqrestore(led->lock, flags);
>   }
>
> +static unsigned long bcm6328_blink_delay(unsigned long delay)
> +{
> +	unsigned long bcm6328_delay;

added empty line here

> +	bcm6328_delay = delay + BCM6328_LED_INTERVAL_MS / 2;
> +	bcm6328_delay = bcm6328_delay / BCM6328_LED_INTERVAL_MS;
> +	if (bcm6328_delay == 0)
> +		bcm6328_delay = 1;

and here

> +	return bcm6328_delay;
> +}
> +
>   static int bcm6328_blink_set(struct led_classdev *led_cdev,
>   			     unsigned long *delay_on, unsigned long *delay_off)
>   {
> @@ -153,16 +163,14 @@ static int bcm6328_blink_set(struct led_classdev *led_cdev,
>   	if (!*delay_off)
>   		*delay_off = BCM6328_LED_DEF_DELAY;
>
> -	if (*delay_on != *delay_off) {
> +	delay = bcm6328_blink_delay(*delay_on);
> +	if (delay != bcm6328_blink_delay(*delay_off)) {
>   		dev_dbg(led_cdev->dev,
>   			"fallback to soft blinking (delay_on != delay_off)\n");
>   		return -EINVAL;
>   	}
>
> -	delay = *delay_on / BCM6328_LED_INTERVAL_MS;
> -	if (delay == 0) {
> -		delay = 1;
> -	} else if (delay > BCM6328_LED_INTV_MASK) {
> +	if (delay > BCM6328_LED_INTV_MASK) {
>   		dev_dbg(led_cdev->dev,
>   			"fallback to soft blinking (delay > %ums)\n",
>   			BCM6328_LED_INTV_MASK * BCM6328_LED_INTERVAL_MS);
>


-- 
Best Regards,
Jacek Anaszewski