Re: [PATCH v4 2/3] clk: qcom: camcc-glymur: Add camera clock controller driver

[Jagadeesh Kona]

On 5/18/2026 5:51 PM, Bryan O'Donoghue wrote:
> On 18/05/2026 11:23, Jagadeesh Kona wrote:
>>
>>
>> On 5/18/2026 1:05 PM, Bryan O'Donoghue wrote:
>>> On 17/05/2026 18:33, Jagadeesh Kona wrote:
>>>> +/* 1200.0 MHz Configuration */
>>>> +static const struct alpha_pll_config cam_cc_pll0_config = {
>>>> +    .l = 0x3e,
>>>> +    .alpha = 0x8000,
>>>> +    .config_ctl_val = 0x25c400e7,
>>>> +    .config_ctl_hi_val = 0x0a8060e0,
>>>> +    .config_ctl_hi1_val = 0xf51dea20,
>>>> +    .user_ctl_val = 0x00008408,
>>>> +    .user_ctl_hi_val = 0x00000002,
>>>> +};
>>>
>>> I'll again push back on these magic numbers.
>>>
>>> At the very least you should be mentioning in the cover letter log why you _aren't_ making that change.
>>>
>>> Just reposting and hoping it slips by the person making the comment isn't too cool.
>>>
>>> Why can't qcom update the python? script that generates this code to enumerate fields instead of magic numbers here ?
>>>
>>> I get you don't want to do it but, just ignoring the review feedback is no OK.
>>>
>>> What gives ?
>>>
>>
>> Hi Bryan,
>>
>> I haven't ignored your comments & already responded to your earlier comment on why the bit fields are not
>> defined. Most of these values are static settings we get from PLL HW team and we program them only once
>> as is during bootup and are never reused again anywhere from PLL code, so these bits are not defined.
>>
>> Please find the earlier responses for your comments below:
>> https://lore.kernel.org/all/b92a2cbb-fe8d-4378-aa02-d91e2e4dfff4@xxxxxxxxxxxxxxxx/
>> https://lore.kernel.org/all/009ecdbb-2297-44eb-862d-233e3290691c@xxxxxxxxxxxxxxxx/
>>
>> Thanks,
>> Jagadeesh
> 
> That's not in your overview letter so generally I'd advise to include things like "did X because Y" - "didn't do Q because Z" anyway, how does it make a difference if the values are static ?
> 
> They are no less magic numbers that way.
> 
> What exactly is the resistance to defining the bits ?
> 
> I'll state again - when a vendor is submitting something upstream where that vendor 100% controls their own documentation - there's no reason at all to be presenting magic hex numbers - even more the case with generated code.
> 
> Just update the script to enumerate the bit fields, I honestly don't get the aversion.
> 

Hi Bryan,

There’s no standard interface for these bits, and bit definitions/fields vary across PLL types.
So, common macros aren’t feasible and would need redefinitions per controller. Since these bits
are not reused elsewhere, IMO directly using values from the hardware documentation keeps the
implementation simpler, avoids unnecessary abstraction, and makes debugging—through direct
comparison with the hardware spec easier.

Thanks,
Jagadeesh