> +/**
> + * ufshcd_get_clk_u32_array - Resolve property in dts to u32 array with
> + * shape check.
> + * @hba: per-adapter instance
> + * @propname: name of property
> + * @cols: column count
> + * @rows: calculated row count with given cols
> + * @array: u32 array pointer of property data with propname
> + */
> +static int ufshcd_get_clk_u32_array(struct ufs_hba *hba, const char *propname,
> + size_t cols, size_t *rows, u32 **array)
> +{
> + struct device *dev = hba->dev;
> + struct device_node *np = dev->of_node;
> + int len = 0, ret = 0;
> + int _rows = 0;
> +
> + if (!of_get_property(np, propname, &len)) {
> + ret = -EINVAL;
> + dev_warn(dev, "%s property not specified.\n", propname);
> + goto out;
devm_kcalloc() has not been called yet.
There is no problem with calling devm_kfree(),
but wouldn't it be nice to return the error right away?
Isn't it "%s: ~~" ?
> + }
> +
> + len = len / sizeof(**array);
> + _rows = len / cols;
> + if (len % cols != 0 || !cols || !_rows) {
Wouldn't it be nice to have a condition test first?
If cols is 0, divide it by 0 and check the condition.
> + dev_warn(dev, "%s property define error.\n", propname);
> + ret = -EINVAL;
> + goto out;
> + }
> +
> + *array = devm_kcalloc(dev, len, sizeof(**array), GFP_KERNEL);
> + if (!*array) {
> + ret = -ENOMEM;
> + goto out;
> + }
> +
> + ret = of_property_read_u32_array(np, propname, *array, len);
> +
> + if (ret) {
Wouldn't it be better to check the return value without a space?
> @@ -99,11 +146,13 @@ static int ufshcd_parse_clock_info(struct ufs_hba *hba)
>
> if (!strcmp(name, "ref_clk"))
> clki->keep_link_active = true;
> - dev_dbg(dev, "%s: min %u max %u name %s\n", "freq-table-hz",
> - clki->min_freq, clki->max_freq, clki->name);
> + dev_dbg(dev, "clk %s: scalable(%u), min(%u), max(%u)\n",
> + clki->name, clki->scalable, clki->min_freq, clki->max_freq);
> list_add_tail(&clki->list, &hba->clk_list_head);
> }
> out:
> + devm_kfree(dev, scalable);
> + devm_kfree(dev, clkfreq);
I have a question.
scalable and clkfreq are used only in this function. (I think... not managed resource but temporary)
Do you have the advantage of using devm_kcalloc instead of kcalloc?
> @@ -958,41 +959,27 @@ static int ufshcd_set_clk_freq(struct ufs_hba *hba, bool scale_up)
>
> list_for_each_entry(clki, head, list) {
> if (!IS_ERR_OR_NULL(clki->clk)) {
> - if (scale_up && clki->max_freq) {
> - if (clki->curr_freq == clki->max_freq)
> - continue;
> -
> - ret = clk_set_rate(clki->clk, clki->max_freq);
> - if (ret) {
> - dev_err(hba->dev, "%s: %s clk set rate(%dHz) failed, %d\n",
> - __func__, clki->name,
> - clki->max_freq, ret);
> - break;
> - }
> - trace_ufshcd_clk_scaling(dev_name(hba->dev),
> - "scaled up", clki->name,
> - clki->curr_freq,
> - clki->max_freq);
> + target_rate = scale_up ? clki->max_freq : clki->min_freq;
>
> - clki->curr_freq = clki->max_freq;
> + if (!target_rate || clki->curr_freq == target_rate)
> + continue;
>
> - } else if (!scale_up && clki->min_freq) {
> - if (clki->curr_freq == clki->min_freq)
> - continue;
> + if (!clki->scalable)
> + goto skip;
>
> - ret = clk_set_rate(clki->clk, clki->min_freq);
> - if (ret) {
> - dev_err(hba->dev, "%s: %s clk set rate(%dHz) failed, %d\n",
> - __func__, clki->name,
> - clki->min_freq, ret);
> - break;
> - }
> - trace_ufshcd_clk_scaling(dev_name(hba->dev),
> - "scaled down", clki->name,
> - clki->curr_freq,
> - clki->min_freq);
> - clki->curr_freq = clki->min_freq;
> + ret = clk_set_rate(clki->clk, target_rate);
> + if (ret) {
> + dev_err(hba->dev, "%s: %s clk set rate(%dHz) failed, %d\n",
> + __func__, clki->name,
> + target_rate, ret);
> + break;
> }
> +skip:
> + trace_ufshcd_clk_scaling(dev_name(hba->dev),
> + clki->name, scale_up,
> + clki->curr_freq,
> + target_rate);
> + clki->curr_freq = target_rate;
> }
> dev_dbg(hba->dev, "%s: clk: %s, rate: %lu\n", __func__,
> clki->name, clk_get_rate(clki->clk));
clki->scalable should always be confirmed before calling clk_set_rate().
Making this as a function,
I think you can get rid of goto state
and prevent the case to be called clk_set_rate() without checking the clki->scalable. :)
Thanks,
Jinyoung.
