Re: [PATCH 2/6] ASoC: meson: add audin main module and I2S formatter

[Valerio Setti <vsetti@baylibre.com>]

Thanks for your review and feedbacks.


On 9/13/26 11:41 AM, Krzysztof Kozlowski wrote:
> On Sat, Sep 12, 2026 at 12:14:32AM +0200, Valerio Setti wrote:
>> +static int meson_gx_audin_probe(struct platform_device *pdev)
>> +{
>> +	struct device *dev = &pdev->dev;
>> +	struct resource *res;
>> +	void __iomem *mmio;
>> +	struct regmap *regmap;
>> +	struct clk *clk;
>> +	int ret;
>> +
>> +	ret = device_reset(dev);
>> +	if (ret)
>> +		return dev_err_probe(dev, ret, "Failed to reset device\n");
>> +
>> +	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
>> +	if (!res)
>> +		return dev_err_probe(dev, -EINVAL, "Failed to get memory resource\n");
>> +
>> +	/*
>> +	 * Do not use devm_platform_ioremap_resource() here: it would claim the
>> +	 * whole AUDIN window exclusively and the FIFO children would then fail
>> +	 * to request their own sub-ranges.
>> +	 */
> 
> And this should point you that child is not a separate device.
> 
> Please open your datasheet and read what is the address space of this
> device. You claim here ENTIRE address space. If you insist that entire
> address space is not owned by this device, then you misrepresented
> hardware in the DT anyway.

As I mentioned in the cover letter I designed it this way so that I 
could easily re-use the same driver code for all 3 FIFOs just offsetting 
the base address. I knew it was a bit hackish a solution, but IMO that 
was worth it since it was simplifying a bit driver's code. Perhaps it 
was more software-oriented rather than hardware related.
The other possible approach is to fully copy what AIU is doing and 
allocate each FIFO as a DAI. I'll follow that design then, but if you 
have any better idea please let me know.

>> +	mmio = devm_ioremap(dev, res->start, resource_size(res));
>> +	if (!mmio)
>> +		return dev_err_probe(dev, -ENOMEM, "Failed to remap memory\n");
>> +
>> +	regmap = devm_regmap_init_mmio(dev, mmio, &audin_regmap_cfg);
>> +	if (IS_ERR(regmap))
>> +		return dev_err_probe(dev, PTR_ERR(regmap), "Failed to init regmap\n");
>> +
>> +	clk = devm_clk_get_enabled(dev, NULL);
>> +	if (IS_ERR(clk))
>> +		return dev_err_probe(dev, PTR_ERR(clk), "Failed to get clock\n");
>> +
>> +	ret = gx_formatter_create(dev, &audin_dapm_widgets[0], &audin_formatter_i2s_drv, regmap);
>> +	if (ret)
>> +		return dev_err_probe(dev, ret, "Failed to allocate formatter\n");
>> +
>> +	ret = devm_snd_soc_register_component(dev, &audin_component, NULL, 0);
>> +	if (ret)
>> +		return dev_err_probe(dev, ret, "Failed to register component\n");
>> +
>> +	ret = devm_of_platform_populate(dev);
>> +	if (ret)
>> +		return dev_err_probe(dev, ret, "Failed to probe child nodes\n");
>> +
>> +	return 0;
>> +}
>> +
>> +static void meson_gx_audin_remove(struct platform_device *pdev)
>> +{
>> +	gx_formatter_free(&audin_dapm_widgets[0]);
>> +}
>> +
>> +static const struct of_device_id meson_gx_audin_of_match[] = {
>> +	{ .compatible = "amlogic,meson-gxbb-audin" },
>> +	{ .compatible = "amlogic,meson-gxl-audin" },
> 
> Why do you have two entries here and no fallbacks? This makes no sense,
> really. I have no more doubts that your "gx" is a real SoC/

OK. Genuine question about naming here: would "amlogic,gx-audin" be 
fine? Because in "meson-gx.dtsi" I see many surrounding nodes have 
"amlogic,meson-gx-" prefix, but that doesn't seem to follow design rules 
from "writing-bindings.rst".


-- 
Valerio