Messages in this thread | | | Date | Tue, 3 Oct 2023 13:13:33 +0100 | From | Cristian Marussi <> | Subject | Re: [PATCH v4 2/2] clk: scmi: add set/get_parent support |
| |
On Tue, Oct 03, 2023 at 07:48:49PM +0800, Peng Fan (OSS) wrote: > From: Peng Fan <peng.fan@nxp.com> > > SCMI v3.2 adds set/get parent clock commands, so update the clk driver > to support them. >
Hi,
in general LGTM, BUT I have just spotted one more *bad* thing that have to be fixe down below which I have missed previously, my bad.
> Signed-off-by: Peng Fan <peng.fan@nxp.com> > --- > drivers/clk/clk-scmi.c | 58 +++++++++++++++++++++++++++++++++++++++++++++++++- > 1 file changed, 57 insertions(+), 1 deletion(-) > > diff --git a/drivers/clk/clk-scmi.c b/drivers/clk/clk-scmi.c > index 2e1337b511eb..e7a27fda561b 100644 > --- a/drivers/clk/clk-scmi.c > +++ b/drivers/clk/clk-scmi.c > @@ -24,6 +24,7 @@ struct scmi_clk { > struct clk_hw hw; > const struct scmi_clock_info *info; > const struct scmi_protocol_handle *ph; > + struct clk_parent_data *parent_data; > }; > > #define to_scmi_clk(clk) container_of(clk, struct scmi_clk, hw) > @@ -78,6 +79,43 @@ static int scmi_clk_set_rate(struct clk_hw *hw, unsigned long rate, > return scmi_proto_clk_ops->rate_set(clk->ph, clk->id, rate); > } > > +static int scmi_clk_set_parent(struct clk_hw *hw, u8 parent_index) > +{ > + struct scmi_clk *clk = to_scmi_clk(hw); > + > + return scmi_proto_clk_ops->parent_set(clk->ph, clk->id, parent_index); > +} > + > +static u8 scmi_clk_get_parent(struct clk_hw *hw) > +{ > + struct scmi_clk *clk = to_scmi_clk(hw); > + u32 parent_id, p_idx; > + int ret; > + > + ret = scmi_proto_clk_ops->parent_get(clk->ph, clk->id, &parent_id); > + if (ret) > + return 0; > + > + for (p_idx = 0; p_idx < clk->info->num_parents; p_idx++) { > + if (clk->parent_data[p_idx].index == parent_id) > + break; > + } > + > + if (p_idx == clk->info->num_parents) > + return 0; > + > + return p_idx; > +} > + > +static int scmi_clk_determine_rate(struct clk_hw *hw, struct clk_rate_request *req) > +{ > + /* > + * Suppose all the requested rates are supported, and let firmware > + * to handle the left work. > + */ > + return 0; > +} > + > static int scmi_clk_enable(struct clk_hw *hw) > { > struct scmi_clk *clk = to_scmi_clk(hw); > @@ -139,6 +177,9 @@ static const struct clk_ops scmi_clk_ops = { > .set_rate = scmi_clk_set_rate, > .prepare = scmi_clk_enable, > .unprepare = scmi_clk_disable, > + .set_parent = scmi_clk_set_parent, > + .get_parent = scmi_clk_get_parent, > + .determine_rate = scmi_clk_determine_rate, > }; > > static const struct clk_ops scmi_atomic_clk_ops = { > @@ -148,6 +189,9 @@ static const struct clk_ops scmi_atomic_clk_ops = { > .enable = scmi_clk_atomic_enable, > .disable = scmi_clk_atomic_disable, > .is_enabled = scmi_clk_atomic_is_enabled, > + .set_parent = scmi_clk_set_parent, > + .get_parent = scmi_clk_get_parent, > + .determine_rate = scmi_clk_determine_rate, > }; > > static int scmi_clk_ops_init(struct device *dev, struct scmi_clk *sclk, > @@ -158,9 +202,10 @@ static int scmi_clk_ops_init(struct device *dev, struct scmi_clk *sclk, > > struct clk_init_data init = { > .flags = CLK_GET_RATE_NOCACHE, > - .num_parents = 0, > + .num_parents = sclk->info->num_parents, > .ops = scmi_ops, > .name = sclk->info->name, > + .parent_data = sclk->parent_data, > }; > > sclk->hw.init = &init; > @@ -250,6 +295,17 @@ static int scmi_clocks_probe(struct scmi_device *sdev) > else > scmi_ops = &scmi_clk_ops; > > + /* Initialize clock parent data. */ > + if (sclk->info->num_parents > 0) { > + sclk->parent_data = devm_kcalloc(dev, sclk->info->num_parents, > + sizeof(*sclk->parent_data), GFP_KERNEL); > +
Here, you missed to check the return value of devm_kcalloc() before carrying on.
> + for (int i = 0; i < sclk->info->num_parents; i++) { > + sclk->parent_data[i].index = sclk->info->parents[i]; > + sclk->parent_data[i].hw = hws[sclk->info->parents[i]]; > + } > + } > +
Other than this, FWIW:
Reviewed-by: Cristian Marussi <cristian.marussi@arm.com>
Thanks, Cristian
| |