RE: [PATCH 1/1] scsi: ufs: add clock-scalable property for clk scaling

[Jinyoung CHOI]

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