On Wed 13 Apr 02:07 PDT 2022, Krzysztof Kozlowski wrote:
> On 12/04/2022 19:15, Bjorn Andersson wrote:
> >>
> >> + opp_table->clks = kmalloc_array(1, sizeof(*opp_table->clks),
> >> + GFP_KERNEL);
> >
> > This seems to be 81 chars long, perhaps worth not line breaking?
>
> I doubt that it will increase the readability:
>
> opp_table->clks = kmalloc_array(1,
> sizeof(*opp_table->clks),
> GFP_KERNEL);
>
> 80-character is not anymore that strict hard limit and in such case
> using 1-2 characters longer improves the code.
>
I was suggesting that you remove the line break
opp_table->clks = kmalloc_array(1, sizeof(*opp_table->clks), GFP_KERNEL);
Seems to be 81 chars long, which is fine in my book with or without the
80-char guideline.
> >
> >> + if (!opp_table->clks)
> >> + return ERR_PTR(-ENOMEM);
> >> +
> >> /* Find clk for the device */
> >> - opp_table->clk = clk_get(dev, NULL);
> >> + opp_table->clks[0] = clk_get(dev, NULL);
> >>
> >> - ret = PTR_ERR_OR_ZERO(opp_table->clk);
> >> - if (!ret)
> >> + ret = PTR_ERR_OR_ZERO(opp_table->clks[0]);
> >> + if (!ret) {
> >> + opp_table->clk_count = 1;
> >> return opp_table;
> >> + }
> > [..]
> >> +struct opp_table *dev_pm_opp_set_clknames(struct device *dev,
> >> + const char * const names[],
> >> + unsigned int count)
> >> {
> >> struct opp_table *opp_table;
> >> - int ret;
> >> + struct clk *clk;
> >> + int ret, i;
> >>
> >> opp_table = _add_opp_table(dev, false);
> >> if (IS_ERR(opp_table))
> >> @@ -2159,70 +2259,92 @@ struct opp_table *dev_pm_opp_set_clkname(struct device *dev, const char *name)
> >> }
> >>
> >> /* clk shouldn't be initialized at this point */
> >> - if (WARN_ON(opp_table->clk)) {
> >> + if (WARN_ON(opp_table->clks)) {
> >> ret = -EBUSY;
> >> goto err;
> >> }
> >>
> >> - /* Find clk for the device */
> >> - opp_table->clk = clk_get(dev, name);
> >> - if (IS_ERR(opp_table->clk)) {
> >> - ret = dev_err_probe(dev, PTR_ERR(opp_table->clk),
> >> - "%s: Couldn't find clock\n", __func__);
> >> + opp_table->clks = kmalloc_array(count, sizeof(*opp_table->clks),
> >> + GFP_KERNEL);
> >> + if (!opp_table->clks) {
> >> + ret = -ENOMEM;
> >> goto err;
> >> }
> >>
> >> + for (i = 0; i < count; i++) {
> >> + clk = clk_get(dev, names[i]);
> >> + if (IS_ERR(clk)) {
> >> + ret = dev_err_probe(dev, PTR_ERR(clk),
> >> + "%s: Couldn't find clock %s\n",
> >> + __func__, names[i]);
> >> + goto free_clks;
> >> + }
> >> +
> >> + opp_table->clks[i] = clk;
> >> + }
> >
> > Wouldn't it be convenient to make clks a struct clk_bulk_data array
> > and use clk_bulk_get()/clk_bulk_put() instead?
>
> I was thinking about this but clk_bulk_get() requires struct
> clk_bulk_data, so the code in "get" is not actually smaller if function
> receives array of clock names.
>
> OTOH, usage of clk_bulk_get() would reduce code in: _put_clocks(). Rest
> of the code would be more-or-less the same, including all corner cases
> when clocks are missing.
>
Fair enough, I think you're right that it's not going to be much
difference.
Regards,
Bjorn
> >
> >> +
> >> + opp_table->clk_count = count;
> >> +
> >> return opp_table;
> >>
> >> +free_clks:
> >> + while (i != 0)
> >> + clk_put(opp_table->clks[--i]);
> >> +
> >> + kfree(opp_table->clks);
> >> + opp_table->clks = NULL;
> >> + opp_table->clk_count = -1;
> >> err:
> >> dev_pm_opp_put_opp_table(opp_table);
> >>
> >> return ERR_PTR(ret);
> >> }
> >> -EXPORT_SYMBOL_GPL(dev_pm_opp_set_clkname);
> >> +EXPORT_SYMBOL_GPL(dev_pm_opp_set_clknames);
> > [..]
> >> +static int _read_clocks(struct dev_pm_opp *opp, struct opp_table *opp_table,
> >> + struct device_node *np)
> >> +{
> >> + int count, ret;
> >> + u64 *freq;
> >> +
> >> + count = of_property_count_u64_elems(np, "opp-hz");
> >> + if (count < 0) {
> >> + pr_err("%s: Invalid %s property (%d)\n",
> >> + __func__, of_node_full_name(np), count);
> >
> > Wouldn't %pOF be convenient to use here, seems like it becomes short
> > enough that you don't have to wrap this line then.
>
> Yes, I forgot about %pOF.
>
> >
> >> + return count;
> >> + }
> >> +
> >> + if (count != opp_table->clk_count) {
> >> + pr_err("%s: number of rates %d does not match number of clocks %d in %s\n",
> >> + __func__, count, opp_table->clk_count,
> >> + of_node_full_name(np));
> >> + return -EINVAL;
> >> + }
> >> +
> >> + freq = kmalloc_array(count, sizeof(*freq), GFP_KERNEL);
> >> + if (!freq)
> >> + return -ENOMEM;
> >> +
> >> + ret = of_property_read_u64_array(np, "opp-hz", freq, count);
> >> + if (ret) {
> >> + pr_err("%s: error parsing %s: %d\n", __func__,
> >> + of_node_full_name(np), ret);
> >> + ret = -EINVAL;
> >> + goto free_freq;
> >> + }
> >
> > Regards,
> > Bjorn
>
>
> Best regards,
> Krzysztof
