On 03/09/2025 15:56, Tudor Ambarus wrote:
> +
> +static int acpm_clk_probe(struct platform_device *pdev)
> +{
> + enum acpm_clk_dev_type type = platform_get_device_id(pdev)->driver_data;
> + const struct acpm_clk_driver_data *drv_data;
> + const struct acpm_handle *acpm_handle;
> + struct clk_hw_onecell_data *clk_data;
> + struct clk_hw **hws;
> + struct device *dev = &pdev->dev;
> + struct acpm_clk *aclks;
> + unsigned int mbox_chan_id;
> + int i, err, count;
> +
> + switch (type) {
> + case GS101_ACPM_CLK_ID:
> + drv_data = &acpm_clk_gs101;
Just use acpm_clk_gs101 directly (see also further comment).
> + break;
> + default:
> + return dev_err_probe(dev, -EINVAL, "Invalid device type\n");
> + }
> +
> + acpm_handle = devm_acpm_get_by_node(dev, dev->parent->of_node);
> + if (IS_ERR(acpm_handle))
> + return dev_err_probe(dev, PTR_ERR(acpm_handle),
> + "Failed to get acpm handle.\n");
> +
> + count = drv_data->nr_clks;
> + mbox_chan_id = drv_data->mbox_chan_id;
> +
> + clk_data = devm_kzalloc(dev, struct_size(clk_data, hws, count),
> + GFP_KERNEL);
> + if (!clk_data)
> + return -ENOMEM;
> +
> + clk_data->num = count;
> + hws = clk_data->hws;
> +
> + aclks = devm_kcalloc(dev, count, sizeof(*aclks), GFP_KERNEL);
> + if (!aclks)
> + return -ENOMEM;
> +
> + for (i = 0; i < count; i++) {
> + const struct acpm_clk_variant *variant = &drv_data->clks[i];
> + unsigned int id = variant->id;
> + struct acpm_clk *aclk;
> +
> + if (id >= count)
This is not possible. You control the IDs build time, so this must be
either build time check or no check. I vote for no check, because I
don't think the ID is anyhow related to number of clocks. What if (not
recommended but what if) the IDs have a gap and next ID is 1000. I see
your code using ID:
> + return dev_err_probe(dev, -EINVAL,
> + "Invalid ACPM clock ID.\n");
> +
> + aclk = &aclks[id];
> + aclk->id = id;
> + aclk->handle = acpm_handle;
> + aclk->mbox_chan_id = mbox_chan_id;
> +
> + hws[id] = &aclk->hw;
^^^ here, but why do you need it? Why it cannot be hws[i]?
> +
> + err = acpm_clk_ops_init(dev, aclk, variant->name);
> + if (err)
> + return dev_err_probe(dev, err,
> + "Failed to register clock.\n");
> + }
> +
> + return devm_of_clk_add_hw_provider(dev, of_clk_hw_onecell_get,
> + clk_data);
> +}
> +
> +static const struct platform_device_id acpm_clk_id[] = {
> + { "gs101-acpm-clk", GS101_ACPM_CLK_ID },
Please drop GS101_ACPM_CLK_ID here and in other places. It's dead code
now. It should be introduced with next users/devices.
> + {},
> +};
> +MODULE_DEVICE_TABLE(platform, acpm_clk_id);
Best regards,
Krzysztof
