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

[Marek Vasut <marex@nabladev.com>]

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.