Re: [PATCH 2/3] power: sequenceing: Add power sequence for Amlogic WCN chips

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



Dear Krzysztof

Thanks for your comments.
On 05/07/2024 13:13, Yang Li via B4 Relay wrote:
From: Yang Li <yang.li@xxxxxxxxxxx>

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@xxxxxxxxxxx>
+#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





[Index of Archives]     [Device Tree Compilter]     [Device Tree Spec]     [Linux Driver Backports]     [Video for Linux]     [Linux USB Devel]     [Linux PCI Devel]     [Linux Audio Users]     [Linux Kernel]     [Linux SCSI]     [XFree86]     [Yosemite Backpacking]


  Powered by Linux