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

Simon Glass sjg at chromium.org
Sat Jul 18 01:57:22 CEST 2015


On 1 July 2015 at 03:44, Przemyslaw Marczak <p.marczak at 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 at 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 at samsung.com>
> Acked-by: Przemyslaw Marczak <p.marczak at samsung.com>
>
> Best regards,
> --
> Przemyslaw Marczak
> Samsung R&D Institute Poland
> Samsung Electronics
> p.marczak at samsung.com

Applied to u-boot-dm.


More information about the U-Boot mailing list