RE: [PATCH 1/2] clk: imx93: Drop macro IMX93_CLK_END

RE: [PATCH 1/2] clk: imx93: Drop macro IMX93_CLK_END

[Peng Fan]

> Subject: [PATCH 1/2] clk: imx93: Drop macro IMX93_CLK_END
> 
> 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@nxp.com>

Reviewed-by: Peng Fan <peng.fan@nxp.com>

RE: [PATCH 2/2] dt-bindings: clock: imx93: Drop IMX93_CLK_END macro definition

[Peng Fan]

> Subject: [PATCH 2/2] dt-bindings: clock: imx93: Drop IMX93_CLK_END
> macro definition
> 
> IMX93_CLK_END should be dropped as it is not part of the ABI.
> 
> Signed-off-by: Pengfei Li <pengfei.li_1@nxp.com>

Acked-by: Peng Fan <peng.fan@nxp.com>

Re: [PATCH 1/2] clk: imx93: Drop macro IMX93_CLK_END

[Krzysztof Kozlowski]

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@nxp.com>
> ---
>  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

Re: [PATCH 2/2] dt-bindings: clock: imx93: Drop IMX93_CLK_END macro definition

[Krzysztof Kozlowski]

On 25/06/2024 19:51, Pengfei Li wrote:
> IMX93_CLK_END should be dropped as it is not part of the ABI.
> 
> Signed-off-by: Pengfei Li <pengfei.li_1@nxp.com>
> ---

Acked-by: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>

Best regards,
Krzysztof

Re: [PATCH 0/2] clk: imx93: Drop macro IMX93_CLK_END

[Krzysztof Kozlowski]

On 25/06/2024 19:51, Pengfei Li wrote:
> 'IMX93_CLK_END' macro 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.

Fix your clock, so this will not appear in 10 hours ahead, on top of
mailbox pretending to be the newest patch to review. Trying to get ahead
of the queue is a straight way to get grumpy reviews or just being ignored.

Best regards,
Krzysztof

Re: [PATCH 1/2] clk: imx93: Drop macro IMX93_CLK_END

[Pengfei Li]

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@nxp.com>
> > ---
> >  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.

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.

BR,
Pengfei Li

Re: [PATCH 1/2] clk: imx93: Drop macro IMX93_CLK_END

[Krzysztof Kozlowski]

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@nxp.com>
>>> ---
>>>  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.

Best regards,
Krzysztof

[PATCH 0/2] clk: imx93: Drop macro IMX93_CLK_END

[Pengfei Li]

'IMX93_CLK_END' macro 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.

Pengfei Li (2):
  clk: imx93: Drop macro IMX93_CLK_END
  dt-bindings: clock: imx93: Drop IMX93_CLK_END macro definition

 drivers/clk/imx/clk-imx93.c             | 25 +++++++++++++++++++++----
 include/dt-bindings/clock/imx93-clock.h |  1 -
 2 files changed, 21 insertions(+), 5 deletions(-)

-- 
2.34.1

[PATCH 1/2] clk: imx93: Drop macro IMX93_CLK_END

[Pengfei Li]

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@nxp.com>
---
 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;
 	clks = clk_hw_data->hws;
 
 	clks[IMX93_CLK_DUMMY] = imx_clk_hw_fixed("dummy", 0);
@@ -335,7 +352,7 @@ static int imx93_clocks_probe(struct platform_device *pdev)
 						  clks[IMX93_CLK_ARM_PLL]->clk,
 						  clks[IMX93_CLK_A55_GATE]->clk);
 
-	imx_check_clk_hws(clks, IMX93_CLK_END);
+	imx_check_clk_hws(clks, clks_num);
 
 	ret = of_clk_add_hw_provider(np, of_clk_hw_onecell_get, clk_hw_data);
 	if (ret < 0) {
@@ -348,7 +365,7 @@ static int imx93_clocks_probe(struct platform_device *pdev)
 	return 0;
 
 unregister_hws:
-	imx_unregister_hw_clocks(clks, IMX93_CLK_END);
+	imx_unregister_hw_clocks(clks, clks_num);
 
 	return ret;
 }
-- 
2.34.1

[PATCH 2/2] dt-bindings: clock: imx93: Drop IMX93_CLK_END macro definition

[Pengfei Li]

IMX93_CLK_END should be dropped as it is not part of the ABI.

Signed-off-by: Pengfei Li <pengfei.li_1@nxp.com>
---
 include/dt-bindings/clock/imx93-clock.h | 1 -
 1 file changed, 1 deletion(-)

diff --git a/include/dt-bindings/clock/imx93-clock.h b/include/dt-bindings/clock/imx93-clock.h
index 787c9e74dc96..a1d0b326bb6b 100644
--- a/include/dt-bindings/clock/imx93-clock.h
+++ b/include/dt-bindings/clock/imx93-clock.h
@@ -204,6 +204,5 @@
 #define IMX93_CLK_A55_SEL		199
 #define IMX93_CLK_A55_CORE		200
 #define IMX93_CLK_PDM_IPG		201
-#define IMX93_CLK_END			202
 
 #endif
-- 
2.34.1

RE: [PATCH 1/2] clk: imx93: Drop macro IMX93_CLK_END

[Peng Fan]

> 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@nxp.com>
> >>> ---
> >>>  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
> 

Re: [PATCH 1/2] clk: imx93: Drop macro IMX93_CLK_END

[Krzysztof Kozlowski]

On 28/06/2024 03:17, Peng Fan wrote:
>> 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@nxp.com>
>>>>> ---
>>>>>  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`.

There is no issue. Srsly, this would be the only, only one driver having
that issue.

How is this even possible? How adding one new define for pre-processor
would cause driver issues or some sort of bisectability problems?

These are basics of C we talk about now...

Best regards,
Krzysztof

RE: [PATCH 1/2] clk: imx93: Drop macro IMX93_CLK_END

[Peng Fan]

> Subject: Re: [PATCH 1/2] clk: imx93: Drop macro IMX93_CLK_END
> 
> On 28/06/2024 03:17, Peng Fan wrote:
> >> 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@nxp.com>
> >>>>> ---
> >>>>>  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`.
> 
> There is no issue. Srsly, this would be the only, only one driver having
> that issue.
> 
> How is this even possible? How adding one new define for pre-
> processor would cause driver issues or some sort of bisectability
> problems?

Ah, I was wrong, I just thought driver update is applied first.

With binding update applied first, then driver update applied,
there is no issue.

Regards,
Peng.

> 
> These are basics of C we talk about now...
> 
> Best regards,
> Krzysztof
>