[PATCH 0/2] simple-card: Add samplerate and samplewidth constraints

[PATCH 0/2] simple-card: Add samplerate and samplewidth constraints

[Jyri Sarha]

These constraints help to sort out the format and samplerate
combinations that do not properly work on a specific HW. 

Jyri Sarha (2):
  ASoC: simple-card: Remove trailing whitespace
  ASoC: simple-card: Add support for samplerate and samplewidth
    constraints

 .../devicetree/bindings/sound/simple-card.txt      |  6 ++
 sound/soc/generic/simple-card.c                    | 89 +++++++++++++++++++++-
 2 files changed, 94 insertions(+), 1 deletion(-)

-- 
1.9.1

--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH 1/2] ASoC: simple-card: Remove trailing whitespace

[Jyri Sarha]

Signed-off-by: Jyri Sarha <jsarha-l0cyMroinI0@public.gmane.org>
---
 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 f7c6734..d15c919 100644
--- a/sound/soc/generic/simple-card.c
+++ b/sound/soc/generic/simple-card.c
@@ -50,7 +50,7 @@ static int asoc_simple_card_startup(struct snd_pcm_substream *substream)
 	ret = clk_prepare_enable(dai_props->cpu_dai.clk);
 	if (ret)
 		return ret;
-	
+
 	ret = clk_prepare_enable(dai_props->codec_dai.clk);
 	if (ret)
 		clk_disable_unprepare(dai_props->cpu_dai.clk);
-- 
1.9.1

--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 1/2] ASoC: simple-card: Remove trailing whitespace

[Mark Brown]

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

On Mon, Mar 02, 2015 at 04:14:32PM +0200, Jyri Sarha wrote:
> Signed-off-by: Jyri Sarha <jsarha-l0cyMroinI0@public.gmane.org>

Applied, thanks.

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 473 bytes --]

[PATCH 2/2] ASoC: simple-card: Add support for samplerate and samplewidth constraints

[Jyri Sarha]

Add DT properties to dailink for setting samplerate and samplewidth
constraints. The DT binding document has been updated.

Signed-off-by: Jyri Sarha <jsarha-l0cyMroinI0@public.gmane.org>
---
 .../devicetree/bindings/sound/simple-card.txt      |  6 ++
 sound/soc/generic/simple-card.c                    | 87 ++++++++++++++++++++++
 2 files changed, 93 insertions(+)

diff --git a/Documentation/devicetree/bindings/sound/simple-card.txt b/Documentation/devicetree/bindings/sound/simple-card.txt
index 73bf314..185a466 100644
--- a/Documentation/devicetree/bindings/sound/simple-card.txt
+++ b/Documentation/devicetree/bindings/sound/simple-card.txt
@@ -44,6 +44,10 @@ Required dai-link subnodes:
 
 Optional dai-link subnode properties:
 
+- samplewidth-constraints		: List of integers describing supported
+					  sample widths in number of bits.
+- rate-constraints			: List of integers describing supported
+					  sample samplerates in Hz.
 - format				: CPU/CODEC common audio format.
 					  "i2s", "right_j", "left_j" , "dsp_a"
 					  "dsp_b", "ac97", "pdm", "msb", "lsb"
@@ -97,6 +101,8 @@ sound {
 		"MIC_IN", "Microphone Jack",
 		"Headphone Jack", "HP_OUT",
 		"External Speaker", "LINE_OUT";
+	simple-audio-card,samplewidth-constraints = <16 24 32>;
+	simple-audio-card,samplerate-constraints = <22050 44100 88200>;
 
 	simple-audio-card,cpu {
 		sound-dai = <&sh_fsi2 0>;
diff --git a/sound/soc/generic/simple-card.c b/sound/soc/generic/simple-card.c
index d15c919..f718c5e 100644
--- a/sound/soc/generic/simple-card.c
+++ b/sound/soc/generic/simple-card.c
@@ -8,6 +8,7 @@
  * it under the terms of the GNU General Public License version 2 as
  * published by the Free Software Foundation.
  */
+
 #include <linux/clk.h>
 #include <linux/device.h>
 #include <linux/gpio.h>
@@ -20,12 +21,15 @@
 #include <sound/simple_card.h>
 #include <sound/soc-dai.h>
 #include <sound/soc.h>
+#include <sound/pcm_params.h>
 
 struct simple_card_data {
 	struct snd_soc_card snd_card;
 	struct simple_dai_props {
 		struct asoc_simple_dai cpu_dai;
 		struct asoc_simple_dai codec_dai;
+		struct snd_mask *format_constraint_mask;
+		struct snd_pcm_hw_constraint_list *rate_constraint;
 	} *dai_props;
 	unsigned int mclk_fs;
 	int gpio_hp_det;
@@ -41,12 +45,31 @@ struct simple_card_data {
 
 static int asoc_simple_card_startup(struct snd_pcm_substream *substream)
 {
+	struct snd_pcm_runtime *runtime = substream->runtime;
 	struct snd_soc_pcm_runtime *rtd = substream->private_data;
 	struct simple_card_data *priv =	snd_soc_card_get_drvdata(rtd->card);
 	struct simple_dai_props *dai_props =
 		&priv->dai_props[rtd - rtd->card->rtd];
+	struct device *dev = simple_priv_to_dev(priv);
 	int ret;
 
+	if (dai_props->format_constraint_mask) {
+		struct snd_mask *fmt = constrs_mask(&runtime->hw_constraints,
+						    SNDRV_PCM_HW_PARAM_FORMAT);
+		*fmt = *dai_props->format_constraint_mask;
+	}
+
+	if (dai_props->rate_constraint) {
+		ret = snd_pcm_hw_constraint_list(runtime, 0,
+						 SNDRV_PCM_HW_PARAM_RATE,
+						 dai_props->rate_constraint);
+		if (ret) {
+			dev_err(dev, "%s: Seting rate constraint failed: %d\n",
+				__func__, ret);
+			return ret;
+		}
+	}
+
 	ret = clk_prepare_enable(dai_props->cpu_dai.clk);
 	if (ret)
 		return ret;
@@ -264,6 +287,66 @@ asoc_simple_card_sub_parse_of(struct device_node *np,
 	return 0;
 }
 
+static void asoc_simple_format_mask(u32 width, struct snd_mask *mask)
+{
+	int i;
+
+	for (i = 0; i < SNDRV_PCM_FORMAT_LAST; i++)
+		if (snd_pcm_format_width(i) == width)
+			snd_mask_set(mask, i);
+}
+
+static int asoc_simple_card_parse_constraints(struct device_node *node,
+					      struct simple_card_data *priv,
+					      char *prefix, int idx)
+{
+	struct device *dev = simple_priv_to_dev(priv);
+	struct simple_dai_props *dai_props = simple_priv_to_props(priv, idx);
+	char prop[128];
+	const u32 *list;
+	u32 len;
+	int i;
+
+	snprintf(prop, sizeof(prop), "%ssamplewidth-constraints", prefix);
+	list = of_get_property(node, prop, &len);
+	len /= sizeof(*list);
+	if (list) {
+		struct snd_mask *mask =
+			devm_kzalloc(dev, sizeof(*mask), GFP_KERNEL);
+
+		if (!mask)
+			return -ENOMEM;
+		snd_mask_none(mask);
+		for (i = 0; i < len; i++) {
+			asoc_simple_format_mask(be32_to_cpu(list[i]), mask);
+			dev_dbg(dev, "%s: samplewidth %u\n", __func__,
+				be32_to_cpu(list[i]));
+		}
+		dai_props->format_constraint_mask = mask;
+	}
+
+	snprintf(prop, sizeof(prop), "%ssamplerate-constraints", prefix);
+	list = of_get_property(node, prop, &len);
+	len /= sizeof(*list);
+	if (list) {
+		struct snd_pcm_hw_constraint_list *constr =
+			devm_kzalloc(dev, sizeof(*constr), GFP_KERNEL);
+		unsigned int *clist =
+			devm_kzalloc(dev, sizeof(int) * len, GFP_KERNEL);
+
+		if (!constr || !clist)
+			return -ENOMEM;
+		constr->count = len;
+		for (i = 0; i < len; i++) {
+			clist[i] = (unsigned int) be32_to_cpu(list[i]);
+			dev_dbg(dev, "%s: samplerate %u\n", __func__, clist[i]);
+		}
+		constr->list = clist;
+		dai_props->rate_constraint = constr;
+	}
+	return 0;
+}
+
 static int asoc_simple_card_parse_daifmt(struct device_node *node,
 					 struct simple_card_data *priv,
 					 struct device_node *codec,
@@ -341,6 +424,10 @@ static int asoc_simple_card_dai_link_of(struct device_node *node,
 		goto dai_link_of_err;
 	}
 
+	ret = asoc_simple_card_parse_constraints(node, priv, prefix, idx);
+	if (ret < 0)
+		goto dai_link_of_err;
+
 	ret = asoc_simple_card_parse_daifmt(node, priv,
 					    codec, prefix, idx);
 	if (ret < 0)
-- 
1.9.1

--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [alsa-devel] [PATCH 2/2] ASoC: simple-card: Add support for samplerate and samplewidth constraints

[Pierre-Louis Bossart]

Hi Jyri,

On 3/2/15 8:14 AM, Jyri Sarha wrote:
>
>   Optional dai-link subnode properties:
>
> +- samplewidth-constraints		: List of integers describing supported
> +					  sample widths in number of bits.

There are quite a few platforms where the number of bits for valid audio 
bits differs from the number of bits per slot, e.g. 24 bits with 25 bit 
slots, or 24 bits in 32 bit slots. How would this be represented? This 
concept is present for the TDM case but not the simple one.
-Pierre

> +- rate-constraints			: List of integers describing supported
> +					  sample samplerates in Hz.
>   - format				: CPU/CODEC common audio format.
>   					  "i2s", "right_j", "left_j" , "dsp_a"

--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 2/2] ASoC: simple-card: Add support for samplerate and samplewidth constraints

[Jyri Sarha]

On 03/02/2015 09:35 PM, Pierre-Louis Bossart wrote:
> Hi Jyri,
>
> On 3/2/15 8:14 AM, Jyri Sarha wrote:
>>
>>   Optional dai-link subnode properties:
>>
>> +- samplewidth-constraints        : List of integers describing supported
>> +                      sample widths in number of bits.
>
> There are quite a few platforms where the number of bits for valid audio
> bits differs from the number of bits per slot, e.g. 24 bits with 25 bit
> slots, or 24 bits in 32 bit slots. How would this be represented? This
> concept is present for the TDM case but not the simple one.
> -Pierre

As these are dai-link level properties the idea is to set constraints to 
sample-width on the digital audio link, usually i2s. That is why the 
constraint is set based on sample-width and not physical width.

The number if significant bits in the sample-word should in a normal 
case be a property of the codec, if such a property is needed. On the 
other hand the limits for the physical layout in the system memory are 
usually set by the platform driver (= DMA HW), so such a property should 
- in a normal case - go to cpu-dai node. Since these cases can usually 
be associated to either end of the link, there should be no need for 
such properties in the machine driver.

I am not sure what you have in mind with the TDM case. In some cases a 
similar constraint property for number of channels could help.

Best regards,
Jyri

>
>> +- rate-constraints            : List of integers describing supported
>> +                      sample samplerates in Hz.
>>   - format                : CPU/CODEC common audio format.
>>                         "i2s", "right_j", "left_j" , "dsp_a"
>

Re: [alsa-devel] [PATCH 2/2] ASoC: simple-card: Add support for samplerate and samplewidth constraints

[Lars-Peter Clausen]

On 03/02/2015 03:14 PM, Jyri Sarha wrote:
> Add DT properties to dailink for setting samplerate and samplewidth
> constraints. The DT binding document has been updated.

Can you include a description why this is needed and how and when it is 
supposed to be used?
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 2/2] ASoC: simple-card: Add support for samplerate and samplewidth constraints

[Jyri Sarha]

On 03/02/2015 09:58 PM, Lars-Peter Clausen wrote:
> On 03/02/2015 03:14 PM, Jyri Sarha wrote:
>> Add DT properties to dailink for setting samplerate and samplewidth
>> constraints. The DT binding document has been updated.
>
> Can you include a description why this is needed and how and when it is
> supposed to be used?

Would this addition do:
------------------------------------------------------------
These constraints help to disable the sample-format and sample-rate
combinations that do not properly work on a specific HW.
------------------------------------------------------------

The reason why we need these is coming from limitations in McASP clock 
generation. With a simple divider one can only produce certain 
bit-clocks. With those bit-clocks we can only play/capture some 
sample-rate and sample-width combinations accurately.

The McASP driver could try to set the constraints automatically. 
However, since the constraint code can not select sample-width and 
sample-rate combinations there is a compromise to be made between them. 
Making such compromises automatically does not usually work that well.

In our case these properties could of course be added to McASP driver, 
but then again I would expect that there is a wider need for this kind 
of functionality. And it may not always be clear if either end of the 
link alone is responsible for less than perfect operation.

Best regards,
Jyri

Re: [alsa-devel] [PATCH 2/2] ASoC: simple-card: Add support for samplerate and samplewidth constraints

[Mark Brown]

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

On Tue, Mar 03, 2015 at 12:09:14PM +0200, Jyri Sarha wrote:
> On 03/02/2015 09:58 PM, Lars-Peter Clausen wrote:

> >Can you include a description why this is needed and how and when it is
> >supposed to be used?

> Would this addition do:
> ------------------------------------------------------------
> These constraints help to disable the sample-format and sample-rate
> combinations that do not properly work on a specific HW.
> ------------------------------------------------------------

Not entirely...

> The reason why we need these is coming from limitations in McASP clock
> generation. With a simple divider one can only produce certain bit-clocks.
> With those bit-clocks we can only play/capture some sample-rate and
> sample-width combinations accurately.

> The McASP driver could try to set the constraints automatically. However,
> since the constraint code can not select sample-width and sample-rate
> combinations there is a compromise to be made between them. Making such
> compromises automatically does not usually work that well.

...this is more the point.  Perhaps the constraints language needs
improvement here?

> In our case these properties could of course be added to McASP driver, but
> then again I would expect that there is a wider need for this kind of
> functionality. And it may not always be clear if either end of the link
> alone is responsible for less than perfect operation.

The trouble with this sort of interface is that it's a quick and dirty
way for people to bodge around things rather than actually fixing them
properly.  Of course sometimes fixing things properly is really hard and
that means we want a temporary bodge but having to put them in DT is
really unfortunate.

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 473 bytes --]

Re: [alsa-devel] [PATCH 2/2] ASoC: simple-card: Add support for samplerate and samplewidth constraints

[Jyri Sarha]

On 03/03/2015 01:30 PM, Mark Brown wrote:
> On Tue, Mar 03, 2015 at 12:09:14PM +0200, Jyri Sarha wrote:
>> On 03/02/2015 09:58 PM, Lars-Peter Clausen wrote:
>
>>> Can you include a description why this is needed and how and when it is
>>> supposed to be used?
>
>> Would this addition do:
>> ------------------------------------------------------------
>> These constraints help to disable the sample-format and sample-rate
>> combinations that do not properly work on a specific HW.
>> ------------------------------------------------------------
>
> Not entirely...
>
>> The reason why we need these is coming from limitations in McASP clock
>> generation. With a simple divider one can only produce certain bit-clocks.
>> With those bit-clocks we can only play/capture some sample-rate and
>> sample-width combinations accurately.
>
>> The McASP driver could try to set the constraints automatically. However,
>> since the constraint code can not select sample-width and sample-rate
>> combinations there is a compromise to be made between them. Making such
>> compromises automatically does not usually work that well.
>
> ...this is more the point.  Perhaps the constraints language needs
> improvement here?
>

Improving constraint functionality would certainly help, however the way 
that code works is beyond my understanding and I do not believe such an 
improvement would be coming from anybody else any time soon either.

>> In our case these properties could of course be added to McASP driver, but
>> then again I would expect that there is a wider need for this kind of
>> functionality. And it may not always be clear if either end of the link
>> alone is responsible for less than perfect operation.
>
> The trouble with this sort of interface is that it's a quick and dirty
> way for people to bodge around things rather than actually fixing them
> properly.  Of course sometimes fixing things properly is really hard and
> that means we want a temporary bodge but having to put them in DT is
> really unfortunate.
>

I agree with that. However, the simple-card binding goes already now 
quite a bit beyond just describing the hardware. The binding for 
instance decides the configuration that is going to be used over the 
dai-link. These constraints could be seen as an extension to that 
configuration.

I am wondering if there would be some better way to select the dai-link 
configuration than writing it to DT or creating a custom machine driver 
for each setup.

But about this patch. Should I just give it up, or would you be willing 
to apply it if I improve the description more and add a warning against 
using these properties to work around driver bugs to the binding document?

Best regards,
Jyri
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 2/2] ASoC: simple-card: Add support for samplerate and samplewidth constraints

[Lars-Peter Clausen]

On 03/03/2015 01:00 PM, Jyri Sarha wrote:
> On 03/03/2015 01:30 PM, Mark Brown wrote:
>> On Tue, Mar 03, 2015 at 12:09:14PM +0200, Jyri Sarha wrote:
>>> On 03/02/2015 09:58 PM, Lars-Peter Clausen wrote:
>>
>>>> Can you include a description why this is needed and how and when it is
>>>> supposed to be used?
>>
>>> Would this addition do:
>>> ------------------------------------------------------------
>>> These constraints help to disable the sample-format and sample-rate
>>> combinations that do not properly work on a specific HW.
>>> ------------------------------------------------------------
>>
>> Not entirely...
>>
>>> The reason why we need these is coming from limitations in McASP clock
>>> generation. With a simple divider one can only produce certain bit-clocks.
>>> With those bit-clocks we can only play/capture some sample-rate and
>>> sample-width combinations accurately.
>>
>>> The McASP driver could try to set the constraints automatically. However,
>>> since the constraint code can not select sample-width and sample-rate
>>> combinations there is a compromise to be made between them. Making such
>>> compromises automatically does not usually work that well.
>>
>> ...this is more the point.  Perhaps the constraints language needs
>> improvement here?
>>
>
> Improving constraint functionality would certainly help, however the way
> that code works is beyond my understanding and I do not believe such an
> improvement would be coming from anybody else any time soon either.

Restricting the available sample formats based on the sample rate and vice 
versa is possible with the current constraint framework. Take a look at what 
Peter Rosin recently did to the pcm512x driver. Your restrictions sound very 
similar to what he did.

>
>>> In our case these properties could of course be added to McASP driver, but
>>> then again I would expect that there is a wider need for this kind of
>>> functionality. And it may not always be clear if either end of the link
>>> alone is responsible for less than perfect operation.
>>
>> The trouble with this sort of interface is that it's a quick and dirty
>> way for people to bodge around things rather than actually fixing them
>> properly.  Of course sometimes fixing things properly is really hard and
>> that means we want a temporary bodge but having to put them in DT is
>> really unfortunate.
>>
>
> I agree with that. However, the simple-card binding goes already now quite a
> bit beyond just describing the hardware. The binding for instance decides
> the configuration that is going to be used over the dai-link. These
> constraints could be seen as an extension to that configuration.
>
> I am wondering if there would be some better way to select the dai-link
> configuration than writing it to DT or creating a custom machine driver for
> each setup.
>
> But about this patch. Should I just give it up, or would you be willing to
> apply it if I improve the description more and add a warning against using
> these properties to work around driver bugs to the binding document?

Well, your description is basically saying that you want to use this to work 
around a driver bug, so...

Re: [PATCH 2/2] ASoC: simple-card: Add support for samplerate and samplewidth constraints

[Jyri Sarha]

On 03/03/2015 05:31 PM, Lars-Peter Clausen wrote:
> On 03/03/2015 01:00 PM, Jyri Sarha wrote:
>> On 03/03/2015 01:30 PM, Mark Brown wrote:
>>> On Tue, Mar 03, 2015 at 12:09:14PM +0200, Jyri Sarha wrote:
>>>> On 03/02/2015 09:58 PM, Lars-Peter Clausen wrote:
>>>
>>>>> Can you include a description why this is needed and how and when
>>>>> it is
>>>>> supposed to be used?
>>>
>>>> Would this addition do:
>>>> ------------------------------------------------------------
>>>> These constraints help to disable the sample-format and sample-rate
>>>> combinations that do not properly work on a specific HW.
>>>> ------------------------------------------------------------
>>>
>>> Not entirely...
>>>
>>>> The reason why we need these is coming from limitations in McASP clock
>>>> generation. With a simple divider one can only produce certain
>>>> bit-clocks.
>>>> With those bit-clocks we can only play/capture some sample-rate and
>>>> sample-width combinations accurately.
>>>
>>>> The McASP driver could try to set the constraints automatically.
>>>> However,
>>>> since the constraint code can not select sample-width and sample-rate
>>>> combinations there is a compromise to be made between them. Making such
>>>> compromises automatically does not usually work that well.
>>>
>>> ...this is more the point.  Perhaps the constraints language needs
>>> improvement here?
>>>
>>
>> Improving constraint functionality would certainly help, however the way
>> that code works is beyond my understanding and I do not believe such an
>> improvement would be coming from anybody else any time soon either.
>
> Restricting the available sample formats based on the sample rate and
> vice versa is possible with the current constraint framework. Take a
> look at what Peter Rosin recently did to the pcm512x driver. Your
> restrictions sound very similar to what he did.
>

Interesting. It indeed looks like the rule functionality could do what I 
want. I'll look into than. Thanks!

>>
>>>> In our case these properties could of course be added to McASP
>>>> driver, but
>>>> then again I would expect that there is a wider need for this kind of
>>>> functionality. And it may not always be clear if either end of the link
>>>> alone is responsible for less than perfect operation.
>>>
>>> The trouble with this sort of interface is that it's a quick and dirty
>>> way for people to bodge around things rather than actually fixing them
>>> properly.  Of course sometimes fixing things properly is really hard and
>>> that means we want a temporary bodge but having to put them in DT is
>>> really unfortunate.
>>>
>>
>> I agree with that. However, the simple-card binding goes already now
>> quite a
>> bit beyond just describing the hardware. The binding for instance decides
>> the configuration that is going to be used over the dai-link. These
>> constraints could be seen as an extension to that configuration.
>>
>> I am wondering if there would be some better way to select the dai-link
>> configuration than writing it to DT or creating a custom machine
>> driver for
>> each setup.
>>
>> But about this patch. Should I just give it up, or would you be
>> willing to
>> apply it if I improve the description more and add a warning against
>> using
>> these properties to work around driver bugs to the binding document?
>
> Well, your description is basically saying that you want to use this to
> work around a driver bug, so...

Calling missing feature a bug is a bit harsh, but now that it seems 
there is a better to deal with this, I'll look into that.

Best regards,
Jyri

Re: [PATCH 2/2] ASoC: simple-card: Add support for samplerate and samplewidth constraints

[Jyri Sarha]

On 03/04/15 09:48, Jyri Sarha wrote:
> On 03/03/2015 05:31 PM, Lars-Peter Clausen wrote:
>> On 03/03/2015 01:00 PM, Jyri Sarha wrote:
>>> On 03/03/2015 01:30 PM, Mark Brown wrote:
>>>> On Tue, Mar 03, 2015 at 12:09:14PM +0200, Jyri Sarha wrote:
>>>>> On 03/02/2015 09:58 PM, Lars-Peter Clausen wrote:
>>>>
>>>>>> Can you include a description why this is needed and how and when
>>>>>> it is
>>>>>> supposed to be used?
>>>>
>>>>> Would this addition do:
>>>>> ------------------------------------------------------------
>>>>> These constraints help to disable the sample-format and sample-rate
>>>>> combinations that do not properly work on a specific HW.
>>>>> ------------------------------------------------------------
>>>>
>>>> Not entirely...
>>>>
>>>>> The reason why we need these is coming from limitations in McASP clock
>>>>> generation. With a simple divider one can only produce certain
>>>>> bit-clocks.
>>>>> With those bit-clocks we can only play/capture some sample-rate and
>>>>> sample-width combinations accurately.
>>>>
>>>>> The McASP driver could try to set the constraints automatically.
>>>>> However,
>>>>> since the constraint code can not select sample-width and sample-rate
>>>>> combinations there is a compromise to be made between them. Making
>>>>> such
>>>>> compromises automatically does not usually work that well.
>>>>
>>>> ...this is more the point.  Perhaps the constraints language needs
>>>> improvement here?
>>>>
>>>
>>> Improving constraint functionality would certainly help, however the way
>>> that code works is beyond my understanding and I do not believe such an
>>> improvement would be coming from anybody else any time soon either.
>>
>> Restricting the available sample formats based on the sample rate and
>> vice versa is possible with the current constraint framework. Take a
>> look at what Peter Rosin recently did to the pcm512x driver. Your
>> restrictions sound very similar to what he did.
>>
>
> Interesting. It indeed looks like the rule functionality could do what I
> want. I'll look into than. Thanks!
>

I went ahead and implemented in mcasp driver a similar rule to the one 
already found from pcm512x. The rule indeed forbids the sample-rate and 
fame-size combinations that can not be played accurately.

However, once a user tries to play a bad sample-rate and fame-size 
combination, aplay greets him with:

aplay: pcm_params.c:170: snd1_pcm_hw_param_get_min: Assertion 
`!snd_interval_empty(i)' failed.

And even worse, playing to plughw gives the same error. So in practice 
the rule is pretty useless. The user is in the same situation as before: 
His use-case does not work and he has no idea why.

I guess in the long run the ideal solution would making the user-space 
behave better in such a situation, but for now I thing a human selected 
constraints are the only proper way to solve the problem.

Putting the constraints into dts may not be the most elegant choice, so 
I look into adding a new compatible match for each new card and hard 
code the DAI-link configurations for each match in the code instead of dts.

I'll send the rule patch too (as soon as I have it cleaned it up) as it 
does hurt to have it, even if it is not enough to make all the 
configurations properly usable.

Best regards,
Jyri

Re: [PATCH 2/2] ASoC: simple-card: Add support for samplerate and samplewidth constraints

[Mark Brown]

[-- Attachment #1.1: Type: text/plain, Size: 1938 bytes --]

On Tue, Mar 03, 2015 at 02:00:31PM +0200, Jyri Sarha wrote:
> On 03/03/2015 01:30 PM, Mark Brown wrote:

> >...this is more the point.  Perhaps the constraints language needs
> >improvement here?

> Improving constraint functionality would certainly help, however the way
> that code works is beyond my understanding and I do not believe such an
> improvement would be coming from anybody else any time soon either.

It's probably worth putting together a description of the constraint and
asking people like Takashi who understand the code - ideally it'd be
easy to implement but I suspect you're right about timescales.

> >The trouble with this sort of interface is that it's a quick and dirty
> >way for people to bodge around things rather than actually fixing them
> >properly.  Of course sometimes fixing things properly is really hard and
> >that means we want a temporary bodge but having to put them in DT is
> >really unfortunate.

> I agree with that. However, the simple-card binding goes already now quite a
> bit beyond just describing the hardware. The binding for instance decides
> the configuration that is going to be used over the dai-link. These
> constraints could be seen as an extension to that configuration.

> I am wondering if there would be some better way to select the dai-link
> configuration than writing it to DT or creating a custom machine driver for
> each setup.

> But about this patch. Should I just give it up, or would you be willing to
> apply it if I improve the description more and add a warning against using
> these properties to work around driver bugs to the binding document?

I'm not totally against the idea so it's worth continuing.  Just not
happy either but computer.

It just occurred to me that we may be able to sidestep the issue by
calling them "suggested rates/widths" so the implementation can ignore
them later.  That's a *tiny* bit gross but does sidestep the ABI issues.

[-- Attachment #1.2: Digital signature --]
[-- Type: application/pgp-signature, Size: 473 bytes --]

[-- Attachment #2: Type: text/plain, Size: 0 bytes --]

Re: [PATCH 2/2] ASoC: simple-card: Add support for samplerate and samplewidth constraints

[Jyri Sarha]

On 03/03/2015 05:34 PM, Mark Brown wrote:
> On Tue, Mar 03, 2015 at 02:00:31PM +0200, Jyri Sarha wrote:
>> On 03/03/2015 01:30 PM, Mark Brown wrote:
>
>>> ...this is more the point.  Perhaps the constraints language needs
>>> improvement here?
>
>> Improving constraint functionality would certainly help, however the way
>> that code works is beyond my understanding and I do not believe such an
>> improvement would be coming from anybody else any time soon either.
>
> It's probably worth putting together a description of the constraint and
> asking people like Takashi who understand the code - ideally it'd be
> easy to implement but I suspect you're right about timescales.
>

Now that Lars-Peter pointed me to the right direction, it seems there is 
a proper way to deal with issue after all.

>>> The trouble with this sort of interface is that it's a quick and dirty
>>> way for people to bodge around things rather than actually fixing them
>>> properly.  Of course sometimes fixing things properly is really hard and
>>> that means we want a temporary bodge but having to put them in DT is
>>> really unfortunate.
>
>> I agree with that. However, the simple-card binding goes already now quite a
>> bit beyond just describing the hardware. The binding for instance decides
>> the configuration that is going to be used over the dai-link. These
>> constraints could be seen as an extension to that configuration.
>
>> I am wondering if there would be some better way to select the dai-link
>> configuration than writing it to DT or creating a custom machine driver for
>> each setup.
>

Continuing this tought. I wonder if it would be better to introduce a 
new compatible match for each new card, with some clever way to manage 
the accumulating matches in the code, and hard code DAI-link 
configurations for each match. This way the old configurations would not 
be carved to stone in the old dtbs.

>> But about this patch. Should I just give it up, or would you be willing to
>> apply it if I improve the description more and add a warning against using
>> these properties to work around driver bugs to the binding document?
>
> I'm not totally against the idea so it's worth continuing.  Just not
> happy either but computer.
>
> It just occurred to me that we may be able to sidestep the issue by
> calling them "suggested rates/widths" so the implementation can ignore
> them later.  That's a *tiny* bit gross but does sidestep the ABI issues.
>

As there is a proper way to deal with this, I'll look into that first. 
However, if there still is a need for these properties I am happy to 
finish the patch.

Best regards,
Jyri

Re: [PATCH 2/2] ASoC: simple-card: Add support for samplerate and samplewidth constraints

[Mark Brown]

[-- Attachment #1.1: Type: text/plain, Size: 969 bytes --]

On Wed, Mar 04, 2015 at 09:56:24AM +0200, Jyri Sarha wrote:
> On 03/03/2015 05:34 PM, Mark Brown wrote:

> >>I am wondering if there would be some better way to select the dai-link
> >>configuration than writing it to DT or creating a custom machine driver for
> >>each setup.

> Continuing this tought. I wonder if it would be better to introduce a new
> compatible match for each new card, with some clever way to manage the
> accumulating matches in the code, and hard code DAI-link configurations for
> each match. This way the old configurations would not be carved to stone in
> the old dtbs.

I would prefer that, yes - if only for override capability.

> As there is a proper way to deal with this, I'll look into that first.
> However, if there still is a need for these properties I am happy to finish
> the patch.

OK, a proper fix is of course better.  Might be worth looking at tooling
to make it easier to find/use but IIRC there's not a lot doing there.

[-- Attachment #1.2: Digital signature --]
[-- Type: application/pgp-signature, Size: 473 bytes --]

[-- Attachment #2: Type: text/plain, Size: 0 bytes --]