Re: [RFC 2.6.26-rc3 06/10] gumstix: conversion to MFP support and add bluetooth support

[Eric Miao <eric.y.miao@gmail.com>]

Jaya Kumar wrote:
> This patch converts gumstix over to the MFP configuration system and adds
> bluetooth support. Added a CLK_32K clock to enable bluetooth.
> 

Overall looks ok to me, some concerns about the 32K timer, see my
comments below.

> Signed-off-by: Jaya Kumar <jayakumar.lkml@gmail.com>
> ---
>  arch/arm/mach-pxa/gumstix.c           |   79 ++++++++++++++++++++++++++-------
>  arch/arm/mach-pxa/pxa25x.c            |   31 +++++++++++++
>  include/asm-arm/arch-pxa/mfp-pxa25x.h |    1 +
>  3 files changed, 95 insertions(+), 16 deletions(-)
> 
> diff --git a/arch/arm/mach-pxa/gumstix.c b/arch/arm/mach-pxa/gumstix.c
> index bdf2397..3e0a962 100644
> --- a/arch/arm/mach-pxa/gumstix.c
> +++ b/arch/arm/mach-pxa/gumstix.c
> @@ -20,8 +20,12 @@
>  #include <linux/init.h>
>  #include <linux/platform_device.h>
>  #include <linux/interrupt.h>
> +#include <linux/delay.h>
>  #include <linux/mtd/mtd.h>
>  #include <linux/mtd/partitions.h>
> +#include <linux/gpio.h>
> +#include <linux/err.h>
> +#include <linux/clk.h>
>  
>  #include <asm/setup.h>
>  #include <asm/memory.h>
> @@ -40,7 +44,7 @@
>  
>  #include <asm/arch/pxa-regs.h>
>  #include <asm/arch/pxa2xx-regs.h>
> -#include <asm/arch/pxa2xx-gpio.h>
> +#include <asm/arch/mfp-pxa25x.h>
>  
>  #include "generic.h"
>  
> @@ -85,21 +89,8 @@ static struct platform_device *devices[] __initdata = {
>  };
>  
>  #ifdef CONFIG_MMC_PXA

I'd suggest we use

#if defined(CONFIG_MMC_PXA) || defined(CONFIG_MMC_PXA_MODULE)

so that even when PXA MMCI is built as a module, the platform data
is still available for later bind. Same below to the BT.

> -static struct pxamci_platform_data gumstix_mci_platform_data;
> -
> -static int gumstix_mci_init(struct device *dev, irq_handler_t detect_int,
> -				void *data)
> -{
> -	pxa_gpio_mode(GPIO6_MMCCLK_MD);
> -	pxa_gpio_mode(GPIO53_MMCCLK_MD);
> -	pxa_gpio_mode(GPIO8_MMCCS0_MD);
> -
> -	return 0;
> -}
> -
>  static struct pxamci_platform_data gumstix_mci_platform_data = {
>  	.ocr_mask	= MMC_VDD_32_33|MMC_VDD_33_34,
> -	.init		= gumstix_mci_init,
>  };
>  
>  static void __init gumstix_mmc_init(void)
> @@ -109,7 +100,7 @@ static void __init gumstix_mmc_init(void)
>  #else
>  static void __init gumstix_mmc_init(void)
>  {
> -	printk(KERN_INFO "Gumstix mmc disabled\n");
> +	pr_debug("Gumstix mmc disabled\n");
>  }
>  #endif
>  
> @@ -126,12 +117,68 @@ static void __init gumstix_udc_init(void)
>  #else
>  static void gumstix_udc_init(void)
>  {
> -	printk(KERN_INFO "Gumstix udc is disabled\n");
> +	pr_debug("Gumstix udc is disabled\n");
> +}
> +#endif
> +
> +#ifdef CONFIG_BT
> +static void __init gumstix_bluetooth_init(void)
> +{
> +	int err;
> +	struct clk *clk32k;
> +
> +	clk32k = clk_get(NULL, "CLK_32K");
> +	if (IS_ERR(clk32k)) {
> +		pr_err("gumstix: failed to find 32kHz clock\n");
> +		return;
> +	}
> +	clk_enable(clk32k);

The 32KHz oscillator should be on by default, so I'm thinking of:

1. remove this clock source

or

2. enable/disable the clk by enable/disable the pin configuration
(i.e. switch between GPIO12_32KHz and GPIO12_GPIO)

> +
> +	err = gpio_direction_output(GPIO_GUMSTIX_BTRESET, 1);
> +	if (err) {
> +		pr_err("gumstix: can't reset bluetooth\n");
> +		return;
> +	}
> +	gpio_set_value(GPIO_GUMSTIX_BTRESET, 0);
> +	udelay(100);
> +	gpio_set_value(GPIO_GUMSTIX_BTRESET, 1);
> +}
> +#else
> +static void gumstix_bluetooth_init(void)
> +{
> +	pr_debug("Gumstix Bluetooth is disabled\n");
>  }
>  #endif
>  
> +static unsigned long gumstix_pin_config[] __initdata = {
> +#ifdef CONFIG_BT
> +	GPIO12_32KHz,
> +	/* BTUART */
> +	GPIO42_HWUART_RXD,
> +	GPIO43_HWUART_TXD,
> +	GPIO44_HWUART_CTS,
> +	GPIO45_HWUART_RTSL,
> +#endif
> +#ifdef CONFIG_MMC_PXA
> +	/* MMC */
> +	GPIO6_MMC_CLK,
> +	GPIO53_MMC_CLK,
> +	GPIO8_MMC_CS0,
> +#endif
> +	/* these are used by AM200EPD */
> +	GPIO51_GPIO,
> +	GPIO49_GPIO,
> +	GPIO48_GPIO,
> +	GPIO32_GPIO,
> +	GPIO17_GPIO,
> +	GPIO16_GPIO,
> +};
> +
>  static void __init gumstix_init(void)
>  {
> +	pxa2xx_mfp_config(ARRAY_AND_SIZE(gumstix_pin_config));
> +
> +	gumstix_bluetooth_init();
>  	gumstix_udc_init();
>  	gumstix_mmc_init();
>  	(void) platform_add_devices(devices, ARRAY_SIZE(devices));
> diff --git a/arch/arm/mach-pxa/pxa25x.c b/arch/arm/mach-pxa/pxa25x.c
> index e5b417d..1ffe773 100644
> --- a/arch/arm/mach-pxa/pxa25x.c
> +++ b/arch/arm/mach-pxa/pxa25x.c
> @@ -22,6 +22,7 @@
>  #include <linux/platform_device.h>
>  #include <linux/suspend.h>
>  #include <linux/sysdev.h>
> +#include <linux/delay.h>
>  
>  #include <asm/hardware.h>
>  #include <asm/arch/irqs.h>
> @@ -108,6 +109,30 @@ static const struct clkops clk_pxa25x_lcd_ops = {
>  	.getrate	= clk_pxa25x_lcd_getrate,
>  };
>  
> +static void clk_32k_enable(struct clk *clk)
> +{
> +	int timeout = 500;
> +
> +	OSCC |= OSCC_OON;
> +	do {
> +		if (OSCC & OSCC_OOK)
> +			break;
> +		udelay(1);
> +	} while (--timeout);
> +	if (!timeout)
> +		pr_err("Failed to start 32kHz clock\n");
> +}

As said, the 32K timer should really be started on the very begining,
I'd prefer a some kind of warning here if OSCC_OOK isn't set.

> +
> +static void clk_32k_disable(struct clk *clk)
> +{
> +	OSCC &= ~OSCC_OON;
> +}
> +

The 32K timer cannot be disabled once started, until a power-on or
hardware reset occurs.

> +static const struct clkops clk_32k_ops = {
> +	.enable		= clk_32k_enable,
> +	.disable	= clk_32k_disable,
> +};
> +
>  /*
>   * 3.6864MHz -> OST, GPIO, SSP, PWM, PLLs (95.842MHz, 147.456MHz)
>   * 95.842MHz -> MMC 19.169MHz, I2C 31.949MHz, FICP 47.923MHz, USB 47.923MHz
> @@ -118,6 +143,12 @@ static struct clk pxa25x_hwuart_clk =
>  ;
>  
>  static struct clk pxa25x_clks[] = {
> +	{
> +		.name           = "CLK_32K",
> +		.ops            = &clk_32k_ops,
> +		.rate           = 32000,
> +		.delay          = 70,
> +	},
>  	INIT_CK("LCDCLK", LCD, &clk_pxa25x_lcd_ops, &pxa_device_fb.dev),
>  	INIT_CKEN("UARTCLK", FFUART, 14745600, 1, &pxa_device_ffuart.dev),
>  	INIT_CKEN("UARTCLK", BTUART, 14745600, 1, &pxa_device_btuart.dev),
> diff --git a/include/asm-arm/arch-pxa/mfp-pxa25x.h b/include/asm-arm/arch-pxa/mfp-pxa25x.h
> index 0499323..6c82a74 100644
> --- a/include/asm-arm/arch-pxa/mfp-pxa25x.h
> +++ b/include/asm-arm/arch-pxa/mfp-pxa25x.h
> @@ -79,6 +79,7 @@
>  #define GPIO43_HWUART_TXD	MFP_CFG_OUT(GPIO43, AF3, DRIVE_HIGH)
>  #define GPIO44_HWUART_CTS	MFP_CFG_IN(GPIO44, AF3)
>  #define GPIO45_HWUART_RTS	MFP_CFG_OUT(GPIO45, AF3, DRIVE_HIGH)
> +#define GPIO45_HWUART_RTSL	MFP_CFG_OUT(GPIO45, AF3, DRIVE_LOW)

Is this board specific or globally applicable? ISTR, RTS is low active,
so a HIGH in low power mode would be more appropriate that the remote
DCE will hold its data to send.

>  #define GPIO48_HWUART_TXD	MFP_CFG_OUT(GPIO48, AF1, DRIVE_HIGH)
>  #define GPIO49_HWUART_RXD	MFP_CFG_IN(GPIO49, AF1)
>  #define GPIO50_HWUART_CTS	MFP_CFG_IN(GPIO50, AF1)


-------------------------------------------------------------------------
Check out the new SourceForge.net Marketplace.
It's the best place to buy or sell services for
just about anything Open Source.
http://sourceforge.net/services/buy/index.php