(no subject)

[Gregory CLEMENT]

Subject: [PATCH 0/2] regulator: Add the device tree version to the regulator_get family

Hi,

Currently it is not possible to associate a regulator to a child node
which is not a device. The several ports of an ahci controller are
represented as subnodes but they are not created as devices. In order
to be able to associate each port with a regulator the framework API
needs to be extended.

The second patch adds the device tree version (of_) for each member of
the regulator_get family: normal, exclusive, optional and all of the
manageable version.The of_regulator_get* functions allow using a
device node to get the regulator instead using the device object.

The first patch is not related to the second one, but it is little
improvement.

Gregory

Gregory CLEMENT (2):
  regulator: core: Add a sanity check on the regulator_ enable/disable
    functions
  regulator: core: Add the device tree version to the regulator_get
    family

 drivers/regulator/core.c           | 127 +++++++++++++++++++++++++++++++++----
 drivers/regulator/devres.c         |  70 ++++++++++++++++++--
 include/linux/regulator/consumer.h |  20 ++++++
 3 files changed, 198 insertions(+), 19 deletions(-)

-- 
1.9.1

[PATCH 1/2] regulator: core: Add a sanity check on the regulator_ enable/disable functions

[Gregory CLEMENT]

These two functions use the pointer passed in parameter without any
check. By adding a NULL pointer check, it allows using those functions
from a driver in a more generic way. It is useful especially for the
disable case if the regulator is optional.

Signed-off-by: Gregory CLEMENT <gregory.clement@free-electrons.com>
---
 drivers/regulator/core.c | 13 +++++++++++--
 1 file changed, 11 insertions(+), 2 deletions(-)

diff --git a/drivers/regulator/core.c b/drivers/regulator/core.c
index e225711bb8bc..de29399b5430 100644
--- a/drivers/regulator/core.c
+++ b/drivers/regulator/core.c
@@ -1911,12 +1911,16 @@ static int _regulator_enable(struct regulator_dev *rdev)
  */
 int regulator_enable(struct regulator *regulator)
 {
-	struct regulator_dev *rdev = regulator->rdev;
+	struct regulator_dev *rdev;
 	int ret = 0;
 
+	if (!regulator)
+		return 0;
+
 	if (regulator->always_on)
 		return 0;
 
+	rdev = regulator->rdev;
 	if (rdev->supply) {
 		ret = regulator_enable(rdev->supply);
 		if (ret != 0)
@@ -2024,12 +2028,17 @@ static int _regulator_disable(struct regulator_dev *rdev)
  */
 int regulator_disable(struct regulator *regulator)
 {
-	struct regulator_dev *rdev = regulator->rdev;
+	struct regulator_dev *rdev;
 	int ret = 0;
 
+	if (!regulator)
+		return 0;
+
 	if (regulator->always_on)
 		return 0;
 
+	rdev = regulator->rdev;
+
 	mutex_lock(&rdev->mutex);
 	ret = _regulator_disable(rdev);
 	mutex_unlock(&rdev->mutex);
-- 
1.9.1

[PATCH 2/2] regulator: core: Add the device tree version to the regulator_get family

[Gregory CLEMENT]

This patch adds the device tree version (of_) for each member of the
regulator_get family: normal, exclusive, optional and all of the
manageable version.

The of_regulator_get* functions allow using a device node to get the
regulator instead using the device object. It is needed for the
regulator associated to a child node which is not a device, it is the
case of the SATA ports of an ahci controller for instance.

Signed-off-by: Gregory CLEMENT <gregory.clement@free-electrons.com>
---
 drivers/regulator/core.c           | 114 +++++++++++++++++++++++++++++++++----
 drivers/regulator/devres.c         |  70 ++++++++++++++++++++---
 include/linux/regulator/consumer.h |  20 +++++++
 3 files changed, 187 insertions(+), 17 deletions(-)

diff --git a/drivers/regulator/core.c b/drivers/regulator/core.c
index de29399b5430..74167b98797a 100644
--- a/drivers/regulator/core.c
+++ b/drivers/regulator/core.c
@@ -127,26 +127,37 @@ static bool have_full_constraints(void)
 
 /**
  * of_get_regulator - get a regulator device node based on supply name
+ * and on device node if provided
+ *
  * @dev: Device pointer for the consumer (of regulator) device
+ * @np: Device tree node pointer on the node containing the regulator
  * @supply: regulator supply name
  *
  * Extract the regulator device node corresponding to the supply name.
  * returns the device node corresponding to the regulator if found, else
  * returns NULL.
  */
-static struct device_node *of_get_regulator(struct device *dev, const char *supply)
+static struct device_node *of_get_regulator_by_node(struct device *dev,
+						    const char *supply,
+						    struct device_node *np)
 {
 	struct device_node *regnode = NULL;
 	char prop_name[32]; /* 32 is max size of property name */
+	struct device_node *node;
+
+	if (np)
+		node = np;
+	else
+		node = dev->of_node;
 
 	dev_dbg(dev, "Looking up %s-supply from device tree\n", supply);
 
 	snprintf(prop_name, 32, "%s-supply", supply);
-	regnode = of_parse_phandle(dev->of_node, prop_name, 0);
+	regnode = of_parse_phandle(node, prop_name, 0);
 
 	if (!regnode) {
 		dev_dbg(dev, "Looking up %s property in node %s failed",
-				prop_name, dev->of_node->full_name);
+				prop_name, node->full_name);
 		return NULL;
 	}
 	return regnode;
@@ -1268,6 +1279,7 @@ static void regulator_supply_alias(struct device **dev, const char **supply)
 
 static struct regulator_dev *regulator_dev_lookup(struct device *dev,
 						  const char *supply,
+						  struct device_node *np,
 						  int *ret)
 {
 	struct regulator_dev *r;
@@ -1278,8 +1290,8 @@ static struct regulator_dev *regulator_dev_lookup(struct device *dev,
 	regulator_supply_alias(&dev, &supply);
 
 	/* first do a dt based lookup */
-	if (dev && dev->of_node) {
-		node = of_get_regulator(dev, supply);
+	if ((dev && dev->of_node) || np) {
+		node = of_get_regulator_by_node(dev, supply, np);
 		if (node) {
 			list_for_each_entry(r, &regulator_list, list)
 				if (r->dev.parent &&
@@ -1322,6 +1334,7 @@ static struct regulator_dev *regulator_dev_lookup(struct device *dev,
 
 /* Internal regulator request function */
 static struct regulator *_regulator_get(struct device *dev, const char *id,
+					struct device_node *node,
 					bool exclusive, bool allow_dummy)
 {
 	struct regulator_dev *rdev;
@@ -1344,7 +1357,7 @@ static struct regulator *_regulator_get(struct device *dev, const char *id,
 
 	mutex_lock(&regulator_list_mutex);
 
-	rdev = regulator_dev_lookup(dev, id, &ret);
+	rdev = regulator_dev_lookup(dev, id, node, &ret);
 	if (rdev)
 		goto found;
 
@@ -1431,7 +1444,7 @@ out:
  */
 struct regulator *regulator_get(struct device *dev, const char *id)
 {
-	return _regulator_get(dev, id, false, true);
+	return _regulator_get(dev, id, NULL, false, true);
 }
 EXPORT_SYMBOL_GPL(regulator_get);
 
@@ -1458,7 +1471,7 @@ EXPORT_SYMBOL_GPL(regulator_get);
  */
 struct regulator *regulator_get_exclusive(struct device *dev, const char *id)
 {
-	return _regulator_get(dev, id, true, false);
+	return _regulator_get(dev, id, NULL, true, false);
 }
 EXPORT_SYMBOL_GPL(regulator_get_exclusive);
 
@@ -1484,10 +1497,91 @@ EXPORT_SYMBOL_GPL(regulator_get_exclusive);
  */
 struct regulator *regulator_get_optional(struct device *dev, const char *id)
 {
-	return _regulator_get(dev, id, false, false);
+	return _regulator_get(dev, id, NULL, false, false);
 }
 EXPORT_SYMBOL_GPL(regulator_get_optional);
 
+/**
+ * of_regulator_get - lookup and obtain a reference to a regulator.
+ * @dev: device for regulator "consumer"
+ * @node: device node for which to get the regulator
+ * @id: Supply name or regulator ID.
+ *
+ * Returns a struct regulator corresponding to the regulator producer,
+ * or IS_ERR() condition containing errno.
+ *
+ * Use of supply names configured via regulator_set_device_supply() is
+ * strongly encouraged.  It is recommended that the supply name used
+ * should match the name used for the supply and/or the relevant
+ * device pins in the datasheet.
+ */
+struct regulator *of_regulator_get(struct device *dev,
+				   const char *id,
+				struct device_node *node)
+{
+	return _regulator_get(dev, id, node, false, true);
+}
+EXPORT_SYMBOL_GPL(of_regulator_get);
+
+/**
+ * of_regulator_get_exclusive - obtain exclusive access to a regulator.
+ * @dev: device for regulator "consumer"
+ * @node: device node for which to get the regulator
+ * @id: Supply name or regulator ID.
+ *
+ * Returns a struct regulator corresponding to the regulator producer,
+ * or IS_ERR() condition containing errno.  Other consumers will be
+ * unable to obtain this regulator while this reference is held and the
+ * use count for the regulator will be initialised to reflect the current
+ * state of the regulator.
+ *
+ * This is intended for use by consumers which cannot tolerate shared
+ * use of the regulator such as those which need to force the
+ * regulator off for correct operation of the hardware they are
+ * controlling.
+ *
+ * Use of supply names configured via regulator_set_device_supply() is
+ * strongly encouraged.  It is recommended that the supply name used
+ * should match the name used for the supply and/or the relevant
+ * device pins in the datasheet.
+ */
+struct regulator *of_regulator_get_exclusive(struct device *dev,
+					     const char *id,
+					     struct device_node *node)
+{
+	return _regulator_get(dev, id, node, true, false);
+}
+EXPORT_SYMBOL_GPL(of_regulator_get_exclusive);
+
+/**
+ * of_regulator_get_optional - obtain optional access to a regulator.
+ * @dev: device for regulator "consumer"
+ * @node: device node for which to get the regulator
+ * @id: Supply name or regulator ID.
+ *
+ * Returns a struct regulator corresponding to the regulator producer,
+ * or IS_ERR() condition containing errno.
+ *
+ * This is intended for use by consumers for devices which can have
+ * some supplies unconnected in normal use, such as some MMC devices.
+ * It can allow the regulator core to provide stub supplies for other
+ * supplies requested using normal regulator_get() calls without
+ * disrupting the operation of drivers that can handle absent
+ * supplies.
+ *
+ * Use of supply names configured via regulator_set_device_supply() is
+ * strongly encouraged.  It is recommended that the supply name used
+ * should match the name used for the supply and/or the relevant
+ * device pins in the datasheet.
+ */
+struct regulator *of_regulator_get_optional(struct device *dev,
+					    const char *id,
+					    struct device_node *node)
+{
+	return _regulator_get(dev, id, node, false, false);
+}
+EXPORT_SYMBOL_GPL(of_regulator_get_optional);
+
 /* Locks held by regulator_put() */
 static void _regulator_put(struct regulator *regulator)
 {
@@ -3714,7 +3808,7 @@ regulator_register(const struct regulator_desc *regulator_desc,
 	if (supply) {
 		struct regulator_dev *r;
 
-		r = regulator_dev_lookup(dev, supply, &ret);
+		r = regulator_dev_lookup(dev, supply, NULL, &ret);
 
 		if (ret == -ENODEV) {
 			/*
diff --git a/drivers/regulator/devres.c b/drivers/regulator/devres.c
index 8f785bc9e510..755fc07ebc33 100644
--- a/drivers/regulator/devres.c
+++ b/drivers/regulator/devres.c
@@ -30,7 +30,9 @@ static void devm_regulator_release(struct device *dev, void *res)
 	regulator_put(*(struct regulator **)res);
 }
 
-static struct regulator *_devm_regulator_get(struct device *dev, const char *id,
+static struct regulator *_devm_regulator_get(struct device *dev,
+					     const char *id,
+					     struct device_node *node,
 					     int get_type)
 {
 	struct regulator **ptr, *regulator;
@@ -41,13 +43,13 @@ static struct regulator *_devm_regulator_get(struct device *dev, const char *id,
 
 	switch (get_type) {
 	case NORMAL_GET:
-		regulator = regulator_get(dev, id);
+		regulator = of_regulator_get(dev, id, node);
 		break;
 	case EXCLUSIVE_GET:
-		regulator = regulator_get_exclusive(dev, id);
+		regulator = of_regulator_get_exclusive(dev, id, node);
 		break;
 	case OPTIONAL_GET:
-		regulator = regulator_get_optional(dev, id);
+		regulator = of_regulator_get_optional(dev, id, node);
 		break;
 	default:
 		regulator = ERR_PTR(-EINVAL);
@@ -74,7 +76,7 @@ static struct regulator *_devm_regulator_get(struct device *dev, const char *id,
  */
 struct regulator *devm_regulator_get(struct device *dev, const char *id)
 {
-	return _devm_regulator_get(dev, id, NORMAL_GET);
+	return _devm_regulator_get(dev, id, NULL, NORMAL_GET);
 }
 EXPORT_SYMBOL_GPL(devm_regulator_get);
 
@@ -90,7 +92,7 @@ EXPORT_SYMBOL_GPL(devm_regulator_get);
 struct regulator *devm_regulator_get_exclusive(struct device *dev,
 					       const char *id)
 {
-	return _devm_regulator_get(dev, id, EXCLUSIVE_GET);
+	return _devm_regulator_get(dev, id, NULL, EXCLUSIVE_GET);
 }
 EXPORT_SYMBOL_GPL(devm_regulator_get_exclusive);
 
@@ -106,10 +108,64 @@ EXPORT_SYMBOL_GPL(devm_regulator_get_exclusive);
 struct regulator *devm_regulator_get_optional(struct device *dev,
 					      const char *id)
 {
-	return _devm_regulator_get(dev, id, OPTIONAL_GET);
+	return _devm_regulator_get(dev, id, NULL, OPTIONAL_GET);
 }
 EXPORT_SYMBOL_GPL(devm_regulator_get_optional);
 
+/**
+ * devm_regulator_get - Resource managed regulator_get()
+ * @dev: device for regulator "consumer"
+ * @node: device node for which to get the regulator
+ * @id: Supply name or regulator ID.
+ *
+ * Managed regulator_get(). Regulators returned from this function are
+ * automatically regulator_put() on driver detach. See regulator_get()
+ * for more information.
+ */
+struct regulator *devm_of_regulator_get(struct device *dev,
+					const char *id,
+					struct device_node *node)
+{
+	return _devm_regulator_get(dev, id, node, NORMAL_GET);
+}
+EXPORT_SYMBOL_GPL(devm_of_regulator_get);
+
+/**
+ * devm_regulator_get_exclusive - Resource managed regulator_get_exclusive()
+ * @dev: device for regulator "consumer"
+ * @node: device node for which to get the regulator
+ * @id: Supply name or regulator ID.
+ *
+ * Managed regulator_get_exclusive(). Regulators returned from this
+ * function are automatically regulator_put() on driver detach. See
+ * regulator_get_exclusive() for more information.
+ */
+struct regulator *devm_of_regulator_get_exclusive(struct device *dev,
+						 const char *id,
+						 struct device_node *node)
+{
+	return _devm_regulator_get(dev, id, node, EXCLUSIVE_GET);
+}
+EXPORT_SYMBOL_GPL(devm_of_regulator_get_exclusive);
+
+/**
+ * devm_regulator_get_optional - Resource managed regulator_get_optional()
+ * @dev: device for regulator "consumer"
+ * @node: device node for which to get the regulator
+ * @id: Supply name or regulator ID.
+ *
+ * Managed regulator_get_optional(). Regulators returned from this
+ * function are automatically regulator_put() on driver detach. See
+ * regulator_get_optional() for more information.
+ */
+struct regulator *devm_of_regulator_get_optional(struct device *dev,
+						 const char *id,
+						 struct device_node *node)
+{
+	return _devm_regulator_get(dev, id, node, OPTIONAL_GET);
+}
+EXPORT_SYMBOL_GPL(devm_of_regulator_get_optional);
+
 static int devm_regulator_match(struct device *dev, void *res, void *data)
 {
 	struct regulator **r = res;
diff --git a/include/linux/regulator/consumer.h b/include/linux/regulator/consumer.h
index d17e1ff7ad01..1c88ce1aaa3f 100644
--- a/include/linux/regulator/consumer.h
+++ b/include/linux/regulator/consumer.h
@@ -40,6 +40,7 @@
 struct device;
 struct notifier_block;
 struct regmap;
+struct device_node;
 
 /*
  * Regulator operating modes.
@@ -162,14 +163,33 @@ struct regulator *__must_check regulator_get(struct device *dev,
 					     const char *id);
 struct regulator *__must_check devm_regulator_get(struct device *dev,
 					     const char *id);
+struct regulator *__must_check of_regulator_get(struct device *dev,
+						const char *id,
+						struct device_node *node);
+struct regulator *__must_check devm_of_regulator_get(struct device *dev,
+						const char *id,
+						struct device_node *node);
 struct regulator *__must_check regulator_get_exclusive(struct device *dev,
 						       const char *id);
 struct regulator *__must_check devm_regulator_get_exclusive(struct device *dev,
 							const char *id);
+struct regulator *__must_check of_regulator_get_exclusive(struct device *dev,
+						      const char *id,
+						      struct device_node *node);
+struct regulator *__must_check devm_of_regulator_get_exclusive(struct device *dev,
+						      const char *id,
+						      struct device_node *node);
 struct regulator *__must_check regulator_get_optional(struct device *dev,
 						      const char *id);
 struct regulator *__must_check devm_regulator_get_optional(struct device *dev,
 							   const char *id);
+struct regulator *__must_check of_regulator_get_optional(struct device *dev,
+						      const char *id,
+						      struct device_node *node);
+struct regulator *__must_check devm_of_regulator_get_optional(struct device *dev,
+						      const char *id,
+						      struct device_node *node);
+
 void regulator_put(struct regulator *regulator);
 void devm_regulator_put(struct regulator *regulator);
 
-- 
1.9.1

Re: [PATCH 1/2] regulator: core: Add a sanity check on the regulator_ enable/disable functions

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 730 bytes --]

On Fri, Dec 26, 2014 at 06:26:38PM +0100, Gregory CLEMENT wrote:
> These two functions use the pointer passed in parameter without any
> check. By adding a NULL pointer check, it allows using those functions
> from a driver in a more generic way. It is useful especially for the
> disable case if the regulator is optional.

No, especially in the case of regulator_enable() this is deliberate -
we're trying to ensure that if people are using regulators they're being
careful about it, checking error codes and so on.  I'd really want to
see some persuasive use case for this.  What you're saying here sounds
like the consumer shouldn't be treating the regulator as optional at
all but should instead be using a normal regulator.

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 473 bytes --]

Re: [PATCH 2/2] regulator: core: Add the device tree version to the regulator_get family

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 1288 bytes --]

On Fri, Dec 26, 2014 at 06:26:39PM +0100, Gregory CLEMENT wrote:

> This patch adds the device tree version (of_) for each member of the
> regulator_get family: normal, exclusive, optional and all of the
> manageable version.

I'm really not keen on this - this sort of firmware specific interface
is just going to encourage the sort of problematic Linux MFD in DT stuff
people seem so enthusiastic about and needlessly DT specific code.

> The of_regulator_get* functions allow using a device node to get the
> regulator instead using the device object. It is needed for the
> regulator associated to a child node which is not a device, it is the
> case of the SATA ports of an ahci controller for instance.

It would be much better to provide a way of mapping the supplies to the
regulator using the normal APIs, for example by making the child nodes
devices (I'm not seeing anything that says why that's not possible and
it sounds like they're supposed to be physically separate devices which
sound like good candidates for being struct device) or by providing some
other alternative mapping mechanism which isn't DT specific.

Or take another look at what the bindings are doing to avoid the
requirement for this, I'd have expected these new bindings to be part of
the review here...

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 473 bytes --]

Re: [PATCH 1/2] regulator: core: Add a sanity check on the regulator_ enable/disable functions

[Gregory CLEMENT]

Hi Mark,

On 29/12/2014 16:40, Mark Brown wrote:
> On Fri, Dec 26, 2014 at 06:26:38PM +0100, Gregory CLEMENT wrote:
>> These two functions use the pointer passed in parameter without any
>> check. By adding a NULL pointer check, it allows using those functions
>> from a driver in a more generic way. It is useful especially for the
>> disable case if the regulator is optional.
> 
> No, especially in the case of regulator_enable() this is deliberate -
> we're trying to ensure that if people are using regulators they're being
> careful about it, checking error codes and so on.  I'd really want to

OK so at least we should check that the pointer is not NULL before using it
and inform the user of it by using a WARNING() or even a BUG() instead of
just let the kernel crash latter.

> see some persuasive use case for this.  What you're saying here sounds
> like the consumer shouldn't be treating the regulator as optional at
> all but should instead be using a normal regulator.
> 

Being able to deal with NULL pointer in the disable function is convenient
and is done in other similar subsystems such as phy or clk for example. Instead
of having a check on the NULL pointer in each driver, it seems more logical to
do it directly in the disable function.

Thanks for you review,

Gregory


-- 
Gregory Clement, Free Electrons
Kernel, drivers, real-time and embedded Linux
development, consulting, training and support.
http://free-electrons.com

Re: [PATCH 1/2] regulator: core: Add a sanity check on the regulator_ enable/disable functions

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 1860 bytes --]

On Tue, Jan 06, 2015 at 12:36:02PM +0100, Gregory CLEMENT wrote:
> Hi Mark,
> 
> On 29/12/2014 16:40, Mark Brown wrote:
> > On Fri, Dec 26, 2014 at 06:26:38PM +0100, Gregory CLEMENT wrote:

> > No, especially in the case of regulator_enable() this is deliberate -
> > we're trying to ensure that if people are using regulators they're being
> > careful about it, checking error codes and so on.  I'd really want to

> OK so at least we should check that the pointer is not NULL before using it
> and inform the user of it by using a WARNING() or even a BUG() instead of
> just let the kernel crash latter.

Just crashing on the NULL is just about as good in terms of
discoverabilty and any consumer that is assuming NULL is not a valid
regulator is buggy in any case, any non-error pointer could be a valid
regulator as far as users are concerned.  

> > see some persuasive use case for this.  What you're saying here sounds
> > like the consumer shouldn't be treating the regulator as optional at
> > all but should instead be using a normal regulator.

> Being able to deal with NULL pointer in the disable function is convenient
> and is done in other similar subsystems such as phy or clk for example. Instead
> of having a check on the NULL pointer in each driver, it seems more logical to
> do it directly in the disable function.

This really only applies if it's likely that some thing that always gets
used if it's there might be missing which isn't the case for regulators,
it's not at all common to have power supplies that might be missing and
if they are missing NULL isn't a good way to track them.

If you're having problems with this and need workarounds in the core to
make your driver code look OK that sounds like things are working since
it sounds like the driver code is probably abusing the API here.

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 473 bytes --]

Re: [PATCH 1/2] regulator: core: Add a sanity check on the regulator_ enable/disable functions

[Gregory CLEMENT]

Hi Mark,

On 06/01/2015 13:00, Mark Brown wrote:
> On Tue, Jan 06, 2015 at 12:36:02PM +0100, Gregory CLEMENT wrote:
>> Hi Mark,
>>
>> On 29/12/2014 16:40, Mark Brown wrote:
>>> On Fri, Dec 26, 2014 at 06:26:38PM +0100, Gregory CLEMENT wrote:
> 
>>> No, especially in the case of regulator_enable() this is deliberate -
>>> we're trying to ensure that if people are using regulators they're being
>>> careful about it, checking error codes and so on.  I'd really want to
> 
>> OK so at least we should check that the pointer is not NULL before using it
>> and inform the user of it by using a WARNING() or even a BUG() instead of
>> just let the kernel crash latter.
> 
> Just crashing on the NULL is just about as good in terms of
> discoverabilty and any consumer that is assuming NULL is not a valid
> regulator is buggy in any case, any non-error pointer could be a valid
> regulator as far as users are concerned.  
> 
>>> see some persuasive use case for this.  What you're saying here sounds
>>> like the consumer shouldn't be treating the regulator as optional at
>>> all but should instead be using a normal regulator.
> 
>> Being able to deal with NULL pointer in the disable function is convenient
>> and is done in other similar subsystems such as phy or clk for example. Instead
>> of having a check on the NULL pointer in each driver, it seems more logical to
>> do it directly in the disable function.
> 
> This really only applies if it's likely that some thing that always gets
> used if it's there might be missing which isn't the case for regulators,
> it's not at all common to have power supplies that might be missing and

Well the pattern the following pattern is very common in the drivers using
the regulator:

if (!IS_ERR(regulator_pointer)
	regulator_disable(regulator_pointer);


So for me it was a good hint that we can factorize it.

> if they are missing NULL isn't a good way to track them.
> 
> If you're having problems with this and need workarounds in the core to
> make your driver code look OK that sounds like things are working since
> it sounds like the driver code is probably abusing the API here.

I don't _need_ it at all. It was just an improvement but if you don't want it,
I am fine with it.


Thanks,

Gregory




-- 
Gregory Clement, Free Electrons
Kernel, drivers, real-time and embedded Linux
development, consulting, training and support.
http://free-electrons.com

Re: [PATCH 1/2] regulator: core: Add a sanity check on the regulator_ enable/disable functions

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 593 bytes --]

On Tue, Jan 06, 2015 at 01:26:28PM +0100, Gregory CLEMENT wrote:
> On 06/01/2015 13:00, Mark Brown wrote:

> > This really only applies if it's likely that some thing that always gets
> > used if it's there might be missing which isn't the case for regulators,
> > it's not at all common to have power supplies that might be missing and

> Well the pattern the following pattern is very common in the drivers using
> the regulator:

> if (!IS_ERR(regulator_pointer)
> 	regulator_disable(regulator_pointer);

> So for me it was a good hint that we can factorize it.

It is?  It shouldn't be...

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 473 bytes --]