Re: [v2] spi: spi-mtk-nor: Modify the clock architecture of nor controller

[AngeloGioacchino Del Regno <angelogioacchino.delregno@collabora.com>]

Il 03/03/25 12:45, mtk22730 ha scritto:
> The clocks used by different platforms are not same. So it is
> necessary to modify the clock architecture to be adaptable to more
> platforms.
> 
> Signed-off-by: Cloud Zhang <cloud.zhang@mediatek.com>

You really shall fix your identity on outgoing emails: your name appears as
"mtk22730", that's not right.

Also, the title could be clarified a bit...

spi: spi-mtk-nor: Migrate to clk_bulk API

...and then: this should be a series, and the first commit shall be adjusting
the dt-binding for this driver accordigly, otherwise whatever you're doing here
will turn out to be unusable.

> ---
> Changes in v2:
>    -Use clk_bulk_xxx related functions to enable/disable clocks.
> 
> Changes in v1:
>    -Add new function mtk_nor_parse_clk() to parse nor clock parameters.
> ---
> ---
>   drivers/spi/spi-mtk-nor.c | 103 ++++++++++++++++++++------------------
>   1 file changed, 54 insertions(+), 49 deletions(-)
> 
> diff --git a/drivers/spi/spi-mtk-nor.c b/drivers/spi/spi-mtk-nor.c
> index 85ab5ce96c4d..4863b9cb2706 100644
> --- a/drivers/spi/spi-mtk-nor.c
> +++ b/drivers/spi/spi-mtk-nor.c
> @@ -99,6 +99,8 @@
>   
>   #define CLK_TO_US(sp, clkcnt)		DIV_ROUND_UP(clkcnt, sp->spi_freq / 1000000)
>   
> +#define MAX_CLOCK_CNT		6
> +
>   struct mtk_nor_caps {
>   	u8 dma_bits;
>   
> @@ -116,10 +118,8 @@ struct mtk_nor {
>   	void __iomem *base;
>   	u8 *buffer;
>   	dma_addr_t buffer_dma;
> -	struct clk *spi_clk;
> -	struct clk *ctlr_clk;
> -	struct clk *axi_clk;
> -	struct clk *axi_s_clk;
> +	struct clk_bulk_data clocks[MAX_CLOCK_CNT];

What about having this as a pointer?

	struct clk_bulk_data *clks;

...Then you count the clocks, and devm allocate the number of clks that you need.

> +	int clock_cnt;

u8 clock_cnt;

>   	unsigned int spi_freq;
>   	bool wbuf_en;
>   	bool has_irq;
> @@ -703,44 +703,68 @@ static int mtk_nor_transfer_one_message(struct spi_controller *host,
>   
>   static void mtk_nor_disable_clk(struct mtk_nor *sp)

A function with one call means that you don't need a function at all.

>   {
> -	clk_disable_unprepare(sp->spi_clk);
> -	clk_disable_unprepare(sp->ctlr_clk);
> -	clk_disable_unprepare(sp->axi_clk);
> -	clk_disable_unprepare(sp->axi_s_clk);
> +	clk_bulk_disable_unprepare(sp->clock_cnt, sp->clocks);
>   }
>   
>   static int mtk_nor_enable_clk(struct mtk_nor *sp)
>   {

You can also remove this function and transfer the contents into mtk_nor_probe():
it's just something like 6 lines and even called only once, so... :-)

>   	int ret;
> +	int i;
>   
> -	ret = clk_prepare_enable(sp->spi_clk);
> -	if (ret)
> -		return ret;
> -
> -	ret = clk_prepare_enable(sp->ctlr_clk);
> +	ret = clk_bulk_prepare_enable(sp->clock_cnt, sp->clocks);
>   	if (ret) {
> -		clk_disable_unprepare(sp->spi_clk);
> +		dev_err(sp->dev, "enable clk failed\n");
>   		return ret;
>   	}
>   
> -	ret = clk_prepare_enable(sp->axi_clk);
> -	if (ret) {
> -		clk_disable_unprepare(sp->spi_clk);
> -		clk_disable_unprepare(sp->ctlr_clk);
> -		return ret;
> -	}
> +	for (i = 0; i < sp->clock_cnt; i++) {
> +		if (IS_ERR(sp->clocks[i].clk)) {
> +			dev_err(sp->dev, "get %s fail\n", sp->clocks[i].id);
> +			return PTR_ERR(sp->clocks[i].clk);
> +		}
>   
> -	ret = clk_prepare_enable(sp->axi_s_clk);
> -	if (ret) {
> -		clk_disable_unprepare(sp->spi_clk);
> -		clk_disable_unprepare(sp->ctlr_clk);
> -		clk_disable_unprepare(sp->axi_clk);
> -		return ret;
> +		if (!strcmp(sp->clocks[i].id, "spi"))
> +			sp->spi_freq = clk_get_rate(sp->clocks[i].clk);
>   	}
>   
>   	return 0;
>   }
>   
> +static int mtk_nor_parse_clk(struct device *dev, struct mtk_nor *sp)
> +{
> +	struct device_node *np = dev->of_node;
> +	int ret;
> +	const char *name;
> +	int cnt, i;

	struct device_node *np = dev->of_node;
	const char *name;
	int cnt, i, ret;


> +
> +	cnt = of_property_count_strings(np, "clock-names");
> +	if (!cnt || (cnt == -EINVAL)) {
> +		dev_err(dev, "Unable to find clocks\n");

return -EINVAL;

> +		ret = -EINVAL;
> +		goto out;
> +	} else if (cnt < 0) {
> +		dev_err(dev, "Count clock strings failed, err %d\n", cnt);

return cnt;

> +		ret = cnt;
> +		goto out;
> +	}
> +
> +	sp->clock_cnt = cnt;
> +	for (i = 0; i < cnt; i++) {
> +		ret = of_property_read_string_index(np, "clock-names", i, &name);
> +		if (ret) {
> +			dev_err(dev, "failed to get clock string\n");
> +			return ret;
> +		}
> +
> +		sp->clocks[i].id = name;
> +	}
> +
> +	ret = devm_clk_bulk_get(dev, sp->clock_cnt, sp->clocks);

	if (ret)
		return ret;

	return 0;

> +
> +out:
> +	return ret;
> +}
> +

Cheers,
Angelo