Re: [RFC PATCH 01/15] power: add power sequencer subsystem

On Mon 16 Aug 17:54 PDT 2021, Dmitry Baryshkov wrote:

Basing on MMC's pwrseq support code, add separate power sequencer
subsystem. It will be used by other drivers to handle device power up
requirements.

Some more background to why we need a pwrseq framework wouldn't hurt.

[..]

The typical format is:

// SPDX using C++ style comment
/*
 * Copyright stuff using C style comment
 */

+
+#include <linux/device.h>
+#include <linux/idr.h>
+#include <linux/module.h>
+#include <linux/mutex.h>
+#include <linux/pm_runtime.h>
+#include <linux/pwrseq/consumer.h>
+#include <linux/pwrseq/driver.h>
+#include <linux/slab.h>
+
+#define	to_pwrseq(a)	(container_of((a), struct pwrseq, dev))

No need for the extra parenthesis around container_of()

+
+static DEFINE_IDA(pwrseq_ida);
+static DEFINE_MUTEX(pwrseq_provider_mutex);
+static LIST_HEAD(pwrseq_provider_list);
+
+struct pwrseq_provider {
+	struct device		*dev;
+	struct module		*owner;
+	struct list_head	list;
+	void			*data;
+	struct pwrseq * (*of_xlate)(void *data, struct of_phandle_args *args);
+};
+
+void pwrseq_put(struct device *dev, struct pwrseq *pwrseq)
+{
+	device_link_remove(dev, &pwrseq->dev);
+
+	module_put(pwrseq->owner);
+	put_device(&pwrseq->dev);
+}
+EXPORT_SYMBOL_GPL(pwrseq_put);
+
+static struct pwrseq_provider *of_pwrseq_provider_lookup(struct device_node *node)
+{
+	struct pwrseq_provider *pwrseq_provider;
+
+	list_for_each_entry(pwrseq_provider, &pwrseq_provider_list, list) {
+		if (pwrseq_provider->dev->of_node == node)
+			return pwrseq_provider;
+	}
+
+	return ERR_PTR(-EPROBE_DEFER);
+}
+
+static struct pwrseq *_of_pwrseq_get(struct device *dev, const char *id)
+{
+	struct pwrseq_provider *pwrseq_provider;
+	struct pwrseq *pwrseq;
+	struct of_phandle_args args;
+	char prop_name[64]; /* 64 is max size of property name */
+	int ret;
+
+	snprintf(prop_name, 64, "%s-pwrseq", id);

sizeof(prop_name), to avoid giving others a chance to "fix" it later?

+	ret = of_parse_phandle_with_args(dev->of_node, prop_name, "#pwrseq-cells", 0, &args);
+	if (ret) {
+		struct device_node *dn;
+
+		/*
+		 * Parsing failed. Try locating old bindings for mmc-pwrseq,
+		 * which did not use #pwrseq-cells.
+		 */
+		if (strcmp(id, "mmc"))
+			return ERR_PTR(-ENODEV);
+
+		dn = of_parse_phandle(dev->of_node, prop_name, 0);
+		if (!dn)
+			return ERR_PTR(-ENODEV);
+
+		args.np = dn;
+		args.args_count = 0;
+	}
+
+	mutex_lock(&pwrseq_provider_mutex);
+	pwrseq_provider = of_pwrseq_provider_lookup(args.np);
+	if (IS_ERR(pwrseq_provider) || !try_module_get(pwrseq_provider->owner)) {
+		pwrseq = ERR_PTR(-EPROBE_DEFER);
+		goto out_unlock;
+	}
+
+	if (!of_device_is_available(args.np)) {
+		dev_warn(pwrseq_provider->dev, "Requested pwrseq is disabled\n");
+		pwrseq = ERR_PTR(-ENODEV);
+		goto out_put_module;
+	}
+
+	pwrseq = pwrseq_provider->of_xlate(pwrseq_provider->data, &args);
+
+out_put_module:
+	module_put(pwrseq_provider->owner);
+
+out_unlock:
+	mutex_unlock(&pwrseq_provider_mutex);
+	of_node_put(args.np);
+
+	return pwrseq;
+}
+

[..]

+int pwrseq_pre_power_on(struct pwrseq *pwrseq)
+{
+	if (pwrseq && pwrseq->ops->pre_power_on)
+		return pwrseq->ops->pre_power_on(pwrseq);
+
+	return 0;
+}
+EXPORT_SYMBOL_GPL(pwrseq_pre_power_on);
+
+int pwrseq_power_on(struct pwrseq *pwrseq)

Wouldn't it make sense to refcount the power on/off operations and at
least warn about unbalanced disables?

My concern is related to the qca-driver's reliance on the regulator
framework to refcount the on/off of the shared resources and additional
power_off from either the WiFi or BT client would result in the other
client getting its power disabled unexpectedly - which might be
annoying to debug.

+{
+	if (pwrseq && pwrseq->ops->power_on)
+		return pwrseq->ops->power_on(pwrseq);
+
+	return 0;
+}
+EXPORT_SYMBOL_GPL(pwrseq_power_on);
+
+void pwrseq_power_off(struct pwrseq *pwrseq)
+{
+	if (pwrseq && pwrseq->ops->power_off)
+		pwrseq->ops->power_off(pwrseq);
+}
+EXPORT_SYMBOL_GPL(pwrseq_power_off);
+
+void pwrseq_reset(struct pwrseq *pwrseq)
+{
+	if (pwrseq && pwrseq->ops->reset)
+		pwrseq->ops->reset(pwrseq);
+}
+EXPORT_SYMBOL_GPL(pwrseq_reset);
+

Regards,
Bjorn