[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <00fe66b7-1c04-48ec-a8ed-404e941dfeab@amlogic.com>
Date: Mon, 8 Jul 2024 15:41:27 +0800
From: Yang Li <yang.li@...ogic.com>
To: Krzysztof Kozlowski <krzk@...nel.org>, Rob Herring <robh@...nel.org>,
Krzysztof Kozlowski <krzk+dt@...nel.org>, Conor Dooley
<conor+dt@...nel.org>, Bartosz Golaszewski <brgl@...ev.pl>
Cc: devicetree@...r.kernel.org, linux-kernel@...r.kernel.org,
linux-pm@...r.kernel.org
Subject: Re: [PATCH 2/3] power: sequenceing: Add power sequence for Amlogic
WCN chips
Dear Krzysztof
Thanks for your comments.
> On 05/07/2024 13:13, Yang Li via B4 Relay wrote:
>> From: Yang Li <yang.li@...ogic.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@...ogic.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
>
Powered by blists - more mailing lists