[U-Boot] [PATCH v2 05/12] dm: pmic: new commands: pmic and regulator

[Przemyslaw Marczak <p.marczak@samsung.com>]

Hello Simon,

On 03/06/2015 03:13 PM, Simon Glass wrote:
> Hi Przemyslaw,
>
> On 3 March 2015 at 09:24, Przemyslaw Marczak <p.marczak@samsung.com> wrote:
>> This introduces new commands:
>> - pmic (new) - CONFIG_DM_PMIC_CMD
>> - regulator - CONFIG_DM_REGULATOR_CMD
>> Both uses a common code and dm pmic api.
>>
>> To avoid code mess the old pmic command is kept without changes.
>>
>> Command pmic
>> The new pmic command uses driver model pmic api. The previous pmic
>> I/O functionality is keept. And now read/write is the main pmic command
>> feature. This command can be used only for UCLASS_PMIC devices,
>> since this uclass is designed for pmic I/O operations only and provides
>> pmic platform data.
>>
>> Command options (pmic [option]):
>> - list                     - list available PMICs
>> - dev <id>                 - set id to current pmic device
>> - pmic dump                - dump registers
>> - pmic read <reg>          - read register
>> - pmic write <reg> <value> - write register
>>
>> The user interface is changed. Before any operation, first the device
>> should be chosen.
>>
>> Command regulator
>> It uses the same code, but provides user interface for regulator devices.
>>
>> This was designed to access the regulator device without it's documentation.
>> It is possible, if driver implements uclass features, e.g. output descriptors.
>>
>> Available commands:
>> - list     - list UCLASS regulator devices
>> - dev [id] - show or set current regulator device
>> - dump     - dump registers of current regulator device
>> - [ldo/buck/dvs][N] [name/state/desc]- print regulator(s) info
>> - [ldoN/buckN/dvsN] [setval/setmode] [mV/modeN] [-f] - set val (mV)
>>     or mode - only if descriptor exists
>>
>> The regulator descriptor 'min' and 'max' limits prevents setting
>> unsafe value. But sometimes it is useful to change the regulator
>> value for some test - so the force option (-f) is available.
>> This option is not available for change the mode, since this depends
>> on pmic device design.
>
> This help could go in Kconfig.
>
>>
>> Signed-off-by: Przemyslaw Marczak <p.marczak@samsung.com>
>> ---
>> Changes v2:
>> - remove errno_str() call
>> - pmic command: move some code to pmic uclass
>> - pmic command: code cleanup
>> - fix data types
>> - add command line, regulator on/off setting feature
>> - adjust to new pmic api
>> - cleanup
>> ---
>>   drivers/power/Makefile   |   2 +
>>   drivers/power/cmd_pmic.c | 820 +++++++++++++++++++++++++++++++++++++++++++++++
>>   2 files changed, 822 insertions(+)
>>   create mode 100644 drivers/power/cmd_pmic.c
>>
>> diff --git a/drivers/power/Makefile b/drivers/power/Makefile
>> index a6b7012..943b38f 100644
>> --- a/drivers/power/Makefile
>> +++ b/drivers/power/Makefile
>> @@ -22,4 +22,6 @@ obj-$(CONFIG_POWER_FSL) += power_fsl.o
>>   obj-$(CONFIG_POWER_I2C) += power_i2c.o
>>   obj-$(CONFIG_POWER_SPI) += power_spi.o
>>   obj-$(CONFIG_DM_PMIC) += pmic-uclass.o
>> +obj-$(CONFIG_DM_PMIC_CMD) += cmd_pmic.o
>>   obj-$(CONFIG_DM_REGULATOR) += regulator-uclass.o
>> +obj-$(CONFIG_DM_REGULATOR_CMD) += cmd_pmic.o
>
> Can we put this in its own file? Also perhaps it should go in common?
> Not sure about that though.
>
>> diff --git a/drivers/power/cmd_pmic.c b/drivers/power/cmd_pmic.c
>> new file mode 100644
>> index 0000000..996bfe7
>> --- /dev/null
>> +++ b/drivers/power/cmd_pmic.c
>> @@ -0,0 +1,820 @@
>> +/*
>> + * Copyright (C) 2014-2015 Samsung Electronics
>> + * Przemyslaw Marczak <p.marczak@samsung.com>
>> + *
>> + * SPDX-License-Identifier:    GPL-2.0+
>> + */
>> +#include <common.h>
>> +#include <linux/types.h>
>> +#include <linux/ctype.h>
>> +#include <fdtdec.h>
>> +#include <dm.h>
>> +#include <power/pmic.h>
>> +#include <power/regulator.h>
>> +#include <dm/device-internal.h>
>> +#include <dm/uclass-internal.h>
>> +#include <dm/root.h>
>> +#include <dm/lists.h>
>
> Please sort includes:
>
> common.h
> others in order
> <asm/...>
> <dm/...>
> <linux/...>
> "local includes"
>
>> +#include <i2c.h>
>> +#include <compiler.h>
>> +#include <errno.h>
>> +
>> +#define LINE_BUF_LIMIT 80
>> +#define STR_BUF_LEN    26
>> +#define ID_STR_LIMIT   4
>> +#define UC_STR_LIMIT   16
>> +#define DRV_STR_LIMIT  26
>> +#define IF_STR_LIMIT   12
>> +
>> +static struct udevice *pmic_curr;
>> +static struct udevice *reg_curr;
>> +
>> +#ifdef CONFIG_DM_REGULATOR_CMD
>> +#define TYPE_INFO(_id, _name) { \
>> +       .id = _id, \
>> +       .len = ARRAY_SIZE(_name) - 1, \
>> +       .name = _name, \
>> +}
>> +
>> +enum display_info {
>> +       INFO_NAME,
>> +       INFO_STATE,
>> +       INFO_DESC,
>> +       INFO_DESC_MODE,
>> +       INFO_DESC_VAL,
>> +};
>> +
>> +struct regulator_type_info {
>> +       int id;
>> +       int len;
>> +       char *name;
>> +};
>> +
>> +static struct regulator_type_info type_info[] = {
>> +       TYPE_INFO(REGULATOR_TYPE_LDO, "ldo"),
>> +       TYPE_INFO(REGULATOR_TYPE_BUCK, "buck"),
>> +       TYPE_INFO(REGULATOR_TYPE_DVS, "dvs"),
>> +};
>> +
>> +char *regulator_type_str(int regulator_type)
>> +{
>> +       switch (regulator_type) {
>> +       case REGULATOR_TYPE_LDO:
>> +       case REGULATOR_TYPE_BUCK:
>> +       case REGULATOR_TYPE_DVS:
>> +               return type_info[regulator_type].name;
>> +       default:
>> +               return NULL;
>> +       }
>> +}
>> +#endif /* CONFIG_DM_REGULATOR_CMD */
>> +
>> +static int set_curr_dev(struct udevice *dev)
>> +{
>> +       if (!dev)
>> +               return -EINVAL;
>> +
>> +       if (!dev->driver)
>> +               return -EINVAL;
>> +
>> +       switch (dev->driver->id) {
>> +       case UCLASS_PMIC:
>> +               pmic_curr = dev;
>> +               break;
>> +       case UCLASS_PMIC_REGULATOR:
>> +               reg_curr = dev;
>> +               break;
>> +       default:
>> +               error("Bad driver uclass!\n");
>> +               return -EINVAL;
>> +       }
>> +
>> +       return 0;
>> +}
>> +
>> +static struct udevice *get_curr_dev(int uclass_id)
>> +{
>> +       switch (uclass_id) {
>> +       case UCLASS_PMIC:
>> +               return pmic_curr;
>> +       case UCLASS_PMIC_REGULATOR:
>> +               return reg_curr;
>> +       default:
>> +               error("Bad uclass ID!\n");
>> +               return NULL;
>> +       }
>> +}
>> +
>> +static int curr_dev_name(int uclass_id)
>> +{
>> +       struct udevice *dev = get_curr_dev(uclass_id);
>> +
>> +       if (dev) {
>> +               printf("PMIC: %s\n", dev->name);
>> +               return 0;
>> +       } else {
>> +               puts("PMIC dev is not set!\n");
>> +               return -ENODEV;
>> +       }
>> +}
>> +
>> +static int pmic_dump(int uclass_id)
>> +{
>> +       struct udevice *dev;
>> +       struct udevice *io_dev;
>> +       int i, ret, max_offset;
>> +       unsigned char val;
>> +
>> +       dev = get_curr_dev(uclass_id);
>> +       if (!dev)
>> +               return -ENODEV;
>> +
>> +       if (pmic_io_dev(dev, &io_dev)) {
>> +               printf("PMIC I/O dev not found\n");
>> +               return -ENODEV;
>> +       }
>> +
>> +       ret = curr_dev_name(uclass_id);
>> +       if (ret)
>> +               return CMD_RET_USAGE;
>> +
>> +       max_offset = pmic_if_max_offset(io_dev);
>> +       printf("Register count: %u\n", max_offset);
>> +
>> +       for (i = 0; i < max_offset; i++) {
>> +               ret = pmic_read(io_dev, i, &val);
>> +               if (ret) {
>> +                       printf("PMIC: Registers dump failed: %d\n", ret);
>> +                       return -EIO;
>> +               }
>> +
>> +               if (!(i % 16))
>> +                       printf("\n0x%02x: ", i);
>> +
>> +               printf("%2.2x ", val);
>> +       }
>> +       puts("\n");
>> +       return 0;
>> +}
>> +
>> +/**
>> + * display_column - display a string from buffer limited by the column len.
>> + *                  Allows display well aligned string columns.
>> + *
>> + * @buf: a pointer to the buffer with the string to display as a column
>> + * @str_len: len of the string
>> + * @column_len: a number of characters to be printed, but it is actually
>> + *              decremented by 1 for the ending character: '|'
>> +*/
>> +static int display_column(char *buf, unsigned int str_len,
>> +                         unsigned int column_len)
>
> What about using printf("%.*s", str_len, buf) instead of this function?
>
>> +{
>> +       int i = column_len;
>> +
>> +       if (str_len < column_len - 1) {
>> +               for (i = str_len; i < column_len; i++)
>> +                       buf[i] = ' ';
>> +       }
>> +
>> +       buf[column_len - 1] = '|';
>> +       buf[column_len] = '\0';
>> +
>> +       puts(buf);
>> +
>> +       return i;
>> +}
>> +
>> +static int pmic_list_uclass_devices(int uclass_id)
>> +{
>> +       ALLOC_CACHE_ALIGN_BUFFER(char, buf, LINE_BUF_LIMIT);
>> +       struct uclass_driver *uc_drv = NULL;
>> +       struct udevice *dev = NULL;
>> +       struct driver *drv = NULL;
>> +       struct uclass *uc = NULL;
>> +       int len;
>> +       int ret;
>> +
>> +       puts("| Id | Uclass        | Driver                  | Interface |\n");
>> +
>> +       ret = uclass_get(uclass_id, &uc);
>> +       if (ret) {
>> +               error("PMIC uclass: %d not found!\n", uclass_id);
>> +               return -EINVAL;
>> +       }
>> +
>> +       uc_drv = uc->uc_drv;
>> +       uclass_foreach_dev(dev, uc) {
>> +               if (dev)
>> +                       device_probe(dev);
>> +               else
>> +                       continue;
>> +
>> +               printf("| %2.d |", dev->seq);
>
> Why do you need the '.'?
>
>> +
>> +               len = snprintf(buf, STR_BUF_LEN, " %2.d@%.10s",
>> +                               uc_drv->id,  uc_drv->name);
>> +
>> +               display_column(buf, len, UC_STR_LIMIT);
>> +
>> +               drv = dev->driver;
>> +               if (drv && drv->name)
>> +                       len = snprintf(buf, STR_BUF_LEN, " %.26s", drv->name);
>> +               else
>> +                       len = snprintf(buf, STR_BUF_LEN, " - ");
>> +
>> +               display_column(buf, len, DRV_STR_LIMIT);
>> +
>> +               len = snprintf(buf, STR_BUF_LEN, " %s%d at 0x%x",
>> +                              pmic_if_str(dev),
>> +                              pmic_if_bus_num(dev),
>> +                              pmic_if_addr_cs(dev));
>> +
>> +               display_column(buf, len, IF_STR_LIMIT);
>
> Then I hope you might get all this into one printf() and drop buf?
>
>> +
>> +               puts("\n");
>> +       }
>> +
>> +       return 0;
>> +}
>> +
>> +static int pmic_cmd(char *const argv[], int argc, int uclass_id)
>> +{
>> +       const char *cmd = argv[0];
>> +       struct udevice *dev = NULL;
>> +       int seq;
>> +
>> +       if (strcmp(cmd, "list") == 0)
>> +               return pmic_list_uclass_devices(uclass_id);
>> +
>> +       if (strcmp(cmd, "dev") == 0) {
>> +               switch (argc) {
>> +               case 2:
>> +                       seq = simple_strtoul(argv[1], NULL, 0);
>> +                       uclass_get_device_by_seq(uclass_id, seq, &dev);
>> +                       if (dev)
>> +                               set_curr_dev(dev);
>> +                       else
>> +                               printf("Device: %d not found\n", seq);
>> +               case 1:
>> +                       return curr_dev_name(uclass_id);
>> +               }
>> +       }
>> +
>> +       if (strcmp(cmd, "dump") == 0)
>> +               return pmic_dump(uclass_id);
>
> Can we use a sub-command table here, like i2c?
>
>> +
>> +       if (!get_curr_dev(uclass_id))
>> +               return -ENODEV;
>> +
>> +       return 1;
>> +}
>> +
>> +#ifdef CONFIG_DM_REGULATOR_CMD
>> +static char *get_reg_mode_name(int reg, int mode,
>> +                              struct regulator_mode_desc *mode_desc,
>> +                              int desc_cnt)
>> +{
>> +       int i;
>> +       char *mode_name = NULL;
>> +
>> +       if (!mode_desc)
>> +               return NULL;
>> +
>> +       for (i = 0; i < desc_cnt; i++) {
>> +               if (mode_desc[i].mode == mode)
>> +                       mode_name = mode_desc[i].name;
>> +       }
>> +
>> +       return mode_name;
>> +}
>> +
>> +static int set_reg_state(int type, int reg_num, int on_off)
>> +{
>> +       struct udevice *dev;
>> +
>> +       dev = get_curr_dev(UCLASS_PMIC_REGULATOR);
>> +       if (!dev)
>> +               return -ENODEV;
>> +
>> +       return regulator_set_state(dev,type, reg_num, on_off);
>> +}
>> +
>> +/**
>> + * display_reg_info: list regualtor(s) info
>> + * @start_reg; @end_reg - first and last regulator number to list
>> + * if start_reg == end_reg - then displays only one regulator info
>> + *
>> + * @display_info options:
>> + *
>> + * =?INFO_NAME:
>> + * regN: name
>> + *
>> + * =?INFO_STATE:
>> + * regN: name
>> + *  - Vout: val mV mode: N (name)
>> + *
>> + * =?INFO_DESC
>> + * regN: name
>> + *  - Vout: val mV
>> + *  - Vmin: val mV
>> + *  - Vmax: val mV
>> + *  - mode: 1 (name1) [active]
>> + *  - mode: 2 (name2) [active]
>> + *  - mode: N (nameN) [active]
>> + *
>> + * =?INFO_DESC_VAL
>> + * regN:
>> + *  - Vout: val mV
>> + *  - Vmin: val mV
>> + *  - Vmax: val mV
>> + *
>> + * =?INFO_DESC_VAL
>> + * regN:
>> + *  - mode: 1 (name1) [active]
>> + *  - mode: 2 (name2) [active]
>> + *  - mode: N (nameN) [active]
>> + */
>> +static int display_reg_info(int type, int start_reg,
>> +                           int end_reg, int display_info)
>> +{
>> +       struct udevice *dev;
>> +       struct regulator_value_desc *desc;
>> +       struct regulator_mode_desc *mode_desc;
>> +       char *mode_name;
>> +       int mode_cnt;
>> +       int mode;
>> +       int ret;
>> +       int state;
>> +       int reg_val, i, j;
>> +       int div = 1000;
>
> This should be a #define or const int.
>
>> +
>> +       if (start_reg > end_reg)
>> +               return -EINVAL;
>> +
>> +       dev = get_curr_dev(UCLASS_PMIC_REGULATOR);
>> +       if (!dev)
>> +               return -ENODEV;
>> +
>> +       switch (display_info) {
>> +       case INFO_STATE:
>> +               puts(" RegN:  val mV  state  mode");
>
> I think we are supposed to use printf() now instead of puts(), unless
> in SPL for code size reasons.
>
>> +               break;
>> +       case INFO_DESC:
>> +               puts(" RegN:  @descriptors data");
>> +               break;
>> +       case INFO_DESC_VAL:
>> +       case INFO_DESC_MODE:
>> +               break;
>> +       case INFO_NAME:
>> +               puts(" RegN: name");
>> +               break;
>> +       default:
>> +               return -EINVAL;
>> +       }
>> +
>> +       for (i = start_reg; i <= end_reg; i++) {
>> +               ret = regulator_get_mode(dev, type, i, &mode);
>> +               ret |= regulator_get_value(dev, type, i, &reg_val);
>> +               ret |= regulator_get_value_desc(dev, type, i, &desc);
>> +               ret |= regulator_get_mode_desc(dev, type, i, &mode_cnt,
>> +                                              &mode_desc);
>> +
>> +               /* Probably no such regulator number */
>> +               if (ret)
>> +                       continue;
>> +
>> +               /* Display in mV */
>> +               reg_val /= div;
>> +
>> +               printf("\n%s%.2d:", regulator_type_str(type), i);
>> +
>> +               switch (display_info) {
>> +               case INFO_STATE:
>> +                       printf(" %d mV ",  reg_val);
>> +
>> +                       regulator_get_state(dev, type, i, &state);
>> +
>> +                       printf("   %s", state ? "ON " : "OFF");
>> +
>> +                       mode_name = get_reg_mode_name(i, mode, mode_desc,
>> +                                                       mode_cnt);
>> +                       if (mode_name)
>> +                               printf("  %d@%s", mode, mode_name);
>> +
>> +                       continue;
>> +               case INFO_DESC:
>> +               case INFO_DESC_VAL:
>> +                       if (desc)
>> +                               printf("\n @name: %s", desc->name);
>> +
>> +                       /* display voltage range */
>> +                       printf("\n @Vout: %d mV", reg_val);
>> +
>> +                       if (!desc)
>> +                               puts("\n @no value descriptors");
>> +                       else
>> +                               printf("\n @Vmin: %d mV\n @Vmax: %d mV",
>> +                                                       (desc->min_uV / div),
>> +                                                       (desc->max_uV / div));
>> +
>> +                       if (display_info != INFO_DESC)
>> +                               continue;
>> +               case INFO_DESC_MODE:
>> +                       if (!mode_desc) {
>> +                               puts("\n @no mode descriptors");
>> +                               continue;
>> +                       }
>> +
>> +                       /* display operation modes info */
>> +                       for (j = 0; j < mode_cnt; j++) {
>> +                               printf("\n @mode: %d (%s)", mode_desc[j].mode,
>> +                                                         mode_desc[j].name);
>> +                               if (mode_desc[j].mode == mode)
>> +                                       puts(" (active)");
>> +                       }
>> +                       continue;
>> +               case INFO_NAME:
>> +                       if (desc)
>> +                               printf(" %s", desc->name);
>> +                       else
>> +                               puts(" -");
>> +                       continue;
>> +               }
>> +       }
>> +
>> +       puts("\n");
>> +
>> +       return 0;
>> +}
>> +
>> +/**
>> + * check_reg_mode: check if the given regulator supports the given mode
>> + *
>> + * @dev: device to check
>> + * @reg_num: given devices regulator number
>> + * @set_mode: mode to check - a mode value of struct regulator_mode_desc
>> + * @type: descriptor type: REGULATOR_TYPE_LDO or REGULATOR_TYPE_BUCK
>> + */
>> +static int check_reg_mode(struct udevice *dev, int reg_num, int set_mode,
>> +                         int type)
>> +{
>> +       struct regulator_mode_desc *mode_desc;
>> +       int i, mode_cnt;
>> +
>> +       regulator_get_mode_desc(dev, type, reg_num, &mode_cnt, &mode_desc);
>> +       if (!mode_desc)
>> +               goto nodesc;
>> +
>> +       for (i = 0; i < mode_cnt; i++) {
>> +               if (mode_desc[i].mode == set_mode)
>> +                       return 0;
>> +       }
>> +
>> +       printf("Mode:%d not found in the descriptor:", set_mode);
>> +
>> +       display_reg_info(type, reg_num, reg_num, INFO_DESC_MODE);
>> +
>> +       return -EINVAL;
>> +
>> +nodesc:
>> +       printf("%s%.2d - no descriptor found\n", regulator_type_str(type),
>> +                                                reg_num);
>> +       return -ENODEV;
>> +}
>> +
>> +static int set_reg_mode(int type, int reg_num, int set_mode)
>> +{
>> +       int ret;
>> +       struct udevice *dev;
>> +
>> +       dev = get_curr_dev(UCLASS_PMIC_REGULATOR);
>> +       if (!dev)
>> +               return -ENODEV;
>> +
>> +       if (check_reg_mode(dev, reg_num, set_mode, type))
>> +               return -EPERM;
>> +
>> +       printf("Set %s%.2d mode: %d\n", regulator_type_str(type),
>
> Why %2d? Why not just %d?
>
>> +                                       reg_num, set_mode);
>> +
>> +       ret = regulator_set_mode(dev, type, reg_num, set_mode);
>> +
>> +       return ret;
>> +}
>> +
>> +/**
>> + * check_reg_val: check if the given regulator value meets
>> + *                the descriptor min and max values
>> + *
>> + * @dev: device to check
>> + * @reg_num: given devices regulator number
>> + * @set_val: value to check - in uV
>> + * @type: descriptor type: REGULATOR_TYPE_LDO or REGULATOR_TYPE_BUCK
>> + */
>> +static int check_reg_val(struct udevice *dev, int reg_num, int set_val,
>> +                        int type)
>> +{
>> +       struct regulator_value_desc *desc;
>> +
>> +       regulator_get_value_desc(dev, type, reg_num, &desc);
>> +       if (!desc)
>> +               goto nodesc;
>> +
>> +       if (set_val >= desc->min_uV && set_val <= desc->max_uV)
>> +               return 0;
>> +
>> +       printf("Value: %d mV exceeds descriptor limits:", (set_val / 1000));
>> +
>> +       display_reg_info(type, reg_num, reg_num, INFO_DESC_VAL);
>> +
>> +       return -EINVAL;
>> +nodesc:
>> +       printf("%s%.2d - no descriptor found\n", regulator_type_str(type),
>> +                                                reg_num);
>> +       return -ENODEV;
>> +}
>> +
>> +static int set_reg_val(int type, int reg_num, int set_val, int set_force)
>> +{
>> +       int ret;
>> +       struct udevice *dev;
>> +
>> +       dev = get_curr_dev(UCLASS_PMIC_REGULATOR);
>> +       if (!dev)
>> +               return -ENODEV;
>> +
>> +       /* get mV and set as uV */
>> +       set_val *= 1000;
>
> Perhaps the command should work in uV too? Or maybe a 'mV' suffix on numbers?
>
>> +
>> +       if (!set_force && check_reg_val(dev, reg_num, set_val, type))
>> +               return -EPERM;
>> +
>> +       printf("Set %s%.2d val: %d mV", regulator_type_str(type),
>> +                                       reg_num, (set_val / 1000));
>> +
>> +       if (set_force)
>> +               puts(" (force)\n");
>> +       else
>> +               puts("\n");
>> +
>> +       ret = regulator_set_value(dev, type, reg_num, set_val);
>> +       return ret;
>
> You don't need ret.
>
>> +}
>> +
>> +static int regulator_subcmd(char * const argv[], int argc, int type,
>> +                     int reg_num, int reg_cnt)
>> +{
>> +       int start_reg, end_reg;
>> +       int set_val, set_force;
>> +       const char *cmd;
>> +
>> +       /* If reg number is given */
>> +       if (reg_num >= 0) {
>> +               start_reg = reg_num;
>> +               end_reg = reg_num;
>> +       } else {
>> +               start_reg = 0;
>> +               end_reg = reg_cnt;
>> +       }
>> +
>> +       cmd = argv[0];
>> +       if (!strcmp("name", cmd))
>> +               return display_reg_info(type, start_reg, end_reg, INFO_NAME);
>> +       else if (!strcmp("state", cmd))
>> +               return display_reg_info(type, start_reg, end_reg, INFO_STATE);
>> +       else if (!strcmp("desc", cmd))
>> +               return display_reg_info(type, start_reg, end_reg, INFO_DESC);
>> +
>> +       /* Check if regulator number is given */
>> +       if (reg_num < 0) {
>> +               puts("reg num?\n");
>> +               return -EINVAL;
>> +       }
>> +
>> +       if (!strcmp("on", cmd))
>> +               return set_reg_state(type, reg_num, REGULATOR_ON);
>> +
>> +       if (!strcmp("off", cmd))
>> +               return set_reg_state(type, reg_num, REGULATOR_OFF);
>
> Can we use a sub command table again?
>
>> +
>> +       /* Check argument value */
>> +       if (argc < 2 || !isdigit(*argv[1])) {
>> +               puts("Expected positive value!\n");
>> +               return -EINVAL;
>> +       }
>> +
>> +       set_val = simple_strtoul(argv[1], NULL, 0);
>> +       set_force = 0;
>> +
>> +       if (!strcmp("setval", cmd)) {
>> +               if (argc == 3 && !strcmp("-f", argv[2]))
>> +                       set_force = 1;
>> +
>> +               return set_reg_val(type, reg_num, set_val, set_force);
>> +       }
>> +
>> +       if (!strcmp("setmode", cmd))
>> +               return set_reg_mode(type, reg_num, set_val);
>> +
>> +       return -EINVAL;
>> +}
>> +
>> +static int do_regulator(cmd_tbl_t *cmdtp, int flag, int argc,
>> +                       char * const argv[])
>> +{
>> +       struct udevice *dev = NULL;
>> +       int cmd_len;
>> +       int ret = 0;
>> +       int reg_num = -1;
>> +       int reg_cnt;
>> +       int i;
>> +       char *cmd;
>> +
>> +       if (!argc)
>> +               return CMD_RET_USAGE;
>> +
>> +       /* list, dev, dump */
>> +       ret = pmic_cmd(argv, argc, UCLASS_PMIC_REGULATOR);
>> +       if (ret != 1)
>> +               goto finish;
>> +
>> +       cmd = argv[0];
>> +       cmd_len = strlen(cmd);
>> +
>> +       dev = get_curr_dev(UCLASS_PMIC_REGULATOR);
>> +       if (!dev)
>> +               return CMD_RET_USAGE;
>> +
>> +       for (i = 0; i < ARRAY_SIZE(type_info); i++) {
>> +               if (!strncmp(cmd, type_info[i].name, type_info[i].len)) {
>> +                       ret = regulator_get_cnt(dev, type_info[i].id, &reg_cnt);
>> +                       break;
>> +               }
>> +       }
>> +
>> +       if (!reg_cnt) {
>> +               printf("Bad regulator type!\n");
>> +               goto finish;
>> +       }
>> +
>> +       /* Get regulator number */
>> +       reg_num = -1;
>> +       if (cmd_len > type_info[i].len && isdigit(cmd[type_info[i].len]))
>> +               reg_num = (int)simple_strtoul(cmd + type_info[i].len , NULL, 0);
>> +
>> +       if (reg_num > reg_cnt) {
>> +               printf("Max dev %s number is: %d\n", type_info[i].name,
>> +                                                    reg_cnt);
>> +               return CMD_RET_SUCCESS;
>> +       }
>> +
>> +       /* Subcommand missed? */
>> +       if (argc == 1) {
>> +               printf("name/state/desc/setval/setmode/on/off ?\n");
>> +               return CMD_RET_SUCCESS;
>> +       }
>> +
>> +       argv++;
>> +       argc--;
>> +       ret = regulator_subcmd(argv, argc, type_info[i].id, reg_num, reg_cnt);
>> +
>> +finish:
>> +       if (ret < 0)
>> +               return CMD_RET_FAILURE;
>> +
>> +       return CMD_RET_SUCCESS;
>> +}
>> +#endif /* CONFIG_DM_REGULATOR_CMD */
>> +
>> +#ifdef CONFIG_DM_PMIC_CMD
>> +static int do_pmic(cmd_tbl_t *cmdtp, int flag, int argc, char * const argv[])
>> +{
>> +       struct udevice *dev, *io_dev;
>> +       unsigned reg;
>> +       unsigned char val;
>> +       char *cmd;
>> +       int ret = 0;
>> +
>> +       if (!argc)
>> +               return CMD_RET_USAGE;
>> +
>> +       /* list, dev, dump */
>> +       ret = pmic_cmd(argv, argc, UCLASS_PMIC);
>> +       if (ret != 1)
>> +               goto finish;
>> +
>> +       dev = get_curr_dev(UCLASS_PMIC);
>> +       ret = pmic_io_dev(dev, &io_dev);
>> +       if (ret)
>> +               goto finish;
>> +
>> +       cmd = argv[0];
>> +       if (strcmp(cmd, "read") == 0) {
>
> Probably not worth a sub-command table here.
>
>> +               if (argc != 2) {
>> +                       ret = -EINVAL;
>> +                       goto finish;
>> +               }
>> +
>> +               printf("cur dev: %p name: %s\n", dev, dev->name);
>> +               reg = simple_strtoul(argv[1], NULL, 0);
>> +
>> +               printf("read reg: %#x\n", reg);
>> +               ret = pmic_read(io_dev, reg, &val);
>> +               if (ret)
>> +                       puts("PMIC: Register read failed\n");
>> +               else
>> +                       printf("0x%02x: 0x%2.2x\n", reg, val);
>> +
>> +               goto finish;
>> +       }
>> +
>> +       if (strcmp(cmd, "write") == 0) {
>> +               if (argc != 3) {
>> +                       ret = -EINVAL;
>> +                       goto finish;
>> +               }
>> +
>> +               reg = simple_strtoul(argv[1], NULL, 0);
>> +               val = simple_strtoul(argv[2], NULL, 0);
>> +
>> +               printf("Write reg:%#x  val: %#x\n", reg, val);
>> +
>> +               ret = pmic_write(io_dev, reg, val);
>> +               if (ret)
>> +                       puts("PMIC: Register write failed\n");
>> +
>> +               goto finish;
>> +       }
>> +
>> +finish:
>> +       if (ret < 0) {
>> +               printf("Error: %d\n", ret);
>> +               return CMD_RET_FAILURE;
>> +       }
>> +
>> +       return CMD_RET_SUCCESS;
>> +}
>> +#endif /* CONFIG_DM_PMIC_CMD */
>> +
>> +static cmd_tbl_t cmd_pmic[] = {
>> +#ifdef CONFIG_DM_PMIC_CMD
>> +       U_BOOT_CMD_MKENT(pmic, 5, 1, do_pmic, "", ""),
>> +#endif
>> +#ifdef CONFIG_DM_REGULATOR_CMD
>> +       U_BOOT_CMD_MKENT(regulator, 5, 1, do_regulator, "", ""),
>> +#endif
>> +};
>> +
>> +static int do_pmicops(cmd_tbl_t *cmdtp, int flag, int argc, char * const argv[])
>> +{
>> +       cmd_tbl_t *cmd;
>> +
>> +       cmd = find_cmd_tbl(argv[0], cmd_pmic, ARRAY_SIZE(cmd_pmic));
>> +       if (cmd == NULL || argc > cmd->maxargs)
>> +               return CMD_RET_USAGE;
>> +
>> +       argc--;
>> +       argv++;
>> +
>> +       return cmd->cmd(cmdtp, flag, argc, argv);
>> +}
>> +
>> +#ifdef CONFIG_DM_PMIC_CMD
>> +U_BOOT_CMD(pmic,       CONFIG_SYS_MAXARGS, 1, do_pmicops,
>> +       "PMIC",
>> +       "list                - list available PMICs\n"
>> +       "pmic dev <id>            - set id to current pmic device\n"
>> +       "pmic dump                - dump registers\n"
>> +       "pmic read <reg>          - read register\n"
>> +       "pmic write <reg> <value> - write register\n"
>> +);
>> +#endif /* CONFIG_DM_PMIC_CMD */
>> +#ifdef CONFIG_DM_REGULATOR_CMD
>> +U_BOOT_CMD(regulator, CONFIG_SYS_MAXARGS, 1, do_pmicops,
>> +       "PMIC Regulator",
>> +       "list\n\t- list UCLASS regulator devices\n"
>> +       "regulator dev [id]\n\t- show or set operating regulator device\n"
>> +       "regulator dump\n\t- dump registers of current regulator\n"
>> +       "regulator [ldo/buck/dvs][N] [name/state/desc]"
>> +       "\n\t- print regulator(s) info\n"
>> +       "regulator [ldoN/buckN/dvsN] [setval/setmode] [mV/modeN] [-f]"
>> +       "\n\t- set val (mV) (forcibly) or mode - only if desc available\n\n"
>> +       "Example of usage:\n"
>> +       "First get available commands:\n"
>> +       " # regulator\n"
>> +       "Look after the regulator device 'Id' number:\n"
>> +       " # regulator list\n"
>> +       "Set the operating device:\n"
>> +       " # regulator dev 'Id'\n"
>> +       "List state of device ldo's:\n"
>> +       " # regulator ldo state\n"
>> +       "List descriptors of device ldo's:\n"
>> +       " # regulator ldo desc\n"
>> +       "Set the voltage of ldo number 1 to 1200mV\n"
>> +       " # regulator ldo1 setval 1200\n"
>> +       "Use option: '-f', if given value exceeds the limits(be careful!):\n"
>> +       " # regulator ldo1 setval 1200 -f\n"
>> +       "Set the mode of ldo number 1 to '5' (force not available):\n"
>> +       " # regulator ldo1 setmode 5\n"
>
> So the regulators in the device have numbers. I presume they also have
> names by virtue of the device tree?
>
>> +);
>> +#endif /* CONFIG_DM_REGULATOR_CMD */
>> --
>> 1.9.1
>>
>
> Regards,
> Simon
>

Thanks for the suggestions. The commands in the V3 are reimplemented 
into a separated files. The rework of API allowed to make this more clear.

Best regards,
-- 
Przemyslaw Marczak
Samsung R&D Institute Poland
Samsung Electronics
p.marczak at samsung.com