Re: [PATCH] leds: triggers: add invert to heartbeat

[Jacek Anaszewski]

Hi Jiri,

On 10/07/2015 04:14 PM, JiÅÃ Prchal wrote:
> Hi Jacek,
> comments below...
>
> On 7.10.2015 15:32, Jacek Anaszewski wrote:
> > Hi Jiri,
> >
> > Thanks for the patch. It's nice in general, but please see my
> > comments below.
> >
> > On 10/07/2015 11:31 AM, Jiri Prchal wrote:
> > > This patcht adds possibility to invert heartbeat blinking. The
> > > inverted LED is
> > > more time ON then OFF.
> > > It's because it looks better when the heartbeat LED is next to other
> > > LED which
> > > is most time ON.
> > > The invert value is exported same way via sysfs in file invert like
> > > oneshot. I
> > > get inspiration from this trigger.
> >
> > Please adjust commit message so as it wouldn't exceed 75 characters line
> > length limit. scripts/checkpatch.pl is useful for  catching this kind of
> > problems.
> >
> > > Signed-off-by: Jiri Prchal <jiri.prchal@xxxxxxxxxxx>
> > > ---
> > >   drivers/leds/trigger/ledtrig-heartbeat.c | 49
> > > ++++++++++++++++++++++++++++++--
> > >   1 file changed, 47 insertions(+), 2 deletions(-)
> > >
> > > diff --git a/drivers/leds/trigger/ledtrig-heartbeat.c
> > > b/drivers/leds/trigger/ledtrig-heartbeat.c
> > > index fea6871..c09c30b 100644
> > > --- a/drivers/leds/trigger/ledtrig-heartbeat.c
> > > +++ b/drivers/leds/trigger/ledtrig-heartbeat.c
> > > @@ -27,6 +27,7 @@ struct heartbeat_trig_data {
> > >       unsigned int phase;
> > >       unsigned int period;
> > >       struct timer_list timer;
> > > +    unsigned int invert;
> > >   };
> > >
> > >   static void led_heartbeat_function(unsigned long data)
> > > @@ -56,21 +57,27 @@ static void led_heartbeat_function(unsigned long
> > > data)
> > >               msecs_to_jiffies(heartbeat_data->period);
> > >           delay = msecs_to_jiffies(70);
> > >           heartbeat_data->phase++;
> > > -        brightness = led_cdev->max_brightness;
> > > +        if (!heartbeat_data->invert)
> > > +            brightness = led_cdev->max_brightness;
> > >           break;
> > >       case 1:
> > >           delay = heartbeat_data->period / 4 - msecs_to_jiffies(70);
> > >           heartbeat_data->phase++;
> > > +        if (heartbeat_data->invert)
> > > +            brightness = led_cdev->max_brightness;
> >
> > As you are at it, could you change, in a separate patch,
> > led_cdev->max_brightness to led_cdev->brightness? I think there is no
> > reason for which we shouldn't allow other values than max.
> I changed it but it doesn't blink at all.

Indeed, we can't rely on led_cdev->brightness as it is being zeroed
in the meantime.

We would have to use another variable for storing current brightness.
Probably we could use led_cdev->delayed_set_value, similarly as in
case of software blinking. It would require changes in
led_set_brightness(), so that it updated led_cdev->delayed_set_value
not only when timer trigger is enabled, but also in case other triggers
are.

I'll take care of that after recent patch set with LED core
improvements is applied.

> > >           break;
> > >       case 2:
> > >           delay = msecs_to_jiffies(70);
> > >           heartbeat_data->phase++;
> > > -        brightness = led_cdev->max_brightness;
> > > +        if (!heartbeat_data->invert)
> > > +            brightness = led_cdev->max_brightness;
> > >           break;
> > >       default:
> > >           delay = heartbeat_data->period - heartbeat_data->period / 4 -
> > >               msecs_to_jiffies(70);
> > >           heartbeat_data->phase = 0;
> > > +        if (heartbeat_data->invert)
> > > +            brightness = led_cdev->max_brightness;
> > >           break;
> > >       }
> > >
> > > @@ -78,15 +85,50 @@ static void led_heartbeat_function(unsigned long
> > > data)
> > >       mod_timer(&heartbeat_data->timer, jiffies + delay);
> > >   }
> > >
> > > +static ssize_t led_invert_show(struct device *dev,
> > > +        struct device_attribute *attr, char *buf)
> > > +{
> > > +    struct led_classdev *led_cdev = dev_get_drvdata(dev);
> > > +    struct heartbeat_trig_data *heartbeat_data =
> > > led_cdev->trigger_data;
> > > +
> > > +    return sprintf(buf, "%u\n", heartbeat_data->invert);
> > > +}
> > > +
> > > +static ssize_t led_invert_store(struct device *dev,
> > > +        struct device_attribute *attr, const char *buf, size_t size)
> > > +{
> > > +    struct led_classdev *led_cdev = dev_get_drvdata(dev);
> > > +    struct heartbeat_trig_data *heartbeat_data =
> > > led_cdev->trigger_data;
> > > +    unsigned long state;
> > > +    int ret;
> > > +
> > > +    ret = kstrtoul(buf, 0, &state);
> > > +    if (ret)
> > > +        return ret;
> > > +
> > > +    heartbeat_data->invert = !!state;
> > > +
> > > +    return size;
> > > +}
> > > +
> > > +static DEVICE_ATTR(invert, 0644, led_invert_show, led_invert_store);
> > > +
> > >   static void heartbeat_trig_activate(struct led_classdev *led_cdev)
> > >   {
> > >       struct heartbeat_trig_data *heartbeat_data;
> > > +    int rc;
> > >
> > >       heartbeat_data = kzalloc(sizeof(*heartbeat_data), GFP_KERNEL);
> > >       if (!heartbeat_data)
> > >           return;
> > >
> > >       led_cdev->trigger_data = heartbeat_data;
> > > +    rc = device_create_file(led_cdev->dev, &dev_attr_invert);
> > > +    if (rc) {
> > > +        kfree(led_cdev->trigger_data);
> > > +        return;
> > > +    }
> > > +
> > >       setup_timer(&heartbeat_data->timer,
> > >               led_heartbeat_function, (unsigned long) led_cdev);
> > >       heartbeat_data->phase = 0;
> > > @@ -100,9 +142,12 @@ static void heartbeat_trig_deactivate(struct
> > > led_classdev *led_cdev)
> > >
> > >       if (led_cdev->activated) {
> > >           del_timer_sync(&heartbeat_data->timer);
> > > +        device_remove_file(led_cdev->dev, &dev_attr_invert);
> > >           kfree(heartbeat_data);
> > >           led_cdev->activated = false;
> > >       }
> > > +
> > > +    led_set_brightness(led_cdev, LED_OFF);
> >
> > I believe this is a fix. Could you split it into a separate patch
> > and explain its merit?
> I'm not sure of necessity of it, I copied it from oneshot.
> Without it could leave LED in ON state. But never happens to me.
> Should I produce separate patch to discuss it in separate thread?

This is not necessary. led_trigger_remove() calls
let_trigger_set(led_cdev, NULL), which in turn calls
led_set_brightness(led_cdev, LED_OFF).

Please remove above line and adjust commit message format
so that checkpatch.pl doesn't complain.

> > >   }
> > >
> > >   static struct led_trigger heartbeat_led_trigger = {
> --
> To unsubscribe from this list: send the line "unsubscribe linux-leds" in
> the body of a message to majordomo@xxxxxxxxxxxxxxx
> More majordomo info at  http://vger.kernel.org/majordomo-info.html


--
Best Regards,
Jacek Anaszewski
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/