Skip to contentSkip to conversation

ASoC: da7210: Add support for spi regmap

PATCH Superseded Ashish Chavan 2 participants 6 messages Mar 21, 2012

Subsystems (3)

v1 of 2 Superseded Latest is v2 →

lore
Share & tools
lore
#0 lore raw

PATCH 1/1 ASoC: da7210: Add support for spi regmap

This patch adds support for spi regmap feature to existing da7210 driver.

---
 sound/soc/codecs/da7210.c |   90 +++++++++++++++++++++++++++++++++++++++++++++
 1 files changed, 90 insertions(+), 0 deletions(-)
sound/soc/codecs/da7210.c +90 -0
index 7843711..0f13fc4 100644
@@ -17,6 +17,7 @@
 
 #include <linux/delay.h>
 #include <linux/i2c.h>
+#include <linux/spi/spi.h>
 #include <linux/regmap.h>
 #include <linux/slab.h>
 #include <linux/module.h>
@@ -905,6 +906,9 @@ static int da7210_probe(struct snd_soc_codec *codec)
 {
 	struct da7210_priv *da7210 = snd_soc_codec_get_drvdata(codec);
 	int ret;
+#if defined(CONFIG_SPI_MASTER)
+	unsigned int val;
+#endif
 
 	dev_info(codec->dev, "DA7210 Audio Codec %s\n", DA7210_VERSION);
 
@@ -927,10 +931,23 @@ static int da7210_probe(struct snd_soc_codec *codec)
 	 *      DA7210_PLL_DIV3 :: DA7210_MCLK_RANGExxx
 	 */
 
+#if defined(CONFIG_SPI_MASTER)
+	/* Dummy read to give two pulses over nCS */
+	regmap_read(da7210->regmap, DA7210_STATUS, &val);
+	regmap_read(da7210->regmap, DA7210_STATUS, &val);
+	regmap_read(da7210->regmap, DA7210_STATUS, &val);
+#endif
+
 	/*
 	 * make sure that DA7210 use bypass mode before start up
 	 */
 	snd_soc_write(codec, DA7210_STARTUP1, 0);
+
+#if defined(CONFIG_SPI_MASTER)
+	/* Dummy read to complete last write operation */
+	regmap_read(da7210->regmap, DA7210_STATUS, &val);
+#endif
+
 	snd_soc_write(codec, DA7210_PLL_DIV3,
 		     DA7210_MCLK_RANGE_10_20_MHZ | DA7210_PLL_BYP);
 
@@ -1025,6 +1042,10 @@ static int da7210_probe(struct snd_soc_codec *codec)
 		     DA7210_MCLK_RANGE_10_20_MHZ | DA7210_PLL_BYP);
 	snd_soc_update_bits(codec, DA7210_PLL, DA7210_PLL_EN, DA7210_PLL_EN);
 
+#if defined(CONFIG_SPI_MASTER)
+	snd_soc_write(codec, 0x00, 0x80);
+#endif
+
 	/* As suggested by Dialog */
 	/* unlock */
 	regmap_write(da7210->regmap, DA7210_A_HID_UNLOCK,	0x8B);
@@ -1035,6 +1056,10 @@ static int da7210_probe(struct snd_soc_codec *codec)
 	regmap_write(da7210->regmap, DA7210_A_HID_UNLOCK,	0x00);
 	regmap_write(da7210->regmap, DA7210_A_TEST_UNLOCK,	0x00);
 
+#if defined(CONFIG_SPI_MASTER)
+	snd_soc_write(codec, 0x00, 0x00);
+#endif
+
 	/* Activate all enabled subsystem */
 	snd_soc_write(codec, DA7210_STARTUP1, DA7210_SC_MST_EN);
 
@@ -1128,12 +1153,74 @@ static struct i2c_driver da7210_i2c_driver = {
 };
 #endif
 
+#if defined(CONFIG_SPI_MASTER)
+static int __devinit da7210_spi_probe(struct spi_device *spi)
+{
+	struct da7210_priv *da7210;
+	int ret;
+
+	da7210 = kzalloc(sizeof(struct da7210_priv), GFP_KERNEL);
+	if (!da7210)
+		return -ENOMEM;
+
+	spi_set_drvdata(spi, da7210);
+
+	da7210_regmap.read_flag_mask = 0x01;
+	da7210_regmap.write_flag_mask = 0x00;
+	da7210->regmap = regmap_init_spi(spi, &da7210_regmap);
+	if (IS_ERR(da7210->regmap)) {
+		ret = PTR_ERR(da7210->regmap);
+		dev_err(&spi->dev, "Failed to register regmap: %d\n", ret);
+		goto err_alloc;
+	}
+
+	ret =  snd_soc_register_codec(&spi->dev,
+			&soc_codec_dev_da7210, &da7210_dai, 1);
+	if (ret < 0)
+		goto err_regmap;
+
+	return ret;
+
+err_regmap:
+	regmap_exit(da7210->regmap);
+err_alloc:
+	kfree(da7210);
+
+	return ret;
+}
+
+static int __devexit da7210_spi_remove(struct spi_device *spi)
+{
+	struct da7210_priv *da7210 = spi_get_drvdata(spi);
+	snd_soc_unregister_codec(&spi->dev);
+	regmap_exit(da7210->regmap);
+	kfree(da7210);
+	return 0;
+}
+
+static struct spi_driver da7210_spi_driver = {
+	.driver = {
+		.name = "da7210-codec",
+		.owner = THIS_MODULE,
+	},
+	.probe = da7210_spi_probe,
+	.remove = __devexit_p(da7210_spi_remove)
+};
+#endif
+
 static int __init da7210_modinit(void)
 {
 	int ret = 0;
 #if defined(CONFIG_I2C) || defined(CONFIG_I2C_MODULE)
 	ret = i2c_add_driver(&da7210_i2c_driver);
 #endif
+#if defined(CONFIG_SPI_MASTER)
+	ret = spi_register_driver(&da7210_spi_driver);
+	if (ret) {
+		printk(KERN_ERR "Failed to register da7210 SPI driver: %d\n",
+		       ret);
+	}
+#endif
 	return ret;
 }
 module_init(da7210_modinit);
@@ -1143,6 +1230,9 @@ static void __exit da7210_exit(void)
 #if defined(CONFIG_I2C) || defined(CONFIG_I2C_MODULE)
 	i2c_del_driver(&da7210_i2c_driver);
 #endif
+#if defined(CONFIG_SPI_MASTER)
+	spi_unregister_driver(&da7210_spi_driver);
+#endif
 }
 module_exit(da7210_exit);
 
Signature · 1 line
1.7.1
#1 lore raw

sound/soc/codecs/da7210.c static int da7210_probe(struct snd_soc_codec *codec) · line 939

Quoted 6 lines · Ashish Chavan
sound/soc/codecs/da7210.c +6 -0
+#if defined(CONFIG_SPI_MASTER)
+	/* Dummy read to give two pulses over nCS */
+	regmap_read(da7210->regmap, DA7210_STATUS, &val);
+	regmap_read(da7210->regmap, DA7210_STATUS, &val);
+	regmap_read(da7210->regmap, DA7210_STATUS, &val);
+#endif

This ifdef stuff should all be runtime configured based on the bus type, unless the cost of the reads is considered immaterial in which case it should be unconditional.

sound/soc/codecs/da7210.c static int da7210_probe(struct snd_soc_codec *codec) · line 1047

Quoted 3 lines
sound/soc/codecs/da7210.c +3 -0
+#if defined(CONFIG_SPI_MASTER)
+	snd_soc_write(codec, 0x00, 0x80);
+#endif

Hrm?

sound/soc/codecs/da7210.c static int da7210_probe(struct snd_soc_codec *codec) · line 1057

Quoted 5 lines
sound/soc/codecs/da7210.c +0 -0
 	/* unlock */
 	regmap_write(da7210->regmap, DA7210_A_HID_UNLOCK,	0x8B);
@@ -1035,6 +1056,10 @@ static int da7210_probe(struct snd_soc_codec *codec)
 	regmap_write(da7210->regmap, DA7210_A_HID_UNLOCK,	0x00);
 	regmap_write(da7210->regmap, DA7210_A_TEST_UNLOCK,	0x00);

I also note that you've not yet updated this to use a regmap patch as was previously requested.

sound/soc/codecs/da7210.c static struct i2c_driver da7210_i2c_driver = { · line 1164

Quoted 9 lines
sound/soc/codecs/da7210.c +9 -0
+#if defined(CONFIG_SPI_MASTER)
+static int __devinit da7210_spi_probe(struct spi_device *spi)
+{
+	struct da7210_priv *da7210;
+	int ret;
+
+	da7210 = kzalloc(sizeof(struct da7210_priv), GFP_KERNEL);
+	if (!da7210)
+		return -ENOMEM;

devm_kzalloc().

sound/soc/codecs/da7210.c static struct i2c_driver da7210_i2c_driver = { · line 1169

Quoted 2 lines
sound/soc/codecs/da7210.c +2 -0
+	da7210_regmap.read_flag_mask = 0x01;
+	da7210_regmap.write_flag_mask = 0x00;

Just have a second, static, regmap variable. The regmap should be declared const.

sound/soc/codecs/da7210.c static struct i2c_driver da7210_i2c_driver = { · line 1170

Quoted 1 line
sound/soc/codecs/da7210.c +1 -0
+	da7210->regmap = regmap_init_spi(spi, &da7210_regmap);

devm_regmap_init_spi() (will come in in the merge window).

sound/soc/codecs/da7210.c static struct i2c_driver da7210_i2c_driver = { · line 1203

Quoted 3 lines
sound/soc/codecs/da7210.c +3 -0
+static struct spi_driver da7210_spi_driver = {
+	.driver = {
+		.name = "da7210-codec",

No -codec.

#2 lore raw
Quoted 13 lines · Mark Brown

sound/soc/codecs/da7210.c static int da7210_probe(struct snd_soc_codec *codec) · line 939

Quoted 6 lines · Ashish Chavan
sound/soc/codecs/da7210.c +6 -0
+#if defined(CONFIG_SPI_MASTER)
+	/* Dummy read to give two pulses over nCS */
+	regmap_read(da7210->regmap, DA7210_STATUS, &val);
+	regmap_read(da7210->regmap, DA7210_STATUS, &val);
+	regmap_read(da7210->regmap, DA7210_STATUS, &val);
+#endif

This ifdef stuff should all be runtime configured based on the bus type, unless the cost of the reads is considered immaterial in which case it should be unconditional.

OK, will make it runtime.

Quoted 5 lines

sound/soc/codecs/da7210.c static int da7210_probe(struct snd_soc_codec *codec) · line 1047

Quoted 3 lines
sound/soc/codecs/da7210.c +3 -0
+#if defined(CONFIG_SPI_MASTER)
+	snd_soc_write(codec, 0x00, 0x80);
+#endif

Hrm?

This is writing to page register. SPI register space is divided in to two pages. Registers from 0x01 to 0x80 fall in to first page. If we want to write to any register above 0x80, first we need to set page register with PAGE1. May be I should put comments to make it obvious.

Quoted 9 lines

sound/soc/codecs/da7210.c static int da7210_probe(struct snd_soc_codec *codec) · line 1057

Quoted 6 lines
sound/soc/codecs/da7210.c +0 -0
 	/* unlock */
 	regmap_write(da7210->regmap, DA7210_A_HID_UNLOCK,	0x8B);
@@ -1035,6 +1056,10 @@ static int da7210_probe(struct snd_soc_codec *codec)
 	regmap_write(da7210->regmap, DA7210_A_HID_UNLOCK,	0x00);
 	regmap_write(da7210->regmap, DA7210_A_TEST_UNLOCK,	0x00);

I also note that you've not yet updated this to use a regmap patch as was previously requested.

Actually I don't have enough details about the problem that you mentioned with this. I am waiting for inputs from some body else. Can you please elaborate the problem if you have details? so that I can correct it.

Quoted 12 lines

sound/soc/codecs/da7210.c static struct i2c_driver da7210_i2c_driver = { · line 1164

Quoted 10 lines
sound/soc/codecs/da7210.c +9 -0
+#if defined(CONFIG_SPI_MASTER)
+static int __devinit da7210_spi_probe(struct spi_device *spi)
+{
+	struct da7210_priv *da7210;
+	int ret;
+
+	da7210 = kzalloc(sizeof(struct da7210_priv), GFP_KERNEL);
+	if (!da7210)
+		return -ENOMEM;

devm_kzalloc().

OK for this and rest of all comments. I will take care of them in next submission soon.

#3 lore raw
Quoted 6 lines · Ashish Chavan

On Wed, 2012-03-21 at 15:56 +0000, Mark Brown wrote:

This is writing to page register. SPI register space is divided in to two pages. Registers from 0x01 to 0x80 fall in to first page. If we want to write to any register above 0x80, first we need to set page register with PAGE1. May be I should put comments to make it obvious.

You also need to make sure that the register cache doesn't get confused.

Quoted 11 lines
Quoted 8 lines

sound/soc/codecs/da7210.c static int da7210_probe(struct snd_soc_codec *codec) · line 1057

Quoted 5 lines
sound/soc/codecs/da7210.c +0 -0
 	/* unlock */
 	regmap_write(da7210->regmap, DA7210_A_HID_UNLOCK,	0x8B);
@@ -1035,6 +1056,10 @@ static int da7210_probe(struct snd_soc_codec *codec)
 	regmap_write(da7210->regmap, DA7210_A_HID_UNLOCK,	0x00);
 	regmap_write(da7210->regmap, DA7210_A_TEST_UNLOCK,	0x00);

I also note that you've not yet updated this to use a regmap patch as was previously requested.

Actually I don't have enough details about the problem that you mentioned with this. I am waiting for inputs from some body else. Can

...and didn't bother asking any questions so it's unlikely anyone will say anything...

Quoted 2 lines

you please elaborate the problem if you have details? so that I can correct it.

What is unclear in the previous feedback? You should be converting this to use a regmap patch...

#4 lore raw
Quoted 6 lines
Quoted 3 lines

Actually I don't have enough details about the problem that you mentioned with this. I am waiting for inputs from some body else. Can

...and didn't bother asking any questions so it's unlikely anyone will say anything...

No, actually it's not like that. You mentioned that the sequence should be changed, which will fix an issue that the driver has with suspend and resume. I asked other users of this driver about the issue and waiting for their feedback. I just wanted to have clear picture about the issue before I commit any change there. Just to make sure that only changing of sequence is enough or some thing extra need to be done.

Quoted 6 lines
Quoted 3 lines

you please elaborate the problem if you have details? so that I can correct it.

What is unclear in the previous feedback? You should be converting this to use a regmap patch...

OK, thanks!

#5 lore raw
Quoted 6 lines · Ashish Chavan
Quoted 5 lines
Quoted 2 lines

Actually I don't have enough details about the problem that you mentioned with this. I am waiting for inputs from some body else. Can

...and didn't bother asking any questions so it's unlikely anyone will say anything...

Quoted 6 lines

No, actually it's not like that. You mentioned that the sequence should be changed, which will fix an issue that the driver has with suspend and resume. I asked other users of this driver about the issue and waiting for their feedback. I just wanted to have clear picture about the issue before I commit any change there. Just to make sure that only changing of sequence is enough or some thing extra need to be done.

No, I didn't say that the sequence should be changed. I said that it should be implemented using the relevant framework features rather than being open coded. The sequence itself is fine from a framework point of view.