RE: [PATCH v3 2/2] clk: clk-conf: support assigned-clock-rates-u64

[Peng Fan]

Hi Stephen,

> Subject: Re: [PATCH v3 2/2] clk: clk-conf: support assigned-clock-rates-
> u64
> 
> Quoting Peng Fan (OSS) (2024-07-30 01:57:55)
> > From: Peng Fan <peng.fan@xxxxxxx>
> >
> > i.MX95 System Management Control Firmware(SCMI) manages the
> clock
> > function, it exposes PLL VCO which could support up to 5GHz rate
> that
> > exceeds UINT32_MAX. So add assigned-clock-rates-u64 support to
> set
> > rate that exceeds UINT32_MAX.
> >
> > Signed-off-by: Peng Fan <peng.fan@xxxxxxx>
> > ---
> >  drivers/clk/clk-conf.c | 42
> > +++++++++++++++++++++++++++++++++++++-----
> >  1 file changed, 37 insertions(+), 5 deletions(-)
> >
> > diff --git a/drivers/clk/clk-conf.c b/drivers/clk/clk-conf.c index
> > 058420562020..684e0c0738b3 100644
> > --- a/drivers/clk/clk-conf.c
> > +++ b/drivers/clk/clk-conf.c
> > @@ -81,11 +81,44 @@ static int __set_clk_parents(struct
> device_node
> > *node, bool clk_supplier)  static int __set_clk_rates(struct
> > device_node *node, bool clk_supplier)  {
> >         struct of_phandle_args clkspec;
> > -       int rc, index = 0;
> > +       int rc, count, index;
> >         struct clk *clk;
> > -       u32 rate;
> > +       u32 *rates __free(kfree);
> > +       bool rate_64 = false;
> > +
> > +       count = of_property_count_u64_elems(node, "assigned-clock-
> rates-u64");
> > +       if (count <= 0) {
> > +               count = of_property_count_u32_elems(node, "assigned-
> clock-rates");
> > +               if (count <= 0)
> > +                       return 0;
> > +
> > +               rates = kcalloc(count, sizeof(u32), GFP_KERNEL);
> > +               if (!rates)
> > +                       return -ENOMEM;
> > +               rc = of_property_read_variable_u32_array(node,
> > +                                                        "assigned-clock-rates",
> > +                                                        rates,
> > +                                                        1, count);
> > +       } else {
> > +               rates = kcalloc(count, sizeof(u64), GFP_KERNEL);
> > +               if (!rates)
> > +                       return -ENOMEM;
> > +               rc = of_property_read_variable_u64_array(node,
> > +                                                        "assigned-clock-rates-u64",
> > +                                                        (u64 *)rates,
> > +                                                        1, count);
> > +               rate_64 = true;
> > +       }
> 
> Can this be less indented somehow?
> 
> 	u64 *rates_64 __free(kfree) = NULL;
> 	u32 *rates __free(kfree) = NULL;
> 	int count_64, count;
> 
> 	count = of_property_count_u32_elems(node, "assigned-clock-
> rates");
> 	count_64 = of_property_count_u64_elems(node, "assigned-
> clock-rates-u64");
> 	if (count_64 > 0) {
> 		count = count_64;
> 		rates_64 = kcalloc(count, sizeof(*rates_64),
> GFP_KERNEL);
> 		if (!rates_64)
> 			return -ENOMEM;
> 
> 		rc = of_property_read_u64_array(node,
> 						"assigned-clock-
> rates-u64",
> 						rates_64, count);
> 	} else if (count > 0) {
> 		rates = kcalloc(count, sizeof(*rates), GFP_KERNEL));
> 		if (!rates)
> 			return -ENOMEM;
> 
> 		rc = of_property_read_u32_array(node, "assigned-
> clock-rates",
> 						rates, count);
> 	} else {
> 		return 0;
> 	}
> 
> 	if (rc)
> 		return rc;
> 
> 	for (index = 0; index < count; index++) {
> 		unsigned long rate;
> 
> 		if (rates_64)
> 			rate = rates_64[index];
> 		else
> 			rate = rates[index];
> 

Thanks for writing down the code piece, looks good.

> > +
> > +
> > +       for (index = 0; index < count; index++) {
> > +               unsigned long rate;
> > +
> > +               if (rate_64)
> > +                       rate = ((u64 *)rates)[index];
> 
> Please no casts.

sure.

Thanks,
Peng.
> 
> > +               else
> > +                       rate = rates[index];
> >