* [PATCH 0/2] ASoC: Intel: boards: use software node API @ 2021-06-07 22:35 Pierre-Louis Bossart 2021-06-07 22:35 ` [PATCH 1/2] ASoC: Intel: boards: use software node API in SoundWire machines Pierre-Louis Bossart 2021-06-07 22:35 ` [PATCH 2/2] ASoC: Intel: boards: use software node API in Atom boards Pierre-Louis Bossart 0 siblings, 2 replies; 8+ messages in thread From: Pierre-Louis Bossart @ 2021-06-07 22:35 UTC (permalink / raw) To: alsa-devel Cc: tiwai, Hans de Goede, broonie, Pierre-Louis Bossart, Andy Shevchenko This is an update on an earlier contribution from Heikki Krogerus, with mainly changes to avoid the use of devm_ routines for Baytrail machine drivers. Heikki Krogerus (2): ASoC: Intel: boards: use software node API in SoundWire machines ASoC: Intel: boards: use software node API in Atom boards sound/soc/intel/boards/bytcht_es8316.c | 25 +++++++++-- sound/soc/intel/boards/bytcr_rt5640.c | 42 +++++++++++++++--- sound/soc/intel/boards/bytcr_rt5651.c | 47 +++++++++++++++++---- sound/soc/intel/boards/sof_sdw_rt711.c | 20 ++++++--- sound/soc/intel/boards/sof_sdw_rt711_sdca.c | 20 ++++++--- 5 files changed, 127 insertions(+), 27 deletions(-) -- 2.25.1 ^ permalink raw reply [flat|nested] 8+ messages in thread
* [PATCH 1/2] ASoC: Intel: boards: use software node API in SoundWire machines 2021-06-07 22:35 [PATCH 0/2] ASoC: Intel: boards: use software node API Pierre-Louis Bossart @ 2021-06-07 22:35 ` Pierre-Louis Bossart 2021-06-07 22:35 ` [PATCH 2/2] ASoC: Intel: boards: use software node API in Atom boards Pierre-Louis Bossart 1 sibling, 0 replies; 8+ messages in thread From: Pierre-Louis Bossart @ 2021-06-07 22:35 UTC (permalink / raw) To: alsa-devel Cc: Heikki Krogerus, tiwai, Pierre-Louis Bossart, Hans de Goede, broonie, Andy Shevchenko From: Heikki Krogerus <heikki.krogerus@linux.intel.com> The function device_add_properties() is going to be removed. Replacing it with software node API equivalents. Signed-off-by: Heikki Krogerus <heikki.krogerus@linux.intel.com> Signed-off-by: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com> --- sound/soc/intel/boards/sof_sdw_rt711.c | 20 +++++++++++++++----- sound/soc/intel/boards/sof_sdw_rt711_sdca.c | 20 +++++++++++++++----- 2 files changed, 30 insertions(+), 10 deletions(-) diff --git a/sound/soc/intel/boards/sof_sdw_rt711.c b/sound/soc/intel/boards/sof_sdw_rt711.c index 04074c09dded..b7c635c0fadd 100644 --- a/sound/soc/intel/boards/sof_sdw_rt711.c +++ b/sound/soc/intel/boards/sof_sdw_rt711.c @@ -24,19 +24,29 @@ static int rt711_add_codec_device_props(const char *sdw_dev_name) { struct property_entry props[MAX_NO_PROPS] = {}; + struct fwnode_handle *fwnode; struct device *sdw_dev; int ret; + if (!SOF_RT711_JDSRC(sof_sdw_quirk)) + return 0; + sdw_dev = bus_find_device_by_name(&sdw_bus_type, NULL, sdw_dev_name); if (!sdw_dev) return -EPROBE_DEFER; - if (SOF_RT711_JDSRC(sof_sdw_quirk)) { - props[0] = PROPERTY_ENTRY_U32("realtek,jd-src", - SOF_RT711_JDSRC(sof_sdw_quirk)); + props[0] = PROPERTY_ENTRY_U32("realtek,jd-src", + SOF_RT711_JDSRC(sof_sdw_quirk)); + + fwnode = fwnode_create_software_node(props, NULL); + if (IS_ERR(fwnode)) { + put_device(sdw_dev); + return PTR_ERR(fwnode); } - ret = device_add_properties(sdw_dev, props); + ret = device_add_software_node(sdw_dev, to_software_node(fwnode)); + + fwnode_handle_put(fwnode); put_device(sdw_dev); return ret; @@ -144,7 +154,7 @@ int sof_sdw_rt711_exit(struct device *dev, struct snd_soc_dai_link *dai_link) if (!sdw_dev) return -EINVAL; - device_remove_properties(sdw_dev); + device_remove_software_node(sdw_dev); put_device(sdw_dev); return 0; diff --git a/sound/soc/intel/boards/sof_sdw_rt711_sdca.c b/sound/soc/intel/boards/sof_sdw_rt711_sdca.c index 19496f0f9110..300a52d15506 100644 --- a/sound/soc/intel/boards/sof_sdw_rt711_sdca.c +++ b/sound/soc/intel/boards/sof_sdw_rt711_sdca.c @@ -24,19 +24,29 @@ static int rt711_sdca_add_codec_device_props(const char *sdw_dev_name) { struct property_entry props[MAX_NO_PROPS] = {}; + struct fwnode_handle *fwnode; struct device *sdw_dev; int ret; + if (!SOF_RT711_JDSRC(sof_sdw_quirk)) + return 0; + sdw_dev = bus_find_device_by_name(&sdw_bus_type, NULL, sdw_dev_name); if (!sdw_dev) return -EPROBE_DEFER; - if (SOF_RT711_JDSRC(sof_sdw_quirk)) { - props[0] = PROPERTY_ENTRY_U32("realtek,jd-src", - SOF_RT711_JDSRC(sof_sdw_quirk)); + props[0] = PROPERTY_ENTRY_U32("realtek,jd-src", + SOF_RT711_JDSRC(sof_sdw_quirk)); + + fwnode = fwnode_create_software_node(props, NULL); + if (IS_ERR(fwnode)) { + put_device(sdw_dev); + return PTR_ERR(fwnode); } - ret = device_add_properties(sdw_dev, props); + ret = device_add_software_node(sdw_dev, to_software_node(fwnode)); + + fwnode_handle_put(fwnode); put_device(sdw_dev); return ret; @@ -144,7 +154,7 @@ int sof_sdw_rt711_sdca_exit(struct device *dev, struct snd_soc_dai_link *dai_lin if (!sdw_dev) return -EINVAL; - device_remove_properties(sdw_dev); + device_remove_software_node(sdw_dev); put_device(sdw_dev); return 0; -- 2.25.1 ^ permalink raw reply related [flat|nested] 8+ messages in thread
* [PATCH 2/2] ASoC: Intel: boards: use software node API in Atom boards 2021-06-07 22:35 [PATCH 0/2] ASoC: Intel: boards: use software node API Pierre-Louis Bossart 2021-06-07 22:35 ` [PATCH 1/2] ASoC: Intel: boards: use software node API in SoundWire machines Pierre-Louis Bossart @ 2021-06-07 22:35 ` Pierre-Louis Bossart 2021-06-08 8:18 ` Hans de Goede 1 sibling, 1 reply; 8+ messages in thread From: Pierre-Louis Bossart @ 2021-06-07 22:35 UTC (permalink / raw) To: alsa-devel Cc: Heikki Krogerus, tiwai, Pierre-Louis Bossart, Hans de Goede, broonie, Andy Shevchenko From: Heikki Krogerus <heikki.krogerus@linux.intel.com> The function device_add_properties() is going to be removed. Replacing it with software node API equivalents. Co-developed-by: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com> Signed-off-by: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com> Signed-off-by: Heikki Krogerus <heikki.krogerus@linux.intel.com> --- sound/soc/intel/boards/bytcht_es8316.c | 25 ++++++++++++-- sound/soc/intel/boards/bytcr_rt5640.c | 42 +++++++++++++++++++---- sound/soc/intel/boards/bytcr_rt5651.c | 47 +++++++++++++++++++++----- 3 files changed, 97 insertions(+), 17 deletions(-) diff --git a/sound/soc/intel/boards/bytcht_es8316.c b/sound/soc/intel/boards/bytcht_es8316.c index a0af91580184..ef8ed3ff53af 100644 --- a/sound/soc/intel/boards/bytcht_es8316.c +++ b/sound/soc/intel/boards/bytcht_es8316.c @@ -38,6 +38,7 @@ struct byt_cht_es8316_private { struct snd_soc_jack jack; struct gpio_desc *speaker_en_gpio; bool speaker_en; + struct device *codec_dev; }; enum { @@ -461,6 +462,7 @@ static int snd_byt_cht_es8316_mc_probe(struct platform_device *pdev) const struct dmi_system_id *dmi_id; struct device *dev = &pdev->dev; struct snd_soc_acpi_mach *mach; + struct fwnode_handle *fwnode; const char *platform_name; struct acpi_device *adev; struct device *codec_dev; @@ -543,7 +545,16 @@ static int snd_byt_cht_es8316_mc_probe(struct platform_device *pdev) props[cnt++] = PROPERTY_ENTRY_BOOL("everest,jack-detect-inverted"); if (cnt) { - ret = device_add_properties(codec_dev, props); + fwnode = fwnode_create_software_node(props, NULL); + if (IS_ERR(fwnode)) { + put_device(codec_dev); + return PTR_ERR(fwnode); + } + + ret = device_add_software_node(codec_dev, to_software_node(fwnode)); + + fwnode_handle_put(fwnode); + if (ret) { put_device(codec_dev); return ret; @@ -556,6 +567,7 @@ static int snd_byt_cht_es8316_mc_probe(struct platform_device *pdev) /* see comment in byt_cht_es8316_resume */ GPIOD_OUT_LOW | GPIOD_FLAGS_BIT_NONEXCLUSIVE); put_device(codec_dev); + priv->codec_dev = codec_dev; if (IS_ERR(priv->speaker_en_gpio)) { ret = PTR_ERR(priv->speaker_en_gpio); @@ -567,7 +579,7 @@ static int snd_byt_cht_es8316_mc_probe(struct platform_device *pdev) dev_err(dev, "get speaker GPIO failed: %d\n", ret); fallthrough; case -EPROBE_DEFER: - return ret; + goto err; } } @@ -605,10 +617,15 @@ static int snd_byt_cht_es8316_mc_probe(struct platform_device *pdev) if (ret) { gpiod_put(priv->speaker_en_gpio); dev_err(dev, "snd_soc_register_card failed: %d\n", ret); - return ret; + goto err; } platform_set_drvdata(pdev, &byt_cht_es8316_card); return 0; + +err: + device_remove_software_node(priv->codec_dev); + + return ret; } static int snd_byt_cht_es8316_mc_remove(struct platform_device *pdev) @@ -617,6 +634,8 @@ static int snd_byt_cht_es8316_mc_remove(struct platform_device *pdev) struct byt_cht_es8316_private *priv = snd_soc_card_get_drvdata(card); gpiod_put(priv->speaker_en_gpio); + device_remove_software_node(priv->codec_dev); + return 0; } diff --git a/sound/soc/intel/boards/bytcr_rt5640.c b/sound/soc/intel/boards/bytcr_rt5640.c index 91a6d712eb58..b3597b0f6836 100644 --- a/sound/soc/intel/boards/bytcr_rt5640.c +++ b/sound/soc/intel/boards/bytcr_rt5640.c @@ -87,6 +87,7 @@ enum { struct byt_rt5640_private { struct snd_soc_jack jack; struct clk *mclk; + struct device *codec_dev; }; static bool is_bytcr; @@ -912,9 +913,11 @@ static const struct dmi_system_id byt_rt5640_quirk_table[] = { * Note this MUST be called before snd_soc_register_card(), so that the props * are in place before the codec component driver's probe function parses them. */ -static int byt_rt5640_add_codec_device_props(const char *i2c_dev_name) +static int byt_rt5640_add_codec_device_props(const char *i2c_dev_name, + struct byt_rt5640_private *priv) { struct property_entry props[MAX_NO_PROPS] = {}; + struct fwnode_handle *fwnode; struct device *i2c_dev; int ret, cnt = 0; @@ -960,7 +963,18 @@ static int byt_rt5640_add_codec_device_props(const char *i2c_dev_name) if (byt_rt5640_quirk & BYT_RT5640_JD_NOT_INV) props[cnt++] = PROPERTY_ENTRY_BOOL("realtek,jack-detect-not-inverted"); - ret = device_add_properties(i2c_dev, props); + fwnode = fwnode_create_software_node(props, NULL); + if (IS_ERR(fwnode)) { + /* put_device() is not handled in caller */ + put_device(i2c_dev); + return PTR_ERR(fwnode); + } + + ret = device_add_software_node(i2c_dev, to_software_node(fwnode)); + + fwnode_handle_put(fwnode); + priv->codec_dev = i2c_dev; + put_device(i2c_dev); return ret; @@ -1401,7 +1415,7 @@ static int snd_byt_rt5640_mc_probe(struct platform_device *pdev) } /* Must be called before register_card, also see declaration comment. */ - ret_val = byt_rt5640_add_codec_device_props(byt_rt5640_codec_name); + ret_val = byt_rt5640_add_codec_device_props(byt_rt5640_codec_name, priv); if (ret_val) return ret_val; @@ -1434,7 +1448,7 @@ static int snd_byt_rt5640_mc_probe(struct platform_device *pdev) * for all other errors, including -EPROBE_DEFER */ if (ret_val != -ENOENT) - return ret_val; + goto err; byt_rt5640_quirk &= ~BYT_RT5640_MCLK_EN; } } @@ -1467,7 +1481,7 @@ static int snd_byt_rt5640_mc_probe(struct platform_device *pdev) ret_val = snd_soc_fixup_dai_links_platform_name(&byt_rt5640_card, platform_name); if (ret_val) - return ret_val; + goto err; sof_parent = snd_soc_acpi_sof_parent(&pdev->dev); @@ -1489,10 +1503,25 @@ static int snd_byt_rt5640_mc_probe(struct platform_device *pdev) if (ret_val) { dev_err(&pdev->dev, "devm_snd_soc_register_card failed %d\n", ret_val); - return ret_val; + goto err; } platform_set_drvdata(pdev, &byt_rt5640_card); return ret_val; + +err: + device_remove_software_node(priv->codec_dev); + + return ret_val; +} + +static int snd_byt_rt5640_mc_remove(struct platform_device *pdev) +{ + struct snd_soc_card *card = platform_get_drvdata(pdev); + struct byt_rt5640_private *priv = snd_soc_card_get_drvdata(card); + + device_remove_software_node(priv->codec_dev); + + return 0; } static struct platform_driver snd_byt_rt5640_mc_driver = { @@ -1500,6 +1529,7 @@ static struct platform_driver snd_byt_rt5640_mc_driver = { .name = "bytcr_rt5640", }, .probe = snd_byt_rt5640_mc_probe, + .remove = snd_byt_rt5640_mc_remove }; module_platform_driver(snd_byt_rt5640_mc_driver); diff --git a/sound/soc/intel/boards/bytcr_rt5651.c b/sound/soc/intel/boards/bytcr_rt5651.c index e13c0c63a949..3066d00f3466 100644 --- a/sound/soc/intel/boards/bytcr_rt5651.c +++ b/sound/soc/intel/boards/bytcr_rt5651.c @@ -85,6 +85,7 @@ struct byt_rt5651_private { struct gpio_desc *ext_amp_gpio; struct gpio_desc *hp_detect; struct snd_soc_jack jack; + struct device *codec_dev; }; static const struct acpi_gpio_mapping *byt_rt5651_gpios; @@ -527,10 +528,13 @@ static const struct dmi_system_id byt_rt5651_quirk_table[] = { * Note this MUST be called before snd_soc_register_card(), so that the props * are in place before the codec component driver's probe function parses them. */ -static int byt_rt5651_add_codec_device_props(struct device *i2c_dev) +static int byt_rt5651_add_codec_device_props(struct device *i2c_dev, + struct byt_rt5651_private *priv) { struct property_entry props[MAX_NO_PROPS] = {}; + struct fwnode_handle *fwnode; int cnt = 0; + int ret; props[cnt++] = PROPERTY_ENTRY_U32("realtek,jack-detect-source", BYT_RT5651_JDSRC(byt_rt5651_quirk)); @@ -547,7 +551,18 @@ static int byt_rt5651_add_codec_device_props(struct device *i2c_dev) if (byt_rt5651_quirk & BYT_RT5651_JD_NOT_INV) props[cnt++] = PROPERTY_ENTRY_BOOL("realtek,jack-detect-not-inverted"); - return device_add_properties(i2c_dev, props); + fwnode = fwnode_create_software_node(props, NULL); + if (IS_ERR(fwnode)) { + /* put_device(i2c_dev) is handled in caller */ + return PTR_ERR(fwnode); + } + + ret = device_add_software_node(i2c_dev, to_software_node(fwnode)); + + fwnode_handle_put(fwnode); + priv->codec_dev = i2c_dev; + + return ret; } static int byt_rt5651_init(struct snd_soc_pcm_runtime *runtime) @@ -994,7 +1009,7 @@ static int snd_byt_rt5651_mc_probe(struct platform_device *pdev) } /* Must be called before register_card, also see declaration comment. */ - ret_val = byt_rt5651_add_codec_device_props(codec_dev); + ret_val = byt_rt5651_add_codec_device_props(codec_dev, priv); if (ret_val) { put_device(codec_dev); return ret_val; @@ -1023,7 +1038,7 @@ static int snd_byt_rt5651_mc_probe(struct platform_device *pdev) fallthrough; case -EPROBE_DEFER: put_device(codec_dev); - return ret_val; + goto err; } } priv->hp_detect = devm_fwnode_gpiod_get(&pdev->dev, @@ -1043,7 +1058,7 @@ static int snd_byt_rt5651_mc_probe(struct platform_device *pdev) fallthrough; case -EPROBE_DEFER: put_device(codec_dev); - return ret_val; + goto err; } } } @@ -1073,7 +1088,7 @@ static int snd_byt_rt5651_mc_probe(struct platform_device *pdev) * for all other errors, including -EPROBE_DEFER */ if (ret_val != -ENOENT) - return ret_val; + goto err; byt_rt5651_quirk &= ~BYT_RT5651_MCLK_EN; } } @@ -1102,7 +1117,7 @@ static int snd_byt_rt5651_mc_probe(struct platform_device *pdev) ret_val = snd_soc_fixup_dai_links_platform_name(&byt_rt5651_card, platform_name); if (ret_val) - return ret_val; + goto err; sof_parent = snd_soc_acpi_sof_parent(&pdev->dev); @@ -1124,10 +1139,25 @@ static int snd_byt_rt5651_mc_probe(struct platform_device *pdev) if (ret_val) { dev_err(&pdev->dev, "devm_snd_soc_register_card failed %d\n", ret_val); - return ret_val; + goto err; } platform_set_drvdata(pdev, &byt_rt5651_card); return ret_val; + +err: + device_remove_software_node(priv->codec_dev); + + return ret_val; +} + +static int snd_byt_rt5651_mc_remove(struct platform_device *pdev) +{ + struct snd_soc_card *card = platform_get_drvdata(pdev); + struct byt_rt5651_private *priv = snd_soc_card_get_drvdata(card); + + device_remove_software_node(priv->codec_dev); + + return 0; } static struct platform_driver snd_byt_rt5651_mc_driver = { @@ -1135,6 +1165,7 @@ static struct platform_driver snd_byt_rt5651_mc_driver = { .name = "bytcr_rt5651", }, .probe = snd_byt_rt5651_mc_probe, + .remove = snd_byt_rt5651_mc_remove, }; module_platform_driver(snd_byt_rt5651_mc_driver); -- 2.25.1 ^ permalink raw reply related [flat|nested] 8+ messages in thread
* Re: [PATCH 2/2] ASoC: Intel: boards: use software node API in Atom boards 2021-06-07 22:35 ` [PATCH 2/2] ASoC: Intel: boards: use software node API in Atom boards Pierre-Louis Bossart @ 2021-06-08 8:18 ` Hans de Goede 2021-06-08 9:02 ` Andy Shevchenko 2021-06-08 11:22 ` Pierre-Louis Bossart 0 siblings, 2 replies; 8+ messages in thread From: Hans de Goede @ 2021-06-08 8:18 UTC (permalink / raw) To: Pierre-Louis Bossart, alsa-devel Cc: tiwai, Heikki Krogerus, broonie, Andy Shevchenko Hi, On 6/8/21 12:35 AM, Pierre-Louis Bossart wrote: > From: Heikki Krogerus <heikki.krogerus@linux.intel.com> > > The function device_add_properties() is going to be removed. > Replacing it with software node API equivalents. > > Co-developed-by: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com> > Signed-off-by: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com> > Signed-off-by: Heikki Krogerus <heikki.krogerus@linux.intel.com> > --- > sound/soc/intel/boards/bytcht_es8316.c | 25 ++++++++++++-- > sound/soc/intel/boards/bytcr_rt5640.c | 42 +++++++++++++++++++---- > sound/soc/intel/boards/bytcr_rt5651.c | 47 +++++++++++++++++++++----- > 3 files changed, 97 insertions(+), 17 deletions(-) > > diff --git a/sound/soc/intel/boards/bytcht_es8316.c b/sound/soc/intel/boards/bytcht_es8316.c > index a0af91580184..ef8ed3ff53af 100644 > --- a/sound/soc/intel/boards/bytcht_es8316.c > +++ b/sound/soc/intel/boards/bytcht_es8316.c > @@ -38,6 +38,7 @@ struct byt_cht_es8316_private { > struct snd_soc_jack jack; > struct gpio_desc *speaker_en_gpio; > bool speaker_en; > + struct device *codec_dev; > }; > > enum { > @@ -461,6 +462,7 @@ static int snd_byt_cht_es8316_mc_probe(struct platform_device *pdev) > const struct dmi_system_id *dmi_id; > struct device *dev = &pdev->dev; > struct snd_soc_acpi_mach *mach; > + struct fwnode_handle *fwnode; > const char *platform_name; > struct acpi_device *adev; > struct device *codec_dev; > @@ -543,7 +545,16 @@ static int snd_byt_cht_es8316_mc_probe(struct platform_device *pdev) > props[cnt++] = PROPERTY_ENTRY_BOOL("everest,jack-detect-inverted"); > > if (cnt) { > - ret = device_add_properties(codec_dev, props); > + fwnode = fwnode_create_software_node(props, NULL); > + if (IS_ERR(fwnode)) { > + put_device(codec_dev); > + return PTR_ERR(fwnode); > + } > + > + ret = device_add_software_node(codec_dev, to_software_node(fwnode)); > + > + fwnode_handle_put(fwnode); > + > if (ret) { > put_device(codec_dev); > return ret; > @@ -556,6 +567,7 @@ static int snd_byt_cht_es8316_mc_probe(struct platform_device *pdev) > /* see comment in byt_cht_es8316_resume */ > GPIOD_OUT_LOW | GPIOD_FLAGS_BIT_NONEXCLUSIVE); > put_device(codec_dev); > + priv->codec_dev = codec_dev; > > if (IS_ERR(priv->speaker_en_gpio)) { > ret = PTR_ERR(priv->speaker_en_gpio); > @@ -567,7 +579,7 @@ static int snd_byt_cht_es8316_mc_probe(struct platform_device *pdev) > dev_err(dev, "get speaker GPIO failed: %d\n", ret); > fallthrough; > case -EPROBE_DEFER: > - return ret; > + goto err; > } > } > > @@ -605,10 +617,15 @@ static int snd_byt_cht_es8316_mc_probe(struct platform_device *pdev) > if (ret) { > gpiod_put(priv->speaker_en_gpio); > dev_err(dev, "snd_soc_register_card failed: %d\n", ret); > - return ret; > + goto err; > } > platform_set_drvdata(pdev, &byt_cht_es8316_card); > return 0; > + > +err: > + device_remove_software_node(priv->codec_dev); > + > + return ret; > } > > static int snd_byt_cht_es8316_mc_remove(struct platform_device *pdev) > @@ -617,6 +634,8 @@ static int snd_byt_cht_es8316_mc_remove(struct platform_device *pdev) > struct byt_cht_es8316_private *priv = snd_soc_card_get_drvdata(card); > > gpiod_put(priv->speaker_en_gpio); > + device_remove_software_node(priv->codec_dev); This is a problem, nothing guarantees codec_dev not going away before snd_byt_cht_es8316_mc_remove() runs. Although the only thing which I can come up with where this happens is unbinding the i2c-controller driver I still would like us to take this scenario into account. I think it would be better to use device_create_managed_software_node() to tie the lifetime of the swnode to the lifetime of the device, this also removes the need for all the goto err cases (and introducing a remove function in the bytcr_rt5640.c case). This does mean that we could end up calling device_create_managed_software_node() on the same device twice, when the machine driver gets unbound + rebound, but that is an already existing issue with our current device_add_properties() usage. We could fix this (in a separate commit since it is an already existing issue) by adding a PROPERTY_ENTRY_BOOL("cht_es8316,swnode-created") property to the properties and checking for that being set with device_property_read_bool(codec, "cht_es8316,swnode-created") Or we could move the device_put(codec_dev) to snd_byt_cht_es8316_mc_remove(). I've a slight preference for using device_create_managed_software_node() + some mechanism to avoid a double adding of the properties, since I would like to try and avoid the "goto err" additions. Ideally device_create_managed_software_node() would detect the double-add itself and it would return -EEXISTS. Heikki, would that be feasible ? Regards, Hans > + > return 0; > } > > diff --git a/sound/soc/intel/boards/bytcr_rt5640.c b/sound/soc/intel/boards/bytcr_rt5640.c > index 91a6d712eb58..b3597b0f6836 100644 > --- a/sound/soc/intel/boards/bytcr_rt5640.c > +++ b/sound/soc/intel/boards/bytcr_rt5640.c > @@ -87,6 +87,7 @@ enum { > struct byt_rt5640_private { > struct snd_soc_jack jack; > struct clk *mclk; > + struct device *codec_dev; > }; > static bool is_bytcr; > > @@ -912,9 +913,11 @@ static const struct dmi_system_id byt_rt5640_quirk_table[] = { > * Note this MUST be called before snd_soc_register_card(), so that the props > * are in place before the codec component driver's probe function parses them. > */ > -static int byt_rt5640_add_codec_device_props(const char *i2c_dev_name) > +static int byt_rt5640_add_codec_device_props(const char *i2c_dev_name, > + struct byt_rt5640_private *priv) > { > struct property_entry props[MAX_NO_PROPS] = {}; > + struct fwnode_handle *fwnode; > struct device *i2c_dev; > int ret, cnt = 0; > > @@ -960,7 +963,18 @@ static int byt_rt5640_add_codec_device_props(const char *i2c_dev_name) > if (byt_rt5640_quirk & BYT_RT5640_JD_NOT_INV) > props[cnt++] = PROPERTY_ENTRY_BOOL("realtek,jack-detect-not-inverted"); > > - ret = device_add_properties(i2c_dev, props); > + fwnode = fwnode_create_software_node(props, NULL); > + if (IS_ERR(fwnode)) { > + /* put_device() is not handled in caller */ > + put_device(i2c_dev); > + return PTR_ERR(fwnode); > + } > + > + ret = device_add_software_node(i2c_dev, to_software_node(fwnode)); > + > + fwnode_handle_put(fwnode); > + priv->codec_dev = i2c_dev; > + > put_device(i2c_dev); > > return ret; > @@ -1401,7 +1415,7 @@ static int snd_byt_rt5640_mc_probe(struct platform_device *pdev) > } > > /* Must be called before register_card, also see declaration comment. */ > - ret_val = byt_rt5640_add_codec_device_props(byt_rt5640_codec_name); > + ret_val = byt_rt5640_add_codec_device_props(byt_rt5640_codec_name, priv); > if (ret_val) > return ret_val; > > @@ -1434,7 +1448,7 @@ static int snd_byt_rt5640_mc_probe(struct platform_device *pdev) > * for all other errors, including -EPROBE_DEFER > */ > if (ret_val != -ENOENT) > - return ret_val; > + goto err; > byt_rt5640_quirk &= ~BYT_RT5640_MCLK_EN; > } > } > @@ -1467,7 +1481,7 @@ static int snd_byt_rt5640_mc_probe(struct platform_device *pdev) > ret_val = snd_soc_fixup_dai_links_platform_name(&byt_rt5640_card, > platform_name); > if (ret_val) > - return ret_val; > + goto err; > > sof_parent = snd_soc_acpi_sof_parent(&pdev->dev); > > @@ -1489,10 +1503,25 @@ static int snd_byt_rt5640_mc_probe(struct platform_device *pdev) > if (ret_val) { > dev_err(&pdev->dev, "devm_snd_soc_register_card failed %d\n", > ret_val); > - return ret_val; > + goto err; > } > platform_set_drvdata(pdev, &byt_rt5640_card); > return ret_val; > + > +err: > + device_remove_software_node(priv->codec_dev); > + > + return ret_val; > +} > + > +static int snd_byt_rt5640_mc_remove(struct platform_device *pdev) > +{ > + struct snd_soc_card *card = platform_get_drvdata(pdev); > + struct byt_rt5640_private *priv = snd_soc_card_get_drvdata(card); > + > + device_remove_software_node(priv->codec_dev); > + > + return 0; > } > > static struct platform_driver snd_byt_rt5640_mc_driver = { > @@ -1500,6 +1529,7 @@ static struct platform_driver snd_byt_rt5640_mc_driver = { > .name = "bytcr_rt5640", > }, > .probe = snd_byt_rt5640_mc_probe, > + .remove = snd_byt_rt5640_mc_remove > }; > > module_platform_driver(snd_byt_rt5640_mc_driver); > diff --git a/sound/soc/intel/boards/bytcr_rt5651.c b/sound/soc/intel/boards/bytcr_rt5651.c > index e13c0c63a949..3066d00f3466 100644 > --- a/sound/soc/intel/boards/bytcr_rt5651.c > +++ b/sound/soc/intel/boards/bytcr_rt5651.c > @@ -85,6 +85,7 @@ struct byt_rt5651_private { > struct gpio_desc *ext_amp_gpio; > struct gpio_desc *hp_detect; > struct snd_soc_jack jack; > + struct device *codec_dev; > }; > > static const struct acpi_gpio_mapping *byt_rt5651_gpios; > @@ -527,10 +528,13 @@ static const struct dmi_system_id byt_rt5651_quirk_table[] = { > * Note this MUST be called before snd_soc_register_card(), so that the props > * are in place before the codec component driver's probe function parses them. > */ > -static int byt_rt5651_add_codec_device_props(struct device *i2c_dev) > +static int byt_rt5651_add_codec_device_props(struct device *i2c_dev, > + struct byt_rt5651_private *priv) > { > struct property_entry props[MAX_NO_PROPS] = {}; > + struct fwnode_handle *fwnode; > int cnt = 0; > + int ret; > > props[cnt++] = PROPERTY_ENTRY_U32("realtek,jack-detect-source", > BYT_RT5651_JDSRC(byt_rt5651_quirk)); > @@ -547,7 +551,18 @@ static int byt_rt5651_add_codec_device_props(struct device *i2c_dev) > if (byt_rt5651_quirk & BYT_RT5651_JD_NOT_INV) > props[cnt++] = PROPERTY_ENTRY_BOOL("realtek,jack-detect-not-inverted"); > > - return device_add_properties(i2c_dev, props); > + fwnode = fwnode_create_software_node(props, NULL); > + if (IS_ERR(fwnode)) { > + /* put_device(i2c_dev) is handled in caller */ > + return PTR_ERR(fwnode); > + } > + > + ret = device_add_software_node(i2c_dev, to_software_node(fwnode)); > + > + fwnode_handle_put(fwnode); > + priv->codec_dev = i2c_dev; > + > + return ret; > } > > static int byt_rt5651_init(struct snd_soc_pcm_runtime *runtime) > @@ -994,7 +1009,7 @@ static int snd_byt_rt5651_mc_probe(struct platform_device *pdev) > } > > /* Must be called before register_card, also see declaration comment. */ > - ret_val = byt_rt5651_add_codec_device_props(codec_dev); > + ret_val = byt_rt5651_add_codec_device_props(codec_dev, priv); > if (ret_val) { > put_device(codec_dev); > return ret_val; > @@ -1023,7 +1038,7 @@ static int snd_byt_rt5651_mc_probe(struct platform_device *pdev) > fallthrough; > case -EPROBE_DEFER: > put_device(codec_dev); > - return ret_val; > + goto err; > } > } > priv->hp_detect = devm_fwnode_gpiod_get(&pdev->dev, > @@ -1043,7 +1058,7 @@ static int snd_byt_rt5651_mc_probe(struct platform_device *pdev) > fallthrough; > case -EPROBE_DEFER: > put_device(codec_dev); > - return ret_val; > + goto err; > } > } > } > @@ -1073,7 +1088,7 @@ static int snd_byt_rt5651_mc_probe(struct platform_device *pdev) > * for all other errors, including -EPROBE_DEFER > */ > if (ret_val != -ENOENT) > - return ret_val; > + goto err; > byt_rt5651_quirk &= ~BYT_RT5651_MCLK_EN; > } > } > @@ -1102,7 +1117,7 @@ static int snd_byt_rt5651_mc_probe(struct platform_device *pdev) > ret_val = snd_soc_fixup_dai_links_platform_name(&byt_rt5651_card, > platform_name); > if (ret_val) > - return ret_val; > + goto err; > > sof_parent = snd_soc_acpi_sof_parent(&pdev->dev); > > @@ -1124,10 +1139,25 @@ static int snd_byt_rt5651_mc_probe(struct platform_device *pdev) > if (ret_val) { > dev_err(&pdev->dev, "devm_snd_soc_register_card failed %d\n", > ret_val); > - return ret_val; > + goto err; > } > platform_set_drvdata(pdev, &byt_rt5651_card); > return ret_val; > + > +err: > + device_remove_software_node(priv->codec_dev); > + > + return ret_val; > +} > + > +static int snd_byt_rt5651_mc_remove(struct platform_device *pdev) > +{ > + struct snd_soc_card *card = platform_get_drvdata(pdev); > + struct byt_rt5651_private *priv = snd_soc_card_get_drvdata(card); > + > + device_remove_software_node(priv->codec_dev); > + > + return 0; > } > > static struct platform_driver snd_byt_rt5651_mc_driver = { > @@ -1135,6 +1165,7 @@ static struct platform_driver snd_byt_rt5651_mc_driver = { > .name = "bytcr_rt5651", > }, > .probe = snd_byt_rt5651_mc_probe, > + .remove = snd_byt_rt5651_mc_remove, > }; > > module_platform_driver(snd_byt_rt5651_mc_driver); > ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH 2/2] ASoC: Intel: boards: use software node API in Atom boards 2021-06-08 8:18 ` Hans de Goede @ 2021-06-08 9:02 ` Andy Shevchenko 2021-06-08 9:19 ` Hans de Goede 2021-06-08 11:22 ` Pierre-Louis Bossart 1 sibling, 1 reply; 8+ messages in thread From: Andy Shevchenko @ 2021-06-08 9:02 UTC (permalink / raw) To: Hans de Goede Cc: tiwai, alsa-devel, broonie, Pierre-Louis Bossart, Heikki Krogerus On Tue, Jun 08, 2021 at 10:18:08AM +0200, Hans de Goede wrote: > On 6/8/21 12:35 AM, Pierre-Louis Bossart wrote: > > From: Heikki Krogerus <heikki.krogerus@linux.intel.com> > > > > The function device_add_properties() is going to be removed. > > Replacing it with software node API equivalents. ... > > + device_remove_software_node(priv->codec_dev); > > This is a problem, nothing guarantees codec_dev not going away before > snd_byt_cht_es8316_mc_remove() runs. Although the only thing which I can come up > with where this happens is unbinding the i2c-controller driver I still would like > us to take this scenario into account. > > I think it would be better to use device_create_managed_software_node() to tie > the lifetime of the swnode to the lifetime of the device, this also removes > the need for all the goto err cases (and introducing a remove function in > the bytcr_rt5640.c case). Which device? If you are telling about codec, the unload-load of the machine driver won't be successful or will produce a leak / warning / etc. If you are telling about machine related device, it simply doesn't belong to it. Am I got all this right? > This does mean that we could end up calling device_create_managed_software_node() > on the same device twice, when the machine driver gets unbound + rebound, but > that is an already existing issue with our current device_add_properties() usage. Yep. > We could fix this (in a separate commit since it is an already existing issue) > by adding a PROPERTY_ENTRY_BOOL("cht_es8316,swnode-created") property to the > properties and checking for that being set with > device_property_read_bool(codec, "cht_es8316,swnode-created") Not sure it's a good idea, this sounds like a hack. > Or we could move the device_put(codec_dev) to snd_byt_cht_es8316_mc_remove(). This sounds better. > I've a slight preference for using device_create_managed_software_node() + > some mechanism to avoid a double adding of the properties, since I would like > to try and avoid the "goto err" additions. > > Ideally device_create_managed_software_node() would detect the double-add > itself and it would return -EEXISTS. Heikki, would that be feasible ? If I got the big picture correct, the SOF needs to switch to use fwnode graphs. -- With Best Regards, Andy Shevchenko ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH 2/2] ASoC: Intel: boards: use software node API in Atom boards 2021-06-08 9:02 ` Andy Shevchenko @ 2021-06-08 9:19 ` Hans de Goede 0 siblings, 0 replies; 8+ messages in thread From: Hans de Goede @ 2021-06-08 9:19 UTC (permalink / raw) To: Andy Shevchenko Cc: tiwai, alsa-devel, broonie, Pierre-Louis Bossart, Heikki Krogerus Hi, On 6/8/21 11:02 AM, Andy Shevchenko wrote: > On Tue, Jun 08, 2021 at 10:18:08AM +0200, Hans de Goede wrote: >> On 6/8/21 12:35 AM, Pierre-Louis Bossart wrote: >>> From: Heikki Krogerus <heikki.krogerus@linux.intel.com> >>> >>> The function device_add_properties() is going to be removed. >>> Replacing it with software node API equivalents. > > ... > >>> + device_remove_software_node(priv->codec_dev); >> >> This is a problem, nothing guarantees codec_dev not going away before >> snd_byt_cht_es8316_mc_remove() runs. Although the only thing which I can come up >> with where this happens is unbinding the i2c-controller driver I still would like >> us to take this scenario into account. >> >> I think it would be better to use device_create_managed_software_node() to tie >> the lifetime of the swnode to the lifetime of the device, this also removes >> the need for all the goto err cases (and introducing a remove function in >> the bytcr_rt5640.c case). > > Which device? If you are telling about codec, the unload-load of the machine > driver won't be successful or will produce a leak / warning / etc. Yes I'm talking about the codec, and yes if the codec device goes away before the machine-driver is unbound things will likely already break. But the machine driver does not hold any explicit reference on the codec-device, so this might happen (I guess there might be a reference somewhere inside the ASoC code once devm_snd_soc_register_card() has returned successfully). > If you are telling about machine related device, it simply doesn't belong to it. > > Am I got all this right? > >> This does mean that we could end up calling device_create_managed_software_node() >> on the same device twice, when the machine driver gets unbound + rebound, but >> that is an already existing issue with our current device_add_properties() usage. > > Yep. > >> We could fix this (in a separate commit since it is an already existing issue) >> by adding a PROPERTY_ENTRY_BOOL("cht_es8316,swnode-created") property to the >> properties and checking for that being set with >> device_property_read_bool(codec, "cht_es8316,swnode-created") > > Not sure it's a good idea, this sounds like a hack. Right, which is why I also suggested that device_create_managed_software_node() could be modified to fail when called a second time on the same device, this is a check which probably would be good to add regardless. More specifically I guess that set_secondary_fwnode() could be made to return an error when replacing an existing secondary fwnode with a non NULL value, rather then just replacing it. >> Or we could move the device_put(codec_dev) to snd_byt_cht_es8316_mc_remove(). > > This sounds better. As I already mentioned I'm not a fan of all the goto err-s these patches introduce, this won't fix that. Regards, Hans ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH 2/2] ASoC: Intel: boards: use software node API in Atom boards 2021-06-08 8:18 ` Hans de Goede 2021-06-08 9:02 ` Andy Shevchenko @ 2021-06-08 11:22 ` Pierre-Louis Bossart 2021-06-08 12:47 ` Hans de Goede 1 sibling, 1 reply; 8+ messages in thread From: Pierre-Louis Bossart @ 2021-06-08 11:22 UTC (permalink / raw) To: Hans de Goede, alsa-devel Cc: tiwai, Heikki Krogerus, broonie, Andy Shevchenko >> static int snd_byt_cht_es8316_mc_remove(struct platform_device *pdev) >> @@ -617,6 +634,8 @@ static int snd_byt_cht_es8316_mc_remove(struct platform_device *pdev) >> struct byt_cht_es8316_private *priv = snd_soc_card_get_drvdata(card); >> >> gpiod_put(priv->speaker_en_gpio); >> + device_remove_software_node(priv->codec_dev); > > This is a problem, nothing guarantees codec_dev not going away before > snd_byt_cht_es8316_mc_remove() runs. Although the only thing which I can come up > with where this happens is unbinding the i2c-controller driver I still would like > us to take this scenario into account. Is this possible really? the codec driver will register a component that's used by the ASoC card, I was assuming that there was some sort of reference count already to prevent this unbinding from happening. > I think it would be better to use device_create_managed_software_node() to tie > the lifetime of the swnode to the lifetime of the device, this also removes > the need for all the goto err cases (and introducing a remove function in > the bytcr_rt5640.c case). > > This does mean that we could end up calling device_create_managed_software_node() > on the same device twice, when the machine driver gets unbound + rebound, but > that is an already existing issue with our current device_add_properties() usage. > > We could fix this (in a separate commit since it is an already existing issue) > by adding a PROPERTY_ENTRY_BOOL("cht_es8316,swnode-created") property to the > properties and checking for that being set with > device_property_read_bool(codec, "cht_es8316,swnode-created") > > Or we could move the device_put(codec_dev) to snd_byt_cht_es8316_mc_remove(). that sounds like a better plan if you want to manage explicit dependencies - regardless of how which API is used to manage properties. ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH 2/2] ASoC: Intel: boards: use software node API in Atom boards 2021-06-08 11:22 ` Pierre-Louis Bossart @ 2021-06-08 12:47 ` Hans de Goede 0 siblings, 0 replies; 8+ messages in thread From: Hans de Goede @ 2021-06-08 12:47 UTC (permalink / raw) To: Pierre-Louis Bossart, alsa-devel Cc: tiwai, Heikki Krogerus, broonie, Andy Shevchenko Hi, On 6/8/21 1:22 PM, Pierre-Louis Bossart wrote: > >>> static int snd_byt_cht_es8316_mc_remove(struct platform_device *pdev) >>> @@ -617,6 +634,8 @@ static int snd_byt_cht_es8316_mc_remove(struct platform_device *pdev) >>> struct byt_cht_es8316_private *priv = snd_soc_card_get_drvdata(card); >>> gpiod_put(priv->speaker_en_gpio); >>> + device_remove_software_node(priv->codec_dev); >> >> This is a problem, nothing guarantees codec_dev not going away before >> snd_byt_cht_es8316_mc_remove() runs. Although the only thing which I can come up >> with where this happens is unbinding the i2c-controller driver I still would like >> us to take this scenario into account. > > Is this possible really? the codec driver will register a component that's used by the ASoC card, I was assuming that there was some sort of reference count already to prevent this unbinding from happening. There might very well be some reference count elsewhere, but IMHO if the machine-driver is going to keep a pointer to the device around it should keep its own reference. >> I think it would be better to use device_create_managed_software_node() to tie >> the lifetime of the swnode to the lifetime of the device, this also removes >> the need for all the goto err cases (and introducing a remove function in >> the bytcr_rt5640.c case). >> >> This does mean that we could end up calling device_create_managed_software_node() >> on the same device twice, when the machine driver gets unbound + rebound, but >> that is an already existing issue with our current device_add_properties() usage. >> >> We could fix this (in a separate commit since it is an already existing issue) >> by adding a PROPERTY_ENTRY_BOOL("cht_es8316,swnode-created") property to the >> properties and checking for that being set with >> device_property_read_bool(codec, "cht_es8316,swnode-created") >> >> Or we could move the device_put(codec_dev) to snd_byt_cht_es8316_mc_remove(). > > that sounds like a better plan if you want to manage explicit dependencies - regardless of how which API is used to manage properties. Ok, so lets go with that then. Regards, Hans ^ permalink raw reply [flat|nested] 8+ messages in thread
end of thread, other threads:[~2021-06-08 12:48 UTC | newest] Thread overview: 8+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2021-06-07 22:35 [PATCH 0/2] ASoC: Intel: boards: use software node API Pierre-Louis Bossart 2021-06-07 22:35 ` [PATCH 1/2] ASoC: Intel: boards: use software node API in SoundWire machines Pierre-Louis Bossart 2021-06-07 22:35 ` [PATCH 2/2] ASoC: Intel: boards: use software node API in Atom boards Pierre-Louis Bossart 2021-06-08 8:18 ` Hans de Goede 2021-06-08 9:02 ` Andy Shevchenko 2021-06-08 9:19 ` Hans de Goede 2021-06-08 11:22 ` Pierre-Louis Bossart 2021-06-08 12:47 ` Hans de Goede
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).