Re: [U-Boot] [PATCH v3 36/54] dm: pmic: Split output from function

On 1 July 2015 at 03:44, Przemyslaw Marczak p.marczak@samsung.com wrote:

Hello Simon,

On 06/23/2015 11:38 PM, Simon Glass wrote:

...

The regulator_autoset() function mixes printf() output and PMIC adjustment code. It provides a boolean to control the output. It is better to avoid missing logic and output, and this permits a smaller SPL code size. So split the output into a separate function.

Also rename the function to have a by_name() suffix, since we would like to be able to pass a device when we know it, and thus avoid the name search.

Signed-off-by: Simon Glass sjg@chromium.org

---

Changes in v3: None Changes in v2: None

drivers/power/regulator/regulator-uclass.c | 98 +++++++++++------------------- include/power/regulator.h | 34 ++++++++--- include/power/sandbox_pmic.h | 4 +- test/dm/regulator.c | 2 +- 4 files changed, 63 insertions(+), 75 deletions(-)

diff --git a/drivers/power/regulator/regulator-uclass.c b/drivers/power/regulator/regulator-uclass.c index 0f1ca77..687d3b1 100644 --- a/drivers/power/regulator/regulator-uclass.c +++ b/drivers/power/regulator/regulator-uclass.c @@ -138,87 +138,57 @@ int regulator_get_by_devname(const char *devname, struct udevice **devp) return uclass_get_device_by_name(UCLASS_REGULATOR, devname, devp); }

-static int failed(int ret, bool verbose, const char *fmt, ...) +int regulator_autoset(struct udevice *dev) {

•   va_list args;
  

•   char buf[64];
  

•   if (verbose == false)
  

•           return ret;
  

•   struct dm_regulator_uclass_platdata *uc_pdata;
  

•   int ret = 0;
  
  

•   va_start(args, fmt);
  

•   vscnprintf(buf, sizeof(buf), fmt, args);
  

•   va_end(args);
  

•   uc_pdata = dev_get_uclass_platdata(dev);
  

•   if (!uc_pdata->always_on && !uc_pdata->boot_on)
  

•           return -EMEDIUMTYPE;
  
  

•   printf(buf);
  

•   if (uc_pdata->flags & REGULATOR_FLAG_AUTOSET_UV)
  

•           ret = regulator_set_value(dev, uc_pdata->min_uV);
  

•   if (!ret && (uc_pdata->flags & REGULATOR_FLAG_AUTOSET_UA))
  

•           ret = regulator_set_current(dev, uc_pdata->min_uA);
  
    if (!ret)
  

•           return 0;
  

•   printf(" (ret: %d)", ret);
  

•           ret = regulator_set_enable(dev, true);
  
    return ret;
  

  }

-int regulator_autoset(const char *platname,

•                 struct udevice **devp,
  

•                 bool verbose)
  

+static void regulator_show(struct udevice *dev, int ret) { struct dm_regulator_uclass_platdata *uc_pdata;

•   struct udevice *dev;
  

•   int ret;
  

•   if (devp)
  

•           *devp = NULL;
  

•   ret = regulator_get_by_platname(platname, &dev);
  

•   if (ret) {
  

•           error("Can get the regulator: %s!", platname);
  

•           return ret;
  

•   }
  
    uc_pdata = dev_get_uclass_platdata(dev);
  

•   if (!uc_pdata) {
  

•           error("Can get the regulator %s uclass platdata!",
  

platname);

•           return -ENXIO;
  

•   }
  

•   if (!uc_pdata->always_on && !uc_pdata->boot_on)
  

•           goto retdev;
  
  

•   if (verbose)
  

•           printf("%s@%s: ", dev->name, uc_pdata->name);
  

•   /* Those values are optional (-ENODATA if unset) */
  

•   if ((uc_pdata->min_uV != -ENODATA) &&
  

•       (uc_pdata->max_uV != -ENODATA) &&
  

•       (uc_pdata->min_uV == uc_pdata->max_uV)) {
  

•           ret = regulator_set_value(dev, uc_pdata->min_uV);
  

•           if (failed(ret, verbose, "set %d uV", uc_pdata->min_uV))
  

•                   goto exit;
  

•   }
  

•   /* Those values are optional (-ENODATA if unset) */
  

•   if ((uc_pdata->min_uA != -ENODATA) &&
  

•       (uc_pdata->max_uA != -ENODATA) &&
  

•       (uc_pdata->min_uA == uc_pdata->max_uA)) {
  

•           ret = regulator_set_current(dev, uc_pdata->min_uA);
  

•           if (failed(ret, verbose, "; set %d uA", uc_pdata->min_uA))
  

•                   goto exit;
  

•   }
  

•   printf("%s@%s: ", dev->name, uc_pdata->name);
  

•   if (uc_pdata->flags & REGULATOR_FLAG_AUTOSET_UV)
  

•           printf("set %d uV", uc_pdata->min_uV);
  

•   if (uc_pdata->flags & REGULATOR_FLAG_AUTOSET_UA)
  

•           printf("; set %d uA", uc_pdata->min_uA);
  

•   printf("; enabling");
  

•   if (ret)
  

•           printf(" (ret: %d)\n", ret);
  

•   printf("\n");
  

+}

•   ret = regulator_set_enable(dev, true);
  

•   if (failed(ret, verbose, "; enabling", uc_pdata->min_uA))
  

•           goto exit;
  

+int regulator_autoset_by_name(const char *platname, struct udevice **devp) +{

•   struct udevice *dev;
  

•   int ret;
  
  

-retdev:

•   ret = regulator_get_by_platname(platname, &dev);
    if (devp)
            *devp = dev;
  

-exit:

•   if (verbose)
  

•           printf("\n");
  

•   if (ret) {
  

•           debug("Can get the regulator: %s!", platname);
  

•           return ret;
  

•   }
  
  

•   return ret;
  

•   return regulator_autoset(dev);
  

  }

  int regulator_list_autoset(const char *list_platname[],

@@ -229,7 +199,9 @@ int regulator_list_autoset(const char *list_platname[], int error = 0, i = 0, ret;

    while (list_platname[i]) {

•           ret = regulator_autoset(list_platname[i], &dev, verbose);
  

•           ret = regulator_autoset_by_name(list_platname[i], &dev);
  

•           if (ret != -EMEDIUMTYPE && verbose)
  

•                   regulator_show(dev, ret);
            if (ret & !error)
                    error = ret;
  
  

diff --git a/include/power/regulator.h b/include/power/regulator.h index 79ce0a4..86e9c3b 100644 --- a/include/power/regulator.h +++ b/include/power/regulator.h @@ -316,9 +316,28 @@ int regulator_get_mode(struct udevice *dev); int regulator_set_mode(struct udevice *dev, int mode_id);

/**

• • regulator_autoset: setup the regulator given by its uclass's platform

data

• • name field. The setup depends on constraints found in device's

uclass's

• • platform data (struct dm_regulator_uclass_platdata):

• • regulator_autoset: setup the the voltage/current on a regulator

duplicated "the"

...

• • The setup depends on constraints found in device's uclass's platform

data

• • (struct dm_regulator_uclass_platdata):
• • • Enable - will set - if any of: 'always_on' or 'boot_on' is set to

true,

• • or if both are unset, then the function returns
• • • Voltage value - will set - if '.min_uV' and '.max_uV' values are

equal

• • • Current limit - will set - if '.min_uA' and '.max_uA' values are

equal

• • The function returns on the first-encountered error.
• • @platname - expected string for dm_regulator_uclass_platdata .name

field

• • @devp - returned pointer to the regulator device - if non-NULL

passed

• • @return: 0 on success or negative value of errno.
• */

+int regulator_autoset(struct udevice *dev);

+/**

• • regulator_autoset_by_name: setup the regulator given by its uclass's
• • platform data name field. The setup depends on constraints found in

device's

• • uclass's platform data (struct dm_regulator_uclass_platdata):
  • • Enable - will set - if any of: 'always_on' or 'boot_on' is set to

true,

• or if both are unset, then the function returns
• • Voltage value - will set - if '.min_uV' and '.max_uV' values are

equal @@ -328,21 +347,18 @@ int regulator_set_mode(struct udevice *dev, int mode_id);

• @platname - expected string for dm_regulator_uclass_platdata .name

field

• @devp - returned pointer to the regulator device - if non-NULL

passed

• • @verbose - (true/false) print regulator setup info, or be quiet
  • @return: 0 on success or negative value of errno.
  • The returned 'regulator' device can be used with:
  • • regulator_get/set_*
  */

-int regulator_autoset(const char *platname,

•                 struct udevice **devp,
  

•                 bool verbose);
  

+int regulator_autoset_by_name(const char *platname, struct udevice **devp);

/**

• regulator_list_autoset: setup the regulators given by list of their

uclass's

• platform data name field. The setup depends on constraints found in

device's

• uclass's platform data. The function loops with calls to:

• • regulator_autoset() for each name from the list.

• • regulator_autoset_by_name() for each name from the list.
  • @list_platname - an array of expected strings for .name field of each

  •              regulator's uclass platdata
    

@@ -383,7 +399,7 @@ int regulator_get_by_devname(const char *devname, struct udevice **devp);

• Search by name, found in regulator uclass platdata.
• @platname - expected string for uc_pdata->name of regulator uclass

platdata

• • @devp - returned pointer to the regulator device

• • @devp - returns pointer to the regulator device or NULL on error
  • @return 0 on success or negative value of errno.
  • The returned 'regulator' device is probed and can be used with:

diff --git a/include/power/sandbox_pmic.h b/include/power/sandbox_pmic.h index ae14292..8547674 100644 --- a/include/power/sandbox_pmic.h +++ b/include/power/sandbox_pmic.h @@ -117,11 +117,11 @@ enum {

/*

• Expected regulators setup after call of:

• • • regulator_autoset()

• • • regulator_autoset_by_name()
  • • regulator_list_autoset()
  */

-/* BUCK1: for testing regulator_autoset() */ +/* BUCK1: for testing regulator_autoset_by_name() */ #define SANDBOX_BUCK1_AUTOSET_EXPECTED_UV 1200000 #define SANDBOX_BUCK1_AUTOSET_EXPECTED_UA 200000 #define SANDBOX_BUCK1_AUTOSET_EXPECTED_ENABLE true diff --git a/test/dm/regulator.c b/test/dm/regulator.c index d279c04..3d0056f 100644 --- a/test/dm/regulator.c +++ b/test/dm/regulator.c @@ -210,7 +210,7 @@ static int dm_test_power_regulator_autoset(struct unit_test_state *uts) * Expected output state: uV=1200000; uA=200000; output enabled */ platname = regulator_names[BUCK1][PLATNAME];

•   ut_assertok(regulator_autoset(platname, &dev_autoset, false));
  

•   ut_assertok(regulator_autoset_by_name(platname, &dev_autoset));
  
    /* Check, that the returned device is proper */
    ut_assertok(regulator_get_by_platname(platname, &dev));
  
  

Tested on:

• Odroid U3 (odroid_defconfig)
• Sandbox - ut pmic/regulator

Tested-by: Przemyslaw Marczak p.marczak@samsung.com Acked-by: Przemyslaw Marczak p.marczak@samsung.com

Best regards,

Przemyslaw Marczak Samsung R&D Institute Poland Samsung Electronics p.marczak@samsung.com

Applied to u-boot-dm.