Re: [PATCH] ASoC: da7219: read fmw property to get mclk for non-dts systems

From: Mark Brown <broonie@kernel.org>
To: cujomalainey@chromium.org
Cc: alsa-devel@alsa-project.org,
	Akshu Agrawal <akshu.agrawal@amd.com>,
	Karthik Ramasubramanian <kramasub@google.com>,
	Support Opensource <support.opensource@diasemi.com>,
	Jaroslav Kysela <perex@perex.cz>, Takashi Iwai <tiwai@suse.com>,
	Liam Girdwood <lgirdwood@gmail.com>
Subject: Re: [PATCH] ASoC: da7219: read fmw property to get mclk for non-dts systems
Date: Mon, 4 Mar 2024 21:23:38 +0000	[thread overview]
Message-ID: <fcd7d8d8-3e71-4c43-b5e8-075369b4b9ec@sirena.org.uk> (raw)
In-Reply-To: <20240304211444.1031693-1-cujomalainey@chromium.org>

[-- Attachment #1: Type: text/plain, Size: 1342 bytes --]

On Mon, Mar 04, 2024 at 01:14:43PM -0800, cujomalainey@chromium.org wrote:

> Non-dts based systems can use ACPI DSDT to pass on the mclk
> to da7219.
> This enables da7219 mclk to be linked to system clock.
> Enable/Disable of the mclk is already handled in the codec so
> platform drivers don't have to explicitly do handling of mclk.

...

> +	device_property_read_string(dev, "dlg,mclk-name", &pdata->mclk_name);
> +

...

> -	da7219->mclk = clk_get(component->dev, "mclk");
> +	if (da7219->pdata->mclk_name)
> +		da7219->mclk = clk_get(NULL, da7219->pdata->mclk_name);
> +	if (!da7219->mclk)
> +		da7219->mclk = clk_get(component->dev, "mclk");

I would never have guessed from the changelog that what this change
actually does is provide a mechanism for overriding the name we use to
request the MCLK.  I had thought this was adding clock handling to a
driver that had none.  The changelog should say what the change is
doing.

Having a firmware property for this is obviously broken for DT systems,
this should be limited to ACPI systems if it's going to be there at all.
It would be nicer if it were implementeded by having some ACPI specific
code link whatever the configured clock name is to "mclk" - I don't know
if the clock API has an equivalent to regulator_register_supply_alias()
but that's the sort of thing I'm thinking of.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]