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

[Krzysztof Kozlowski <krzk@kernel.org>]

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.

> +	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