diff mbox series

[v2,6/7,RESEND] cmd: button: store button state in the 'button' env

Message ID 20201215165439.13165-1-m.szyprowski@samsung.com
State New
Headers show
Series None | expand

Commit Message

Marek Szyprowski Dec. 15, 2020, 4:54 p.m. UTC 
Save examined button state in 'button' environment variable to enable
checking button state in the scripts.

Signed-off-by: Marek Szyprowski <m.szyprowski@samsung.com>

---
Resend reason: get rid of the Change-Id tag, that was still in v2.
---
 cmd/button.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

-- 
2.17.1

Comments

Heinrich Schuchardt Dec. 15, 2020, 7:07 p.m. UTC | #1 
On 12/15/20 5:54 PM, Marek Szyprowski wrote:
> Save examined button state in 'button' environment variable to enable

> checking button state in the scripts.

>

> Signed-off-by: Marek Szyprowski <m.szyprowski@samsung.com>

> ---

> Resend reason: get rid of the Change-Id tag, that was still in v2.

> ---

>   cmd/button.c | 4 +++-

>   1 file changed, 3 insertions(+), 1 deletion(-)

>

> diff --git a/cmd/button.c b/cmd/button.c

> index 64c5a8fa04..8da911068a 100644

> --- a/cmd/button.c

> +++ b/cmd/button.c

> @@ -23,8 +23,10 @@ static int show_button_state(struct udevice *dev)

>   	ret = button_get_state(dev);

>   	if (ret >= BUTTON_COUNT)

>   		ret = -EINVAL;

> -	if (ret >= 0)

> +	if (ret >= 0) {

>   		printf("%s\n", state_label[ret]);

> +		env_set("button", state_label[ret]);


Using a hard coded environment variable does not make much sense to me.
The button command has a return value. So just use

button mybutton; setenv myvar $?

Best regards

Heinrich

> +	}

>

>   	return ret;

>   }

>
Marek Szyprowski Dec. 16, 2020, 7:08 a.m. UTC | #2 
On 15.12.2020 20:07, Heinrich Schuchardt wrote:
> On 12/15/20 5:54 PM, Marek Szyprowski wrote:

>> Save examined button state in 'button' environment variable to enable

>> checking button state in the scripts.

>>

>> Signed-off-by: Marek Szyprowski <m.szyprowski@samsung.com>

>> ---

>> Resend reason: get rid of the Change-Id tag, that was still in v2.

>> ---

>>   cmd/button.c | 4 +++-

>>   1 file changed, 3 insertions(+), 1 deletion(-)

>>

>> diff --git a/cmd/button.c b/cmd/button.c

>> index 64c5a8fa04..8da911068a 100644

>> --- a/cmd/button.c

>> +++ b/cmd/button.c

>> @@ -23,8 +23,10 @@ static int show_button_state(struct udevice *dev)

>>       ret = button_get_state(dev);

>>       if (ret >= BUTTON_COUNT)

>>           ret = -EINVAL;

>> -    if (ret >= 0)

>> +    if (ret >= 0) {

>>           printf("%s\n", state_label[ret]);

>> +        env_set("button", state_label[ret]);

>

> Using a hard coded environment variable does not make much sense to me.

> The button command has a return value. So just use

>

> button mybutton; setenv myvar $?

>

Thanks for the hint, I wasn't aware that uboot supports '$?'. By setting 
the 'button' env variable I've tried to mimic the behavior of the 
various network and file related commands, which sets 'filesize' env 
variable.

I will need to add the return value propagation to the button command 
anyway to make it usable from the scripts.

Best regards
-- 
Marek Szyprowski, PhD
Samsung R&D Institute Poland
Heinrich Schuchardt Dec. 16, 2020, 7:29 a.m. UTC | #3 
On 12/16/20 8:08 AM, Marek Szyprowski wrote:
> On 15.12.2020 20:07, Heinrich Schuchardt wrote:

>> On 12/15/20 5:54 PM, Marek Szyprowski wrote:

>>> Save examined button state in 'button' environment variable to enable

>>> checking button state in the scripts.

>>>

>>> Signed-off-by: Marek Szyprowski <m.szyprowski@samsung.com>

>>> ---

>>> Resend reason: get rid of the Change-Id tag, that was still in v2.

>>> ---

>>>    cmd/button.c | 4 +++-

>>>    1 file changed, 3 insertions(+), 1 deletion(-)

>>>

>>> diff --git a/cmd/button.c b/cmd/button.c

>>> index 64c5a8fa04..8da911068a 100644

>>> --- a/cmd/button.c

>>> +++ b/cmd/button.c

>>> @@ -23,8 +23,10 @@ static int show_button_state(struct udevice *dev)

>>>        ret = button_get_state(dev);

>>>        if (ret >= BUTTON_COUNT)

>>>            ret = -EINVAL;

>>> -    if (ret >= 0)

>>> +    if (ret >= 0) {

>>>            printf("%s\n", state_label[ret]);

>>> +        env_set("button", state_label[ret]);

>>

>> Using a hard coded environment variable does not make much sense to me.

>> The button command has a return value. So just use

>>

>> button mybutton; setenv myvar $?

>>

> Thanks for the hint, I wasn't aware that uboot supports '$?'. By setting

> the 'button' env variable I've tried to mimic the behavior of the

> various network and file related commands, which sets 'filesize' env

> variable.

>

> I will need to add the return value propagation to the button command

> anyway to make it usable from the scripts.


Nothing to be done for the button command. Since

     a6bfd71a96201127836d59736abcb54dc2d5e1a5
     cmd/button: return button status

the button command returns

     0 (true) - pressed, on
     1 (false) - not pressed, off

Best regards

Heinrich
Marek Szyprowski Dec. 16, 2020, 7:42 a.m. UTC | #4 
On 16.12.2020 08:29, Heinrich Schuchardt wrote:
> On 12/16/20 8:08 AM, Marek Szyprowski wrote:

>> On 15.12.2020 20:07, Heinrich Schuchardt wrote:

>>> On 12/15/20 5:54 PM, Marek Szyprowski wrote:

>>>> Save examined button state in 'button' environment variable to enable

>>>> checking button state in the scripts.

>>>>

>>>> Signed-off-by: Marek Szyprowski <m.szyprowski@samsung.com>

>>>> ---

>>>> Resend reason: get rid of the Change-Id tag, that was still in v2.

>>>> ---

>>>>    cmd/button.c | 4 +++-

>>>>    1 file changed, 3 insertions(+), 1 deletion(-)

>>>>

>>>> diff --git a/cmd/button.c b/cmd/button.c

>>>> index 64c5a8fa04..8da911068a 100644

>>>> --- a/cmd/button.c

>>>> +++ b/cmd/button.c

>>>> @@ -23,8 +23,10 @@ static int show_button_state(struct udevice *dev)

>>>>        ret = button_get_state(dev);

>>>>        if (ret >= BUTTON_COUNT)

>>>>            ret = -EINVAL;

>>>> -    if (ret >= 0)

>>>> +    if (ret >= 0) {

>>>>            printf("%s\n", state_label[ret]);

>>>> +        env_set("button", state_label[ret]);

>>>

>>> Using a hard coded environment variable does not make much sense to me.

>>> The button command has a return value. So just use

>>>

>>> button mybutton; setenv myvar $?

>>>

>> Thanks for the hint, I wasn't aware that uboot supports '$?'. By setting

>> the 'button' env variable I've tried to mimic the behavior of the

>> various network and file related commands, which sets 'filesize' env

>> variable.

>>

>> I will need to add the return value propagation to the button command

>> anyway to make it usable from the scripts.

>

> Nothing to be done for the button command. Since

>

>     a6bfd71a96201127836d59736abcb54dc2d5e1a5

>     cmd/button: return button status

>

> the button command returns

>

>     0 (true) - pressed, on

>     1 (false) - not pressed, off

>

Right, I've missed that commit. I've developed my code on top of 
v2020.10 release.

Best regards
-- 
Marek Szyprowski, PhD
Samsung R&D Institute Poland
Simon Glass Dec. 19, 2020, 2:29 a.m. UTC | #5 
Hi Marek,

On Tue, 15 Dec 2020 at 09:55, Marek Szyprowski <m.szyprowski@samsung.com> wrote:
>

> Save examined button state in 'button' environment variable to enable

> checking button state in the scripts.

>

> Signed-off-by: Marek Szyprowski <m.szyprowski@samsung.com>

> ---

> Resend reason: get rid of the Change-Id tag, that was still in v2.


If you use patman it does that for you.

Regards,
Simon
diff mbox series

Patch

diff --git a/cmd/button.c b/cmd/button.c
index 64c5a8fa04..8da911068a 100644
--- a/cmd/button.c
+++ b/cmd/button.c
@@ -23,8 +23,10 @@  static int show_button_state(struct udevice *dev)
 	ret = button_get_state(dev);
 	if (ret >= BUTTON_COUNT)
 		ret = -EINVAL;
-	if (ret >= 0)
+	if (ret >= 0) {
 		printf("%s\n", state_label[ret]);
+		env_set("button", state_label[ret]);
+	}
 
 	return ret;
 }