| Message ID | 20240304211444.1031693-1-cujomalainey@chromium.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | ASoC: da7219: read fmw property to get mclk for non-dts systems | expand |
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.
On Mon, Mar 4, 2024 at 1:23 PM Mark Brown <broonie@kernel.org> wrote: > > 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. No problem, I can clean it up, I figured a good starting point would be to just revive the original that was sent in 2018 https://mailman.alsa-project.org/hyperkitty/list/alsa-devel@alsa-project.org/message/26IVUCF7KMKNL7LZWAWWNFF3KB2TURSA/ We have been carrying this in our tree the whole time, just caught it and figure I would try and get it up stream again > > > 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. I will take a look at this, note that it appears the original author is no longer at AMD as the emails are bouncing.
On Mon, Mar 04, 2024 at 01:50:27PM -0800, Curtis Malainey wrote: > On Mon, Mar 4, 2024 at 1:23 PM Mark Brown <broonie@kernel.org> wrote: > > 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. > No problem, I can clean it up, I figured a good starting point would > be to just revive the original that was sent in 2018 > https://mailman.alsa-project.org/hyperkitty/list/alsa-devel@alsa-project.org/message/26IVUCF7KMKNL7LZWAWWNFF3KB2TURSA/ That seems to have had 0day find build failures immediately so didn't get looked at :/
On Mon, Mar 4, 2024 at 1:50 PM Curtis Malainey <cujomalainey@google.com> wrote: > > On Mon, Mar 4, 2024 at 1:23 PM Mark Brown <broonie@kernel.org> wrote: > > > > On Mon, Mar 04, 2024 at 01:14:43PM -0800, cujomalainey@chromium.org wrote: > > > > > No problem, I can clean it up, I figured a good starting point would > be to just revive the original that was sent in 2018 > https://mailman.alsa-project.org/hyperkitty/list/alsa-devel@alsa-project.org/message/26IVUCF7KMKNL7LZWAWWNFF3KB2TURSA/ > > We have been carrying this in our tree the whole time, just caught it > and figure I would try and get it up stream again So after a bunch of debugging on our 5.10 kernel it actually appears this property is useless and the system is already attaching the mclk under the clock defined in the property. Maybe someone pushed a coreboot update to fix this later which rendered this patch useless? > > > > > > > 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. > > I will take a look at this, note that it appears the original author > is no longer at AMD as the emails are bouncing. To answer this for the sake of sharing knowledge, there is a clk_add_alias API.
diff --git a/include/sound/da7219.h b/include/sound/da7219.h index dde4542b084f3..4cf4c52be3f7f 100644 --- a/include/sound/da7219.h +++ b/include/sound/da7219.h @@ -40,6 +40,8 @@ struct da7219_pdata { const char *dai_clk_names[DA7219_DAI_NUM_CLKS]; + const char *mclk_name; + /* Mic */ enum da7219_micbias_voltage micbias_lvl; enum da7219_mic_amp_in_sel mic_amp_in_sel; diff --git a/sound/soc/codecs/da7219.c b/sound/soc/codecs/da7219.c index 311ea7918b312..800f61d993043 100644 --- a/sound/soc/codecs/da7219.c +++ b/sound/soc/codecs/da7219.c @@ -1782,6 +1782,8 @@ static struct da7219_pdata *da7219_fw_to_pdata(struct device *dev) pdata->dai_clk_names[DA7219_DAI_WCLK_IDX], pdata->dai_clk_names[DA7219_DAI_BCLK_IDX]); + device_property_read_string(dev, "dlg,mclk-name", &pdata->mclk_name); + if (device_property_read_u32(dev, "dlg,micbias-lvl", &of_val32) >= 0) pdata->micbias_lvl = da7219_fw_micbias_lvl(dev, of_val32); else @@ -2521,7 +2523,11 @@ static int da7219_probe(struct snd_soc_component *component) da7219_handle_pdata(component); /* Check if MCLK provided */ - 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"); + if (IS_ERR(da7219->mclk)) { if (PTR_ERR(da7219->mclk) != -ENOENT) { ret = PTR_ERR(da7219->mclk);