[U-Boot] [PATCH 09/19] dm: pmic: new commands: pmic and regulator

[Tom Rini]

On Wed, Oct 08, 2014 at 10:48:45PM +0200, Przemyslaw Marczak wrote:

> This introduces new commands:
> - pmic (new) - CONFIG_DM_PMIC
> - regulator - CONFIG_DM_PMIC
> Both uses a common code and dm pmic api.
> 
> To avoid code mess the old pmic command is kept without changes.
> 
> Command pmic
> This is based on an old pmic command code - the new one uses
> driver model pmic api. The previous pmic I/O functionalities,
> stays unchanged. And now read/write is the main pmic command
> purpose. This command can use only UCLASS_PMIC devices,
> since this uclass is designed for pmic I/O operations only.
> 
> 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 similar to the 'mmc' command.
> 
> Each pmic device driver should provide at least UCLASS_PMIC support,
> as a basic pmic device I/O interface.
> 
> Command regulator
> The extension for 'pmic' command is 'regulator' command.
> This actually uses the same code, but provides an interface
> to pmic optional 'UCLASS_PMIC_REGULATOR' operations.
> 
> If pmic device driver provides support to this another pmic
> uclass, then this command provides useful user interface.
> 
> The regulator operations and required structures can be found
> in this header: 'include/power/regulator.h'
> 
> This was designed to allow safe I/O access to pmic device
> without the pmic documentation. If driver provide each regulator
> - value and mode descriptor - then user can operate on it.
> 
> Command options (regulator [option]):
> - list     - list UCLASS regulator devices
> - dev [id] - show or set current regulator device
> - dump     - dump registers of current regulator
> - [ldo/buck][N] [name/state/desc]- print regulator(s) info
> - [ldoN/buckN] [setval/setmode] [mV/modeN] [-f] - set val (mV)
>   (forcibly) or mode - only if desc available
> 
> Example of usage:
> regulator list      - look after regulator 'Id' number
> regulator dev 'Id'  - set current device
> regulator ldo state - list state of current device all ldo's
> regulator ldo desc  - list ldo's descriptors
> regulator ldo1 setval 1000    - set device ldo 1 voltage to 1000mV
> !!And the next one can be risky!!
> regulator ldo1 setval 1200 -f - if value exceeds limits - set force
> regulator ldo1 setmode 5 - set device ldo 1 mode to '5' (force no available)
> 
> The same for 'buck' regulators.
> 
> 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, but the required voltage value can change,
> e.g. if some hardware uses pins header.
> 
> Signed-off-by: Przemyslaw Marczak <p.marczak at samsung.com>
[snip]
> +	if (ret < 0) {
> +		printf("Error: %s\n", errno_str(ret));
> +		return CMD_RET_FAILURE;
> +	}
[snip]
> +finish:
> +	if (ret < 0) {
> +		printf("Error: %s\n", errno_str(ret));
> +		return CMD_RET_FAILURE;
> +	}

So, my main concern with this patch is we have here the only users of
errno_str.  In my reading over of the patch it looked like in nearly
every case it was -ENODEV/-EINVAL and we had already printed a useful
looking error message just before.  Further, I'm not convinced (yet!)
that the errno print is adding to the utility in discovering what was
passed in wrong.  Can you please re-work this patch to not need
errno_str(), ensure that errors still print _something_ and see what the
delta is, both in binary and lines of code?  Thanks!

-- 
Tom
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 836 bytes
Desc: Digital signature
URL: <http://lists.denx.de/pipermail/u-boot/attachments/20141022/cf9e34b5/attachment.pgp>