Re: [PATCH v4 5/6] clk: fsl-sai: Extract clock setup into fsl_sai_clk_register()

[Marek Vasut]

On 4/7/26 9:03 PM, Brian Masney wrote:

[...]

> > +static int fsl_sai_clk_register(struct device *dev, void __iomem *base,
> > +				spinlock_t *lock, struct clk_divider *div,
> > +				struct clk_gate *gate, struct clk_hw **hw,
> > +				const int gate_bit, const int dir_bit,
> > +				const int div_reg, char *name)
> > +{
> > +	const struct fsl_sai_data *data = device_get_match_data(dev);
> > +	struct clk_parent_data pdata = { .index = 0 };
> > +	struct clk_hw *chw;
> > +	char *cname;
> > +
> > +	gate->reg = base + data->offset + I2S_CSR;
> > +	gate->bit_idx = gate_bit;
> > +	gate->lock = lock;
> > +
> > +	div->reg = base + div_reg;
> > +	div->shift = CR2_DIV_SHIFT;
> > +	div->width = CR2_DIV_WIDTH;
> > +	div->lock = lock;
> > +
> > +	cname = devm_kasprintf(dev, GFP_KERNEL, "%s.%s",
> > +			       of_node_full_name(dev->of_node), name);
> > +	if (!cname)
> > +		return -ENOMEM;
> > +
> > +	chw = devm_clk_hw_register_composite_pdata(dev, cname,
> > +						   &pdata, 1, NULL, NULL,
> > +						   &div->hw,
> > +						   &clk_divider_ops,
> > +						   &gate->hw,
> > +						   &clk_gate_ops,
> > +						   CLK_SET_RATE_GATE);
> > +	if (IS_ERR(hw))
> > +		return PTR_ERR(hw);
>
> s/hw/chw/ on the two lines above.
>
> > +
> > +	*hw = chw;
> > +
> > +	/* Set clock direction */
> > +	writel(dir_bit, base + div_reg);
>
> The previous behavior of the code was to call writel() and then register
> the clk. This flips it. Just to be sure no regressions are introduced,
> should we keep the old behavior?
I believe this is the correct ordering -- if the clock registration 
fails, the clock direction is not going to be configured into hardware. 
The clock driver does not operate the clock direction bit after this 
write, so whether this write happens before or after the clock 
registration has no other impact.

The rest of the items are addressed in V5, thanks !