| Message ID | 20210829131305.534417-2-dmitry.baryshkov@linaro.org (mailing list archive) |
|---|---|
| State | RFC |
| Delegated to: | Luiz Von Dentz |
| Headers | show |
| Series | create power sequencing subsystem | expand |
This is automated email and please do not reply to this email!
Dear submitter,
Thank you for submitting the patches to the linux bluetooth mailing list.
This is a CI test results with your patch series:
PW Link:https://patchwork.kernel.org/project/bluetooth/list/?series=538865
---Test result---
Test Summary:
CheckPatch FAIL 10.08 seconds
GitLint FAIL 1.29 seconds
BuildKernel PASS 513.89 seconds
TestRunner: Setup PASS 340.37 seconds
TestRunner: l2cap-tester PASS 2.58 seconds
TestRunner: bnep-tester PASS 1.92 seconds
TestRunner: mgmt-tester PASS 30.13 seconds
TestRunner: rfcomm-tester PASS 2.11 seconds
TestRunner: sco-tester PASS 2.03 seconds
TestRunner: smp-tester FAIL 2.12 seconds
TestRunner: userchan-tester PASS 1.94 seconds
Details
##############################
Test: CheckPatch - FAIL - 10.08 seconds
Run checkpatch.pl script with rule in .checkpatch.conf
power: add power sequencer subsystem
WARNING: added, moved or deleted file(s), does MAINTAINERS need updating?
#31:
new file mode 100644
WARNING: please write a paragraph that describes the config symbol fully
#37: FILE: drivers/power/pwrseq/Kconfig:2:
+menuconfig PWRSEQ
ERROR: "foo * bar" should be "foo *bar"
#169: FILE: drivers/power/pwrseq/core.c:105:
+struct pwrseq * __pwrseq_get(struct device *dev, const char *id, bool optional)
ERROR: "foo * bar" should be "foo *bar"
#192: FILE: drivers/power/pwrseq/core.c:128:
+struct pwrseq * pwrseq_get(struct device *dev, const char *id)
ERROR: "foo * bar" should be "foo *bar"
#205: FILE: drivers/power/pwrseq/core.c:141:
+struct pwrseq * devm_pwrseq_get(struct device *dev, const char *id)
ERROR: "foo * bar" should be "foo *bar"
#225: FILE: drivers/power/pwrseq/core.c:161:
+struct pwrseq * pwrseq_get_optional(struct device *dev, const char *id)
ERROR: "foo * bar" should be "foo *bar"
#231: FILE: drivers/power/pwrseq/core.c:167:
+struct pwrseq * devm_pwrseq_get_optional(struct device *dev, const char *id)
WARNING: line length of 114 exceeds 100 columns
#297: FILE: drivers/power/pwrseq/core.c:233:
+struct pwrseq *__pwrseq_create(struct device *dev, struct module *owner, const struct pwrseq_ops *ops, void *data)
WARNING: line length of 119 exceeds 100 columns
#367: FILE: drivers/power/pwrseq/core.c:303:
+struct pwrseq *__devm_pwrseq_create(struct device *dev, struct module *owner, const struct pwrseq_ops *ops, void *data)
WARNING: ENOSYS means 'invalid syscall nr' and nothing else
#514: FILE: include/linux/pwrseq/consumer.h:32:
+ return ERR_PTR(-ENOSYS);
WARNING: ENOSYS means 'invalid syscall nr' and nothing else
#520: FILE: include/linux/pwrseq/consumer.h:38:
+ return ERR_PTR(-ENOSYS);
WARNING: ENOSYS means 'invalid syscall nr' and nothing else
#541: FILE: include/linux/pwrseq/consumer.h:59:
+ return -ENOSYS;
WARNING: ENOSYS means 'invalid syscall nr' and nothing else
#546: FILE: include/linux/pwrseq/consumer.h:64:
+ return -ENOSYS;
WARNING: line length of 115 exceeds 100 columns
#605: FILE: include/linux/pwrseq/driver.h:29:
+struct pwrseq *__pwrseq_create(struct device *dev, struct module *owner, const struct pwrseq_ops *ops, void *data);
WARNING: line length of 120 exceeds 100 columns
#606: FILE: include/linux/pwrseq/driver.h:30:
+struct pwrseq *__devm_pwrseq_create(struct device *dev, struct module *owner, const struct pwrseq_ops *ops, void *data);
total: 5 errors, 10 warnings, 600 lines checked
NOTE: For some of the reported defects, checkpatch may be able to
mechanically convert to the typical style using --fix or --fix-inplace.
"[PATCH] power: add power sequencer subsystem" has style problems, please review.
NOTE: If any of the errors are false positives, please report
them to the maintainer, see CHECKPATCH in MAINTAINERS.
pwrseq: port MMC's pwrseq drivers to new pwrseq subsystem
WARNING: added, moved or deleted file(s), does MAINTAINERS need updating?
#12:
rename from Documentation/devicetree/bindings/mmc/mmc-pwrseq-emmc.yaml
WARNING: DT binding docs and includes should be a separate patch. See: Documentation/devicetree/bindings/submitting-patches.rst
WARNING: ENOSYS means 'invalid syscall nr' and nothing else
#908: FILE: drivers/power/pwrseq/pwrseq_simple.c:133:
+ PTR_ERR(pwrseq_simple->reset_gpios) != -ENOSYS) {
total: 0 errors, 3 warnings, 467 lines checked
NOTE: For some of the reported defects, checkpatch may be able to
mechanically convert to the typical style using --fix or --fix-inplace.
"[PATCH] pwrseq: port MMC's pwrseq drivers to new pwrseq subsystem" has style problems, please review.
NOTE: If any of the errors are false positives, please report
them to the maintainer, see CHECKPATCH in MAINTAINERS.
mmc: core: switch to new pwrseq subsystem
WARNING: added, moved or deleted file(s), does MAINTAINERS need updating?
#126:
deleted file mode 100644
total: 0 errors, 1 warnings, 109 lines checked
NOTE: For some of the reported defects, checkpatch may be able to
mechanically convert to the typical style using --fix or --fix-inplace.
"[PATCH] mmc: core: switch to new pwrseq subsystem" has style problems, please review.
NOTE: If any of the errors are false positives, please report
them to the maintainer, see CHECKPATCH in MAINTAINERS.
pwrseq: add support for QCA BT+WiFi power sequencer
WARNING: added, moved or deleted file(s), does MAINTAINERS need updating?
#54:
new file mode 100644
WARNING: Missing a blank line after declarations
#168: FILE: drivers/power/pwrseq/pwrseq_qca.c:110:
+ bool sw_ctrl_state = gpiod_get_value_cansleep(qca_one->common->sw_ctrl);
+ dev_dbg(&pwrseq->dev, "SW_CTRL is %d", sw_ctrl_state);
WARNING: Missing a blank line after declarations
#197: FILE: drivers/power/pwrseq/pwrseq_qca.c:139:
+ bool sw_ctrl_state = gpiod_get_value_cansleep(qca_one->common->sw_ctrl);
+ dev_dbg(&pwrseq->dev, "SW_CTRL is %d", sw_ctrl_state);
WARNING: line length of 107 exceeds 100 columns
#282: FILE: drivers/power/pwrseq/pwrseq_qca.c:224:
+ pwrseq_qca = devm_kzalloc(dev, struct_size(pwrseq_qca, common.vregs, data->num_vregs), GFP_KERNEL);
WARNING: line length of 106 exceeds 100 columns
#299: FILE: drivers/power/pwrseq/pwrseq_qca.c:241:
+ return dev_err_probe(dev, PTR_ERR(gpiod), "failed to acquire WIFI enable GPIO\n");
WARNING: line length of 104 exceeds 100 columns
#307: FILE: drivers/power/pwrseq/pwrseq_qca.c:249:
+ return dev_err_probe(dev, PTR_ERR(gpiod), "failed to acquire BT enable GPIO\n");
WARNING: line length of 106 exceeds 100 columns
#314: FILE: drivers/power/pwrseq/pwrseq_qca.c:256:
+ /* If we have no control over device's enablement, make sure that sleep clock is always running */
WARNING: line length of 102 exceeds 100 columns
#327: FILE: drivers/power/pwrseq/pwrseq_qca.c:269:
+ ret = devm_add_action_or_reset(dev, pwrseq_qca_unprepare_susclk, &pwrseq_qca->common);
WARNING: DT compatible string "qcom,qca6174-pwrseq" appears un-documented -- check ./Documentation/devicetree/bindings/
#409: FILE: drivers/power/pwrseq/pwrseq_qca.c:351:
+ { .compatible = "qcom,qca6174-pwrseq", },
WARNING: DT compatible string "qcom,qca6390-pwrseq" appears un-documented -- check ./Documentation/devicetree/bindings/
#410: FILE: drivers/power/pwrseq/pwrseq_qca.c:352:
+ { .compatible = "qcom,qca6390-pwrseq", .data = &qca_soc_data_qca6390 },
WARNING: DT compatible string "qcom,qca9377-pwrseq" appears un-documented -- check ./Documentation/devicetree/bindings/
#411: FILE: drivers/power/pwrseq/pwrseq_qca.c:353:
+ { .compatible = "qcom,qca9377-pwrseq" },
WARNING: DT compatible string "qcom,wcn3990-pwrseq" appears un-documented -- check ./Documentation/devicetree/bindings/
#412: FILE: drivers/power/pwrseq/pwrseq_qca.c:354:
+ { .compatible = "qcom,wcn3990-pwrseq", .data = &qca_soc_data_wcn3990 },
WARNING: DT compatible string "qcom,wcn3991-pwrseq" appears un-documented -- check ./Documentation/devicetree/bindings/
#413: FILE: drivers/power/pwrseq/pwrseq_qca.c:355:
+ { .compatible = "qcom,wcn3991-pwrseq", .data = &qca_soc_data_wcn3990 },
WARNING: DT compatible string "qcom,wcn3998-pwrseq" appears un-documented -- check ./Documentation/devicetree/bindings/
#414: FILE: drivers/power/pwrseq/pwrseq_qca.c:356:
+ { .compatible = "qcom,wcn3998-pwrseq", .data = &qca_soc_data_wcn3998 },
WARNING: DT compatible string "qcom,wcn6750-pwrseq" appears un-documented -- check ./Documentation/devicetree/bindings/
#415: FILE: drivers/power/pwrseq/pwrseq_qca.c:357:
+ { .compatible = "qcom,wcn6750-pwrseq", .data = &qca_soc_data_wcn6750 },
total: 0 errors, 15 warnings, 397 lines checked
NOTE: For some of the reported defects, checkpatch may be able to
mechanically convert to the typical style using --fix or --fix-inplace.
"[PATCH] pwrseq: add support for QCA BT+WiFi power sequencer" has style problems, please review.
NOTE: If any of the errors are false positives, please report
them to the maintainer, see CHECKPATCH in MAINTAINERS.
pwrseq: add fallback support
WARNING: added, moved or deleted file(s), does MAINTAINERS need updating?
#48:
new file mode 100644
total: 0 errors, 1 warnings, 134 lines checked
NOTE: For some of the reported defects, checkpatch may be able to
mechanically convert to the typical style using --fix or --fix-inplace.
"[PATCH] pwrseq: add fallback support" has style problems, please review.
NOTE: If any of the errors are false positives, please report
them to the maintainer, see CHECKPATCH in MAINTAINERS.
pwrseq: pwrseq_qca: implement fallback support
ERROR: "foo * bar" should be "foo *bar"
#66: FILE: drivers/power/pwrseq/pwrseq_qca.c:403:
+static struct pwrseq * pwrseq_qca_fallback_get(struct device *dev)
WARNING: line length of 103 exceeds 100 columns
#82: FILE: drivers/power/pwrseq/pwrseq_qca.c:419:
+ fallback = devm_kzalloc(dev, struct_size(fallback, common.vregs, data->num_vregs), GFP_KERNEL);
WARNING: line length of 110 exceeds 100 columns
#95: FILE: drivers/power/pwrseq/pwrseq_qca.c:432:
+ return ERR_PTR(dev_err_probe(dev, PTR_ERR(gpiod), "failed to acquire enable GPIO\n"));
WARNING: line length of 106 exceeds 100 columns
#99: FILE: drivers/power/pwrseq/pwrseq_qca.c:436:
+ /* If we have no control over device's enablement, make sure that sleep clock is always running */
ERROR: "foo * bar" should be "foo *bar"
#119: FILE: drivers/power/pwrseq/pwrseq_qca.c:456:
+static struct pwrseq * pwrseq_qca_fallback_get_bt(struct device *dev, const char *id)
ERROR: "foo * bar" should be "foo *bar"
#127: FILE: drivers/power/pwrseq/pwrseq_qca.c:464:
+static struct pwrseq * pwrseq_qca_fallback_get_wifi(struct device *dev, const char *id)
total: 3 errors, 3 warnings, 161 lines checked
NOTE: For some of the reported defects, checkpatch may be able to
mechanically convert to the typical style using --fix or --fix-inplace.
"[PATCH] pwrseq: pwrseq_qca: implement fallback support" has style problems, please review.
NOTE: If any of the errors are false positives, please report
them to the maintainer, see CHECKPATCH in MAINTAINERS.
Bluetooth: hci_qca: switch to using pwrseq
WARNING: Missing commit description - Add an appropriate one
total: 0 errors, 1 warnings, 425 lines checked
NOTE: For some of the reported defects, checkpatch may be able to
mechanically convert to the typical style using --fix or --fix-inplace.
"[PATCH] Bluetooth: hci_qca: switch to using pwrseq" has style problems, please review.
NOTE: If any of the errors are false positives, please report
them to the maintainer, see CHECKPATCH in MAINTAINERS.
arm64: dts: qcom: sdm845-db845c: switch bt+wifi to qca power sequencer
WARNING: DT compatible string "qcom,wcn3990-pwrseq" appears un-documented -- check ./Documentation/devicetree/bindings/
#67: FILE: arch/arm64/boot/dts/qcom/sdm845.dtsi:1055:
+ compatible = "qcom,wcn3990-pwrseq";
total: 0 errors, 1 warnings, 51 lines checked
NOTE: For some of the reported defects, checkpatch may be able to
mechanically convert to the typical style using --fix or --fix-inplace.
"[PATCH] arm64: dts: qcom: sdm845-db845c: switch bt+wifi to qca power" has style problems, please review.
NOTE: If any of the errors are false positives, please report
them to the maintainer, see CHECKPATCH in MAINTAINERS.
arm64: dts: qcom: qrb5165-rb5: add bluetooth support
WARNING: DT compatible string "qcom,qca6390-pwrseq" appears un-documented -- check ./Documentation/devicetree/bindings/
#29: FILE: arch/arm64/boot/dts/qcom/qrb5165-rb5.dts:103:
+ compatible = "qcom,qca6390-pwrseq";
total: 0 errors, 1 warnings, 74 lines checked
NOTE: For some of the reported defects, checkpatch may be able to
mechanically convert to the typical style using --fix or --fix-inplace.
"[PATCH] arm64: dts: qcom: qrb5165-rb5: add bluetooth support" has style problems, please review.
NOTE: If any of the errors are false positives, please report
them to the maintainer, see CHECKPATCH in MAINTAINERS.
WIP: arm64: dts: qcom: qrb5165-rb5: add bus-pwrseq property to pcie0
WARNING: Missing commit description - Add an appropriate one
total: 0 errors, 1 warnings, 7 lines checked
NOTE: For some of the reported defects, checkpatch may be able to
mechanically convert to the typical style using --fix or --fix-inplace.
"[PATCH] WIP: arm64: dts: qcom: qrb5165-rb5: add bus-pwrseq property" has style problems, please review.
NOTE: If any of the errors are false positives, please report
them to the maintainer, see CHECKPATCH in MAINTAINERS.
##############################
Test: GitLint - FAIL - 1.29 seconds
Run gitlint with rule in .gitlint
arm64: dts: qcom: sdm845-db845c: add second channel to qca power sequencer
1: T1 Title exceeds max length (74>72): "arm64: dts: qcom: sdm845-db845c: add second channel to qca power sequencer"
WIP: PCI: qcom: use pwrseq to power up bus devices
1: T5 Title contains the word 'WIP' (case-insensitive): "WIP: PCI: qcom: use pwrseq to power up bus devices"
WIP: arm64: dts: qcom: qrb5165-rb5: add bus-pwrseq property to pcie0
1: T5 Title contains the word 'WIP' (case-insensitive): "WIP: arm64: dts: qcom: qrb5165-rb5: add bus-pwrseq property to pcie0"
##############################
Test: BuildKernel - PASS - 513.89 seconds
Build Kernel with minimal configuration supports Bluetooth
##############################
Test: TestRunner: Setup - PASS - 340.37 seconds
Setup environment for running Test Runner
##############################
Test: TestRunner: l2cap-tester - PASS - 2.58 seconds
Run test-runner with l2cap-tester
Total: 40, Passed: 40 (100.0%), Failed: 0, Not Run: 0
##############################
Test: TestRunner: bnep-tester - PASS - 1.92 seconds
Run test-runner with bnep-tester
Total: 1, Passed: 1 (100.0%), Failed: 0, Not Run: 0
##############################
Test: TestRunner: mgmt-tester - PASS - 30.13 seconds
Run test-runner with mgmt-tester
Total: 452, Passed: 449 (99.3%), Failed: 0, Not Run: 3
##############################
Test: TestRunner: rfcomm-tester - PASS - 2.11 seconds
Run test-runner with rfcomm-tester
Total: 9, Passed: 9 (100.0%), Failed: 0, Not Run: 0
##############################
Test: TestRunner: sco-tester - PASS - 2.03 seconds
Run test-runner with sco-tester
Total: 8, Passed: 8 (100.0%), Failed: 0, Not Run: 0
##############################
Test: TestRunner: smp-tester - FAIL - 2.12 seconds
Run test-runner with smp-tester
Total: 8, Passed: 7 (87.5%), Failed: 1, Not Run: 0
Failed Test Cases
SMP Client - SC Request 2 Failed 0.020 seconds
##############################
Test: TestRunner: userchan-tester - PASS - 1.94 seconds
Run test-runner with userchan-tester
Total: 3, Passed: 3 (100.0%), Failed: 0, Not Run: 0
---
Regards,
Linux Bluetooth
On Sun, 29 Aug 2021 at 15:13, Dmitry Baryshkov <dmitry.baryshkov@linaro.org> 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. This is far too vague. You are suggesting to add a new subsystem, I think that deserves some more explanations as justifications. Additionally, it wouldn't hurt to explain a bit how the actual subsystem is supposed to work, at least from a toplevel point of view. > > Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org> > --- > drivers/power/Kconfig | 1 + > drivers/power/Makefile | 1 + > drivers/power/pwrseq/Kconfig | 11 + > drivers/power/pwrseq/Makefile | 6 + > drivers/power/pwrseq/core.c | 412 ++++++++++++++++++++++++++++++++ > include/linux/pwrseq/consumer.h | 88 +++++++ > include/linux/pwrseq/driver.h | 75 ++++++ > 7 files changed, 594 insertions(+) > create mode 100644 drivers/power/pwrseq/Kconfig > create mode 100644 drivers/power/pwrseq/Makefile > create mode 100644 drivers/power/pwrseq/core.c > create mode 100644 include/linux/pwrseq/consumer.h > create mode 100644 include/linux/pwrseq/driver.h I noticed there is no update of the MAINTAINERS file. We need that to be a part of the $subject patch as well, I think. But, let's discuss that later. > > diff --git a/drivers/power/Kconfig b/drivers/power/Kconfig > index 696bf77a7042..c87cd2240a74 100644 > --- a/drivers/power/Kconfig > +++ b/drivers/power/Kconfig > @@ -1,3 +1,4 @@ > # SPDX-License-Identifier: GPL-2.0-only > +source "drivers/power/pwrseq/Kconfig" > source "drivers/power/reset/Kconfig" > source "drivers/power/supply/Kconfig" > diff --git a/drivers/power/Makefile b/drivers/power/Makefile > index effbf0377f32..1dbce454a8c4 100644 > --- a/drivers/power/Makefile > +++ b/drivers/power/Makefile > @@ -1,3 +1,4 @@ > # SPDX-License-Identifier: GPL-2.0-only > obj-$(CONFIG_POWER_RESET) += reset/ > obj-$(CONFIG_POWER_SUPPLY) += supply/ > +obj-$(CONFIG_PWRSEQ) += pwrseq/ > diff --git a/drivers/power/pwrseq/Kconfig b/drivers/power/pwrseq/Kconfig > new file mode 100644 > index 000000000000..8904ec9ed541 > --- /dev/null > +++ b/drivers/power/pwrseq/Kconfig > @@ -0,0 +1,11 @@ > +# SPDX-License-Identifier: GPL-2.0-only > +menuconfig PWRSEQ > + bool "Power Sequencer drivers" > + help > + Provides support for special power sequencing drivers. This needs more description. The name "power sequencer" isn't entirely self-explanatory, for when this should be used. I am not saying you should invent a new name, rather just extend the description so people get a better idea of what this is supposed to be used for. > + > + Say Y here to enable support for such devices > + > +if PWRSEQ > + > +endif > diff --git a/drivers/power/pwrseq/Makefile b/drivers/power/pwrseq/Makefile > new file mode 100644 > index 000000000000..108429ff6445 > --- /dev/null > +++ b/drivers/power/pwrseq/Makefile > @@ -0,0 +1,6 @@ > +# SPDX-License-Identifier: GPL-2.0 > +# > +# Makefile for power sequencer drivers. > +# > + > +obj-$(CONFIG_PWRSEQ) += core.o > diff --git a/drivers/power/pwrseq/core.c b/drivers/power/pwrseq/core.c > new file mode 100644 > index 000000000000..2e4e9d123e60 > --- /dev/null > +++ b/drivers/power/pwrseq/core.c > @@ -0,0 +1,412 @@ > +// SPDX-License-Identifier: GPL-2.0-only > +/* > + * Copyright 2021 (c) Linaro Ltd. > + * Author: Dmitry Baryshkov <dmitry.baryshkov@linaro.org> > + * > + * Based on phy-core.c: > + * Copyright (C) 2013 Texas Instruments Incorporated - http://www.ti.com > + */ > + > +#include <linux/device.h> > +#include <linux/idr.h> > +#include <linux/module.h> > +#include <linux/mutex.h> > +#include <linux/of.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)) > + > +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); device_links - why do we need these at this initial step? Please drop them so we can start with a simple implementation - and then possibly extend it. > + > + 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); > + ret = of_parse_phandle_with_args(dev->of_node, prop_name, "#pwrseq-cells", 0, &args); This means that you are parsing a new DT binding/property. Please fold in a DT binding patch, preceding $subject patch, so that new binding that it can be discussed as well. > + if (ret) { > + /* > + * Parsing failed. Try locating old bindings for mmc-pwrseq, > + * which did not use #pwrseq-cells. > + */ > + if (strcmp(id, "mmc")) > + return NULL; > + > + ret = of_parse_phandle_with_args(dev->of_node, prop_name, NULL, 0, &args); > + if (ret) > + return NULL; > + > + dev_warn(dev, "old mmc-pwrseq binding used, add #pwrseq-cells to the provider\n"); To start simple and thus to also make review easier, I suggest to skip the mmc-pwrseq binding for now. Let's see if we can deal with that as a standalone change on top, later, instead. > + } > + > + 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; > +} > + > +struct pwrseq * __pwrseq_get(struct device *dev, const char *id, bool optional) > +{ > + struct pwrseq *pwrseq; > + struct device_link *link; > + > + pwrseq = _of_pwrseq_get(dev, id); > + if (pwrseq == NULL) > + return optional ? NULL : ERR_PTR(-ENODEV); I think we can manage this without "optional". The optional should typically be the default behaviour, I think. The caller should expect to get a handle to a pwrseq - if there is property in the DT file that says there should be one. If not, the caller should be happy to just receive "NULL". And if there is an error, we should return ERR_PTR, as you do. > + else if (IS_ERR(pwrseq)) > + return pwrseq; > + > + if (!try_module_get(pwrseq->owner)) > + return ERR_PTR(-EPROBE_DEFER); > + > + get_device(&pwrseq->dev); > + link = device_link_add(dev, &pwrseq->dev, DL_FLAG_STATELESS); > + if (!link) > + dev_dbg(dev, "failed to create device link to %s\n", > + dev_name(pwrseq->dev.parent)); > + > + return pwrseq; > +} > + > +struct pwrseq * pwrseq_get(struct device *dev, const char *id) > +{ > + return __pwrseq_get(dev, id, false); > +} > +EXPORT_SYMBOL_GPL(pwrseq_get); > + > +static void devm_pwrseq_release(struct device *dev, void *res) > +{ > + struct pwrseq *pwrseq = *(struct pwrseq **)res; > + > + pwrseq_put(dev, pwrseq); > +} > + > +struct pwrseq * devm_pwrseq_get(struct device *dev, const char *id) > +{ > + struct pwrseq **ptr, *pwrseq; > + > + ptr = devres_alloc(devm_pwrseq_release, sizeof(*ptr), GFP_KERNEL); > + if (!ptr) > + return ERR_PTR(-ENOMEM); > + > + pwrseq = pwrseq_get(dev, id); > + if (!IS_ERR(pwrseq)) { > + *ptr = pwrseq; > + devres_add(dev, ptr); > + } else { > + devres_free(ptr); > + } > + > + return pwrseq; > +} > +EXPORT_SYMBOL_GPL(devm_pwrseq_get); > + > +struct pwrseq * pwrseq_get_optional(struct device *dev, const char *id) > +{ > + return __pwrseq_get(dev, id, true); > +} > +EXPORT_SYMBOL_GPL(pwrseq_get_optional); This can be dropped, if we make this the default behaviour. > + > +struct pwrseq * devm_pwrseq_get_optional(struct device *dev, const char *id) > +{ > + struct pwrseq **ptr, *pwrseq; > + > + ptr = devres_alloc(devm_pwrseq_release, sizeof(*ptr), GFP_KERNEL); > + if (!ptr) > + return ERR_PTR(-ENOMEM); > + > + pwrseq = pwrseq_get_optional(dev, id); > + if (!IS_ERR_OR_NULL(pwrseq)) { > + *ptr = pwrseq; > + devres_add(dev, ptr); > + } else { > + devres_free(ptr); > + } > + > + return pwrseq; > +} > +EXPORT_SYMBOL_GPL(devm_pwrseq_get_optional); Ditto. > + > +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) > +{ > + 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); > + > +static void pwrseq_dev_release(struct device *dev) > +{ > + struct pwrseq *pwrseq = to_pwrseq(dev); > + > + ida_free(&pwrseq_ida, pwrseq->id); > + of_node_put(dev->of_node); > + kfree(pwrseq); > +} > + > +static struct class pwrseq_class = { > + .name = "pwrseq", > + .dev_release = pwrseq_dev_release, > +}; > + > +struct pwrseq *__pwrseq_create(struct device *dev, struct module *owner, const struct pwrseq_ops *ops, void *data) > +{ > + struct pwrseq *pwrseq; > + int ret; > + > + if (WARN_ON(!dev)) > + return ERR_PTR(-EINVAL); > + > + pwrseq = kzalloc(sizeof(*pwrseq), GFP_KERNEL); > + if (!pwrseq) > + return ERR_PTR(-ENOMEM); > + > + ret = ida_alloc(&pwrseq_ida, GFP_KERNEL); > + if (ret < 0) > + goto free_pwrseq; > + > + pwrseq->id = ret; > + > + device_initialize(&pwrseq->dev); > + > + pwrseq->dev.class = &pwrseq_class; > + pwrseq->dev.parent = dev; > + pwrseq->dev.of_node = of_node_get(dev->of_node); > + pwrseq->ops = ops; > + pwrseq->owner = owner; > + > + dev_set_drvdata(&pwrseq->dev, data); > + > + ret = dev_set_name(&pwrseq->dev, "pwrseq-%s.%u", dev_name(dev), pwrseq->id); > + if (ret) > + goto put_dev; > + > + ret = device_add(&pwrseq->dev); > + if (ret) > + goto put_dev; > + > + if (pm_runtime_enabled(dev)) { > + pm_runtime_enable(&pwrseq->dev); > + pm_runtime_no_callbacks(&pwrseq->dev); > + } I don't think we should bother with runtime PM, at least in this initial step. Please drop it, to start simple. > + > + return pwrseq; > + > +put_dev: > + /* will call pwrseq_dev_release() to free resources */ > + put_device(&pwrseq->dev); > + > + return ERR_PTR(ret); > + > +free_pwrseq: > + kfree(pwrseq); > + > + return ERR_PTR(ret); > +} > +EXPORT_SYMBOL_GPL(__pwrseq_create); > + > +void pwrseq_destroy(struct pwrseq *pwrseq) > +{ > + pm_runtime_disable(&pwrseq->dev); > + device_unregister(&pwrseq->dev); > +} > +EXPORT_SYMBOL_GPL(pwrseq_destroy); > + > +static void devm_pwrseq_destroy(struct device *dev, void *res) > +{ > + struct pwrseq *pwrseq = *(struct pwrseq **)res; > + > + pwrseq_destroy(pwrseq); > +} > + > +struct pwrseq *__devm_pwrseq_create(struct device *dev, struct module *owner, const struct pwrseq_ops *ops, void *data) > +{ > + struct pwrseq **ptr, *pwrseq; > + > + ptr = devres_alloc(devm_pwrseq_destroy, sizeof(*ptr), GFP_KERNEL); > + if (!ptr) > + return ERR_PTR(-ENOMEM); > + > + pwrseq = __pwrseq_create(dev, owner, ops, data); > + if (!IS_ERR(pwrseq)) { > + *ptr = pwrseq; > + devres_add(dev, ptr); > + } else { > + devres_free(ptr); > + } > + > + return pwrseq; > +} > +EXPORT_SYMBOL_GPL(__devm_pwrseq_create); > + > +struct pwrseq_provider *__of_pwrseq_provider_register(struct device *dev, > + struct module *owner, > + struct pwrseq * (*of_xlate)(void *data, > + struct of_phandle_args *args), > + void *data) > +{ > + struct pwrseq_provider *pwrseq_provider; > + > + pwrseq_provider = kzalloc(sizeof(*pwrseq_provider), GFP_KERNEL); > + if (!pwrseq_provider) > + return ERR_PTR(-ENOMEM); > + > + pwrseq_provider->dev = dev; > + pwrseq_provider->owner = owner; > + pwrseq_provider->of_xlate = of_xlate; > + pwrseq_provider->data = data; > + > + mutex_lock(&pwrseq_provider_mutex); > + list_add_tail(&pwrseq_provider->list, &pwrseq_provider_list); > + mutex_unlock(&pwrseq_provider_mutex); > + > + return pwrseq_provider; > +} > +EXPORT_SYMBOL_GPL(__of_pwrseq_provider_register); > + > +void of_pwrseq_provider_unregister(struct pwrseq_provider *pwrseq_provider) > +{ > + if (IS_ERR(pwrseq_provider)) > + return; > + > + mutex_lock(&pwrseq_provider_mutex); > + list_del(&pwrseq_provider->list); > + kfree(pwrseq_provider); > + mutex_unlock(&pwrseq_provider_mutex); > +} > +EXPORT_SYMBOL_GPL(of_pwrseq_provider_unregister); > + > +static void devm_pwrseq_provider_unregister(struct device *dev, void *res) > +{ > + struct pwrseq_provider *pwrseq_provider = *(struct pwrseq_provider **)res; > + > + of_pwrseq_provider_unregister(pwrseq_provider); > +} > + > +struct pwrseq_provider *__devm_of_pwrseq_provider_register(struct device *dev, > + struct module *owner, > + struct pwrseq * (*of_xlate)(void *data, > + struct of_phandle_args *args), > + void *data) > +{ > + struct pwrseq_provider **ptr, *pwrseq_provider; > + > + ptr = devres_alloc(devm_pwrseq_provider_unregister, sizeof(*ptr), GFP_KERNEL); > + if (!ptr) > + return ERR_PTR(-ENOMEM); > + > + pwrseq_provider = __of_pwrseq_provider_register(dev, owner, of_xlate, data); > + if (!IS_ERR(pwrseq_provider)) { > + *ptr = pwrseq_provider; > + devres_add(dev, ptr); > + } else { > + devres_free(ptr); > + } > + > + return pwrseq_provider; > +} > +EXPORT_SYMBOL_GPL(__devm_of_pwrseq_provider_register); > + > +struct pwrseq *of_pwrseq_xlate_onecell(void *data, struct of_phandle_args *args) > +{ > + struct pwrseq_onecell_data *pwrseq_data = data; > + unsigned int idx; > + > + if (args->args_count != 1) > + return ERR_PTR(-EINVAL); > + > + idx = args->args[0]; > + if (idx >= pwrseq_data->num) { > + pr_err("%s: invalid index %u\n", __func__, idx); > + return ERR_PTR(-EINVAL); > + } In many cases it's reasonable to leave room for future extensions, so that a provider could serve with more than one power-sequencer. I guess that is what you intend to do here, right? In my opinion, I don't think what would happen, especially since a power-sequence is something that should be specific to one particular device (a Qcom WiFi/Blutooth chip, for example). That said, I suggest limiting this to a 1:1 mapping between the device node and power-sequencer. I think that should simplify the code a bit. > + > + return pwrseq_data->pwrseqs[idx]; > +} > + > +static int __init pwrseq_core_init(void) > +{ > + return class_register(&pwrseq_class); > +} > +device_initcall(pwrseq_core_init); > diff --git a/include/linux/pwrseq/consumer.h b/include/linux/pwrseq/consumer.h > new file mode 100644 > index 000000000000..fbcdc1fc0751 > --- /dev/null > +++ b/include/linux/pwrseq/consumer.h > @@ -0,0 +1,88 @@ > +/* SPDX-License-Identifier: GPL-2.0-or-later */ > +/* > + * Copyright (c) 2021 Linaro Ltd. > + */ > + > +#ifndef __LINUX_PWRSEQ_CONSUMER_H__ > +#define __LINUX_PWRSEQ_CONSUMER_H__ > + > +struct pwrseq; > +struct device; > + > +#if defined(CONFIG_PWRSEQ) > + > +struct pwrseq *__must_check pwrseq_get(struct device *dev, const char *id); > +struct pwrseq *__must_check devm_pwrseq_get(struct device *dev, const char *id); > + > +struct pwrseq *__must_check pwrseq_get_optional(struct device *dev, const char *id); > +struct pwrseq *__must_check devm_pwrseq_get_optional(struct device *dev, const char *id); > + > +void pwrseq_put(struct device *dev, struct pwrseq *pwrseq); > + > +int pwrseq_pre_power_on(struct pwrseq *pwrseq); > +int pwrseq_power_on(struct pwrseq *pwrseq); > +void pwrseq_power_off(struct pwrseq *pwrseq); > +void pwrseq_reset(struct pwrseq *pwrseq); > + > +#else > + > +static inline struct pwrseq *__must_check > +pwrseq_get(struct device *dev, const char *id) > +{ > + return ERR_PTR(-ENOSYS); > +} > + > +static inline struct pwrseq *__must_check > +devm_pwrseq_get(struct device *dev, const char *id) > +{ > + return ERR_PTR(-ENOSYS); > +} > + > +static inline struct pwrseq *__must_check > +pwrseq_get_optional(struct device *dev, const char *id) > +{ > + return NULL; > +} > + > +static inline struct pwrseq *__must_check > +devm_pwrseq_get_optional(struct device *dev, const char *id) > +{ > + return NULL; > +} > + > +static inline void pwrseq_put(struct device *dev, struct pwrseq *pwrseq) > +{ > +} > + > +static inline int pwrseq_pre_power_on(struct pwrseq *pwrseq) > +{ > + return -ENOSYS; > +} > + > +static inline int pwrseq_power_on(struct pwrseq *pwrseq) > +{ > + return -ENOSYS; > +} > + > +static inline void pwrseq_power_off(struct pwrseq *pwrseq) > +{ > +} > + > +static inline void pwrseq_reset(struct pwrseq *pwrseq) > +{ > +} > + > +#endif > + > +static inline int pwrseq_full_power_on(struct pwrseq *pwrseq) > +{ > + int ret; > + > + ret = pwrseq_pre_power_on(pwrseq); > + if (ret) > + return ret; > + > + return pwrseq_power_on(pwrseq); > +} > + > +#endif /* __LINUX_PWRSEQ_CONSUMER_H__ */ > diff --git a/include/linux/pwrseq/driver.h b/include/linux/pwrseq/driver.h > new file mode 100644 > index 000000000000..b2bc46624d7e > --- /dev/null > +++ b/include/linux/pwrseq/driver.h > @@ -0,0 +1,75 @@ > +/* SPDX-License-Identifier: GPL-2.0-or-later */ > +/* > + * Copyright (c) 2021 Linaro Ltd. > + */ > + > +#ifndef __LINUX_PWRSEQ_DRIVER_H__ > +#define __LINUX_PWRSEQ_DRIVER_H__ > + > +#include <linux/device.h> > + > +struct pwrseq; > + > +struct pwrseq_ops { > + int (*pre_power_on)(struct pwrseq *pwrseq); > + int (*power_on)(struct pwrseq *pwrseq); > + void (*power_off)(struct pwrseq *pwrseq); > + void (*reset)(struct pwrseq *pwrseq); > +}; > + > +struct module; > + > +struct pwrseq { > + struct device dev; > + const struct pwrseq_ops *ops; > + unsigned int id; > + struct module *owner; > +}; > + > +struct pwrseq *__pwrseq_create(struct device *dev, struct module *owner, const struct pwrseq_ops *ops, void *data); > +struct pwrseq *__devm_pwrseq_create(struct device *dev, struct module *owner, const struct pwrseq_ops *ops, void *data); > + > +#define pwrseq_create(dev, ops, data) __pwrseq_create((dev), THIS_MODULE, (ops), (data)) > +#define devm_pwrseq_create(dev, ops, data) __devm_pwrseq_create((dev), THIS_MODULE, (ops), (data)) > + > +void pwrseq_destroy(struct pwrseq *pwrseq); > + > +static inline void *pwrseq_get_data(struct pwrseq *pwrseq) > +{ > + return dev_get_drvdata(&pwrseq->dev); > +} > + > +#define of_pwrseq_provider_register(dev, xlate, data) \ > + __of_pwrseq_provider_register((dev), THIS_MODULE, (xlate), (data)) > + > +#define devm_of_pwrseq_provider_register(dev, xlate, data) \ > + __devm_of_pwrseq_provider_register((dev), THIS_MODULE, (xlate), (data)) > + > +struct of_phandle_args; > + > +struct pwrseq_provider *__of_pwrseq_provider_register(struct device *dev, > + struct module *owner, > + struct pwrseq * (*of_xlate)(void *data, > + struct of_phandle_args *args), > + void *data); > +struct pwrseq_provider *__devm_of_pwrseq_provider_register(struct device *dev, > + struct module *owner, > + struct pwrseq * (*of_xlate)(void *data, > + struct of_phandle_args *args), > + void *data); > +void of_pwrseq_provider_unregister(struct pwrseq_provider *pwrseq_provider); > + > +static inline struct pwrseq *of_pwrseq_xlate_single(void *data, > + struct of_phandle_args *args) > +{ > + return data; > +} > + > +struct pwrseq_onecell_data { > + unsigned int num; > + struct pwrseq *pwrseqs[]; > +}; According to my earlier comment, I think a lot can be removed from here - if you would limit the provider to only use of_pwrseq_xlate_single. Again, I think it's better to start simple, as it simplifies the review. > + > +struct pwrseq *of_pwrseq_xlate_onecell(void *data, struct of_phandle_args *args); > + > +#endif /* __LINUX_PWRSEQ_DRIVER_H__ */ > -- > 2.33.0 > Other than my comments, overall I think this looks like a good start. Kind regards Uffe
On 10/09/2021 13:02, Ulf Hansson wrote: > On Sun, 29 Aug 2021 at 15:13, Dmitry Baryshkov > <dmitry.baryshkov@linaro.org> 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. > > This is far too vague. You are suggesting to add a new subsystem, I > think that deserves some more explanations as justifications. > > Additionally, it wouldn't hurt to explain a bit how the actual > subsystem is supposed to work, at least from a toplevel point of view. Ack, will add more explanations together with the some inline documentation. > >> >> Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org> >> --- >> drivers/power/Kconfig | 1 + >> drivers/power/Makefile | 1 + >> drivers/power/pwrseq/Kconfig | 11 + >> drivers/power/pwrseq/Makefile | 6 + >> drivers/power/pwrseq/core.c | 412 ++++++++++++++++++++++++++++++++ >> include/linux/pwrseq/consumer.h | 88 +++++++ >> include/linux/pwrseq/driver.h | 75 ++++++ >> 7 files changed, 594 insertions(+) >> create mode 100644 drivers/power/pwrseq/Kconfig >> create mode 100644 drivers/power/pwrseq/Makefile >> create mode 100644 drivers/power/pwrseq/core.c >> create mode 100644 include/linux/pwrseq/consumer.h >> create mode 100644 include/linux/pwrseq/driver.h > > I noticed there is no update of the MAINTAINERS file. We need that to > be a part of the $subject patch as well, I think. But, let's discuss > that later. > >> >> diff --git a/drivers/power/Kconfig b/drivers/power/Kconfig >> index 696bf77a7042..c87cd2240a74 100644 >> --- a/drivers/power/Kconfig >> +++ b/drivers/power/Kconfig >> @@ -1,3 +1,4 @@ >> # SPDX-License-Identifier: GPL-2.0-only >> +source "drivers/power/pwrseq/Kconfig" >> source "drivers/power/reset/Kconfig" >> source "drivers/power/supply/Kconfig" >> diff --git a/drivers/power/Makefile b/drivers/power/Makefile >> index effbf0377f32..1dbce454a8c4 100644 >> --- a/drivers/power/Makefile >> +++ b/drivers/power/Makefile >> @@ -1,3 +1,4 @@ >> # SPDX-License-Identifier: GPL-2.0-only >> obj-$(CONFIG_POWER_RESET) += reset/ >> obj-$(CONFIG_POWER_SUPPLY) += supply/ >> +obj-$(CONFIG_PWRSEQ) += pwrseq/ >> diff --git a/drivers/power/pwrseq/Kconfig b/drivers/power/pwrseq/Kconfig >> new file mode 100644 >> index 000000000000..8904ec9ed541 >> --- /dev/null >> +++ b/drivers/power/pwrseq/Kconfig >> @@ -0,0 +1,11 @@ >> +# SPDX-License-Identifier: GPL-2.0-only >> +menuconfig PWRSEQ >> + bool "Power Sequencer drivers" >> + help >> + Provides support for special power sequencing drivers. > > This needs more description. The name "power sequencer" isn't entirely > self-explanatory, for when this should be used. I am not saying you > should invent a new name, rather just extend the description so people > get a better idea of what this is supposed to be used for. > >> + >> + Say Y here to enable support for such devices >> + >> +if PWRSEQ >> + >> +endif >> diff --git a/drivers/power/pwrseq/Makefile b/drivers/power/pwrseq/Makefile >> new file mode 100644 >> index 000000000000..108429ff6445 >> --- /dev/null >> +++ b/drivers/power/pwrseq/Makefile >> @@ -0,0 +1,6 @@ >> +# SPDX-License-Identifier: GPL-2.0 >> +# >> +# Makefile for power sequencer drivers. >> +# >> + >> +obj-$(CONFIG_PWRSEQ) += core.o >> diff --git a/drivers/power/pwrseq/core.c b/drivers/power/pwrseq/core.c >> new file mode 100644 >> index 000000000000..2e4e9d123e60 >> --- /dev/null >> +++ b/drivers/power/pwrseq/core.c >> @@ -0,0 +1,412 @@ >> +// SPDX-License-Identifier: GPL-2.0-only >> +/* >> + * Copyright 2021 (c) Linaro Ltd. >> + * Author: Dmitry Baryshkov <dmitry.baryshkov@linaro.org> >> + * >> + * Based on phy-core.c: >> + * Copyright (C) 2013 Texas Instruments Incorporated - http://www.ti.com >> + */ >> + >> +#include <linux/device.h> >> +#include <linux/idr.h> >> +#include <linux/module.h> >> +#include <linux/mutex.h> >> +#include <linux/of.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)) >> + >> +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); > > device_links - why do we need these at this initial step? > > Please drop them so we can start with a simple implementation - and > then possibly extend it. Ack > >> + >> + 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); >> + ret = of_parse_phandle_with_args(dev->of_node, prop_name, "#pwrseq-cells", 0, &args); > > This means that you are parsing a new DT binding/property. > > Please fold in a DT binding patch, preceding $subject patch, so that > new binding that it can be discussed as well. > >> + if (ret) { >> + /* >> + * Parsing failed. Try locating old bindings for mmc-pwrseq, >> + * which did not use #pwrseq-cells. >> + */ >> + if (strcmp(id, "mmc")) >> + return NULL; >> + >> + ret = of_parse_phandle_with_args(dev->of_node, prop_name, NULL, 0, &args); >> + if (ret) >> + return NULL; >> + >> + dev_warn(dev, "old mmc-pwrseq binding used, add #pwrseq-cells to the provider\n"); > > To start simple and thus to also make review easier, I suggest to skip > the mmc-pwrseq binding for now. Let's see if we can deal with that as > a standalone change on top, later, instead. Ack, will split to the separate patch > >> + } >> + >> + 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; >> +} >> + >> +struct pwrseq * __pwrseq_get(struct device *dev, const char *id, bool optional) >> +{ >> + struct pwrseq *pwrseq; >> + struct device_link *link; >> + >> + pwrseq = _of_pwrseq_get(dev, id); >> + if (pwrseq == NULL) >> + return optional ? NULL : ERR_PTR(-ENODEV); > > I think we can manage this without "optional". The optional should > typically be the default behaviour, I think. > > The caller should expect to get a handle to a pwrseq - if there is > property in the DT file that says there should be one. If not, the > caller should be happy to just receive "NULL". And if there is an > error, we should return ERR_PTR, as you do. Ack. > >> + else if (IS_ERR(pwrseq)) >> + return pwrseq; >> + >> + if (!try_module_get(pwrseq->owner)) >> + return ERR_PTR(-EPROBE_DEFER); >> + >> + get_device(&pwrseq->dev); >> + link = device_link_add(dev, &pwrseq->dev, DL_FLAG_STATELESS); >> + if (!link) >> + dev_dbg(dev, "failed to create device link to %s\n", >> + dev_name(pwrseq->dev.parent)); >> + >> + return pwrseq; >> +} >> + >> +struct pwrseq * pwrseq_get(struct device *dev, const char *id) >> +{ >> + return __pwrseq_get(dev, id, false); >> +} >> +EXPORT_SYMBOL_GPL(pwrseq_get); >> + >> +static void devm_pwrseq_release(struct device *dev, void *res) >> +{ >> + struct pwrseq *pwrseq = *(struct pwrseq **)res; >> + >> + pwrseq_put(dev, pwrseq); >> +} >> + >> +struct pwrseq * devm_pwrseq_get(struct device *dev, const char *id) >> +{ >> + struct pwrseq **ptr, *pwrseq; >> + >> + ptr = devres_alloc(devm_pwrseq_release, sizeof(*ptr), GFP_KERNEL); >> + if (!ptr) >> + return ERR_PTR(-ENOMEM); >> + >> + pwrseq = pwrseq_get(dev, id); >> + if (!IS_ERR(pwrseq)) { >> + *ptr = pwrseq; >> + devres_add(dev, ptr); >> + } else { >> + devres_free(ptr); >> + } >> + >> + return pwrseq; >> +} >> +EXPORT_SYMBOL_GPL(devm_pwrseq_get); >> + >> +struct pwrseq * pwrseq_get_optional(struct device *dev, const char *id) >> +{ >> + return __pwrseq_get(dev, id, true); >> +} >> +EXPORT_SYMBOL_GPL(pwrseq_get_optional); > > This can be dropped, if we make this the default behaviour. > >> + >> +struct pwrseq * devm_pwrseq_get_optional(struct device *dev, const char *id) >> +{ >> + struct pwrseq **ptr, *pwrseq; >> + >> + ptr = devres_alloc(devm_pwrseq_release, sizeof(*ptr), GFP_KERNEL); >> + if (!ptr) >> + return ERR_PTR(-ENOMEM); >> + >> + pwrseq = pwrseq_get_optional(dev, id); >> + if (!IS_ERR_OR_NULL(pwrseq)) { >> + *ptr = pwrseq; >> + devres_add(dev, ptr); >> + } else { >> + devres_free(ptr); >> + } >> + >> + return pwrseq; >> +} >> +EXPORT_SYMBOL_GPL(devm_pwrseq_get_optional); > > Ditto. > >> + >> +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) >> +{ >> + 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); >> + >> +static void pwrseq_dev_release(struct device *dev) >> +{ >> + struct pwrseq *pwrseq = to_pwrseq(dev); >> + >> + ida_free(&pwrseq_ida, pwrseq->id); >> + of_node_put(dev->of_node); >> + kfree(pwrseq); >> +} >> + >> +static struct class pwrseq_class = { >> + .name = "pwrseq", >> + .dev_release = pwrseq_dev_release, >> +}; >> + >> +struct pwrseq *__pwrseq_create(struct device *dev, struct module *owner, const struct pwrseq_ops *ops, void *data) >> +{ >> + struct pwrseq *pwrseq; >> + int ret; >> + >> + if (WARN_ON(!dev)) >> + return ERR_PTR(-EINVAL); >> + >> + pwrseq = kzalloc(sizeof(*pwrseq), GFP_KERNEL); >> + if (!pwrseq) >> + return ERR_PTR(-ENOMEM); >> + >> + ret = ida_alloc(&pwrseq_ida, GFP_KERNEL); >> + if (ret < 0) >> + goto free_pwrseq; >> + >> + pwrseq->id = ret; >> + >> + device_initialize(&pwrseq->dev); >> + >> + pwrseq->dev.class = &pwrseq_class; >> + pwrseq->dev.parent = dev; >> + pwrseq->dev.of_node = of_node_get(dev->of_node); >> + pwrseq->ops = ops; >> + pwrseq->owner = owner; >> + >> + dev_set_drvdata(&pwrseq->dev, data); >> + >> + ret = dev_set_name(&pwrseq->dev, "pwrseq-%s.%u", dev_name(dev), pwrseq->id); >> + if (ret) >> + goto put_dev; >> + >> + ret = device_add(&pwrseq->dev); >> + if (ret) >> + goto put_dev; >> + >> + if (pm_runtime_enabled(dev)) { >> + pm_runtime_enable(&pwrseq->dev); >> + pm_runtime_no_callbacks(&pwrseq->dev); >> + } > > I don't think we should bother with runtime PM, at least in this > initial step. Please drop it, to start simple. Ack > >> + >> + return pwrseq; >> + >> +put_dev: >> + /* will call pwrseq_dev_release() to free resources */ >> + put_device(&pwrseq->dev); >> + >> + return ERR_PTR(ret); >> + >> +free_pwrseq: >> + kfree(pwrseq); >> + >> + return ERR_PTR(ret); >> +} >> +EXPORT_SYMBOL_GPL(__pwrseq_create); >> + >> +void pwrseq_destroy(struct pwrseq *pwrseq) >> +{ >> + pm_runtime_disable(&pwrseq->dev); >> + device_unregister(&pwrseq->dev); >> +} >> +EXPORT_SYMBOL_GPL(pwrseq_destroy); >> + >> +static void devm_pwrseq_destroy(struct device *dev, void *res) >> +{ >> + struct pwrseq *pwrseq = *(struct pwrseq **)res; >> + >> + pwrseq_destroy(pwrseq); >> +} >> + >> +struct pwrseq *__devm_pwrseq_create(struct device *dev, struct module *owner, const struct pwrseq_ops *ops, void *data) >> +{ >> + struct pwrseq **ptr, *pwrseq; >> + >> + ptr = devres_alloc(devm_pwrseq_destroy, sizeof(*ptr), GFP_KERNEL); >> + if (!ptr) >> + return ERR_PTR(-ENOMEM); >> + >> + pwrseq = __pwrseq_create(dev, owner, ops, data); >> + if (!IS_ERR(pwrseq)) { >> + *ptr = pwrseq; >> + devres_add(dev, ptr); >> + } else { >> + devres_free(ptr); >> + } >> + >> + return pwrseq; >> +} >> +EXPORT_SYMBOL_GPL(__devm_pwrseq_create); >> + >> +struct pwrseq_provider *__of_pwrseq_provider_register(struct device *dev, >> + struct module *owner, >> + struct pwrseq * (*of_xlate)(void *data, >> + struct of_phandle_args *args), >> + void *data) >> +{ >> + struct pwrseq_provider *pwrseq_provider; >> + >> + pwrseq_provider = kzalloc(sizeof(*pwrseq_provider), GFP_KERNEL); >> + if (!pwrseq_provider) >> + return ERR_PTR(-ENOMEM); >> + >> + pwrseq_provider->dev = dev; >> + pwrseq_provider->owner = owner; >> + pwrseq_provider->of_xlate = of_xlate; >> + pwrseq_provider->data = data; >> + >> + mutex_lock(&pwrseq_provider_mutex); >> + list_add_tail(&pwrseq_provider->list, &pwrseq_provider_list); >> + mutex_unlock(&pwrseq_provider_mutex); >> + >> + return pwrseq_provider; >> +} >> +EXPORT_SYMBOL_GPL(__of_pwrseq_provider_register); >> + >> +void of_pwrseq_provider_unregister(struct pwrseq_provider *pwrseq_provider) >> +{ >> + if (IS_ERR(pwrseq_provider)) >> + return; >> + >> + mutex_lock(&pwrseq_provider_mutex); >> + list_del(&pwrseq_provider->list); >> + kfree(pwrseq_provider); >> + mutex_unlock(&pwrseq_provider_mutex); >> +} >> +EXPORT_SYMBOL_GPL(of_pwrseq_provider_unregister); >> + >> +static void devm_pwrseq_provider_unregister(struct device *dev, void *res) >> +{ >> + struct pwrseq_provider *pwrseq_provider = *(struct pwrseq_provider **)res; >> + >> + of_pwrseq_provider_unregister(pwrseq_provider); >> +} >> + >> +struct pwrseq_provider *__devm_of_pwrseq_provider_register(struct device *dev, >> + struct module *owner, >> + struct pwrseq * (*of_xlate)(void *data, >> + struct of_phandle_args *args), >> + void *data) >> +{ >> + struct pwrseq_provider **ptr, *pwrseq_provider; >> + >> + ptr = devres_alloc(devm_pwrseq_provider_unregister, sizeof(*ptr), GFP_KERNEL); >> + if (!ptr) >> + return ERR_PTR(-ENOMEM); >> + >> + pwrseq_provider = __of_pwrseq_provider_register(dev, owner, of_xlate, data); >> + if (!IS_ERR(pwrseq_provider)) { >> + *ptr = pwrseq_provider; >> + devres_add(dev, ptr); >> + } else { >> + devres_free(ptr); >> + } >> + >> + return pwrseq_provider; >> +} >> +EXPORT_SYMBOL_GPL(__devm_of_pwrseq_provider_register); >> + >> +struct pwrseq *of_pwrseq_xlate_onecell(void *data, struct of_phandle_args *args) >> +{ >> + struct pwrseq_onecell_data *pwrseq_data = data; >> + unsigned int idx; >> + >> + if (args->args_count != 1) >> + return ERR_PTR(-EINVAL); >> + >> + idx = args->args[0]; >> + if (idx >= pwrseq_data->num) { >> + pr_err("%s: invalid index %u\n", __func__, idx); >> + return ERR_PTR(-EINVAL); >> + } > > In many cases it's reasonable to leave room for future extensions, so > that a provider could serve with more than one power-sequencer. I > guess that is what you intend to do here, right? > > In my opinion, I don't think what would happen, especially since a > power-sequence is something that should be specific to one particular > device (a Qcom WiFi/Blutooth chip, for example). > > That said, I suggest limiting this to a 1:1 mapping between the device > node and power-sequencer. I think that should simplify the code a bit. In fact the WiFi/BT example itself provides a non 1:1 mapping. In my current design the power sequencer provides two instances (one for WiFi, one for BT). This allows us to move the knowledge about "enable" pins to the pwrseq. Once the QCA BT driver acquires and powers up the pwrseq, the BT part is ready. No need to toggle any additional pins. Once the WiFi pwrseq is powered up, the WiFi part is present on the bus and ready, without any additional pin toggling. I can move onecell support to the separate patch if you think this might simplify the code review. > >> + >> + return pwrseq_data->pwrseqs[idx]; >> +} >> + >> +static int __init pwrseq_core_init(void) >> +{ >> + return class_register(&pwrseq_class); >> +} >> +device_initcall(pwrseq_core_init); >> diff --git a/include/linux/pwrseq/consumer.h b/include/linux/pwrseq/consumer.h >> new file mode 100644 >> index 000000000000..fbcdc1fc0751 >> --- /dev/null >> +++ b/include/linux/pwrseq/consumer.h >> @@ -0,0 +1,88 @@ >> +/* SPDX-License-Identifier: GPL-2.0-or-later */ >> +/* >> + * Copyright (c) 2021 Linaro Ltd. >> + */ >> + >> +#ifndef __LINUX_PWRSEQ_CONSUMER_H__ >> +#define __LINUX_PWRSEQ_CONSUMER_H__ >> + >> +struct pwrseq; >> +struct device; >> + >> +#if defined(CONFIG_PWRSEQ) >> + >> +struct pwrseq *__must_check pwrseq_get(struct device *dev, const char *id); >> +struct pwrseq *__must_check devm_pwrseq_get(struct device *dev, const char *id); >> + >> +struct pwrseq *__must_check pwrseq_get_optional(struct device *dev, const char *id); >> +struct pwrseq *__must_check devm_pwrseq_get_optional(struct device *dev, const char *id); >> + >> +void pwrseq_put(struct device *dev, struct pwrseq *pwrseq); >> + >> +int pwrseq_pre_power_on(struct pwrseq *pwrseq); >> +int pwrseq_power_on(struct pwrseq *pwrseq); >> +void pwrseq_power_off(struct pwrseq *pwrseq); >> +void pwrseq_reset(struct pwrseq *pwrseq); >> + >> +#else >> + >> +static inline struct pwrseq *__must_check >> +pwrseq_get(struct device *dev, const char *id) >> +{ >> + return ERR_PTR(-ENOSYS); >> +} >> + >> +static inline struct pwrseq *__must_check >> +devm_pwrseq_get(struct device *dev, const char *id) >> +{ >> + return ERR_PTR(-ENOSYS); >> +} >> + >> +static inline struct pwrseq *__must_check >> +pwrseq_get_optional(struct device *dev, const char *id) >> +{ >> + return NULL; >> +} >> + >> +static inline struct pwrseq *__must_check >> +devm_pwrseq_get_optional(struct device *dev, const char *id) >> +{ >> + return NULL; >> +} >> + >> +static inline void pwrseq_put(struct device *dev, struct pwrseq *pwrseq) >> +{ >> +} >> + >> +static inline int pwrseq_pre_power_on(struct pwrseq *pwrseq) >> +{ >> + return -ENOSYS; >> +} >> + >> +static inline int pwrseq_power_on(struct pwrseq *pwrseq) >> +{ >> + return -ENOSYS; >> +} >> + >> +static inline void pwrseq_power_off(struct pwrseq *pwrseq) >> +{ >> +} >> + >> +static inline void pwrseq_reset(struct pwrseq *pwrseq) >> +{ >> +} >> + >> +#endif >> + >> +static inline int pwrseq_full_power_on(struct pwrseq *pwrseq) >> +{ >> + int ret; >> + >> + ret = pwrseq_pre_power_on(pwrseq); >> + if (ret) >> + return ret; >> + >> + return pwrseq_power_on(pwrseq); >> +} >> + >> +#endif /* __LINUX_PWRSEQ_CONSUMER_H__ */ >> diff --git a/include/linux/pwrseq/driver.h b/include/linux/pwrseq/driver.h >> new file mode 100644 >> index 000000000000..b2bc46624d7e >> --- /dev/null >> +++ b/include/linux/pwrseq/driver.h >> @@ -0,0 +1,75 @@ >> +/* SPDX-License-Identifier: GPL-2.0-or-later */ >> +/* >> + * Copyright (c) 2021 Linaro Ltd. >> + */ >> + >> +#ifndef __LINUX_PWRSEQ_DRIVER_H__ >> +#define __LINUX_PWRSEQ_DRIVER_H__ >> + >> +#include <linux/device.h> >> + >> +struct pwrseq; >> + >> +struct pwrseq_ops { >> + int (*pre_power_on)(struct pwrseq *pwrseq); >> + int (*power_on)(struct pwrseq *pwrseq); >> + void (*power_off)(struct pwrseq *pwrseq); >> + void (*reset)(struct pwrseq *pwrseq); >> +}; >> + >> +struct module; >> + >> +struct pwrseq { >> + struct device dev; >> + const struct pwrseq_ops *ops; >> + unsigned int id; >> + struct module *owner; >> +}; >> + >> +struct pwrseq *__pwrseq_create(struct device *dev, struct module *owner, const struct pwrseq_ops *ops, void *data); >> +struct pwrseq *__devm_pwrseq_create(struct device *dev, struct module *owner, const struct pwrseq_ops *ops, void *data); >> + >> +#define pwrseq_create(dev, ops, data) __pwrseq_create((dev), THIS_MODULE, (ops), (data)) >> +#define devm_pwrseq_create(dev, ops, data) __devm_pwrseq_create((dev), THIS_MODULE, (ops), (data)) >> + >> +void pwrseq_destroy(struct pwrseq *pwrseq); >> + >> +static inline void *pwrseq_get_data(struct pwrseq *pwrseq) >> +{ >> + return dev_get_drvdata(&pwrseq->dev); >> +} >> + >> +#define of_pwrseq_provider_register(dev, xlate, data) \ >> + __of_pwrseq_provider_register((dev), THIS_MODULE, (xlate), (data)) >> + >> +#define devm_of_pwrseq_provider_register(dev, xlate, data) \ >> + __devm_of_pwrseq_provider_register((dev), THIS_MODULE, (xlate), (data)) >> + >> +struct of_phandle_args; >> + >> +struct pwrseq_provider *__of_pwrseq_provider_register(struct device *dev, >> + struct module *owner, >> + struct pwrseq * (*of_xlate)(void *data, >> + struct of_phandle_args *args), >> + void *data); >> +struct pwrseq_provider *__devm_of_pwrseq_provider_register(struct device *dev, >> + struct module *owner, >> + struct pwrseq * (*of_xlate)(void *data, >> + struct of_phandle_args *args), >> + void *data); >> +void of_pwrseq_provider_unregister(struct pwrseq_provider *pwrseq_provider); >> + >> +static inline struct pwrseq *of_pwrseq_xlate_single(void *data, >> + struct of_phandle_args *args) >> +{ >> + return data; >> +} >> + >> +struct pwrseq_onecell_data { >> + unsigned int num; >> + struct pwrseq *pwrseqs[]; >> +}; > > According to my earlier comment, I think a lot can be removed from > here - if you would limit the provider to only use > of_pwrseq_xlate_single. > > Again, I think it's better to start simple, as it simplifies the review. > >> + >> +struct pwrseq *of_pwrseq_xlate_onecell(void *data, struct of_phandle_args *args); >> + >> +#endif /* __LINUX_PWRSEQ_DRIVER_H__ */ >> -- >> 2.33.0 >> > > Other than my comments, overall I think this looks like a good start. > > Kind regards > Uffe >
[...] > >> + > >> +struct pwrseq *of_pwrseq_xlate_onecell(void *data, struct of_phandle_args *args) > >> +{ > >> + struct pwrseq_onecell_data *pwrseq_data = data; > >> + unsigned int idx; > >> + > >> + if (args->args_count != 1) > >> + return ERR_PTR(-EINVAL); > >> + > >> + idx = args->args[0]; > >> + if (idx >= pwrseq_data->num) { > >> + pr_err("%s: invalid index %u\n", __func__, idx); > >> + return ERR_PTR(-EINVAL); > >> + } > > > > In many cases it's reasonable to leave room for future extensions, so > > that a provider could serve with more than one power-sequencer. I > > guess that is what you intend to do here, right? > > > > In my opinion, I don't think what would happen, especially since a > > power-sequence is something that should be specific to one particular > > device (a Qcom WiFi/Blutooth chip, for example). > > > > That said, I suggest limiting this to a 1:1 mapping between the device > > node and power-sequencer. I think that should simplify the code a bit. > > In fact the WiFi/BT example itself provides a non 1:1 mapping. In my > current design the power sequencer provides two instances (one for WiFi, > one for BT). This allows us to move the knowledge about "enable" pins to > the pwrseq. Once the QCA BT driver acquires and powers up the pwrseq, > the BT part is ready. No need to toggle any additional pins. Once the > WiFi pwrseq is powered up, the WiFi part is present on the bus and > ready, without any additional pin toggling. Aha, that seems reasonable. > > I can move onecell support to the separate patch if you think this might > simplify the code review. It doesn't matter, both options work for me. [...] Kind regards Uffe
diff --git a/drivers/power/Kconfig b/drivers/power/Kconfig index 696bf77a7042..c87cd2240a74 100644 --- a/drivers/power/Kconfig +++ b/drivers/power/Kconfig @@ -1,3 +1,4 @@ # SPDX-License-Identifier: GPL-2.0-only +source "drivers/power/pwrseq/Kconfig" source "drivers/power/reset/Kconfig" source "drivers/power/supply/Kconfig" diff --git a/drivers/power/Makefile b/drivers/power/Makefile index effbf0377f32..1dbce454a8c4 100644 --- a/drivers/power/Makefile +++ b/drivers/power/Makefile @@ -1,3 +1,4 @@ # SPDX-License-Identifier: GPL-2.0-only obj-$(CONFIG_POWER_RESET) += reset/ obj-$(CONFIG_POWER_SUPPLY) += supply/ +obj-$(CONFIG_PWRSEQ) += pwrseq/ diff --git a/drivers/power/pwrseq/Kconfig b/drivers/power/pwrseq/Kconfig new file mode 100644 index 000000000000..8904ec9ed541 --- /dev/null +++ b/drivers/power/pwrseq/Kconfig @@ -0,0 +1,11 @@ +# SPDX-License-Identifier: GPL-2.0-only +menuconfig PWRSEQ + bool "Power Sequencer drivers" + help + Provides support for special power sequencing drivers. + + Say Y here to enable support for such devices + +if PWRSEQ + +endif diff --git a/drivers/power/pwrseq/Makefile b/drivers/power/pwrseq/Makefile new file mode 100644 index 000000000000..108429ff6445 --- /dev/null +++ b/drivers/power/pwrseq/Makefile @@ -0,0 +1,6 @@ +# SPDX-License-Identifier: GPL-2.0 +# +# Makefile for power sequencer drivers. +# + +obj-$(CONFIG_PWRSEQ) += core.o diff --git a/drivers/power/pwrseq/core.c b/drivers/power/pwrseq/core.c new file mode 100644 index 000000000000..2e4e9d123e60 --- /dev/null +++ b/drivers/power/pwrseq/core.c @@ -0,0 +1,412 @@ +// SPDX-License-Identifier: GPL-2.0-only +/* + * Copyright 2021 (c) Linaro Ltd. + * Author: Dmitry Baryshkov <dmitry.baryshkov@linaro.org> + * + * Based on phy-core.c: + * Copyright (C) 2013 Texas Instruments Incorporated - http://www.ti.com + */ + +#include <linux/device.h> +#include <linux/idr.h> +#include <linux/module.h> +#include <linux/mutex.h> +#include <linux/of.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)) + +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); + ret = of_parse_phandle_with_args(dev->of_node, prop_name, "#pwrseq-cells", 0, &args); + if (ret) { + /* + * Parsing failed. Try locating old bindings for mmc-pwrseq, + * which did not use #pwrseq-cells. + */ + if (strcmp(id, "mmc")) + return NULL; + + ret = of_parse_phandle_with_args(dev->of_node, prop_name, NULL, 0, &args); + if (ret) + return NULL; + + dev_warn(dev, "old mmc-pwrseq binding used, add #pwrseq-cells to the provider\n"); + } + + 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; +} + +struct pwrseq * __pwrseq_get(struct device *dev, const char *id, bool optional) +{ + struct pwrseq *pwrseq; + struct device_link *link; + + pwrseq = _of_pwrseq_get(dev, id); + if (pwrseq == NULL) + return optional ? NULL : ERR_PTR(-ENODEV); + else if (IS_ERR(pwrseq)) + return pwrseq; + + if (!try_module_get(pwrseq->owner)) + return ERR_PTR(-EPROBE_DEFER); + + get_device(&pwrseq->dev); + link = device_link_add(dev, &pwrseq->dev, DL_FLAG_STATELESS); + if (!link) + dev_dbg(dev, "failed to create device link to %s\n", + dev_name(pwrseq->dev.parent)); + + return pwrseq; +} + +struct pwrseq * pwrseq_get(struct device *dev, const char *id) +{ + return __pwrseq_get(dev, id, false); +} +EXPORT_SYMBOL_GPL(pwrseq_get); + +static void devm_pwrseq_release(struct device *dev, void *res) +{ + struct pwrseq *pwrseq = *(struct pwrseq **)res; + + pwrseq_put(dev, pwrseq); +} + +struct pwrseq * devm_pwrseq_get(struct device *dev, const char *id) +{ + struct pwrseq **ptr, *pwrseq; + + ptr = devres_alloc(devm_pwrseq_release, sizeof(*ptr), GFP_KERNEL); + if (!ptr) + return ERR_PTR(-ENOMEM); + + pwrseq = pwrseq_get(dev, id); + if (!IS_ERR(pwrseq)) { + *ptr = pwrseq; + devres_add(dev, ptr); + } else { + devres_free(ptr); + } + + return pwrseq; +} +EXPORT_SYMBOL_GPL(devm_pwrseq_get); + +struct pwrseq * pwrseq_get_optional(struct device *dev, const char *id) +{ + return __pwrseq_get(dev, id, true); +} +EXPORT_SYMBOL_GPL(pwrseq_get_optional); + +struct pwrseq * devm_pwrseq_get_optional(struct device *dev, const char *id) +{ + struct pwrseq **ptr, *pwrseq; + + ptr = devres_alloc(devm_pwrseq_release, sizeof(*ptr), GFP_KERNEL); + if (!ptr) + return ERR_PTR(-ENOMEM); + + pwrseq = pwrseq_get_optional(dev, id); + if (!IS_ERR_OR_NULL(pwrseq)) { + *ptr = pwrseq; + devres_add(dev, ptr); + } else { + devres_free(ptr); + } + + return pwrseq; +} +EXPORT_SYMBOL_GPL(devm_pwrseq_get_optional); + +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) +{ + 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); + +static void pwrseq_dev_release(struct device *dev) +{ + struct pwrseq *pwrseq = to_pwrseq(dev); + + ida_free(&pwrseq_ida, pwrseq->id); + of_node_put(dev->of_node); + kfree(pwrseq); +} + +static struct class pwrseq_class = { + .name = "pwrseq", + .dev_release = pwrseq_dev_release, +}; + +struct pwrseq *__pwrseq_create(struct device *dev, struct module *owner, const struct pwrseq_ops *ops, void *data) +{ + struct pwrseq *pwrseq; + int ret; + + if (WARN_ON(!dev)) + return ERR_PTR(-EINVAL); + + pwrseq = kzalloc(sizeof(*pwrseq), GFP_KERNEL); + if (!pwrseq) + return ERR_PTR(-ENOMEM); + + ret = ida_alloc(&pwrseq_ida, GFP_KERNEL); + if (ret < 0) + goto free_pwrseq; + + pwrseq->id = ret; + + device_initialize(&pwrseq->dev); + + pwrseq->dev.class = &pwrseq_class; + pwrseq->dev.parent = dev; + pwrseq->dev.of_node = of_node_get(dev->of_node); + pwrseq->ops = ops; + pwrseq->owner = owner; + + dev_set_drvdata(&pwrseq->dev, data); + + ret = dev_set_name(&pwrseq->dev, "pwrseq-%s.%u", dev_name(dev), pwrseq->id); + if (ret) + goto put_dev; + + ret = device_add(&pwrseq->dev); + if (ret) + goto put_dev; + + if (pm_runtime_enabled(dev)) { + pm_runtime_enable(&pwrseq->dev); + pm_runtime_no_callbacks(&pwrseq->dev); + } + + return pwrseq; + +put_dev: + /* will call pwrseq_dev_release() to free resources */ + put_device(&pwrseq->dev); + + return ERR_PTR(ret); + +free_pwrseq: + kfree(pwrseq); + + return ERR_PTR(ret); +} +EXPORT_SYMBOL_GPL(__pwrseq_create); + +void pwrseq_destroy(struct pwrseq *pwrseq) +{ + pm_runtime_disable(&pwrseq->dev); + device_unregister(&pwrseq->dev); +} +EXPORT_SYMBOL_GPL(pwrseq_destroy); + +static void devm_pwrseq_destroy(struct device *dev, void *res) +{ + struct pwrseq *pwrseq = *(struct pwrseq **)res; + + pwrseq_destroy(pwrseq); +} + +struct pwrseq *__devm_pwrseq_create(struct device *dev, struct module *owner, const struct pwrseq_ops *ops, void *data) +{ + struct pwrseq **ptr, *pwrseq; + + ptr = devres_alloc(devm_pwrseq_destroy, sizeof(*ptr), GFP_KERNEL); + if (!ptr) + return ERR_PTR(-ENOMEM); + + pwrseq = __pwrseq_create(dev, owner, ops, data); + if (!IS_ERR(pwrseq)) { + *ptr = pwrseq; + devres_add(dev, ptr); + } else { + devres_free(ptr); + } + + return pwrseq; +} +EXPORT_SYMBOL_GPL(__devm_pwrseq_create); + +struct pwrseq_provider *__of_pwrseq_provider_register(struct device *dev, + struct module *owner, + struct pwrseq * (*of_xlate)(void *data, + struct of_phandle_args *args), + void *data) +{ + struct pwrseq_provider *pwrseq_provider; + + pwrseq_provider = kzalloc(sizeof(*pwrseq_provider), GFP_KERNEL); + if (!pwrseq_provider) + return ERR_PTR(-ENOMEM); + + pwrseq_provider->dev = dev; + pwrseq_provider->owner = owner; + pwrseq_provider->of_xlate = of_xlate; + pwrseq_provider->data = data; + + mutex_lock(&pwrseq_provider_mutex); + list_add_tail(&pwrseq_provider->list, &pwrseq_provider_list); + mutex_unlock(&pwrseq_provider_mutex); + + return pwrseq_provider; +} +EXPORT_SYMBOL_GPL(__of_pwrseq_provider_register); + +void of_pwrseq_provider_unregister(struct pwrseq_provider *pwrseq_provider) +{ + if (IS_ERR(pwrseq_provider)) + return; + + mutex_lock(&pwrseq_provider_mutex); + list_del(&pwrseq_provider->list); + kfree(pwrseq_provider); + mutex_unlock(&pwrseq_provider_mutex); +} +EXPORT_SYMBOL_GPL(of_pwrseq_provider_unregister); + +static void devm_pwrseq_provider_unregister(struct device *dev, void *res) +{ + struct pwrseq_provider *pwrseq_provider = *(struct pwrseq_provider **)res; + + of_pwrseq_provider_unregister(pwrseq_provider); +} + +struct pwrseq_provider *__devm_of_pwrseq_provider_register(struct device *dev, + struct module *owner, + struct pwrseq * (*of_xlate)(void *data, + struct of_phandle_args *args), + void *data) +{ + struct pwrseq_provider **ptr, *pwrseq_provider; + + ptr = devres_alloc(devm_pwrseq_provider_unregister, sizeof(*ptr), GFP_KERNEL); + if (!ptr) + return ERR_PTR(-ENOMEM); + + pwrseq_provider = __of_pwrseq_provider_register(dev, owner, of_xlate, data); + if (!IS_ERR(pwrseq_provider)) { + *ptr = pwrseq_provider; + devres_add(dev, ptr); + } else { + devres_free(ptr); + } + + return pwrseq_provider; +} +EXPORT_SYMBOL_GPL(__devm_of_pwrseq_provider_register); + +struct pwrseq *of_pwrseq_xlate_onecell(void *data, struct of_phandle_args *args) +{ + struct pwrseq_onecell_data *pwrseq_data = data; + unsigned int idx; + + if (args->args_count != 1) + return ERR_PTR(-EINVAL); + + idx = args->args[0]; + if (idx >= pwrseq_data->num) { + pr_err("%s: invalid index %u\n", __func__, idx); + return ERR_PTR(-EINVAL); + } + + return pwrseq_data->pwrseqs[idx]; +} + +static int __init pwrseq_core_init(void) +{ + return class_register(&pwrseq_class); +} +device_initcall(pwrseq_core_init); diff --git a/include/linux/pwrseq/consumer.h b/include/linux/pwrseq/consumer.h new file mode 100644 index 000000000000..fbcdc1fc0751 --- /dev/null +++ b/include/linux/pwrseq/consumer.h @@ -0,0 +1,88 @@ +/* SPDX-License-Identifier: GPL-2.0-or-later */ +/* + * Copyright (c) 2021 Linaro Ltd. + */ + +#ifndef __LINUX_PWRSEQ_CONSUMER_H__ +#define __LINUX_PWRSEQ_CONSUMER_H__ + +struct pwrseq; +struct device; + +#if defined(CONFIG_PWRSEQ) + +struct pwrseq *__must_check pwrseq_get(struct device *dev, const char *id); +struct pwrseq *__must_check devm_pwrseq_get(struct device *dev, const char *id); + +struct pwrseq *__must_check pwrseq_get_optional(struct device *dev, const char *id); +struct pwrseq *__must_check devm_pwrseq_get_optional(struct device *dev, const char *id); + +void pwrseq_put(struct device *dev, struct pwrseq *pwrseq); + +int pwrseq_pre_power_on(struct pwrseq *pwrseq); +int pwrseq_power_on(struct pwrseq *pwrseq); +void pwrseq_power_off(struct pwrseq *pwrseq); +void pwrseq_reset(struct pwrseq *pwrseq); + +#else + +static inline struct pwrseq *__must_check +pwrseq_get(struct device *dev, const char *id) +{ + return ERR_PTR(-ENOSYS); +} + +static inline struct pwrseq *__must_check +devm_pwrseq_get(struct device *dev, const char *id) +{ + return ERR_PTR(-ENOSYS); +} + +static inline struct pwrseq *__must_check +pwrseq_get_optional(struct device *dev, const char *id) +{ + return NULL; +} + +static inline struct pwrseq *__must_check +devm_pwrseq_get_optional(struct device *dev, const char *id) +{ + return NULL; +} + +static inline void pwrseq_put(struct device *dev, struct pwrseq *pwrseq) +{ +} + +static inline int pwrseq_pre_power_on(struct pwrseq *pwrseq) +{ + return -ENOSYS; +} + +static inline int pwrseq_power_on(struct pwrseq *pwrseq) +{ + return -ENOSYS; +} + +static inline void pwrseq_power_off(struct pwrseq *pwrseq) +{ +} + +static inline void pwrseq_reset(struct pwrseq *pwrseq) +{ +} + +#endif + +static inline int pwrseq_full_power_on(struct pwrseq *pwrseq) +{ + int ret; + + ret = pwrseq_pre_power_on(pwrseq); + if (ret) + return ret; + + return pwrseq_power_on(pwrseq); +} + +#endif /* __LINUX_PWRSEQ_CONSUMER_H__ */ diff --git a/include/linux/pwrseq/driver.h b/include/linux/pwrseq/driver.h new file mode 100644 index 000000000000..b2bc46624d7e --- /dev/null +++ b/include/linux/pwrseq/driver.h @@ -0,0 +1,75 @@ +/* SPDX-License-Identifier: GPL-2.0-or-later */ +/* + * Copyright (c) 2021 Linaro Ltd. + */ + +#ifndef __LINUX_PWRSEQ_DRIVER_H__ +#define __LINUX_PWRSEQ_DRIVER_H__ + +#include <linux/device.h> + +struct pwrseq; + +struct pwrseq_ops { + int (*pre_power_on)(struct pwrseq *pwrseq); + int (*power_on)(struct pwrseq *pwrseq); + void (*power_off)(struct pwrseq *pwrseq); + void (*reset)(struct pwrseq *pwrseq); +}; + +struct module; + +struct pwrseq { + struct device dev; + const struct pwrseq_ops *ops; + unsigned int id; + struct module *owner; +}; + +struct pwrseq *__pwrseq_create(struct device *dev, struct module *owner, const struct pwrseq_ops *ops, void *data); +struct pwrseq *__devm_pwrseq_create(struct device *dev, struct module *owner, const struct pwrseq_ops *ops, void *data); + +#define pwrseq_create(dev, ops, data) __pwrseq_create((dev), THIS_MODULE, (ops), (data)) +#define devm_pwrseq_create(dev, ops, data) __devm_pwrseq_create((dev), THIS_MODULE, (ops), (data)) + +void pwrseq_destroy(struct pwrseq *pwrseq); + +static inline void *pwrseq_get_data(struct pwrseq *pwrseq) +{ + return dev_get_drvdata(&pwrseq->dev); +} + +#define of_pwrseq_provider_register(dev, xlate, data) \ + __of_pwrseq_provider_register((dev), THIS_MODULE, (xlate), (data)) + +#define devm_of_pwrseq_provider_register(dev, xlate, data) \ + __devm_of_pwrseq_provider_register((dev), THIS_MODULE, (xlate), (data)) + +struct of_phandle_args; + +struct pwrseq_provider *__of_pwrseq_provider_register(struct device *dev, + struct module *owner, + struct pwrseq * (*of_xlate)(void *data, + struct of_phandle_args *args), + void *data); +struct pwrseq_provider *__devm_of_pwrseq_provider_register(struct device *dev, + struct module *owner, + struct pwrseq * (*of_xlate)(void *data, + struct of_phandle_args *args), + void *data); +void of_pwrseq_provider_unregister(struct pwrseq_provider *pwrseq_provider); + +static inline struct pwrseq *of_pwrseq_xlate_single(void *data, + struct of_phandle_args *args) +{ + return data; +} + +struct pwrseq_onecell_data { + unsigned int num; + struct pwrseq *pwrseqs[]; +}; + +struct pwrseq *of_pwrseq_xlate_onecell(void *data, struct of_phandle_args *args); + +#endif /* __LINUX_PWRSEQ_DRIVER_H__ */
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. Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org> --- drivers/power/Kconfig | 1 + drivers/power/Makefile | 1 + drivers/power/pwrseq/Kconfig | 11 + drivers/power/pwrseq/Makefile | 6 + drivers/power/pwrseq/core.c | 412 ++++++++++++++++++++++++++++++++ include/linux/pwrseq/consumer.h | 88 +++++++ include/linux/pwrseq/driver.h | 75 ++++++ 7 files changed, 594 insertions(+) create mode 100644 drivers/power/pwrseq/Kconfig create mode 100644 drivers/power/pwrseq/Makefile create mode 100644 drivers/power/pwrseq/core.c create mode 100644 include/linux/pwrseq/consumer.h create mode 100644 include/linux/pwrseq/driver.h