[alsa-devel] [PATCH 1/2] ALSA: ASoC: add DT bindings for cs4270
Signed-off-by: Daniel Mack zonque@gmail.com Cc: Timur Tabi timur@freescale.com --- Documentation/devicetree/bindings/sound/cs4270.txt | 16 ++++++++++++++++ sound/soc/codecs/cs4270.c | 11 +++++++++++ 2 files changed, 27 insertions(+) create mode 100644 Documentation/devicetree/bindings/sound/cs4270.txt
diff --git a/Documentation/devicetree/bindings/sound/cs4270.txt b/Documentation/devicetree/bindings/sound/cs4270.txt new file mode 100644 index 0000000..7f0bfd8 --- /dev/null +++ b/Documentation/devicetree/bindings/sound/cs4270.txt @@ -0,0 +1,16 @@ +CS4270 audio CODEC + +The driver for this device currently only supports I2C. + +Required properties: + + - compatible : "cirrus,cs4270" + + - reg : the I2C address of the device for I2C + +Example: + +codec: cs4270@48 { + compatible = "cirrus,cs4270"; + reg = <0x48>; +}; diff --git a/sound/soc/codecs/cs4270.c b/sound/soc/codecs/cs4270.c index 047917f..4b71b01 100644 --- a/sound/soc/codecs/cs4270.c +++ b/sound/soc/codecs/cs4270.c @@ -29,6 +29,7 @@ #include <linux/i2c.h> #include <linux/delay.h> #include <linux/regulator/consumer.h> +#include <linux/of_device.h>
/* * The codec isn't really big-endian or little-endian, since the I2S @@ -639,6 +640,15 @@ static const struct snd_soc_codec_driver soc_codec_device_cs4270 = { .reg_cache_default = cs4270_default_reg_cache, };
+/* + * cs4270_of_match - the device tree bindings + */ +static const struct of_device_id cs4270_of_match[] = { + { .compatible = "cirrus,cs4270", }, + { } +}; +MODULE_DEVICE_TABLE(of, cs4270_of_match); + /** * cs4270_i2c_probe - initialize the I2C interface of the CS4270 * @i2c_client: the I2C client object @@ -718,6 +728,7 @@ static struct i2c_driver cs4270_i2c_driver = { .driver = { .name = "cs4270", .owner = THIS_MODULE, + .of_match_table = cs4270_of_match, }, .id_table = cs4270_id, .probe = cs4270_i2c_probe,
In the process of moving over from static board files to the device tree, reset pins of peripheral reset pins should be handled by their corresponding drivers.
Add a reset-gpio DT property to the cs4270 driver, and de-assert it before probing the chip. The logic could be augmented some day to re-assert it when codec is put to suspend.
Signed-off-by: Daniel Mack zonque@gmail.com Cc: Timur Tabi timur@freescale.com --- Documentation/devicetree/bindings/sound/cs4270.txt | 5 +++++ sound/soc/codecs/cs4270.c | 16 ++++++++++++++++ 2 files changed, 21 insertions(+)
diff --git a/Documentation/devicetree/bindings/sound/cs4270.txt b/Documentation/devicetree/bindings/sound/cs4270.txt index 7f0bfd8..6b222f9 100644 --- a/Documentation/devicetree/bindings/sound/cs4270.txt +++ b/Documentation/devicetree/bindings/sound/cs4270.txt @@ -8,6 +8,11 @@ Required properties:
- reg : the I2C address of the device for I2C
+Optional properties: + + - reset-gpio : a GPIO spec for the reset pin. If specified, it will be + deasserted before communication to the codec starts. + Example:
codec: cs4270@48 { diff --git a/sound/soc/codecs/cs4270.c b/sound/soc/codecs/cs4270.c index 4b71b01..7d206be 100644 --- a/sound/soc/codecs/cs4270.c +++ b/sound/soc/codecs/cs4270.c @@ -30,6 +30,7 @@ #include <linux/delay.h> #include <linux/regulator/consumer.h> #include <linux/of_device.h> +#include <linux/of_gpio.h>
/* * The codec isn't really big-endian or little-endian, since the I2S @@ -660,9 +661,24 @@ MODULE_DEVICE_TABLE(of, cs4270_of_match); static int cs4270_i2c_probe(struct i2c_client *i2c_client, const struct i2c_device_id *id) { + struct device_node *np = i2c_client->dev.of_node; struct cs4270_private *cs4270; int ret;
+ /* See if we way to bring the codec out of reset */ + if (np) { + enum of_gpio_flags reset_gpio_flags; + int reset_gpio = of_get_named_gpio_flags(np, "reset-gpio", 0, + &reset_gpio_flags); + + if (devm_gpio_request_one(&i2c_client->dev, reset_gpio, + reset_gpio_flags & OF_GPIO_ACTIVE_LOW ? + GPIOF_OUT_INIT_LOW : GPIOF_OUT_INIT_HIGH, + "cs4270 reset") < 0) { + reset_gpio = -EINVAL; + } + } + /* Verify that we have a CS4270 */
ret = i2c_smbus_read_byte_data(i2c_client, CS4270_CHIPID);
Daniel Mack wrote:
+/*
- cs4270_of_match - the device tree bindings
- */
+static const struct of_device_id cs4270_of_match[] = {
- { .compatible = "cirrus,cs4270", },
- { }
+}; +MODULE_DEVICE_TABLE(of, cs4270_of_match);
Why is this necessary? The CS4270 driver works just fine on our device-tree enabled boards.
On 24.07.2012 22:22, Timur Tabi wrote:
Daniel Mack wrote:
+/*
- cs4270_of_match - the device tree bindings
- */
+static const struct of_device_id cs4270_of_match[] = {
- { .compatible = "cirrus,cs4270", },
- { }
+}; +MODULE_DEVICE_TABLE(of, cs4270_of_match);
Why is this necessary? The CS4270 driver works just fine on our device-tree enabled boards.
How do you instanciate it then? Somehow the DT core must know which device to probe for a node, right?
Daniel
On 24.07.2012 22:30, Timur Tabi wrote:
Daniel Mack wrote:
How do you instanciate it then? Somehow the DT core must know which device to probe for a node, right?
drivers/of/of_i2c.c
It scans the device tree looking for I2C devices and registers them.
Yes, I'm aware of this, I just wonder how does your DT note looks like?
And are you ok with the 2nd patch?
Daniel
Daniel Mack wrote:
Yes, I'm aware of this, I just wonder how does your DT note looks like?
Take a look at arch/powerpc/boot/dts/mpc8610_hpcd.dts
And are you ok with the 2nd patch?
Isn't that something that should be done in platform code? It seems very platform-specific for a codec driver. I have no problem using the CS4270 on my board, and I don't need this feature.
- /* See if we way to bring the codec out of reset */
- if (np) {
enum of_gpio_flags reset_gpio_flags;
Blank line after variable declarations
int reset_gpio = of_get_named_gpio_flags(np, "reset-gpio", 0,&reset_gpio_flags);
Can you make this line shorter by using shorter variable names or something?
if (devm_gpio_request_one(&i2c_client->dev, reset_gpio,reset_gpio_flags & OF_GPIO_ACTIVE_LOW ?GPIOF_OUT_INIT_LOW : GPIOF_OUT_INIT_HIGH,"cs4270 reset") < 0) {reset_gpio = -EINVAL;}
I don't see where you test whether the reset-gpio property is present. It won't be present in my device tree.
- }
On 24.07.2012 22:39, Timur Tabi wrote:
Daniel Mack wrote:
Yes, I'm aware of this, I just wonder how does your DT note looks like?
Take a look at arch/powerpc/boot/dts/mpc8610_hpcd.dts
Interesting, I didn't know that works without enabling the specific string inside the driver. And that makes me wonder why many of the Wolfson codec have them (like wm8580). I might lack something here, maybe Mark can explain?
And are you ok with the 2nd patch?
Isn't that something that should be done in platform code? It seems very platform-specific for a codec driver.
Sure, the code I have at the moment does it that way, but the idea is to have little to no platform specific code. And also, if anything in the system needs to know when and how to drive the reset line, it should be the driver, right?
I have no problem using the CS4270 on my board, and I don't need this feature.
Because you care for that either in the bootloader or the platform code I believe?
- /* See if we way to bring the codec out of reset */
- if (np) {
enum of_gpio_flags reset_gpio_flags;Blank line after variable declarations
int reset_gpio = of_get_named_gpio_flags(np, "reset-gpio", 0,&reset_gpio_flags);Can you make this line shorter by using shorter variable names or something?
Right, fixed.
if (devm_gpio_request_one(&i2c_client->dev, reset_gpio,reset_gpio_flags & OF_GPIO_ACTIVE_LOW ?GPIOF_OUT_INIT_LOW : GPIOF_OUT_INIT_HIGH,"cs4270 reset") < 0) {reset_gpio = -EINVAL;}I don't see where you test whether the reset-gpio property is present. It won't be present in my device tree.
The idea is that reset_gpio will be < 0 in that case, and so devm_gpio_request_one() fails. So your platform should be ok and no further checks are necessary.
Thanks for the review, Daniel
Daniel Mack wrote:
Sure, the code I have at the moment does it that way, but the idea is to have little to no platform specific code. And also, if anything in the system needs to know when and how to drive the reset line, it should be the driver, right?
Maybe. We do have a lot of similar code in the boot loader, but of course that means that it's a static configuration.
I have no problem using the CS4270 on my board, and I don't need this feature.
Because you care for that either in the bootloader or the platform code I believe?
I don't have any platform code that initializes the codec.
Of course, that doesn't mean that you shouldn't need any. I'm not fundamentally opposed to your patch, I just don't have any context to go by.
Also, I have a gut feeling that if someone else needs to do the same thing, then this code:
devm_gpio_request_one(&i2c_client->dev, reset_gpio, reset_gpio_flags & OF_GPIO_ACTIVE_LOW ? GPIOF_OUT_INIT_LOW : GPIOF_OUT_INIT_HIGH, "cs4270 reset")
won't work for him, because it's not generic enough.
I don't see where you test whether the reset-gpio property is present. It won't be present in my device tree.
The idea is that reset_gpio will be < 0 in that case, and so devm_gpio_request_one() fails. So your platform should be ok and no further checks are necessary.
Well, I would prefer that you check for the property before making any calls that use it.
Also, I just noticed a typo here:
/* See if we way to bring the codec out of reset */
On 24.07.2012 23:01, Timur Tabi wrote:
Also, I have a gut feeling that if someone else needs to do the same thing, then this code:
devm_gpio_request_one(&i2c_client->dev, reset_gpio, reset_gpio_flags & OF_GPIO_ACTIVE_LOW ? GPIOF_OUT_INIT_LOW : GPIOF_OUT_INIT_HIGH, "cs4270 reset")
won't work for him, because it's not generic enough.
Why is that? The GPIO in the spec can be located anywhere on the board, either directly driven by the SoC or by any arbitrary other part on on the board that exposes itself as GPIO chip. And with the ACTIVE_LOW flag, it's even possible to compensate external circuitry that changes the polarity.
Which case can you imagine where that flexibility wouldn't suffice?
Daniel
On 07/24/2012 02:39 PM, Timur Tabi wrote:
Daniel Mack wrote:
Yes, I'm aware of this, I just wonder how does your DT note looks like?
Take a look at arch/powerpc/boot/dts/mpc8610_hpcd.dts
And are you ok with the 2nd patch?
Isn't that something that should be done in platform code? It seems very platform-specific for a codec driver. I have no problem using the CS4270 on my board, and I don't need this feature.
Perhaps your board permanently ties the reset line to de-asserted, or the bootloader/... deasserts the codecs reset pin before the kernel boots?
Stephen Warren wrote:
Perhaps your board permanently ties the reset line to de-asserted, or the bootloader/... deasserts the codecs reset pin before the kernel boots?
Probably. We have a pretty sophisticate FPGA on our reference boards that handles a lot of stuff like this.
On Tue, Jul 24, 2012 at 05:02:35PM -0500, Timur Tabi wrote:
Stephen Warren wrote:
Perhaps your board permanently ties the reset line to de-asserted, or the bootloader/... deasserts the codecs reset pin before the kernel boots?
Probably. We have a pretty sophisticate FPGA on our reference boards that handles a lot of stuff like this.
More likely it's just tied to whichever of ground or a supply holds the device out of reset, if it's the FPGA I'd expect you to have runtime control of it.
On Tue, Jul 24, 2012 at 03:39:21PM -0500, Timur Tabi wrote:
Daniel Mack wrote:
And are you ok with the 2nd patch?
...which requires the first one anyway.
Isn't that something that should be done in platform code? It seems very platform-specific for a codec driver. I have no problem using the CS4270 on my board, and I don't need this feature.
Using a reset signal on the CODEC isn't at all platform specific - it's something the device itself supports so I'd expect the driver for the device to support it so it can be joined up with it for register I/O and so on, besides there's not really anywhere else to put it in the DT.
On 07/24/2012 02:30 PM, Timur Tabi wrote:
Daniel Mack wrote:
How do you instanciate it then? Somehow the DT core must know which device to probe for a node, right?
drivers/of/of_i2c.c
It scans the device tree looking for I2C devices and registers them.
The I2C code matches the compatible values from the device tree against two tables in the kernel. First, the of_match_table that this patch adds, and then if there's no match it falls back to the I2C device ID table, and matches that against the DT compatible value with the vendor prefix stripped off.
So while the second method does work as a fall-back, in the past I've received guidance (I think from Grant Likely) that we should still add the of_match_table to drivers in order to be explicit.
Stephen Warren wrote:
The I2C code matches the compatible values from the device tree against two tables in the kernel. First, the of_match_table that this patch adds, and then if there's no match it falls back to the I2C device ID table, and matches that against the DT compatible value with the vendor prefix stripped off.
So while the second method does work as a fall-back, in the past I've received guidance (I think from Grant Likely) that we should still add the of_match_table to drivers in order to be explicit.
So you're saying that all I2C device drivers should have an of_match_table()? If so, then we should probably fix all codec drivers in one patch.
On Tue, Jul 24, 2012 at 05:03:35PM -0500, Timur Tabi wrote:
Stephen Warren wrote:
The I2C code matches the compatible values from the device tree against two tables in the kernel. First, the of_match_table that this patch adds, and then if there's no match it falls back to the I2C device ID table, and matches that against the DT compatible value with the vendor prefix stripped off.
So while the second method does work as a fall-back, in the past I've received guidance (I think from Grant Likely) that we should still add the of_match_table to drivers in order to be explicit.
So you're saying that all I2C device drivers should have an
It's certainly better to add one, it avoids any ambiguity in part numbers (there's at least one other chip vendor (Wondermedia) using wm for example).
of_match_table()? If so, then we should probably fix all codec drivers in one patch.
Meh, it's fine doing it piecemeal really. It's hardly urgent anyway.
On 25.07.2012 00:44, Mark Brown wrote:
On Tue, Jul 24, 2012 at 05:03:35PM -0500, Timur Tabi wrote:
Stephen Warren wrote:
The I2C code matches the compatible values from the device tree against two tables in the kernel. First, the of_match_table that this patch adds, and then if there's no match it falls back to the I2C device ID table, and matches that against the DT compatible value with the vendor prefix stripped off.
So while the second method does work as a fall-back, in the past I've received guidance (I think from Grant Likely) that we should still add the of_match_table to drivers in order to be explicit.
So you're saying that all I2C device drivers should have an
It's certainly better to add one, it avoids any ambiguity in part numbers (there's at least one other chip vendor (Wondermedia) using wm for example).
of_match_table()? If so, then we should probably fix all codec drivers in one patch.
Meh, it's fine doing it piecemeal really. It's hardly urgent anyway.
Ok, thanks for all the explanation. So I'll repost a cleaned up version of the 2nd patch then. Timur, are you ok with that?
Daniel
In the process of moving over from static board files to the device tree, reset pins of peripheral reset pins should be handled by their corresponding drivers.
Add a reset-gpio DT property to the cs4270 driver, and de-assert it before probing the chip. The logic could be augmented some day to re-assert it when codec is put to suspend.
Signed-off-by: Daniel Mack zonque@gmail.com Cc: Timur Tabi timur@freescale.com --- Documentation/devicetree/bindings/sound/cs4270.txt | 5 +++++ sound/soc/codecs/cs4270.c | 14 ++++++++++++++ 2 files changed, 19 insertions(+)
diff --git a/Documentation/devicetree/bindings/sound/cs4270.txt b/Documentation/devicetree/bindings/sound/cs4270.txt index 7f0bfd8..6b222f9 100644 --- a/Documentation/devicetree/bindings/sound/cs4270.txt +++ b/Documentation/devicetree/bindings/sound/cs4270.txt @@ -8,6 +8,11 @@ Required properties:
- reg : the I2C address of the device for I2C
+Optional properties: + + - reset-gpio : a GPIO spec for the reset pin. If specified, it will be + deasserted before communication to the codec starts. + Example:
codec: cs4270@48 { diff --git a/sound/soc/codecs/cs4270.c b/sound/soc/codecs/cs4270.c index 4b71b01..1b03058 100644 --- a/sound/soc/codecs/cs4270.c +++ b/sound/soc/codecs/cs4270.c @@ -30,6 +30,7 @@ #include <linux/delay.h> #include <linux/regulator/consumer.h> #include <linux/of_device.h> +#include <linux/of_gpio.h>
/* * The codec isn't really big-endian or little-endian, since the I2S @@ -660,9 +661,22 @@ MODULE_DEVICE_TABLE(of, cs4270_of_match); static int cs4270_i2c_probe(struct i2c_client *i2c_client, const struct i2c_device_id *id) { + struct device_node *np = i2c_client->dev.of_node; struct cs4270_private *cs4270; int ret;
+ /* See if we have a way to bring the codec out of reset */ + if (np) { + enum of_gpio_flags flags; + int gpio = of_get_named_gpio_flags(np, "reset-gpio", 0, &flags); + + if (gpio_is_valid(gpio)) + devm_gpio_request_one(&i2c_client->dev, gpio, + flags & OF_GPIO_ACTIVE_LOW ? + GPIOF_OUT_INIT_LOW : GPIOF_OUT_INIT_HIGH, + "cs4270 reset"); + } + /* Verify that we have a CS4270 */
ret = i2c_smbus_read_byte_data(i2c_client, CS4270_CHIPID);
On Wed, Jul 25, 2012 at 09:03:29AM +0200, Daniel Mack wrote:
In the process of moving over from static board files to the device tree, reset pins of peripheral reset pins should be handled by their corresponding drivers.
Add a reset-gpio DT property to the cs4270 driver, and de-assert it before probing the chip. The logic could be augmented some day to re-assert it when codec is put to suspend.
I'm missing 1/2... Please also don't bury patches in the middle of previous threads.
enum of_gpio_flags flags;int gpio = of_get_named_gpio_flags(np, "reset-gpio", 0, &flags);if (gpio_is_valid(gpio))devm_gpio_request_one(&i2c_client->dev, gpio,flags & OF_GPIO_ACTIVE_LOW ?GPIOF_OUT_INIT_LOW : GPIOF_OUT_INIT_HIGH,"cs4270 reset");
This ignores the return code and won't work well with probe deferral, if we manage to get a GPIO from the DT then we should fail if we're unable request it. Passing back the return code should get you deferral support for free in 3.6 and onwards.
On 25.07.2012 15:18, Mark Brown wrote:
On Wed, Jul 25, 2012 at 09:03:29AM +0200, Daniel Mack wrote:
In the process of moving over from static board files to the device tree, reset pins of peripheral reset pins should be handled by their corresponding drivers.
Add a reset-gpio DT property to the cs4270 driver, and de-assert it before probing the chip. The logic could be augmented some day to re-assert it when codec is put to suspend.
I'm missing 1/2... Please also don't bury patches in the middle of previous threads.
enum of_gpio_flags flags;int gpio = of_get_named_gpio_flags(np, "reset-gpio", 0, &flags);if (gpio_is_valid(gpio))devm_gpio_request_one(&i2c_client->dev, gpio,flags & OF_GPIO_ACTIVE_LOW ?GPIOF_OUT_INIT_LOW : GPIOF_OUT_INIT_HIGH,"cs4270 reset");This ignores the return code and won't work well with probe deferral, if we manage to get a GPIO from the DT then we should fail if we're unable request it. Passing back the return code should get you deferral support for free in 3.6 and onwards.
Ok, will resent both patches.
participants (4)
-
Daniel Mack -
Mark Brown -
Stephen Warren -
Timur Tabi