Re: [PATCH] ASoC: audio-graph-card: Add audio mixer for Motorola mdm6600
Hi Pavel > motmdm.c handles audio configuration on "gsmtty2"; it needs to know > whether we are in call or not; that part is in motmdm-state.c and > listens on "gsmtty1". Subject is very confusable. I think this patch is not related to audio-graph-card ? I think it can be - ASoC: audio-graph-card: Add audio mixer for Motorola mdm6600 : ASoC: motmdm: Add audio mixer for Motorola mdm6600 Thank you for your help !! Best regards --- Kuninori Morimoto
Re: [PATCH] dt-bindings: Drop type references on common properties
Hi Rob > Users of common properties shouldn't have a type definition as the > common schemas already have one. Drop all the unnecessary type > references in the tree. > > A meta-schema update to catch these is pending. > > Cc: Nicolas Saenz Julienne > Cc: Maxime Ripard > Cc: Linus Walleij > Cc: Bartosz Golaszewski > Cc: Bjorn Andersson > Cc: Krzysztof Kozlowski > Cc: Marc Kleine-Budde > Cc: "David S. Miller" > Cc: Jakub Kicinski > Cc: Srinivas Kandagatla > Cc: Ohad Ben-Cohen > Cc: Mark Brown > Cc: Cheng-Yi Chiang > Cc: Benson Leung > Cc: Zhang Rui > Cc: Daniel Lezcano > Cc: Greg Kroah-Hartman > Cc: Stefan Wahren > Cc: Masahiro Yamada > Cc: Odelu Kukatla > Cc: Alex Elder > Cc: Suman Anna > Cc: Kuninori Morimoto > Cc: Dmitry Baryshkov > Cc: linux-g...@vger.kernel.org > Cc: linux...@vger.kernel.org > Cc: linux-...@vger.kernel.org > Cc: net...@vger.kernel.org > Cc: linux-remotep...@vger.kernel.org > Cc: alsa-de...@alsa-project.org > Cc: linux-...@vger.kernel.org > Signed-off-by: Rob Herring > --- For below Acked-by: Kuninori Morimoto > Documentation/devicetree/bindings/sound/ak4642.yaml | 2 -- > Documentation/devicetree/bindings/sound/renesas,rsnd.yaml| 1 - Thank you for your help !! Best regards --- Kuninori Morimoto
Re: [PATCH 1/3] ASoC: simple-card-utils: Fix device module clock
Hi Sameer > > /* > > * Parse dai->sysclk come from "clocks = <>" > > * (if system has common clock) > > * or "system-clock-frequency = " > > * or device's module clock. > > */ > > Yes, this can be rephrased now. Thanks. It is not a big-deal. no streass :) > > And also, this patch has too many unneeded exchange, > > thus it was difficult to read for me. > > I think it can be simply like this ? > > It is understandable what it want to do. > > I think the patch does exactly the same thing as what you are > suggesting below. Am I missing anything? Yes, it is 100% same, but is simple patch. I wanted to tell was it is easy to read/understand. Your patch is already applied, so nothing we can do now ;) Thank you for your help !! Best regards --- Kuninori Morimoto
Re: [PATCH 1/3] ASoC: simple-card-utils: Fix device module clock
Hi Sameer
> diff --git a/sound/soc/generic/simple-card-utils.c
> b/sound/soc/generic/simple-card-utils.c
> index bc0b62e..0754d70 100644
> --- a/sound/soc/generic/simple-card-utils.c
> +++ b/sound/soc/generic/simple-card-utils.c
> @@ -173,16 +173,15 @@ int asoc_simple_parse_clk(struct device *dev,
>* or device's module clock.
>*/
> clk = devm_get_clk_from_child(dev, node, NULL);
> - if (!IS_ERR(clk)) {
> - simple_dai->sysclk = clk_get_rate(clk);
> + if (IS_ERR(clk))
> + clk = devm_get_clk_from_child(dev, dlc->of_node, NULL);
>
> + if (!IS_ERR(clk)) {
> simple_dai->clk = clk;
> - } else if (!of_property_read_u32(node, "system-clock-frequency", ))
> {
> + simple_dai->sysclk = clk_get_rate(clk);
> + } else if (!of_property_read_u32(node, "system-clock-frequency",
> + )) {
> simple_dai->sysclk = val;
> - } else {
> - clk = devm_get_clk_from_child(dev, dlc->of_node, NULL);
> - if (!IS_ERR(clk))
> - simple_dai->sysclk = clk_get_rate(clk);
> }
The comment is indicating that that the clock parsing order,
but this patch exchanges it.
This comment also should be updated, I think.
/*
* Parse dai->sysclk come from "clocks = <>"
* (if system has common clock)
* or "system-clock-frequency = "
* or device's module clock.
*/
asoc_simple_set_clk_rate() will be called if it has simple_dai->clk.
CPU or Codec component clock rate will be exchanged by this patch, I think.
I'm not sure the effect of this patch to existing boards.
And also, this patch has too many unneeded exchange,
thus it was difficult to read for me.
I think it can be simply like this ?
It is understandable what it want to do.
diff --git a/sound/soc/generic/simple-card-utils.c
b/sound/soc/generic/simple-card-utils.c
index 8c423afb9d2e..d441890de4dc 100644
--- a/sound/soc/generic/simple-card-utils.c
+++ b/sound/soc/generic/simple-card-utils.c
@@ -168,16 +168,14 @@ int asoc_simple_parse_clk(struct device *dev,
* or device's module clock.
*/
clk = devm_get_clk_from_child(dev, node, NULL);
+ if (IS_ERR(clk))
+ clk = devm_get_clk_from_child(dev, dlc->of_node, NULL);
+
if (!IS_ERR(clk)) {
simple_dai->sysclk = clk_get_rate(clk);
-
simple_dai->clk = clk;
} else if (!of_property_read_u32(node, "system-clock-frequency", ))
{
simple_dai->sysclk = val;
- } else {
- clk = devm_get_clk_from_child(dev, dlc->of_node, NULL);
- if (!IS_ERR(clk))
- simple_dai->sysclk = clk_get_rate(clk);
}
if (of_property_read_bool(node, "system-clock-direction-out"))
Thank you for your help !!
Best regards
---
Kuninori Morimoto
Re: [PATCH v2 0/3] Rename audio graph export functions
Hi Sameer > This series renames exported functions from audio graph for a better > global visibility. In doing so update the references in audio graph > and Tegra audio graph card drivers. I guess [1/3] and [2/3] should be merged/squashed ? Otherwise, there is git-bisect error. Except it Acked-by: Kuninori Morimoto Thank you for your help !! Best regards --- Kuninori Morimoto
Re: [PATCH 1/2] ASoC: audio-graph: Export graph_remove() function
Hi > > > > +int graph_remove(struct platform_device *pdev); > > > > I think this needs better namespacing if it's going to be exported. > > > audio_graph_remove() can be a better choice? > > Yeah, that looks reasonable. Nice naming I think. In such case, update also graph_parse_of() is nice idea for me. - int graph_parse_of(...) + int audio_graph_parse_of() Thank you for your help !! Best regards --- Kuninori Morimoto
Re: [PATCH 1/2] ASoC: audio-graph: Export graph_remove() function
Hi Sameer
> Audio graph based sound card drivers can call graph_remove() function
> for cleanups during driver removal. To facilitate this export above
> mentioned function.
>
> Signed-off-by: Sameer Pujar
> Cc: Kuninori Morimoto
(snip)
> -static int graph_remove(struct platform_device *pdev)
> +int graph_remove(struct platform_device *pdev)
> {
> struct snd_soc_card *card = platform_get_drvdata(pdev);
>
> return asoc_simple_clean_reference(card);
> }
> +EXPORT_SYMBOL_GPL(graph_remove);
Not a big deal, but
it is just calling asoc_simple_clean_reference() which is
already global function.
Thank you for your help !!
Best regards
---
Kuninori Morimoto
Re: [PATCHv2] ASoC: audio-graph-card: Add audio mixer for Motorola mdm6600
Hi Pavel > motmdm.c handles audio configuration on "gsmtty2"; it needs to know > whether we are in call or not; that part is in motmdm-state.c and > listens on "gsmtty1". > > To configure Alsamixer for voice calls do for example: > > Speaker Right -> Voice > Call Noise Cancellation -> Unmute > Call Output -> Speakerphone > Call -> 100 > Mic2 -> 40 > Left -> Mic 2 > Voice -> 55 > Mic2 -> 40 > Left -> Mic 2 > > Tony wrote original version using custom interface to n_gsm, Pavel > switched it to plain serdev and split it to two drivers to be easier > to debug and understand. Credit is Tony's, bugs are probably Pavel's. > > Signed-off-by: Pavel Machek > Co-authored-by: Tony Lindgren > --- This is not a big deal, but the this patch is not directly related to audio-graph-card. Including it on Subject is confusable. Thank you for your help !! Best regards --- Kuninori Morimoto
Re: [PATCH] of: property: add port base loop
Hi Rob Thank you for your feedback > > In such case, people want to have port base loop > > instead of endpoints base loop. > > This patch adds such functions/macros. > > I'm a bit hesitant on these too. A driver should generally know what > each port # is (since the binding has to define them), and it should > just request the port (or its connection) it wants. At least that was > the premise behind of_graph_get_remote_node() and the cleanups (mostly > DRM drivers) I did to use it. I'd rather see things move in that > direction. I'm working for ALSA SoC now. To use ALSA SoC sound, it needs at least 3 drivers. The purpose of "Card" here is for glueing SoC and Codec. SoC <-> Card <-> Codec If board/platform needs special handling, user needs to create specific Card driver. In such case, it needs to know detail of each ports. But many cases, we want to use generic Card driver. Because it is generic, it don't know how many ports are used, and endpoint base loop is not enough for it. This is my motivation for this patch. > In any case, this needs a user before merging. OK. I will repost it again as part of above patch-set. Thank you for your help !! Best regards --- Kuninori Morimoto
[PATCH] of: property: add port base loop
From: Kuninori Morimoto
We have endpoint base functions
- of_graph_get_next_endpoint()
- of_graph_get_endpoint_count()
- for_each_endpoint_of_node()
Here, for_each_endpoint_of_node() loop finds endpoint
ports {
port@0 {
(1) endpoint {...};
};
port@1 {
(2) endpoint {...};
};
...
};
In above case, for_each_endpoint_of_node() loop
finds endpoint as (1) -> (2) -> ...
We can check endpoint parent to get its port
if user want to do something to it.
But port can have multi endpoints.
In such case, it is difficult to find
port@0 -> port@1 -> ...
ports {
port@0 {
(1) endpoint@0 {...};
(2) endpoint@1 {...};
};
port@1 {
(3) endpoint {...};
};
...
};
In such case, people want to have port base loop
instead of endpoints base loop.
This patch adds such functions/macros.
Signed-off-by: Kuninori Morimoto
---
drivers/of/property.c| 69 ++--
include/linux/of_graph.h | 14
2 files changed, 73 insertions(+), 10 deletions(-)
diff --git a/drivers/of/property.c b/drivers/of/property.c
index 5f9eed79a8aa..9b511cfe97b3 100644
--- a/drivers/of/property.c
+++ b/drivers/of/property.c
@@ -631,15 +631,7 @@ struct device_node *of_graph_get_next_endpoint(const
struct device_node *parent,
* parent port node.
*/
if (!prev) {
- struct device_node *node;
-
- node = of_get_child_by_name(parent, "ports");
- if (node)
- parent = node;
-
- port = of_get_child_by_name(parent, "port");
- of_node_put(node);
-
+ port = of_graph_get_next_port(parent, NULL);
if (!port) {
pr_err("graph: no port node found in %pOF\n", parent);
return NULL;
@@ -666,14 +658,59 @@ struct device_node *of_graph_get_next_endpoint(const
struct device_node *parent,
/* No more endpoints under this port, try the next one. */
prev = NULL;
+ port = of_graph_get_next_port(parent, port);
+ if (!port)
+ return NULL;
+ }
+}
+EXPORT_SYMBOL(of_graph_get_next_endpoint);
+
+/**
+ * of_graph_get_next_port() - get next port node
+ * @parent: pointer to the parent device node
+ * @prev: previous port node, or NULL to get first
+ *
+ * Return: An 'port' node pointer with refcount incremented. Refcount
+ * of the passed @prev node is decremented.
+ */
+struct device_node *of_graph_get_next_port(const struct device_node *parent,
+ struct device_node *prev)
+{
+ struct device_node *port = prev;
+
+ if (!parent)
+ return NULL;
+
+ /*
+* Start by locating the port node. If no previous endpoint is specified
+* search for the first port node, otherwise get the previous endpoint
+* parent port node.
+*/
+ if (!port) {
+ struct device_node *node;
+
+ node = of_get_child_by_name(parent, "ports");
+ if (node)
+ parent = node;
+
+ port = of_get_child_by_name(parent, "port");
+ of_node_put(node);
+
+ if (!port) {
+ pr_err("graph: no port node found in %pOF\n", parent);
+ return NULL;
+ }
+ } else {
do {
port = of_get_next_child(parent, port);
if (!port)
return NULL;
} while (!of_node_name_eq(port, "port"));
}
+
+ return port;
}
-EXPORT_SYMBOL(of_graph_get_next_endpoint);
+EXPORT_SYMBOL(of_graph_get_next_port);
/**
* of_graph_get_endpoint_by_regs() - get endpoint node of specific identifiers
@@ -800,6 +837,18 @@ int of_graph_get_endpoint_count(const struct device_node
*np)
}
EXPORT_SYMBOL(of_graph_get_endpoint_count);
+int of_graph_get_port_count(const struct device_node *np)
+{
+ struct device_node *port;
+ int num = 0;
+
+ for_each_port_of_node(np, port)
+ num++;
+
+ return num;
+}
+EXPORT_SYMBOL(of_graph_get_port_count);
+
/**
* of_graph_get_remote_node() - get remote parent device_node for given
port/endpoint
* @node: pointer to parent device_node containing graph port/endpoint
diff --git a/include/linux/of_graph.h b/include/linux/of_graph.h
index 4d7756087b6b..8cd3bd674ebd 100644
--- a/include/linux/of_graph.h
+++ b/include/linux/of_graph.h
@@ -26,6 +26,17 @@ struct of_endpoint {
const struct devic
Re: [PATCH v4 3/6] ASoC: audio-graph-card: Support setting component plls and sysclks
Hi Richard > > As I mentioned in v3, adding *general* pll to common card driver is > > maybe difficult. > > You did say that. But you did not say why. > Can you be more specific about what problem you see with adding it > to the generic driver? > > > Using your own customized audio-graph-card driver is better idea, > > instead of adding code to common driver. > > I just don't want to duplicate code without good reason. Ahh, sorry for my unclear comment. I think "PLL settings" is very board/platform specific, so, adding such code to common driver will be issue in the future. This is the reason why I don't want add it to audio-graph-card. But, as I mentioned above and Sameer is already doing, you can reuse audio-graph-card and customize it. Reuse audio-graph-card + Use your own PLL code = your own customized audio-graph-card You can reuse audio-graph-card code by calling graph_parse_of(), and customize before/after that. I think no duplicate code is needed. I hope it can help you. Thank you for your help !! Best regards --- Kuninori Morimoto
Re: [PATCH v4 3/6] ASoC: audio-graph-card: Support setting component plls and sysclks
Hi Richard > Some codecs need plls and/or sysclks to be configured using the > snd_soc_component_set_[sysclk|pll] functions. These drivers cannot > necessarily be converted to use the clock framework. If the codec is on > a I2C/SPI bus, a nested clk_get would be needed to enable the bus clock. > But the clock framework does not support nested operations and this would > deadlock. > > This patch adds new dt properties that list phandles of components with > the pll/sysclk settings to be applied. Multiple settings can be given for > the same phandle to allow for components with multiple clocks and plls. > The plls and sysclks are enabled when the card bias level moves to STANDBY > and disabled when it moves to OFF. > > The implementation does not attempt to handle specifying complex clock > ordering interdependencies between components. The plls and sysclks are > applied to a component as it is passed to the card set_bias_level/ > set_bias_level_post callbacks. It follows from this that the order > components are configured is the order that they are passed to those > callbacks. > > Signed-off-by: Richard Fitzgerald > --- As I mentioned in v3, adding *general* pll to common card driver is maybe difficult. Using your own customized audio-graph-card driver is better idea, instead of adding code to common driver. I think Sameer's Tegra driver (= [3/6]) is good sample for you ? https://lore.kernel.org/r/1606413823-19885-1-git-send-email-spu...@nvidia.com Thank you for your help !! Best regards --- Kuninori Morimoto
Re: [PATCH v3 3/6] ASoC: audio-graph-card: Support setting component plls and sysclks
Hi Richard Thank you for your patch. This is v3 patch, but I think this is the first time for me to receive patch... > Some codecs need plls and/or sysclks to be configured using the > snd_soc_component_set_[sysclk|pll] functions. These drivers cannot > necessarily be converted to use the clock framework. If the codec is on > a I2C/SPI bus, a nested clk_get would be needed to enable the bus clock. > But the clock framework does not support nested operations and this would > deadlock. > > This patch adds new dt properties that list phandles of components with > the pll/sysclk settings to be applied. Multiple settings can be given for > the same phandle to allow for components with multiple clocks and plls. > The plls and sysclks are enabled when the card bias level moves to STANDBY > and disabled when it moves to OFF. > > The implementation does not attempt to handle specifying complex clock > ordering interdependencies between components. The plls and sysclks are > applied to a component as it is passed to the card set_bias_level/ > set_bias_level_post callbacks. It follows from this that the order > components are configured is the order that they are passed to those > callbacks. > > Signed-off-by: Richard Fitzgerald > --- > include/sound/simple_card_utils.h | 25 +++ > sound/soc/generic/audio-graph-card.c | 16 +- > sound/soc/generic/simple-card-utils.c | 236 ++ > 3 files changed, 275 insertions(+), 2 deletions(-) I understand that you need sysclk/pll and .set_bias_level_xxx(). But I guess makes it generic code is difficult (?). Thus, as Sameer doing on Tegra, creating custom audio-graph-card is better idea for you ? # Now I'm creating new audio-graph-card2 which also supports # overwriting/customizing each/all functions. # It is not full compatible with audio-graph-card, but almost same if you # uses normal connection. # I hope I can post it next year Thank you for your help !! Best regards --- Kuninori Morimoto
Re: [PATCH v0] ASoC: rsnd: core: Check convert rate in rsnd_hw_params
Hi Mikhail
Thank you for your patch
> + switch (rsnd_mod_id(src_mod)) {
> + /*
> + * SRC0 can downsample 4, 6 and 8 channel audio up to 4
> times.
> + * SRC1, SRC3 and SRC4 can downsample 4 channel audio
> + * up to 4 times.
> + * SRC1, SRC3 and SRC4 can downsample 6 and 8 channel
> audio
> + * no more than twice.
> + */
> + case 1:
> + case 3:
> + case 4:
> + if (channel > 4) {
> + k_down = 2;
> + break;
> + }
> + case 0:
> + if (channel > 2)
> + k_down = 4;
> + break;
> +
> + /* Other SRC units do not support more than 2 channels
> */
> + default:
> + if (channel > 2)
> + return -EINVAL;
> + }
I think you want to add "fallthrough" between case 1/3/4 and case 0 ?
Thank you for your help !!
Best regards
---
Kuninori Morimoto
Re: [PATCH 0/2] Refactor Audio Graph schema
Hi Sameer
> There can be custom sound cards reusing most of the audio-graph
> implementation. To allow this refactor the audio-graph-card schema into
> following files.
> * audio-graph.yaml : defines all common bindings
> * audio-graph-card.yaml : define compatible property
>
> Custom sound cards can just reference 'audio-graph.yaml' and define its
> own compatible and specific properties.
Thank you for your patches.
I'm not expert of Json-schema, but for audio-graph-card Doc customizing
Acked-by: Kuninori Morimoto
Thank you for your help !!
Best regards
---
Kuninori Morimoto
Re: [PATCH v5 0/7] Audio Graph Updates
Hi Mark > > This series is a prepraration for using generic graph driver for Tegra210 > > audio. Tegra audio graph series will be sent in a separate series because > > it has some dependency over other series for documentation work. This > > series can focus on the generic ASoC driver updates. Below are the summary > > of changes done. > > Morimoto-san, are you OK with these? Yes. For audio-graph part Acked-by: Kuninori Morimoto Thank you for your help !! Best regards --- Kuninori Morimoto
Re: [PATCH v4 09/15] ASoC: dt-bindings: audio-graph: Convert bindings to json-schema
Hi Sameer > >> Convert device tree bindings of audio graph card to YAML format. Also > >> expose some common definitions which can be used by similar graph based > >> audio sound cards. > >> > >> Signed-off-by: Sameer Pujar > >> Cc: Kuninori Morimoto > >> --- > > I'm posting this patch to Rob & DT ML. > > Not yet accepted, though.. > > Thanks for letting me know. I guess below is your patch, > http://patchwork.ozlabs.org/project/devicetree-bindings/patch/877dtlvsxk.wl-kuninori.morimoto...@renesas.com/ > Do you have plans to resend this or send next revision? > > I can drop my patch once yours is merged and refer the same for Tegra > audio graph card. I'm waiting response from Rob now. It is merge window now. I will re-post it without his response if -rc1 was released. Thank you for your help !! Best regards --- Kuninori Morimoto
Re: [PATCH v4 09/15] ASoC: dt-bindings: audio-graph: Convert bindings to json-schema
Hi Sameer
> Convert device tree bindings of audio graph card to YAML format. Also
> expose some common definitions which can be used by similar graph based
> audio sound cards.
>
> Signed-off-by: Sameer Pujar
> Cc: Kuninori Morimoto
> ---
I'm posting this patch to Rob & DT ML.
Not yet accepted, though..
> .../devicetree/bindings/sound/audio-graph-card.txt | 337 -
> .../bindings/sound/audio-graph-card.yaml | 548
> +
> 2 files changed, 548 insertions(+), 337 deletions(-)
> delete mode 100644
> Documentation/devicetree/bindings/sound/audio-graph-card.txt
> create mode 100644
> Documentation/devicetree/bindings/sound/audio-graph-card.yaml
>
> diff --git a/Documentation/devicetree/bindings/sound/audio-graph-card.txt
> b/Documentation/devicetree/bindings/sound/audio-graph-card.txt
> deleted file mode 100644
> index d5f6919..000
> --- a/Documentation/devicetree/bindings/sound/audio-graph-card.txt
> +++ /dev/null
> @@ -1,337 +0,0 @@
> -Audio Graph Card:
> -
> -Audio Graph Card specifies audio DAI connections of SoC <-> codec.
> -It is based on common bindings for device graphs.
> -see ${LINUX}/Documentation/devicetree/bindings/graph.txt
> -
> -Basically, Audio Graph Card property is same as Simple Card.
> -see ${LINUX}/Documentation/devicetree/bindings/sound/simple-card.yaml
> -
> -Below are same as Simple-Card.
> -
> -- label
> -- widgets
> -- routing
> -- dai-format
> -- frame-master
> -- bitclock-master
> -- bitclock-inversion
> -- frame-inversion
> -- mclk-fs
> -- hp-det-gpio
> -- mic-det-gpio
> -- dai-tdm-slot-num
> -- dai-tdm-slot-width
> -- clocks / system-clock-frequency
> -
> -Required properties:
> -
> -- compatible : "audio-graph-card";
> -- dais : list of CPU DAI port{s}
> -
> -Optional properties:
> -- pa-gpios: GPIO used to control external amplifier.
> -
>
> -Example: Single DAI case
>
> -
> - sound_card {
> - compatible = "audio-graph-card";
> -
> - dais = <_port>;
> - };
> -
> - dai-controller {
> - ...
> - cpu_port: port {
> - cpu_endpoint: endpoint {
> - remote-endpoint = <_endpoint>;
> -
> - dai-format = "left_j";
> - ...
> - };
> - };
> - };
> -
> - audio-codec {
> - ...
> - port {
> - codec_endpoint: endpoint {
> - remote-endpoint = <_endpoint>;
> - };
> - };
> - };
> -
>
> -Example: Multi DAI case
>
> -
> - sound-card {
> - compatible = "audio-graph-card";
> -
> - label = "sound-card";
> -
> - dais = <_port0
> - _port1
> - _port2>;
> - };
> -
> - audio-codec@0 {
> - ...
> - port {
> - codec0_endpoint: endpoint {
> - remote-endpoint = <_endpoint0>;
> - };
> - };
> - };
> -
> - audio-codec@1 {
> - ...
> - port {
> - codec1_endpoint: endpoint {
> - remote-endpoint = <_endpoint1>;
> - };
> - };
> - };
> -
> - audio-codec@2 {
> - ...
> - port {
> - codec2_endpoint: endpoint {
> - remote-endpoint = <_endpoint2>;
> - };
> - };
> - };
> -
> - dai-controller {
> - ...
> - ports {
> - cpu_port0: port@0 {
> - cpu_endpoint0: endpoint {
> - remote-endpoint = <_endpoint>;
> -
> - dai-format = "left_j";
> - ...
> - };
> - };
> - cpu_port1: port@1 {
> - cpu_endpoint1: endpoint {
> - remote-endpoint = <_endpoint>;
> -
> - dai-format
Re: [PATCH v3 10/13] ASoC: tegra: Add audio graph based card driver
Hi Sameer
> > This is just an idea,
> > but can we use hooks here somehow ?
> >
> > .ops_hook_pre
> > .ops_hook_func
> > .ops_hook_post
> >
> > if (priv->ops_hook_pre->func)
> > priv->ops_hook_pre->func_pre(...);
> >
> > if (priv->ops_hook_func->func)
> > priv->ops_hook_func->func(...); /* driver's function */
> > else
> > graph_func(...);/* audio-graph function */
> >
> > if (priv->ops_hook_post->func)
> > priv->ops_hook_post->func(...);
>
> Right now I just required to populate some flags or structures and do
> not have any specific pre()/post() functions to be called. Can this be
> reserved for later?
Yeah, of course :)
> > These are almost same as graph_probe().
> > Maybe we can separate graph_probe() and export function ?
>
> Yes possible, I can move more stuff into graph_parse_of() which is
> already an exported function in the current series. This can be
> utilized by both generic audio graph and Tegra audio graph.
>
> Something like below,
>
> static int tegra_audio_graph_probe(struct platform_device *pdev)
> {
> struct tegra_audio_priv *priv;
> struct device *dev = >dev;
> struct snd_soc_card *card;
>
> priv = devm_kzalloc(dev, sizeof(*priv), GFP_KERNEL);
> if (!priv)
> return -ENOMEM;
>
> card = simple_priv_to_card(>simple);
>
> card->owner = THIS_MODULE;
> card->dev = dev;
> card->probe = tegra_audio_graph_card_probe;
>
> /* graph_parse_of() depends on below */
> card->component_chaining = 1;
> priv->simple.ops = _audio_graph_ops;
> priv->simple.force_dpcm = 1;
>
> return graph_parse_of(>simple);
> }
I think graph side can handle card->owner / card->dev,
but, it looks good to me
Thank you for your help !!
Best regards
---
Kuninori Morimoto
Re: [PATCH v3 10/13] ASoC: tegra: Add audio graph based card driver
Hi Sameer
> Add Tegra audio machine driver which is based on generic audio graph card
> driver. It re-uses most of the common stuff from audio graph driver and
> uses the same DT binding. Required Tegra specific customizations are done
> in the driver.
(snip)
> +static const struct snd_soc_ops tegra_audio_graph_ops = {
> + .startup= asoc_simple_startup,
> + .shutdown = asoc_simple_shutdown,
> + .hw_params = tegra_audio_graph_hw_params,
> +};
This is just an idea,
but can we use hooks here somehow ?
.ops_hook_pre
.ops_hook_func
.ops_hook_post
if (priv->ops_hook_pre->func)
priv->ops_hook_pre->func_pre(...);
if (priv->ops_hook_func->func)
priv->ops_hook_func->func(...); /* driver's function */
else
graph_func(...);/* audio-graph function */
if (priv->ops_hook_post->func)
priv->ops_hook_post->func(...);
> +static int tegra_audio_graph_probe(struct platform_device *pdev)
> +{
> + struct asoc_simple_priv *priv;
> + struct device *dev = >dev;
> + struct snd_soc_card *card;
> + struct link_info li;
> + int err;
> +
> + /* Allocate the private data and the DAI link array */
> + priv = devm_kzalloc(dev, sizeof(*priv), GFP_KERNEL);
> + if (!priv)
> + return -ENOMEM;
> +
> + priv->data = (struct tegra_audio_graph_data *)
> + devm_kzalloc(dev, sizeof(*priv->data), GFP_KERNEL);
> + if (!priv->data)
> + return -ENOMEM;
> +
> + card = simple_priv_to_card(priv);
> +
> + card->owner = THIS_MODULE;
> + card->dev = dev;
> + card->component_chaining = true;
> + card->probe = tegra_audio_graph_card_probe;
> +
> + priv->ops = _audio_graph_ops;
> + priv->force_dpcm = 1;
> +
> + memset(, 0, sizeof(li));
> + graph_get_dais_count(priv, );
> + if (!li.link || !li.dais)
> + return -EINVAL;
> +
> + err = asoc_simple_init_priv(priv, );
> + if (err < 0)
> + return err;
> +
> + err = graph_parse_of(priv);
> + if (err < 0) {
> + if (err != -EPROBE_DEFER)
> + dev_err(dev, "Parse error %d\n", err);
> + goto cleanup;
> + }
> +
> + snd_soc_card_set_drvdata(card, priv);
> +
> + asoc_simple_debug_info(priv);
> +
> + err = devm_snd_soc_register_card(dev, card);
> + if (err < 0)
> + goto cleanup;
> +
> + return 0;
> +
> +cleanup:
> + asoc_simple_clean_reference(card);
> +
> + return err;
> +}
These are almost same as graph_probe().
Maybe we can separate graph_probe() and export function ?
struct tegra_audio_graph_data
{
struct asoc_simple_priv simple;
...
};
#define simple_to_priv(_simple) container_of((_simple), struct my_priv,
simple)
static int tegra_audio_graph_probe(struct platform_device *pdev)
{
struct tegra_audio_graph_data *data;
struct asoc_simple_priv *priv;
/* Allocate the private data */
data = devm_kzalloc(dev, sizeof(*data), GFP_KERNEL);
if (!data)
return -ENOMEM;
/* initial audio-graph */
ret = audio_graph_init(priv, pdev);
if (ret < 0)
return -xxx;
/* over-write for own settings */
card = simple_priv_to_card(priv);
card->component_chaining = true;
card->probe = tegra_audio_graph_card_probe;
priv = >simple;
priv->ops_hook_pre = _audio_graph_ops;
priv->force_dpcm = 1;
/* audio-graph remain */
return audio_graph_prove(priv, pdev);
}
Thank you for your help !!
Best regards
---
Kuninori Morimoto
Re: [PATCH v3 07/13] ASoC: audio-graph: Update driver as per new exposed members
Hi Sameer > As per the members exposed earlier in the series, audio graph driver > is updated to make use of these. Functionally there is no change > in behavior if these are not populated. So following changes are made > as part of this. > > - Update 'dai_link->ops' for DPCM links if a custom 'snd_soc_ops' >is defined by the vendor driver. > > - Consider 'force_dpcm' flag status when deciding if a DAI link >needs to be treated as DPCM or not. In doing so the logic is >moved to a separate inline function for a better readability. > > - Populate 'dpcm_selectable' flag which is then used to detect >DPCM DAI links. > > Signed-off-by: Sameer Pujar > Cc: Kuninori Morimoto > --- Can we merge [06/13] and [07/13] patches ? Thank you for your help !! Best regards --- Kuninori Morimoto
Re: [PATCH v3 06/13] ASoC: simple-card-utils: Expose new members for asoc_simple_priv
Hi Sameer
Thank you for the patch
> Add new members in struct 'asoc_simple_priv'. Idea is to leverage
> simple or graph card driver as much as possible and vendor can
> maintain a thin driver to control the behavior by populating these
> newly exposed members.
re-use simple/audio-graph driver is nice idea.
My planning next new audio-graph2 can renuse it.
> diff --git a/include/sound/simple_card_utils.h
> b/include/sound/simple_card_utils.h
> index 86a1e95..9825308 100644
> --- a/include/sound/simple_card_utils.h
> +++ b/include/sound/simple_card_utils.h
> @@ -56,6 +56,10 @@ struct asoc_simple_priv {
> struct asoc_simple_dai *dais;
> struct snd_soc_codec_conf *codec_conf;
> struct gpio_desc *pa_gpio;
> + const struct snd_soc_ops *ops;
> + unsigned int force_dpcm:1;
> + uintptr_t dpcm_selectable;
> + void *data;
> };
I have opinions about these.
About dpcm_selectable, indeed current audio-graph is using it as "uintptr_t",
but as you know, it checks whether it was non-NULL or not only.
This means we can use it as bit-field.
BTW, do we need to have dpcm_selectable at priv ?
One note is that, -scu- user is only me (locally),
and it will be removed when audio-graph2 was created.
(My plan is keep code for you, but remove compatible)
About *data, I think we can avoid *data
if driver side priv includes asoc_simple_priv ?
struct my_priv {
struct asoc_simple_priv *simple;
...
};
#define simple_to_priv(_simple) container_of((_simple), struct my_priv,
simple)
Thank you for your help !!
Best regards
---
Kuninori Morimoto
Re: [PATCH v2 3/9] ASoC: audio-graph: Identify 'no_pcm' DAI links for DPCM
Hi Sameer > Sure. BTW, there are more such candidates which require 'lock' version > of these helpers. > For example: soc_find_component(), snd_soc_add/remove_pcm_runtime() > and snd_soc_register_dai(). soc_find_component() is static function, no need to care about mutex. other functions are indeed exported function, but is used from topology.c which is calling it under mutex. Thank you for your help !! Best regards --- Kuninori Morimoto
Re: [PATCH v2 3/9] ASoC: audio-graph: Identify 'no_pcm' DAI links for DPCM
Hi Sameer > > Other solution is create both snd_soc_find_dai_with_mutex()/without_mutex(). > > I'm not sure which style is best. > > I don't know how complex it is to have a unified solution. But if we > can protect snd_soc_find_dai() itself, things would be simpler may be > in long term. Right now there are separate source files for soc-core, > soc-dai and soc-component, but because of two approaches looks like > the function need to be moved around and need to be placed in > soc-core. Also the issue might go unnoticed if LOCKDEP is not enabled. > > May be start with a wrapper for now and eventually unify? Yeah, it seems has _with_mutex() can be better idea. I'm posting patch, but I noticed that Mark's branch vs Linus branch have some mismatch (?), and now I'm asking it to him. I can post _with_mutex() version as v2 if I could get answer. After that I'm happy your next patch can re-use it. Thank you for your help !! Best regards --- Kuninori Morimoto
Re: [PATCH v2 3/9] ASoC: audio-graph: Identify 'no_pcm' DAI links for DPCM
Hi Sameer > > Yes, I'm posting fixup patch. > > > > https://patchwork.kernel.org/patch/11719919/ > > Just curious that why snd_soc_find_dai() itself cannot be protected, > instead of leaving this to callers. Because, snd_soc_find_dai() is called both with/without client_mutex. (same/sof are calling it with mutex, simple-card/audio-graph are calling without mutex) Other solution is create both snd_soc_find_dai_with_mutex()/without_mutex(). I'm not sure which style is best. Thank you for your help !! Best regards --- Kuninori Morimoto
Re: [PATCH v2 3/9] ASoC: audio-graph: Identify 'no_pcm' DAI links for DPCM
Hi Sameer
> +static bool soc_component_is_pcm(struct
> snd_soc_dai_link_component *dlc)
> +{
> + struct snd_soc_dai *dai = snd_soc_find_dai(dlc);
> +
> + if (dai && (dai->component->driver->pcm_construct ||
> + dai->driver->pcm_new))
> + return true;
> +
> + return false;
> +}
(snip)
> I tried testing this with LOCKDEP config enabled at my end.
> It seems I don't see warning originated from above function.
> Are you suggesting that, in general, snd_soc_find_dai()
> should be called with client_mutex held?
Hmm ? strange...
snd_soc_find_dai() is using lockdep_assert_held()
struct snd_soc_dai *snd_soc_find_dai(...)
{
...
=> lockdep_assert_held(_mutex);
...
}
and lockdep_assert_held() will indicate WARN_ON()
-- lockdep.h --
...
#ifdef CONFIG_LOCKDEP
...
#define lockdep_assert_held(l) do {\
=> WARN_ON(debug_locks && !lockdep_is_held(l));\
} while (0)
> May be snd_soc_dai_link_set_capabilities() requires similar fix?
Yes, I'm posting fixup patch.
https://patchwork.kernel.org/patch/11719919/
Thank you for your help !!
Best regards
---
Kuninori Morimoto
Re: [PATCH v2 0/9] Audio graph card updates and usage with Tegra210 audio
Hi Sameer > >> This series proposes following enhancements to audio-graph card driver. > >> * Support multiple instances of a component. > >> * Support open platforms with empty Codec endpoint. > >> * Identify no-pcm DPCM DAI links which can be used in BE<->BE > >> connections. > >> * Add new compatible to support DPCM based DAI chaining. > >> > >> This pushes DT support for Tegra210 based platforms which uses audio-graph > >> card and above enhancements. > >> > >> The series is based on following references where DPCM usgae for Tegra > >> Audio and simple-card driver proposal were discussed. > >> * https://lkml.org/lkml/2020/4/30/519 (DPCM for Tegra) > >> * https://lkml.org/lkml/2020/6/27/4 (simple-card driver) > > I will try to test this patch-set this week, and report/review it. > > Thank you for review so far. Have you also got a chance to review > remaining commits in the series? I have no comment/opinion for other patches. Thanks Thank you for your help !! Best regards --- Kuninori Morimoto
Re: [PATCH v2 3/9] ASoC: audio-graph: Identify 'no_pcm' DAI links for DPCM
Hi again
> PCM devices are created for FE dai links with 'no-pcm' flag as '0'.
> Such DAI links have CPU component which implement either pcm_construct()
> or pcm_new() at component or dai level respectively. Based on this,
> current patch exposes a helper function to identify such components
> and populate 'no_pcm' flag for DPCM DAI link.
>
> This helps to have BE<->BE component links where PCM devices need
> not be created for CPU component involved in such links.
>
> Signed-off-by: Sameer Pujar
> ---
(snip)
> +static bool soc_component_is_pcm(struct snd_soc_dai_link_component *dlc)
> +{
> + struct snd_soc_dai *dai = snd_soc_find_dai(dlc);
> +
> + if (dai && (dai->component->driver->pcm_construct ||
> + dai->driver->pcm_new))
> + return true;
> +
> + return false;
> +}
This snd_soc_find_dai() will indicate WARNING
if .config has CONFIG_LOCKDEP for me.
Maybe implement it at soc-core.c with client_mutex lock
is needed.
Thank you for your help !!
Best regards
---
Kuninori Morimoto
Re: [PATCH v2 3/9] ASoC: audio-graph: Identify 'no_pcm' DAI links for DPCM
Hi Sameer > PCM devices are created for FE dai links with 'no-pcm' flag as '0'. > Such DAI links have CPU component which implement either pcm_construct() > or pcm_new() at component or dai level respectively. Based on this, > current patch exposes a helper function to identify such components > and populate 'no_pcm' flag for DPCM DAI link. > > This helps to have BE<->BE component links where PCM devices need > not be created for CPU component involved in such links. > > Signed-off-by: Sameer Pujar > --- (snip) > @@ -259,6 +270,16 @@ static int graph_dai_link_of_dpcm(struct > asoc_simple_priv *priv, > if (ret < 0) > goto out_put_node; > > + /* > + * In BE<->BE connections it is not required to create > + * PCM devices at CPU end of the dai link and thus 'no_pcm' > + * flag needs to be set. It is useful when there are many > + * BE components and some of these have to be connected to > + * form a valid audio path. > + */ > + if (!soc_component_is_pcm(cpus)) > + dai_link->no_pcm = 1; > + For safety, I want to judge with data->component_chaining. if (data->component_chaining && !soc_component_is_pcm(cpus)) dai_link->no_pcm = 1; Thank you for your help !! Best regards --- Kuninori Morimoto
Re: [PATCH v2 2/9] ASoC: audio-graph: Use of_node and DAI for DPCM DAI link names
Hi > For multiple instances of components, using DAI name alone for DAI links > is causing conflicts. Components can define multiple DAIs and hence using > just a device name won't help either. Thus DT device node reference and > DAI names are used to uniquely represent DAI link names. > > Signed-off-by: Sameer Pujar > --- Acked-by: Kuninori Morimoto Thank you for your help !! Best regards --- Kuninori Morimoto
Re: [PATCH v2 0/9] Audio graph card updates and usage with Tegra210 audio
Hi Sameer Cc Mark > This series proposes following enhancements to audio-graph card driver. > * Support multiple instances of a component. > * Support open platforms with empty Codec endpoint. > * Identify no-pcm DPCM DAI links which can be used in BE<->BE connections. > * Add new compatible to support DPCM based DAI chaining. > > This pushes DT support for Tegra210 based platforms which uses audio-graph > card and above enhancements. > > The series is based on following references where DPCM usgae for Tegra > Audio and simple-card driver proposal were discussed. > * https://lkml.org/lkml/2020/4/30/519 (DPCM for Tegra) > * https://lkml.org/lkml/2020/6/27/4 (simple-card driver) I will try to test this patch-set this week, and report/review it. Thank you for your help !! Best regards --- Kuninori Morimoto
Re: [PATCH 03/10] ASoC: audio-graph: Support Codec with multiple endpoints
Hi Sameer
> >> diff --git a/sound/soc/generic/audio-graph-card.c
> >> b/sound/soc/generic/audio-graph-card.c
> >> index 1e20562..b1903f9 100644
> >> --- a/sound/soc/generic/audio-graph-card.c
> >> +++ b/sound/soc/generic/audio-graph-card.c
> >> @@ -201,8 +201,7 @@ static void graph_parse_mclk_fs(struct device_node
> >> *top,
> >> static int graph_dai_link_of_dpcm(struct asoc_simple_priv *priv,
> >> struct device_node *cpu_ep,
> >> struct device_node *codec_ep,
> >> - struct link_info *li,
> >> - int dup_codec)
> >> + struct link_info *li)
> > This patch breaks DPCM connection which is used for MIXer
>
> Could you please elaborate a bit more as to what is broken with this?
> The problem I am trying to solve here is to have multiple endpoints
> for Codec port.
For example MIXer, it is like below.
If you removes "dup_codec", it breaks MIXer and/or TDM split mode.
CPU0 ---+ DAI
|
CPU1 ---+
cpu {
ports {
port@0 {
cpu_0: endpoint {
remote-endpoint = <_0>;
};
};
port@1 {
cpu_1: endpoint {
remote-endpoint = <_1>;
};
};
};
};
codec {
port {
codec_0: endpoint {
remote-endpoint = <_0>;
};
codec_1: endpoint {
remote-endpoint = <_1>;
};
}
};
Thank you for your help !!
Best regards
---
Kuninori Morimoto
Re: [PATCH 09/10] arm64: tegra: Audio graph header for Tegra210
Hi Sameer
> >> +_admaif {
> >> + admaif_port: port {
> >> + admaif0: endpoint@0 {
> >> + remote-endpoint = <_admaif0>;
> >> + };
> >> + admaif1: endpoint@1 {
> >> + remote-endpoint = <_admaif1>;
> >> + };
> > (snip)
> >> + admaif8: endpoint@8 {
> >> + remote-endpoint = <_admaif8>;
> >> + };
> >> + admaif9: endpoint@9 {
> >> + remote-endpoint = <_admaif9>;
> >> + };
> >> + };
> >> +};
> > I'm not familiar with your system, so, one question.
> > Does this ADMAIF has total 10 interface which is used in the same time ?
> > or select one of 10 connections when use ?
>
> One ore more ADMAIF interfaces can be used simultaneously. In fact all
> of them can be used at the same time.
Ah, sorry my questoin was not correct.
I want to clarify was that below.
In my understanding,
if your system has 10 interfaces,
you need to create 10 ports, not 10 endpoints.
If your system has 1 interface, but is connected from 10
devices, it has 1 port 10 endpoints.
Thank you for your help !!
Best regards
---
Kuninori Morimoto
Re: [PATCH 09/10] arm64: tegra: Audio graph header for Tegra210
Hi Sameer
> +_admaif {
> + admaif_port: port {
> + admaif0: endpoint@0 {
> + remote-endpoint = <_admaif0>;
> + };
> + admaif1: endpoint@1 {
> + remote-endpoint = <_admaif1>;
> + };
(snip)
> + admaif8: endpoint@8 {
> + remote-endpoint = <_admaif8>;
> + };
> + admaif9: endpoint@9 {
> + remote-endpoint = <_admaif9>;
> + };
> + };
> +};
I'm not familiar with your system, so, one question.
Does this ADMAIF has total 10 interface which is used in the same time ?
or select one of 10 connections when use ?
Thank you for your help !!
Best regards
---
Kuninori Morimoto
Re: [PATCH 03/10] ASoC: audio-graph: Support Codec with multiple endpoints
Hi Sameer > If a Codec port has multiple endpoints, only first endpoint gets parsed > and remaining are ignored. This can be fixed by removing 'dup_codec' flag > passed to graph_dai_link_of_dpcm() and thus it loops over all endpoints > of Codec. Similarly graph_count_dpcm() is updated as well. > > Signed-off-by: Sameer Pujar > --- > sound/soc/generic/audio-graph-card.c | 28 +--- > 1 file changed, 9 insertions(+), 19 deletions(-) > > diff --git a/sound/soc/generic/audio-graph-card.c > b/sound/soc/generic/audio-graph-card.c > index 1e20562..b1903f9 100644 > --- a/sound/soc/generic/audio-graph-card.c > +++ b/sound/soc/generic/audio-graph-card.c > @@ -201,8 +201,7 @@ static void graph_parse_mclk_fs(struct device_node *top, > static int graph_dai_link_of_dpcm(struct asoc_simple_priv *priv, > struct device_node *cpu_ep, > struct device_node *codec_ep, > - struct link_info *li, > - int dup_codec) > + struct link_info *li) This patch breaks DPCM connection which is used for MIXer Thank you for your help !! Best regards --- Kuninori Morimoto
Re: [PATCH v2] ASoC: soc-component: Add missed return for calling soc_component_ret
Hi Shengjiu
> Add missed return for calling soc_component_ret, otherwise the return
> value is wrong.
>
> Fixes: e2329eeba45f ("ASoC: soc-component: add soc_component_err()")
> Signed-off-by: Shengjiu Wang
> ---
Acked-by: Kuninori Morimoto
Thank you for your help !!
Best regards
---
Kuninori Morimoto
Re: [PATCH] ASoC: soc-component: Add missed return for snd_soc_pcm_component_mmap
Hi Shengjiu
> Add missed return for snd_soc_pcm_component_mmap, otherwise it always
> return -EINVAL.
>
> Fixes: e2329eeba45f ("ASoC: soc-component: add soc_component_err()")
> Signed-off-by: Shengjiu Wang
> ---
Oops, indeed.
Thank you for the patch.
But it seems these functions are also missing "return"
snd_soc_pcm_component_new()
snd_soc_pcm_component_sync_stop()
Can you please care these, too ?
Thank you for your help !!
Best regards
---
Kuninori Morimoto
Re: [PATCH v4 15/23] ASoC: soc-core: Identify 'no_pcm' DAI links for DPCM
Hi Sameer > > I wonder component->driver->non_legacy_dai_naming can't work for you ? > > I see currently in simple-card driver that, BE<->BE link would be > treated as CODEC<->CODEC link if 'non_legacy_dai_naming' flag is set > at both ends of BE. Do we need to set this flag for all BE? > However I am not sure how this will work out for a BE<->BE DPCM DAI > link considering the fact that I want to use chain of components and I > guess routing map would get complicated. Also going by the flag name > it was not meant to differentiate between a FE and BE? OK, non_legacy_dai_naming was just my quick idea. Maybe your soc_component_is_pcm() idea can work, but it seems a littl bit hackish for me. So, can you please 1) Add soc_component_is_pcm() on simple-card, not soc-core ? Maybe we can move it to soc-core later, but want to keep it under simple-card, so far. 2) Use it with data->component_chaining, and some comment ? non component_chaining user doesn't get damage in worst case, and easy to understand. /* * This is for BE<->BE connection. * It needs to ... * It is assumng ... * Note is ... */ if (data->component_chaining && !soc_component_is_pcm(cpus)) dai_link->no_pcm = 1; 3) maybe you can reuse snd_soc_find_dai() for soc_component_is_pcm() ? dai = snd_soc_find_dai(dlc); if (dai && (dai->pcm_new || dai->component->driver->pcm_construct)) return xxx Thank you for your help !! Best regards --- Kuninori Morimoto
Re: [PATCH v4 15/23] ASoC: soc-core: Identify 'no_pcm' DAI links for DPCM
Hi Sameer > > At least my CPU driver doesn't use component:pcm_construct > > but is using DAI:pcm_new for some reasons. > > I'm not sure checking DAI:pcm here is enough, or not... > > OK. If adding DAI:pcm_new above here is not sufficient, then a flag > can be used to describe FE component? or is there a better > alternative? soc_component_is_pcm() is called from simple_dai_link_of_dpcm :: "FE" side. I wonder component->driver->non_legacy_dai_naming can't work for you ? Thank you for your help !! Best regards --- Kuninori Morimoto
Re: [PATCH v4 12/23] ASoC: simple-card: Support DPCM DAI link with multiple Codecs
Hi Sameer Thank you for explaining detail at off-list mail. Your issue happen on (C) case, and you are tring to solve it. It is easy to understand if it was indicated at log area. I have imagined other system from "multiple CPU/Codec support". (A)(B) FE <-> BE (C)(D) BE <-> BE > > I'm not sure, this is just my guess. > > I'm happy if we can support it more easily :) > Right now I am trying re-use simple-card driver as much as possible > and still make it work for flexible sound cards. I will be happy to > discuss and improve the patch wherever necessary. Please help me > understand which part you think looks to be hacky. > > But, if it was difficult to keep compatibility on simple-card, > > we/you need to have new one. > Patch 17/23 and 18/23 introduce new compatible > 'simple-cc-audio-card'. Idea was to use component chaining which > allows connection of FE<->BE and multiple BE<->BE components along the > DAPM path (patch 16/23). Do you think it would be fine? This seems very complex system for current simple-card. "concept" of simple-card is simple (but "code" is not so simple...) Because of it, it doesn't assume flexible connection. Maybe your patch works for you, but might breaks other systems. Or, makes code or DT settings more complex/ununderstandable. Not sure, but my guess. Using cpu@0 node for BE is the 1st confusable point for me. Using fe/be instead of cpu/codec is easy to understand. Thank you for your help !! Best regards --- Kuninori Morimoto
Re: [PATCH v4 15/23] ASoC: soc-core: Identify 'no_pcm' DAI links for DPCM
Hi Sameer
> PCM devices are created for dai links with 'no-pcm' flag as '0'.
> Such DAI links have CPU component which implement pcm_construct()
> and pcm_destruct() callbacks. Based on this, current patch exposes
> a helper function to identify such components and populate 'no_pcm'
> flag for DPCM DAI link.
>
> This helps to have BE<->BE component links where PCM devices need
> not be created for CPU components involved in the links.
>
> Signed-off-by: Sameer Pujar
> ---
(snip)
> +bool soc_component_is_pcm(struct snd_soc_dai_link_component *dlc)
> +{
> + struct snd_soc_component *component;
> + struct snd_soc_dai *dai;
> +
> + for_each_component(component) {
> + if (!component->driver)
> + continue;
> +
> + for_each_component_dais(component, dai) {
> + if (!dai->name || !dlc->dai_name)
> + continue;
> +
> + if (strcmp(dai->name, dlc->dai_name))
> + continue;
> +
> + if (component->driver->pcm_construct)
> + return true;
> + }
> + }
> +
> + return false;
> +}
At least my CPU driver doesn't use component:pcm_construct
but is using DAI:pcm_new for some reasons.
I'm not sure checking DAI:pcm here is enough, or not...
Thank you for your help !!
Best regards
---
Kuninori Morimoto
Re: [PATCH v4 10/23] ASoC: simple-card: Wrong daifmt for CPU end of DPCM DAI link
Hi Sameer > For DPCM links I don't want to flip based on one Codec reference. My > goal was to make the binding work for multiple CPU/Codec link. Hence I > thought it would be better to explicitly describe the 'Master' DAI. We > can eventually get rid of 'codec' argument from > simple_dai_link_of_dpcm(). Yes. 'codec' argument on current simple_dai_link_of_dpcm() is not good match for multi Codec support. Thank you for your help !! Best regards --- Kuninori Morimoto
Re: [PATCH v4 11/23] ASoC: simple-card: Loop over all children for 'mclk-fs'
Hi Sameer > >>snprintf(prop, sizeof(prop), "%smclk-fs", prefix); > >>of_property_read_u32(node, prop, >mclk_fs); > >>of_property_read_u32(cpu, prop, >mclk_fs); > >> - of_property_read_u32(codec, prop, >mclk_fs); > >> + > >> + if (cpu != codec) > >> + of_property_read_u32(codec, prop, >mclk_fs); > > Maybe we want to have "cpu" in simple_dai_link_of_dpcm() side > > without using magical code in simple_parse_mclk_fs() side ? > > Are you suggesting if we should simplify simple_parse_mclk_fs() by > either passing 'cpu' or 'codec'? Oops, sorry I was misunderstand. But I still not 100% understand what do you want to do here. Maybe 50% is my English skill, but in your code (C) of_property_read_u32(cpu, prop, >mclk_fs); - of_property_read_u32(codec, prop, >mclk_fs); + + if (cpu != codec) (B) + of_property_read_u32(codec, prop, >mclk_fs); and - simple_parse_mclk_fs(top, np, codec, dai_props, prefix); (A) + simple_parse_mclk_fs(top, np, np, dai_props, prefix); Because of (A), cpu = codec = np in simple_parse_mclk_fs(). Do we have chance to call (B) ? And it still have read_u32(cpu, ...) at (C), this means all np will read mclk_fs anyway ? For me, if you don't want/need mclk_fs, don't set it on DT is the best answer, but am I misunderstanding ? Thank you for your help !! Best regards --- Kuninori Morimoto
Re: [PATCH v4 12/23] ASoC: simple-card: Support DPCM DAI link with multiple Codecs
Hi Sameer > > Maybe base issue for multiple codec support > > is that simple_for_each_link() is caring first codec only ? (snip) > Ideally I wanted to remove above two lines and allow empty codec > list. But some users may expect the parsing to fail if no 'Codec' is > found in the DAI link, hence did not remove above. If it is fine to > remove above two lines it would be simpler. The loop inside > simple_for_each_link() would anyway loop for each child node of DAI > link and simple_dai_link_of_dpcm() can parse each 'np'. Current simple-card is not assuming multi Codec, thus, we need to update it correctly, not quick-hack. I'm not sure how to do it, but it seems we need to update many functions to support it, not only simple-card driver. For example, simple-card-utils, soc-core, etc, etc... I'm not sure, this is just my guess. I'm happy if we can support it more easily :) But, if it was difficult to keep compatibility on simple-card, we/you need to have new one. Actually, I had a plan to create more flexible sound card driver, but it is not hi priority for me in these days. Thank you for your help !! Best regards --- Kuninori Morimoto
Re: [PATCH v4 10/23] ASoC: simple-card: Wrong daifmt for CPU end of DPCM DAI link
Hi Sameer
> snd_soc_runtime_set_dai_fmt() {
> ...
>
> if (cpu_dai->component->driver->non_legacy_dai_naming)
> fmt = inv_dai_fmt;
>
> ...
> }
>
> Above flips polarity for 'cpu_dai' if 'non_legacy_dai_naming' flag is set.
>
> 1. Hence example mentioned in the commit message does not work if my 'cpu_dai'
> driver does not have this flag set.
?
Do you want fo flip it ? or don't flip?
It is for Codec <-> Codec connection.
> 2. While it is true that we consider reference of 'Codec' mode for simple
> CPU<->
> Codec DAI links, for DPCM this does not seem flexible. For DPCM links CPU and
> Codec are not directly connected (CPU<->Dummy or Dummy<->Codec). Please
> consider, for example, if the DAI link has multiple CPU/Codecs. Which 'Codec'
> reference needs to be considered? Isn't it better if we explicitly mention
> which
> DAI we want to operate as 'Master'?
I think Lars-Peter has (had ?) plan for this SND_SOC_DAIFMT_CBx_CFx
flag flexibility ? Yes maybe it is needed for multi CPU/Codec system.
Thank you for your help !!
Best regards
---
Kuninori Morimoto
Re: [PATCH -next] ASoC: Documentation: fix reference to renamed source file
Hi
> From: Randy Dunlap
>
> sound/soc/soc-io.c was merged into sound/soc/soc-component.c, so fixup
> the Documentation to use the updated file name.
>
> Error: Cannot open file ../sound/soc/soc-io.c
> WARNING: kernel-doc '../scripts/kernel-doc -rst -enable-lineno
> ../sound/soc/soc-io.c' failed with return code 1
>
> Signed-off-by: Randy Dunlap
> Cc: Kuninori Morimoto
> Cc: Mark Brown
> ---
We want to have
Fixes: 460b42d162e3 ("ASoC: soc-component: merge soc-io.c into
soc-component.c")
Acked-by: Kuninori Morimoto
Thank you for your help !!
Best regards
---
Kuninori Morimoto
Re: [PATCH v4 16/23] ASoC: soc-pcm: Get all BEs along DAPM path
Hi Sameer
> dpcm_end_walk_at_be() stops the graph walk when first BE is found for
> the given FE component. In a component model we may want to connect
> multiple DAIs from different components. A new flag is introduced in
> 'snd_soc_card', which when set allows DAI/component chaining. Later
> PCM operations can be called for all these listed components for a
> valid DAPM path.
(snip)
> @@ -1069,6 +1069,7 @@ struct snd_soc_card {
> int num_of_dapm_widgets;
> const struct snd_soc_dapm_route *of_dapm_routes;
> int num_of_dapm_routes;
> + bool component_chaining;
snd_soc_card has many /* bit field */ variables.
Please use it instead of bool.
Thank you for your help !!
Best regards
---
Kuninori Morimoto
Re: [PATCH v4 15/23] ASoC: soc-core: Identify 'no_pcm' DAI links for DPCM
Hi Sameer
> PCM devices are created for dai links with 'no-pcm' flag as '0'.
> Such DAI links have CPU component which implement pcm_construct()
> and pcm_destruct() callbacks. Based on this, current patch exposes
> a helper function to identify such components and populate 'no_pcm'
> flag for DPCM DAI link.
(snip)
> +bool soc_component_is_pcm(struct snd_soc_dai_link_component *dlc)
> +{
> + struct snd_soc_component *component;
> + struct snd_soc_dai *dai;
> +
> + for_each_component(component) {
> + if (!component->driver)
> + continue;
> +
> + for_each_component_dais(component, dai) {
> + if (!dai->name || !dlc->dai_name)
> + continue;
> +
> + if (strcmp(dai->name, dlc->dai_name))
> + continue;
We can/should NULL poinster check for "dlc->dai_name" on top of
this function instead of inside loop ?
And then, we can remove "dai->name" check because next strcmp()
automatically fail if dai->name was NULL.
Thank you for your help !!
Best regards
---
Kuninori Morimoto
Re: [PATCH v4 12/23] ASoC: simple-card: Support DPCM DAI link with multiple Codecs
Hi Sameer
> The simple-card driver supports multiple CPU and single Codec entries
> for DPCM DAI links. In some cases it is required to have multiple
> CPU/Codecs. Currently parsing logic for DPCM link loops over all
> children of DAI link but assumes that there is a single Codec entry.
> When DAI link has multiple Codecs it considers only the first Codec
> entry and remaining Codecs are wrongly treated as CPU. This happens
> because first Codec is used as reference for parsing all other child
> nodes.
(snip)
> @@ -137,8 +136,13 @@ static int simple_dai_link_of_dpcm(struct
> asoc_simple_priv *priv,
>* Codec |return|Pass
>* np
>*/
> - if (li->cpu == (np == codec))
> - return 0;
> + if (li->cpu) {
> + if (!strcmp(np->name, "codec"))
> + return 0;
> + } else {
> + if (!strcmp(np->name, "cpu"))
> + return 0;
> + }
Checking node name is maybe nice idea,
but please consider "prefix" here.
Maybe base issue for multiple codec support
is that simple_for_each_link() is caring first codec only ?
simple_for_each_link(...)
{
...
do {
=> /* get codec */
=> codec = of_get_child_by_name(...);
...
}
}
Remove above and having simple_node_is_codec(np, xxx) function
or something can help it ?
Thank you for your help !!
Best regards
---
Kuninori Morimoto
Re: [PATCH v4 11/23] ASoC: simple-card: Loop over all children for 'mclk-fs'
Hi Sameer > CPU/Codec in DPCM DAI links are connected as CPU<->Dummy and Dummy<->Codec. > Though mostly CPU won't use/require 'mclk-fs' property, looping over > 'np' (current child node in a DAI link) can help in cases where multiple > Codecs are defined. This further helps to get rid of 'codec' argument > from simple_dai_link_of_dpcm() function, which gets called for DPCM links. > > Signed-off-by: Sameer Pujar > --- (snip) > diff --git a/sound/soc/generic/simple-card.c b/sound/soc/generic/simple-card.c > index 39cdc71..02d6295 100644 > --- a/sound/soc/generic/simple-card.c > +++ b/sound/soc/generic/simple-card.c > @@ -107,7 +107,9 @@ static void simple_parse_mclk_fs(struct device_node *top, > snprintf(prop, sizeof(prop), "%smclk-fs", prefix); > of_property_read_u32(node, prop, >mclk_fs); > of_property_read_u32(cpu, prop, >mclk_fs); > - of_property_read_u32(codec, prop, >mclk_fs); > + > + if (cpu != codec) > + of_property_read_u32(codec, prop, >mclk_fs); Maybe we want to have "cpu" in simple_dai_link_of_dpcm() side without using magical code in simple_parse_mclk_fs() side ? Thank you for your help !! Best regards --- Kuninori Morimoto
Re: [PATCH v4 10/23] ASoC: simple-card: Wrong daifmt for CPU end of DPCM DAI link
Hi Sameer
> simple-audio-card,dai-link@xxx {
> format = "i2s";
> bitclock-master=<>;
> frame-master=<>;
>
> cpu1: cpu@0 {
> ...
> };
>
> codec@0 {
> ...
> };
>
> ...
> };
>
> In above case CPU is expected to be configured as a master and Codec as
> a slave device. But both CPU/Codec are being configured as slave devices.
> This happens because asoc_simple_parse_daifmt() uses Codec reference and
> sets up the 'dai_link->dai_fmt' accordingly while parsing both CPU and
> Codec.
I'm sorry but I don't 100% understand about this case...
asoc_simple_parse_daifmt() should work in this case
The reason why it needs codec node is that
SND_SOC_DAIFMT_CBx_CFx are "Codec" base Master/Slave.
Thank you for your help !!
Best regards
---
Kuninori Morimoto
Re: [PATCH v4 08/23] ASoC: soc-core: Fix component name_prefix parsing
Hi Sameer
Thank you for your patch
# I guess there was ML registering magic until v3 ?
# This is 1st time for me to get this patch series...
> The "prefix" can be defined in DAI link node or it can be specified as
> part of the component node itself. Currently "sound-name-prefix" defined
> in a component is not taking effect. Actually the property is not getting
> parsed. It can be fixed by parsing "sound-name-prefix" property whenever
> "prefix" is missing in DAI link Codec node.
>
> Signed-off-by: Sameer Pujar
(snip)
> @@ -,8 +,10 @@ static void soc_set_name_prefix(struct snd_soc_card
> *card,
> struct snd_soc_codec_conf *map = >codec_conf[i];
>
> if (snd_soc_is_matching_component(>dlc, component)) {
> - component->name_prefix = map->name_prefix;
> - return;
> + if (map->name_prefix) {
> + component->name_prefix = map->name_prefix;
> + return;
> + }
> }
> }
This is nit-pick but it can be like this ?
if (snd_soc_is_matching_component(>dlc, component) &&
map->name_prefix) {
...
}
Thank you for your help !!
Best regards
---
Kuninori Morimoto
[PATCH] ASoC: dt-bindings: ak4642: switch to yaml base Documentation
From: Kuninori Morimoto
This patch switches from .txt base to .yaml base Document.
Signed-off-by: Kuninori Morimoto
---
.../devicetree/bindings/sound/ak4642.txt | 37
.../devicetree/bindings/sound/ak4642.yaml | 57 +++
2 files changed, 57 insertions(+), 37 deletions(-)
delete mode 100644 Documentation/devicetree/bindings/sound/ak4642.txt
create mode 100644 Documentation/devicetree/bindings/sound/ak4642.yaml
diff --git a/Documentation/devicetree/bindings/sound/ak4642.txt
b/Documentation/devicetree/bindings/sound/ak4642.txt
deleted file mode 100644
index 58e48ee97175..
--- a/Documentation/devicetree/bindings/sound/ak4642.txt
+++ /dev/null
@@ -1,37 +0,0 @@
-AK4642 I2C transmitter
-
-This device supports I2C mode only.
-
-Required properties:
-
- - compatible : "asahi-kasei,ak4642" or "asahi-kasei,ak4643" or
"asahi-kasei,ak4648"
- - reg : The chip select number on the I2C bus
-
-Optional properties:
-
- - #clock-cells : common clock binding; shall be set to 0
- - clocks : common clock binding; MCKI clock
- - clock-frequency : common clock binding; frequency of MCKO
- - clock-output-names : common clock binding; MCKO clock name
-
-Example 1:
-
- {
- ak4648: ak4648@12 {
- compatible = "asahi-kasei,ak4642";
- reg = <0x12>;
- };
-};
-
-Example 2:
-
- {
- ak4643: codec@12 {
- compatible = "asahi-kasei,ak4643";
- reg = <0x12>;
- #clock-cells = <0>;
- clocks = <_clock>;
- clock-frequency = <12288000>;
- clock-output-names = "ak4643_mcko";
- };
-};
diff --git a/Documentation/devicetree/bindings/sound/ak4642.yaml
b/Documentation/devicetree/bindings/sound/ak4642.yaml
new file mode 100644
index ..c8ffc86fd2d3
--- /dev/null
+++ b/Documentation/devicetree/bindings/sound/ak4642.yaml
@@ -0,0 +1,57 @@
+# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/sound/ak4642.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: AK4642 I2C transmitter Device Tree Bindings
+
+maintainers:
+ - Kuninori Morimoto
+
+properties:
+ compatible:
+enum:
+ - asahi-kasei,ak4642
+ - asahi-kasei,ak4643
+ - asahi-kasei,ak4648
+
+ reg:
+maxItems: 1
+
+ "#clock-cells":
+const: 0
+
+ "#sound-dai-cells":
+const: 0
+
+ clocks:
+maxItems: 1
+
+ clock-frequency:
+description: frequency of MCKO
+
+ clock-output-names:
+maxItems: 1
+
+required:
+ - compatible
+ - reg
+
+additionalProperties: false
+
+examples:
+ - |
+i2c {
+#address-cells = <1>;
+#size-cells = <0>;
+ak4643: codec@12 {
+compatible = "asahi-kasei,ak4643";
+#sound-dai-cells = <0>;
+reg = <0x12>;
+#clock-cells = <0>;
+clocks = <_clock>;
+clock-frequency = <12288000>;
+clock-output-names = "ak4643_mcko";
+};
+};
--
2.25.1
[PATCH] ASoC: dt-bindings: ak4613: switch to yaml base Documentation
From: Kuninori Morimoto
This patch switches from .txt base to .yaml base Document.
Signed-off-by: Kuninori Morimoto
---
v1 -> v2
- use patternProperties
.../devicetree/bindings/sound/ak4613.txt | 27 --
.../devicetree/bindings/sound/ak4613.yaml | 49 +++
2 files changed, 49 insertions(+), 27 deletions(-)
delete mode 100644 Documentation/devicetree/bindings/sound/ak4613.txt
create mode 100644 Documentation/devicetree/bindings/sound/ak4613.yaml
diff --git a/Documentation/devicetree/bindings/sound/ak4613.txt
b/Documentation/devicetree/bindings/sound/ak4613.txt
deleted file mode 100644
index 49a2e74fd9cb..
--- a/Documentation/devicetree/bindings/sound/ak4613.txt
+++ /dev/null
@@ -1,27 +0,0 @@
-AK4613 I2C transmitter
-
-This device supports I2C mode only.
-
-Required properties:
-
-- compatible : "asahi-kasei,ak4613"
-- reg : The chip select number on the I2C bus
-
-Optional properties:
-- asahi-kasei,in1-single-end : Boolean. Indicate input / output pins are
single-ended.
-- asahi-kasei,in2-single-end rather than differential.
-- asahi-kasei,out1-single-end
-- asahi-kasei,out2-single-end
-- asahi-kasei,out3-single-end
-- asahi-kasei,out4-single-end
-- asahi-kasei,out5-single-end
-- asahi-kasei,out6-single-end
-
-Example:
-
- {
- ak4613: ak4613@10 {
- compatible = "asahi-kasei,ak4613";
- reg = <0x10>;
- };
-};
diff --git a/Documentation/devicetree/bindings/sound/ak4613.yaml
b/Documentation/devicetree/bindings/sound/ak4613.yaml
new file mode 100644
index ..eb915377bc5d
--- /dev/null
+++ b/Documentation/devicetree/bindings/sound/ak4613.yaml
@@ -0,0 +1,49 @@
+# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/sound/ak4613.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: AK4613 I2C transmitter Device Tree Bindings
+
+maintainers:
+ - Kuninori Morimoto
+
+properties:
+ compatible:
+const: asahi-kasei,ak4613
+
+ reg:
+maxItems: 1
+
+ clocks:
+maxItems: 1
+
+ "#sound-dai-cells":
+const: 0
+
+patternProperties:
+ "^asahi-kasei,in[1-2]-single-end$":
+description: Input Pin 1 - 2.
+$ref: /schemas/types.yaml#/definitions/flag
+
+ "^asahi-kasei,out[1-6]-single-end$":
+description: Output Pin 1 - 6.
+$ref: /schemas/types.yaml#/definitions/flag
+
+required:
+ - compatible
+ - reg
+
+additionalProperties: false
+
+examples:
+ - |
+i2c {
+#address-cells = <1>;
+#size-cells = <0>;
+ak4613: ak4613@10 {
+compatible = "asahi-kasei,ak4613";
+reg = <0x10>;
+};
+};
--
2.25.1
[PATCH] ASoC: dt-bindings: renesas,fsi: use patternProperties for FSI-A/B
From: Kuninori Morimoto FSI has FSI-A and FSI-B, and has fsia-xxx/fsib-xxx properties. This patch uses patternProperties, and reduce verbose settings. Signed-off-by: Kuninori Morimoto --- .../bindings/sound/renesas,fsi.yaml | 19 --- 1 file changed, 4 insertions(+), 15 deletions(-) diff --git a/Documentation/devicetree/bindings/sound/renesas,fsi.yaml b/Documentation/devicetree/bindings/sound/renesas,fsi.yaml index 8a4406be387a..0dd3f7361399 100644 --- a/Documentation/devicetree/bindings/sound/renesas,fsi.yaml +++ b/Documentation/devicetree/bindings/sound/renesas,fsi.yaml @@ -43,30 +43,19 @@ properties: '#sound-dai-cells': const: 1 - fsia,spdif-connection: +patternProperties: + "^fsi(a|b),spdif-connection$": $ref: /schemas/types.yaml#/definitions/flag description: FSI is connected by S/PDIF - fsia,stream-mode-support: + "^fsi(a|b),stream-mode-support$": $ref: /schemas/types.yaml#/definitions/flag description: FSI supports 16bit stream mode - fsia,use-internal-clock: + "^fsi(a|b),use-internal-clock$": $ref: /schemas/types.yaml#/definitions/flag description: FSI uses internal clock when master mode - fsib,spdif-connection: -$ref: /schemas/types.yaml#/definitions/flag -description: same as fsia - - fsib,stream-mode-support: -$ref: /schemas/types.yaml#/definitions/flag -description: same as fsia - - fsib,use-internal-clock: -$ref: /schemas/types.yaml#/definitions/flag -description: same as fsia - required: - compatible - reg -- 2.25.1
ARM: dts: motorola-mapphone-common: remove unneeded "simple-graph-card"
From: Kuninori Morimoto
Audio Graph Card is using "audio-graph-card" prefix instead of
"simple-graph-card", and moreover "widgets / routing" doesn't need it.
This patch removes unsupported "simple-graph-card" prefix from
motorola-mapphone-common.dtsi and vendor-prefixes.yaml.
Signed-off-by: Kuninori Morimoto
---
Documentation/devicetree/bindings/vendor-prefixes.yaml | 2 +-
arch/arm/boot/dts/motorola-mapphone-common.dtsi| 4 ++--
2 files changed, 3 insertions(+), 3 deletions(-)
diff --git a/Documentation/devicetree/bindings/vendor-prefixes.yaml
b/Documentation/devicetree/bindings/vendor-prefixes.yaml
index 9aeab66be85f..147afcfe81fe 100644
--- a/Documentation/devicetree/bindings/vendor-prefixes.yaml
+++ b/Documentation/devicetree/bindings/vendor-prefixes.yaml
@@ -20,7 +20,7 @@ patternProperties:
"^(keypad|m25p|max8952|max8997|max8998|mpmc),.*": true
"^(pinctrl-single|#pinctrl-single|PowerPC),.*": true
"^(pl022|pxa-mmc|rcar_sound|rotary-encoder|s5m8767|sdhci),.*": true
- "^(simple-audio-card|simple-graph-card|st-plgpio|st-spics|ts),.*": true
+ "^(simple-audio-card|st-plgpio|st-spics|ts),.*": true
# Keep list in alphabetical order.
"^abilis,.*":
diff --git a/arch/arm/boot/dts/motorola-mapphone-common.dtsi
b/arch/arm/boot/dts/motorola-mapphone-common.dtsi
index 06fbffa81636..1990239cc6af 100644
--- a/arch/arm/boot/dts/motorola-mapphone-common.dtsi
+++ b/arch/arm/boot/dts/motorola-mapphone-common.dtsi
@@ -140,13 +140,13 @@ soundcard {
compatible = "audio-graph-card";
label = "Droid 4 Audio";
- simple-graph-card,widgets =
+ widgets =
"Speaker", "Earpiece",
"Speaker", "Loudspeaker",
"Headphone", "Headphone Jack",
"Microphone", "Internal Mic";
- simple-graph-card,routing =
+ routing =
"Earpiece", "EP",
"Loudspeaker", "SPKR",
"Headphone Jack", "HSL",
--
2.25.1
Question about "xxx,yyy" style property
The Subject was "Re: [PATCH] ASoC: dt-bindings: simple-card: care missing
address #address-cells"
Hi Rob
I'm trying to create v2 of simple-card patch,
And got issue which I can't solve by myself.
I think "xxx,yyy" (= which has "," at the property name)
needs special care, but it is very un-understandable...
Now, I'm give up.
So, can I ask you 2 things about Yaml Doc "xxx,yyy" type property ?
1) reference own definitions from "xxx,yyy"
I guess "xxx,yyy" naming property needs to has "description", right ?
But, it is OK if it references "/schemas/"
--- OK --
xxx,yyy:
description: xxx
$ref: /schemas/types.yaml#/definitions/phandle-array
-
but, will be error if it references own definitions
--- NG --
xxx,yyy:
description: xxx
$ref: "#/definitions/mydef"
-
This is the related error
-- error(?) --
xxx.yaml: properties:xxx,yyy:\
$ref: '#/definitions/mydef' does not match
'types.yaml#[/]{0,1}definitions/.*'
--
# but, there is no problem if it was defined as "patternProperties"
Q. The "xxx,yyy" property can't references own definitions,
or needs some magical extra settings ??
2) phandle for "xxx,yyy"
I noticed that it seems "xxx,yyy" property can't be referenced.
Here, "xxx,yyy" has "type: object" and "additionalProperties: false"
(below didn't happen if it doesn't have "additionalProperties: false")
If "xxx,yyy" has phandle, but not referenced,
This is not a problem.
--- OK ---
...
foo = <>;
...
xxx_yyy: xxx,yyy {
...
};
--
But will be error if it is referenced.
--- NG ---
foo = <_yyy>;
...
xxx_yyy: xxx,yyy {
...
};
The error is
-- error ---
xxx.yaml: xxx.yyy: \
Additional properties are not allowed ('phandle' was unexpected)
Q. The "xxx,yyy" needs magical settings to be referenced, or can't be ?
Re: [PATCH 2/2] ARM: dts: r8a7742: Add audio support
Hi > > Add sound support for the RZ/G1H SoC (a.k.a. R8A7742). > > > > This work is based on similar work done on the R8A7744 SoC. > > > > Signed-off-by: Lad Prabhakar > > Reviewed-by: Marian-Cristian Rotariu > > > > Reviewed-by: Geert Uytterhoeven > i.e. will queue in renesas-devel for v5.9. Acked-by: Kuninori Morimoto Thank you for your help !! Best regards --- Kuninori Morimoto
[PATCH] ASoC: dt-bindings: simple-card: care missing address #address-cells
From: Kuninori Morimoto
Current simple-card will get below error,
because it doesn't care about #address-cells at some part.
DTC
Documentation/devicetree/bindings/sound/simple-card.example.dt.yaml
Documentation/devicetree/bindings/sound/simple-card.example.dts:171.46-173.15: \
Warning (unit_address_vs_reg):
/example-4/sound/simple-audio-card,cpu@0: \
node has a unit name, but no reg or ranges property
Documentation/devicetree/bindings/sound/simple-card.example.dts:175.37-177.15: \
Warning (unit_address_vs_reg):
/example-4/sound/simple-audio-card,cpu@1: \
node has a unit name, but no reg or ranges property
...
This patch fixup this issue.
Signed-off-by: Kuninori Morimoto
---
.../bindings/sound/simple-card.yaml | 25 ++-
1 file changed, 24 insertions(+), 1 deletion(-)
diff --git a/Documentation/devicetree/bindings/sound/simple-card.yaml
b/Documentation/devicetree/bindings/sound/simple-card.yaml
index cb2bb5fac0e1..6c4c2c6d6d3c 100644
--- a/Documentation/devicetree/bindings/sound/simple-card.yaml
+++ b/Documentation/devicetree/bindings/sound/simple-card.yaml
@@ -208,6 +208,11 @@ patternProperties:
reg:
maxItems: 1
+ "#address-cells":
+const: 1
+ "#size-cells":
+const: 0
+
# common properties
frame-master:
$ref: "#/definitions/frame-master"
@@ -288,7 +293,6 @@ examples:
#address-cells = <1>;
#size-cells = <0>;
-
simple-audio-card,dai-link@0 { /* I2S - HDMI */
reg = <0>;
format = "i2s";
@@ -392,11 +396,15 @@ examples:
simple-audio-card,routing = "ak4642 Playback", "DAI0 Playback",
"ak4642 Playback", "DAI1 Playback";
+#address-cells = <1>;
+#size-cells = <0>;
dpcmcpu: simple-audio-card,cpu@0 {
+reg = <0>;
sound-dai = <_sound 0>;
};
simple-audio-card,cpu@1 {
+reg = <1>;
sound-dai = <_sound 1>;
};
@@ -427,7 +435,12 @@ examples:
"pcm3168a Playback", "DAI3 Playback",
"pcm3168a Playback", "DAI4 Playback";
+#address-cells = <1>;
+#size-cells = <0>;
+
simple-audio-card,dai-link@0 {
+reg = <0>;
+
format = "left_j";
bitclock-master = <>;
frame-master = <>;
@@ -441,22 +454,30 @@ examples:
};
simple-audio-card,dai-link@1 {
+reg = <1>;
+
format = "i2s";
bitclock-master = <>;
frame-master = <>;
convert-channels = <8>; /* TDM Split */
+#address-cells = <1>;
+#size-cells = <0>;
sndcpu1: cpu@0 {
+reg = <0>;
sound-dai = <_sound 1>;
};
cpu@1 {
+reg = <1>;
sound-dai = <_sound 2>;
};
cpu@2 {
+reg = <2>;
sound-dai = <_sound 3>;
};
cpu@3 {
+reg = <3>;
sound-dai = <_sound 4>;
};
codec {
@@ -468,6 +489,8 @@ examples:
};
simple-audio-card,dai-link@2 {
+reg = <2>;
+
format = "i2s";
bitclock-master = <>;
frame-master = <>;
--
2.17.1
Re: [PATCH] ASoC: rsnd: add interrupt support for SSI BUSIF buffer
Hi Yongbo
Thank you for the patch
> SSI BUSIF buffer is possible to overflow or underflow, especially in a
> hypervisor environment. If there is no interrupt support, it will eventually
> lead to errors in pcm data.
> This patch adds overflow and underflow interrupt support for SSI BUSIF buffer.
>
> Cc: Kuninori Morimoto
> Reported-by: Chen Li
> Signed-off-by: Yongbo Zhang
> Tested-by: Chen Li
> ---
(snip)
> @@ -635,6 +713,19 @@ static int rsnd_ssi_irq(struct rsnd_mod *mod,
> if (enable)
> val = rsnd_ssi_is_dma_mode(mod) ? 0x0e00 : 0x0f00;
>
> + if (is_tdm || is_tdm_split) {
> + switch (id) {
> + case 0:
> + case 1:
> + case 2:
> + case 3:
> + case 4:
> + case 9:
> + val |= 0xff00;
> + break;
> + }
> + }
This is small things, but we like to have
- val |= 0xff00;
+ val |= 0xff00;
Except above
Acked-by: Kuninori Morimoto
Thank you for your help !!
Best regards
---
Kuninori Morimoto
Re: [PATCH] ASoC: rsnd: dma: set bus width to data width for monaural data
Hi > According to R-Car3 HW manual 40.3.3 (Data Format on Audio Local Bus), > in case of monaural data writing or reading through Audio-DMAC, > it's always in Left Justified format, so both src and dst > DMA Bus width should be equal to physical data width. > > Therefore set src and dst's DMA bus width to: > - [monaural case] data width > - [non-monaural case] 32bits (as prior applying the patch) > > Cc: Andrew Gabbasov > Cc: Timo Wischer > Signed-off-by: Jiada Wang > Signed-off-by: Eugeniu Rosca > --- I don't know how Monaural works, but I know you are using it. Thus don't say Acked-by and/or Reviewed-by, but I have no objection about this patch. Thank you for your help !! Best regards --- Kuninori Morimoto
Re: [RESEND PATCH] ASoC: rsnd: dma: fix SSI9 4/5/6/7 busif dma address
Hi
> From: Jiada Wang
>
> Currently each SSI unit's busif dma address is calculated by
> following calculation formula:
> 0xec54 + 0x1000 * id + busif / 4 * 0xA000 + busif % 4 * 0x400
>
> But according to R-Car3 HW manual 41.1.4 Register Configuration,
> ssi9 4/5/6/7 busif data register address
> (SSI9_4_BUSIF/SSI9_5_BUSIF/SSI9_6_BUSIF/SSI9_7_BUSIF)
> are out of this rule.
>
> This patch updates the calculation formula to correct
> ssi9 4/5/6/7 busif data register address.
>
> Fixes: 5e45a6fab3b9 ("ASoc: rsnd: dma: Calculate dma address with consider of
> BUSIF")
> Signed-off-by: Jiada Wang
> Signed-off-by: Timo Wischer
> [erosca: minor improvements in commit description]
> Cc: Andrew Gabbasov
> Cc: sta...@vger.kernel.org # v4.20+
> Signed-off-by: Eugeniu Rosca
> ---
Acked-by: Kuninori Morimoto
Re: [PATCH] ASoC: rsnd: Reinitialize bit clock inversion flag for every format setting
Hi
> Unlike other format-related DAI parameters, rdai->bit_clk_inv flag
> is not properly re-initialized when setting format for new stream
> processing. The inversion, if requested, is then applied not to default,
> but to a previous value, which leads to SCKP bit in SSICR register being
> set incorrectly.
> Fix this by re-setting the flag to its initial value, determined by format.
>
> Fixes: 1a7889ca8aba3 ("ASoC: rsnd: fixup SND_SOC_DAIFMT_xB_xF behavior")
> Cc: Andrew Gabbasov
> Cc: Jiada Wang
> Cc: Timo Wischer
> Cc: sta...@vger.kernel.org # v3.17+
> Signed-off-by: Junya Monden
> Signed-off-by: Eugeniu Rosca
> ---
Acked-by: Kuninori Morimoto
Re: [alsa-devel] [RFC PATCH 2/2] ASoC: simple-card: Add documentation for force-dpcm property
Hi > > > > DPCM is an implementation detail of Linux (and one that we want to phase > > > > out going forwards too), we shouldn't be putting it in the DT bindings > > > > where it becomes an ABI. > > > > > I see your point. This is way I marked the patch series as RFC. I need to > > > find > > > another way to reuse simple-card as machine driver for SOF. > > > > Have a look at the way the Renesas systems are using this and the audio > > graph card - they have DPCM. > > Indeed we (= Renesas) are using this as DPCM, but unfortunately > it is not upstreamed. Using local patch. I mean DT part. Thank you for your help !! Best regards --- Kuninori Morimoto
Re: [alsa-devel] [RFC PATCH 2/2] ASoC: simple-card: Add documentation for force-dpcm property
Hi > > > DPCM is an implementation detail of Linux (and one that we want to phase > > > out going forwards too), we shouldn't be putting it in the DT bindings > > > where it becomes an ABI. > > > I see your point. This is way I marked the patch series as RFC. I need to > > find > > another way to reuse simple-card as machine driver for SOF. > > Have a look at the way the Renesas systems are using this and the audio > graph card - they have DPCM. Indeed we (= Renesas) are using this as DPCM, but unfortunately it is not upstreamed. Using local patch. Thank you for your help !! Best regards --- Kuninori Morimoto
Re: [alsa-devel] [PATCH 2/2] ASoC: simple_card_utils.h: Fix potential multiple redefinition error
Hi
> asoc_simple_debug_info and asoc_simple_debug_dai must be static
> otherwise we might a compilation error if the compiler decides
> not to inline the given function.
>
> Fixes: 0580dde59438686d ("ASoC: simple-card-utils: add
> asoc_simple_debug_info()")
> Signed-off-by: Daniel Baluta
> ---
Acked-by: Kuninori Morimoto
Thank you for your help !!
Best regards
---
Kuninori Morimoto
Re: [PATCH] SoC: simple-card-utils: set 0Hz to sysclk when shutdown
Hi Katsuhiro > >> Bad scenario as follows (mclk-fs = 256): > >> - Initialize sysclk by correct value (Ex. 12.288MHz) > >> - Codec set constraints of PCM rate by sysclk > >> 48kHz (1/256), 32kHz (1/384), 24kHz (1/512) > >> - Play 48kHz sound, it's acceptable > >> - Sysclk is not changed > >> > >> - Play 32kHz sound, it's acceptable > >> - Set sysclk to 8.192MHz (= fs * mclk-fs = 32k * 256) > >> - Codec set constraints of PCM rate by sysclk > >> 32kHz (1/256), 21.33kHz (1/384), 16kHz (1/512) > >> > >> - Play 48kHz again, but it's NOT acceptable because constraints > >> do not allow 48kHz (snip) > Ah, sorry for confusing. It's not either. hw_params() of machine > driver has been called even if constraints don't have a requested > PCM rate. But it's not expected. > > For example, if constraints are 32k, 21.33k, 16k, hw_params() will > be called with 32k when an user requests to play 48k sounds. Oh, I see. Thank you for explaining. Thank you for your help !! Best regards --- Kuninori Morimoto
Re: [PATCH] SoC: simple-card-utils: set 0Hz to sysclk when shutdown
Hi Katsuhiro > This patch set 0Hz to sysclk when shutdown the card. > > Some codecs set rate constraints that derives from sysclk. This > mechanism works correctly if machine drivers give fixed frequency. > > But simple-audio and audio-graph card set variable clock rate if > 'mclk-fs' property exists. In this case, rate constraints will go > bad scenario. For example a codec accepts three limited rates > (mclk / 256, mclk / 384, mclk / 512). > > Bad scenario as follows (mclk-fs = 256): >- Initialize sysclk by correct value (Ex. 12.288MHz) > - Codec set constraints of PCM rate by sysclk >48kHz (1/256), 32kHz (1/384), 24kHz (1/512) >- Play 48kHz sound, it's acceptable >- Sysclk is not changed > >- Play 32kHz sound, it's acceptable >- Set sysclk to 8.192MHz (= fs * mclk-fs = 32k * 256) > - Codec set constraints of PCM rate by sysclk >32kHz (1/256), 21.33kHz (1/384), 16kHz (1/512) > >- Play 48kHz again, but it's NOT acceptable because constraints > do not allow 48kHz > > So codecs treat 0Hz sysclk as signal of applying no constraints to > avoid this problem. > > Signed-off-by: Katsuhiro Suzuki > --- I'm not 100% understand your issue. .hw_params (= set mclk/sysclk) is not called in bad case ?? Or it is called but Codec driver ignores it somehow ?? Thank you for your help !! Best regards --- Kuninori Morimoto
Re: [PATCH v1 2/3] ASoC: rsnd: Allow reconfiguration of clock rate
Hi Jiada
> > 2nd, can we keep usrcnt setup as-is ?
> > I guess we can just avoid rsnd_ssi_master_clk_start() if ssi->rate was not
> > 0 ?
>
> I don't fully understand your 2nd question,
> in case of rsnd_ssi_master_clk_stop(), if avoid
> rsnd_ssi_master_clk_stop() when ssi->rate is 0 by apply following
> change
>
> static void rsnd_ssi_master_clk_stop(struct rsnd_mod *mod,
>struct rsnd_dai_stream *io)
> {
> ...
> -if (ssi->usrcnt > 1)
> +if (ssi->rate == 0)
> return;
> ...
> }
>
> then when any IO stream with same SSI calls .hw_free,
> the other IO stream's clock will be stopped too.
I think we can find more simple solution if we can find good ideas.
For example, how about to add new counter for hw_params/hw_free ?
Anyway, [3/3] patch is too much over-kill to me.
And, please don't exchange usrcnt inc/dec position at [2/3].
It is for open/close.
Thank you for your help !!
Best regards
---
Kuninori Morimoto
Re: [PATCH v1 2/3] ASoC: rsnd: Allow reconfiguration of clock rate
Hi Jiada, again
> The solution looks very over-kill to me,
> especiallyq [3/3] patch is too much to me.
>
> 1st, can we start clock at .hw_param, instead of .prepare ?
> and stop it at .hw_free ?
>
> 2nd, can we keep usrcnt setup as-is ?
> I guess we can just avoid rsnd_ssi_master_clk_start() if ssi->rate was not 0 ?
> similar for rsnd_ssi_master_clk_stop()
In my quick check, I used your [1/3] patch and below.
It works for me, but I don't know detail of your test case.
--
diff --git a/sound/soc/sh/rcar/ssi.c b/sound/soc/sh/rcar/ssi.c
index f6a7466..f26ffd1 100644
--- a/sound/soc/sh/rcar/ssi.c
+++ b/sound/soc/sh/rcar/ssi.c
@@ -286,19 +286,8 @@ static int rsnd_ssi_master_clk_start(struct rsnd_mod *mod,
if (rsnd_ssi_is_multi_slave(mod, io))
return 0;
- if (ssi->usrcnt > 0) {
- if (ssi->rate != rate) {
- dev_err(dev, "SSI parent/child should use same rate\n");
- return -EINVAL;
- }
-
- if (ssi->chan != chan) {
- dev_err(dev, "SSI parent/child should use same chan\n");
- return -EINVAL;
- }
-
+ if (ssi->rate)
return 0;
- }
if (rsnd_runtime_is_tdm_split(io))
chan = rsnd_io_converted_chan(io);
@@ -349,7 +338,7 @@ static void rsnd_ssi_master_clk_stop(struct rsnd_mod *mod,
if (!rsnd_ssi_can_output_clk(mod))
return;
- if (ssi->usrcnt > 1)
+ if (!ssi->rate)
return;
ssi->cr_clk = 0;
@@ -539,6 +528,17 @@ static int rsnd_ssi_hw_params(struct rsnd_mod *mod,
return 0;
}
+static int rsnd_ssi_hw_free(struct rsnd_mod *mod, struct rsnd_dai_stream *io,
+ struct snd_pcm_substream *substream)
+{
+ if (!rsnd_ssi_is_run_mods(mod, io))
+ return 0;
+
+ rsnd_ssi_master_clk_stop(mod, io);
+
+ return 0;
+}
+
static int rsnd_ssi_start(struct rsnd_mod *mod,
struct rsnd_dai_stream *io,
struct rsnd_priv *priv)
@@ -925,6 +925,7 @@ static struct rsnd_mod_ops rsnd_ssi_pio_ops = {
.pointer= rsnd_ssi_pio_pointer,
.pcm_new= rsnd_ssi_pcm_new,
.hw_params = rsnd_ssi_hw_params,
+ .hw_free= rsnd_ssi_hw_free,
.prepare= rsnd_ssi_prepare,
.get_status = rsnd_ssi_get_status,
};
@@ -1012,6 +1013,7 @@ static struct rsnd_mod_ops rsnd_ssi_dma_ops = {
.pcm_new= rsnd_ssi_pcm_new,
.fallback = rsnd_ssi_fallback,
.hw_params = rsnd_ssi_hw_params,
+ .hw_free= rsnd_ssi_hw_free,
.prepare= rsnd_ssi_prepare,
.get_status = rsnd_ssi_get_status,
};
--
Re: [PATCH v1 2/3] ASoC: rsnd: Allow reconfiguration of clock rate
Hi Jiada
The solution looks very over-kill to me,
especiallyq [3/3] patch is too much to me.
1st, can we start clock at .hw_param, instead of .prepare ?
and stop it at .hw_free ?
2nd, can we keep usrcnt setup as-is ?
I guess we can just avoid rsnd_ssi_master_clk_start() if ssi->rate was not 0 ?
similar for rsnd_ssi_master_clk_stop()
static int rsnd_ssi_master_clk_start(struct rsnd_mod *mod,
struct rsnd_dai_stream *io)
{
...
if (ssi->rate)
return 0;
...
}
static void rsnd_ssi_master_clk_stop(struct rsnd_mod *mod,
struct rsnd_dai_stream *io)
{
...
- if (ssi->usrcnt > 1)
+ if (ssi->rate == 0)
return;
...
}
> From: Timo Wischer
>
> Currently after clock rate is started in .prepare reconfiguration
> of clock rate is not allowed, unless the stream is stopped.
>
> But there is use case in Gstreamer ALSA sink, in case of changed caps
> Gsreatmer reconfigs and it calls snd_pcm_hw_free() before snd_pcm_prepre().
> See gstreamer1.0-plugins-base/
> gst-libs/gst/audio/gstaudiobasesink.c: gst_audio_base_sink_setcaps():
> - gst_audio_ring_buffer_release()
> - gst_audio_sink_ring_buffer_release()
> - gst_alsasink_unprepare()
> - snd_pcm_hw_free()
> is called before
> - gst_audio_ring_buffer_acquire()
> - gst_audio_sink_ring_buffer_acquire()
> - gst_alsasink_prepare()
> - set_hwparams()
> - snd_pcm_hw_params()
> - snd_pcm_prepare()
>
> The issue mentioned in this commit can be reproduced with the following
> aplay patch:
>
> >diff --git a/aplay/aplay.c b/aplay/aplay.c
> >@@ -2760,6 +2760,8 @@ static void playback_go(int fd, size_t loaded,
> > header(rtype, name);
> > set_params();
> >+ hwparams.rate = (hwparams.rate == 48000) ? 44100 : 48000;
> >+ set_params();
> >
> > while (loaded > chunk_bytes && written < count && !in_aborting)
> > {
> > if (pcm_write(audiobuf + written, chunk_size) <= 0)
>
> $ aplay -Dplughw:0,0,0 -c8 -fS24_LE -r48000 /dev/zero
> rcar_sound ec50.sound: SSI parent/child should use same rate
> rcar_sound ec50.sound: ssi[3] : prepare error -22
> rcar_sound ec50.sound: ASoC: cpu DAI prepare error: -22
> rsnd_link0: ASoC: prepare FE rsnd_link0 failed
>
> this patch address the issue by stop clock in .hw_free,
> to allow reconfiguration of clock rate without stop of the stream.
>
> Signed-off-by: Timo Wischer
> Signed-off-by: Jiada Wang
> ---
> sound/soc/sh/rcar/ssi.c | 53 +
> 1 file changed, 38 insertions(+), 15 deletions(-)
>
> diff --git a/sound/soc/sh/rcar/ssi.c b/sound/soc/sh/rcar/ssi.c
> index f6a7466622ea..f43937d2c588 100644
> --- a/sound/soc/sh/rcar/ssi.c
> +++ b/sound/soc/sh/rcar/ssi.c
> @@ -286,7 +286,7 @@ static int rsnd_ssi_master_clk_start(struct rsnd_mod *mod,
> if (rsnd_ssi_is_multi_slave(mod, io))
> return 0;
>
> - if (ssi->usrcnt > 0) {
> + if (ssi->rate) {
> if (ssi->rate != rate) {
> dev_err(dev, "SSI parent/child should use same rate\n");
> return -EINVAL;
> @@ -471,13 +471,9 @@ static int rsnd_ssi_init(struct rsnd_mod *mod,
>struct rsnd_dai_stream *io,
>struct rsnd_priv *priv)
> {
> - struct rsnd_ssi *ssi = rsnd_mod_to_ssi(mod);
> -
> if (!rsnd_ssi_is_run_mods(mod, io))
> return 0;
>
> - ssi->usrcnt++;
> -
> rsnd_mod_power_on(mod);
>
> rsnd_ssi_config_init(mod, io);
> @@ -505,18 +501,8 @@ static int rsnd_ssi_quit(struct rsnd_mod *mod,
> return -EIO;
> }
>
> - rsnd_ssi_master_clk_stop(mod, io);
> -
> rsnd_mod_power_off(mod);
>
> - ssi->usrcnt--;
> -
> - if (!ssi->usrcnt) {
> - ssi->cr_own = 0;
> - ssi->cr_mode= 0;
> - ssi->wsr= 0;
> - }
> -
> return 0;
> }
>
> @@ -525,6 +511,7 @@ static int rsnd_ssi_hw_params(struct rsnd_mod *mod,
> struct snd_pcm_substream *substream,
> struct snd_pcm_hw_params *params)
> {
> + struct rsnd_ssi *ssi = rsnd_mod_to_ssi(mod);
> struct rsnd_dai *rdai = rsnd_io_to_rdai(io);
> unsigned int fmt_width = snd_pcm_format_width(params_format(params));
>
> @@ -536,6 +523,11 @@ static int rsnd_ssi_hw_params(struct rsnd_mod *mod,
> return -EINVAL;
> }
>
> + if (!rsnd_ssi_is_run_mods(mod, io))
> + return 0;
> +
> + ssi->usrcnt++;
> +
> return 0;
> }
>
> @@ -913,6 +905,35 @@ static int rsnd_ssi_prepare(struct rsnd_mod *mod,
> return rsnd_ssi_master_clk_start(mod, io);
> }
>
> +static int rsnd_ssi_hw_free(struct rsnd_mod *mod,
Re: [PATCH v2] ASoC: audio-graph-card: Constify reg in graph_get_dai_id
Hi Nathan
> clang errors:
>
> sound/soc/generic/audio-graph-card.c:87:7: error: assigning to 'u32 *'
> (aka 'unsigned int *') from 'const void *' discards qualifiers
> [-Werror,-Wincompatible-pointer-types-discards-qualifiers]
> reg = of_get_property(node, "reg", NULL);
> ^ ~~
> 1 error generated.
>
> Move the declaration up a bit to keep the reverse christmas tree look.
>
> Fixes: c152f8491a8d ("ASoC: audio-graph-card: fix an use-after-free in
> graph_get_dai_id()")
> Link: https://github.com/ClangBuiltLinux/linux/issues/600
> Signed-off-by: Nathan Chancellor
> ---
Maybe
ec3042ad39d4e2ddbc3a3344f90bb10d8feb53bc
("ASoC: audio-graph-card: add missing const at graph_get_dai_id()")
Thank you for your help !!
Best regards
---
Kuninori Morimoto
Re: [PATCH 0/4] Fix some use-after-free problems in sound/soc/generic
Hi Wen > We developed a coccinelle SmPL to detect sound/sooc/generic code and > found some use-after-free problems. > This patch series fixes those problems. > > Wen Yang (4): > ASoC: simple-card: fix an use-after-free in simple_dai_link_of_dpcm() > ASoC: simple-card: fix an use-after-free in simple_for_each_link() > ASoC: audio-graph-card: fix use-after-free in graph_dai_link_of_dpcm() > ASoC: audio-graph-card: fix an use-after-free in graph_get_dai_id() Actually, I was curious about these issue. Thank you for your patches. Acked-by: Kuninori Morimoto Thank you for your help !! Best regards --- Kuninori Morimoto
Re: [PATCH 2/2] ASoC: soc-core: support dai_link with platforms_num != 1
Hi Jerome
Thank you for your patch.
This is nice !
> Add support platforms_num != 1 in dai_link. Initially, the main purpose of
> this change was to make the platform optional in the dai_link, instead of
> inserting the dummy platform driver.
>
> This particular case had just been solved by Kuninori Morimoto with
> commit 1d7689892878 ("ASoC: soc-core: allow no Platform on dai_link").
>
> However, this change may still be useful for those who need multiple
> platform components on a single dai_link (it solves one of the FIXME
> note in soc-core)
>
> Cc: Kuninori Morimoto
> Signed-off-by: Jerome Brunet
> ---
(snip)
> diff --git a/sound/soc/soc-core.c b/sound/soc/soc-core.c
> index 002ddbf4e5a3..3053a4a461b3 100644
> --- a/sound/soc/soc-core.c
> +++ b/sound/soc/soc-core.c
> @@ -887,7 +887,7 @@ static int soc_bind_dai_link(struct snd_soc_card *card,
> struct snd_soc_dai_link *dai_link)
> {
> struct snd_soc_pcm_runtime *rtd;
> - struct snd_soc_dai_link_component *codecs;
> + struct snd_soc_dai_link_component *dlc;
> struct snd_soc_component *component;
> int i;
This is nitpick, but having both "codecs" and "platforms" is
easy to read / understand code.
> @@ -1051,22 +1053,22 @@ static int soc_init_dai_link(struct snd_soc_card
> *card,
>struct snd_soc_dai_link *link)
> {
> int i;
> - struct snd_soc_dai_link_component *codec;
> + struct snd_soc_dai_link_component *dlc;
Same here.
For other parts
Acked-by: Kuninori Morimoto
Thank you for your help !!
Best regards
---
Kuninori Morimoto
Re: [PATCH v2 1/1] ASoC: rsnd: fixup mod ID calculation in rsnd_ctu_probe_
Hi
> From: Nilkanth Ahirrao
>
> commit c16015f36cc1 ("ASoC: rsnd: add .get_id/.get_id_sub")
> introduces rsnd_ctu_id which calcualates and gives
> the main Device id of the CTU by dividing the id by 4.
> rsnd_mod_id uses this interface to get the CTU main
> Device id. But this commit forgets to revert the main
> Device id calcution previously done in rsnd_ctu_probe_
> which also divides the id by 4. This path corrects the
> same to get the correct main Device id.
>
> The issue is observered when rsnd_ctu_probe_ is done for CTU1
>
> Fixes: c16015f36cc1 ("ASoC: rsnd: add .get_id/.get_id_sub")
>
> Signed-off-by: Nilkanth Ahirrao
> Signed-off-by: Suresh Udipi
> Signed-off-by: Jiada Wang
> ---
Acked-by: Kuninori Morimoto
Re: [PATCH v1 1/1] ASoC: rsnd: fixup mod ID calculation in rsnd_ctu_probe_
Hi Jiada
> From: Nilkanth Ahirrao
>
> commit 7e4f3419ebfe ("ASoC: rsnd: add .get_id/.get_id_sub")
> introduces rsnd_ctu_id which calcualates and gives
> the main Device id of the CTU by dividing the id by 4.
> rsnd_mod_id uses this interface to get the CTU main
> Device id. But this commit forgets to revert the main
> Device id calcution previously done in rsnd_ctu_probe_
> which also divides the id by 4. This path corrects the
> same to get the correct main Device id.
>
> The issue is observered when rsnd_ctu_probe_ is done for CTU1
>
> Fixes: 7e4f3419ebfe ("ASoC: rsnd: add .get_id/.get_id_sub")
>
> Signed-off-by: Nilkanth Ahirrao
> Signed-off-by: Suresh Udipi
> Signed-off-by: Jiada Wang
> ---
Thanks !!
Acked-by: Kuninori Morimoto
But, I guess commit ID is this for upstream ?
c16015f36cc128244c910152663de45c3b99f551
("ASoC: rsnd: add .get_id/.get_id_sub")
> sound/soc/sh/rcar/ctu.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/sound/soc/sh/rcar/ctu.c b/sound/soc/sh/rcar/ctu.c
> index 8cb06dab234e..7647b3d4c0ba 100644
> --- a/sound/soc/sh/rcar/ctu.c
> +++ b/sound/soc/sh/rcar/ctu.c
> @@ -108,7 +108,7 @@ static int rsnd_ctu_probe_(struct rsnd_mod *mod,
> struct rsnd_dai_stream *io,
> struct rsnd_priv *priv)
> {
> - return rsnd_cmd_attach(io, rsnd_mod_id(mod) / 4);
> + return rsnd_cmd_attach(io, rsnd_mod_id(mod));
> }
>
> static void rsnd_ctu_value_init(struct rsnd_dai_stream *io,
> --
> 2.19.2
>
Re: [PATCH linux-next v2 0/2] misc fix in src.c
Hi Jiada
> two issues were found due to linux-next commit
> 7674bec4fc09 ("ASoC: rsnd: update BSDSR/BSDISR handling")
> this patch-set address these two issues
>
> ---
> v2: Update rsnd_of_match table instead of set priv->flags in probe
>
> v1: initial version
>
> Jiada Wang (2):
> ASoC: rsnd: src: Avoid a potential deadlock
> ASoC: rsnd: src: fix compiler warnings
Thank you for your patches.
For both patches
Acked-by: Kuninori Morimoto
Re: [PATCH linux-next v1 2/2] ASoC: rsnd: src: fix compiler warnings
Hi Jiada
Thank you for your patch
> This patch moves the 'static' keyword to the front of the
> declaration to fix the compiler warnings
>
> Fixes: linux-next commit 7674bec4fc09 ("ASoC: rsnd: update BSDSR/BSDISR
> handling")
> Signed-off-by: Jiada Wang
> ---
Acked-by: Kuninori Morimoto
Re: [PATCH linux-next v1 1/2] ASoC: rsnd: src: Avoid a potential deadlock
Hi Jiada
Thank you for your patch
> lockdep warns us that priv->lock and k->k_lock can cause a
> deadlock when after acquire of k->k_lock, process is interrupted
> by src, while in another routine of src .init, k->k_lock is
> acquired with priv->lock held.
>
> This patch avoids a potential deadlock by not calling soc_device_match()
> in SRC .init callback, instead it adds new soc fields in priv->flags to
> differentiate SoCs.
>
> Fixes: linux-next commit 7674bec4fc09 ("ASoC: rsnd: update BSDSR/BSDISR
> handling")
> Signed-off-by: Jiada Wang
> ---
(snip)
> @@ -1706,6 +1707,8 @@ static int rsnd_probe(struct platform_device *pdev)
>
> priv->pdev = pdev;
> priv->flags = (unsigned long)of_device_get_match_data(dev);
> + if (of_device_is_compatible(np, "renesas,rcar_sound-r8a77990"))
> + priv->flags |= RSND_SOC_E;
> spin_lock_init(>lock);
Updating rsnd_of_match is nice idea
Maybe like this (not tested)
diff --git a/sound/soc/sh/rcar/core.c b/sound/soc/sh/rcar/core.c
index 9834474..e7ebc75 100644
--- a/sound/soc/sh/rcar/core.c
+++ b/sound/soc/sh/rcar/core.c
@@ -107,9 +107,12 @@
SNDRV_PCM_FMTBIT_S24_LE)
static const struct of_device_id rsnd_of_match[] = {
+ /* General Handling */
{ .compatible = "renesas,rcar_sound-gen1", .data = (void *)RSND_GEN1 },
{ .compatible = "renesas,rcar_sound-gen2", .data = (void *)RSND_GEN2 },
{ .compatible = "renesas,rcar_sound-gen3", .data = (void *)RSND_GEN3 },
+ /* Special Handling */
+ { .compatible = "renesas,rcar_sound-r8a77990", .data = (void
*)(RSND_GEN3 | RSND_SOC_E) },
{},
};
Re: [PATCH v2] arm64: dts: renesas: r8a77965: add SSIU support for sound
Hi
> rsnd driver supports SSIU now, let's use it.
> Then, BUSIF DMA settings on rcar_sound,ssi (= rxu, txu) are
> no longer needed.
> Applies commit 8d14bfa074db ("arm64: dts: renesas: r8a7796: add SSIU
> support for sound") and commit 10bd03fa896e ("arm64: dts: renesas:
> r8a7796: remove BUSIF0 settings from rcar_sound,ssi") for r8a77965.
>
> Signed-off-by: Jiada Wang
> Signed-off-by: Timo Wischer
> ---
Acked-by: Kuninori Morimoto
Re: [PATCH v2] ASoC: rsnd: gen: fix SSI9 4/5/6/7 busif related register address
HI
> From: Jiada Wang
>
> Currently each SSI unit 's busif mode/adinr/dalign address is
> registered by: (in busif4 case)
> RSND_GEN_M_REG(SSI_BUSIF4_MODE, 0x500, 0x80)
> RSND_GEN_M_REG(SSI_BUSIF4_ADINR,0x504, 0x80)
> RSND_GEN_M_REG(SSI_BUSIF4_DALIGN, 0x508, 0x80)
>
> But according to user manual 41.1.4 Register Configuration
> ssi9 4/5/6/7 busif mode/adinr/dalign register address
> ( SSI9-[4/5/6/7]_BUSIF_[MODE/ADINR/DALIGN] )
> are out of this rule.
>
> This patch registers ssi9 4/5/6/7 mode/adinr/dalign register
> as single register, and access these registers in case of
> SSI9 BUSIF 4/5/6/7.
>
> Fixes: commit 8c9d75033340 ("ASoC: rsnd: ssiu: Support BUSIF other than
> BUSIF0")
> Signed-off-by: Jiada Wang
> Signed-off-by: Timo Wischer
> ---
Acked-by: Kuninori Morimoto
Re: [PATCH] arm64: dts: renesas: r8a77965: add SSIU support for sound
Hi Jiada
Thank you for your patch
> From: Jiada Wang
>
> rsnd driver supports SSIU now, let's use it.
> Then, BUSIF DMA settings on rcar_sound,ssi (= rxu, txu) are
> no longer needed.
> Applies commit 8d14bfa074db ("arm64: dts: renesas: r8a7796: add SSIU
> support for sound") for r8a77965.
>
> Signed-off-by: Jiada Wang
> Signed-off-by: Timo Wischer
> ---
I think ARM maintainer doesn't want duplicate/copied patches.
So, we want/need to merge into this patch, too
10bd03fa896ee5e25fa3548a3d1112b9beb76943
("arm64: dts: renesas: r8a7796: remove BUSIF0 settings from
rcar_sound,ssi")
And I guess this patch is related, so, we need to merge it too ?
arm64: dts: renesas: r8a7796: use extended audio dmac register
Re: [PATCH] arm64: dts: renesas: r8a7796: remove unneeded sound #address/size-cells
Hi
> From: Jiada Wang
>
> commit 78bc93b3ffb2 ("arm64: dts: renesas: r8a7796: Add address
> properties to rcar_sound port nodes") added missing #address-cells
> and #size-cells for sound ports.
> But, these are based on platform, not on SoC. This patch cleanups it.
>
> Signed-off-by: Jiada Wang
> Signed-off-by: Timo Wischer
> ---
I thought I had mentioned about this, but it seems no fix...
Anyway
Acked-by: Kuninori Morimoto
Re: [PATCH] ASoC: rsnd: gen: fix SSI9 4/5/6/7 busif related register address
Hi Jiada
Thank you for your patch
> Currently each SSI unit 's busif mode/adinr/dalign address is
> registered by: (in busif4 case)
> RSND_GEN_M_REG(SSI_BUSIF4_MODE, 0x500, 0x80)
> RSND_GEN_M_REG(SSI_BUSIF4_ADINR,0x504, 0x80)
> RSND_GEN_M_REG(SSI_BUSIF4_DALIGN, 0x508, 0x80)
>
> But according to user manual 41.1.4 Register Configuration
> ssi9 4/5/6/7 busif mode/adinr/dalign register address
> ( SSI9-[4/5/6/7]_BUSIF_[MODE/ADINR/DALIGN] )
> are out of this rule.
>
> This patch registers ssi9 4/5/6/7 mode/adinr/dalign register
> as single register, and access these these registers in case
> of SSI9 BUSIF 4/5/6/7.
I think
- and access these these registers
+ and access these registers
> if ((id == 9) && (busif >= 4)) {
> - struct device *dev = rsnd_priv_to_dev(priv);
> -
> - dev_err(dev, "This driver doesn't support SSI%d-%d, so
> far",
> - id, busif);
> + rsnd_mod_write(mod, SSI9_BUSIF_ADINR(busif),
> +rsnd_get_adinr_bit(mod, io) | chnl);
> + rsnd_mod_write(mod, SSI9_BUSIF_MODE(busif),
> +rsnd_get_busif_shift(io, mod) | 1);
> + rsnd_mod_write(mod, SSI9_BUSIF_DALIGN(busif),
> +rsnd_get_dalign(mod, io));
> + } else {
> + rsnd_mod_write(mod, SSI_BUSIF_ADINR(busif),
> +rsnd_get_adinr_bit(mod, io) | chnl);
> + rsnd_mod_write(mod, SSI_BUSIF_MODE(busif),
> +rsnd_get_busif_shift(io, mod) | 1);
> + rsnd_mod_write(mod, SSI_BUSIF_DALIGN(busif),
> +rsnd_get_dalign(mod, io));
> }
> -
> - rsnd_mod_write(mod, SSI_BUSIF_ADINR(busif),
> -rsnd_get_adinr_bit(mod, io) |
> -(rsnd_io_is_play(io) ?
> - rsnd_runtime_channel_after_ctu(io) :
> - rsnd_runtime_channel_original(io)));
> - rsnd_mod_write(mod, SSI_BUSIF_MODE(busif),
> -rsnd_get_busif_shift(io, mod) | 1);
> - rsnd_mod_write(mod, SSI_BUSIF_DALIGN(busif),
> -rsnd_get_dalign(mod, io));
> }
Necessary register on rsnd_mod_write() is just number today.
So how about this ? Code will be more simple/readable
if ((id == 9) && (busif >= 4)) {
adinr = SSI9_BUSIF_ADINR();
mode= SSI9_BUSIF_MODE();
daligh = SSI9_BUSIF_DALIGN();
} else {
adinr = SSI_BUSIF_ADINR();
mode= SSI_BUSIF_MODE();
daligh = SSI_BUSIF_DALIGN();
}
rsnd_mod_write(mod, adinr,
rsnd_get_adinr_bit(mod, io) | chnl);
rsnd_mod_write(mod, mode,
rsnd_get_busif_shift(io, mod) | 1);
rsnd_mod_write(mod, dalign,
rsnd_get_dalign(mod, io));
Re: [PATCH] ASoC: rsnd: dma: fix SSI9 4/5/6/7 busif dma address
Hi Jiada, again
> > Currently each SSI unit 's busif dma address is calculated by
> > following calculation formulation:
> > 0xec54 + 0x1000 * id + busif / 4 * 0xA000 + busif % 4 * 0x400
> >
> > But according to user manual 41.1.4 Register Configuration
> > ssi9 4/5/6/7 busif data register address
> > (SSI9_4_BUSIF/SSI9_5_BUSIF/SSI9_6_BUSIF/SSI9_7_BUSIF)
> > are out of this rule.
> >
> > This patch updates the calculation formulation to correct
> > ssi9 4/5/6/7 busif data register address
> >
> > Fixes: commit 5e45a6fab3b9 ("ASoc: rsnd: dma: Calculate dma address with
> > consider of BUSIF")
> > Signed-off-by: Jiada Wang
> > Signed-off-by: Timo Wischer
> > ---
>
> We don't need below anymore by this patch ?
>
> --- dma.c
> /*
>* FIXME
>*
>* We can't support SSI9-4/5/6/7, because its address is
>* out of calculation rule
>*/
> if ((id == 9) && (busif >= 4))
> dev_err(dev, "This driver doesn't support SSI%d-%d, so far",
> id, busif);
Oops, next patch is removing this.
Please ignore this mail.
Re: [PATCH] ASoC: rsnd: dma: fix SSI9 4/5/6/7 busif dma address
Hi Jiada
Thank you for your patch
> Currently each SSI unit 's busif dma address is calculated by
> following calculation formulation:
> 0xec54 + 0x1000 * id + busif / 4 * 0xA000 + busif % 4 * 0x400
>
> But according to user manual 41.1.4 Register Configuration
> ssi9 4/5/6/7 busif data register address
> (SSI9_4_BUSIF/SSI9_5_BUSIF/SSI9_6_BUSIF/SSI9_7_BUSIF)
> are out of this rule.
>
> This patch updates the calculation formulation to correct
> ssi9 4/5/6/7 busif data register address
>
> Fixes: commit 5e45a6fab3b9 ("ASoc: rsnd: dma: Calculate dma address with
> consider of BUSIF")
> Signed-off-by: Jiada Wang
> Signed-off-by: Timo Wischer
> ---
We don't need below anymore by this patch ?
--- dma.c
/*
* FIXME
*
* We can't support SSI9-4/5/6/7, because its address is
* out of calculation rule
*/
if ((id == 9) && (busif >= 4))
dev_err(dev, "This driver doesn't support SSI%d-%d, so far",
id, busif);
Best regards
---
Kuninori Morimoto
Re: [PATCH] ASoC: simple-card: Fix refcount underflow
Hi Daniel, Mark, again
> > of_get_child_by_name() takes a reference we'll need to drop
> > later so when we substitute in top we need to take a reference
> > as well as just assigning.
> >
> > Without this patch we hit the following error:
> >
> > [1.246852] OF: ERROR: Bad of_node_put() on /sound-wm8524
> > [1.262261] Hardware name: NXP i.MX8MQ EVK (DT)
> > [1.266807] Workqueue: events deferred_probe_work_func
> > [1.271950] Call trace:
> > [1.274406] dump_backtrace+0x0/0x158
> > [1.278074] show_stack+0x14/0x20
> > [1.281396] dump_stack+0xa8/0xcc
> > [1.284717] of_node_release+0xb0/0xc8
> > [1.288474] kobject_put+0x74/0xf0
> > [1.291879] of_node_put+0x14/0x28
> > [1.295286] __of_get_next_child+0x44/0x70
> > [1.299387] of_get_next_child+0x3c/0x60
> > [1.303315] simple_for_each_link+0x1dc/0x230
> > [1.307676] simple_probe+0x80/0x540
> > [1.311256] platform_drv_probe+0x50/0xa0
> >
> > This patch is based on an earlier version posted by Kuninori Morimoto
> > and commit message includes explanations from Mark Brown.
> >
> > https://patchwork.kernel.org/patch/10814255/
> >
> > Reported-by: Vicente Bergas
> > Signed-off-by: Daniel Baluta
> > ---
>
> I'm not sure which one is correct in this case
>
> Signed-off-by: Kuninori Morimoto
>
> or
>
> Acked-by: Kuninori Morimoto
And we want to add
Fixes: commit da215354eb55c ("ASoC: simple-card: merge simple-scu-card")
Re: [PATCH] ASoC: simple-card: Fix refcount underflow
Hi Daniel, Mark
> of_get_child_by_name() takes a reference we'll need to drop
> later so when we substitute in top we need to take a reference
> as well as just assigning.
>
> Without this patch we hit the following error:
>
> [1.246852] OF: ERROR: Bad of_node_put() on /sound-wm8524
> [1.262261] Hardware name: NXP i.MX8MQ EVK (DT)
> [1.266807] Workqueue: events deferred_probe_work_func
> [1.271950] Call trace:
> [1.274406] dump_backtrace+0x0/0x158
> [1.278074] show_stack+0x14/0x20
> [1.281396] dump_stack+0xa8/0xcc
> [1.284717] of_node_release+0xb0/0xc8
> [1.288474] kobject_put+0x74/0xf0
> [1.291879] of_node_put+0x14/0x28
> [1.295286] __of_get_next_child+0x44/0x70
> [1.299387] of_get_next_child+0x3c/0x60
> [1.303315] simple_for_each_link+0x1dc/0x230
> [1.307676] simple_probe+0x80/0x540
> [1.311256] platform_drv_probe+0x50/0xa0
>
> This patch is based on an earlier version posted by Kuninori Morimoto
> and commit message includes explanations from Mark Brown.
>
> https://patchwork.kernel.org/patch/10814255/
>
> Reported-by: Vicente Bergas
> Signed-off-by: Daniel Baluta
> ---
I'm not sure which one is correct in this case
Signed-off-by: Kuninori Morimoto
or
Acked-by: Kuninori Morimoto
> sound/soc/generic/simple-card.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/sound/soc/generic/simple-card.c b/sound/soc/generic/simple-card.c
> index 08df261024cf..dc18c4492955 100644
> --- a/sound/soc/generic/simple-card.c
> +++ b/sound/soc/generic/simple-card.c
> @@ -445,7 +445,7 @@ static int simple_for_each_link(struct simple_priv *priv,
> /* Check if it has dai-link */
> node = of_get_child_by_name(top, PREFIX "dai-link");
> if (!node) {
> - node = top;
> + node = of_node_get(top);
> is_top = 1;
> }
>
> --
> 2.17.1
>
Re: [PATCH] ASoC: fsi: fix spelling mistake "doens't" -> "doesn't"
Hi Colin
> From: Colin Ian King
>
> There is a spelling mistake in a dev_err message. Fix it.
>
> Signed-off-by: Colin Ian King
> ---
Acked-by: Kuninori Morimoto
> sound/soc/sh/fsi.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/sound/soc/sh/fsi.c b/sound/soc/sh/fsi.c
> index 285afbafa662..3447dbdba1f1 100644
> --- a/sound/soc/sh/fsi.c
> +++ b/sound/soc/sh/fsi.c
> @@ -780,7 +780,7 @@ static int fsi_clk_init(struct device *dev,
> return -EINVAL;
> }
> if (clock->div == clock->own) {
> - dev_err(dev, "cpu doens't support div clock\n");
> + dev_err(dev, "cpu doesn't support div clock\n");
> return -EINVAL;
> }
> }
> --
> 2.20.1
>
Re: compile error at sun6i_video
Hi Sakari Cc Niklas Thank you for your feedback > > /opt/RB02197/home/morimoto/save/WORK/linux/drivers/media/platform/sunxi/sun6i-csi/sun6i_video.c: > > In function 'sun6i_video_start_streaming': > > /opt/RB02197/home/morimoto/save/WORK/linux/drivers/media/platform/sunxi/sun6i-csi/sun6i_video.c:141:29: > > error: passing argument 1 of 'media_pipeline_start' from incompatible > > pointer type [-Werror=incompatible-pointer-types] > > ret = media_pipeline_start(>vdev.entity, >vdev.pipe); > > ^~~ (snip) > Do you have the patches Niklas's v4l2/mux (or my vc) branch in your tree? > They change a few things on the way and drivers need to be converted. > Drivers that have been added since the patches were written do need to be > converted as well, and I suppose the sun6i_video driver is one of them. I could find your branch at LinuxTV, and merged vc branch, but unfortunately it can't solve compile issue. I could understand its background. It is OK for me if it will be solved on the next version. I will skip it so far. Thank you for your help Best regards --- Kuninori Morimoto
compile error at sun6i_video
Hi MultiMedia ML I got below compile error at SH. ... CC drivers/tty/tty_io.o /opt/RB02197/home/morimoto/save/WORK/linux/drivers/media/platform/sunxi/sun6i-csi/sun6i_video.c: In function 'sun6i_video_start_streaming': /opt/RB02197/home/morimoto/save/WORK/linux/drivers/media/platform/sunxi/sun6i-csi/sun6i_video.c:141:29: error: passing argument 1 of 'media_pipeline_start' from incompatible pointer type [-Werror=incompatible-pointer-types] ret = media_pipeline_start(>vdev.entity, >vdev.pipe); ^~~ In file included from /opt/RB02197/home/morimoto/save/WORK/linux/include/media/media-device.h:26, from /opt/RB02197/home/morimoto/save/WORK/linux/include/media/v4l2-device.h:24, from /opt/RB02197/home/morimoto/save/WORK/linux/drivers/media/platform/sunxi/sun6i-csi/sun6i_video.c:10: /opt/RB02197/home/morimoto/save/WORK/linux/include/media/media-entity.h:1030:84: note: expected 'struct media_pad *' but argument is of type 'struct media_entity *' __must_check int media_pipeline_start(struct media_pad *pad, ^ /opt/RB02197/home/morimoto/save/WORK/linux/drivers/media/platform/sunxi/sun6i-csi/sun6i_video.c:205:22: error: passing argument 1 of 'media_pipeline_stop' from incompatible pointer type [-Werror=incompatible-pointer-types] media_pipeline_stop(>vdev.entity); ^~~ In file included from /opt/RB02197/home/morimoto/save/WORK/linux/include/media/media-device.h:26, from /opt/RB02197/home/morimoto/save/WORK/linux/include/media/v4l2-device.h:24, from /opt/RB02197/home/morimoto/save/WORK/linux/drivers/media/platform/sunxi/sun6i-csi/sun6i_video.c:10: /opt/RB02197/home/morimoto/save/WORK/linux/include/media/media-entity.h:1055:44: note: expected 'struct media_pad *' but argument is of type 'struct media_entity *' void media_pipeline_stop(struct media_pad *pad); ~~^~~ /opt/RB02197/home/morimoto/save/WORK/linux/drivers/media/platform/sunxi/sun6i-csi/sun6i_video.c: In function 'sun6i_video_stop_streaming': /opt/RB02197/home/morimoto/save/WORK/linux/drivers/media/platform/sunxi/sun6i-csi/sun6i_video.c:229:22: error: passing argument 1 of 'media_pipeline_stop' from incompatible pointer type [-Werror=incompatible-pointer-types] media_pipeline_stop(>vdev.entity); ^~~ In file included from /opt/RB02197/home/morimoto/save/WORK/linux/include/media/media-device.h:26, from /opt/RB02197/home/morimoto/save/WORK/linux/include/media/v4l2-device.h:24, from /opt/RB02197/home/morimoto/save/WORK/linux/drivers/media/platform/sunxi/sun6i-csi/sun6i_video.c:10: /opt/RB02197/home/morimoto/save/WORK/linux/include/media/media-entity.h:1055:44: note: expected 'struct media_pad *' but argument is of type 'struct media_entity *' void media_pipeline_stop(struct media_pad *pad); ~~^~~ Best regards --- Kuninori Morimoto
Re: [PATCH] ASoC: rsnd: ssiu: correct shift bit for ssiu9
Hi Jiada
> Currently "0xf << 36" is used to
> clear SSIU-9 internal buffer state, which overflows 32-bit value
> according to user reference manual, it is always bit4 ~ bit7
> of SSI_SYS_STATUS[1,3,5,7] registers indicate
> SSIU-9's buffer state, so "0xf << 4" should be used.
>
> This patch fix incorrect shifting issue in SSIU-9 case
>
> Fixes: commit b7169ddea2f2 ("ASoC: rsnd: remove RSND_REG_ from rsnd_reg")
>
> Signed-off-by: Jiada Wang
> ---
Thank you for your patch
Acked-by: Kuninori Morimoto
> sound/soc/sh/rcar/ssiu.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/sound/soc/sh/rcar/ssiu.c b/sound/soc/sh/rcar/ssiu.c
> index 2f3085a54eba..b11677489a5e 100644
> --- a/sound/soc/sh/rcar/ssiu.c
> +++ b/sound/soc/sh/rcar/ssiu.c
> @@ -79,7 +79,7 @@ static int rsnd_ssiu_init(struct rsnd_mod *mod,
> break;
> case 9:
> for (i = 0; i < 4; i++)
> - rsnd_mod_write(mod, SSI_SYS_STATUS((i * 2) + 1), 0xf <<
> (id * 4));
> + rsnd_mod_write(mod, SSI_SYS_STATUS((i * 2) + 1), 0xf <<
> 4);
> break;
> }
>
> --
> 2.19.2
>
Re: [PATCH 1/1] ASoC: rsnd: allow to register kctrl with different cfg
Hi Mark
> Currently kctrl with same name can only be created once,
> but in "MIX Playback Volume" case, same kctrl is used to
> control different MIX volume for different dai-link.
>
> this patch by check kctrl's cfg, allow to create kctrl
> with same name but different cfg.
>
> Fixes: commit 9c698e8481a1 ("ASoC: rsnd: tidyup registering method for
> rsnd_kctrl_new()")
>
> Signed-off-by: Jiada Wang
> ---
I and Jiada discussed about this issue locally,
and agreed we can use my patch instead of this.
I will post it soon, so, please drop this patch.
Best regards
---
Kuninori Morimoto
Re: [alsa-devel] [PATCH v1 3/3] ASoC: soc-core: fix platform name vs. of_node assignement
Hi Jon > > Indeed there is such case so far, but my understanding is that current > > driver should select "legacy style" or "modern style". > > If driver setup it as "legacy", but access to "modern" member, > > it is driver side bug, right ? > > Yes absolutely it is a driver bug, but looking at the snd_soc_dai_link > structure today it is not clear what the driver should be setting and > what is 'modern' and what is 'legacy'. You need to dig through the git > history and code to figure this out. So you could say it is not very > well documented/commented from a soc-core perspective and could be easy > for a driver writer to get themselves in a pickle/mess. Anyway, that is > easy to fix and we could add some comments to clear it up. Thank you for your feedback. Yes, indeed there is no enough information/documentation about legacy/modern style, and its plan (= all driver will be switched to modern, legacy will be removed, etc, etc..). So, can you agree about these ? 1) Add enough information/documentation about legacy/modern style and its plan. 2) Add dirty pointer fixup patch as workaround 3) switch to modern style as much as possible 1) and 2) are needed immediately. 3) needs more time, but we can try Best regards --- Kuninori Morimoto
Re: [alsa-devel] [PATCH v1 3/3] ASoC: soc-core: fix platform name vs. of_node assignement
Hi Jon > Actually no. Looking at snd_soc_init_multicodec() it always allocates > memory if any of the 'legacy' codec members > (codec_name/codec_of_node/codec_dai_name) are populated. However, this > looks quite fragile to me and is susceptible to leaking memory if the > user/machine driver already incorrectly allocated the memory as well as > populating these legacy codec members. Yeah, sorry I was 100% misunderstood. I thought there is a case that defered card connected to unbind_card_list, and try snd_soc_init_platform/codec again without freeing memory... very mess > My concern about all of this is it is not fool proof and hard to detect > if a machine driver is doing something bad that it should not. Yeah, agree. Best solution is removing legacy style, I think. > It is still fragile. Again the machine driver could have incorrectly set > these 'allocated_platform/codecs' members as they are exposed to the > machine driver. You have no way to determine if the machine driver is > doing the correct thing or not. The problem becomes more complex with > probe deferral. Indeed there is such case so far, but my understanding is that current driver should select "legacy style" or "modern style". If driver setup it as "legacy", but access to "modern" member, it is driver side bug, right ? Best regards --- Kuninori Morimoto
Re: [alsa-devel] Applied "ASoC: soc-core: defer card probe until all component is added to list" to the asoc tree
Hi Rohit > > I got below WARNING by this patch. > > I guess we need mutex_lock() on snd_soc_register_card() ? > > Right, we should have client_mutex lock before calling > soc_find_component(). > > We will post fix. Thanks !! Best regards --- Kuninori Morimoto
Re: [alsa-devel] [PATCH v1 3/3] ASoC: soc-core: fix platform name vs. of_node assignement
Hi Mark, Jon again > My posted quick-patch can solve "dirty pointer" issue, > but it can't solve "memory leak" issue. > This issue will be solved if all driver can switch to > modern style, but it needs more time. > Are these correct ? Sorry I was very confused. I think the issue is only "dirty pointer", there is no "memory leak" issue (= devm_kzalloc()). And the things Jon is worry about is that why we need to have platform pointer. And the answer is we need multi platform support in the future (pointer is prepare for it). Are these correct ? If so my posted patch can solve all issues ? Best regards --- Kuninori Morimoto
Re: [alsa-devel] Applied "ASoC: soc-core: defer card probe until all component is added to list" to the asoc tree
Hi Mark, Ajit > From: Ajit Pandey > Date: Wed, 9 Jan 2019 14:17:07 +0530 > Subject: [PATCH] ASoC: soc-core: defer card probe until all component is added > to list > > DAI component probe is not called if it is not present > in component list during sound card registration. > Check if component is available in component list for > platform and cpu dai before soundcard registration. > > Signed-off-by: Ajit Pandey > Signed-off-by: Rohit kumar > Signed-off-by: Mark Brown > --- I got below WARNING by this patch. I guess we need mutex_lock() on snd_soc_register_card() ? ... [drm] Device feb0.display probed [drm] Cannot find any crtc or sizes [drm] Cannot find any crtc or sizes WARNING: CPU: 0 PID: 76 at sound/soc/soc-core.c:739 soc_find_component+0xb8/0xc0 CPU: 0 PID: 76 Comm: kworker/0:1 Not tainted 5.0.0-rc1+ #1360 Hardware name: Renesas H3ULCB Kingfisher board based on r8a7795 ES2.0+ (DT) Workqueue: events deferred_probe_work_func pstate: 6005 (nZCv daif -PAN -UAO) pc : soc_find_component+0xb8/0xc0 lr : soc_find_component+0xb4/0xc0 sp : 1217ba10 x29: 1217ba10 x28: x27: 10a82920 x26: x25: 8006f9c59900 x24: 105f33a0 x23: 10948d70 x22: x21: 10af1720 x20: 8006ff80b6d8 x19: 8006f8523080 x18: 0010 x17: x16: x15: x14: 0720072007200720 x13: 0720072007200720 x12: 0720072007200720 x11: 0720072007200720 x10: 0720072007200720 x9 : x8 : 10a85000 x7 : 10114bd4 x6 : 0001 x5 : 0018 x4 : 0001 x3 : x2 : 0003 x1 : 10af17b0 x0 : Call trace: soc_find_component+0xb8/0xc0 soc_init_dai_link+0x18c/0x210 snd_soc_register_card.part.16+0x138/0x198 snd_soc_register_card+0x30/0x48 devm_snd_soc_register_card+0x4c/0xa0 graph_probe+0x2d8/0x388 platform_drv_probe+0x58/0xa8 really_probe+0x1c0/0x2a0 driver_probe_device+0x5c/0xf0 __device_attach_driver+0x9c/0xe0 ...
Re: [alsa-devel] [PATCH v1 3/3] ASoC: soc-core: fix platform name vs. of_node assignement
Hi Mark, Jon > No. Offline you had suggested using kmalloc and not devm_kzalloc and so > I was worried in that case about a memory leak. Right now I am only > concerned about an invalid pointer that is not being handled correctly. I'm sorry I was confused/misunderstood, kmalloc idea was wrong. > I would like someone to explain what multi-platform means? Even if a > soundcard supports multiple platforms, there is only one platform you > are using at any time so ... (snip) > ... I don't understand why you would ever need a 'num_platform' as the > machine driver just needs to understand which platform is using it at > any given time. Right? As Mark explained, "platform" on ALSA SoC means "DMA", and we might have multiple DMA sound sysytem (= multi-platform) in the future. Currently, all driver/sysytem is using single DMA for 1 sound card. > > # So, I guess if your driver can switch to use > > # snd_soc_init_platform style directly, your problem can gone ? > > Yes that is an alternative and I can convert all the Tegra machine > drivers to use this now. However, that will not solve the problem for > non-Tegra devices and everyone will have to do this. Yeah I agree. But my concern is that the same problem happen on codec side too, by same logic, because snd_soc_init_multicodec() is overwriting dai_link too. Your posted patch solved platform side only, I think. > > >> Yes that is an alternative and I can convert all the Tegra machine > > >> drivers to use this now. However, that will not solve the problem for > > >> non-Tegra devices and everyone will have to do this. > > > > We're going to have to go through another round of conversions that > > > touch everything at some point no matter what :/ > > > Do you have a preference here? Do you think that we can fix-up the > > soc-core or should I go ahead and migrate the Tegra machine driver to > > workaround this issue now? > > We're going to need to migrate Tegra regardless so it'd be good to do > that whatever happens, I'm intending to try to properly review the patch > today. As I mentioned above, I think we have same issue on codec side too. exchanging *platform to platform doesn't solve all issues. And we need to exchange all driver again if we had multi-platform support in the future (I don't know when it can happen though...) My posted quick-patch can solve "dirty pointer" issue, but it can't solve "memory leak" issue. This issue will be solved if all driver can switch to modern style, but it needs more time. Are these correct ? So, how about this ? I will try to add snd_soc_dai_link_component support for CPU, and switch all driver to use modern style for v5.1 (or v5.2 ?). Until then, as temporary solution, we can use above quick-patch style. And to avoid "memory leak crash" attach, it temporary have bind dai_link limitation (max 5time?). If it goes to max limitation, ALSA SoC doesn't allow to try again. In such case, all related CPU/Codec driver need to rmmod/unbind, and insmod/bind again. Then, the limitation will be 0 cleared. You can try bind again. It can solve "dirty pointer" issue, "memory leak" issue, and "memory leak attack" issue. The problem is that code can be dirty temporary. But it will be removed if all driver can be swtich to modern style. Best regards --- Kuninori Morimoto
Re: [PATCH] ASoC: core: Fix deferral of machine drivers
Hi Jon
> Commit daecf46ee0e5 ("ASoC: soc-core: use snd_soc_dai_link_component for
> platform") added a new member to the snd_soc_dai_link structure for
> storing a pointer for a platform link component. The pointer for this
> platform link component was allocated, if not already populated by the
> machine driver, using devm_kzalloc() such that the memory would be
> automatically freed on error or removal of the soundcard. However, this
> introduced a new problem, because if the probing of the soundcard is
> deferred, then although the memory allocated for the platform link
> component is freed, if the snd_soc_dai_link structure is declared
> statically by the machine driver, then the pointer in the DAI link
> structure will never be clearer. This means that when the soundcard is
> probed again, memory for the platform link component will not be
> allocated again because the address of the pointer was not cleared
> and this causes sound core to access memory that is no longer valid.
>
> In most cases this causes the following error condition to be triggered
> and causes probing the soundcard to fail.
>
> tegra-snd-sgtl5000 sound: ASoC: Both platform name/of_node are set for
> sgtl5000
>
> Unfortunately, because this platform link component is allocated before
> the DAI links are added to the soundcard, there is no easy way to clear
> this pointer on teardown if an error occurs.
>
> The pointer for this platform link component was added for future
> proofing and communalising the structures for storing various data.
> Although a machine driver maybe used by more than one platform and so
> this platform data may vary from platform to platform, there is only
> ever a single instance for a given platform. Therefore, rather than
> dynamically allocate the platform link component structure, make it a
> static member of the snd_soc_dai_link to fix the problem.
>
> It should be noted that if the platform_name of platform_of_node members
> of the snd_soc_dai_link structure are populated, these will always be
> used regardless of if the new platform.name or platform.of_node members
> are populated.
>
> Fixes: daecf46ee0e5 ("ASoC: soc-core: use snd_soc_dai_link_component for
> platform")
>
> Reported-by: Marcel Ziswiler
> Signed-off-by: Jon Hunter
> ---
Thank you for your patch.
The reason why it is allocating memory is it is for glue Legacy vs Modern style.
This means, this allocation style will be removed if ALSA SoC become modern
style.
The missing part so far is CPU.
Only CPU is not yet supporting snd_soc_dai_link_component style.
If all CPU/Codec/Platform supports snd_soc_dai_link_component style,
all driver can switch to it, and then, all will be static style.
Currently, simple card series is only(?) using this style.
The reason why platform is using pointer style is that
someone (not me ;P) will support multi platform style in the future.
Best regards
---
Kuninori Morimoto
