[PATCH 1/2] ASoC: cs42l52: Add Device Tree support for CS42L52

[PATCH 1/2] ASoC: cs42l52: Add Device Tree support for CS42L52

[Brian Austin]

This patch adds support for DT platform data and I2C/RST initialization
support

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

[Brian Austin]

Add Device Tree Binding for the CS42L52 Codec

Signed-off-by: Brian Austin <brian.austin-jGc1dHjMKG3QT0dZR+AlfA@public.gmane.org>
---
 .../devicetree/bindings/sound/cs42l52.txt          | 34 ++++++++++++++++++++++
 1 file changed, 34 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..7540198
--- /dev/null
+++ b/Documentation/devicetree/bindings/sound/cs42l52.txt
@@ -0,0 +1,34 @@
+CS42L52 audio CODEC
+
+Required properties:
+
+  - compatible : "cirrus,cs42l52"
+
+  - reg : the I2C address of the device for I2C
+
+Optional properties:
+
+  - reset_gpio : a GPIO spec for the reset pin.
+  - chgfreq    : Charge Pump Frequency values 0x00-0x0F
+  - mica_sel   : MIC A single ended input select MIC1/MIC2
+  - micb_sel   : MIC B single ended input select MIC1/MIC2
+  - mica_cfg   : MIC A single-ended or differential select
+  - micb_cfg   : MIC A single-ended or differential select
+  - 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_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 2/2] dt: binding: sound cs42l52 driver

[Mark Rutland]

On Thu, Nov 07, 2013 at 08:22:17PM +0000, Brian Austin wrote:
> Add Device Tree Binding for the CS42L52 Codec
> 
> Signed-off-by: Brian Austin <brian.austin-jGc1dHjMKG3QT0dZR+AlfA@public.gmane.org>
> ---
>  .../devicetree/bindings/sound/cs42l52.txt          | 34 ++++++++++++++++++++++
>  1 file changed, 34 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..7540198
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/sound/cs42l52.txt
> @@ -0,0 +1,34 @@
> +CS42L52 audio CODEC
> +
> +Required properties:
> +
> +  - compatible : "cirrus,cs42l52"
> +
> +  - reg : the I2C address of the device for I2C
> +
> +Optional properties:
> +
> +  - reset_gpio : a GPIO spec for the reset pin.

Nit: device tree convention is to use '-' rather than '_'. So this
should be reset-gpio rather than reset_gpio.

s/_/-/ for the later property names too.

s/GPIO spec/phandle + gpio-specifier/

> +  - chgfreq    : Charge Pump Frequency values 0x00-0x0F

In general, longer but more easily understood names are preferred (e.g.
"clock-frequency"). This could be "charge-pump-frequency".

When you say "values 0x00-0x0F" you mean the value is in that range?
Inclusive?

Due to punctuation, upon first reading I read this as being an array of
values.  It would be nice to have some clarification to prevent others
maknig the smae mistake.

> +  - mica_sel   : MIC A single ended input select MIC1/MIC2
> +  - micb_sel   : MIC B single ended input select MIC1/MIC2

Is this a boolean? What values are expected?

Could you elaborate on what this describes?

> +  - mica_cfg   : MIC A single-ended or differential select
> +  - micb_cfg   : MIC A single-ended or differential select

Could you elaborate on what these describe, what values are expected,
etc?

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

Is this not something we'd want to change at runtime instead? Why does
this need to be in the dt?

Thanks,
Mark.
--
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] dt: binding: sound cs42l52 driver

[Mark Brown]

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

On Mon, Nov 11, 2013 at 10:16:22AM +0000, Mark Rutland wrote:
> On Thu, Nov 07, 2013 at 08:22:17PM +0000, Brian Austin wrote:

> > +  - chgfreq    : Charge Pump Frequency values 0x00-0x0F

> In general, longer but more easily understood names are preferred (e.g.
> "clock-frequency"). This could be "charge-pump-frequency".

I rather suspect that this is the name of the register that gets the raw
value specified written to it - using the register name seems reasonable
for that.

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

> Is this not something we'd want to change at runtime instead? Why does
> this need to be in the dt?

This is something that would come from the electrical design of the
system rather than something that users would tune at runtime.

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

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

Re: [alsa-devel] [PATCH 2/2] dt: binding: sound cs42l52 driver

[Brian Austin]

>
> Nit: device tree convention is to use '-' rather than '_'. So this should be reset-gpio rather than reset_gpio.
>
> s/_/-/ for the later property names too.
>

Will do, thanks

> s/GPIO spec/phandle + gpio-specifier/

OK, Same here


>
>> +  - chgfreq    : Charge Pump Frequency values 0x00-0x0F
>
> In general, longer but more easily understood names are preferred (e.g.
> "clock-frequency"). This could be "charge-pump-frequency".
>
As Mark Brown pointed out, I would prefer to leave this as a reference to 
a register in the device. In general I was hoping to keep the original 
platform data names as carried over to dt.

> When you say "values 0x00-0x0F" you mean the value is in that range?
> Inclusive?
>
I'll be more specific, Thanks

> Due to punctuation, upon first reading I read this as being an array of values.  It would be nice to have some clarification to prevent others maknig the smae mistake.
>
>> +  - mica_sel   : MIC A single ended input select MIC1/MIC2
>> +  - micb_sel   : MIC B single ended input select MIC1/MIC2
>
> Is this a boolean? What values are expected?
>
> Could you elaborate on what this describes?
>
>> +  - mica_cfg   : MIC A single-ended or differential select
>> +  - micb_cfg   : MIC A single-ended or differential select
>
> Could you elaborate on what these describe, what values are expected, etc?
>

I will explain values better, Thanks


>> +  - 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
>
> Is this not something we'd want to change at runtime instead? Why does this need to be in the dt?
>

This is set based on the system design so it usually will be a one time 
setting at boot.

I'll send a v2 shortly,

Thank you for the review and tips for the next ones.

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