| Message ID | 1418226513-14105-5-git-send-email-a.hajda@samsung.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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.
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
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.
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.
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
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(®ulator_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;
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(+)