| Message ID | 20211228190931.df5d518220080a734532ebfd@uvos.xyz |
|---|---|
| State | New |
| Headers | show |
| Series | [RFC,1/3] ASoC: simple-card-utils: add support for componants provideing jack events via set_jack | expand |
Hi Carl Thank you for your patch. > This allows componants that want a jack to report state on to do so by calling > set_jack on components implementing this function. > > Im not entirely sure this is the right way to do this so RFC (snip) > + for_each_rtd_components(rtd, i, component) { > + if (component->driver->set_jack) { > + if (!priv->hp_jack) { > + priv->hp_jack = devm_kzalloc(priv->snd_card.dev, > + sizeof(*priv->hp_jack), GFP_KERNEL); > + snd_soc_card_jack_new(&priv->snd_card, > + "Headphones", > + SND_JACK_HEADPHONE, > + &priv->hp_jack->jack, > + NULL, 0); > + } > + snd_soc_component_set_jack(component, &priv->hp_jack->jack, NULL); > + } > + } I'm sorry but I don't understand what you want to do by this patch. Is main code of this patch asoc_simple_dai_init() update (= call set_jack() for all component) ? > int asoc_simple_init_jack(struct snd_soc_card *card, > - struct asoc_simple_jack *sjack, > + struct asoc_simple_jack **sjack, > int is_hp, char *prefix, char *pin); ${LINUX}/sound/soc/fsl/fsl-asoc-card.c is using this function, too. We will have compile error without update it. > int asoc_simple_init_jack(struct snd_soc_card *card, > - struct asoc_simple_jack *sjack, > + struct asoc_simple_jack **sjack, > int is_hp, char *prefix, > char *pin) (snip) > if (gpio_is_valid(det)) { > - sjack->pin.pin = pin_name; > - sjack->pin.mask = mask; > + struct asoc_simple_jack *sjack_d; > + > + sjack = devm_kzalloc(dev, sizeof(*(*sjack)), GFP_KERNEL); > + sjack_d = *sjack; Am I misunderstanding ? I think you need to do here is this ? - sjack = devm_kzalloc(dev, sizeof(*(*sjack)), GFP_KERNEL); + *sjack = devm_kzalloc(dev, sizeof(*(*sjack)), GFP_KERNEL); Thank you for your help !! Best regards --- Kuninori Morimoto
Hi, > I'm sorry but I don't understand what you want to do by this patch. > Is main code of this patch asoc_simple_dai_init() update > (= call set_jack() for all component) ? Yes, so asoc-audio-graph-card currently only supports headphone jack plug detection on devices that provide a simple gpio to sense plug state. The intent of this patch is to allow the componant driver to implement jack detection in cases where something else has to be done to sense state, sutch as comunicating with device firmware or using a shared irq. See the other patches in this series for an example. This is performed by sharing the jack with the component via set_jack(). > ${LINUX}/sound/soc/fsl/fsl-asoc-card.c is using this function, too. > We will have compile error without update it. indeed, will do. > > + sjack = devm_kzalloc(dev, sizeof(*(*sjack)), GFP_KERNEL); > > + sjack_d = *sjack; > > Am I misunderstanding ? > I think you need to do here is this ? > > - sjack = devm_kzalloc(dev, sizeof(*(*sjack)), GFP_KERNEL); > + *sjack = devm_kzalloc(dev, sizeof(*(*sjack)), GFP_KERNEL); Ah yes thank you, another problem is i lack hardware to test this (the gpio) path.
Hi Carl > Yes, so asoc-audio-graph-card currently only supports headphone jack plug > detection on devices that provide a simple gpio to sense plug state. The > intent of this patch is to allow the componant driver to implement jack > detection in cases where something else has to be done to sense state, > sutch as comunicating with device firmware or using a shared irq. See > the other patches in this series for an example. This is performed by > sharing the jack with the component via set_jack(). OK, I see. It seems other drivers are using dai_link->init for detecting and set_jack(). I guess we can call set_jack() at asoc_simple_init_jack() and call asoc_simple_init_hp() at dai_link->init ? It is not tested, but I added sample code below. I hope my understanding was correct and it solves your issue. > sutch as comunicating with device firmware or using a shared irq. See > the other patches in this series for an example. This is performed by I now noticed that I had mailing list issue... Thank you for your help !! ----------- From: Kuninori Morimoto <kuninori.morimoto.gx@renesas.com> Subject: [PATCH] ASoC: simple_card_utils: call snd_soc_component_set_jack() at asoc_simple_init_jack() Current simple-card / audio-graph-card are detecting HP/MIC at card->probe timing (= A), and not calling snd_soc_component_set_jack() for it. Other sound card drivers are using dai_link->init timing (= B) for both detecting and set_jack(). static int snd_soc_bind_card(...) { .... (A) ret = snd_soc_card_probe(card); ... for_each_card_rtds(card, rtd) { (B) ret = soc_init_pcm_runtime(card, rtd); ... } ... } This patch do (a) call set_jack() (= Y) at asoc_simple_init_jack() (= X) which is used to detect HP/MIC. (b) call it from dai_link->init timing (= B) instead of card->probe timing (= A). (c) remove card->init (= A) timing function from simple-card / audio-graph-card. (X) int asoc_simple_init_jack(...) { ... if (gpio_is_valid(det)) { ... snd_soc_card_jack_new(...); snd_soc_jack_add_gpios(...); for_each_card_components(card, component) (Y) snd_soc_component_set_jack(component, ...); } ... } One note here is that simple-card needs PREFIX to detecting HP/MIC, but it is not needed on audio-graph-card. Thus simple-card uses local function for it, and audio-graph-card is using global function and sharing the code with audio-graph-card / audio-graph-card2 / audio-graph-card2-custom-sample / tegra_audio_graph_card. Reported-by: Carl Philipp Klemm <philipp@uvos.xyz> Signed-off-by: Kuninori Morimoto <kuninori.morimoto.gx@renesas.com> --- include/sound/simple_card_utils.h | 2 +- sound/soc/generic/audio-graph-card.c | 3 +- .../generic/audio-graph-card2-custom-sample.c | 3 +- sound/soc/generic/audio-graph-card2.c | 3 +- sound/soc/generic/simple-card-utils.c | 14 ++++++- sound/soc/generic/simple-card.c | 40 ++++++++++--------- sound/soc/tegra/tegra_audio_graph_card.c | 2 +- 7 files changed, 39 insertions(+), 28 deletions(-) diff --git a/include/sound/simple_card_utils.h b/include/sound/simple_card_utils.h index df430f1c2a10..34891da5a0fa 100644 --- a/include/sound/simple_card_utils.h +++ b/include/sound/simple_card_utils.h @@ -182,7 +182,7 @@ int asoc_simple_init_priv(struct asoc_simple_priv *priv, struct link_info *li); int asoc_simple_remove(struct platform_device *pdev); -int asoc_graph_card_probe(struct snd_soc_card *card); +int asoc_graph_dai_init(struct snd_soc_pcm_runtime *rtd); int asoc_graph_is_ports0(struct device_node *port); #ifdef DEBUG diff --git a/sound/soc/generic/audio-graph-card.c b/sound/soc/generic/audio-graph-card.c index 2b598af8feef..456db1f894af 100644 --- a/sound/soc/generic/audio-graph-card.c +++ b/sound/soc/generic/audio-graph-card.c @@ -259,7 +259,7 @@ static int graph_link_init(struct asoc_simple_priv *priv, if (ret < 0) return ret; - dai_link->init = asoc_simple_dai_init; + dai_link->init = asoc_graph_dai_init; dai_link->ops = &graph_ops; if (priv->ops) dai_link->ops = priv->ops; @@ -716,7 +716,6 @@ static int graph_probe(struct platform_device *pdev) card = simple_priv_to_card(priv); card->dapm_widgets = graph_dapm_widgets; card->num_dapm_widgets = ARRAY_SIZE(graph_dapm_widgets); - card->probe = asoc_graph_card_probe; if (of_device_get_match_data(dev)) priv->dpcm_selectable = 1; diff --git a/sound/soc/generic/audio-graph-card2-custom-sample.c b/sound/soc/generic/audio-graph-card2-custom-sample.c index 4a2c743e286c..da6cb69faa8d 100644 --- a/sound/soc/generic/audio-graph-card2-custom-sample.c +++ b/sound/soc/generic/audio-graph-card2-custom-sample.c @@ -34,8 +34,7 @@ static int custom_card_probe(struct snd_soc_card *card) custom_priv->custom_params = 1; - /* you can use generic probe function */ - return asoc_graph_card_probe(card); + return 0; } static int custom_hook_pre(struct asoc_simple_priv *priv) diff --git a/sound/soc/generic/audio-graph-card2.c b/sound/soc/generic/audio-graph-card2.c index c3947347dda3..75a1aeb75d2c 100644 --- a/sound/soc/generic/audio-graph-card2.c +++ b/sound/soc/generic/audio-graph-card2.c @@ -710,7 +710,7 @@ static void graph_link_init(struct asoc_simple_priv *priv, daiclk = snd_soc_daifmt_clock_provider_fliped(daiclk); dai_link->dai_fmt = daifmt | daiclk; - dai_link->init = asoc_simple_dai_init; + dai_link->init = asoc_graph_dai_init; dai_link->ops = &graph_ops; if (priv->ops) dai_link->ops = priv->ops; @@ -1180,7 +1180,6 @@ int audio_graph2_parse_of(struct asoc_simple_priv *priv, struct device *dev, if (!li) return -ENOMEM; - card->probe = asoc_graph_card_probe; card->owner = THIS_MODULE; card->dev = dev; diff --git a/sound/soc/generic/simple-card-utils.c b/sound/soc/generic/simple-card-utils.c index 850e968677f1..398fc4cb1d07 100644 --- a/sound/soc/generic/simple-card-utils.c +++ b/sound/soc/generic/simple-card-utils.c @@ -588,6 +588,8 @@ int asoc_simple_init_jack(struct snd_soc_card *card, return -EPROBE_DEFER; if (gpio_is_valid(det)) { + struct snd_soc_component *component; + sjack->pin.pin = pin_name; sjack->pin.mask = mask; @@ -603,6 +605,9 @@ int asoc_simple_init_jack(struct snd_soc_card *card, snd_soc_jack_add_gpios(&sjack->jack, 1, &sjack->gpio); + + for_each_card_components(card, component) + snd_soc_component_set_jack(component, &sjack->jack, NULL); } return 0; @@ -758,11 +763,16 @@ int asoc_simple_remove(struct platform_device *pdev) } EXPORT_SYMBOL_GPL(asoc_simple_remove); -int asoc_graph_card_probe(struct snd_soc_card *card) +int asoc_graph_dai_init(struct snd_soc_pcm_runtime *rtd) { + struct snd_soc_card *card = rtd->card; struct asoc_simple_priv *priv = snd_soc_card_get_drvdata(card); int ret; + ret = asoc_simple_dai_init(rtd); + if (ret < 0) + return ret; + ret = asoc_simple_init_hp(card, &priv->hp_jack, NULL); if (ret < 0) return ret; @@ -773,7 +783,7 @@ int asoc_graph_card_probe(struct snd_soc_card *card) return 0; } -EXPORT_SYMBOL_GPL(asoc_graph_card_probe); +EXPORT_SYMBOL_GPL(asoc_graph_dai_init); int asoc_graph_is_ports0(struct device_node *np) { diff --git a/sound/soc/generic/simple-card.c b/sound/soc/generic/simple-card.c index a89d1cfdda32..e76bfae6ced4 100644 --- a/sound/soc/generic/simple-card.c +++ b/sound/soc/generic/simple-card.c @@ -149,6 +149,27 @@ static int simple_parse_node(struct asoc_simple_priv *priv, return 0; } +static int simple_dai_init(struct snd_soc_pcm_runtime *rtd) +{ + struct snd_soc_card *card = rtd->card; + struct asoc_simple_priv *priv = snd_soc_card_get_drvdata(card); + int ret; + + ret = asoc_simple_dai_init(rtd); + if (ret < 0) + return ret; + + ret = asoc_simple_init_hp(card, &priv->hp_jack, PREFIX); + if (ret < 0) + return ret; + + ret = asoc_simple_init_mic(card, &priv->mic_jack, PREFIX); + if (ret < 0) + return ret; + + return 0; +} + static int simple_link_init(struct asoc_simple_priv *priv, struct device_node *node, struct device_node *codec, @@ -164,7 +185,7 @@ static int simple_link_init(struct asoc_simple_priv *priv, if (ret < 0) return 0; - dai_link->init = asoc_simple_dai_init; + dai_link->init = simple_dai_init; dai_link->ops = &simple_ops; return asoc_simple_set_dailink_name(dev, dai_link, name); @@ -587,22 +608,6 @@ static int simple_get_dais_count(struct asoc_simple_priv *priv, simple_count_dpcm); } -static int simple_soc_probe(struct snd_soc_card *card) -{ - struct asoc_simple_priv *priv = snd_soc_card_get_drvdata(card); - int ret; - - ret = asoc_simple_init_hp(card, &priv->hp_jack, PREFIX); - if (ret < 0) - return ret; - - ret = asoc_simple_init_mic(card, &priv->mic_jack, PREFIX); - if (ret < 0) - return ret; - - return 0; -} - static int asoc_simple_probe(struct platform_device *pdev) { struct asoc_simple_priv *priv; @@ -620,7 +625,6 @@ static int asoc_simple_probe(struct platform_device *pdev) card = simple_priv_to_card(priv); card->owner = THIS_MODULE; card->dev = dev; - card->probe = simple_soc_probe; card->driver_name = "simple-card"; li = devm_kzalloc(dev, sizeof(*li), GFP_KERNEL); diff --git a/sound/soc/tegra/tegra_audio_graph_card.c b/sound/soc/tegra/tegra_audio_graph_card.c index 1f2c5018bf5a..404762d40389 100644 --- a/sound/soc/tegra/tegra_audio_graph_card.c +++ b/sound/soc/tegra/tegra_audio_graph_card.c @@ -184,7 +184,7 @@ static int tegra_audio_graph_card_probe(struct snd_soc_card *card) return PTR_ERR(priv->clk_plla_out0); } - return asoc_graph_card_probe(card); + return 0; } static int tegra_audio_graph_probe(struct platform_device *pdev)
Hi, > (X) int asoc_simple_init_jack(...) > { > ... > if (gpio_is_valid(det)) { > ... > > snd_soc_card_jack_new(...); > snd_soc_jack_add_gpios(...); > for_each_card_components(card, component) > (Y) snd_soc_component_set_jack(component, ...); > } > ... > } So for the case of cpcap codec on motorola mapphones this dosent help, because we dont have a gpio to sense plug state, thus no gpio in dts and thus gpio_is_valid will return false, therefore, no jack. Moving sjack->pin.pin = pin_name; sjack->pin.mask = mask; snd_soc_card_jack_new(card, pin_name, mask, &sjack->jack, &sjack->pin, 1); and for_each_card_components(card, component) snd_soc_component_set_jack(component, &sjack->jack, NULL); outside of the if block should make this work, at least cpcap gets the jack then.
Hi Carl >> (X) int asoc_simple_init_jack(...) >> { >> ... >> if (gpio_is_valid(det)) { >> ... >> >> snd_soc_card_jack_new(...); >> snd_soc_jack_add_gpios(...); >> for_each_card_components(card, component) >> (Y) snd_soc_component_set_jack(component, ...); >> } >> ... >> } > > So for the case of cpcap codec on motorola mapphones this dosent help, > because we dont have a gpio to sense plug state, thus no gpio in dts > and thus gpio_is_valid will return false, therefore, no jack. > > Moving > > sjack->pin.pin = pin_name; > sjack->pin.mask = mask; > > snd_soc_card_jack_new(card, pin_name, mask, > &sjack->jack, > &sjack->pin, 1); > > and > > for_each_card_components(card, component) > snd_soc_component_set_jack(component, &sjack->jack, NULL); > > outside of the if block should make this work, at least cpcap gets the jack then. I see. simple-card is checking hp-det-gpio on DT now. I guess it can help you if simple-card also check "hp-det" (no gpio) (and customize previous patch) ? Is "enable-hp" better naming... ? Best regards --- Kuninori Morimoto
diff --git a/include/sound/simple_card_utils.h b/include/sound/simple_card_utils.h index 51b3b485a92e..547ad537613d 100644 --- a/include/sound/simple_card_utils.h +++ b/include/sound/simple_card_utils.h @@ -57,8 +57,8 @@ struct asoc_simple_priv { struct prop_nums num; unsigned int mclk_fs; } *dai_props; - struct asoc_simple_jack hp_jack; - struct asoc_simple_jack mic_jack; + struct asoc_simple_jack *hp_jack; + struct asoc_simple_jack *mic_jack; struct snd_soc_dai_link *dai_link; struct asoc_simple_dai *dais; struct snd_soc_dai_link_component *dlcs; @@ -173,7 +173,7 @@ int asoc_simple_parse_pin_switches(struct snd_soc_card *card, char *prefix); int asoc_simple_init_jack(struct snd_soc_card *card, - struct asoc_simple_jack *sjack, + struct asoc_simple_jack **sjack, int is_hp, char *prefix, char *pin); int asoc_simple_init_priv(struct asoc_simple_priv *priv, struct link_info *li); diff --git a/sound/soc/generic/simple-card-utils.c b/sound/soc/generic/simple-card-utils.c index 10c63b73900c..1899feba16cc 100644 --- a/sound/soc/generic/simple-card-utils.c +++ b/sound/soc/generic/simple-card-utils.c @@ -395,6 +395,7 @@ int asoc_simple_dai_init(struct snd_soc_pcm_runtime *rtd) struct asoc_simple_priv *priv = snd_soc_card_get_drvdata(rtd->card); struct simple_dai_props *props = simple_priv_to_props(priv, rtd->num); struct asoc_simple_dai *dai; + struct snd_soc_component *component; int i, ret; for_each_prop_dai_codec(props, i, dai) { @@ -412,6 +413,21 @@ int asoc_simple_dai_init(struct snd_soc_pcm_runtime *rtd) if (ret < 0) return ret; + for_each_rtd_components(rtd, i, component) { + if (component->driver->set_jack) { + if (!priv->hp_jack) { + priv->hp_jack = devm_kzalloc(priv->snd_card.dev, + sizeof(*priv->hp_jack), GFP_KERNEL); + snd_soc_card_jack_new(&priv->snd_card, + "Headphones", + SND_JACK_HEADPHONE, + &priv->hp_jack->jack, + NULL, 0); + } + snd_soc_component_set_jack(component, &priv->hp_jack->jack, NULL); + } + } + return 0; } EXPORT_SYMBOL_GPL(asoc_simple_dai_init); @@ -554,7 +570,7 @@ int asoc_simple_parse_pin_switches(struct snd_soc_card *card, EXPORT_SYMBOL_GPL(asoc_simple_parse_pin_switches); int asoc_simple_init_jack(struct snd_soc_card *card, - struct asoc_simple_jack *sjack, + struct asoc_simple_jack **sjack, int is_hp, char *prefix, char *pin) { @@ -569,8 +585,6 @@ int asoc_simple_init_jack(struct snd_soc_card *card, if (!prefix) prefix = ""; - sjack->gpio.gpio = -ENOENT; - if (is_hp) { snprintf(prop, sizeof(prop), "%shp-det-gpio", prefix); pin_name = pin ? pin : "Headphones"; @@ -588,21 +602,26 @@ int asoc_simple_init_jack(struct snd_soc_card *card, return -EPROBE_DEFER; if (gpio_is_valid(det)) { - sjack->pin.pin = pin_name; - sjack->pin.mask = mask; + struct asoc_simple_jack *sjack_d; + + sjack = devm_kzalloc(dev, sizeof(*(*sjack)), GFP_KERNEL); + sjack_d = *sjack; + + sjack_d->pin.pin = pin_name; + sjack_d->pin.mask = mask; - sjack->gpio.name = gpio_name; - sjack->gpio.report = mask; - sjack->gpio.gpio = det; - sjack->gpio.invert = !!(flags & OF_GPIO_ACTIVE_LOW); - sjack->gpio.debounce_time = 150; + sjack_d->gpio.name = gpio_name; + sjack_d->gpio.report = mask; + sjack_d->gpio.gpio = det; + sjack_d->gpio.invert = !!(flags & OF_GPIO_ACTIVE_LOW); + sjack_d->gpio.debounce_time = 150; snd_soc_card_jack_new(card, pin_name, mask, - &sjack->jack, - &sjack->pin, 1); + &sjack_d->jack, + &sjack_d->pin, 1); - snd_soc_jack_add_gpios(&sjack->jack, 1, - &sjack->gpio); + snd_soc_jack_add_gpios(&sjack_d->jack, 1, + &sjack_d->gpio); } return 0;
This allows componants that want a jack to report state on to do so by calling set_jack on components implementing this function. Im not entirely sure this is the right way to do this so RFC Signed-off-by: Carl Philipp Klemm <philipp@uvos.xyz> --- include/sound/simple_card_utils.h | 6 ++-- sound/soc/generic/simple-card-utils.c | 47 +++++++++++++++++++-------- 2 files changed, 36 insertions(+), 17 deletions(-)