[v2,15/18] dt-bindings: sound: Add bindings for Cirrus Logic Madera codecs

Message ID 1493050124-5970-16-git-send-email-rf@opensource.wolfsonmicro.com
State Changes Requested, archived
Headers show

Commit Message

Richard Fitzgerald April 24, 2017, 4:08 p.m.
The Cirrus Logic Madera codecs are a family of related codecs with
extensive digital and analogue I/O, digital mixing and routing,
signal processing and programmable DSPs.

Signed-off-by: Richard Fitzgerald <rf@opensource.wolfsonmicro.com>
---
Changes since V1:
- these bindings split out from the main source patch
- some minor edits to the property descriptions to match the pdata
  descriptions

 Documentation/devicetree/bindings/sound/madera.txt | 66 ++++++++++++++++++++++
 1 file changed, 66 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/sound/madera.txt

Comments

Mark Brown April 25, 2017, 3:52 p.m. | #1
On Mon, Apr 24, 2017 at 05:08:41PM +0100, Richard Fitzgerald wrote:
> The Cirrus Logic Madera codecs are a family of related codecs with
> extensive digital and analogue I/O, digital mixing and routing,
> signal processing and programmable DSPs.

Please submit patches using subject lines reflecting the style for the
subsystem.  This makes it easier for people to identify relevant
patches.  Look at what existing commits in the area you're changing are
doing and make sure your subject lines visually resemble what they're
doing.

> +Required properties:
> +  - compatible : One of the following chip-specific strings:
> +        "cirrus,cs47l35-codec"
> +        "cirrus,cs47l85-codec"
> +        "cirrus,cs47l90-codec"

You shouldn't have compatible strings for subfunctions of a MFD unless
these represent meaningful reusable IPs that can exist separately from
the parent chip, that's clearly not the case here.  All you're doing
here is encoding Linux internal abstractions which aren't OS neutral and
might change in future (for example clocking might move more into the
clock API).
Richard Fitzgerald April 25, 2017, 4:27 p.m. | #2
On Tue, 2017-04-25 at 16:52 +0100, Mark Brown wrote:
> On Mon, Apr 24, 2017 at 05:08:41PM +0100, Richard Fitzgerald wrote:
> > The Cirrus Logic Madera codecs are a family of related codecs with
> > extensive digital and analogue I/O, digital mixing and routing,
> > signal processing and programmable DSPs.
> 
> Please submit patches using subject lines reflecting the style for the
> subsystem.  This makes it easier for people to identify relevant
> patches.  Look at what existing commits in the area you're changing are
> doing and make sure your subject lines visually resemble what they're
> doing.
> 
> > +Required properties:
> > +  - compatible : One of the following chip-specific strings:
> > +        "cirrus,cs47l35-codec"
> > +        "cirrus,cs47l85-codec"
> > +        "cirrus,cs47l90-codec"
> 
> You shouldn't have compatible strings for subfunctions of a MFD unless
> these represent meaningful reusable IPs that can exist separately from
> the parent chip, that's clearly not the case here.  All you're doing
> here is encoding Linux internal abstractions which aren't OS neutral and
> might change in future (for example clocking might move more into the
> clock API).

While that's nice, the of_node doesn't get populated if there isn't a
compatible string. And people don't like workarounds for the missing
of_node.

--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Rob Herring April 28, 2017, 6:06 p.m. | #3
On Tue, Apr 25, 2017 at 04:52:57PM +0100, Mark Brown wrote:
> On Mon, Apr 24, 2017 at 05:08:41PM +0100, Richard Fitzgerald wrote:
> > The Cirrus Logic Madera codecs are a family of related codecs with
> > extensive digital and analogue I/O, digital mixing and routing,
> > signal processing and programmable DSPs.
> 
> Please submit patches using subject lines reflecting the style for the
> subsystem.  This makes it easier for people to identify relevant
> patches.  Look at what existing commits in the area you're changing are
> doing and make sure your subject lines visually resemble what they're
> doing.
> 
> > +Required properties:
> > +  - compatible : One of the following chip-specific strings:
> > +        "cirrus,cs47l35-codec"
> > +        "cirrus,cs47l85-codec"
> > +        "cirrus,cs47l90-codec"
> 
> You shouldn't have compatible strings for subfunctions of a MFD unless
> these represent meaningful reusable IPs that can exist separately from
> the parent chip, that's clearly not the case here.  All you're doing
> here is encoding Linux internal abstractions which aren't OS neutral and
> might change in future (for example clocking might move more into the
> clock API).

s/compatible strings/child nodes/ and I agree. If you have child nodes, 
then they need to have compatible strings so we can tell what child is 
which. Node name can be used, but there's no way to version node names. 
A compatible implies what properties are valid.

Rob

--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Mark Brown May 14, 2017, 10:04 a.m. | #4
On Tue, Apr 25, 2017 at 05:27:44PM +0100, Richard Fitzgerald wrote:
> On Tue, 2017-04-25 at 16:52 +0100, Mark Brown wrote:

> > > +Required properties:
> > > +  - compatible : One of the following chip-specific strings:
> > > +        "cirrus,cs47l35-codec"
> > > +        "cirrus,cs47l85-codec"
> > > +        "cirrus,cs47l90-codec"

> > You shouldn't have compatible strings for subfunctions of a MFD unless
> > these represent meaningful reusable IPs that can exist separately from
> > the parent chip, that's clearly not the case here.  All you're doing
> > here is encoding Linux internal abstractions which aren't OS neutral and
> > might change in future (for example clocking might move more into the
> > clock API).

> While that's nice, the of_node doesn't get populated if there isn't a
> compatible string. And people don't like workarounds for the missing
> of_node.

What workarounds are you referring to?  Why would this need any kind of
workaround, there is no requirement for magic broken nodes like this in
the subsystem and if there is we should fix that rather than bodge the
ABI.

Patch

diff --git a/Documentation/devicetree/bindings/sound/madera.txt b/Documentation/devicetree/bindings/sound/madera.txt
new file mode 100644
index 0000000..b114f7b
--- /dev/null
+++ b/Documentation/devicetree/bindings/sound/madera.txt
@@ -0,0 +1,66 @@ 
+Cirrus Logic Madera class audio codecs
+
+This is a subnode of the parent mfd node.
+
+See also the core bindings for the parent MFD driver:
+See Documentation/devicetree/bindings/mfd/madera.txt
+
+Required properties:
+  - compatible : One of the following chip-specific strings:
+        "cirrus,cs47l35-codec"
+        "cirrus,cs47l85-codec"
+        "cirrus,cs47l90-codec"
+
+Optional properties:
+  - cirrus,dmic-ref : DMIC bias reference for each input, one cell per input
+    <IN1 IN2 IN3 ...>
+    A value of 0 indicates MICVDD and is the default, other values depend on the
+    codec - see the datasheet for the INn_DMIC_SUP field
+
+  - cirrus,inmode : A list of input mode settings for each input. A maximum of
+    16 cells, with four cells per input in the order INnAL, INnAR INnBL INnBR.
+    For non-muxed inputs the first two cells for that input set the mode for
+    the left and right channel and the second two cells must be 0.
+    For muxed inputs the first two cells for that input set the mode of the
+    left and right A inputs and the second two cells set the mode of the left
+    and right B inputs.
+    Valid mode values are one of the MADERA_INMODE_xxx (see
+    include/dt-bindings/sound/madera.h). If the array is shorter than the number
+    of inputs the unspecified inputs default to MADERA_INMODE_DIFF.
+
+  - cirrus,out-mono : Mono bit for each output, must contain six cells if
+    specified. A non-zero value indicates the corresponding output is mono.
+
+  - cirrus,max-channels-clocked : Maximum number of channels that I2S clocks
+    will be generated for. Useful when clock master for systems where the I2S
+    bus has multiple data lines.
+    One cell for each AIF, use a value of zero for AIFs that should be handled
+    normally.
+
+  - cirrus,pdm-fmt : PDM speaker data format, must contain 2 cells
+    (OUT5 and OUT6). See the PDM_SPKn_FMT field in the datasheet for a
+    description of this value.
+    The second cell is ignored for codecs that do not have OUT6.
+
+  - cirrus,pdm-mute : PDM mute format, must contain 2 cells
+    (OUT5 and OUT6). See the PDM_SPKn_CTRL_1 register in the datasheet for a
+    description of this value.
+    The second cell is ignored for codecs that do not have OUT6.
+
+Example:
+
+codec: cs47l35@0 {
+	compatible = "cirrus,cs47l35";
+
+	codec {
+		compatible = "cirrus,cs47l35-codec";
+		cirrus,dmic-ref = <0 0 1 0>;
+		cirrus,dmic-clksrc = <0 0 0 0>;
+		cirrus,inmode = <
+			2 2 1 1 /* IN1A digital, IN1B single-ended */
+			0 0 0 0	/* IN2 differential */
+		>;
+		cirrus,out-mono = <0 0 0 0 0 0>;
+		cirrus,max-channels-clocked = <2 0 0>;
+	};
+};