Re: [RFC PATCH v2 4/6] PM: opp: allow control of multiple clocks

[Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>]

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.

> 
>> +	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.

> 
>> +
>> +	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