[PATCH v2 1/2] ASoC: cs42l52: Add DeviceTree Support for CS42L52

[PATCH v2 1/2] ASoC: cs42l52: Add DeviceTree Support for CS42L52

[Brian Austin]

This patch adds device tree support for the CS42L52 CODEC

v2 changes dt bindings to use '-' instead of '_'.
reset_gpio = reset-gpio. Pdata variables are not changed.

Signed-off-by: Brian Austin <brian.austin-jGc1dHjMKG3QT0dZR+AlfA@public.gmane.org>
---
 sound/soc/codecs/cs42l52.c | 51 ++++++++++++++++++++++++++++++++++++++++++++--
 1 file changed, 49 insertions(+), 2 deletions(-)

diff --git a/sound/soc/codecs/cs42l52.c b/sound/soc/codecs/cs42l52.c
index 8b427c9..ea55ee5 100644
--- a/sound/soc/codecs/cs42l52.c
+++ b/sound/soc/codecs/cs42l52.c
@@ -17,7 +17,7 @@
 #include <linux/kernel.h>
 #include <linux/init.h>
 #include <linux/delay.h>
-#include <linux/gpio.h>
+#include <linux/of_gpio.h>
 #include <linux/pm.h>
 #include <linux/i2c.h>
 #include <linux/input.h>
@@ -1174,6 +1174,7 @@ static int cs42l52_i2c_probe(struct i2c_client *i2c_client,
 	struct cs42l52_private *cs42l52;
 	struct cs42l52_platform_data *pdata = dev_get_platdata(&i2c_client->dev);
 	int ret;
+	u32 val32;
 	unsigned int devid = 0;
 	unsigned int reg;
 
@@ -1190,8 +1191,47 @@ static int cs42l52_i2c_probe(struct i2c_client *i2c_client,
 		return ret;
 	}
 
-	if (pdata)
+	if (pdata) {
 		cs42l52->pdata = *pdata;
+	} else {
+		pdata = devm_kzalloc(&i2c_client->dev,
+				     sizeof(struct cs42l52_platform_data),
+				GFP_KERNEL);
+		if (!pdata) {
+			dev_err(&i2c_client->dev, "could not allocate pdata\n");
+			return -ENOMEM;
+		}
+		if (i2c_client->dev.of_node) {
+			if (of_property_read_u32(i2c_client->dev.of_node,
+				"mica-cfg", &val32) >= 0)
+				pdata->mica_cfg = val32;
+
+			if (of_property_read_u32(i2c_client->dev.of_node,
+				"micb-cfg", &val32) >= 0)
+				pdata->micb_cfg = val32;
+
+			if (of_property_read_u32(i2c_client->dev.of_node,
+				"mica-sel", &val32) >= 0)
+				pdata->mica_sel = val32;
+
+			if (of_property_read_u32(i2c_client->dev.of_node,
+				"micb-sel", &val32) >= 0)
+				pdata->micb_sel = val32;
+
+			if (of_property_read_u32(i2c_client->dev.of_node,
+				"micbias-lvl", &val32) >= 0)
+				pdata->micbias_lvl = val32;
+
+			if (of_property_read_u32(i2c_client->dev.of_node,
+				"chgfreq", &val32) >= 0)
+				pdata->chgfreq = val32;
+
+			pdata->reset_gpio =
+				of_get_named_gpio(i2c_client->dev.of_node,
+						"reset-gpio", 0);
+		}
+		cs42l52->pdata = *pdata;
+	}
 
 	if (cs42l52->pdata.reset_gpio) {
 		ret = gpio_request_one(cs42l52->pdata.reset_gpio,
@@ -1274,6 +1314,12 @@ static int cs42l52_i2c_remove(struct i2c_client *client)
 	return 0;
 }
 
+static const struct of_device_id cs42l52_of_match[] = {
+	{ .compatible = "cirrus,cs42l52", },
+	{},
+};
+MODULE_DEVICE_TABLE(of, cs42l52_of_match);
+
 static const struct i2c_device_id cs42l52_id[] = {
 	{ "cs42l52", 0 },
 	{ }
@@ -1284,6 +1330,7 @@ static struct i2c_driver cs42l52_i2c_driver = {
 	.driver = {
 		.name = "cs42l52",
 		.owner = THIS_MODULE,
+		.of_match_table = cs42l52_of_match,
 	},
 	.id_table = cs42l52_id,
 	.probe =    cs42l52_i2c_probe,
-- 
1.8.4.rc3


--
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 v2 2/2] dt: binding: sound cs42l52 driver

[Brian Austin]

Add Device Tree Binding for the CS42L52 Codec

v2 Adds clearer explaination of mic configs and select.
Renames bindings to '-' instead of '_'.
Definition of GPIO for reset pin.

Signed-off-by: Brian Austin <brian.austin-jGc1dHjMKG3QT0dZR+AlfA@public.gmane.org>
---
 .../devicetree/bindings/sound/cs42l52.txt          | 48 ++++++++++++++++++++++
 1 file changed, 48 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/sound/cs42l52.txt

diff --git a/Documentation/devicetree/bindings/sound/cs42l52.txt b/Documentation/devicetree/bindings/sound/cs42l52.txt
new file mode 100644
index 0000000..bd91ecf
--- /dev/null
+++ b/Documentation/devicetree/bindings/sound/cs42l52.txt
@@ -0,0 +1,48 @@
+CS42L52 audio CODEC
+
+Required properties:
+
+  - compatible : "cirrus,cs42l52"
+
+  - reg : the I2C address of the device for I2C
+
+Optional properties:
+
+  - reset-gpio : GPIO controller's phandle and the number
+		 of the GPIO used to reset the codec.
+  - chgfreq    : Charge Pump Frequency values. Allowable values of
+		 0x00 through 0x0F.
+		 Frequency = (64xFs)/(N+2)
+  - mica-cfg   : MIC A single-ended or differential select.
+		 0x00 = Single-Ended
+		 0x01 = Differential
+  - micb-cfg   : MIC B single-ended or differential select.
+		 0x00 = Single-Ended
+		 0x01 = Differential
+  - mica-sel   : MIC A single ended input select.  For Single-Ended
+		 configuration, select which MIC to use.
+		 0x00 = MIC1
+		 0x01 = MIC2
+  - micb-sel   : MIC B single ended input select.  For Single-Ended
+		 configuration, select which MIC to use.
+		 0x00 = MIC1
+		 0x01 = MIC2
+  - micbias-lvl: Set the output voltage level on the MICBIAS Pin
+		 0x00 = 0.5 x VA
+		 0x01 = 0.6 x VA
+		 0x02 = 0.7 x VA
+		 0x03 = 0.8 x VA
+		 0x04 = 0.83 x VA
+		 0x05 = 0.91 x VA
+
+Example:
+
+codec: cs42l52@4a {
+	compatible = "cirrus,cs42l52";
+	reg = <0x4a>;
+	reset-gpio = <&gpio 10 0>;
+	chgfreq = <0x05>;
+	mica-cfg = <0x00>;
+	mica-sel = <0x01>;
+	micbias-lvl = <0x05>;
+};
-- 
1.8.4.rc3


--
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 v2 2/2] dt: binding: sound cs42l52 driver

[Kumar Gala]

On Nov 13, 2013, at 9:44 AM, Brian Austin <brian.austin-jGc1dHjMKG3QT0dZR+AlfA@public.gmane.org> wrote:

> Add Device Tree Binding for the CS42L52 Codec
> 
> v2 Adds clearer explaination of mic configs and select.
> Renames bindings to '-' instead of '_'.
> Definition of GPIO for reset pin.
> 
> Signed-off-by: Brian Austin <brian.austin-jGc1dHjMKG3QT0dZR+AlfA@public.gmane.org>
> ---
> .../devicetree/bindings/sound/cs42l52.txt          | 48 ++++++++++++++++++++++
> 1 file changed, 48 insertions(+)
> create mode 100644 Documentation/devicetree/bindings/sound/cs42l52.txt
> 
> diff --git a/Documentation/devicetree/bindings/sound/cs42l52.txt b/Documentation/devicetree/bindings/sound/cs42l52.txt
> new file mode 100644
> index 0000000..bd91ecf
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/sound/cs42l52.txt
> @@ -0,0 +1,48 @@
> +CS42L52 audio CODEC
> +
> +Required properties:
> +
> +  - compatible : "cirrus,cs42l52"
> +
> +  - reg : the I2C address of the device for I2C
> +
> +Optional properties:
> +
> +  - reset-gpio : GPIO controller's phandle and the number
> +		 of the GPIO used to reset the codec.
> +  - chgfreq    : Charge Pump Frequency values. Allowable values of
> +		 0x00 through 0x0F.
> +		 Frequency = (64xFs)/(N+2)
> +  - mica-cfg   : MIC A single-ended or differential select.
> +		 0x00 = Single-Ended
> +		 0x01 = Differential
> +  - micb-cfg   : MIC B single-ended or differential select.
> +		 0x00 = Single-Ended
> +		 0x01 = Differential
> +  - mica-sel   : MIC A single ended input select.  For Single-Ended
> +		 configuration, select which MIC to use.
> +		 0x00 = MIC1
> +		 0x01 = MIC2
> +  - micb-sel   : MIC B single ended input select.  For Single-Ended
> +		 configuration, select which MIC to use.
> +		 0x00 = MIC1
> +		 0x01 = MIC2
> +  - micbias-lvl: Set the output voltage level on the MICBIAS Pin
> +		 0x00 = 0.5 x VA
> +		 0x01 = 0.6 x VA
> +		 0x02 = 0.7 x VA
> +		 0x03 = 0.8 x VA
> +		 0x04 = 0.83 x VA
> +		 0x05 = 0.91 x VA

these properties should be cirrus, prefixed.

> +
> +Example:
> +
> +codec: cs42l52@4a {
> +	compatible = "cirrus,cs42l52";
> +	reg = <0x4a>;
> +	reset-gpio = <&gpio 10 0>;
> +	chgfreq = <0x05>;
> +	mica-cfg = <0x00>;
> +	mica-sel = <0x01>;
> +	micbias-lvl = <0x05>;
> +};
> -- 
> 1.8.4.rc3
> 
> 
> --
> 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

-- 
Employee of Qualcomm Innovation Center, Inc.
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, hosted by The Linux Foundation

--
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 v2 2/2] dt: binding: sound cs42l52 driver

[Brian Austin]

>> +Required properties:
>> +
>> +  - compatible : "cirrus,cs42l52"
>> +
>> +  - reg : the I2C address of the device for I2C
>> +
>> +Optional properties:
>> +
>> +  - reset-gpio : GPIO controller's phandle and the number
>> +		 of the GPIO used to reset the codec.
>> +  - chgfreq    : Charge Pump Frequency values. Allowable values of
>> +		 0x00 through 0x0F.
>> +		 Frequency = (64xFs)/(N+2)
>> +  - mica-cfg   : MIC A single-ended or differential select.
>> +		 0x00 = Single-Ended
>> +		 0x01 = Differential
>> +  - micb-cfg   : MIC B single-ended or differential select.
>> +		 0x00 = Single-Ended
>> +		 0x01 = Differential
>> +  - mica-sel   : MIC A single ended input select.  For Single-Ended
>> +		 configuration, select which MIC to use.
>> +		 0x00 = MIC1
>> +		 0x01 = MIC2
>> +  - micb-sel   : MIC B single ended input select.  For Single-Ended
>> +		 configuration, select which MIC to use.
>> +		 0x00 = MIC1
>> +		 0x01 = MIC2
>> +  - micbias-lvl: Set the output voltage level on the MICBIAS Pin
>> +		 0x00 = 0.5 x VA
>> +		 0x01 = 0.6 x VA
>> +		 0x02 = 0.7 x VA
>> +		 0x03 = 0.8 x VA
>> +		 0x04 = 0.83 x VA
>> +		 0x05 = 0.91 x VA
>
> these properties should be cirrus, prefixed.
>

This is for a Cirrus CODEC, not related to the Cirrus ARM9. I didn't think 
we would prefix for a binding on a codec. I thought that was only for 
SOC/Platforms


--
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 v2 2/2] dt: binding: sound cs42l52 driver

[Tomasz Figa]

Hi Brian,

On Wednesday 13 of November 2013 09:44:34 Brian Austin wrote:
> Add Device Tree Binding for the CS42L52 Codec
> 
> v2 Adds clearer explaination of mic configs and select.
> Renames bindings to '-' instead of '_'.
> Definition of GPIO for reset pin.
> 
> Signed-off-by: Brian Austin <brian.austin-jGc1dHjMKG3QT0dZR+AlfA@public.gmane.org>
> ---
>  .../devicetree/bindings/sound/cs42l52.txt          | 48 ++++++++++++++++++++++
>  1 file changed, 48 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/sound/cs42l52.txt
> 
> diff --git a/Documentation/devicetree/bindings/sound/cs42l52.txt b/Documentation/devicetree/bindings/sound/cs42l52.txt
> new file mode 100644
> index 0000000..bd91ecf
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/sound/cs42l52.txt
> @@ -0,0 +1,48 @@
> +CS42L52 audio CODEC
> +
> +Required properties:
> +
> +  - compatible : "cirrus,cs42l52"
> +
> +  - reg : the I2C address of the device for I2C
> +
> +Optional properties:
> +
> +  - reset-gpio : GPIO controller's phandle and the number
> +		 of the GPIO used to reset the codec.

As Kumar has already mentioned, all device-specific properties should be
prefixed with vendor prefix, "cirrus," in this case.

> +  - chgfreq    : Charge Pump Frequency values. Allowable values of
> +		 0x00 through 0x0F.
> +		 Frequency = (64xFs)/(N+2)

I suppose N means the value of chgfreq property here, but this should be
clearly stated. Same for Fs - I suppose it is sampling frequency, but
is it default, minimum, maximum or maybe something else?

Personally I don't like this kind of raw values being passed using Device
Tree, but this one can't be really represented reasonably in a generic
way (such as in Hz units) without too much effort to calculate original
value in the driver, then it's fine.

> +  - mica-cfg   : MIC A single-ended or differential select.
> +		 0x00 = Single-Ended
> +		 0x01 = Differential

I'd rather make it boolean and call it cirrus,mic-a-differential.

> +  - micb-cfg   : MIC B single-ended or differential select.
> +		 0x00 = Single-Ended
> +		 0x01 = Differential

Ditto.

> +  - mica-sel   : MIC A single ended input select.  For Single-Ended
> +		 configuration, select which MIC to use.
> +		 0x00 = MIC1
> +		 0x01 = MIC2
> +  - micb-sel   : MIC B single ended input select.  For Single-Ended
> +		 configuration, select which MIC to use.
> +		 0x00 = MIC1
> +		 0x01 = MIC2

Could you explain what are MIC A, MIC B, MIC1 and MIC2?

> +  - micbias-lvl: Set the output voltage level on the MICBIAS Pin
> +		 0x00 = 0.5 x VA
> +		 0x01 = 0.6 x VA
> +		 0x02 = 0.7 x VA
> +		 0x03 = 0.8 x VA
> +		 0x04 = 0.83 x VA
> +		 0x05 = 0.91 x VA

For such small range of values, decimal representation is preferred from
readability reasons.

> +
> +Example:
> +
> +codec: cs42l52@4a {

coding style: Node name should be generic, i.e. codec@4a.

Best regards,
Tomasz

--
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 v2 2/2] dt: binding: sound cs42l52 driver

[Mark Brown]

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

On Wed, Nov 13, 2013 at 05:33:30PM +0100, Tomasz Figa wrote:
> On Wednesday 13 of November 2013 09:44:34 Brian Austin wrote:

> > +  - reset-gpio : GPIO controller's phandle and the number
> > +		 of the GPIO used to reset the codec.

> As Kumar has already mentioned, all device-specific properties should be
> prefixed with vendor prefix, "cirrus," in this case.

The best practice on this one seems to vary somewhat randomly -
sometimes it's a requirement, sometimes it isn't.

> > +codec: cs42l52@4a {

> coding style: Node name should be generic, i.e. codec@4a.

Is this a constructive thing from a style point of view?  We're not
allowed to actually do anything useful with the value at runtime so
people may as well choose what they like.

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

Re: [PATCH v2 2/2] dt: binding: sound cs42l52 driver

[Tomasz Figa]

On Wednesday 13 of November 2013 16:43:28 Mark Brown wrote:
> On Wed, Nov 13, 2013 at 05:33:30PM +0100, Tomasz Figa wrote:
> > On Wednesday 13 of November 2013 09:44:34 Brian Austin wrote:
> 
> > > +  - reset-gpio : GPIO controller's phandle and the number
> > > +		 of the GPIO used to reset the codec.
> 
> > As Kumar has already mentioned, all device-specific properties should be
> > prefixed with vendor prefix, "cirrus," in this case.
> 
> The best practice on this one seems to vary somewhat randomly -
> sometimes it's a requirement, sometimes it isn't.

AFAIR we decided on ARM Mini Summit to mandate this, but maybe my memory
is misleading me.

> 
> > > +codec: cs42l52@4a {
> 
> > coding style: Node name should be generic, i.e. codec@4a.
> 
> Is this a constructive thing from a style point of view?  We're not
> allowed to actually do anything useful with the value at runtime so
> people may as well choose what they like.

This is what ePAPR says and I believe this is reasonable, because looking
at device tree sources you don't need to think what kind of hardware
a cs42l52 is. The information that it's a cs42l52 is still contained
inside compatible string.

Best regards,
Tomasz

--
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 v2 2/2] dt: binding: sound cs42l52 driver

[Mark Brown]

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

On Wed, Nov 13, 2013 at 05:47:07PM +0100, Tomasz Figa wrote:
> On Wednesday 13 of November 2013 16:43:28 Mark Brown wrote:

> > The best practice on this one seems to vary somewhat randomly -
> > sometimes it's a requirement, sometimes it isn't.

> AFAIR we decided on ARM Mini Summit to mandate this, but maybe my memory
> is misleading me.

OK, I hadn't heard that - it's good that folks made their mind up.

> > Is this a constructive thing from a style point of view?  We're not
> > allowed to actually do anything useful with the value at runtime so
> > people may as well choose what they like.

> This is what ePAPR says and I believe this is reasonable, because looking
> at device tree sources you don't need to think what kind of hardware
> a cs42l52 is. The information that it's a cs42l52 is still contained
> inside compatible string.

Given that a meaningful name was already specified for the handle it's
really not going to help anything - it's just going to duplicate that
most likely.  Given that it can't be actually used for anything it seems
better to just let people write whatever they feel like in there (even
if it's just a single letter to keep the parser happy) rather than
nitpick over their choices.

Really it looks like one of those silly things standards makers do where
they start specifying a feature (in this case device classes) but don't
define any useful behaviour for it.

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

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

Re: [PATCH v2 2/2] dt: binding: sound cs42l52 driver

[Tomasz Figa]

On Wednesday 13 of November 2013 18:16:22 Mark Brown wrote:
> On Wed, Nov 13, 2013 at 05:47:07PM +0100, Tomasz Figa wrote:
> > On Wednesday 13 of November 2013 16:43:28 Mark Brown wrote:
> 
> > > The best practice on this one seems to vary somewhat randomly -
> > > sometimes it's a requirement, sometimes it isn't.
> 
> > AFAIR we decided on ARM Mini Summit to mandate this, but maybe my memory
> > is misleading me.
> 
> OK, I hadn't heard that - it's good that folks made their mind up.
> 
> > > Is this a constructive thing from a style point of view?  We're not
> > > allowed to actually do anything useful with the value at runtime so
> > > people may as well choose what they like.
> 
> > This is what ePAPR says and I believe this is reasonable, because looking
> > at device tree sources you don't need to think what kind of hardware
> > a cs42l52 is. The information that it's a cs42l52 is still contained
> > inside compatible string.
> 
> Given that a meaningful name was already specified for the handle it's
> really not going to help anything - it's just going to duplicate that
> most likely.  Given that it can't be actually used for anything it seems
> better to just let people write whatever they feel like in there (even
> if it's just a single letter to keep the parser happy) rather than
> nitpick over their choices.

Well, the label is just for the parser and it does not get into the DTB.
This is where the DTS author can make things up just for their own
convenience (like main_codec, aux_codec or even cs42l52). 

I know this is really more bikeshedding than anything useful, but I'd
rather try to follow the written rules in ePAPR, instead of nothing at
all. At least just to make things more consistent.

Best regards,
Tomasz

--
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 v2 2/2] dt: binding: sound cs42l52 driver

[Mark Brown]

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

On Wed, Nov 13, 2013 at 07:24:04PM +0100, Tomasz Figa wrote:

> Well, the label is just for the parser and it does not get into the DTB.
> This is where the DTS author can make things up just for their own
> convenience (like main_codec, aux_codec or even cs42l52). 

If only we could have comments :)

> I know this is really more bikeshedding than anything useful, but I'd
> rather try to follow the written rules in ePAPR, instead of nothing at
> all. At least just to make things more consistent.

My feeling here is that we should be looking more critically at ePAPR -
if we're picking people up for having what's essentially a comment
that's too specific we're probably doing something wrong especially
since the corrected example would look something like:

	codec: codec@12 {

which is a bit redundant.

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

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

Re: [PATCH v2 2/2] dt: binding: sound cs42l52 driver

[Tomasz Figa]

On Wednesday 13 of November 2013 19:49:15 Mark Brown wrote:
> On Wed, Nov 13, 2013 at 07:24:04PM +0100, Tomasz Figa wrote:
> 
> > Well, the label is just for the parser and it does not get into the DTB.
> > This is where the DTS author can make things up just for their own
> > convenience (like main_codec, aux_codec or even cs42l52). 
> 
> If only we could have comments :)

Comments in DTB? That would be at least interesting. ;)

> 
> > I know this is really more bikeshedding than anything useful, but I'd
> > rather try to follow the written rules in ePAPR, instead of nothing at
> > all. At least just to make things more consistent.
> 
> My feeling here is that we should be looking more critically at ePAPR -
> if we're picking people up for having what's essentially a comment
> that's too specific we're probably doing something wrong especially
> since the corrected example would look something like:
> 
> 	codec: codec@12 {
> 
> which is a bit redundant.

Still, this is not a comment, because it's also available in the
resulting binary representation.

Well, maybe I'm a bit too strict. I wouldn't probably notice this if
I didn't have other comments, so let's say that was just a nitpick of
mine, fixing of which doesn't cost anything, since another version will
have to be spinned anyway.

Best regards,
Tomasz

--
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 v2 2/2] dt: binding: sound cs42l52 driver

[Brian Austin]

>> +  - compatible : "cirrus,cs42l52"
>> +
>> +  - reg : the I2C address of the device for I2C
>> +
>> +Optional properties:
>> +
>> +  - reset-gpio : GPIO controller's phandle and the number
>> +		 of the GPIO used to reset the codec.
>
> As Kumar has already mentioned, all device-specific properties should be
> prefixed with vendor prefix, "cirrus," in this case.
>

I can do that, Thanks


>> +  - chgfreq    : Charge Pump Frequency values. Allowable values of
>> +		 0x00 through 0x0F.
>> +		 Frequency = (64xFs)/(N+2)
>
> I suppose N means the value of chgfreq property here, but this should be
> clearly stated. Same for Fs - I suppose it is sampling frequency, but
> is it default, minimum, maximum or maybe something else?
>
Frequency = (64xFs)/(N+2)
N = chgfreq value
Fs = sample rate


> Personally I don't like this kind of raw values being passed using Device
> Tree, but this one can't be really represented reasonably in a generic
> way (such as in Hz units) without too much effort to calculate original
> value in the driver, then it's fine.
>
>> +  - mica-cfg   : MIC A single-ended or differential select.
>> +		 0x00 = Single-Ended
>> +		 0x01 = Differential
>
> I'd rather make it boolean and call it cirrus,mic-a-differential.
>
>> +  - micb-cfg   : MIC B single-ended or differential select.
>> +		 0x00 = Single-Ended
>> +		 0x01 = Differential
>
> Ditto.
>

I can do that as well. Thanks

>> +  - mica-sel   : MIC A single ended input select.  For Single-Ended
>> +		 configuration, select which MIC to use.
>> +		 0x00 = MIC1
>> +		 0x01 = MIC2
>> +  - micb-sel   : MIC B single ended input select.  For Single-Ended
>> +		 configuration, select which MIC to use.
>> +		 0x00 = MIC1
>> +		 0x01 = MIC2
>
> Could you explain what are MIC A, MIC B, MIC1 and MIC2?
>
I can add a little more but it is best explained in the datasheet. I can 
add a little more explaination and reference the datasheet section. I feel 
this file might be the wrong place to go into too much depth of the pieces 
of the device when the datasheet could be referenced. Would a reference be 
OK?


>> +  - micbias-lvl: Set the output voltage level on the MICBIAS Pin
>> +		 0x00 = 0.5 x VA
>> +		 0x01 = 0.6 x VA
>> +		 0x02 = 0.7 x VA
>> +		 0x03 = 0.8 x VA
>> +		 0x04 = 0.83 x VA
>> +		 0x05 = 0.91 x VA
>
> For such small range of values, decimal representation is preferred from
> readability reasons.
>
OK, Thanks
>> +
>> +Example:
>> +
>> +codec: cs42l52@4a {
>
> coding style: Node name should be generic, i.e. codec@4a.

I can change this as well.  While we are talking about coding style, is 
there a new format document somewhere with the style that was agreed to at 
the conference just recently?

Thanks,
Brian

Re: [PATCH v2 2/2] dt: binding: sound cs42l52 driver

[Tomasz Figa]

Hi Brian,

On Wednesday 13 of November 2013 12:58:28 Brian Austin wrote:
> >> +  - compatible : "cirrus,cs42l52"
> >> +
> >> +  - reg : the I2C address of the device for I2C
> >> +
> >> +Optional properties:
> >> +
> >> +  - reset-gpio : GPIO controller's phandle and the number
> >> +		 of the GPIO used to reset the codec.
> >
> > As Kumar has already mentioned, all device-specific properties should be
> > prefixed with vendor prefix, "cirrus," in this case.
> >
> 
> I can do that, Thanks
> 
> 
> >> +  - chgfreq    : Charge Pump Frequency values. Allowable values of
> >> +		 0x00 through 0x0F.
> >> +		 Frequency = (64xFs)/(N+2)
> >
> > I suppose N means the value of chgfreq property here, but this should be
> > clearly stated. Same for Fs - I suppose it is sampling frequency, but
> > is it default, minimum, maximum or maybe something else?
> >
> Frequency = (64xFs)/(N+2)
> N = chgfreq value
> Fs = sample rate

As I mentioned in second part of my comment, is it default, minimum,
maximum or what kind of sample rate? Or is the sample rate fixed?

> 
> 
> > Personally I don't like this kind of raw values being passed using Device
> > Tree, but this one can't be really represented reasonably in a generic
> > way (such as in Hz units) without too much effort to calculate original
> > value in the driver, then it's fine.
> >
> >> +  - mica-cfg   : MIC A single-ended or differential select.
> >> +		 0x00 = Single-Ended
> >> +		 0x01 = Differential
> >
> > I'd rather make it boolean and call it cirrus,mic-a-differential.
> >
> >> +  - micb-cfg   : MIC B single-ended or differential select.
> >> +		 0x00 = Single-Ended
> >> +		 0x01 = Differential
> >
> > Ditto.
> >
> 
> I can do that as well. Thanks
> 
> >> +  - mica-sel   : MIC A single ended input select.  For Single-Ended
> >> +		 configuration, select which MIC to use.
> >> +		 0x00 = MIC1
> >> +		 0x01 = MIC2
> >> +  - micb-sel   : MIC B single ended input select.  For Single-Ended
> >> +		 configuration, select which MIC to use.
> >> +		 0x00 = MIC1
> >> +		 0x01 = MIC2
> >
> > Could you explain what are MIC A, MIC B, MIC1 and MIC2?
> >
> I can add a little more but it is best explained in the datasheet. I can 
> add a little more explaination and reference the datasheet section. I feel 
> this file might be the wrong place to go into too much depth of the pieces 
> of the device when the datasheet could be referenced. Would a reference be 
> OK?
> 

I meant just explaining to me, for the purpose of this review. However it
seems like the datasheet is publicly available, so let me just check this
there.

> 
> >> +  - micbias-lvl: Set the output voltage level on the MICBIAS Pin
> >> +		 0x00 = 0.5 x VA
> >> +		 0x01 = 0.6 x VA
> >> +		 0x02 = 0.7 x VA
> >> +		 0x03 = 0.8 x VA
> >> +		 0x04 = 0.83 x VA
> >> +		 0x05 = 0.91 x VA
> >
> > For such small range of values, decimal representation is preferred from
> > readability reasons.
> >
> OK, Thanks
> >> +
> >> +Example:
> >> +
> >> +codec: cs42l52@4a {
> >
> > coding style: Node name should be generic, i.e. codec@4a.
> 
> I can change this as well.  While we are talking about coding style, is 
> there a new format document somewhere with the style that was agreed to at 
> the conference just recently?

I believe that the latest document is in the works, but as for style
guidelines, most of recommendations are to be taken from ePAPR[1].

[1] https://www.power.org/wp-content/uploads/2012/06/Power_ePAPR_APPROVED_v1.1.pdf

Best regards,
Tomasz

--
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 v2 2/2] dt: binding: sound cs42l52 driver

[Brian Austin]

On Wed, 13 Nov 2013, Tomasz Figa wrote:

>>>> +		 0x00 through 0x0F.
>>>> +		 Frequency = (64xFs)/(N+2)
>>>
>>> I suppose N means the value of chgfreq property here, but this should be
>>> clearly stated. Same for Fs - I suppose it is sampling frequency, but
>>> is it default, minimum, maximum or maybe something else?
>>>
>> Frequency = (64xFs)/(N+2)
>> N = chgfreq value
>> Fs = sample rate
>
> As I mentioned in second part of my comment, is it default, minimum,
> maximum or what kind of sample rate? Or is the sample rate fixed?
>
ah! Sorry, the rate is fixed for the stream. So it varies for what rate is 
being used.


>>
>>
>>> Personally I don't like this kind of raw values being passed using Device
>>> Tree, but this one can't be really represented reasonably in a generic
>>> way (such as in Hz units) without too much effort to calculate original
>>> value in the driver, then it's fine.
>>>
>>>> +  - mica-cfg   : MIC A single-ended or differential select.
>>>> +		 0x00 = Single-Ended
>>>> +		 0x01 = Differential
>>>
>>> I'd rather make it boolean and call it cirrus,mic-a-differential.
>>>
>>>> +  - micb-cfg   : MIC B single-ended or differential select.
>>>> +		 0x00 = Single-Ended
>>>> +		 0x01 = Differential
>>>
>>> Ditto.
>>>
>>
>> I can do that as well. Thanks
>>
>>>> +  - mica-sel   : MIC A single ended input select.  For Single-Ended
>>>> +		 configuration, select which MIC to use.
>>>> +		 0x00 = MIC1
>>>> +		 0x01 = MIC2
>>>> +  - micb-sel   : MIC B single ended input select.  For Single-Ended
>>>> +		 configuration, select which MIC to use.
>>>> +		 0x00 = MIC1
>>>> +		 0x01 = MIC2
>>>
>>> Could you explain what are MIC A, MIC B, MIC1 and MIC2?
>>>
>> I can add a little more but it is best explained in the datasheet. I can
>> add a little more explaination and reference the datasheet section. I feel
>> this file might be the wrong place to go into too much depth of the pieces
>> of the device when the datasheet could be referenced. Would a reference be
>> OK?
>>
>
> I meant just explaining to me, for the purpose of this review. However it
> seems like the datasheet is publicly available, so let me just check this
> there.
>
Sorry about that.

The CS42L52 has multiple input selections for microphones. Single-ended 
and differential inputs are allowed for both Left Channel (MICA) and Right 
Channel (MICB). The +- pins can be used for stereo or mono mics.

>>>> +Example:
>>>> +
>>>> +codec: cs42l52@4a {
>>>
>>> coding style: Node name should be generic, i.e. codec@4a.
>>
>> I can change this as well.  While we are talking about coding style, is
>> there a new format document somewhere with the style that was agreed to at
>> the conference just recently?
>
> I believe that the latest document is in the works, but as for style
> guidelines, most of recommendations are to be taken from ePAPR[1].
>
> [1] https://www.power.org/wp-content/uploads/2012/06/Power_ePAPR_APPROVED_v1.1.pdf
>
Thank you very much. I will take a look at this now.

Thanks again,
Brian


--
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 v2 2/2] dt: binding: sound cs42l52 driver

[Tomasz Figa]

On Wednesday 13 of November 2013 13:10:51 Brian Austin wrote:
> On Wed, 13 Nov 2013, Tomasz Figa wrote:
> 
> >>>> +		 0x00 through 0x0F.
> >>>> +		 Frequency = (64xFs)/(N+2)
> >>>
> >>> I suppose N means the value of chgfreq property here, but this should be
> >>> clearly stated. Same for Fs - I suppose it is sampling frequency, but
> >>> is it default, minimum, maximum or maybe something else?
> >>>
> >> Frequency = (64xFs)/(N+2)
> >> N = chgfreq value
> >> Fs = sample rate
> >
> > As I mentioned in second part of my comment, is it default, minimum,
> > maximum or what kind of sample rate? Or is the sample rate fixed?
> >
> ah! Sorry, the rate is fixed for the stream. So it varies for what rate is 
> being used.
> 

OK, so the charge pump frequency varies with sampling rate. Well, I guess
that's fine to provide a raw register value then. The name is still a bit
misleading, though.

Maybe calling it cirrus,chgfreq-divisor would be better instead? Also the
description in binding documentation should explicitly state that it's the
raw value that needs to be written to the CHGFREQ register.

> >>>> +  - mica-sel   : MIC A single ended input select.  For Single-Ended
> >>>> +		 configuration, select which MIC to use.
> >>>> +		 0x00 = MIC1
> >>>> +		 0x01 = MIC2
> >>>> +  - micb-sel   : MIC B single ended input select.  For Single-Ended
> >>>> +		 configuration, select which MIC to use.
> >>>> +		 0x00 = MIC1
> >>>> +		 0x01 = MIC2
> >>>
> >>> Could you explain what are MIC A, MIC B, MIC1 and MIC2?
> >>>
> >> I can add a little more but it is best explained in the datasheet. I can
> >> add a little more explaination and reference the datasheet section. I feel
> >> this file might be the wrong place to go into too much depth of the pieces
> >> of the device when the datasheet could be referenced. Would a reference be
> >> OK?
> >>
> >
> > I meant just explaining to me, for the purpose of this review. However it
> > seems like the datasheet is publicly available, so let me just check this
> > there.
> >
> Sorry about that.
> 
> The CS42L52 has multiple input selections for microphones. Single-ended 
> and differential inputs are allowed for both Left Channel (MICA) and Right 
> Channel (MICB). The +- pins can be used for stereo or mono mics.

OK. After looking at the datasheet for a bit, I'm pretty much convinced
that these two properties should be rather represented as mixer controls
instead, for the purpose of channel mixing.

Whether the "Mic X Sel" control would be present in the mixer or not would
be implied by mic-X-differential property in DT.

Best regards,
Tomasz

--
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 v2 2/2] dt: binding: sound cs42l52 driver

[Brian Austin]

On Wed, 13 Nov 2013, Tomasz Figa wrote:

> On Wednesday 13 of November 2013 13:10:51 Brian Austin wrote:
>> On Wed, 13 Nov 2013, Tomasz Figa wrote:
>>
>>>>>> +		 0x00 through 0x0F.
>>>>>> +		 Frequency = (64xFs)/(N+2)
>>>>>
>>>>> I suppose N means the value of chgfreq property here, but this should be
>>>>> clearly stated. Same for Fs - I suppose it is sampling frequency, but
>>>>> is it default, minimum, maximum or maybe something else?
>>>>>
>>>> Frequency = (64xFs)/(N+2)
>>>> N = chgfreq value
>>>> Fs = sample rate
>>>
>>> As I mentioned in second part of my comment, is it default, minimum,
>>> maximum or what kind of sample rate? Or is the sample rate fixed?
>>>
>> ah! Sorry, the rate is fixed for the stream. So it varies for what rate is
>> being used.
>>
>
> OK, so the charge pump frequency varies with sampling rate. Well, I guess
> that's fine to provide a raw register value then. The name is still a bit
> misleading, though.
>
> Maybe calling it cirrus,chgfreq-divisor would be better instead? Also the
> description in binding documentation should explicitly state that it's the
> raw value that needs to be written to the CHGFREQ register.
>

Will do,
Thanks
>>>>>> +  - mica-sel   : MIC A single ended input select.  For Single-Ended
>>>>>> +		 configuration, select which MIC to use.
>>>>>> +		 0x00 = MIC1
>>>>>> +		 0x01 = MIC2
>>>>>> +  - micb-sel   : MIC B single ended input select.  For Single-Ended
>>>>>> +		 configuration, select which MIC to use.
>>>>>> +		 0x00 = MIC1
>>>>>> +		 0x01 = MIC2
>>>>>
>>>>> Could you explain what are MIC A, MIC B, MIC1 and MIC2?
>>>>>
>>>> I can add a little more but it is best explained in the datasheet. I can
>>>> add a little more explaination and reference the datasheet section. I feel
>>>> this file might be the wrong place to go into too much depth of the pieces
>>>> of the device when the datasheet could be referenced. Would a reference be
>>>> OK?
>>>>
>>>
>>> I meant just explaining to me, for the purpose of this review. However it
>>> seems like the datasheet is publicly available, so let me just check this
>>> there.
>>>
>> Sorry about that.
>>
>> The CS42L52 has multiple input selections for microphones. Single-ended
>> and differential inputs are allowed for both Left Channel (MICA) and Right
>> Channel (MICB). The +- pins can be used for stereo or mono mics.
>
> OK. After looking at the datasheet for a bit, I'm pretty much convinced
> that these two properties should be rather represented as mixer controls
> instead, for the purpose of channel mixing.
>
> Whether the "Mic X Sel" control would be present in the mixer or not would
> be implied by mic-X-differential property in DT.

That sounds reasonable and yes, the config would be static but you
could have cases where you switch which mic to use as the input to the
PGA.

Thanks for the review,
Brian

--
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