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

[Bryan O'Donoghue]

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.

---
bod