On 6/17/2019 7:04 PM, Andrey Smirnov wrote:
> Simplify clock initialization code by converting it to use clk-bulk,
> devres and soc_device_match() match table. No functional change
> intended.
Subject is misspelled.
> +struct clk_bulk_caam {
> + const struct clk_bulk_data *clks;
> + int num_clks;
> +};
clks could be an array[0] at the end to avoid an additional allocation.
> +static void disable_clocks(void *private)
> +{
> + struct clk_bulk_caam *context = private;
> +
> + clk_bulk_disable_unprepare(context->num_clks,
> + (struct clk_bulk_data *)context->clks);
> +}
Not sure using devm for this is worthwhile. Maybe someday CAAM clks will
be enabled dynamically?
It would be make sense to reference "clk" instead of "clocks".
> +static int init_clocks(struct device *dev,
> + const struct clk_bulk_caam *data)
> +{
> + struct clk_bulk_data *clks;
> + struct clk_bulk_caam *context;
> + int num_clks;
> + int ret;
> +
> + num_clks = data->num_clks;
> + clks = devm_kmemdup(dev, data->clks,
> + data->num_clks * sizeof(data->clks[0]),
> + GFP_KERNEL);
> + if (!clks)
> + return -ENOMEM;
> +
> + ret = devm_clk_bulk_get(dev, num_clks, clks);
> + if (ret) {
> + dev_err(dev,
> + "Failed to request all necessary clocks\n");
> + return ret;
> + }
> +
> + ret = clk_bulk_prepare_enable(num_clks, clks);
> + if (ret) {
> + dev_err(dev,
> + "Failed to prepare/enable all necessary clocks\n");
> + return ret;
> + }
> +
> + context = devm_kzalloc(dev, sizeof(*context), GFP_KERNEL);
> + if (!context)
> + return -ENOMEM;
Aren't clks left enabled if this fails? Can move this allocation higher.
> + context->num_clks = num_clks;
> + context->clks = clks;
> +
> + ret = devm_add_action_or_reset(dev, disable_clocks, context);
> + if (ret)
> + return ret;
> +
> + return 0;
> +}
> static int caam_probe(struct platform_device *pdev)
> {
> int ret, ring, gen_sk, ent_delay = RTSDCTL_ENT_DLY_MIN;
> u64 caam_id;
> - static const struct soc_device_attribute imx_soc[] = {
> - {.family = "Freescale i.MX"},
> - {},
> - };
> + const struct soc_device_attribute *soc_attr;
This "soc_attr" is difficult to understand, maybe rename to something
like "imx_soc_match"?
