Re: [PATCH v6 3/8] clk: qcom: Add generic clkref_en support

[Qiang Yu]

On Mon, Jun 22, 2026 at 02:13:22PM +0200, Konrad Dybcio wrote:
> On 6/22/26 7:11 AM, Qiang Yu wrote:
> > Before XO refclk is distributed to PCIe/USB/eDP PHYs, it passes through
> > a QREF block. QREF is powered by dedicated LDO rails, and the clkref_en
> > register controls whether refclk is gated through to the PHY side.
> > 
> > These clkref controls are different from typical GCC branch clocks:
> > - only a single enable bit is present, without branch-style config bits
> > - regulators must be voted before enable and unvoted after disable
> > 
> > Model this as a dedicated clk_ref clock type with custom clk_ops instead
> > of reusing struct clk_branch semantics.
> > 
> > Also provide a common registration/probe API so the same clkref model
> > can be reused regardless of where clkref_en registers are placed, e.g.
> > TCSR on glymur and TLMM on SM8750.
> > 
> > Signed-off-by: Qiang Yu <qiang.yu@xxxxxxxxxxxxxxxx>
> > ---
> 
> [...]
> 
> > +	for (clk_idx = 0; clk_idx < num_clk_refs; clk_idx++) {
> > +		clk_ref = &clk_refs[clk_idx];
> > +		desc = &descs[clk_idx];
> > +
> > +		if (!desc->name)
> > +			continue;
> 
> Carrying over from the previous discussion:
> 
> > // this allows "holes" in dt-bindings for $reasons
> > if (!desc)
> > 	continue;
> > 
> > // this makes sure the programmer did not omit something important
> > // while not taking the entire system down
> > if (WARN_ON(!desc->name)
> > 	continue;
> >
> The NULL name check is intentional - the descriptor array is indexed by
> clock ID, and mahua has fewer clocks than glymur, leaving holes at
> certain indices. So this is expected at runtime. WARN_ON would be noise
> log here.
> 
> 
> ->
> 
> Your worry is captured by nullchecking `desc` (i.e. descs[clk_idx])
> 
> because in the mahua case we've got (ephemeral indices)
> 
> tcsr_cc_mahua_clk_descs[] = {
> 	[0] = { foo },
> 	// [1] is unassigned - OK
> 	[2] = { bar },
> };
> 
> while (!desc->name) checks for:
> 
> tcsr_cc_mahua_clk_descs[] = {
> 	[0] = { .name = "foo", .offset = 0x10 },
> 	// name is NULL by virtue of partial struct initialization
> 	[1] = { .offset = 0x20 },
> };
> 
> however I overlooked that we actually just have a normal array of
> structs.. if we turn it into a struct pointer array with assigmnents
> like:
> 
> [TCSR_EDP_CLKREF_EN] = &(const struct qcom_clk_ref_desc) {
> 	.name = "foo",
> 	...
> };
> 
> we can achieve that

Okay, I got you. I'll switch to a pointer array in v7 so that we can check
!desc and !desc->name for different purpose.

- Qiang Yu