From: Yang Li <yang.li@amlogic.com>
Add power sequence for Bluetooth and Wi-Fi respectively, including chip_en
pull-up and bt_en pull-up, and generation of the 32.768 clock.
Signed-off-by: Yang Li <yang.li@amlogic.com>
---
drivers/power/sequencing/Kconfig | 7 +
drivers/power/sequencing/Makefile | 1 +
drivers/power/sequencing/pwrseq-aml-wcn.c | 209 ++++++++++++++++++++++++++++++
3 files changed, 217 insertions(+)
diff --git a/drivers/power/sequencing/Kconfig b/drivers/power/sequencing/Kconfig
index c9f1cdb66524..65d3b2c20bfb 100644
--- a/drivers/power/sequencing/Kconfig
+++ b/drivers/power/sequencing/Kconfig
@@ -26,4 +26,11 @@ config POWER_SEQUENCING_QCOM_WCN
this driver is needed for correct power control or else we'd risk not
respecting the required delays between enabling Bluetooth and WLAN.
+config POWER_SEQUENCING_AML_WCN
+ tristate "Amlogic WCN family PMU driver"
+ default m if ARCH_MESON
+ help
+ Say Y here to enable the power sequencing driver for Amlogic
+ WCN Bluetooth/WLAN chipsets.
+
endif
diff --git a/drivers/power/sequencing/Makefile b/drivers/power/sequencing/Makefile
index 2eec2df7912d..32706daf8f0f 100644
--- a/drivers/power/sequencing/Makefile
+++ b/drivers/power/sequencing/Makefile
@@ -4,3 +4,4 @@ obj-$(CONFIG_POWER_SEQUENCING) += pwrseq-core.o
pwrseq-core-y := core.o
obj-$(CONFIG_POWER_SEQUENCING_QCOM_WCN) += pwrseq-qcom-wcn.o
+obj-$(CONFIG_POWER_SEQUENCING_AML_WCN) += pwrseq-aml-wcn.o
diff --git a/drivers/power/sequencing/pwrseq-aml-wcn.c b/drivers/power/sequencing/pwrseq-aml-wcn.c
new file mode 100644
index 000000000000..6f5bfcf60b9c
--- /dev/null
+++ b/drivers/power/sequencing/pwrseq-aml-wcn.c
@@ -0,0 +1,209 @@
+// SPDX-License-Identifier: (GPL-2.0-only OR MIT)
+/*
+ * Copyright (C) 2024 Amlogic, Inc. All rights reserved
+ */
+
+#include <linux/clk.h>
+#include <linux/delay.h>
+#include <linux/device.h>
+#include <linux/mod_devicetable.h>
+#include <linux/module.h>
+#include <linux/mutex.h>
+#include <linux/of.h>
+#include <linux/gpio.h>
+#include <linux/of_gpio.h>
+#include <linux/platform_device.h>
+#include <linux/pwrseq/provider.h>
+#include <linux/string.h>
+#include <linux/types.h>
+
+struct pwrseq_aml_wcn_ctx {
+ struct pwrseq_device *pwrseq;
+ int bt_enable_gpio;
+ int chip_enable_gpio;
+ struct clk *lpo_clk;
+ unsigned int pwr_count;
+};
+
+static DEFINE_MUTEX(pwrseq_lock);
+
+static int pwrseq_aml_wcn_chip_enable(struct pwrseq_device *pwrseq)
+{
+ struct pwrseq_aml_wcn_ctx *ctx = pwrseq_device_get_drvdata(pwrseq);
+ int err;
+
+ mutex_lock(&pwrseq_lock);
+ if (ctx->pwr_count == 0) {
+ gpio_request(ctx->chip_enable_gpio, "chip-enable-gpios");
+ gpio_direction_output(ctx->chip_enable_gpio, 1);
+ gpio_free(ctx->chip_enable_gpio);
+
+ if (!IS_ERR(ctx->lpo_clk)) {
+ err = clk_prepare_enable(ctx->lpo_clk);
+ if (err) {
+ mutex_unlock(&pwrseq_lock);
+ return err;
+ }
+ }
+ }
+
+ ctx->pwr_count++;
+ mutex_unlock(&pwrseq_lock);
+ return 0;
+}
+
+static int pwrseq_aml_wcn_chip_disable(struct pwrseq_device *pwrseq)
+{
+ struct pwrseq_aml_wcn_ctx *ctx = pwrseq_device_get_drvdata(pwrseq);
+
+ mutex_lock(&pwrseq_lock);
+ if (--ctx->pwr_count == 0) {
+ gpio_request(ctx->chip_enable_gpio, "chip-enable-gpios");
+ gpio_direction_output(ctx->chip_enable_gpio, 0);
+ gpio_free(ctx->chip_enable_gpio);
+
+ if (!IS_ERR(ctx->lpo_clk))
+ clk_disable_unprepare(ctx->lpo_clk);
+ }
+
+ mutex_unlock(&pwrseq_lock);
+ return 0;
+}
+
+static const struct pwrseq_unit_data pwrseq_aml_wcn_chip_power_unit_data = {
+ .name = "chip-enable",
+ .enable = pwrseq_aml_wcn_chip_enable,
+ .disable = pwrseq_aml_wcn_chip_disable,
+};
+
+static const struct pwrseq_unit_data *pwrseq_aml_wcn_unit_deps[] = {
+ &pwrseq_aml_wcn_chip_power_unit_data,
+ NULL
+};
+
+static int pwrseq_aml_wcn_bt_enable(struct pwrseq_device *pwrseq)
+{
+ struct pwrseq_aml_wcn_ctx *ctx = pwrseq_device_get_drvdata(pwrseq);
+
+ gpio_request(ctx->bt_enable_gpio, "bt-enable-gpios");
+ gpio_direction_output(ctx->bt_enable_gpio, 1);
+ gpio_free(ctx->bt_enable_gpio);
+
+ /* wait 100ms for bluetooth controller power on */
+ msleep(100);
+
+ return 0;
+}
+
+static int pwrseq_aml_wcn_bt_disable(struct pwrseq_device *pwrseq)
+{
+ struct pwrseq_aml_wcn_ctx *ctx = pwrseq_device_get_drvdata(pwrseq);
+
+ gpio_request(ctx->bt_enable_gpio, "bt-enable-gpios");
+ gpio_direction_output(ctx->bt_enable_gpio, 0);
+ gpio_free(ctx->bt_enable_gpio);
+
+ return 0;
+}
+
+static const struct pwrseq_unit_data pwrseq_aml_wcn_bt_unit_data = {
+ .name = "bluetooth-enable",
+ .deps = pwrseq_aml_wcn_unit_deps,
+ .enable = pwrseq_aml_wcn_bt_enable,
+ .disable = pwrseq_aml_wcn_bt_disable,
+};
+
+static const struct pwrseq_unit_data pwrseq_aml_wcn_wlan_unit_data = {
+ .name = "wlan-enable",
+ .deps = pwrseq_aml_wcn_unit_deps,
+};
+
+static const struct pwrseq_target_data pwrseq_aml_wcn_bt_target_data = {
+ .name = "bluetooth",
+ .unit = &pwrseq_aml_wcn_bt_unit_data,
+};
+
+static const struct pwrseq_target_data pwrseq_aml_wcn_wlan_target_data = {
+ .name = "wlan",
+ .unit = &pwrseq_aml_wcn_wlan_unit_data,
+};
+
+static const struct pwrseq_target_data *pwrseq_aml_wcn_targets[] = {
+ &pwrseq_aml_wcn_bt_target_data,
+ &pwrseq_aml_wcn_wlan_target_data,
+ NULL
+};
+
+static int pwrseq_aml_wcn_match(struct pwrseq_device *pwrseq,
+ struct device *dev)
+{
+ struct device_node *dev_node = dev->of_node;
+
+ if (!of_property_present(dev_node, "amlogic,wcn-pwrseq"))
+ return 0;
+
+ return 1;
+}
+
+static int pwrseq_aml_wcn_probe(struct platform_device *pdev)
+{
+ struct device *dev = &pdev->dev;
+ struct pwrseq_aml_wcn_ctx *ctx;
+ struct pwrseq_config config;
+
+ ctx = devm_kzalloc(dev, sizeof(*ctx), GFP_KERNEL);
+ if (!ctx)
+ return -ENOMEM;
+
+ ctx->bt_enable_gpio = of_get_named_gpio(dev->of_node,
+ "amlogic,bt-enable-gpios", 0);
+ if (!gpio_is_valid(ctx->bt_enable_gpio))
+ return dev_err_probe(dev, ctx->bt_enable_gpio,
+ "Failed to get the bt enable GPIO");
+
+ ctx->chip_enable_gpio = of_get_named_gpio(dev->of_node,
+ "amlogic,chip-enable-gpios", 0);
+ if (!gpio_is_valid(ctx->chip_enable_gpio))
+ return dev_err_probe(dev, ctx->bt_enable_gpio,
+ "Failed to get the chip enable GPIO");
+
+ ctx->lpo_clk = devm_clk_get_optional(dev, NULL);
+ if (IS_ERR(ctx->lpo_clk))
+ return dev_err_probe(dev, PTR_ERR(ctx->lpo_clk),
+ "Failed to get the clock source");
+
+ memset(&config, 0, sizeof(config));
+
+ config.parent = dev;
+ config.owner = THIS_MODULE;
+ config.drvdata = ctx;
+ config.match = pwrseq_aml_wcn_match;
+ config.targets = pwrseq_aml_wcn_targets;
+
+ ctx->pwr_count = 0;
+ ctx->pwrseq = devm_pwrseq_device_register(dev, &config);
+ if (IS_ERR(ctx->pwrseq))
+ return dev_err_probe(dev, PTR_ERR(ctx->pwrseq),
+ "Failed to register the power sequencer\n");
+
+ return 0;
+}
+
+static const struct of_device_id pwrseq_aml_wcn_of_match[] = {
+ { .compatible = "amlogic,w155s2-pwrseq" },
+ { /* sentinel */ },
+};
+MODULE_DEVICE_TABLE(of, pwrseq_aml_wcn_of_match);
+
+static struct platform_driver pwrseq_aml_wcn_driver = {
+ .driver = {
+ .name = "pwrseq-aml_wcn",
+ .of_match_table = pwrseq_aml_wcn_of_match,
+ },
+ .probe = pwrseq_aml_wcn_probe,
+};
+module_platform_driver(pwrseq_aml_wcn_driver);
+
+MODULE_AUTHOR("Yang Li <yang.li@amlogic.com>");
+MODULE_DESCRIPTION("Amlogic WCN PMU power sequencing driver");
+MODULE_LICENSE("GPL");
--
2.42.0On 05/07/2024 13:13, Yang Li via B4 Relay wrote:
> From: Yang Li <yang.li@amlogic.com>
>
> Add power sequence for Bluetooth and Wi-Fi respectively, including chip_en
> pull-up and bt_en pull-up, and generation of the 32.768 clock.
>
> Signed-off-by: Yang Li <yang.li@amlogic.com>
> +#include <linux/clk.h>
> +#include <linux/delay.h>
> +#include <linux/device.h>
> +#include <linux/mod_devicetable.h>
> +#include <linux/module.h>
> +#include <linux/mutex.h>
> +#include <linux/of.h>
> +#include <linux/gpio.h>
> +#include <linux/of_gpio.h>
> +#include <linux/platform_device.h>
> +#include <linux/pwrseq/provider.h>
> +#include <linux/string.h>
> +#include <linux/types.h>
> +
> +struct pwrseq_aml_wcn_ctx {
> + struct pwrseq_device *pwrseq;
> + int bt_enable_gpio;
Why? It's never used... or you use wrong API. Confusing code.
> + int chip_enable_gpio;
> + struct clk *lpo_clk;
> + unsigned int pwr_count;
> +};
> +
> +static DEFINE_MUTEX(pwrseq_lock);
Why this is not part of structure above?
> +
...
> +
> +static int pwrseq_aml_wcn_match(struct pwrseq_device *pwrseq,
> + struct device *dev)
> +{
> + struct device_node *dev_node = dev->of_node;
> +
> + if (!of_property_present(dev_node, "amlogic,wcn-pwrseq"))
You cannot have undocumented properties, sorry, that's a NAK.
> + return 0;
> +
> + return 1;
> +}
> +
> +static int pwrseq_aml_wcn_probe(struct platform_device *pdev)
> +{
> + struct device *dev = &pdev->dev;
> + struct pwrseq_aml_wcn_ctx *ctx;
> + struct pwrseq_config config;
> +
> + ctx = devm_kzalloc(dev, sizeof(*ctx), GFP_KERNEL);
> + if (!ctx)
> + return -ENOMEM;
> +
> + ctx->bt_enable_gpio = of_get_named_gpio(dev->of_node,
> + "amlogic,bt-enable-gpios", 0);
> + if (!gpio_is_valid(ctx->bt_enable_gpio))
> + return dev_err_probe(dev, ctx->bt_enable_gpio,
> + "Failed to get the bt enable GPIO");
> +
> + ctx->chip_enable_gpio = of_get_named_gpio(dev->of_node,
> + "amlogic,chip-enable-gpios", 0);
> + if (!gpio_is_valid(ctx->chip_enable_gpio))
> + return dev_err_probe(dev, ctx->bt_enable_gpio,
> + "Failed to get the chip enable GPIO");
> +
> + ctx->lpo_clk = devm_clk_get_optional(dev, NULL);
Clock is not optional, according to you binding.
> + if (IS_ERR(ctx->lpo_clk))
> + return dev_err_probe(dev, PTR_ERR(ctx->lpo_clk),
> + "Failed to get the clock source");
> +
> + memset(&config, 0, sizeof(config));
Just initialize it on the stack with 0.
Best regards,
Krzysztof
Dear Krzysztof
Thanks for your comments.
> On 05/07/2024 13:13, Yang Li via B4 Relay wrote:
>> From: Yang Li <yang.li@amlogic.com>
>>
>> Add power sequence for Bluetooth and Wi-Fi respectively, including chip_en
>> pull-up and bt_en pull-up, and generation of the 32.768 clock.
>>
>> Signed-off-by: Yang Li <yang.li@amlogic.com>
>> +#include <linux/clk.h>
>> +#include <linux/delay.h>
>> +#include <linux/device.h>
>> +#include <linux/mod_devicetable.h>
>> +#include <linux/module.h>
>> +#include <linux/mutex.h>
>> +#include <linux/of.h>
>> +#include <linux/gpio.h>
>> +#include <linux/of_gpio.h>
>> +#include <linux/platform_device.h>
>> +#include <linux/pwrseq/provider.h>
>> +#include <linux/string.h>
>> +#include <linux/types.h>
>> +
>> +struct pwrseq_aml_wcn_ctx {
>> + struct pwrseq_device *pwrseq;
>> + int bt_enable_gpio;
> Why? It's never used... or you use wrong API. Confusing code.
Well, I will used the "struct gpio_desc" replace current method.
>
>> + int chip_enable_gpio;
>> + struct clk *lpo_clk;
>> + unsigned int pwr_count;
>> +};
>> +
>> +static DEFINE_MUTEX(pwrseq_lock);
> Why this is not part of structure above?
Sorry, I referenced some outdated examples.
And I will put it in structure of pwrseq_aml_wcn_ctx.
>> +
>
> ...
>
>> +
>> +static int pwrseq_aml_wcn_match(struct pwrseq_device *pwrseq,
>> + struct device *dev)
>> +{
>> + struct device_node *dev_node = dev->of_node;
>> +
>> + if (!of_property_present(dev_node, "amlogic,wcn-pwrseq"))
> You cannot have undocumented properties, sorry, that's a NAK.
About the match () function I also have some doubts, the
drivers/power/sequence/core.c requirements need to be defined match ()
function is used to determine whether a potential consumers actually
related to the sequencer. So, I need to add a meaningless node
"amlogic,wcn-pwrseq" to both the consumer dt-binding and the provider
dt-binding.
Right now, I add "amlogic,wcn-pwrseq" in binding file of
"amlogic,w155s2-bt.yaml" only, may I need to add this properties
("amlogic,wcn-pwrseq") in the binding file of "amlogic,w155s2-pwrseq.yaml"?
>> + return 0;
>> +
>> + return 1;
>> +}
>> +
>> +static int pwrseq_aml_wcn_probe(struct platform_device *pdev)
>> +{
>> + struct device *dev = &pdev->dev;
>> + struct pwrseq_aml_wcn_ctx *ctx;
>> + struct pwrseq_config config;
>> +
>> + ctx = devm_kzalloc(dev, sizeof(*ctx), GFP_KERNEL);
>> + if (!ctx)
>> + return -ENOMEM;
>> +
>> + ctx->bt_enable_gpio = of_get_named_gpio(dev->of_node,
>> + "amlogic,bt-enable-gpios", 0);
>> + if (!gpio_is_valid(ctx->bt_enable_gpio))
>> + return dev_err_probe(dev, ctx->bt_enable_gpio,
>> + "Failed to get the bt enable GPIO");
>> +
>> + ctx->chip_enable_gpio = of_get_named_gpio(dev->of_node,
>> + "amlogic,chip-enable-gpios", 0);
>> + if (!gpio_is_valid(ctx->chip_enable_gpio))
>> + return dev_err_probe(dev, ctx->bt_enable_gpio,
>> + "Failed to get the chip enable GPIO");
>> +
>> + ctx->lpo_clk = devm_clk_get_optional(dev, NULL);
> Clock is not optional, according to you binding.
Yes, I will used API of devm_clk_get(dev, "extclk") to relace it.
>
>> + if (IS_ERR(ctx->lpo_clk))
>> + return dev_err_probe(dev, PTR_ERR(ctx->lpo_clk),
>> + "Failed to get the clock source");
>> +
>> + memset(&config, 0, sizeof(config));
> Just initialize it on the stack with 0.
Okay, I will delete this line and set config to zero when initializing.
>
>
>
> Best regards,
> Krzysztof
>
On Fri, Jul 5, 2024 at 1:13 PM Yang Li via B4 Relay
<devnull+yang.li.amlogic.com@kernel.org> wrote:
>
> From: Yang Li <yang.li@amlogic.com>
>
> Add power sequence for Bluetooth and Wi-Fi respectively, including chip_en
> pull-up and bt_en pull-up, and generation of the 32.768 clock.
>
> Signed-off-by: Yang Li <yang.li@amlogic.com>
> ---
> drivers/power/sequencing/Kconfig | 7 +
> drivers/power/sequencing/Makefile | 1 +
> drivers/power/sequencing/pwrseq-aml-wcn.c | 209 ++++++++++++++++++++++++++++++
> 3 files changed, 217 insertions(+)
>
> diff --git a/drivers/power/sequencing/Kconfig b/drivers/power/sequencing/Kconfig
> index c9f1cdb66524..65d3b2c20bfb 100644
> --- a/drivers/power/sequencing/Kconfig
> +++ b/drivers/power/sequencing/Kconfig
> @@ -26,4 +26,11 @@ config POWER_SEQUENCING_QCOM_WCN
> this driver is needed for correct power control or else we'd risk not
> respecting the required delays between enabling Bluetooth and WLAN.
>
> +config POWER_SEQUENCING_AML_WCN
> + tristate "Amlogic WCN family PMU driver"
> + default m if ARCH_MESON
> + help
> + Say Y here to enable the power sequencing driver for Amlogic
> + WCN Bluetooth/WLAN chipsets.
> +
> endif
> diff --git a/drivers/power/sequencing/Makefile b/drivers/power/sequencing/Makefile
> index 2eec2df7912d..32706daf8f0f 100644
> --- a/drivers/power/sequencing/Makefile
> +++ b/drivers/power/sequencing/Makefile
> @@ -4,3 +4,4 @@ obj-$(CONFIG_POWER_SEQUENCING) += pwrseq-core.o
> pwrseq-core-y := core.o
>
> obj-$(CONFIG_POWER_SEQUENCING_QCOM_WCN) += pwrseq-qcom-wcn.o
> +obj-$(CONFIG_POWER_SEQUENCING_AML_WCN) += pwrseq-aml-wcn.o
> diff --git a/drivers/power/sequencing/pwrseq-aml-wcn.c b/drivers/power/sequencing/pwrseq-aml-wcn.c
> new file mode 100644
> index 000000000000..6f5bfcf60b9c
> --- /dev/null
> +++ b/drivers/power/sequencing/pwrseq-aml-wcn.c
> @@ -0,0 +1,209 @@
> +// SPDX-License-Identifier: (GPL-2.0-only OR MIT)
> +/*
> + * Copyright (C) 2024 Amlogic, Inc. All rights reserved
> + */
> +
> +#include <linux/clk.h>
> +#include <linux/delay.h>
> +#include <linux/device.h>
> +#include <linux/mod_devicetable.h>
> +#include <linux/module.h>
> +#include <linux/mutex.h>
> +#include <linux/of.h>
> +#include <linux/gpio.h>
Please see line 5 in this file.
> +#include <linux/of_gpio.h>
You don't need this either.
> +#include <linux/platform_device.h>
> +#include <linux/pwrseq/provider.h>
> +#include <linux/string.h>
> +#include <linux/types.h>
> +
> +struct pwrseq_aml_wcn_ctx {
> + struct pwrseq_device *pwrseq;
> + int bt_enable_gpio;
> + int chip_enable_gpio;
> + struct clk *lpo_clk;
> + unsigned int pwr_count;
> +};
> +
> +static DEFINE_MUTEX(pwrseq_lock);
> +
Why is this global?
> +static int pwrseq_aml_wcn_chip_enable(struct pwrseq_device *pwrseq)
> +{
> + struct pwrseq_aml_wcn_ctx *ctx = pwrseq_device_get_drvdata(pwrseq);
> + int err;
> +
> + mutex_lock(&pwrseq_lock);
Please use guard() from linux/cleanup.h.
> + if (ctx->pwr_count == 0) {
> + gpio_request(ctx->chip_enable_gpio, "chip-enable-gpios");
> + gpio_direction_output(ctx->chip_enable_gpio, 1);
> + gpio_free(ctx->chip_enable_gpio);
Not only are these legacy APIs but they are also used wrong. You
almost never want to release the GPIO after setting the direction as
someone else may grab it and use it.
> +
> + if (!IS_ERR(ctx->lpo_clk)) {
> + err = clk_prepare_enable(ctx->lpo_clk);
> + if (err) {
> + mutex_unlock(&pwrseq_lock);
> + return err;
> + }
> + }
> + }
> +
> + ctx->pwr_count++;
> + mutex_unlock(&pwrseq_lock);
> + return 0;
> +}
> +
> +static int pwrseq_aml_wcn_chip_disable(struct pwrseq_device *pwrseq)
> +{
> + struct pwrseq_aml_wcn_ctx *ctx = pwrseq_device_get_drvdata(pwrseq);
> +
> + mutex_lock(&pwrseq_lock);
> + if (--ctx->pwr_count == 0) {
> + gpio_request(ctx->chip_enable_gpio, "chip-enable-gpios");
> + gpio_direction_output(ctx->chip_enable_gpio, 0);
> + gpio_free(ctx->chip_enable_gpio);
> +
> + if (!IS_ERR(ctx->lpo_clk))
> + clk_disable_unprepare(ctx->lpo_clk);
> + }
> +
> + mutex_unlock(&pwrseq_lock);
> + return 0;
> +}
> +
> +static const struct pwrseq_unit_data pwrseq_aml_wcn_chip_power_unit_data = {
> + .name = "chip-enable",
> + .enable = pwrseq_aml_wcn_chip_enable,
> + .disable = pwrseq_aml_wcn_chip_disable,
> +};
> +
> +static const struct pwrseq_unit_data *pwrseq_aml_wcn_unit_deps[] = {
> + &pwrseq_aml_wcn_chip_power_unit_data,
> + NULL
> +};
> +
> +static int pwrseq_aml_wcn_bt_enable(struct pwrseq_device *pwrseq)
> +{
> + struct pwrseq_aml_wcn_ctx *ctx = pwrseq_device_get_drvdata(pwrseq);
> +
> + gpio_request(ctx->bt_enable_gpio, "bt-enable-gpios");
> + gpio_direction_output(ctx->bt_enable_gpio, 1);
> + gpio_free(ctx->bt_enable_gpio);
> +
> + /* wait 100ms for bluetooth controller power on */
> + msleep(100);
> +
> + return 0;
> +}
> +
> +static int pwrseq_aml_wcn_bt_disable(struct pwrseq_device *pwrseq)
> +{
> + struct pwrseq_aml_wcn_ctx *ctx = pwrseq_device_get_drvdata(pwrseq);
> +
> + gpio_request(ctx->bt_enable_gpio, "bt-enable-gpios");
> + gpio_direction_output(ctx->bt_enable_gpio, 0);
> + gpio_free(ctx->bt_enable_gpio);
> +
> + return 0;
> +}
> +
> +static const struct pwrseq_unit_data pwrseq_aml_wcn_bt_unit_data = {
> + .name = "bluetooth-enable",
> + .deps = pwrseq_aml_wcn_unit_deps,
> + .enable = pwrseq_aml_wcn_bt_enable,
> + .disable = pwrseq_aml_wcn_bt_disable,
> +};
> +
> +static const struct pwrseq_unit_data pwrseq_aml_wcn_wlan_unit_data = {
> + .name = "wlan-enable",
> + .deps = pwrseq_aml_wcn_unit_deps,
> +};
> +
> +static const struct pwrseq_target_data pwrseq_aml_wcn_bt_target_data = {
> + .name = "bluetooth",
> + .unit = &pwrseq_aml_wcn_bt_unit_data,
> +};
> +
> +static const struct pwrseq_target_data pwrseq_aml_wcn_wlan_target_data = {
> + .name = "wlan",
> + .unit = &pwrseq_aml_wcn_wlan_unit_data,
> +};
> +
> +static const struct pwrseq_target_data *pwrseq_aml_wcn_targets[] = {
> + &pwrseq_aml_wcn_bt_target_data,
> + &pwrseq_aml_wcn_wlan_target_data,
> + NULL
> +};
> +
> +static int pwrseq_aml_wcn_match(struct pwrseq_device *pwrseq,
> + struct device *dev)
> +{
> + struct device_node *dev_node = dev->of_node;
> +
> + if (!of_property_present(dev_node, "amlogic,wcn-pwrseq"))
> + return 0;
> +
You must never reference the notion of power sequencing in the DT.
Please take a look at the pwrseq-qcom-wcn driver where we model the
PMU with its regulators and then parse them in match() to figure out
if we have the right thing or not.
> + return 1;
> +}
> +
> +static int pwrseq_aml_wcn_probe(struct platform_device *pdev)
> +{
> + struct device *dev = &pdev->dev;
> + struct pwrseq_aml_wcn_ctx *ctx;
> + struct pwrseq_config config;
> +
> + ctx = devm_kzalloc(dev, sizeof(*ctx), GFP_KERNEL);
> + if (!ctx)
> + return -ENOMEM;
> +
> + ctx->bt_enable_gpio = of_get_named_gpio(dev->of_node,
> + "amlogic,bt-enable-gpios", 0);
> + if (!gpio_is_valid(ctx->bt_enable_gpio))
> + return dev_err_probe(dev, ctx->bt_enable_gpio,
> + "Failed to get the bt enable GPIO");
> +
> + ctx->chip_enable_gpio = of_get_named_gpio(dev->of_node,
> + "amlogic,chip-enable-gpios", 0);
You don't need the OF variant. Use the regular devm_gpiod_get(). You
also forgot to release it but the devres variant will take care of it.
> + if (!gpio_is_valid(ctx->chip_enable_gpio))
> + return dev_err_probe(dev, ctx->bt_enable_gpio,
> + "Failed to get the chip enable GPIO");
Wat
> +
> + ctx->lpo_clk = devm_clk_get_optional(dev, NULL);
> + if (IS_ERR(ctx->lpo_clk))
> + return dev_err_probe(dev, PTR_ERR(ctx->lpo_clk),
> + "Failed to get the clock source");
> +
> + memset(&config, 0, sizeof(config));
> +
> + config.parent = dev;
> + config.owner = THIS_MODULE;
> + config.drvdata = ctx;
> + config.match = pwrseq_aml_wcn_match;
> + config.targets = pwrseq_aml_wcn_targets;
> +
> + ctx->pwr_count = 0;
> + ctx->pwrseq = devm_pwrseq_device_register(dev, &config);
> + if (IS_ERR(ctx->pwrseq))
> + return dev_err_probe(dev, PTR_ERR(ctx->pwrseq),
> + "Failed to register the power sequencer\n");
> +
> + return 0;
> +}
> +
> +static const struct of_device_id pwrseq_aml_wcn_of_match[] = {
> + { .compatible = "amlogic,w155s2-pwrseq" },
> + { /* sentinel */ },
> +};
> +MODULE_DEVICE_TABLE(of, pwrseq_aml_wcn_of_match);
> +
> +static struct platform_driver pwrseq_aml_wcn_driver = {
> + .driver = {
> + .name = "pwrseq-aml_wcn",
> + .of_match_table = pwrseq_aml_wcn_of_match,
> + },
> + .probe = pwrseq_aml_wcn_probe,
> +};
> +module_platform_driver(pwrseq_aml_wcn_driver);
> +
> +MODULE_AUTHOR("Yang Li <yang.li@amlogic.com>");
> +MODULE_DESCRIPTION("Amlogic WCN PMU power sequencing driver");
> +MODULE_LICENSE("GPL");
>
> --
> 2.42.0
>
>
Bart
On 2024/7/5 21:46, Bartosz Golaszewski wrote:
> On Fri, Jul 5, 2024 at 1:13 PM Yang Li via B4 Relay
> <devnull+yang.li.amlogic.com@kernel.org> wrote:
>> From: Yang Li<yang.li@amlogic.com>
>>
>> Add power sequence for Bluetooth and Wi-Fi respectively, including chip_en
>> pull-up and bt_en pull-up, and generation of the 32.768 clock.
>>
>> Signed-off-by: Yang Li<yang.li@amlogic.com>
>> ---
>> drivers/power/sequencing/Kconfig | 7 +
>> drivers/power/sequencing/Makefile | 1 +
>> drivers/power/sequencing/pwrseq-aml-wcn.c | 209 ++++++++++++++++++++++++++++++
>> 3 files changed, 217 insertions(+)
>>
>> diff --git a/drivers/power/sequencing/Kconfig b/drivers/power/sequencing/Kconfig
>> index c9f1cdb66524..65d3b2c20bfb 100644
>> --- a/drivers/power/sequencing/Kconfig
>> +++ b/drivers/power/sequencing/Kconfig
>> @@ -26,4 +26,11 @@ config POWER_SEQUENCING_QCOM_WCN
>> this driver is needed for correct power control or else we'd risk not
>> respecting the required delays between enabling Bluetooth and WLAN.
>>
>> +config POWER_SEQUENCING_AML_WCN
>> + tristate "Amlogic WCN family PMU driver"
>> + default m if ARCH_MESON
>> + help
>> + Say Y here to enable the power sequencing driver for Amlogic
>> + WCN Bluetooth/WLAN chipsets.
>> +
>> endif
>> diff --git a/drivers/power/sequencing/Makefile b/drivers/power/sequencing/Makefile
>> index 2eec2df7912d..32706daf8f0f 100644
>> --- a/drivers/power/sequencing/Makefile
>> +++ b/drivers/power/sequencing/Makefile
>> @@ -4,3 +4,4 @@ obj-$(CONFIG_POWER_SEQUENCING) += pwrseq-core.o
>> pwrseq-core-y := core.o
>>
>> obj-$(CONFIG_POWER_SEQUENCING_QCOM_WCN) += pwrseq-qcom-wcn.o
>> +obj-$(CONFIG_POWER_SEQUENCING_AML_WCN) += pwrseq-aml-wcn.o
>> diff --git a/drivers/power/sequencing/pwrseq-aml-wcn.c b/drivers/power/sequencing/pwrseq-aml-wcn.c
>> new file mode 100644
>> index 000000000000..6f5bfcf60b9c
>> --- /dev/null
>> +++ b/drivers/power/sequencing/pwrseq-aml-wcn.c
>> @@ -0,0 +1,209 @@
>> +// SPDX-License-Identifier: (GPL-2.0-only OR MIT)
>> +/*
>> + * Copyright (C) 2024 Amlogic, Inc. All rights reserved
>> + */
>> +
>> +#include <linux/clk.h>
>> +#include <linux/delay.h>
>> +#include <linux/device.h>
>> +#include <linux/mod_devicetable.h>
>> +#include <linux/module.h>
>> +#include <linux/mutex.h>
>> +#include <linux/of.h>
>> +#include <linux/gpio.h>
> Please see line 5 in this file.
I got it, I will remove this line, and include linux/gpio/consumer.h.
>> +#include <linux/of_gpio.h>
> You don't need this either.
Yes, I will remove it.
>> +#include <linux/platform_device.h>
>> +#include <linux/pwrseq/provider.h>
>> +#include <linux/string.h>
>> +#include <linux/types.h>
>> +
>> +struct pwrseq_aml_wcn_ctx {
>> + struct pwrseq_device *pwrseq;
>> + int bt_enable_gpio;
>> + int chip_enable_gpio;
>> + struct clk *lpo_clk;
>> + unsigned int pwr_count;
>> +};
>> +
>> +static DEFINE_MUTEX(pwrseq_lock);
>> +
> Why is this global?
Okay, I will add it to structure of pwrseq_aml_wcn_ctx .
>> +static int pwrseq_aml_wcn_chip_enable(struct pwrseq_device *pwrseq)
>> +{
>> + struct pwrseq_aml_wcn_ctx *ctx = pwrseq_device_get_drvdata(pwrseq);
>> + int err;
>> +
>> + mutex_lock(&pwrseq_lock);
> Please use guard() from linux/cleanup.h.
Well, I will use guard(mutex)(&pwrse_lock) to replace
mutex_lock(&pwrseq_lock).
>> + if (ctx->pwr_count == 0) {
>> + gpio_request(ctx->chip_enable_gpio, "chip-enable-gpios");
>> + gpio_direction_output(ctx->chip_enable_gpio, 1);
>> + gpio_free(ctx->chip_enable_gpio);
> Not only are these legacy APIs but they are also used wrong. You
> almost never want to release the GPIO after setting the direction as
> someone else may grab it and use it.
Okay, I will use consumer API of devm_gpiod_get() to replace them.
>> +
>> + if (!IS_ERR(ctx->lpo_clk)) {
>> + err = clk_prepare_enable(ctx->lpo_clk);
>> + if (err) {
>> + mutex_unlock(&pwrseq_lock);
>> + return err;
>> + }
>> + }
>> + }
>> +
>> + ctx->pwr_count++;
>> + mutex_unlock(&pwrseq_lock);
>> + return 0;
>> +}
>> +
>> +static int pwrseq_aml_wcn_chip_disable(struct pwrseq_device *pwrseq)
>> +{
>> + struct pwrseq_aml_wcn_ctx *ctx = pwrseq_device_get_drvdata(pwrseq);
>> +
>> + mutex_lock(&pwrseq_lock);
>> + if (--ctx->pwr_count == 0) {
>> + gpio_request(ctx->chip_enable_gpio, "chip-enable-gpios");
>> + gpio_direction_output(ctx->chip_enable_gpio, 0);
>> + gpio_free(ctx->chip_enable_gpio);
>> +
>> + if (!IS_ERR(ctx->lpo_clk))
>> + clk_disable_unprepare(ctx->lpo_clk);
>> + }
>> +
>> + mutex_unlock(&pwrseq_lock);
>> + return 0;
>> +}
>> +
>> +static const struct pwrseq_unit_data pwrseq_aml_wcn_chip_power_unit_data = {
>> + .name = "chip-enable",
>> + .enable = pwrseq_aml_wcn_chip_enable,
>> + .disable = pwrseq_aml_wcn_chip_disable,
>> +};
>> +
>> +static const struct pwrseq_unit_data *pwrseq_aml_wcn_unit_deps[] = {
>> + &pwrseq_aml_wcn_chip_power_unit_data,
>> + NULL
>> +};
>> +
>> +static int pwrseq_aml_wcn_bt_enable(struct pwrseq_device *pwrseq)
>> +{
>> + struct pwrseq_aml_wcn_ctx *ctx = pwrseq_device_get_drvdata(pwrseq);
>> +
>> + gpio_request(ctx->bt_enable_gpio, "bt-enable-gpios");
>> + gpio_direction_output(ctx->bt_enable_gpio, 1);
>> + gpio_free(ctx->bt_enable_gpio);
>> +
>> + /* wait 100ms for bluetooth controller power on */
>> + msleep(100);
>> +
>> + return 0;
>> +}
>> +
>> +static int pwrseq_aml_wcn_bt_disable(struct pwrseq_device *pwrseq)
>> +{
>> + struct pwrseq_aml_wcn_ctx *ctx = pwrseq_device_get_drvdata(pwrseq);
>> +
>> + gpio_request(ctx->bt_enable_gpio, "bt-enable-gpios");
>> + gpio_direction_output(ctx->bt_enable_gpio, 0);
>> + gpio_free(ctx->bt_enable_gpio);
>> +
>> + return 0;
>> +}
>> +
>> +static const struct pwrseq_unit_data pwrseq_aml_wcn_bt_unit_data = {
>> + .name = "bluetooth-enable",
>> + .deps = pwrseq_aml_wcn_unit_deps,
>> + .enable = pwrseq_aml_wcn_bt_enable,
>> + .disable = pwrseq_aml_wcn_bt_disable,
>> +};
>> +
>> +static const struct pwrseq_unit_data pwrseq_aml_wcn_wlan_unit_data = {
>> + .name = "wlan-enable",
>> + .deps = pwrseq_aml_wcn_unit_deps,
>> +};
>> +
>> +static const struct pwrseq_target_data pwrseq_aml_wcn_bt_target_data = {
>> + .name = "bluetooth",
>> + .unit = &pwrseq_aml_wcn_bt_unit_data,
>> +};
>> +
>> +static const struct pwrseq_target_data pwrseq_aml_wcn_wlan_target_data = {
>> + .name = "wlan",
>> + .unit = &pwrseq_aml_wcn_wlan_unit_data,
>> +};
>> +
>> +static const struct pwrseq_target_data *pwrseq_aml_wcn_targets[] = {
>> + &pwrseq_aml_wcn_bt_target_data,
>> + &pwrseq_aml_wcn_wlan_target_data,
>> + NULL
>> +};
>> +
>> +static int pwrseq_aml_wcn_match(struct pwrseq_device *pwrseq,
>> + struct device *dev)
>> +{
>> + struct device_node *dev_node = dev->of_node;
>> +
>> + if (!of_property_present(dev_node, "amlogic,wcn-pwrseq"))
>> + return 0;
>> +
> You must never reference the notion of power sequencing in the DT.
> Please take a look at the pwrseq-qcom-wcn driver where we model the
> PMU with its regulators and then parse them in match() to figure out
> if we have the right thing or not.
There is some different between pwrseq-aml-wcn and pwrseq-qcom-wcn,
pwrseq-aml-wcn device is abstracted to manage the chip-en pin, bt-en
pin, and 32.768KHz clock. The drivers/power/sequence/core.c requirements
need to be defined match () function is used to determine whether a
potential consumers actually related to the sequencer. So, I need to add
a meaningless node "amlogic,wcn-pwrseq" to both the consumer dt-binding
and the provider dt-binding.
Right now, I add "amlogic,wcn-pwrseq" in binding file of
"amlogic,w155s2-bt.yaml" only, may I need to add this properties
("amlogic,wcn-pwrseq") in the binding file of "amlogic,w155s2-pwrseq.yaml"?
Or there are any others way to fixed this issue please let me know.
>> + return 1;
>> +}
>> +
>> +static int pwrseq_aml_wcn_probe(struct platform_device *pdev)
>> +{
>> + struct device *dev = &pdev->dev;
>> + struct pwrseq_aml_wcn_ctx *ctx;
>> + struct pwrseq_config config;
>> +
>> + ctx = devm_kzalloc(dev, sizeof(*ctx), GFP_KERNEL);
>> + if (!ctx)
>> + return -ENOMEM;
>> +
>> + ctx->bt_enable_gpio = of_get_named_gpio(dev->of_node,
>> + "amlogic,bt-enable-gpios", 0);
>> + if (!gpio_is_valid(ctx->bt_enable_gpio))
>> + return dev_err_probe(dev, ctx->bt_enable_gpio,
>> + "Failed to get the bt enable GPIO");
>> +
>> + ctx->chip_enable_gpio = of_get_named_gpio(dev->of_node,
>> + "amlogic,chip-enable-gpios", 0);
> You don't need the OF variant. Use the regular devm_gpiod_get(). You
> also forgot to release it but the devres variant will take care of it.
Well, I will do it.
>
>> + if (!gpio_is_valid(ctx->chip_enable_gpio))
>> + return dev_err_probe(dev, ctx->bt_enable_gpio,
>> + "Failed to get the chip enable GPIO");
> Wat
I got it, and I will fix it.
>
>> +
>> + ctx->lpo_clk = devm_clk_get_optional(dev, NULL);
>> + if (IS_ERR(ctx->lpo_clk))
>> + return dev_err_probe(dev, PTR_ERR(ctx->lpo_clk),
>> + "Failed to get the clock source");
>> +
>> + memset(&config, 0, sizeof(config));
>> +
>> + config.parent = dev;
>> + config.owner = THIS_MODULE;
>> + config.drvdata = ctx;
>> + config.match = pwrseq_aml_wcn_match;
>> + config.targets = pwrseq_aml_wcn_targets;
>> +
>> + ctx->pwr_count = 0;
>> + ctx->pwrseq = devm_pwrseq_device_register(dev, &config);
>> + if (IS_ERR(ctx->pwrseq))
>> + return dev_err_probe(dev, PTR_ERR(ctx->pwrseq),
>> + "Failed to register the power sequencer\n");
>> +
>> + return 0;
>> +}
>> +
>> +static const struct of_device_id pwrseq_aml_wcn_of_match[] = {
>> + { .compatible = "amlogic,w155s2-pwrseq" },
>> + { /* sentinel */ },
>> +};
>> +MODULE_DEVICE_TABLE(of, pwrseq_aml_wcn_of_match);
>> +
>> +static struct platform_driver pwrseq_aml_wcn_driver = {
>> + .driver = {
>> + .name = "pwrseq-aml_wcn",
>> + .of_match_table = pwrseq_aml_wcn_of_match,
>> + },
>> + .probe = pwrseq_aml_wcn_probe,
>> +};
>> +module_platform_driver(pwrseq_aml_wcn_driver);
>> +
>> +MODULE_AUTHOR("Yang Li<yang.li@amlogic.com>");
>> +MODULE_DESCRIPTION("Amlogic WCN PMU power sequencing driver");
>> +MODULE_LICENSE("GPL");
>>
>> --
>> 2.42.0
>>
>>
> Bart
© 2016 - 2026 Red Hat, Inc.