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

[Heinrich Schuchardt]

On 8/3/22 13:56, Quentin Schulz wrote:
> 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

The following boards need to be converted to use DM_GPIO or otherwise 
should be dropped:

Simon Guinot
* edminiv2_defconfig

Fabio Estevam
* mx28evk_nand_defconfig
* mx28evk_auart_console_defconfig
* mx28evk_spi_defconfig

Otavio Salvador
* warp_defconfig

Best regards

Heinrich