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

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

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 at samsung.com