[U-Boot] [PATCH v1 2/5] cmd: pinmux: Add pinmux command

[Patrice CHOTARD]

Hi Simon

On 09/26/2018 07:41 AM, Simon Glass wrote:
> Hi Patrice,
> 
> On 20 September 2018 at 07:37, Patrice Chotard <patrice.chotard at st.com> wrote:
>> pinmux command allows to :
>>  - list all pin-controllers available on platforms
>>  - select a pin-controller
>>  - display the muxing of all pins of the current pin-controller
>>    or all pin-controllers depending of given options
>>
>> Signed-off-by: Patrice Chotard <patrice.chotard at st.com>
>> ---
>>
>>  cmd/Kconfig  |   8 +++++
>>  cmd/Makefile |   3 ++
>>  cmd/pinmux.c | 115 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
>>  3 files changed, 126 insertions(+)
>>  create mode 100644 cmd/pinmux.c
>>
>> diff --git a/cmd/Kconfig b/cmd/Kconfig
>> index 13d4c991bf8b..2c687ceecf49 100644
>> --- a/cmd/Kconfig
>> +++ b/cmd/Kconfig
>> @@ -937,6 +937,14 @@ config CMD_PCMCIA
>>           about 1990. These devices are typically removable memory or network
>>           cards using a standard 68-pin connector.
>>
>> +config CMD_PINMUX
>> +       bool "pinmux - show pins muxing"
>> +       depends on PINCTRL
> 
> Should this be default y? It seems that we should normally enable
> commands like this which provide useful info.

Yes, i will make it default y if PINCTRL is set

> 
> [..]
> 
>> diff --git a/cmd/pinmux.c b/cmd/pinmux.c
>> new file mode 100644
>> index 000000000000..1442d6ef63d2
>> --- /dev/null
>> +++ b/cmd/pinmux.c
>> @@ -0,0 +1,115 @@
>> +// SPDX-License-Identifier: GPL-2.0+
>> +/*
>> + * Copyright (C) 2018, STMicroelectronics - All Rights Reserved
>> + */
>> +
>> +#include <command.h>
>> +#include <dm.h>
>> +#include <errno.h>
>> +#include <dm/pinctrl.h>
>> +#include <dm/uclass-internal.h>
>> +
>> +#ifdef CONFIG_PINCTRL
>> +
>> +#define LIMIT_DEVNAME  30
>> +#define LIMIT_OFNAME   32
>> +
>> +static struct udevice *currdev;
>> +
>> +static int do_dev(cmd_tbl_t *cmdtp, int flag, int argc, char * const argv[])
>> +{
>> +       const char *name;
>> +       int ret;
>> +
>> +       switch (argc) {
>> +       case 2:
>> +               name = argv[1];
>> +               ret = uclass_get_device_by_name(UCLASS_PINCTRL, name, &currdev);
>> +               if (ret) {
>> +                       printf("Can't get the pin-controller: %s!\n", name);
>> +                       return CMD_RET_FAILURE;
>> +               }
>> +       case 1:
>> +               if (!currdev) {
>> +                       printf("Pin-controller device is not set!\n\n");
> 
> Did you intend to have two \n ?

No, it's a typo issue

> 
> [..]
> 
>> +
>> +       for (uclass_first_device(UCLASS_PINCTRL, &dev); dev;
>> +            uclass_next_device(&dev)) {
> 
> Can you please add a macro a bit like uclass_foreach_dev() for this
> pattern? It seems to come up a lot.
> 
> I'm not sure what to call it. Perhaps uclass_foreach_dev_probe()?

Yes, no problem, i will add it

> 
>> +               /* insert a separator between each pin-controller display */
>> +               printf("--------------------------\n");
>> +               printf("%s:\n", dev->name);
>> +               ret = pinctrl_pinmux_show(dev);
>> +               if (ret)
>> +                       return ret;
>> +       }
>> +       return CMD_RET_SUCCESS;
> 
> Or just 0. It will always be 0.

CMD_RET_SUCCESS value is already 0

Thanks

Patrice

> 
> 
> Regards,
> Simon
>