[PATCH 1/1] doc: man-page for gpio command

[Heinrich Schuchardt]

On 8/3/22 11:59, Quentin Schulz wrote:
> Hi Heinrich,
> 
> On 8/2/22 14:28, Heinrich Schuchardt wrote:
>> Provide a man-page for the gpio command.
>>
>> Signed-off-by: Heinrich Schuchardt <heinrich.schuchardt at canonical.com>
>> ---
>>   doc/usage/cmd/gpio.rst | 90 ++++++++++++++++++++++++++++++++++++++++++
>>   doc/usage/index.rst    |  1 +
>>   2 files changed, 91 insertions(+)
>>   create mode 100644 doc/usage/cmd/gpio.rst
>>
>> diff --git a/doc/usage/cmd/gpio.rst b/doc/usage/cmd/gpio.rst
>> new file mode 100644
>> index 0000000000..f6a5668388
>> --- /dev/null
>> +++ b/doc/usage/cmd/gpio.rst
>> @@ -0,0 +1,90 @@
>> +.. SPDX-License-Identifier: GPL-2.0+:
>> +
>> +gpio command
>> +============
>> +
>> +Synopsis
>> +--------
>> +
>> +::
>> +
>> +    gpio <input|set|clear|toggle> <pin>
>> +    gpio read <name> <pin>
>> +    gpio status [-a] [<bank>|<pin>]
>> +
>> +The gpio command is used to access General Purpose Inputs/Outputs.
>> +
>> +gpio input
>> +----------
>> +
>> +Switch the GPIO *pin* to input mode.
>> +
>> +gpio set
>> +--------
>> +
>> +Switch the GPIO *pin* to output mode and set the signal to 1.
>> +
> 
> I think this is supposed to follow the 
> GPIOD_ACTIVE_LOW/GPIOD_ACTIVE_HIGH flag. So I think it's better to say 
> "set the state to active"? Maybe add a few words on this active high/low 
> thing to explicit that using gpio set does not necessarily mean that the 
> GPIO output will have some voltage.

There is no string GPIOD_ACTIVE_HIGH in U-Boot.

include/asm-generic/gpio.h:124:
#define GPIOD_ACTIVE_LOW BIT(3)
/* GPIO is active when value is low */

The gpio command lacks the capability to set active high or active low. 
It is not even displayed.

The function description of dm_gpio_get_value() documents the return 
value as 0 for inactive, 1 for active.

The function description of gpio_get_value() documents its return value 
as 0 for low or 1 for high.

The uclass implements gpio_get_value() by calling dm_gpio_get_value() 
without inverting the value.

One of both functions is incorrectly documented!

dm_gpio_get_value() calls _gpio_get_value() which calls the driver's 
get_value() function and inverts the value if GPIOD_ACTIVE_LOW is set.

The structure struct dm_gpio_ops lacks any documentation of the meaning 
of the return value.

To sum it up:

The function documentation is a mess and leaves it unclear what a value
of 0 or 1 in the gpio command means.

The first thing to do is to get the documentation of struct dm_gpio_ops 
updated. Next ensure that all drivers implement it this way.

Once the yet to find maintainer of GPIO has done this we can revisit the 
documentation.

> 
>> +gpio clear
>> +----------
>> +
>> +Switch the GPIO *pin* to output mode and set the signal to 0.
>> +
> 
> Ditto.
> 
>> +gpio toggle
>> +-----------
>> +
>> +Switch the GPIO *pin* to output mode and reverse the signal state.
>> +
>> +gpio read
>> +---------
>> +
>> +Read the signal state of the GPIO *pin* and save it in environment 
>> variable
>> +*name*.
>> +
>> +gpio status
>> +-----------
>> +
>> +Display the status of one or multiple GPIOs. By default only claimed 
>> GPIOs
>> +are displayed.
>> +
>> +-a
>> +    Display GPIOs irrespective of being claimed.
>> +
>> +bank
>> +    Name of a bank of GPIOs to be displayed.
>> +
>> +pin
>> +    Name of a single GPIO to be displayed or manipulated.
>> +
>> +Examples
>> +--------
>> +
>> +Switch the status of a GPIO::
>> +
>> +    => gpio set a5
>> +    gpio: pin a5 (gpio 133) value is 1
> 
> and I guess we should maybe patch the gpio cmd to say here "high" 
> instead of "1"?

For an active low GPIO value 1 might mean low.

It would be helpful to provide a way to display if a GPIO is active high 
or active low. But this patch is about documenting the existing 
functionality.

Best regards

Heinrich

> 
>> +    => gpio clear a5
>> +    gpio: pin a5 (gpio 133) value is 0
> 
> ditto
> 
>> +    => gpio toggle a5
>> +    gpio: pin a5 (gpio 133) value is 1
> 
> ditto
> 
>> +    => gpio read myvar a5
>> +    gpio: pin a5 (gpio 133) value is 1
> 
> ditto
> 
>> +    => echo $myvar
>> +    1
>> +    => gpio toggle a5
>> +    gpio: pin a5 (gpio 133) value is 0
> 
> ditto
> 
>> +    => gpio read myvar a5
>> +    gpio: pin a5 (gpio 133) value is 0
> 
> ditto
> 
> Cheers,
> Quentin