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

[Marek Vasut]

On 4/8/26 11:58 PM, Brian Masney wrote:

Hi,

> > +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);
>
> Sashiko has the following feedback:
> https://sashiko.dev/#/patchset/20260407211123.77602-1-marex%40nabladev.com
>
>      Does using of_node_full_name() break debugfs directory creation?
>      
>      The full node name usually contains slashes (e.g., soc/bus@1000/sai@2000),
>      which causes lookup_one_len() to reject the name with -EACCES when the CCF
>      tries to create the directories.

This is nonsense, the node name is clock-controller@12340000 or 
something like that .

>      Also, if the device is instantiated without Device Tree, this evaluates to
>      "<no-node>", potentially causing collisions if multiple instances exist.
>      Would dev_name(dev) be more appropriate here?

This driver is always instantiated from DT.

> > +	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(chw))
> > +		return PTR_ERR(chw);
> > +
> > +	*hw = chw;
> > +
> > +	/* Set clock direction */
> > +	writel(dir_bit, base + div_reg);
>
> Sashiko also has the following feedback:
>
>      Is it safe to initialize the hardware register after registering the clock
>      with the CCF?
The BCD/MOE/DIVEN/DIV fields of the CR2/MCR registers are all zeroes, so 
yes, either order is safe.