Re: [PATCH V3 2/5] clk: qcom: Remove support to set CAL_L field in lucid evo pll configure

[Konrad Dybcio]

On 9.06.2023 13:49, Jagadeesh Kona wrote:
> Hi Dmitry,
> 
> Thanks for your review!
> 
> On 6/1/2023 8:16 PM, Dmitry Baryshkov wrote:
>> On 01/06/2023 17:34, Jagadeesh Kona wrote:
>>> For lucid evo and ole pll's the CAL_L, RINGOSC_CAL_L and L_VAL are
>>> part of the same register, hence update the l configuration value
>>> to include these fields across all the chipsets.
>>>
>>> Since the l configuration value now includes both L and CAL_L fields,
>>> there is no need to explicitly set CAL_L field again in lucid evo pll
>>> configure, Hence remove support to explicity set CAL_L field for evo pll.
>>>
>>> Fixes: 260e36606a03 ("clk: qcom: clk-alpha-pll: add Lucid EVO PLL configuration interfaces")
>>> Signed-off-by: Taniya Das <quic_tdas@xxxxxxxxxxx>
>>> Signed-off-by: Jagadeesh Kona <quic_jkona@xxxxxxxxxxx>
>>> ---
>>> Changes since V2:
>>>   - Squashed update L val and remove explicit cal_l configuration to single patch
>>>   - Updated L configuration for gpucc-sm8450 as well which was merged recently
>>> Changes since V1:
>>>   - Newly added.
>>>
>>>   drivers/clk/qcom/camcc-sm8450.c  | 24 ++++++++++++++++--------
>>>   drivers/clk/qcom/clk-alpha-pll.c |  6 +-----
>>>   drivers/clk/qcom/dispcc-sm8450.c |  6 ++++--
>>>   drivers/clk/qcom/dispcc-sm8550.c |  6 ++++--
>>>   drivers/clk/qcom/gpucc-sa8775p.c |  6 ++++--
>>>   drivers/clk/qcom/gpucc-sm8450.c  |  6 ++++--
>>>   6 files changed, 33 insertions(+), 21 deletions(-)
>>
>> I'd say, this is still not a correct solution from my point of view. A correct solution would be to follow the existing code and use constants for the constant values (of CAL_L, and RINGOSC_CAL_L).
>>
> 
> Sure, will keep the existing code as is and will remove this patch in the next series.
> 
>>>
>>> diff --git a/drivers/clk/qcom/camcc-sm8450.c b/drivers/clk/qcom/camcc-sm8450.c
>>> index 51338a2884d2..6a5a08f88598 100644
>>> --- a/drivers/clk/qcom/camcc-sm8450.c
>>> +++ b/drivers/clk/qcom/camcc-sm8450.c
>>> @@ -57,7 +57,8 @@ static const struct pll_vco rivian_evo_vco[] = {
>>>   static const struct clk_parent_data pll_parent_data_tcxo = { .index = DT_BI_TCXO };
>>>   static const struct alpha_pll_config cam_cc_pll0_config = {
>>> -    .l = 0x3e,
>>> +    /* .l includes CAL_L_VAL, L_VAL fields */
>>> +    .l = 0x0044003e,
>>>       .alpha = 0x8000,
>>>       .config_ctl_val = 0x20485699,
>>>       .config_ctl_hi_val = 0x00182261,
>>> @@ -128,7 +129,8 @@ static struct clk_alpha_pll_postdiv cam_cc_pll0_out_odd = {
>>>   };
> 
> [skipped]
> 
> Thanks & Regards,
> Jagadeesh
Another non-patch-related nit, you don't have to (and shouldn't) cut off
parts of the email unless it helps you "get to the point". You can also
include your signature after the last paragraph you reply with.

Konrad