[PATCH] ASoC: generic: Add generic DT based simple codec

[PATCH] ASoC: generic: Add generic DT based simple codec

[Jean-Francois Moine]

This patch adds a simple sound codec which is described by the DT.
This codec may be used when no specific codec action is needed.

Signed-off-by: Jean-Francois Moine <moinejf-GANU6spQydw@public.gmane.org>
---
 .../devicetree/bindings/sound/simple-codec.txt | 103 +++++++++++++
 sound/soc/generic/Kconfig                      |   6 +
 sound/soc/generic/Makefile                     |   2 +
 sound/soc/generic/simple-codec.c               | 197 +++++++++++++++++++++++++
 4 files changed, 3088 insertions(+)

diff --git a/Documentation/devicetree/bindings/sound/simple-codec.txt b/Documentation/devicetree/bindings/sound/simple-codec.txt
new file mode 100644
index 0000000..75be747
--- /dev/null
+++ b/Documentation/devicetree/bindings/sound/simple-codec.txt
@@ -0,0 +1,103 @@
+Device-Tree bindings for the simple codec
+
+Required properties:
+- compatible: should be "linux,simple-codec".
+- dai-name: name of the codec
+
+Optional properties:
+- capture: information about capture
+- playback: information about playback
+At least one of the 'capture' or 'playback' nodes must be present.
+
+Child 'capture' and 'playback' required properties:
+- stream-name: name of the stream
+- formats: list of the supported formats (see below)
+- rates: list of the supported rates (see below)
+- #channels: minimum and maximum numbers of channels
+
+Formats:
+	"s8"
+	"u8"
+	"s16_le"
+	"s16_be"
+	"u16_le"
+	"u16_be"
+	"s24_le"
+	"s24_be"
+	"u24_le"
+	"u24_be"
+	"s32_le"
+	"s32_be"
+	"u32_le"
+	"u32_be"
+	"float_le"
+	"float_be"
+	"float64_le"
+	"float64_be"
+	"iec958_subframe_le"
+	"iec958_subframe_be"
+	"mu_law"
+	"a_law"
+	"ima_adpcm"
+	"mpeg"
+	"gsm"
+	"special"
+	"s24_3l"
+	"s24_3be"
+	"u24_3le"
+	"u24_3be"
+	"s20_3le"
+	"s20_3be"
+	"u20_3le"
+	"u20_3be"
+	"s18_3le"
+	"s18_3b"
+	"u18_3le"
+	"u18_3be"
+	"g723_24"
+	"g723_24_1b"
+	"g723_40"
+	"g723_40_1b"
+	"dsd_u8"
+	"dsd_u16_le"
+
+Rates:
+	5512
+	8000
+	11025
+	16000
+	22050
+	32000
+	44100
+	48000
+	64000
+	88200
+	96000
+	176400
+	192000
+
+Examples:
+
+	spdifo: spdif-transmitter {
+		compatible = "linux,simple-codec";
+		dai-name = "dit-hifi";
+		playback {
+			stream-name = "S/PDIF Playback";
+			formats = "s16_le";
+			rates = <8000 11025 16000 22050 32000 44100
+				48000 64000 88200 96000>;
+			#channels = <1 384>;
+		};
+	};
+
+	hdmio: hdmi-transmitter {
+		compatible = "linux,simple-codec";
+		dai-name = "hdmi-hifi";
+		playback {
+			stream-name = "HDMI Playback";
+			formats = "s16_le", "s24_le";
+			rates = <32000 44100 48000 88200 96000
+				176400 192000>;
+			#channels = <2 8>;
+		};
+	};
diff --git a/sound/soc/generic/Kconfig b/sound/soc/generic/Kconfig
index 610f612..d9221539 100644
--- a/sound/soc/generic/Kconfig
+++ b/sound/soc/generic/Kconfig
@@ -2,3 +2,9 @@ config SND_SIMPLE_CARD
 	tristate "ASoC Simple sound card support"
 	help
 	  This option enables generic simple sound card support
+
+config SND_SIMPLE_CODEC
+	tristate "ASoC Simple sound codec support"
+	depends on OF
+	help
+	  This option enables generic simple sound codec support
diff --git a/sound/soc/generic/Makefile b/sound/soc/generic/Makefile
index 9c3b246..f94620e 100644
--- a/sound/soc/generic/Makefile
+++ b/sound/soc/generic/Makefile
@@ -1,3 +1,5 @@
 snd-soc-simple-card-objs	:= simple-card.o
+snd-soc-simple-codec-objs	:= simple-codec.o
 
 obj-$(CONFIG_SND_SIMPLE_CARD)	+= snd-soc-simple-card.o
+obj-$(CONFIG_SND_SIMPLE_CODEC)	+= snd-soc-simple-codec.o
diff --git a/sound/soc/generic/simple-codec.c b/sound/soc/generic/simple-codec.c
new file mode 100644
index 0000000..217cd0e
--- /dev/null
+++ b/sound/soc/generic/simple-codec.c
@@ -0,0 +1,197 @@
+/*
+ * ALSA SoC simple codec driver
+ *
+ * This driver is used by controllers which do not need any codec
+ * (s/pdif, hdmi..).
+ * Its parameters are built from a DT description.
+ *
+ * Copyright: (C) 2013 Jean-François Moine <moinejf-GANU6spQydw@public.gmane.org>
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License version 2 as
+ * published by the Free Software Foundation.
+ */
+
+#include <linux/module.h>
+#include <sound/soc.h>
+#include <linux/of.h>
+
+#define DRV_NAME "simple-codec"
+
+static struct snd_soc_codec_driver soc_codec_simple_codec;
+
+static char *format_tb[] = {
+	"s8", "u8", "s16_le", "s16_be",
+	"u16_le", "u16_be", "s24_le", "s24_be",
+	"u24_le", "u24_be", "s32_le", "s32_be",
+	"u32_le", "u32_be", "float_le", "float_be",
+
+	"float64_le", "float64_be", "iec958_subframe_le", "iec958_subframe_be",
+	"mu_law", "a_law", "ima_adpcm", "mpeg",
+	"gsm", "", "", "",
+	"", "", "", "special",
+
+	"s24_3l", "s24_3be", "u24_3le", "u24_3be",
+	"s20_3le", "s20_3be", "u20_3le", "u20_3be",
+	"s18_3le", "s18_3b", "u18_3le", "u18_3be",
+	"g723_24", "g723_24_1b", "g723_40", "g723_40_1b",
+
+	"dsd_u8", "dsd_u16_le",
+};
+
+#if SNDRV_PCM_RATE_5512 != 1 << 0 || SNDRV_PCM_RATE_192000 != 1 << 12
+#error "Change this table"
+#endif
+static u32 rate_tb[] = { 5512, 8000, 11025, 16000, 22050, 32000, 44100,
+			48000, 64000, 88200, 96000, 176400, 192000 };
+
+static int get_format(u64 *p_formats, char *p)
+{
+	int i;
+
+	for (i = 0; i < ARRAY_SIZE(format_tb); i++) {
+		if (strcmp(format_tb[i], p) == 0) {
+			*p_formats |= 1 << i;
+			return 0;
+		}
+	}
+	return -EINVAL;
+}
+
+static int get_rate(u32 *p_rates, u32 val)
+{
+	int i;
+
+	for (i = 0; i < ARRAY_SIZE(rate_tb); i++) {
+		if (rate_tb[i] == val) {
+			*p_rates |= 1 << i;
+			return 0;
+		}
+	}
+	return -EINVAL;
+}
+
+static int stream_populate(struct device *dev,
+			struct snd_soc_pcm_stream *stream,
+			struct device_node *np)
+{
+	u64 formats;
+	u32 rates, val;
+	char *p;
+	int i, ret;
+
+	ret = of_property_read_string(np, "stream-name", &stream->stream_name);
+	if (ret) {
+		dev_err(dev, "Failed to parse stream-name string\n");
+		return ret;
+	}
+
+	i = 0;
+	formats = 0;
+	for (;;) {
+		ret = of_property_read_string_index(np, "formats", i,
+					(const char **) &p);
+		if (ret)
+			break;
+		ret = get_format(&formats, p);
+		if (ret) {
+			dev_err(dev, "Bad codec format\n");
+			return ret;
+		}
+		i++;
+	}
+	stream->formats = formats;
+
+	i = 0;
+	rates = 0;
+	for (;;) {
+		ret = of_property_read_u32_index(np, "rates", i, &val);
+		if (ret)
+			break;
+		ret = get_rate(&rates, val);
+		if (ret) {
+			dev_err(dev, "Bad rate\n");
+			return ret;
+		}
+		i++;
+	}
+	stream->rates = rates;
+
+	ret = of_property_read_u32_index(np, "#channels", 0, &val);
+	if (ret) {
+		dev_err(dev, "Bad min channel\n");
+		return ret;
+	}
+	stream->channels_min = val;
+	ret = of_property_read_u32_index(np, "#channels", 1, &val);
+	if (ret) {
+		dev_err(dev, "Bad max channel\n");
+		return ret;
+	}
+	stream->channels_max = val;
+
+	return 0;
+}
+
+static int simple_codec_probe(struct platform_device *pdev)
+{
+	struct device *dev = &pdev->dev;
+	struct device_node *np = dev->of_node;
+	struct device_node *np2;
+	struct snd_soc_dai_driver *dai;
+	int ret;
+
+	dai = devm_kzalloc(dev, sizeof *dai, GFP_KERNEL);
+	if (!dai)
+		return -ENOMEM;
+
+	ret = of_property_read_string(np, "dai-name", &dai->name);
+	if (ret) {
+		dev_err(dev, "Failed to parse dai-name\n");
+		return ret;
+	}
+
+	np2 = of_find_node_by_name(np, "capture");
+	if (np2) {
+		ret = stream_populate(dev, &dai->capture, np2);
+		if (ret)
+			return ret;
+	}
+	np2 = of_find_node_by_name(np, "playback");
+	if (np2) {
+		ret = stream_populate(dev, &dai->playback, np2);
+		if (ret)
+			return ret;
+	}
+
+	return snd_soc_register_codec(dev, &soc_codec_simple_codec,
+					dai, 1);
+}
+
+static int simple_codec_remove(struct platform_device *pdev)
+{
+	snd_soc_unregister_codec(&pdev->dev);
+	return 0;
+}
+
+static const struct of_device_id simple_codec_of_match[] = {
+	{ .compatible = "linux,simple-codec", },
+	{ }
+};
+MODULE_DEVICE_TABLE(of, simple_codec_of_match);
+
+static struct platform_driver simple_codec_driver = {
+	.probe		= simple_codec_probe,
+	.remove		= simple_codec_remove,
+	.driver		= {
+		.name	= DRV_NAME,
+		.owner	= THIS_MODULE,
+		.of_match_table = simple_codec_of_match,
+	},
+};
+
+module_platform_driver(simple_codec_driver);
+
+MODULE_AUTHOR("Jean-Francois Moine <moinejf-GANU6spQydw@public.gmane.org>");
+MODULE_DESCRIPTION("simple codec driver");
+MODULE_LICENSE("GPL");


-- 
Ken ar c'hentañ	|	      ** Breizh ha Linux atav! **
Jef		|		http://moinejf.free.fr/
--
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] ASoC: generic: Add generic DT based simple codec

[Mark Brown]

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

On Thu, Sep 19, 2013 at 10:54:37AM +0200, Jean-Francois Moine wrote:
> This patch adds a simple sound codec which is described by the DT.
> This codec may be used when no specific codec action is needed.

Hrm.  I'm generally not enthusiastic about this sort of thing because it
requires every user of the device to understand the capabilities of the
device instead of being able to just say they have a device of type X -
it seems better to type the data in once in a driver and then reference
it.  That said...

> +Child 'capture' and 'playback' required properties:
> +- stream-name: name of the stream

What does this mean in the binding?

> +Formats:
> +	"s8"
> +	"u8"

I'd cut this down to just the PCM formats for now - many of the formats
you list there aren't terribly well defined even within ALSA, I don't
think some of them have ever been used at all.  A binding document
should really document what things mean as well.

> +Rates:
> +	5512
> +	8000

This shouldn't be a list of defined rates, it should allow any number,
and it should support ranges since while some devices do enumerate
specific rates simpler devices often just support a continuous range of
rates.

> --- a/sound/soc/generic/Kconfig
> +++ b/sound/soc/generic/Kconfig

CODEC drivers should go in codecs.

> +#define DRV_NAME "simple-codec"

Just write the string in the one place where it is used.

> +static char *format_tb[] = {
> +	"s8", "u8", "s16_le", "s16_be",
> +	"u16_le", "u16_be", "s24_le", "s24_be",
> +	"u24_le", "u24_be", "s32_le", "s32_be",
> +	"u32_le", "u32_be", "float_le", "float_be",
> +
> +	"float64_le", "float64_be", "iec958_subframe_le", "iec958_subframe_be",
> +	"mu_law", "a_law", "ima_adpcm", "mpeg",
> +	"gsm", "", "", "",
> +	"", "", "", "special",

This seems a bit obscure and fragile - an explicit lookup table would be
better I think.

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

Re: [PATCH] ASoC: generic: Add generic DT based simple codec

[Jean-Francois Moine]

On Thu, 19 Sep 2013 11:40:28 +0100
Mark Brown <broonie-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org> wrote:

> On Thu, Sep 19, 2013 at 10:54:37AM +0200, Jean-Francois Moine wrote:
> > This patch adds a simple sound codec which is described by the DT.
> > This codec may be used when no specific codec action is needed.
> 
> Hrm.  I'm generally not enthusiastic about this sort of thing because it
> requires every user of the device to understand the capabilities of the
> device instead of being able to just say they have a device of type X -
> it seems better to type the data in once in a driver and then reference
> it.  That said...

Well, first, the codecs hdmi and dmic have no DT support.
Then, from the Cubox point of vue, the codec hdmi offers playback
and capture while the Cubox has only playback (via the tda998x HDMI
transmitter), and, also, the codec spdif transmitter has a lot of
sample rates while the Dove audio device supports only 44.1, 48 and 96
kHz.

So, instead of adding new hdmi and spdif_transmitter codecs,
I thought it could be useful to have a generic codec with DT support.

On the other hand, as the first use of this codec is for the Cubox,
an other solution could be to have the codec included in the kirkwood
driver (declared by DT as either i2s or s/pdif). This would simplify
the use or not of s/pdif in this driver.

> > +Child 'capture' and 'playback' required properties:
> > +- stream-name: name of the stream
> 
> What does this mean in the binding?

Indeed, the stream name could be implicit (either "Playback" or
"Capture"), but, as the users are aware of it, a more friendly
name is nicer ("S/PDIF Playback").

> > +Formats:
> > +	"s8"
> > +	"u8"
> 
> I'd cut this down to just the PCM formats for now - many of the formats
> you list there aren't terribly well defined even within ALSA, I don't
> think some of them have ever been used at all.  A binding document
> should really document what things mean as well.

I have no idea about which format is used or not, and there is no
explanation about their meanings in the uapi (but I can search them..).

Otherwise, for Dove, we need at least "s16_le", "s24_le" and "s32_le"
and, perhaps, "iec958_subframe_le" and "iec958_subframe_be".

So, what do you think should be the format set?

> > +Rates:
> > +	5512
> > +	8000
> 
> This shouldn't be a list of defined rates, it should allow any number,
> and it should support ranges since while some devices do enumerate
> specific rates simpler devices often just support a continuous range of
> rates.

There cannot be any number because the rates are coded by discrete
values in a bitmap.

For the continuous range, I was thinking about the value '0' followed
by the min and max rates. Have you any other syntax in mind?

> > --- a/sound/soc/generic/Kconfig
> > +++ b/sound/soc/generic/Kconfig
> 
> CODEC drivers should go in codecs.

OK.

> > +#define DRV_NAME "simple-codec"
> 
> Just write the string in the one place where it is used.

OK.

> > +static char *format_tb[] = {
> > +	"s8", "u8", "s16_le", "s16_be",
> > +	"u16_le", "u16_be", "s24_le", "s24_be",
> > +	"u24_le", "u24_be", "s32_le", "s32_be",
> > +	"u32_le", "u32_be", "float_le", "float_be",
> > +
> > +	"float64_le", "float64_be", "iec958_subframe_le", "iec958_subframe_be",
> > +	"mu_law", "a_law", "ima_adpcm", "mpeg",
> > +	"gsm", "", "", "",
> > +	"", "", "", "special",
> 
> This seems a bit obscure and fragile - an explicit lookup table would be
> better I think.

OK.

-- 
Ken ar c'hentañ	|	      ** Breizh ha Linux atav! **
Jef		|		http://moinejf.free.fr/
--
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] ASoC: generic: Add generic DT based simple codec

[Russell King - ARM Linux]

On Thu, Sep 19, 2013 at 07:34:03PM +0200, Jean-Francois Moine wrote:
> Otherwise, for Dove, we need at least "s16_le", "s24_le" and "s32_le"
> and, perhaps, "iec958_subframe_le" and "iec958_subframe_be".

The audio block can't do IEC958 subframes in the format ALSA wants
them.  The hardware format is different.  I've sent a commit removing
these foramts from sound/soc/kirkwood with a full explanation.  Mark
seems not to have merged even that one though.
--
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] ASoC: generic: Add generic DT based simple codec

[Mark Brown]

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

On Thu, Sep 19, 2013 at 07:34:03PM +0200, Jean-Francois Moine wrote:

> Well, first, the codecs hdmi and dmic have no DT support.
> Then, from the Cubox point of vue, the codec hdmi offers playback
> and capture while the Cubox has only playback (via the tda998x HDMI
> transmitter), and, also, the codec spdif transmitter has a lot of
> sample rates while the Dove audio device supports only 44.1, 48 and 96
> kHz.

All those restrictions should be constrained by either the machine
binding or the driver for the SoC side though.  I do see why you might
want this, I'm just a bit ambivalent about the merits.

> So, instead of adding new hdmi and spdif_transmitter codecs,
> I thought it could be useful to have a generic codec with DT support.

> On the other hand, as the first use of this codec is for the Cubox,
> an other solution could be to have the codec included in the kirkwood
> driver (declared by DT as either i2s or s/pdif). This would simplify
> the use or not of s/pdif in this driver.

That'd work too.

> > > +Child 'capture' and 'playback' required properties:
> > > +- stream-name: name of the stream

> > What does this mean in the binding?

> Indeed, the stream name could be implicit (either "Playback" or
> "Capture"), but, as the users are aware of it, a more friendly
> name is nicer ("S/PDIF Playback").

So you need to say that it's for display purposes.

> I have no idea about which format is used or not, and there is no
> explanation about their meanings in the uapi (but I can search them..).

> Otherwise, for Dove, we need at least "s16_le", "s24_le" and "s32_le"
> and, perhaps, "iec958_subframe_le" and "iec958_subframe_be".

> So, what do you think should be the format set?

PCM plus IEC seems fine, it's more the more obscure formats that ring
alarm bells.

> > > +Rates:
> > > +	5512
> > > +	8000

> > This shouldn't be a list of defined rates, it should allow any number,
> > and it should support ranges since while some devices do enumerate
> > specific rates simpler devices often just support a continuous range of
> > rates.

> There cannot be any number because the rates are coded by discrete
> values in a bitmap.

Not quite - _KNOT and _CONTINUOUS are options in that bitmap...  in
any case this would be a Linux implementation thing.

> For the continuous range, I was thinking about the value '0' followed
> by the min and max rates. Have you any other syntax in mind?

That'd probably work, not sure if there's a more idiomatic way of doing
that in DT though.

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

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

Re: [PATCH] ASoC: generic: Add generic DT based simple codec

[Mark Brown]

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

On Thu, Sep 19, 2013 at 06:49:52PM +0100, Russell King - ARM Linux wrote:
> On Thu, Sep 19, 2013 at 07:34:03PM +0200, Jean-Francois Moine wrote:
> > Otherwise, for Dove, we need at least "s16_le", "s24_le" and "s32_le"
> > and, perhaps, "iec958_subframe_le" and "iec958_subframe_be".

> The audio block can't do IEC958 subframes in the format ALSA wants
> them.  The hardware format is different.  I've sent a commit removing
> these foramts from sound/soc/kirkwood with a full explanation.  Mark
> seems not to have merged even that one though.

It's there in -next, it missed the merge window - there's no DAI in
mainline yet that would allow those formats anyway.

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

Re: [PATCH] ASoC: generic: Add generic DT based simple codec

[Stephen Warren]

On 09/19/2013 02:54 AM, Jean-Francois Moine wrote:
> This patch adds a simple sound codec which is described by the DT.
> This codec may be used when no specific codec action is needed.

> diff --git a/Documentation/devicetree/bindings/sound/simple-codec.txt b/Documentation/devicetree/bindings/sound/simple-codec.txt

> +Device-Tree bindings for the simple codec
> +
> +Required properties:
> +- compatible: should be "linux,simple-codec".

We shouldn't have any Linux-specific bindings. It might be reasonable to
define a binding for a "simple CODEC", but there's no reason it should
be Linux-specific.

> +- dai-name: name of the codec

That should be internal to the driver; it's Linux-specific.

> +Optional properties:
> +- capture: information about capture
> +- playback: information about playback
> +At least one of the 'capture' or 'playback' nodes must be present.

Judging by the example, those are nodes not properties.

> +Child 'capture' and 'playback' required properties:
> +- stream-name: name of the stream

Stream name is also probably too Linux-specific.

> +- formats: list of the supported formats (see below)
> +- rates: list of the supported rates (see below)
> +- #channels: minimum and maximum numbers of channels
> +
> +Formats:
> +	"s8"
> +	"u8"

I think the binding should specify on-the-wire formats, not in-memory
formats. For example, if this binding is used to represent an I2S CODEC,
it's more important to know whether it supports left-justified,
right-justified, DSP mode, etc., and the max number of bits (or perhaps
a list of supported numbers of bits) per channel in each case.

Re: [PATCH] ASoC: generic: Add generic DT based simple codec

[Mark Brown]

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

On Mon, Sep 23, 2013 at 03:19:56PM -0600, Stephen Warren wrote:
> On 09/19/2013 02:54 AM, Jean-Francois Moine wrote:

> > +Required properties:
> > +- compatible: should be "linux,simple-codec".

> We shouldn't have any Linux-specific bindings. It might be reasonable to
> define a binding for a "simple CODEC", but there's no reason it should
> be Linux-specific.

What's the vendor prefix to use then?  The same thought did occur to me
but I wasn't sure what to suggest instead.

> > +- dai-name: name of the codec

> That should be internal to the driver; it's Linux-specific.

I do think it's reasonable to define a name for display purposes however
this binding doesn't say that, I did say something to that effect
already IIRC.

> > +Formats:
> > +	"s8"
> > +	"u8"

> I think the binding should specify on-the-wire formats, not in-memory
> formats. For example, if this binding is used to represent an I2S CODEC,
> it's more important to know whether it supports left-justified,
> right-justified, DSP mode, etc., and the max number of bits (or perhaps
> a list of supported numbers of bits) per channel in each case.

I didn't query this myself since my expecation with this is that the
rest of the wire format properties will come from the binding that pulls
this together into a card (since this will need to be set at that level
for other CODECs which are more flexible) so I saw this as providing
additional information which would normally not be fixed at that level.
Arguably this could be pushed up there too, though it's not normal to
fix the word size.

We should really be defining things like signed and unsigned as well as
the bits per word since they do make a difference, though in practice
you very rarely see anything except unsigned and some of the formats
listed don't seem to have ever even been used in Linux.

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

Re: [PATCH] ASoC: generic: Add generic DT based simple codec

[Stephen Warren]

On 09/23/2013 05:26 PM, Mark Brown wrote:
> On Mon, Sep 23, 2013 at 03:19:56PM -0600, Stephen Warren wrote:
>> On 09/19/2013 02:54 AM, Jean-Francois Moine wrote:
> 
>>> +Required properties: +- compatible: should be
>>> "linux,simple-codec".
> 
>> We shouldn't have any Linux-specific bindings. It might be
>> reasonable to define a binding for a "simple CODEC", but there's
>> no reason it should be Linux-specific.
> 
> What's the vendor prefix to use then?  The same thought did occur
> to me but I wasn't sure what to suggest instead.

For other non-vendor-specific/generic bindings, we've simply omitted
the vendor prefix from the compatible value, e.g.: gpio-keys,
i2c-gpio, regulator-fixed, etc.
--
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] ASoC: generic: Add generic DT based simple codec

[Rob Herring]

On Mon, Sep 23, 2013 at 4:19 PM, Stephen Warren <swarren-3lzwWm7+Weoh9ZMKESR00Q@public.gmane.org> wrote:
> On 09/19/2013 02:54 AM, Jean-Francois Moine wrote:
>> This patch adds a simple sound codec which is described by the DT.
>> This codec may be used when no specific codec action is needed.
>
>> diff --git a/Documentation/devicetree/bindings/sound/simple-codec.txt b/Documentation/devicetree/bindings/sound/simple-codec.txt
>
>> +Device-Tree bindings for the simple codec
>> +
>> +Required properties:
>> +- compatible: should be "linux,simple-codec".
>
> We shouldn't have any Linux-specific bindings. It might be reasonable to
> define a binding for a "simple CODEC", but there's no reason it should
> be Linux-specific.

Further, just define the type of codec h/w. If linux can use its
simple-codec driver for it, then add a match table entry for said
codec. Then the DT is future proof when you decide the simple driver
doesn't really work for that h/w.

Rob
--
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] ASoC: generic: Add generic DT based simple codec

[Mark Brown]

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

On Thu, Sep 26, 2013 at 09:57:30PM -0500, Rob Herring wrote:
> On Mon, Sep 23, 2013 at 4:19 PM, Stephen Warren <swarren@wwwdotorg.org> wrote:

> > We shouldn't have any Linux-specific bindings. It might be reasonable to
> > define a binding for a "simple CODEC", but there's no reason it should
> > be Linux-specific.

> Further, just define the type of codec h/w. If linux can use its
> simple-codec driver for it, then add a match table entry for said
> codec. Then the DT is future proof when you decide the simple driver
> doesn't really work for that h/w.

So, this is what I keep telling people to do but lots of people don't
want to do this - it eliminates the need have this binding at all since
if you know which type of device you have there's no need to specify
fixed properties of the device via DT.

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

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

Re: [PATCH] ASoC: generic: Add generic DT based simple codec

[Stephen Warren]

On 09/26/2013 08:57 PM, Rob Herring wrote:
> On Mon, Sep 23, 2013 at 4:19 PM, Stephen Warren <swarren-3lzwWm7+Weoh9ZMKESR00Q@public.gmane.org> wrote:
>> On 09/19/2013 02:54 AM, Jean-Francois Moine wrote:
>>> This patch adds a simple sound codec which is described by the DT.
>>> This codec may be used when no specific codec action is needed.
>>
>>> diff --git a/Documentation/devicetree/bindings/sound/simple-codec.txt b/Documentation/devicetree/bindings/sound/simple-codec.txt
>>
>>> +Device-Tree bindings for the simple codec
>>> +
>>> +Required properties:
>>> +- compatible: should be "linux,simple-codec".
>>
>> We shouldn't have any Linux-specific bindings. It might be reasonable to
>> define a binding for a "simple CODEC", but there's no reason it should
>> be Linux-specific.
> 
> Further, just define the type of codec h/w. If linux can use its
> simple-codec driver for it, then add a match table entry for said
> codec. Then the DT is future proof when you decide the simple driver
> doesn't really work for that h/w.

The only potential issue with that approach is if the CODEC can be used
in a "simple" mode where e.g. no control GPIOs, regulator manipulation,
etc. are required, or in a more complex mode where such things are
required, you may end up with the device-specific binding being written
to the simple mode, and hence missing features required for the complex
mode. Having a separate simple binding prevents the device-specific
binding from being broken that way.

That said, it's probably better to simply get the device-specific
binding completely fleshed out and representing all resources the first
time around, then there's no issue either.

--
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] ASoC: generic: Add generic DT based simple codec

[Mark Brown]

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

On Mon, Sep 30, 2013 at 10:49:06AM -0600, Stephen Warren wrote:

> The only potential issue with that approach is if the CODEC can be used
> in a "simple" mode where e.g. no control GPIOs, regulator manipulation,
> etc. are required, or in a more complex mode where such things are
> required, you may end up with the device-specific binding being written
> to the simple mode, and hence missing features required for the complex
> mode. Having a separate simple binding prevents the device-specific
> binding from being broken that way.

That would probably indicate a bug in the complex bindings since if the
simple mode exists then usually that would mean that the bindings for
the complex mode ought to be specifying things as optional, these things
are typically just done by strapping pins instead of routing them to the
CPU.

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