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

[Quentin Schulz]

Hi Heinrich,

On 8/3/22 13:21, Heinrich Schuchardt wrote:
> 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.
> 

Considering that _gpio_get_value does the inversion if the 
GPIOD_ACTIVE_LOW flag is set, the expected return value of 
dm_gpio_ops->get_value should be the raw value of the pin as returned by 
the SoC. Not documented, yes, but the meaning is clear to me.

> 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.
> 

It is clear for DM, not for non-DM.

> 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.
> 

Agreed.

We still ave the issue with values returned by non-DM drivers, but 
should we care? The drivers should be moved to DM_GPIO?

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

Indeed.

I thought DM-only GPIO subsystem was on the map, like all subsystems, 
but I cannot find it on 
https://u-boot.readthedocs.io/en/latest/develop/driver-model/migration.html. 
Otherwise, the move to DM-only for GPIO should also help us in having a 
properly defined expectation of what the gpio command should return, 
since the DM version of gpio commands is handling active state. I'd have 
been inclined to say we shouldn't care too much about non-DM GPIO 
drivers since they'll disappear some day, are deprecated (as per 
documentation in header file) and if they get updated they should follow 
the yet-to-be-written in-file documentation, but I'm not too sure since 
DM_GPIO is not on the map of driver model migration at the moment?

>>
>>> +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.
> 

Agreed, it was a suggestion on how to improve the gpio command output 
(in another patch obviously).

Though, this thread sadly highlighted that the gpio command returns 
something that cannot be trusted by the user without digging into the 
driver/datasheet (at least until we get DM-only GPIO subsystem) :/

BR,
Quentin