[1/5] ASoC: alc5623: Add device tree binding
diff mbox

Message ID 1397749994-24983-2-git-send-email-andrew@lunn.ch
State Accepted
Commit 0e19c896630ace5cd413f1a14de789f5c5322459
Headers show

Commit Message

Andrew Lunn April 17, 2014, 3:53 p.m. UTC
Let the ALC5623 codec be instantiated from DT. Add a simple binding
for the additional control register and the jack detect register.

Signed-off-by: Andrew Lunn <andrew@lunn.ch>
---
I followed the example of the WM8903 binding which allows register
values to be placed into DT.
---
 .../devicetree/bindings/sound/alc5623.txt          | 23 ++++++++++++++++++++++
 sound/soc/codecs/alc5623.c                         | 13 ++++++++++++
 2 files changed, 36 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/sound/alc5623.txt

Comments

Mark Brown April 18, 2014, 4:33 p.m. UTC | #1
On Thu, Apr 17, 2014 at 05:53:10PM +0200, Andrew Lunn wrote:

> + - compatible:	"realtek,alc5623"

You've not added an ID table to the driver for this.

> +  - add-ctrl:	Default register value for Reg-40h, Additional Control Register.
> +		If absent, the default is 0.

> +  - jack-det-ctrl:	Default register value for Reg-5Ah, Jack Detect
> +			Control Register. If absent, the default is 0.

I would expect the default for these to be to leave the hardware
defaults untouched - why is it different, what does setting to zero mean?
Andrew Lunn April 18, 2014, 6:17 p.m. UTC | #2
On Fri, Apr 18, 2014 at 05:33:51PM +0100, Mark Brown wrote:
> On Thu, Apr 17, 2014 at 05:53:10PM +0200, Andrew Lunn wrote:
> 
> > + - compatible:	"realtek,alc5623"
> 
> You've not added an ID table to the driver for this.

The driver already has:

static const struct i2c_device_id alc5623_i2c_table[] = {
        {"alc5621", 0x21},
        {"alc5622", 0x22},
        {"alc5623", 0x23},
        {}
};
MODULE_DEVICE_TABLE(i2c, alc5623_i2c_table);

which is enough for the i2c layer to load the driver when it walks the
nodes under the i2c bus driver in the DT.

> > +  - add-ctrl:	Default register value for Reg-40h, Additional Control Register.
> > +		If absent, the default is 0.
> 
> > +  - jack-det-ctrl:	Default register value for Reg-5Ah, Jack Detect
> > +			Control Register. If absent, the default is 0.
> 
> I would expect the default for these to be to leave the hardware
> defaults untouched - why is it different, what does setting to zero mean?

The description is wrong. I will fix it. If the property is absent, or
the value is zero, the register is left alone.

    Andrew
Mark Brown April 18, 2014, 6:44 p.m. UTC | #3
On Fri, Apr 18, 2014 at 08:17:35PM +0200, Andrew Lunn wrote:

> The driver already has:

> static const struct i2c_device_id alc5623_i2c_table[] = {
>         {"alc5621", 0x21},
>         {"alc5622", 0x22},
>         {"alc5623", 0x23},
>         {}
> };
> MODULE_DEVICE_TABLE(i2c, alc5623_i2c_table);

> which is enough for the i2c layer to load the driver when it walks the
> nodes under the i2c bus driver in the DT.

You still need to add the compatible strings to the driver explicitly,
device names can be and are duplicated between manufacturers sometimes
and it makes things directly greppable.
Anil Kumar April 28, 2014, 7:01 a.m. UTC | #4
Hi Andrew,

Please find my comment below.


On Thu, Apr 17, 2014 at 9:23 PM, Andrew Lunn <andrew@lunn.ch> wrote:

> Let the ALC5623 codec be instantiated from DT. Add a simple binding
> for the additional control register and the jack detect register.
>
> Signed-off-by: Andrew Lunn <andrew@lunn.ch>
> ---
> I followed the example of the WM8903 binding which allows register
> values to be placed into DT.
> ---
>  .../devicetree/bindings/sound/alc5623.txt          | 23
> ++++++++++++++++++++++
>  sound/soc/codecs/alc5623.c                         | 13 ++++++++++++
>  2 files changed, 36 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/sound/alc5623.txt
>
> diff --git a/Documentation/devicetree/bindings/sound/alc5623.txt
> b/Documentation/devicetree/bindings/sound/alc5623.txt
> new file mode 100644
> index 000000000000..f7f71346e338
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/sound/alc5623.txt
> @@ -0,0 +1,23 @@
> +ALC5621/ALC5622/ALC5623 audio Codec
> +
> +Required properties:
> +
> + - compatible: "realtek,alc5623"
> + - reg:                the I2C address of the device.
> +
> +Optional properties:
> +
> +  - add-ctrl:  Default register value for Reg-40h, Additional Control
> Register.
> +               If absent, the default is 0.
> +
> +  - jack-det-ctrl:     Default register value for Reg-5Ah, Jack Detect
> +                       Control Register. If absent, the default is 0.
> +
> +Example:
> +
> +                       alc5621: alc5621@1a {
> +                               compatible = "alc5621";
> +                               reg = <0x1a>;
> +                               add-ctrl = <0x3700>;
> +                               jack-det-ctrl = <0x4810>;
> +                       };
> diff --git a/sound/soc/codecs/alc5623.c b/sound/soc/codecs/alc5623.c
> index 2acf82f4a08a..1f9273d20b39 100644
> --- a/sound/soc/codecs/alc5623.c
> +++ b/sound/soc/codecs/alc5623.c
> @@ -23,6 +23,7 @@
>  #include <linux/i2c.h>
>  #include <linux/regmap.h>
>  #include <linux/slab.h>
> +#include <linux/of.h>
>  #include <sound/core.h>
>  #include <sound/pcm.h>
>  #include <sound/pcm_params.h>
> @@ -998,8 +999,10 @@ static int alc5623_i2c_probe(struct i2c_client
> *client,
>  {
>         struct alc5623_platform_data *pdata;
>         struct alc5623_priv *alc5623;
> +       struct device_node *np;
>         unsigned int vid1, vid2;
>         int ret;
> +       u32 val32;
>
>         alc5623 = devm_kzalloc(&client->dev, sizeof(struct alc5623_priv),
>                                GFP_KERNEL);
> @@ -1040,6 +1043,16 @@ static int alc5623_i2c_probe(struct i2c_client
> *client,
>         if (pdata) {
>                 alc5623->add_ctrl = pdata->add_ctrl;
>                 alc5623->jack_det_ctrl = pdata->jack_det_ctrl;
> +       } else {
> +               if (client->dev.of_node) {
> +                       np = client->dev.of_node;
> +                       ret = of_property_read_u32(np, "add-ctrl", &val32);
> +                       if (ret >= 0)
>
of_property_read_u32 returns 0 on success, why  "if (ret >= 0)" check ?


> +                               alc5623->add_ctrl = val32;
> +                       ret = of_property_read_u32(np, "jack-det-ctrl",
> &val32);
>
+                       if (ret >= 0)
>
 same as above

> +                               alc5623->jack_det_ctrl = val32;
> +               }
>         }
>
>         alc5623->id = vid2;
> --
> 1.9.2
>
>
> _______________________________________________
> linux-arm-kernel mailing list
> linux-arm-kernel@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
>

Patch
diff mbox

diff --git a/Documentation/devicetree/bindings/sound/alc5623.txt b/Documentation/devicetree/bindings/sound/alc5623.txt
new file mode 100644
index 000000000000..f7f71346e338
--- /dev/null
+++ b/Documentation/devicetree/bindings/sound/alc5623.txt
@@ -0,0 +1,23 @@ 
+ALC5621/ALC5622/ALC5623 audio Codec
+
+Required properties:
+
+ - compatible:	"realtek,alc5623"
+ - reg:		the I2C address of the device.
+
+Optional properties:
+
+  - add-ctrl:	Default register value for Reg-40h, Additional Control Register.
+		If absent, the default is 0.
+
+  - jack-det-ctrl:	Default register value for Reg-5Ah, Jack Detect
+			Control Register. If absent, the default is 0.
+
+Example:
+
+			alc5621: alc5621@1a {
+				compatible = "alc5621";
+				reg = <0x1a>;
+				add-ctrl = <0x3700>;
+				jack-det-ctrl = <0x4810>;
+			};
diff --git a/sound/soc/codecs/alc5623.c b/sound/soc/codecs/alc5623.c
index 2acf82f4a08a..1f9273d20b39 100644
--- a/sound/soc/codecs/alc5623.c
+++ b/sound/soc/codecs/alc5623.c
@@ -23,6 +23,7 @@ 
 #include <linux/i2c.h>
 #include <linux/regmap.h>
 #include <linux/slab.h>
+#include <linux/of.h>
 #include <sound/core.h>
 #include <sound/pcm.h>
 #include <sound/pcm_params.h>
@@ -998,8 +999,10 @@  static int alc5623_i2c_probe(struct i2c_client *client,
 {
 	struct alc5623_platform_data *pdata;
 	struct alc5623_priv *alc5623;
+	struct device_node *np;
 	unsigned int vid1, vid2;
 	int ret;
+	u32 val32;
 
 	alc5623 = devm_kzalloc(&client->dev, sizeof(struct alc5623_priv),
 			       GFP_KERNEL);
@@ -1040,6 +1043,16 @@  static int alc5623_i2c_probe(struct i2c_client *client,
 	if (pdata) {
 		alc5623->add_ctrl = pdata->add_ctrl;
 		alc5623->jack_det_ctrl = pdata->jack_det_ctrl;
+	} else {
+		if (client->dev.of_node) {
+			np = client->dev.of_node;
+			ret = of_property_read_u32(np, "add-ctrl", &val32);
+			if (ret >= 0)
+				alc5623->add_ctrl = val32;
+			ret = of_property_read_u32(np, "jack-det-ctrl", &val32);
+			if (ret >= 0)
+				alc5623->jack_det_ctrl = val32;
+		}
 	}
 
 	alc5623->id = vid2;