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

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

[Ahmad Fatoum]

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;
+		default:
+			return COMMAND_ERROR_USAGE;
+		}
+	}
 
+	regulators_print();
 	return 0;
 }
 
+BAREBOX_CMD_HELP_START(regulator)
+	BAREBOX_CMD_HELP_TEXT("List and control regulators.")
+	BAREBOX_CMD_HELP_TEXT("Without options, displays regulator info")
+	BAREBOX_CMD_HELP_TEXT("Options:")
+	BAREBOX_CMD_HELP_OPT("-e REGULATOR\t", "enable REGULATOR")
+	BAREBOX_CMD_HELP_OPT("-d REGULATOR\t", "disable REGULATOR")
+BAREBOX_CMD_HELP_END
+
 BAREBOX_CMD_START(regulator)
 	.cmd		= do_regulator,
-	BAREBOX_CMD_DESC("list regulators")
-	BAREBOX_CMD_GROUP(CMD_GRP_INFO)
+	BAREBOX_CMD_DESC("list and control regulators")
+	BAREBOX_CMD_OPTS("[-ed] [REGULATOR]")
+	BAREBOX_CMD_GROUP(CMD_GRP_HWMANIP)
+	BAREBOX_CMD_HELP(cmd_regulator_help)
 BAREBOX_CMD_END
diff --git a/drivers/regulator/core.c b/drivers/regulator/core.c
index b2e5f8caa2ca..35567100c5af 100644
--- a/drivers/regulator/core.c
+++ b/drivers/regulator/core.c
@@ -355,6 +355,14 @@ struct regulator *regulator_get(struct device_d *dev, const char *supply)
 	return r;
 }
 
+void regulator_put(struct regulator *r)
+{
+	if (IS_ERR_OR_NULL(r))
+		return;
+	list_del(&r->list);
+	free(r);
+}
+
 static struct regulator_internal *regulator_by_name(const char *name)
 {
 	struct regulator_internal *ri;
diff --git a/include/regulator.h b/include/regulator.h
index dfdfbf033262..f4cf6726c2ad 100644
--- a/include/regulator.h
+++ b/include/regulator.h
@@ -148,6 +148,7 @@ void regulators_print(void);
 #ifdef CONFIG_REGULATOR
 
 struct regulator *regulator_get(struct device_d *, const char *);
+void regulator_put(struct regulator *r);
 struct regulator *regulator_get_name(const char *name);
 int regulator_enable(struct regulator *);
 int regulator_disable(struct regulator *);
-- 
2.30.2


_______________________________________________
barebox mailing list
barebox@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/barebox

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

[Sascha Hauer]

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.

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 |

_______________________________________________
barebox mailing list
barebox@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/barebox

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

[Ahmad Fatoum]

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?

[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 |

_______________________________________________
barebox mailing list
barebox@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/barebox

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

[Ahmad Fatoum]

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 |