Re: [PATCH] leds: oneshot - Allow default delay to be passed as an argument

[Jacek Anaszewski <jacek.anaszewski@gmail.com>]

On 09/03/2016 05:35 PM, Amitesh Singh wrote:
> Hello Jacek,
>
>
> On Sat, Sep 3, 2016 at 7:14 PM, Jacek Anaszewski
> <jacek.anaszewski@gmail.com <mailto:jacek.anaszewski@gmail.com>> wrote:
>
>     Hi Amitesh,
>
>
>     On 09/03/2016 11:03 AM, Amitesh Singh wrote:
>
>         This patch facilates the blink delay to be passed as
>         an argument at the time of module loading.
>         e.g.
>         insmod ledtrigg-oneshot.ko default_delay=100
>         ---
>          drivers/leds/trigger/ledtrig-oneshot.c | 7 +++++--
>          1 file changed, 5 insertions(+), 2 deletions(-)
>
>         diff --git a/drivers/leds/trigger/ledtrig-oneshot.c
>         b/drivers/leds/trigger/ledtrig-oneshot.c
>         index b8ea9f0..95933a1 100644
>         --- a/drivers/leds/trigger/ledtrig-oneshot.c
>         +++ b/drivers/leds/trigger/ledtrig-oneshot.c
>         @@ -22,6 +22,9 @@
>
>          #define DEFAULT_DELAY 100
>
>         +static unsigned long default_delay = DEFAULT_DELAY;
>         +module_param(default_delay, ulong, S_IRUGO|S_IWUSR);
>         +
>          struct oneshot_trig_data {
>                 unsigned int invert;
>          };
>         @@ -146,8 +149,8 @@ static void oneshot_trig_activate(struct
>         led_classdev *led_cdev)
>                 if (rc)
>                         goto err_out_invert;
>
>         -       led_cdev->blink_delay_on = DEFAULT_DELAY;
>         -       led_cdev->blink_delay_off = DEFAULT_DELAY;
>         +       led_cdev->blink_delay_on = default_delay;
>         +       led_cdev->blink_delay_off = default_delay;
>
>                 led_cdev->activated = true;
>
>
>
>
>     Why do you need this module parameter? You can change
>     delay_on and delay_off values from sysfs.
>
> Yes, indeed. If someone wants to change blink_delay, he has to change
> both delay_on and delay_off in case you want to blink with constant time
> ON and OFF (1 / (T + T) = 1/2T)
> This patch facilitates you to modify delay in one step in case of delay
> on and off are same which I think, is most used case.

I don't think this is real improvement. We spare two sysfs file
write operations only in case the default_delay param value passed
on module loading won't need to be altered later on.

Moreover, three sysfs file writes needed for configuring oneshot
trigger (e.g. echo "oneshot" > trigger; echo 100 > delay_on;
echo 50 > delay_off) are not too expensive taking into account
usual frequency of setting LED trigger (surely less often than
once per second).

-- 
Best regards,
Jacek Anaszewski