[PATCH] EDB93xx: Add support for CS4271 CODEC on EDB93xx boards

[PATCH] EDB93xx: Add support for CS4271 CODEC on EDB93xx boards

[Alexander Sverdlin]

From: Alexander Sverdlin <subaparts@yandex.ru>

Add support for CS4271 SPI-connected CODEC on EDB93xx boards.
Major rework based on code provided by H Hartley Sweeten <hsweeten@visionengravers.com>.
Tested on EDB9302, others (EDB9301, EDB9302A, EDB9307A, EDB0315A) should work.
        
Signed-off-by: Alexander Sverdlin <subaparts@yandex.ru>
---
 arch/arm/mach-ep93xx/edb93xx.c |  117 ++++++++++++++++++++++++++++++++++++++++
 1 files changed, 117 insertions(+), 0 deletions(-)

diff --git a/arch/arm/mach-ep93xx/edb93xx.c b/arch/arm/mach-ep93xx/edb93xx.c
index 4b04316..f20be96 100644
--- a/arch/arm/mach-ep93xx/edb93xx.c
+++ b/arch/arm/mach-ep93xx/edb93xx.c
@@ -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();
 }

[PATCH] EDB93xx: Add support for CS4271 CODEC on EDB93xx boards

[Alexander Sverdlin]

From: Alexander Sverdlin <subaparts@yandex.ru>

Add support for CS4271 SPI-connected CODEC on EDB93xx boards.
Major rework based on code provided by H Hartley Sweeten <hsweeten@visionengravers.com>.
Tested on EDB9302, others (EDB9301, EDB9302A, EDB9307A, EDB0315A) should work.
        
Signed-off-by: Alexander Sverdlin <subaparts@yandex.ru>
---
 arch/arm/mach-ep93xx/edb93xx.c |  117 ++++++++++++++++++++++++++++++++++++++++
 1 files changed, 117 insertions(+), 0 deletions(-)

diff --git a/arch/arm/mach-ep93xx/edb93xx.c b/arch/arm/mach-ep93xx/edb93xx.c
index 4b04316..f20be96 100644
--- a/arch/arm/mach-ep93xx/edb93xx.c
+++ b/arch/arm/mach-ep93xx/edb93xx.c
@@ -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();
 }
 

RE: [PATCH] EDB93xx: Add support for CS4271 CODEC on EDB93xx boards

[H Hartley Sweeten]

On Tuesday, February 01, 2011 4:41 PM, Alexander Sverdlin wrote:
> From: Alexander Sverdlin <subaparts@yandex.ru>
> 
> Add support for CS4271 SPI-connected CODEC on EDB93xx boards.
> Major rework based on code provided by H Hartley Sweeten <hsweeten@visionengravers.com>.
> Tested on EDB9302, others (EDB9301, EDB9302A, EDB9307A, EDB0315A) should work.
>         
> Signed-off-by: Alexander Sverdlin <subaparts@yandex.ru>
> ---
>  arch/arm/mach-ep93xx/edb93xx.c |  117 ++++++++++++++++++++++++++++++++++++++++
>  1 files changed, 117 insertions(+), 0 deletions(-)
> 
> diff --git a/arch/arm/mach-ep93xx/edb93xx.c b/arch/arm/mach-ep93xx/edb93xx.c
> index 4b04316..f20be96 100644
> --- a/arch/arm/mach-ep93xx/edb93xx.c
> +++ b/arch/arm/mach-ep93xx/edb93xx.c
> @@ -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;

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);
	...


> +	} 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.

> +		gpio_nreset = EP93XX_GPIO_LINE_DD2;

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

> +	} 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();
>  }
>  


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

Hartley

[PATCH] EDB93xx: Add support for CS4271 CODEC on EDB93xx boards

[H Hartley Sweeten]

On Tuesday, February 01, 2011 4:41 PM, Alexander Sverdlin wrote:
> From: Alexander Sverdlin <subaparts@yandex.ru>
> 
> Add support for CS4271 SPI-connected CODEC on EDB93xx boards.
> Major rework based on code provided by H Hartley Sweeten <hsweeten@visionengravers.com>.
> Tested on EDB9302, others (EDB9301, EDB9302A, EDB9307A, EDB0315A) should work.
>         
> Signed-off-by: Alexander Sverdlin <subaparts@yandex.ru>
> ---
>  arch/arm/mach-ep93xx/edb93xx.c |  117 ++++++++++++++++++++++++++++++++++++++++
>  1 files changed, 117 insertions(+), 0 deletions(-)
> 
> diff --git a/arch/arm/mach-ep93xx/edb93xx.c b/arch/arm/mach-ep93xx/edb93xx.c
> index 4b04316..f20be96 100644
> --- a/arch/arm/mach-ep93xx/edb93xx.c
> +++ b/arch/arm/mach-ep93xx/edb93xx.c
> @@ -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;

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);
	...


> +	} 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.

> +		gpio_nreset = EP93XX_GPIO_LINE_DD2;

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

> +	} 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();
>  }
>  


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

Hartley

Re: [PATCH] EDB93xx: Add support for CS4271 CODEC on EDB93xx boards

[Alexander Sverdlin]

Dear Hartley, Dear Mark,

On Tue, 2011-02-01 at 18:02 -0600, H Hartley Sweeten wrote:
> > +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.

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.

[PATCH] EDB93xx: Add support for CS4271 CODEC on EDB93xx boards

[Alexander Sverdlin]

Dear Hartley, Dear Mark,

On Tue, 2011-02-01 at 18:02 -0600, H Hartley Sweeten wrote:
> > +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.

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.

Re: [PATCH] EDB93xx: Add support for CS4271 CODEC on EDB93xx boards

[Mark Brown]

On Wed, Feb 02, 2011 at 01:48:08PM +0300, Alexander Sverdlin wrote:

> 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?

[PATCH] EDB93xx: Add support for CS4271 CODEC on EDB93xx boards

[Mark Brown]

On Wed, Feb 02, 2011 at 01:48:08PM +0300, Alexander Sverdlin wrote:

> 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?

Re: [PATCH] EDB93xx: Add support for CS4271 CODEC on EDB93xx boards

[Alexander Sverdlin]

Dear Mark,

On Wed, 2011-02-02 at 10:49 +0000, Mark Brown wrote:
> On Wed, Feb 02, 2011 at 01:48:08PM +0300, Alexander Sverdlin wrote:
> 
> > 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.

[PATCH] EDB93xx: Add support for CS4271 CODEC on EDB93xx boards

[Alexander Sverdlin]

Dear Mark,

On Wed, 2011-02-02 at 10:49 +0000, Mark Brown wrote:
> On Wed, Feb 02, 2011 at 01:48:08PM +0300, Alexander Sverdlin wrote:
> 
> > 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.

Re: [PATCH] EDB93xx: Add support for CS4271 CODEC on EDB93xx boards

[Mark Brown]

On Wed, Feb 02, 2011 at 02:12:47PM +0300, Alexander Sverdlin wrote:

> 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.

[PATCH] EDB93xx: Add support for CS4271 CODEC on EDB93xx boards

[Mark Brown]

On Wed, Feb 02, 2011 at 02:12:47PM +0300, Alexander Sverdlin wrote:

> 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.

Re: [PATCH] EDB93xx: Add support for CS4271 CODEC on EDB93xx boards

[Alexander Sverdlin]

Dear Mark, Dear Hartley,

On Wed, 2011-02-02 at 12:53 +0000, Mark Brown wrote:
> > 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.

[PATCH] EDB93xx: Add support for CS4271 CODEC on EDB93xx boards

[Alexander Sverdlin]

Dear Mark, Dear Hartley,

On Wed, 2011-02-02 at 12:53 +0000, Mark Brown wrote:
> > 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.

RE: [PATCH] EDB93xx: Add support for CS4271 CODEC on EDB93xx boards

[H Hartley Sweeten]

On Wednesday, February 02, 2011 6:27 AM, Alexander Sverdlin wrote:
> 
> Dear Mark, Dear Hartley,
> 
> On Wed, 2011-02-02 at 12:53 +0000, Mark Brown wrote:
>>> 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?

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

[PATCH] EDB93xx: Add support for CS4271 CODEC on EDB93xx boards

[H Hartley Sweeten]

On Wednesday, February 02, 2011 6:27 AM, Alexander Sverdlin wrote:
> 
> Dear Mark, Dear Hartley,
> 
> On Wed, 2011-02-02 at 12:53 +0000, Mark Brown wrote:
>>> 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?

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

Re: [PATCH] EDB93xx: Add support for CS4271 CODEC on EDB93xx boards

[Mark Brown]

On Wed, Feb 02, 2011 at 10:53:15AM -0600, H Hartley Sweeten wrote:

> 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?

[PATCH] EDB93xx: Add support for CS4271 CODEC on EDB93xx boards

[Mark Brown]

On Wed, Feb 02, 2011 at 10:53:15AM -0600, H Hartley Sweeten wrote:

> 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?

RE: [PATCH] EDB93xx: Add support for CS4271 CODEC on EDB93xx boards

[H Hartley Sweeten]

On Wednesday, February 02, 2011 10:00 AM, Mark Brown wrote:
> On Wed, Feb 02, 2011 at 10:53:15AM -0600, H Hartley Sweeten wrote:
>> 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

[PATCH] EDB93xx: Add support for CS4271 CODEC on EDB93xx boards

[H Hartley Sweeten]

On Wednesday, February 02, 2011 10:00 AM, Mark Brown wrote:
> On Wed, Feb 02, 2011 at 10:53:15AM -0600, H Hartley Sweeten wrote:
>> 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?

Re: [PATCH] EDB93xx: Add support for CS4271 CODEC on EDB93xx boards

[Alexander Sverdlin]

Dear Mark,

On Wed, 2011-02-02 at 17:00 +0000, Mark Brown wrote:
> On Wed, Feb 02, 2011 at 10:53:15AM -0600, H Hartley Sweeten wrote:
> 
> > 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.

[PATCH] EDB93xx: Add support for CS4271 CODEC on EDB93xx boards

[Alexander Sverdlin]

Dear Mark,

On Wed, 2011-02-02 at 17:00 +0000, Mark Brown wrote:
> On Wed, Feb 02, 2011 at 10:53:15AM -0600, H Hartley Sweeten wrote:
> 
> > 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.

RE: [PATCH] EDB93xx: Add support for CS4271 CODEC on EDB93xx boards

[H Hartley Sweeten]

On Wednesday, February 02, 2011 10:33 AM, Alexander Sverdlin wrote:
> 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

[PATCH] EDB93xx: Add support for CS4271 CODEC on EDB93xx boards

[H Hartley Sweeten]

On Wednesday, February 02, 2011 10:33 AM, Alexander Sverdlin wrote:
> 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

Re: [PATCH] EDB93xx: Add support for CS4271 CODEC on EDB93xx boards

[Mark Brown]

On Wed, Feb 02, 2011 at 08:33:19PM +0300, Alexander Sverdlin wrote:

> 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.

[PATCH] EDB93xx: Add support for CS4271 CODEC on EDB93xx boards

[Mark Brown]

On Wed, Feb 02, 2011 at 08:33:19PM +0300, Alexander Sverdlin wrote:

> 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.

RE: [PATCH] EDB93xx: Add support for CS4271 CODEC on EDB93xx boards

[H Hartley Sweeten]

On Wednesday, February 02, 2011 6:27 AM, Alexander Sverdlin wrote:
> 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));
}

[PATCH] EDB93xx: Add support for CS4271 CODEC on EDB93xx boards

[H Hartley Sweeten]

On Wednesday, February 02, 2011 6:27 AM, Alexander Sverdlin wrote:
> 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));
}

RE: [PATCH] EDB93xx: Add support for CS4271 CODEC on EDB93xx boards

[H Hartley Sweeten]

On Wednesday, February 02, 2011 3:48 AM, Alexander Sverdlin wrote:
>
> Dear Hartley, Dear Mark,
>
> On Tue, 2011-02-01 at 18:02 -0600, H Hartley Sweeten wrote:
>>> +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.
>
> 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.

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

[PATCH] EDB93xx: Add support for CS4271 CODEC on EDB93xx boards

[H Hartley Sweeten]

On Wednesday, February 02, 2011 3:48 AM, Alexander Sverdlin wrote:
>
> Dear Hartley, Dear Mark,
>
> On Tue, 2011-02-01 at 18:02 -0600, H Hartley Sweeten wrote:
>>> +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.
>
> 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.

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

Re: [PATCH] EDB93xx: Add support for CS4271 CODEC on EDB93xx boards

[Mika Westerberg]

On Wed, Feb 02, 2011 at 02:40:53AM +0300, Alexander Sverdlin wrote:
> From: Alexander Sverdlin <subaparts@yandex.ru>
> 
> Add support for CS4271 SPI-connected CODEC on EDB93xx boards.
> Major rework based on code provided by H Hartley Sweeten <hsweeten@visionengravers.com>.
> Tested on EDB9302, others (EDB9301, EDB9302A, EDB9307A, EDB0315A) should work.

One minor comment, see below.

>         
> Signed-off-by: Alexander Sverdlin <subaparts@yandex.ru>
> ---
>  arch/arm/mach-ep93xx/edb93xx.c |  117 ++++++++++++++++++++++++++++++++++++++++
>  1 files changed, 117 insertions(+), 0 deletions(-)
> 
> diff --git a/arch/arm/mach-ep93xx/edb93xx.c b/arch/arm/mach-ep93xx/edb93xx.c
> index 4b04316..f20be96 100644
> --- a/arch/arm/mach-ep93xx/edb93xx.c
> +++ b/arch/arm/mach-ep93xx/edb93xx.c
> @@ -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)

Should you call gpio_free() for gpio_nreset here?

Regards,
MW

> +		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;
> +}

[alsa-devel] [PATCH] EDB93xx: Add support for CS4271 CODEC on EDB93xx boards

[Mika Westerberg]

On Wed, Feb 02, 2011 at 02:40:53AM +0300, Alexander Sverdlin wrote:
> From: Alexander Sverdlin <subaparts@yandex.ru>
> 
> Add support for CS4271 SPI-connected CODEC on EDB93xx boards.
> Major rework based on code provided by H Hartley Sweeten <hsweeten@visionengravers.com>.
> Tested on EDB9302, others (EDB9301, EDB9302A, EDB9307A, EDB0315A) should work.

One minor comment, see below.

>         
> Signed-off-by: Alexander Sverdlin <subaparts@yandex.ru>
> ---
>  arch/arm/mach-ep93xx/edb93xx.c |  117 ++++++++++++++++++++++++++++++++++++++++
>  1 files changed, 117 insertions(+), 0 deletions(-)
> 
> diff --git a/arch/arm/mach-ep93xx/edb93xx.c b/arch/arm/mach-ep93xx/edb93xx.c
> index 4b04316..f20be96 100644
> --- a/arch/arm/mach-ep93xx/edb93xx.c
> +++ b/arch/arm/mach-ep93xx/edb93xx.c
> @@ -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)

Should you call gpio_free() for gpio_nreset here?

Regards,
MW

> +		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;
> +}

RE: [alsa-devel] [PATCH] EDB93xx: Add support for CS4271 CODEC on EDB93xx boards

[H Hartley Sweeten]

On Wednesday, February 02, 2011 12:55 AM, Mika Westerberg wrote:
>>  /*************************************************************************
>> + * 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)
>
> 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

[alsa-devel] [PATCH] EDB93xx: Add support for CS4271 CODEC on EDB93xx boards

[H Hartley Sweeten]

On Wednesday, February 02, 2011 12:55 AM, Mika Westerberg wrote:
>>  /*************************************************************************
>> + * 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)
>
> 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

Re: [PATCH] EDB93xx: Add support for CS4271 CODEC on EDB93xx boards

[Mark Brown]

On Wed, Feb 02, 2011 at 02:40:53AM +0300, Alexander Sverdlin wrote:

> +	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.

[alsa-devel] [PATCH] EDB93xx: Add support for CS4271 CODEC on EDB93xx boards

[Mark Brown]

On Wed, Feb 02, 2011 at 02:40:53AM +0300, Alexander Sverdlin wrote:

> +	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.

[PATCH] EDB93xx: Add support for CS4271 CODEC on EDB93xx boards

[Alexander Sverdlin]

From: Alexander Sverdlin <subaparts@yandex.ru>

Add support for CS4271 SPI-connected CODEC on EDB93xx boards.
        
Signed-off-by: Alexander Sverdlin <subaparts@yandex.ru>
---
 arch/arm/mach-ep93xx/edb93xx.c |   54 ++++++++++++++++++++++++++++++++++++++++
 1 files changed, 54 insertions(+), 0 deletions(-)

diff --git a/arch/arm/mach-ep93xx/edb93xx.c b/arch/arm/mach-ep93xx/edb93xx.c
index 4b04316..9f9193d 100644
--- a/arch/arm/mach-ep93xx/edb93xx.c
+++ b/arch/arm/mach-ep93xx/edb93xx.c
@@ -30,12 +30,21 @@
 #include <linux/gpio.h>
 #include <linux/i2c.h>
 #include <linux/i2c-gpio.h>
+#include <linux/spi/spi.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 +102,49 @@ static void __init edb93xx_register_i2c(void)
 
 
 /*************************************************************************
+ * EDB93xx SPI peripheral handling
+ *************************************************************************/
+static struct cs4271_platform_data cs4271_edb93xx_gpio = {
+	.gpio_nreset	= EP93XX_GPIO_LINE_EGPIO1,
+	.gpio_disable	= EP93XX_GPIO_LINE_EGPIO6,
+};
+
+static struct spi_board_info edb93xx_spi_board_info[] = {
+	{
+		.modalias	= "cs4271",
+		.max_speed_hz	= 6000000,
+		.bus_num	= 0,
+		.chip_select	= 0,
+		.mode		= SPI_MODE_3,
+		.platform_data	= &cs4271_edb93xx_gpio,
+	},
+};
+
+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 +169,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();
 }

Re: [PATCH] EDB93xx: Add support for CS4271 CODEC on EDB93xx boards

[H Hartley Sweeten]

On Saturday, January 22, 2011 5:51 PM, Alexander Sverdlin wrote:
>
> Add support for CS4271 SPI-connected CODEC on EDB93xx boards.
>       
> Signed-off-by: Alexander Sverdlin <subaparts@yandex.ru>

Alexander,

Where can I find the patches that add the codec and soc support?

I would like to test this patch on an edb9307a but need those in order to
continue.

Thanks,
Hartley

Re: [PATCH] EDB93xx: Add support for CS4271 CODEC on EDB93xx boards

[Mark Brown]

On Mon, Jan 24, 2011 at 10:30:01AM -0600, H Hartley Sweeten wrote:

> Where can I find the patches that add the codec and soc support?

> I would like to test this patch on an edb9307a but need those in order to
> continue.

He posted them to alsa-devel, the CODEC support was merged already.

Re: [PATCH] EDB93xx: Add support for CS4271 CODEC on EDB93xx boards

[Alexander Sverdlin]

Dear Mark,

On Tue, 2011-01-25 at 11:46 +0000, Mark Brown wrote:
> > Where can I find the patches that add the codec and soc support?
> 
> > I would like to test this patch on an edb9307a but need those in order to
> > continue.
> 
> He posted them to alsa-devel, the CODEC support was merged already.


Sorry, didn't CCed to all, but I've sent all of them to Hartley and even
received first suggestions regarding platform code. Will try to
implement them when there will be some time for this again.

Best regards,
	Alexander.