> Subject: Re: [PATCH 1/2] clk: imx93: Drop macro IMX93_CLK_END > > On 25/06/2024 12:43, Pengfei Li wrote: > > On Tue, Jun 25, 2024 at 09:44:42AM +0200, Krzysztof Kozlowski > wrote: > >> On 25/06/2024 19:51, Pengfei Li wrote: > >>> IMX93_CLK_END was previously defined in imx93-clock.h to > indicate > >>> the number of clocks, but it is not part of the ABI, so it should be > >>> dropped. > >>> > >>> Now, the driver gets the number of clks by querying the maximum > >>> index in the clk array. Due to the discontinuity in the definition > >>> of clk index, with some gaps present, the total count cannot be > >>> obtained by summing the array size. > >>> > >>> Signed-off-by: Pengfei Li <pengfei.li_1@xxxxxxx> > >>> --- > >>> drivers/clk/imx/clk-imx93.c | 25 +++++++++++++++++++++---- > >>> 1 file changed, 21 insertions(+), 4 deletions(-) > >>> > >>> diff --git a/drivers/clk/imx/clk-imx93.c > >>> b/drivers/clk/imx/clk-imx93.c index c6a9bc8ecc1f..68c929512e16 > >>> 100644 > >>> --- a/drivers/clk/imx/clk-imx93.c > >>> +++ b/drivers/clk/imx/clk-imx93.c > >>> @@ -257,6 +257,20 @@ static const struct imx93_clk_ccgr > { static > >>> struct clk_hw_onecell_data *clk_hw_data; static struct clk_hw > >>> **clks; > >>> > >>> +static int imx_clks_get_num(void) > >>> +{ > >>> + u32 val = 0; > >>> + int i; > >>> + > >>> + for (i = 0; i < ARRAY_SIZE(root_array); i++) > >>> + val = max_t(u32, val, root_array[i].clk); > >>> + > >>> + for (i = 0; i < ARRAY_SIZE(ccgr_array); i++) > >>> + val = max_t(u32, val, ccgr_array[i].clk); > >>> + > >>> + return val + 1; > >>> +} > >>> + > >>> static int imx93_clocks_probe(struct platform_device *pdev) { > >>> struct device *dev = &pdev->dev; > >>> @@ -264,14 +278,17 @@ static int imx93_clocks_probe(struct > platform_device *pdev) > >>> const struct imx93_clk_root *root; > >>> const struct imx93_clk_ccgr *ccgr; > >>> void __iomem *base, *anatop_base; > >>> + int clks_num; > >>> int i, ret; > >>> > >>> + clks_num = imx_clks_get_num(); > >>> + > >>> clk_hw_data = devm_kzalloc(dev, struct_size(clk_hw_data, > hws, > >>> - IMX93_CLK_END), > GFP_KERNEL); > >>> + clks_num), GFP_KERNEL); > >>> if (!clk_hw_data) > >>> return -ENOMEM; > >>> > >>> - clk_hw_data->num = IMX93_CLK_END; > >>> + clk_hw_data->num = clks_num; > >> > >> Why so complicated code instead of pre-processor define or array > size? > >> > >> Best regards, > >> Krzysztof > >> > >> > > > > Hi Krzysztof, > > > > Thanks for the comment, here are some of our thoughts. > > > > Regarding the predefined method, it's easy to forget to update the > > macro definition when adding some new clocks to imx93-clock.h in > the future. > > Somehow most developers in most platforms can do it... Anyway, that > would be build time detectable so no problem at all. > > > > > Also, we cannot use the array size method in this scenario, as some > > unnecessary clocks have been removed in the past, resulting in > > discontinuous definitions of clock indexes. This means that the > > maximum clock index can be larger than the allocated clk_hw array > size. At this point, using the maximum index to access the clk_hw array > will result in an out of bounds error. > > You mix bindings with array entries. That's independent or just clock > drivers are broken. But there is issue that binding update and clock driver are normally in two patches. So if just use the IMX93_CLK_END in this file, it will be easy to break `git bisect`. Regards, Peng. > > Best regards, > Krzysztof >