| Message ID | 1397749994-24983-2-git-send-email-andrew@lunn.ch (mailing list archive) |
|---|---|
| State | Accepted |
| Commit | 0e19c896630ace5cd413f1a14de789f5c5322459 |
| Headers | show |
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?
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
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.
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 >
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;
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