Re: [PATCH v3 3/5] clk: samsung: add Exynos ACPM clock driver

[Krzysztof Kozlowski <krzk@kernel.org>]

On 08/09/2025 09:39, Tudor Ambarus wrote:
>>> +
>>> +	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
> 
> using BUILD_BUG_ON_MSG? that would work, see below the why.
> 
>> 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]?
> 
> so that it works correctly with of_clk_hw_onecell_get() in case the clocks

Ah true, hws[] has to be indexed by ID.

> IDs are not starting from 0 or are reordered when defined. For example let's
> consider clock ID 1 is wrongly defined at index 0 in the array. When someone
> references clock ID 1 in the device tree, and we use of_clk_hw_onecell_get,
> it would get the clock defined at index 1.
> 
> In my case the clocks start from index 0 and they are defined in ascending
> order with no gaps, so the check is gratuitously made. I wanted to have some
> sanity check. Do you still think I shall remove the check and use hws[i]?


Look at some users of of_clk_hw_onecell_get() - they all don't care
about this and do:

441         for (idx = 0; idx < count; idx++) {
442                 struct scmi_clk *sclk = &sclks[idx];

without any checks.

I just do not see why runtime check is necessary. This is purely build
time relation and either we do not care, because the code should be
synced between one and other place, or (if you care) then it must be
build time check.



Best regards,
Krzysztof