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

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

[Amitesh Singh]

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 | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/drivers/leds/trigger/ledtrig-oneshot.c b/drivers/leds/trigger/ledtrig-oneshot.c
index b8ea9f0..accb241 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;
 };
-- 
2.7.4

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

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

[Amitesh Singh]

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;
 
-- 
2.7.4

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

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

[Jacek Anaszewski]

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.

-- 
Best regards,
Jacek Anaszewski

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

[Amitesh Singh]

Hello Jacek,

On Sat, Sep 3, 2016 at 7:14 PM, Jacek Anaszewski
<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.

> --
> Best regards,
> Jacek Anaszewski

Regards
Amitesh Singh

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

[Jacek Anaszewski]

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

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

[Amitesh Singh]

On Sat, Sep 3, 2016 at 11:42 PM, Jacek Anaszewski
<jacek.anaszewski@gmail.com> wrote:
> 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).
>
>
This is for configuring the led trigger delay on/off parameter when I
instantiate the driver from kernel space.
I am instantiating oneshot trigger at the time of boot up.

> --
> Best regards,
> Jacek Anaszewski

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

[Jacek Anaszewski]

On 09/06/2016 05:05 PM, Amitesh Singh wrote:
> On Sat, Sep 3, 2016 at 11:42 PM, Jacek Anaszewski
> <jacek.anaszewski@gmail.com> wrote:
>> 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).
>>
>>
> This is for configuring the led trigger delay on/off parameter when I
> instantiate the driver from kernel space.
> I am instantiating oneshot trigger at the time of boot up.

In other use cases, when the delay intervals need to be altered
later, it doesn't introduce any gain. Even in your use case
the gain is almost unnoticeable.

-- 
Best regards,
Jacek Anaszewski