diff mbox

[RFC,04/15] regulator: add restrack support

Message ID 1418226513-14105-5-git-send-email-a.hajda@samsung.com (mailing list archive)
State New, archived
Headers show

Commit Message

Andrzej Hajda Dec. 10, 2014, 3:48 p.m. UTC
Regulators supports various methods of lookup.
The patch adds restrack support only to DT based regulators.

Signed-off-by: Andrzej Hajda <a.hajda@samsung.com>
---
 drivers/regulator/core.c           | 77 ++++++++++++++++++++++++++++++++++++++
 include/linux/regulator/consumer.h | 10 +++++
 include/linux/restrack.h           |  1 +
 3 files changed, 88 insertions(+)

Comments

Mark Brown Dec. 10, 2014, 4:07 p.m. UTC | #1
On Wed, Dec 10, 2014 at 04:48:22PM +0100, Andrzej Hajda wrote:
> Regulators supports various methods of lookup.
> The patch adds restrack support only to DT based regulators.

Why, what does this mean and how might one use it?  I've not looked at
the code since I don't know what it's supposed to accomplish...

One very high level thing is that anything that only works for DT only
seems to be a non-starter, the API should be hiding details of the
firmware interface.
Andrzej Hajda Dec. 11, 2014, 10:49 a.m. UTC | #2
Hi Mark,

On 12/10/2014 05:07 PM, Mark Brown wrote:
> On Wed, Dec 10, 2014 at 04:48:22PM +0100, Andrzej Hajda wrote:
>> Regulators supports various methods of lookup.
>> The patch adds restrack support only to DT based regulators.
> Why, what does this mean and how might one use it?  I've not looked at
> the code since I don't know what it's supposed to accomplish...

Looking at this patch makes no sense without looking at cover letter
or at the patch adding restrack framework. In short adding restrack support
to regulators will allow to:
- proper handle regulator provider unbind/re-bind, currently it results
in oopses, crashes and hangs,
- avoid late probe due to deferred probing, currently if probe is
deferred, re-probe occurs in late initcall,
- track appearance of resources which can alter behavior of the driver
if present but they are not required,
  I am not sure if there are use cases for it in case of regulators, but
other resources have such use cases,
- as a bonus we can have simpler allocation of various resources, please
look at cover letter for example.

>
> One very high level thing is that anything that only works for DT only
> seems to be a non-starter, the API should be hiding details of the
> firmware interface.

I agree, but as this is RFC, not everything is finished. It seems I have
forgotten to mention it clearly in cover
letter.
Anyway it seems I should adjust patchset and move matching code from
restrack/track core to specific frameworks.
This way any current or future lookup method should be supported.

But the main purpose of this patchset is to get opinions, if the main
ideas are OK. And if the patchset can be
eventually accepted.

Regards
Andrzej

--
To unsubscribe from this list: send the line "unsubscribe linux-samsung-soc" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Mark Brown Dec. 11, 2014, 12:58 p.m. UTC | #3
On Thu, Dec 11, 2014 at 11:49:54AM +0100, Andrzej Hajda wrote:
> On 12/10/2014 05:07 PM, Mark Brown wrote:
> > On Wed, Dec 10, 2014 at 04:48:22PM +0100, Andrzej Hajda wrote:

> >> Regulators supports various methods of lookup.
> >> The patch adds restrack support only to DT based regulators.

> > Why, what does this mean and how might one use it?  I've not looked at
> > the code since I don't know what it's supposed to accomplish...

> Looking at this patch makes no sense without looking at cover letter
> or at the patch adding restrack framework. In short adding restrack support
> to regulators will allow to:

I did look at the cover letter and glance at the first couple of patches
but I still can't tell what this is really intended to do.  There's a
lot of implementation detail but not a lot of high level overview so
it's not very clear to me how exactly this solves problems or how it is
different to the component framework, it sounds very similar.

I'd expect someone reading the change in the regulator API to have at
least some idea how this fits in with the rest of the API and how to use
it, and probably more importantly I'd expect to be able to understand
why this is DT only.
Russell King - ARM Linux Dec. 11, 2014, 1:43 p.m. UTC | #4
On Thu, Dec 11, 2014 at 12:58:37PM +0000, Mark Brown wrote:
> I'd expect someone reading the change in the regulator API to have at
> least some idea how this fits in with the rest of the API and how to use
> it, and probably more importantly I'd expect to be able to understand
> why this is DT only.

Yep.

This is a repetitive problem, and I fully agree with your concern about
stuff which is supposed to be arch-independent being designed with only
DT in mind.

New core kernel features should *not* be designed with only DT in mind -
DT is not the only firmware description language which the kernel
supports.  Folk need to understand that if they design a new arch
independent kernel feature where the sole use case is with DT, that new
feature is probably going to get rejected, especially when it's
something as generic as resource tracking.

The world is not DT only.
Andrzej Hajda Dec. 12, 2014, 8:21 a.m. UTC | #5
On 12/11/2014 02:43 PM, Russell King - ARM Linux wrote:
> On Thu, Dec 11, 2014 at 12:58:37PM +0000, Mark Brown wrote:
>> I'd expect someone reading the change in the regulator API to have at
>> least some idea how this fits in with the rest of the API and how to use
>> it, and probably more importantly I'd expect to be able to understand
>> why this is DT only.
> 
> Yep.
> 
> This is a repetitive problem, and I fully agree with your concern about
> stuff which is supposed to be arch-independent being designed with only
> DT in mind.
> 
> New core kernel features should *not* be designed with only DT in mind -
> DT is not the only firmware description language which the kernel
> supports.  Folk need to understand that if they design a new arch
> independent kernel feature where the sole use case is with DT, that new
> feature is probably going to get rejected, especially when it's
> something as generic as resource tracking.
> 
> The world is not DT only.
> 

OK. I will post next version of patchset with resource/provider lookup
left to frameworks (regulators, clock, etc), this way it will be fully
firmware agnostic. I will add also better description of the framework.

Regards
Andrzej

--
To unsubscribe from this list: send the line "unsubscribe linux-samsung-soc" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Andrzej Hajda Dec. 12, 2014, 8:22 a.m. UTC | #6
On 12/11/2014 02:43 PM, Russell King - ARM Linux wrote:
> On Thu, Dec 11, 2014 at 12:58:37PM +0000, Mark Brown wrote:
>> I'd expect someone reading the change in the regulator API to have at
>> least some idea how this fits in with the rest of the API and how to use
>> it, and probably more importantly I'd expect to be able to understand
>> why this is DT only.
> 
> Yep.
> 
> This is a repetitive problem, and I fully agree with your concern about
> stuff which is supposed to be arch-independent being designed with only
> DT in mind.
> 
> New core kernel features should *not* be designed with only DT in mind -
> DT is not the only firmware description language which the kernel
> supports.  Folk need to understand that if they design a new arch
> independent kernel feature where the sole use case is with DT, that new
> feature is probably going to get rejected, especially when it's
> something as generic as resource tracking.
> 
> The world is not DT only.
> 

OK. I will post next version of patchset with resource/provider lookup
left to frameworks (regulators, clock, etc), this way it will be fully
firmware agnostic. I will add also better description of the framework.

Regards
Andrzej
--
To unsubscribe from this list: send the line "unsubscribe linux-samsung-soc" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
diff mbox

Patch

diff --git a/drivers/regulator/core.c b/drivers/regulator/core.c
index cd87c0c..5641e85 100644
--- a/drivers/regulator/core.c
+++ b/drivers/regulator/core.c
@@ -32,6 +32,7 @@ 
 #include <linux/regulator/driver.h>
 #include <linux/regulator/machine.h>
 #include <linux/module.h>
+#include <linux/restrack.h>
 
 #define CREATE_TRACE_POINTS
 #include <trace/events/regulator.h>
@@ -3733,6 +3734,10 @@  add_dev:
 	rdev_init_debugfs(rdev);
 out:
 	mutex_unlock(&regulator_list_mutex);
+
+	if (rdev->dev.of_node)
+		restrack_up(RESTRACK_TYPE_REGULATOR, rdev->dev.of_node, rdev);
+
 	return rdev;
 
 unset_supplies:
@@ -3767,6 +3772,9 @@  void regulator_unregister(struct regulator_dev *rdev)
 	if (rdev == NULL)
 		return;
 
+	if (rdev->dev.of_node)
+		restrack_down(RESTRACK_TYPE_REGULATOR, rdev->dev.of_node, rdev);
+
 	if (rdev->supply) {
 		while (rdev->use_count--)
 			regulator_disable(rdev->supply);
@@ -3971,6 +3979,75 @@  static const struct file_operations supply_map_fops = {
 #endif
 };
 
+struct regulator_restrack_desc {
+	struct regulator **ptr;
+	const char *name;
+	struct restrack_desc desc;
+};
+
+static int regulator_restrack_init(struct device *dev,
+		struct restrack_desc *desc)
+{
+	struct regulator_restrack_desc *rd = restrack_desc_to_rd(rd, desc);
+
+	desc->if_id = of_get_regulator(dev, rd->name);
+	return PTR_ERR_OR_ZERO(desc->if_id);
+}
+
+static void regulator_restrack_destroy(struct device *dev,
+		struct restrack_desc *desc)
+{
+	struct regulator_restrack_desc *rd = restrack_desc_to_rd(rd, desc);
+
+	of_node_put(desc->if_id);
+	kfree(rd);
+}
+
+static int regulator_restrack_ifup(struct device *dev,
+		struct restrack_desc *desc, void *data)
+{
+	struct regulator_restrack_desc *rd = restrack_desc_to_rd(rd, desc);
+
+	*rd->ptr = regulator_get(dev, rd->name);
+	return PTR_ERR_OR_ZERO(*rd->ptr);
+}
+
+static void regulator_restrack_ifdown(struct device *dev,
+		struct restrack_desc *desc, void *data)
+{
+	struct regulator_restrack_desc *rd = restrack_desc_to_rd(rd, desc);
+
+	regulator_put(*rd->ptr);
+	*rd->ptr = ERR_PTR(-EPROBE_DEFER);
+}
+
+static const struct restrack_ops regulator_restrack_ops = {
+	.if_type = RESTRACK_TYPE_REGULATOR,
+	.init = regulator_restrack_init,
+	.destroy = regulator_restrack_destroy,
+	.if_up = regulator_restrack_ifup,
+	.if_down = regulator_restrack_ifdown,
+};
+
+/**
+ * regulator_restrack_desc - regulator resource descriptor allocator
+ * @regulator: pointer to variable which will be set to regulator handle
+ * @name: name of regulator
+ *
+ * The function creates resource description for regulator, which shall be used
+ * by *restrack_register functions.
+ */
+struct restrack_desc *regulator_restrack_desc(struct regulator **regulator,
+		const char *name)
+{
+	struct regulator_restrack_desc *rd;
+
+	RESTRACK_DESC_ALLOC(rd, regulator_restrack_ops, regulator, name);
+
+	return rd ? &rd->desc : ERR_PTR(-ENOMEM);
+}
+EXPORT_SYMBOL_GPL(regulator_restrack_desc);
+
 static int __init regulator_init(void)
 {
 	int ret;
diff --git a/include/linux/regulator/consumer.h b/include/linux/regulator/consumer.h
index f540b14..69e71ebb 100644
--- a/include/linux/regulator/consumer.h
+++ b/include/linux/regulator/consumer.h
@@ -551,4 +551,14 @@  static inline int regulator_is_supported_voltage_tol(struct regulator *regulator
 					      target_uV + tol_uV);
 }
 
+struct restrack_desc;
+struct restrack_desc *regulator_restrack_desc(struct regulator **regulator,
+		const char *supply);
+
+static inline struct restrack_desc *
+regulator_bulk_restrack_desc(struct regulator_bulk_data *data)
+{
+	return regulator_restrack_desc(&data->consumer, data->supply);
+}
+
 #endif
diff --git a/include/linux/restrack.h b/include/linux/restrack.h
index af5b617..4e4eec6 100644
--- a/include/linux/restrack.h
+++ b/include/linux/restrack.h
@@ -4,6 +4,7 @@ 
 #include <linux/track.h>
 
 #define RESTRACK_TYPE_DRM_PANEL 1
+#define RESTRACK_TYPE_REGULATOR 2
 
 struct device;
 struct restrack_ctx;