Re: [PATCH 2/4] clk: qcom: common: introduce qcom_cc_sync_state()

[Konrad Dybcio]

On 6/15/26 4:48 PM, Brian Masney wrote:
> Hi Konrad / Dmitry,
> 
> On Mon, Jun 15, 2026 at 04:33:17PM +0200, Konrad Dybcio wrote:
>> On 6/15/26 4:24 PM, Brian Masney wrote:
>>> On Sun, Jun 07, 2026 at 01:30:03PM +0300, Dmitry Baryshkov wrote:
>>>> On Sun, Jun 07, 2026 at 01:43:06AM -0300, Val Packett wrote:
>>>>>
>>>>> On 6/6/26 8:15 AM, Dmitry Baryshkov wrote:
>>>>>> On Wed, Jun 03, 2026 at 10:21:47AM -0400, Brian Masney wrote:
>>>>>>> Several qcom clk providers currently have a sync_state helper set to
>>>>>>> icc_sync_state(). With an upcoming change to the clk framework, if
>>>>>>> sync_state is not defined for the device, then the clk framework sets it
>>>>>>> to clk_sync_state().
>>>>>>> [..]
>>>>>>> @@ -464,5 +466,12 @@ int qcom_cc_probe_by_index(struct platform_device *pdev, int index,
>>>>>>>   }
>>>>>>>   EXPORT_SYMBOL_GPL(qcom_cc_probe_by_index);
>>>>>>> +void qcom_cc_sync_state(struct device *dev)
>>>>>>> +{
>>>>>>> +	icc_sync_state(dev);
>>>>>> Only if desc->icc_hws != 0, otherwise it will mess the interconnect
>>>>>> internals. You might need to set drvdata to desc.
>>>>>
>>>>> Hmm…
>>>>>
>>>>> Currently icc_sync_state does not seem to use the dev argument at all.
>>>>>
>>>>> How would something get messed up, now or whenever icc_sync_state changes?
>>>>> o.0
>>>>
>>>> Yes :-(
>>>
>>> Sorry about the delayed response since I was out of town all last week.
>>> Just to be clear, the missing check for 'desc->icc_hws != 0' is a bug that
>>> existed prior to my change, and I should label it as such with a Fixes
>>> tag when I post my next version?
>>
>> Up until this change, having icc_hws > 0 but lacking icc_sync_state
>> (or the reverse) would be be considered programmer error
> 
> icc_hws > 0 but lacking icc_sync_state (or the reverse) makes sense as a
> programmer error. However...
> 
>> Starting with patch 4, this gets assigned unconditionally, so there's
>> no prior bug to be fixed
> 
> I don't see where that situation happens here. All of the places where
> icc_sync_state() was previously called, the new code now calls
> qcom_cc_sync_state() -> icc_sync_state(). (There is
> qcom_msm8996_cbf_icc_sync_state() that needs to be modified.)
> 
> In patch 4 of this series, it sets up a framework level sync_state()
> callback with dev_set_drv_sync_state(). If a sync_state already exists,
> then that call will fail with -EBUSY, and it will leave the existing
> sync_state() intact. So it's not calling sync_state twice. I will
> clarify that on the comment.

Dmitry and I are referring to the situation where the clock driver isn't
an interconnect provider but icc_sync_state() still executes. That could
not have been the case before, since most clock drivers didn't come with
any sort of .sync_state()

Konrad