[v2,3/3] ASoC: da7213: Add bindings documentation for codec driver
diff mbox

Message ID bfc7c5e61d8c398bc4cfa56a7908032dc3bdf82f.1444223905.git.Adam.Thomson.Opensource@diasemi.com
State New
Headers show

Commit Message

Adam Thomson Oct. 7, 2015, 1:27 p.m. UTC
Signed-off-by: Adam Thomson <Adam.Thomson.Opensource@diasemi.com>
---
 Documentation/devicetree/bindings/sound/da7213.txt | 41 ++++++++++++++++++++++
 1 file changed, 41 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/sound/da7213.txt

--
1.9.3

Comments

Rob Herring Oct. 7, 2015, 4:22 p.m. UTC | #1
On Wed, Oct 7, 2015 at 8:27 AM, Adam Thomson
<Adam.Thomson.Opensource@diasemi.com> wrote:
> Signed-off-by: Adam Thomson <Adam.Thomson.Opensource@diasemi.com>
> ---
>  Documentation/devicetree/bindings/sound/da7213.txt | 41 ++++++++++++++++++++++
>  1 file changed, 41 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/sound/da7213.txt
>
> diff --git a/Documentation/devicetree/bindings/sound/da7213.txt b/Documentation/devicetree/bindings/sound/da7213.txt
> new file mode 100644
> index 0000000..7280e82
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/sound/da7213.txt
> @@ -0,0 +1,41 @@
> +Dialog Semiconductor DA7213 Audio Codec bindings
> +
> +======
> +
> +Required properties:
> +- compatible : Should be "dlg,da7213"
> +- reg: Specifies the I2C slave address
> +
> +Optional properties:
> +- clocks : phandle and clock specifier for codec MCLK.
> +- clock-names : Clock name string for 'clocks' attribute, should be "mclk".
> +
> +- dlg,micbias1-lvl : Voltage (mV) for Mic Bias 1
> +       [<1600>, <2200>, <2500>, <3000>]
> +- dlg,micbias2-lvl : Voltage (mV) for Mic Bias 2
> +       [<1600>, <2200>, <2500>, <3000>]

Please append the units (-microvolt).

> +- dlg,dmic-data-sel : DMIC channel select based on clock edge.
> +       ["lrise_rfall", "lfall_rrise"]
> +- dlg,dmic-samplephase : When to sample audio from DMIC.
> +       ["on_clkedge", "between_clkedge"]

How about boolean for these two.

> +- dlg,dmic-clkrate : DMIC clock frequency (MHz).
> +       [<1500000>, <3000000>]

So 1.5GHz or 3GHz?

Add units (-hz).

> +
> +======
> +
> +Example:
> +
> +       codec_i2c: da7213@1a {
> +               compatible = "dlg,da7213";
> +               reg = <0x1a>;
> +
> +               clocks = <&clks 201>;
> +               clock-names = "mclk";
> +
> +               dlg,micbias1-lvl = <2500>;
> +               dlg,micbias2-lvl = <2500>;
> +
> +               dlg,dmic-data-sel = "lrise_rfall";
> +               dlg,dmic-samplephase = "between_clkedge";
> +               dlg,dmic-clkrate = <3000000>;
> +       };
> --
> 1.9.3
>
Adam Thomson Oct. 8, 2015, 8:42 a.m. UTC | #2
On October 07, 2015 17:22, Rob Herring wrote:

> > +- dlg,micbias1-lvl : Voltage (mV) for Mic Bias 1
> > +       [<1600>, <2200>, <2500>, <3000>]
> > +- dlg,micbias2-lvl : Voltage (mV) for Mic Bias 2
> > +       [<1600>, <2200>, <2500>, <3000>]
> 
> Please append the units (-microvolt).

Given that a user needs to read the bindings document to understand what is
available, and what they're for, this seems a little unnecessary. 

> 
> > +- dlg,dmic-data-sel : DMIC channel select based on clock edge.
> > +       ["lrise_rfall", "lfall_rrise"]
> > +- dlg,dmic-samplephase : When to sample audio from DMIC.
> > +       ["on_clkedge", "between_clkedge"]
> 
> How about boolean for these two.

Wanted these to be explicit, hence not choosing boolean. Would prefer to keep
them as is.

> 
> > +- dlg,dmic-clkrate : DMIC clock frequency (MHz).
> > +       [<1500000>, <3000000>]
> 
> So 1.5GHz or 3GHz?
> 
> Add units (-hz).

Agreed, the description of MHz is misleading so will update to Hz. Thanks.
Again though, I don't see what the suffix gives you, and would prefer to leave
the binding as is.
Rob Herring Oct. 8, 2015, 1:16 p.m. UTC | #3
On Thu, Oct 8, 2015 at 3:42 AM, Opensource [Adam Thomson]
<Adam.Thomson.Opensource@diasemi.com> wrote:
> On October 07, 2015 17:22, Rob Herring wrote:
>
>> > +- dlg,micbias1-lvl : Voltage (mV) for Mic Bias 1
>> > +       [<1600>, <2200>, <2500>, <3000>]
>> > +- dlg,micbias2-lvl : Voltage (mV) for Mic Bias 2
>> > +       [<1600>, <2200>, <2500>, <3000>]
>>
>> Please append the units (-microvolt).
>
> Given that a user needs to read the bindings document to understand what is
> available, and what they're for, this seems a little unnecessary.

You may think so, but it is standard practice.

>>
>> > +- dlg,dmic-data-sel : DMIC channel select based on clock edge.
>> > +       ["lrise_rfall", "lfall_rrise"]
>> > +- dlg,dmic-samplephase : When to sample audio from DMIC.
>> > +       ["on_clkedge", "between_clkedge"]
>>
>> How about boolean for these two.
>
> Wanted these to be explicit, hence not choosing boolean. Would prefer to keep
> them as is.
>
>>
>> > +- dlg,dmic-clkrate : DMIC clock frequency (MHz).
>> > +       [<1500000>, <3000000>]
>>
>> So 1.5GHz or 3GHz?
>>
>> Add units (-hz).
>
> Agreed, the description of MHz is misleading so will update to Hz. Thanks.
> Again though, I don't see what the suffix gives you, and would prefer to leave
> the binding as is.

It tells someone reading the dts what the units are without having to
find the documentation and helps prevent people using properties with
differing units.

Rob
Adam Thomson Oct. 8, 2015, 4:31 p.m. UTC | #4
On October 08, 2015 14:16, Rob Herring wrote:

> >> > +- dlg,micbias1-lvl : Voltage (mV) for Mic Bias 1
> >> > +       [<1600>, <2200>, <2500>, <3000>]
> >> > +- dlg,micbias2-lvl : Voltage (mV) for Mic Bias 2
> >> > +       [<1600>, <2200>, <2500>, <3000>]
> >>
> >> Please append the units (-microvolt).
> >
> > Given that a user needs to read the bindings document to understand what is
> > available, and what they're for, this seems a little unnecessary.
> 
> You may think so, but it is standard practice.
 
I can find a number of situations in the kernel, where this just isn't followed,
for device specific bindings. What percentage have to follow this for it to be
'standard'? Is this documented somewhere so it's clear for those writing
drivers which need to use DT bindings? I had a quick look and couldn't find
anything on this.

> >>
> >> > +- dlg,dmic-clkrate : DMIC clock frequency (MHz).
> >> > +       [<1500000>, <3000000>]
> >>
> >> So 1.5GHz or 3GHz?
> >>
> >> Add units (-hz).
> >
> > Agreed, the description of MHz is misleading so will update to Hz. Thanks.
> > Again though, I don't see what the suffix gives you, and would prefer to leave
> > the binding as is.
> 
> It tells someone reading the dts what the units are without having to
> find the documentation and helps prevent people using properties with
> differing units.

For a lot of device specific bindings, you will need to read the associated
documentation, and possibly the datasheet, to understand what it's for, and what
values are valid. I don't see this suffix really saving you any time. For
standard frameworks ok, but here it seems overkill.

Patch
diff mbox

diff --git a/Documentation/devicetree/bindings/sound/da7213.txt b/Documentation/devicetree/bindings/sound/da7213.txt
new file mode 100644
index 0000000..7280e82
--- /dev/null
+++ b/Documentation/devicetree/bindings/sound/da7213.txt
@@ -0,0 +1,41 @@ 
+Dialog Semiconductor DA7213 Audio Codec bindings
+
+======
+
+Required properties:
+- compatible : Should be "dlg,da7213"
+- reg: Specifies the I2C slave address
+
+Optional properties:
+- clocks : phandle and clock specifier for codec MCLK.
+- clock-names : Clock name string for 'clocks' attribute, should be "mclk".
+
+- dlg,micbias1-lvl : Voltage (mV) for Mic Bias 1
+	[<1600>, <2200>, <2500>, <3000>]
+- dlg,micbias2-lvl : Voltage (mV) for Mic Bias 2
+	[<1600>, <2200>, <2500>, <3000>]
+- dlg,dmic-data-sel : DMIC channel select based on clock edge.
+	["lrise_rfall", "lfall_rrise"]
+- dlg,dmic-samplephase : When to sample audio from DMIC.
+	["on_clkedge", "between_clkedge"]
+- dlg,dmic-clkrate : DMIC clock frequency (MHz).
+	[<1500000>, <3000000>]
+
+======
+
+Example:
+
+	codec_i2c: da7213@1a {
+		compatible = "dlg,da7213";
+ 		reg = <0x1a>;
+
+ 		clocks = <&clks 201>;
+		clock-names = "mclk";
+
+		dlg,micbias1-lvl = <2500>;
+		dlg,micbias2-lvl = <2500>;
+
+		dlg,dmic-data-sel = "lrise_rfall";
+		dlg,dmic-samplephase = "between_clkedge";
+		dlg,dmic-clkrate = <3000000>;
+	};