Re: [PATCH] commands: regulator: add support for enabling/disabling regulators

[Ahmad Fatoum <a.fatoum@pengutronix.de>]

Hello Sascha,

On 05.01.22 10:21, Ahmad Fatoum wrote:
> Hi,
> 
> On 05.01.22 10:14, Sascha Hauer wrote:
>> On Mon, Jan 03, 2022 at 01:03:36PM +0100, Ahmad Fatoum wrote:
>>> For testing regulator drivers, it can be handy to enable/disable them
>>> from the shell prompt. Extend the regulator command to support this.
>>>
>>> Signed-off-by: Ahmad Fatoum <a.fatoum@pengutronix.de>
>>> ---
>>>  commands/regulator.c     | 38 +++++++++++++++++++++++++++++++++++---
>>>  drivers/regulator/core.c |  8 ++++++++
>>>  include/regulator.h      |  1 +
>>>  3 files changed, 44 insertions(+), 3 deletions(-)
>>>
>>> diff --git a/commands/regulator.c b/commands/regulator.c
>>> index 3e2595f8bfc1..e6b2f4852db4 100644
>>> --- a/commands/regulator.c
>>> +++ b/commands/regulator.c
>>> @@ -6,16 +6,48 @@
>>>  #include <common.h>
>>>  #include <command.h>
>>>  #include <regulator.h>
>>> +#include <getopt.h>
>>>  
>>>  static int do_regulator(int argc, char *argv[])
>>>  {
>>> -	regulators_print();
>>> +	struct regulator *chosen;
>>> +	int opt, ret;
>>> +
>>> +	while ((opt = getopt(argc, argv, "e:d:")) > 0) {
>>> +		switch (opt) {
>>> +		case 'e':
>>> +		case 'd':
>>> +			chosen = regulator_get_name(optarg);
>>> +			if (IS_ERR_OR_NULL(chosen)) {
>>> +				printf("regulator not found\n");
>>> +				return COMMAND_ERROR;
>>> +			}
>>> +
>>> +			ret = opt == 'e' ? regulator_enable(chosen)
>>> +				         : regulator_disable(chosen);
>>> +			regulator_put(chosen);
>>> +			return ret;
>>
>> The barebox regulator core distinguishes between struct regulator and
>> struct regulator_internal. regulator_internal represents the physical
>> regulator whereas regulator is allocated for each consumer. If the
>> regulator core were a bit more sophisticated then a regulator would
>> have it's own enable count and you would be warned when a regulators
>> enable count goes below zero.
>>
>> I agree that controlling regulators on the command line would be useful,
>> but I also don't want to block extending the regulator core in such a
>> way.
> 
> Should I move the command implementation then into drivers/regulator/core.c?
> 
> That's what I did here[1], but I seem to recall that you objected to
> moving the command into drivers/ to access internals, when the API
> should suffice/be extended. I can't find the mail right now though or
> perhaps my recollection is erroneous.
> 
> So how to proceed?

I keep forward-porting this patch every time I port a regulator driver.
What do I need to do to get this merged?

Cheers,
Ahmad

> 
> [1]: https://lore.barebox.org/barebox/20191106094459.w32tgsl22ty34vhe@pengutronix.de/#t
> 
> Cheers,
> Ahmad
> 
>>
>> Sascha
>>
> 
> 

-- 
Pengutronix e.K.                           |                             |
Steuerwalder Str. 21                       | http://www.pengutronix.de/  |
31137 Hildesheim, Germany                  | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |