Skip to contentSkip to conversation

EDB93xx: Add support for CS4271 CODEC on EDB93xx boards

PATCH In review Alexander Sverdlin 4 participants 18 messages Feb 1, 2011 – Feb 2, 2011

Subsystems (3)
Arm (32-bit) Arm sub-architectures Arm/Cirrus logic EP93XX Arm architecture
lore
Share & tools
lore

Alexander Sverdlin

author
#0 lore raw

PATCH 1/1 EDB93xx: Add support for CS4271 CODEC on EDB93xx boards

From: Alexander Sverdlin <[email protected]>

Add support for CS4271 SPI-connected CODEC on EDB93xx boards.
Major rework based on code provided by H Hartley Sweeten <[email protected]>. Tested on EDB9302, others (EDB9301, EDB9302A, EDB9307A, EDB0315A) should work.

Signed-off-by Alexander Sverdlin <[email protected]>
---
 arch/arm/mach-ep93xx/edb93xx.c |  117 ++++++++++++++++++++++++++++++++++++++++
 1 files changed, 117 insertions(+), 0 deletions(-)
arch/arm/mach-ep93xx/edb93xx.c +117 -0
index 4b04316..f20be96 100644
@@ -30,12 +30,22 @@
 #include <linux/gpio.h>
 #include <linux/i2c.h>
 #include <linux/i2c-gpio.h>
+#include <linux/spi/spi.h>
+#include <linux/delay.h>
+#include <mach/ep93xx_spi.h>
 
 #include <mach/hardware.h>
 
 #include <asm/mach-types.h>
 #include <asm/mach/arch.h>
 
+#include <sound/cs4271.h>
+
+#define edb93xx_has_audio() (machine_is_edb9301() ||	\
+			     machine_is_edb9302() ||	\
+			     machine_is_edb9302a() ||	\
+			     machine_is_edb9307a() ||	\
+			     machine_is_edb9315a())
 
 static void __init edb93xx_register_flash(void)
 {

@@ -93,6 +103,111 @@ static void __init edb93xx_register_i2c(void)
 
 
 /*************************************************************************
+ * EDB93xx SPI peripheral handling
+ *************************************************************************/
+static int edb93xx_cs4271_hw_setup(struct spi_device *spi)
+{
+	int gpio_nreset;
+	int err;
+
+	if (machine_is_edb9301() || machine_is_edb9302()) {
+		gpio_nreset = EP93XX_GPIO_LINE_EGPIO1;
+	} else if (machine_is_edb9302a() || machine_is_edb9307a()) {
+		ep93xx_devcfg_set_bits(EP93XX_SYSCON_DEVCFG_HONIDE);
+		gpio_nreset = EP93XX_GPIO_LINE_DD2;
+	} else if (machine_is_edb9315a()) {
+		gpio_nreset = EP93XX_GPIO_LINE_EGPIO14;
+	} else {
+		return -EINVAL;
+	}
+
+	err = gpio_request(gpio_nreset, spi->modalias);
+	if (err)
+		return err;
+	err = gpio_request(EP93XX_GPIO_LINE_EGPIO6, spi->modalias);
+	if (err)
+		return err;
+
+	gpio_direction_output(EP93XX_GPIO_LINE_EGPIO6, 1);
+
+	/* Reset codec */
+	gpio_direction_output(gpio_nreset, 0);
+	udelay(1);
+	gpio_set_value(gpio_nreset, 1);
+	/* Give the codec time to wake up */
+	udelay(1);
+
+	return 0;
+}
+
+static void edb93xx_cs4271_hw_cleanup(struct spi_device *spi)
+{
+	int gpio_nreset;
+
+	if (machine_is_edb9301() || machine_is_edb9302())
+		gpio_nreset = EP93XX_GPIO_LINE_EGPIO1;
+	else if (machine_is_edb9302a() || machine_is_edb9307a())
+		gpio_nreset = EP93XX_GPIO_LINE_DD2;
+	else if (machine_is_edb9315a())
+		gpio_nreset = EP93XX_GPIO_LINE_EGPIO14;
+	else
+		return;
+
+	gpio_set_value(gpio_nreset, 0);
+	gpio_free(gpio_nreset);
+
+	gpio_set_value(EP93XX_GPIO_LINE_EGPIO6, 1);
+	gpio_free(EP93XX_GPIO_LINE_EGPIO6);
+}
+
+static void edb93xx_cs4271_hw_cs_control(struct spi_device *spi, int value)
+{
+	gpio_set_value(EP93XX_GPIO_LINE_EGPIO6, value);
+}
+
+static struct ep93xx_spi_chip_ops edb93xx_cs4271_hw = {
+	.setup		= edb93xx_cs4271_hw_setup,
+	.cleanup	= edb93xx_cs4271_hw_cleanup,
+	.cs_control	= edb93xx_cs4271_hw_cs_control,
+};
+
+static struct spi_board_info edb93xx_spi_board_info[] __initdata = {
+	{
+		.modalias		= "cs4271",
+		.controller_data	= &edb93xx_cs4271_hw,
+		.max_speed_hz		= 6000000,
+		.bus_num		= 0,
+		.chip_select		= 0,
+		.mode			= SPI_MODE_3,
+	},
+};
+
+static struct ep93xx_spi_info edb93xx_spi_info = {
+	.num_chipselect	= ARRAY_SIZE(edb93xx_spi_board_info),
+};
+
+static void __init edb93xx_register_spi(void)
+{
+	if (edb93xx_has_audio()) {
+		ep93xx_register_spi(&edb93xx_spi_info,
+				    edb93xx_spi_board_info,
+				    ARRAY_SIZE(edb93xx_spi_board_info));
+	}
+}
+
+
+/*************************************************************************
+ * EDB93xx I2S
+ *************************************************************************/
+static void __init edb93xx_register_i2s(void)
+{
+	if (edb93xx_has_audio()) {
+		ep93xx_register_i2s();
+	}
+}
+
+
+/*************************************************************************
  * EDB93xx pwm
  *************************************************************************/
 static void __init edb93xx_register_pwm(void)

@@ -117,6 +232,8 @@ static void __init edb93xx_init_machine(void)
 	edb93xx_register_flash();
 	ep93xx_register_eth(&edb93xx_eth_data, 1);
 	edb93xx_register_i2c();
+	edb93xx_register_spi();
+	edb93xx_register_i2s();
 	edb93xx_register_pwm();
 }
#1 lore raw
Quoted 41 lines · Alexander Sverdlin

From: Alexander Sverdlin <[email protected]>

Add support for CS4271 SPI-connected CODEC on EDB93xx boards.
Major rework based on code provided by H Hartley Sweeten <[email protected]>. Tested on EDB9302, others (EDB9301, EDB9302A, EDB9307A, EDB0315A) should work.

Signed-off-by Alexander Sverdlin <[email protected]>
---
 arch/arm/mach-ep93xx/edb93xx.c |  117 ++++++++++++++++++++++++++++++++++++++++
 1 files changed, 117 insertions(+), 0 deletions(-)
arch/arm/mach-ep93xx/edb93xx.c +10 -0
index 4b04316..f20be96 100644
@@ -30,12 +30,22 @@
 #include <linux/gpio.h>
 #include <linux/i2c.h>
 #include <linux/i2c-gpio.h>
+#include <linux/spi/spi.h>
+#include <linux/delay.h>
+#include <mach/ep93xx_spi.h>
 
 #include <mach/hardware.h>
 
 #include <asm/mach-types.h>
 #include <asm/mach/arch.h>
 
+#include <sound/cs4271.h>
+
+#define edb93xx_has_audio() (machine_is_edb9301() ||	\
+			     machine_is_edb9302() ||	\
+			     machine_is_edb9302a() ||	\
+			     machine_is_edb9307a() ||	\
+			     machine_is_edb9315a())
 
 static void __init edb93xx_register_flash(void)
 {

@@ -93,6 +103,111 @@ static void __init edb93xx_register_i2c(void)

arch/arm/mach-ep93xx/edb93xx.c static void __init edb93xx_register_i2c(void) · line 114

Quoted 10 lines
arch/arm/mach-ep93xx/edb93xx.c +9 -0
 /*************************************************************************
+ * EDB93xx SPI peripheral handling
+ *************************************************************************/
+static int edb93xx_cs4271_hw_setup(struct spi_device *spi)
+{
+	int gpio_nreset;
+	int err;
+
+	if (machine_is_edb9301() || machine_is_edb9302()) {
+		gpio_nreset = EP93XX_GPIO_LINE_EGPIO1;

Are you planning on removing gpio_nreset and gpio_disable from struct cs4271_platform_data? If not, you should just setup the gpio mapping in edb93xx_register_spi before you actually register the spi_board_info. Then this function and the following two can just do something like the following and not worry about the if ... else if ... etc.

	struct cs4271_platform_data *cs4271 = spi->dev.platform_data;
	int err;

	err = gpio_request(cs4271->gpio_disable, spi->modalias);
	...

arch/arm/mach-ep93xx/edb93xx.c static void __init edb93xx_register_i2c(void) · line 116

Quoted 2 lines
arch/arm/mach-ep93xx/edb93xx.c +2 -0
+	} else if (machine_is_edb9302a() || machine_is_edb9307a()) {
+		ep93xx_devcfg_set_bits(EP93XX_SYSCON_DEVCFG_HONIDE);

You shouldn't need this. The ep93xx gpio core will now defaults all the ports to gpio mode. See commit fd015480c29deb52ae3bfaf41e888c450765edd8.

arch/arm/mach-ep93xx/edb93xx.c static void __init edb93xx_register_i2c(void) · line 117

Quoted 1 line
arch/arm/mach-ep93xx/edb93xx.c +1 -0
+		gpio_nreset = EP93XX_GPIO_LINE_DD2;

Please use EP93XX_GPIO_LINE_H(2) instead. None of the datasheets reference the "DD2" gpio.

Quoted 97 lines
arch/arm/mach-ep93xx/edb93xx.c +93 -0
+	} else if (machine_is_edb9315a()) {
+		gpio_nreset = EP93XX_GPIO_LINE_EGPIO14;
+	} else {
+		return -EINVAL;
+	}
+
+	err = gpio_request(gpio_nreset, spi->modalias);
+	if (err)
+		return err;
+	err = gpio_request(EP93XX_GPIO_LINE_EGPIO6, spi->modalias);
+	if (err)
+		return err;
+
+	gpio_direction_output(EP93XX_GPIO_LINE_EGPIO6, 1);
+
+	/* Reset codec */
+	gpio_direction_output(gpio_nreset, 0);
+	udelay(1);
+	gpio_set_value(gpio_nreset, 1);
+	/* Give the codec time to wake up */
+	udelay(1);
+
+	return 0;
+}
+
+static void edb93xx_cs4271_hw_cleanup(struct spi_device *spi)
+{
+	int gpio_nreset;
+
+	if (machine_is_edb9301() || machine_is_edb9302())
+		gpio_nreset = EP93XX_GPIO_LINE_EGPIO1;
+	else if (machine_is_edb9302a() || machine_is_edb9307a())
+		gpio_nreset = EP93XX_GPIO_LINE_DD2;
+	else if (machine_is_edb9315a())
+		gpio_nreset = EP93XX_GPIO_LINE_EGPIO14;
+	else
+		return;
+
+	gpio_set_value(gpio_nreset, 0);
+	gpio_free(gpio_nreset);
+
+	gpio_set_value(EP93XX_GPIO_LINE_EGPIO6, 1);
+	gpio_free(EP93XX_GPIO_LINE_EGPIO6);
+}
+
+static void edb93xx_cs4271_hw_cs_control(struct spi_device *spi, int value)
+{
+	gpio_set_value(EP93XX_GPIO_LINE_EGPIO6, value);
+}
+
+static struct ep93xx_spi_chip_ops edb93xx_cs4271_hw = {
+	.setup		= edb93xx_cs4271_hw_setup,
+	.cleanup	= edb93xx_cs4271_hw_cleanup,
+	.cs_control	= edb93xx_cs4271_hw_cs_control,
+};
+
+static struct spi_board_info edb93xx_spi_board_info[] __initdata = {
+	{
+		.modalias		= "cs4271",
+		.controller_data	= &edb93xx_cs4271_hw,
+		.max_speed_hz		= 6000000,
+		.bus_num		= 0,
+		.chip_select		= 0,
+		.mode			= SPI_MODE_3,
+	},
+};
+
+static struct ep93xx_spi_info edb93xx_spi_info = {
+	.num_chipselect	= ARRAY_SIZE(edb93xx_spi_board_info),
+};
+
+static void __init edb93xx_register_spi(void)
+{
+	if (edb93xx_has_audio()) {
+		ep93xx_register_spi(&edb93xx_spi_info,
+				    edb93xx_spi_board_info,
+				    ARRAY_SIZE(edb93xx_spi_board_info));
+	}
+}
+
+
+/*************************************************************************
+ * EDB93xx I2S
+ *************************************************************************/
+static void __init edb93xx_register_i2s(void)
+{
+	if (edb93xx_has_audio()) {
+		ep93xx_register_i2s();
+	}
+}
+
+
+/*************************************************************************
  * EDB93xx pwm
  *************************************************************************/
 static void __init edb93xx_register_pwm(void)

arch/arm/mach-ep93xx/edb93xx.c static void __init edb93xx_init_machine(void) · line 239

Quoted 8 lines
arch/arm/mach-ep93xx/edb93xx.c +2 -0
@@ -117,6 +232,8 @@ static void __init edb93xx_init_machine(void)
 	edb93xx_register_flash();
 	ep93xx_register_eth(&edb93xx_eth_data, 1);
 	edb93xx_register_i2c();
+	edb93xx_register_spi();
+	edb93xx_register_i2s();
 	edb93xx_register_pwm();
 }

Other than using struct cs4271_platform_data to hold the gpio information, this looks better. Now other spi devices can live on bus.

Hartley

author
#2 lore raw

Dear Hartley, Dear Mark,

Quoted 4 lines · H Hartley Sweeten
Quoted 4 lines
arch/arm/mach-ep93xx/edb93xx.c +4 -0
+static int edb93xx_cs4271_hw_setup(struct spi_device *spi)
+{
+	int gpio_nreset;
+	int err;
Quoted 8 lines

arch/arm/mach-ep93xx/edb93xx.c static void __init edb93xx_register_i2c(void) · line 114

Quoted 3 lines
arch/arm/mach-ep93xx/edb93xx.c +3 -0
+
+	if (machine_is_edb9301() || machine_is_edb9302()) {
+		gpio_nreset = EP93XX_GPIO_LINE_EGPIO1;

Are you planning on removing gpio_nreset and gpio_disable from struct cs4271_platform_data? If not, you should just setup the gpio mapping in edb93xx_register_spi before you actually register the spi_board_info. Then this function and the following two can just do something like the following and not worry about the if ... else if ... etc.

Initially I had GPIO code in machine driver, but during the conversations in alsa-devel list it has been moved to CODEC driver:
http://mailman.alsa-project.org/pipermail/alsa-devel/2010-October/032358.html

I'm really confused now, where should it be? If I will init cs4271_platform_data with GPIO values, CODEC driver will manage it. Otherwise CODEC driver should be rewritten without GPIO support and I can add it to platform code.

Mark Brown, could you please comment on this, as I've moved GPIO things to CODEC part based on your review.

Best regards,
Alexander Sverdlin.

#3 lore raw
Quoted 4 lines · Alexander Sverdlin

I'm really confused now, where should it be? If I will init cs4271_platform_data with GPIO values, CODEC driver will manage it. Otherwise CODEC driver should be rewritten without GPIO support and I can add it to platform code.

What is the problem?

author
#4 lore raw

Dear Mark,

Quoted 9 lines · Mark Brown
Quoted 4 lines · Alexander Sverdlin

I'm really confused now, where should it be? If I will init cs4271_platform_data with GPIO values, CODEC driver will manage it. Otherwise CODEC driver should be rewritten without GPIO support and I can add it to platform code.

What is the problem?

On 07 OCT 2010 you advised to integrate GPIO handling (reset and SPI-enable) into CODEC code flow.

Now Hartley suggested to move GPIO handling to platform code. This makes sense, as SPI-enable will be managed automatically, and reset is also more related to platform (depending on schematics, inversion and so on).

As for me, I think handle it in platform code is better. So we can now remove all the GPIO code and cs4271_platform_data structure from CS4271 CODEC.

Best regards,
Alexander.

#5 lore raw
Quoted 6 lines · Alexander Sverdlin

On 07 OCT 2010 you advised to integrate GPIO handling (reset and SPI-enable) into CODEC code flow.

Now Hartley suggested to move GPIO handling to platform code. This makes sense, as SPI-enable will be managed automatically, and reset is also more related to platform (depending on schematics, inversion and so on).

What is the advantage of this? The GPIO management has to be done somewhere and replicating it in every platform seems like a complete waste of time. Given that one of the GPIOs is a power control it'll also mean that it's not possible to manage the power dynamically at runtime.

author
#6 lore raw

Dear Mark, Dear Hartley,

Quoted 10 lines · Mark Brown
Quoted 3 lines

Now Hartley suggested to move GPIO handling to platform code. This makes sense, as SPI-enable will be managed automatically, and reset is also more related to platform (depending on schematics, inversion and so on).

What is the advantage of this? The GPIO management has to be done somewhere and replicating it in every platform seems like a complete waste of time. Given that one of the GPIOs is a power control it'll also mean that it's not possible to manage the power dynamically at runtime.

I think we can leave reset management in CODEC (as it will be almost the same for all platforms), leave enable pin management in CODEC unused, but attach enable GPIO to spi_board_info in platform to enable other SPI devices on bus.

Hartley, in this case we cannot use existing cs4271_platform_data and have to leave long if-else statemens as is. Would it be ok?

Best regards,
Alexander.

#7 lore raw
Quoted 15 lines · Alexander Sverdlin

Dear Mark, Dear Hartley,

Quoted 10 lines · Mark Brown
Quoted 3 lines

Now Hartley suggested to move GPIO handling to platform code. This makes sense, as SPI-enable will be managed automatically, and reset is also more related to platform (depending on schematics, inversion and so on).

What is the advantage of this? The GPIO management has to be done somewhere and replicating it in every platform seems like a complete waste of time. Given that one of the GPIOs is a power control it'll also mean that it's not possible to manage the power dynamically at runtime.

Quoted 7 lines

I think we can leave reset management in CODEC (as it will be almost the same for all platforms), leave enable pin management in CODEC unused, but attach enable GPIO to spi_board_info in platform to enable other SPI devices on bus.

Hartley, in this case we cannot use existing cs4271_platform_data and have to leave long if-else statemens as is. Would it be ok?

I don't have any problem with leaving the reset management in the codec. The enable (chip-select) should be handled in the platform code. But, gpio_disable should be removed from struct cs4271_platform_data to avoid any confusion.

I have an idea on how to remove the if-else mess in the ep93xx platform init. Please make any updates you have and post the patches. I will comment on it then.

Thanks,
Hartley

#8 lore raw
Quoted 4 lines · H Hartley Sweeten

I don't have any problem with leaving the reset management in the codec. The enable (chip-select) should be handled in the platform code. But, gpio_disable should be removed from struct cs4271_platform_data to avoid any confusion.

Is that really a chip select or is it a power control?

#9 lore raw
Quoted 9 lines · Mark Brown
Quoted 4 lines · H Hartley Sweeten

I don't have any problem with leaving the reset management in the codec. The enable (chip-select) should be handled in the platform code. But, gpio_disable should be removed from struct cs4271_platform_data to avoid any confusion.

Is that really a chip select or is it a power control?

From the datasheet:

Pin Name   #  Pin Description
--------  --  ----------------------------------------------------------
AD0/nCS   13  Address Bit 0 (I2S)/Control Port Chip Select (SPI) (Input)

- AD0 is a chip address pin in I2C mode; nCS is the chip signal select for SPI format.

The other pin in question:

Pin Name   #  Pin Description
--------  --  ----------------------------------------------------------
nRST      14  Reset (Input) - The device enters a low power mode when

this pin is driven low.

Regards,
Hartley

author
#10 lore raw
In reply to Mark Brown

Dear Mark,

Quoted 9 lines · Mark Brown
Quoted 4 lines · H Hartley Sweeten

I don't have any problem with leaving the reset management in the codec. The enable (chip-select) should be handled in the platform code. But, gpio_disable should be removed from struct cs4271_platform_data to avoid any confusion.

Is that really a chip select or is it a power control?

It's a chip select. The way I've managed it in CODEC it's because Cirrus boards do not have any other SPI devices. Supported by current mainline, at least.

#11 lore raw
Quoted 3 lines · Alexander Sverdlin

It's a chip select. The way I've managed it in CODEC it's because Cirrus boards do not have any other SPI devices. Supported by current mainline, at least.

All the EDB93xx boards also have an spi flash device. And there are a couple drivers in mainline that can be used. Depending on the actual spi flash device: drivers/mtd/m25p80.c, drivers/mtd/sst25l.c, or /drivers/misc/eeprom/at25.c.

There are a couple patches floating around to add the spi flash support to the edb93xx platform init but nothing is merged yet.

BTW, you really should remove the or-gate's that Cirrus used with the spi chip-selects to logically-and the SFRM and GPIO signals. The SFRM signal deasserts when the spi fifo's empty which could cause a chip-select deassertion to the chip in the middle of a transfer. For the spi flash, this breaks any read's from the device. Just connect the gpio line directly to the chip-select of the device.

Regards,
Hartley

#12 lore raw
In reply to Alexander Sverdlin
Quoted 3 lines · Alexander Sverdlin

It's a chip select. The way I've managed it in CODEC it's because Cirrus boards do not have any other SPI devices. Supported by current mainline, at least.

Please do this properly with the SPI core.

#13 lore raw
In reply to Alexander Sverdlin
Quoted 7 lines · Alexander Sverdlin

I think we can leave reset management in CODEC (as it will be almost the same for all platforms), leave enable pin management in CODEC unused, but attach enable GPIO to spi_board_info in platform to enable other SPI devices on bus.

Hartley, in this case we cannot use existing cs4271_platform_data and have to leave long if-else statemens as is. Would it be ok?

Alexander,

Below is what I propose for the platform init code. This assumes that the reset gpio will still be handled by the codec since it's related to power management.

Regards,
Hartley

static struct cs4271_platform_data edb93xx_cs4271_data = {
	.gpio_nreset		= -EINVAL,	/* filled in later */
};

static int edb93xx_cs4271_hw_setup(struct spi_device *spi)

{
	return gpio_request_one(EP93XX_GPIO_LINE_EGPIO6,
				GPIOF_OUT_INIT_HIGH, spi->modalias);
}

static void edb93xx_cs4271_hw_cleanup(struct spi_device *spi)

{
	gpio_free(EP93XX_GPIO_LINE_EGPIO6);
}

static void edb93xx_cs4271_hw_cs_control(struct spi_device *spi, int value)

{
	gpio_set_value(EP93XX_GPIO_LINE_EGPIO6, value);
}
static struct ep93xx_spi_chip_ops edb93xx_cs4721_hw  = {
	.setup		= edb93xx_cs4271_hw_setup,
	.cleanup	= edb93xx_cs4271_hw_cleanup,
	.cs_control	= edb93xx_cs4271_hw_cs_control,
};
static struct spi_board_info edb93xx_spi_board_info[] __initdata = {
	{
		.modalias		= "cs4271",
		.platform_data		= &edb93xx_cs4271_data,
		.controller_data	= &edb93xx_cs4721_hw,
		.max_speed_hz		= 6000000,
		.bus_num		= 0,
		.chip_select		= 0,
		.mode			= SPI_MODE_3,
	},
};
static struct ep93xx_spi_info edb93xx_spi_master __initdata = {
	.num_chipselect		= ARRAY_SIZE(edb93xx_spi_board_info),
};

static void __init edb93xx_register_spi(void)

{
	if (machine_is_edb9301() || machine_is_edb9302()) {
		edb93xx_cs4271_data.gpio_nreset = EP93XX_GPIO_LINE_EGPIO1;
	} else if (machine_is_edb9302a() || machine_is_edb9307a()) {
		edb93xx_cs4271_data.gpio_nreset = EP93XX_GPIO_LINE_H(2);
	} else if (machine_is_edb9315a()) {
		edb93xx_cs4271_data.gpio_nreset = EP93XX_GPIO_LINE_EGPIO14;
	}

	ep93xx_register_spi(&edb93xx_spi_master,
			    edb93xx_spi_board_info,
			    ARRAY_SIZE(edb93xx_spi_board_info));
}
#14 lore raw
In reply to Alexander Sverdlin
Quoted 22 lines · Alexander Sverdlin

Dear Hartley, Dear Mark,

Quoted 5 lines · H Hartley Sweeten
Quoted 5 lines
arch/arm/mach-ep93xx/edb93xx.c +5 -0
+static int edb93xx_cs4271_hw_setup(struct spi_device *spi)
+{
+	int gpio_nreset;
+	int err;
+
Quoted 8 lines

Review

Quoted 2 lines
(quoted diff) +2 -0
+	if (machine_is_edb9301() || machine_is_edb9302()) {
+		gpio_nreset = EP93XX_GPIO_LINE_EGPIO1;

Are you planning on removing gpio_nreset and gpio_disable from struct cs4271_platform_data? If not, you should just setup the gpio mapping in edb93xx_register_spi before you actually register the spi_board_info. Then this function and the following two can just do something like the following and not worry about the if ... else if ... etc.

Initially I had GPIO code in machine driver, but during the conversations in alsa-devel list it has been moved to CODEC driver:
http://mailman.alsa-project.org/pipermail/alsa-devel/2010-October/032358.html

Quoted 7 lines

I'm really confused now, where should it be? If I will init cs4271_platform_data with GPIO values, CODEC driver will manage it. Otherwise CODEC driver should be rewritten without GPIO support and I can add it to platform code.

Mark Brown, could you please comment on this, as I've moved GPIO things to CODEC part based on your review.

The chip-select request/configuration needs to be done before loading the driver so that the spi subsystem can actually select the chip. Also, some spi master drivers might not even use a 'gpio' for the chip-select. They might have dedicated pins that are used.

For the reset, again a gpio might not actually be used. The systems reset signal might be directly connected to the codec.

Regardless, the cs4271 codec driver isn't doing anything with the gpios except in the probe function. And all it's does there is request and configure them, drive the chip-select gpio low (permenatly enabing it), and then toggles the reset line to reset the codec.

It appears to me that none of this should be in the codec driver itself. It's really platform specific and serves no use in the driver. The <sound/cs4271.h> header should just be removed. Let the platform deal with the non-generic configuration and setup of the gpio's.

Regards,
Hartley

#15 lore raw
In reply to Alexander Sverdlin
Quoted 5 lines · Alexander Sverdlin

From: Alexander Sverdlin <[email protected]>

Add support for CS4271 SPI-connected CODEC on EDB93xx boards.
Major rework based on code provided by H Hartley Sweeten <[email protected]>. Tested on EDB9302, others (EDB9301, EDB9302A, EDB9307A, EDB0315A) should work.

One minor comment, see below.

Quoted 50 lines
Signed-off-by Alexander Sverdlin <[email protected]>
---
 arch/arm/mach-ep93xx/edb93xx.c |  117 ++++++++++++++++++++++++++++++++++++++++
 1 files changed, 117 insertions(+), 0 deletions(-)
arch/arm/mach-ep93xx/edb93xx.c +23 -0
index 4b04316..f20be96 100644
@@ -30,12 +30,22 @@
 #include <linux/gpio.h>
 #include <linux/i2c.h>
 #include <linux/i2c-gpio.h>
+#include <linux/spi/spi.h>
+#include <linux/delay.h>
+#include <mach/ep93xx_spi.h>
 
 #include <mach/hardware.h>
 
 #include <asm/mach-types.h>
 #include <asm/mach/arch.h>
 
+#include <sound/cs4271.h>
+
+#define edb93xx_has_audio() (machine_is_edb9301() ||	\
+			     machine_is_edb9302() ||	\
+			     machine_is_edb9302a() ||	\
+			     machine_is_edb9307a() ||	\
+			     machine_is_edb9315a())
 
 static void __init edb93xx_register_flash(void)
 {

@@ -93,6 +103,111 @@ static void __init edb93xx_register_i2c(void)
 
 
 /*************************************************************************
+ * EDB93xx SPI peripheral handling
+ *************************************************************************/
+static int edb93xx_cs4271_hw_setup(struct spi_device *spi)
+{
+	int gpio_nreset;
+	int err;
+
+	if (machine_is_edb9301() || machine_is_edb9302()) {
+		gpio_nreset = EP93XX_GPIO_LINE_EGPIO1;
+	} else if (machine_is_edb9302a() || machine_is_edb9307a()) {
+		ep93xx_devcfg_set_bits(EP93XX_SYSCON_DEVCFG_HONIDE);
+		gpio_nreset = EP93XX_GPIO_LINE_DD2;
+	} else if (machine_is_edb9315a()) {

arch/arm/mach-ep93xx/edb93xx.c static void __init edb93xx_register_i2c(void) · line 128

Quoted 10 lines
arch/arm/mach-ep93xx/edb93xx.c +10 -0
+		gpio_nreset = EP93XX_GPIO_LINE_EGPIO14;
+	} else {
+		return -EINVAL;
+	}
+
+	err = gpio_request(gpio_nreset, spi->modalias);
+	if (err)
+		return err;
+	err = gpio_request(EP93XX_GPIO_LINE_EGPIO6, spi->modalias);
+	if (err)

Should you call gpio_free() for gpio_nreset here?

Regards,
MW

Quoted 13 lines
arch/arm/mach-ep93xx/edb93xx.c +13 -0
+		return err;
+
+	gpio_direction_output(EP93XX_GPIO_LINE_EGPIO6, 1);
+
+	/* Reset codec */
+	gpio_direction_output(gpio_nreset, 0);
+	udelay(1);
+	gpio_set_value(gpio_nreset, 1);
+	/* Give the codec time to wake up */
+	udelay(1);
+
+	return 0;
+}
#16 lore raw
Quoted 18 lines · Mika Westerberg
Quoted 18 lines

/*************************************************************************
+ * EDB93xx SPI peripheral handling
+ *************************************************************************/

arch/arm/mach-ep93xx/edb93xx.c +15 -0
+static int edb93xx_cs4271_hw_setup(struct spi_device *spi)
+{
+	int gpio_nreset;
+	int err;
+
+	if (machine_is_edb9301() || machine_is_edb9302()) {
+		gpio_nreset = EP93XX_GPIO_LINE_EGPIO1;
+	} else if (machine_is_edb9302a() || machine_is_edb9307a()) {
+		ep93xx_devcfg_set_bits(EP93XX_SYSCON_DEVCFG_HONIDE);
+		gpio_nreset = EP93XX_GPIO_LINE_DD2;
+	} else if (machine_is_edb9315a()) {
+		gpio_nreset = EP93XX_GPIO_LINE_EGPIO14;
+	} else {
+		return -EINVAL;
+	}
Quoted 8 lines

arch/arm/mach-ep93xx/edb93xx.c static void __init edb93xx_register_i2c(void) · line 128

Quoted 6 lines
arch/arm/mach-ep93xx/edb93xx.c +6 -0
+
+	err = gpio_request(gpio_nreset, spi->modalias);
+	if (err)
+		return err;
+	err = gpio_request(EP93XX_GPIO_LINE_EGPIO6, spi->modalias);
+	if (err)

Should you call gpio_free() for gpio_nreset here?

Yes. If the second gpio_request fails, the first gpio should be freed.

A cleaner way of handling the gpios would be to use gpio_request_array() and remove the gpio knowledge from the codec driver completely.

Hartley

#17 lore raw
In reply to Alexander Sverdlin

arch/arm/mach-ep93xx/edb93xx.c static void __init edb93xx_register_i2c(void) · line 131

Quoted 8 lines · Alexander Sverdlin
arch/arm/mach-ep93xx/edb93xx.c +8 -0
+	err = gpio_request(gpio_nreset, spi->modalias);
+	if (err)
+		return err;
+	err = gpio_request(EP93XX_GPIO_LINE_EGPIO6, spi->modalias);
+	if (err)
+		return err;
+
+	gpio_direction_output(EP93XX_GPIO_LINE_EGPIO6, 1);

Why is this stuff in the arch/arm code? I'd expect driver to deal with all this sequencing.