Received-SPF: pass (google.com: domain of linux-kernel+bounces-71852-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:45e3:2400::1 as permitted sender) client-ip=2604:1380:45e3:2400::1;
Message-ID: <e760847bd911671f1e364271888481fd.sboyd@kernel.org>
Content-Type: text/plain; charset="utf-8"
Precedence: bulk
MIME-Version: 1.0
Content-Transfer-Encoding: quoted-printable
In-Reply-To: <02461595-16b3-4fea-a029-54190e10e6f5@linaro.org>
References: <20240208163710.512733-1-krzysztof.kozlowski@linaro.org> <38e7e80f61f7c67c984735cf55c3dfb3.sboyd@kernel.org> <02461595-16b3-4fea-a029-54190e10e6f5@linaro.org>
Subject: Re: [PATCH] clk: constify the of_phandle_args argument of of_clk_provider
From: Stephen Boyd <sboyd@kernel.org>
To: Bjorn Andersson <andersson@kernel.org>, Geert Uytterhoeven <geert+renesas@glider.be>, Jaroslav Kysela <perex@perex.cz>, Jonathan Hunter <jonathanh@nvidia.com>, Konrad Dybcio <konrad.dybcio@linaro.org>, Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>, Laurent Pinchart <laurent.pinchart@ideasonboard.com>, Linus Walleij <linus.walleij@linaro.org>, Mark Brown <broonie@kernel.org>, Mauro Carvalho Chehab <mchehab@kernel.org>, Michael Turquette <mturquette@baylibre.com>, NXP Linux Team <linux-imx@nxp.com>, Nishanth Menon <nm@ti.com>, Peng Fan <peng.fan@nxp.com>, Russell King <linux@armlinux.org.uk>, Shawn Guo <shawnguo@kernel.org>, Srinivas Kandagatla <srinivas.kandagatla@linaro.org>, Sudeep Holla <sudeep.holla@arm.com>, Takashi Iwai <tiwai@suse.com>, Thierry Reding <thierry.reding@gmail.com>, Vinod Koul <vkoul@kernel.org>, alsa-devel@alsa-project.org, linux-amlogic@lists.infradead.org, linux-arm-kernel@lists.infradead.org, linux-arm-msm@vger.kernel.org, linux-clk@vger.kernel.org, 
 linux-kernel@vger.kernel.org, linux-media@vger.kernel.org, linux-omap@vger.kernel.org, linux-phy@lists.infradead.org, linux-renesas-soc@vger.kernel.org, linux-sound@vger.kernel.org, linux-stm32@st-md-mailman.stormreply.com, linux-tegra@vger.kernel.org, patches@opensource.cirrus.com
Date: Mon, 19 Feb 2024 11:08:27 -0800
User-Agent: alot/0.10

Quoting Krzysztof Kozlowski (2024-02-15 23:12:29)
> On 16/02/2024 00:12, Stephen Boyd wrote:
> > Quoting Krzysztof Kozlowski (2024-02-08 08:37:10)
> >> None of the implementations of the get() and get_hw() callbacks of
> >> "struct of_clk_provider" modify the contents of received of_phandle_ar=
gs
> >> pointer.  They treat it as read-only variable used to find the clock to
> >> return.  Make obvious that implementations are not supposed to modify
> >> the of_phandle_args, by making it a pointer to const.
> >>
> >> Signed-off-by: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>
> >> ---
> >=20
> > This will almost certainly break the build once it is merged to
> > linux-next. What's your plan to merge this?
>=20
> First problem is that it might not apply... I prepared it on next to be
> sure all subsystems are updated.
>=20
> The idea is to get reviews and acks and then:
> 1. Maybe it applies cleanly to your tree meaning there will be no
> conflicts with other trees,
> 2. If not, then I can keep rebasing it and it should be applied after rc1.
>=20

The struct clk based version is probably not going to be used in any new
code. If you split the patch up and converted the struct clk based ones
first then that would probably apply without breaking anything, because
new code should only be using the struct clk_hw version.

The struct clk_hw version could be done in two steps. Introduce another
get_hw callback with the const signature, and then update the world to
use that callback, finally remove the old callback. We could call this
callback 'get_clk_hw'. This is probably more work than it's worth
though, but at least this way we don't have to worry about applying
after rc1.

Or perhaps we need to cast everything and use macros? It would be bad if
the callback actually did something with the clkspec and we cast it to
const, but your patch shows that nobody is doing that. We would get rid
of this macro garbage once everything is converted.

---8<---
diff --git a/drivers/clk/clk.c b/drivers/clk/clk.c
index 2253c154a824..8e5ed16a97a0 100644
--- a/drivers/clk/clk.c
+++ b/drivers/clk/clk.c
@@ -4818,7 +4818,7 @@ struct of_clk_provider {
 	struct list_head link;
=20
 	struct device_node *node;
-	struct clk *(*get)(struct of_phandle_args *clkspec, void *data);
+	struct clk *(*get)(const struct of_phandle_args *clkspec, void *data);
 	struct clk_hw *(*get_hw)(struct of_phandle_args *clkspec, void *data);
 	void *data;
 };
@@ -4880,8 +4880,8 @@ EXPORT_SYMBOL_GPL(of_clk_hw_onecell_get);
  *
  * This function is *deprecated*. Use of_clk_add_hw_provider() instead.
  */
-int of_clk_add_provider(struct device_node *np,
-			struct clk *(*clk_src_get)(struct of_phandle_args *clkspec,
+int _of_clk_add_provider(struct device_node *np,
+			struct clk *(*clk_src_get)(const struct of_phandle_args *clkspec,
 						   void *data),
 			void *data)
 {
@@ -4914,7 +4914,7 @@ int of_clk_add_provider(struct device_node *np,
=20
 	return ret;
 }
-EXPORT_SYMBOL_GPL(of_clk_add_provider);
+EXPORT_SYMBOL_GPL(_of_clk_add_provider);
=20
 /**
  * of_clk_add_hw_provider() - Register a clock provider for a node
diff --git a/include/linux/clk-provider.h b/include/linux/clk-provider.h
index 1293c38ddb7f..bfc660fa7c8f 100644
--- a/include/linux/clk-provider.h
+++ b/include/linux/clk-provider.h
@@ -1531,10 +1531,11 @@ struct clk_hw_onecell_data {
 	}
=20
 #ifdef CONFIG_OF
-int of_clk_add_provider(struct device_node *np,
-			struct clk *(*clk_src_get)(struct of_phandle_args *args,
+int _of_clk_add_provider(struct device_node *np,
+			struct clk *(*clk_src_get)(const struct of_phandle_args *args,
 						   void *data),
 			void *data);
+
 int of_clk_add_hw_provider(struct device_node *np,
 			   struct clk_hw *(*get)(struct of_phandle_args *clkspec,
 						 void *data),
@@ -1559,8 +1560,8 @@ int of_clk_detect_critical(struct device_node *np, in=
t index,
=20
 #else /* !CONFIG_OF */
=20
-static inline int of_clk_add_provider(struct device_node *np,
-			struct clk *(*clk_src_get)(struct of_phandle_args *args,
+static inline int _of_clk_add_provider(struct device_node *np,
+			struct clk *(*clk_src_get)(const struct of_phandle_args *args,
 						   void *data),
 			void *data)
 {
@@ -1614,6 +1615,12 @@ static inline int of_clk_detect_critical(struct devi=
ce_node *np, int index,
 }
 #endif /* CONFIG_OF */
=20
+typedef struct clk *(*clk_src_get_fn)(const struct of_phandle_args *args, =
void *data);
+
+#define of_clk_add_provider(np, get, data) ({				\
+		_of_clk_add_provider(np, (clk_src_get_fn)(get), data);		\
+})
+
 void clk_gate_restore_context(struct clk_hw *hw);
=20
 #endif /* CLK_PROVIDER_H */