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

[Valerio Setti]

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