Re: [PATCH 1/3] mfd: twl6040: Add initial support

[Felipe Balbi <balbi@ti.com>]

[-- Attachment #1: Type: text/plain, Size: 27406 bytes --]

Hi,

On Tue, Aug 02, 2011 at 02:28:41PM +0300, Peter Ujfalusi wrote:
> From: Misael Lopez Cruz <misael.lopez@ti.com>
> 
> TWL6040 IC provides analog high-end audio codec functions for
> handset applications. It contains several audio analog inputs
> and outputs as well as vibrator support. It's connected to the
> host processor via PDM interface for audio data communication.
> The audio modules are controlled by internal registers that
> can be accessed by I2C and PDM interface.
> 
> TWL6040 MFD will be registered as a child of TWL-CORE, and will
> have two children of its own: twl6040-codec and twl6040-vibra.
> 
> This driver is based on TWL4030 and WM8350 MFD drivers.
> 
> Signed-off-by: Misael Lopez Cruz <misael.lopez@ti.com>
> Signed-off-by: Jorge Eduardo Candelaria <jorge.candelaria@ti.com>
> Signed-off-by: Margarita Olaya Cabrera <magi.olaya@ti.com>
> ---
>  arch/arm/plat-omap/include/plat/irqs.h |   12 +-
>  drivers/mfd/Kconfig                    |    6 +
>  drivers/mfd/Makefile                   |    1 +
>  drivers/mfd/twl-core.c                 |    4 +-
>  drivers/mfd/twl6040-codec.c            |  587 ++++++++++++++++++++++++++++++++
>  drivers/mfd/twl6040-irq.c              |  205 +++++++++++
>  include/linux/i2c/twl.h                |    1 +
>  include/linux/mfd/twl6040-codec.h      |  260 ++++++++++++++
>  8 files changed, 1072 insertions(+), 4 deletions(-)
>  create mode 100644 drivers/mfd/twl6040-codec.c
>  create mode 100644 drivers/mfd/twl6040-irq.c
>  create mode 100644 include/linux/mfd/twl6040-codec.h
> 
> diff --git a/arch/arm/plat-omap/include/plat/irqs.h b/arch/arm/plat-omap/include/plat/irqs.h
> index 5a25098..2cfba51 100644
> --- a/arch/arm/plat-omap/include/plat/irqs.h
> +++ b/arch/arm/plat-omap/include/plat/irqs.h
> @@ -407,11 +407,19 @@
>  #endif
>  #define TWL6030_IRQ_END		(TWL6030_IRQ_BASE + TWL6030_BASE_NR_IRQS)
>  
> +#define TWL6040_CODEC_IRQ_BASE	TWL6030_IRQ_END
> +#ifdef CONFIG_TWL6040_CODEC
> +#define TWL6040_CODEC_NR_IRQS	6
> +#else
> +#define TWL6040_CODEC_NR_IRQS	0
> +#endif
> +#define TWL6040_CODEC_IRQ_END	(TWL6040_CODEC_IRQ_BASE + TWL6040_CODEC_NR_IRQS)

since this is a new driver, please don't pullute this header and use
irq_alloc_descs() instead ?!?

> diff --git a/drivers/mfd/twl6040-codec.c b/drivers/mfd/twl6040-codec.c
> new file mode 100644
> index 0000000..a40cd07
> --- /dev/null
> +++ b/drivers/mfd/twl6040-codec.c
> @@ -0,0 +1,587 @@
> +/*
> + * MFD driver for TWL6040 codec submodule
> + *
> + * Authors:	Misael Lopez Cruz <misael.lopez@ti.com>
> + *		Jorge Eduardo Candelaria <jorge.candelaria@ti.com>
> + *
> + * Copyright:	(C) 2011 Texas Instruments, Inc.
> + *
> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License version 2 as
> + * published by the Free Software Foundation.
> + *
> + * This program is distributed in the hope that it will be useful, but
> + * WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
> + * General Public License for more details.
> + *
> + * You should have received a copy of the GNU General Public License
> + * along with this program; if not, write to the Free Software
> + * Foundation, Inc., 51 Franklin St, Fifth Floor, Boston, MA
> + * 02110-1301 USA
> + *
> + */
> +
> +#include <linux/module.h>
> +#include <linux/types.h>
> +#include <linux/slab.h>
> +#include <linux/kernel.h>
> +#include <linux/platform_device.h>
> +#include <linux/gpio.h>
> +#include <linux/delay.h>
> +#include <linux/i2c/twl.h>
> +#include <linux/mfd/core.h>
> +#include <linux/mfd/twl6040-codec.h>
> +
> +static struct platform_device *twl6040_dev;

this is useless, see below.

> +int twl6040_reg_read(struct twl6040 *twl6040, unsigned int reg)
> +{
> +	int ret;
> +	u8 val = 0;
> +
> +	mutex_lock(&twl6040->io_mutex);
> +	ret = twl_i2c_read_u8(TWL_MODULE_AUDIO_VOICE, &val, reg);
> +	if (ret < 0) {
> +		mutex_unlock(&twl6040->io_mutex);
> +		return ret;
> +	}
> +	mutex_unlock(&twl6040->io_mutex);
> +
> +	return val;
> +}
> +EXPORT_SYMBOL(twl6040_reg_read);

EXPORT_SYMBOL_GPL(), all users of this should be GPL so we have access
to the code. Ditto to all below.

> +int twl6040_reg_write(struct twl6040 *twl6040, unsigned int reg, u8 val)
> +{
> +	int ret;
> +
> +	mutex_lock(&twl6040->io_mutex);
> +	ret = twl_i2c_write_u8(TWL_MODULE_AUDIO_VOICE, val, reg);
> +	mutex_unlock(&twl6040->io_mutex);
> +
> +	return ret;
> +}
> +EXPORT_SYMBOL(twl6040_reg_write);
> +
> +int twl6040_set_bits(struct twl6040 *twl6040, unsigned int reg, u8 mask)
> +{
> +	int ret;
> +	u8 val;
> +
> +	mutex_lock(&twl6040->io_mutex);
> +	ret = twl_i2c_read_u8(TWL_MODULE_AUDIO_VOICE, &val, reg);
> +	if (ret)
> +		goto out;
> +
> +	val |= mask;
> +	ret = twl_i2c_write_u8(TWL_MODULE_AUDIO_VOICE, val, reg);
> +out:
> +	mutex_unlock(&twl6040->io_mutex);
> +	return ret;
> +}
> +EXPORT_SYMBOL(twl6040_set_bits);
> +
> +int twl6040_clear_bits(struct twl6040 *twl6040, unsigned int reg, u8 mask)
> +{
> +	int ret;
> +	u8 val;
> +
> +	mutex_lock(&twl6040->io_mutex);
> +	ret = twl_i2c_read_u8(TWL_MODULE_AUDIO_VOICE, &val, reg);
> +	if (ret)
> +		goto out;
> +
> +	val &= ~mask;
> +	ret = twl_i2c_write_u8(TWL_MODULE_AUDIO_VOICE, val, reg);
> +out:
> +	mutex_unlock(&twl6040->io_mutex);
> +	return ret;
> +}
> +EXPORT_SYMBOL(twl6040_clear_bits);
> +
> +/* twl6040 codec manual power-up sequence */
> +static int twl6040_power_up(struct twl6040 *twl6040)
> +{
> +	u8 ldoctl, ncpctl, lppllctl;
> +	int ret;
> +
> +	/* enable high-side LDO, reference system and internal oscillator */
> +	ldoctl = TWL6040_HSLDOENA | TWL6040_REFENA | TWL6040_OSCENA;
> +	ret = twl6040_reg_write(twl6040, TWL6040_REG_LDOCTL, ldoctl);
> +	if (ret)
> +		return ret;
> +	usleep_range(10000, 10500);
> +
> +	/* enable negative charge pump */
> +	ncpctl = TWL6040_NCPENA;
> +	ret = twl6040_reg_write(twl6040, TWL6040_REG_NCPCTL, ncpctl);
> +	if (ret)
> +		goto ncp_err;
> +	usleep_range(1000, 1500);
> +
> +	/* enable low-side LDO */
> +	ldoctl |= TWL6040_LSLDOENA;
> +	ret = twl6040_reg_write(twl6040, TWL6040_REG_LDOCTL, ldoctl);
> +	if (ret)
> +		goto lsldo_err;
> +	usleep_range(1000, 1500);
> +
> +	/* enable low-power PLL */
> +	lppllctl = TWL6040_LPLLENA;
> +	ret = twl6040_reg_write(twl6040, TWL6040_REG_LPPLLCTL, lppllctl);
> +	if (ret)
> +		goto lppll_err;
> +	usleep_range(5000, 5500);
> +
> +	/* disable internal oscillator */
> +	ldoctl &= ~TWL6040_OSCENA;
> +	ret = twl6040_reg_write(twl6040, TWL6040_REG_LDOCTL, ldoctl);
> +	if (ret)
> +		goto osc_err;
> +
> +	return 0;
> +
> +osc_err:
> +	lppllctl &= ~TWL6040_LPLLENA;
> +	twl6040_reg_write(twl6040, TWL6040_REG_LPPLLCTL, lppllctl);
> +lppll_err:
> +	ldoctl &= ~TWL6040_LSLDOENA;
> +	twl6040_reg_write(twl6040, TWL6040_REG_LDOCTL, ldoctl);
> +lsldo_err:
> +	ncpctl &= ~TWL6040_NCPENA;
> +	twl6040_reg_write(twl6040, TWL6040_REG_NCPCTL, ncpctl);
> +ncp_err:
> +	ldoctl &= ~(TWL6040_HSLDOENA | TWL6040_REFENA | TWL6040_OSCENA);
> +	twl6040_reg_write(twl6040, TWL6040_REG_LDOCTL, ldoctl);
> +
> +	return ret;
> +}
> +
> +/* twl6040 codec manual power-down sequence */
> +static void twl6040_power_down(struct twl6040 *twl6040)
> +{
> +	u8 ncpctl, ldoctl, lppllctl;
> +
> +	ncpctl = twl6040_reg_read(twl6040, TWL6040_REG_NCPCTL);
> +	ldoctl = twl6040_reg_read(twl6040, TWL6040_REG_LDOCTL);
> +	lppllctl = twl6040_reg_read(twl6040, TWL6040_REG_LPPLLCTL);
> +
> +	/* enable internal oscillator */
> +	ldoctl |= TWL6040_OSCENA;
> +	twl6040_reg_write(twl6040, TWL6040_REG_LDOCTL, ldoctl);
> +	usleep_range(1000, 1500);
> +
> +	/* disable low-power PLL */
> +	lppllctl &= ~TWL6040_LPLLENA;
> +	twl6040_reg_write(twl6040, TWL6040_REG_LPPLLCTL, lppllctl);
> +
> +	/* disable low-side LDO */
> +	ldoctl &= ~TWL6040_LSLDOENA;
> +	twl6040_reg_write(twl6040, TWL6040_REG_LDOCTL, ldoctl);
> +
> +	/* disable negative charge pump */
> +	ncpctl &= ~TWL6040_NCPENA;
> +	twl6040_reg_write(twl6040, TWL6040_REG_NCPCTL, ncpctl);
> +
> +	/* disable high-side LDO, reference system and internal oscillator */
> +	ldoctl &= ~(TWL6040_HSLDOENA | TWL6040_REFENA | TWL6040_OSCENA);
> +	twl6040_reg_write(twl6040, TWL6040_REG_LDOCTL, ldoctl);
> +}
> +
> +static irqreturn_t twl6040_naudint_handler(int irq, void *data)
> +{
> +	struct twl6040 *twl6040 = data;
> +	u8 intid;
> +
> +	intid = twl6040_reg_read(twl6040, TWL6040_REG_INTID);
> +
> +	if (intid & TWL6040_READYINT)
> +		complete(&twl6040->ready);
> +
> +	return IRQ_HANDLED;
> +}
> +
> +static int twl6040_power_up_completion(struct twl6040 *twl6040,
> +				       int naudint)
> +{
> +	int time_left;
> +	u8 intid;
> +
> +	time_left = wait_for_completion_timeout(&twl6040->ready,
> +						msecs_to_jiffies(144));

should this be interruptible ?

> +	if (!time_left) {
> +		intid = twl6040_reg_read(twl6040, TWL6040_REG_INTID);
> +		if (!(intid & TWL6040_READYINT)) {
> +			dev_err(&twl6040_dev->dev,

you have twl6040 as argument to this function, meaning:

dev_err(twl6040->dev,

should work just fine.

> +int twl6040_power(struct twl6040 *twl6040, int on)
> +{
> +	int audpwron = twl6040->audpwron;
> +	int naudint = twl6040->irq;
> +	int ret = 0;
> +
> +	mutex_lock(&twl6040->mutex);
> +
> +	if (on) {
> +		/* already powered-up */
> +		if (twl6040->power_count++)
> +			goto out;
> +
> +		if (gpio_is_valid(audpwron)) {
> +			/* use AUDPWRON line */
> +			gpio_set_value(audpwron, 1);
> +			/* wait for power-up completion */
> +			ret = twl6040_power_up_completion(twl6040, naudint);
> +			if (ret) {
> +				dev_err(&twl6040_dev->dev,

ditto here and all below.

> +					"automatic power-down failed\n");
> +				twl6040->power_count = 0;
> +				goto out;
> +			}
> +		} else {
> +			/* use manual power-up sequence */
> +			ret = twl6040_power_up(twl6040);
> +			if (ret) {
> +				dev_err(&twl6040_dev->dev,
> +					"manual power-up failed\n");
> +				twl6040->power_count = 0;
> +				goto out;
> +			}
> +		}
> +		twl6040->pll = TWL6040_LPPLL_ID;
> +		twl6040->sysclk = 19200000;
> +	} else {
> +		/* already powered-down */
> +		if (!twl6040->power_count) {
> +			dev_err(&twl6040_dev->dev,
> +				"device is already powered-off\n");
> +			ret = -EPERM;
> +			goto out;
> +		}
> +
> +		if (--twl6040->power_count)
> +			goto out;
> +
> +		if (gpio_is_valid(audpwron)) {
> +			/* use AUDPWRON line */
> +			gpio_set_value(audpwron, 0);
> +
> +			/* power-down sequence latency */
> +			udelay(500);
> +		} else {
> +			/* use manual power-down sequence */
> +			twl6040_power_down(twl6040);
> +		}
> +		twl6040->pll = TWL6040_NOPLL_ID;
> +		twl6040->sysclk = 0;
> +	}
> +
> +out:
> +	mutex_unlock(&twl6040->mutex);
> +	return ret;
> +}
> +EXPORT_SYMBOL(twl6040_power);

should this function be a ->runtime_resume/suspend method ? Then
children would simply pm_runtime_get_sync() and this would be called by
the pm runtime framework. Does it make sense ?

> +int twl6040_is_powered(struct twl6040 *twl6040)
> +{
> +	return twl6040->power_count;
> +}
> +EXPORT_SYMBOL(twl6040_is_powered);

I'm not sure this should be needed. You allocate your children yourself,
so they will only probe after this has succesfully probed, rendering
this unneeded, right ?

> +int twl6040_set_pll(struct twl6040 *twl6040, enum twl6040_pll_id id,
> +		    unsigned int freq_in, unsigned int freq_out)
> +{
> +	u8 hppllctl, lppllctl;
> +	int ret = 0;
> +
> +	mutex_lock(&twl6040->mutex);
> +
> +	hppllctl = twl6040_reg_read(twl6040, TWL6040_REG_HPPLLCTL);
> +	lppllctl = twl6040_reg_read(twl6040, TWL6040_REG_LPPLLCTL);
> +
> +	switch (id) {
> +	case TWL6040_LPPLL_ID:
> +		/* low-power PLL divider */
> +		switch (freq_out) {
> +		case 17640000:
> +			lppllctl |= TWL6040_LPLLFIN;
> +			break;
> +		case 19200000:
> +			lppllctl &= ~TWL6040_LPLLFIN;
> +			break;
> +		default:
> +			dev_err(&twl6040_dev->dev,

also don't need that global static pointer. twl6040->dev. Ditto all
below

> +				"freq_out %d not supported\n", freq_out);
> +			ret = -EINVAL;
> +			goto pll_out;
> +		}
> +		twl6040_reg_write(twl6040, TWL6040_REG_LPPLLCTL, lppllctl);
> +
> +		switch (freq_in) {
> +		case 32768:
> +			lppllctl |= TWL6040_LPLLENA;
> +			twl6040_reg_write(twl6040, TWL6040_REG_LPPLLCTL,
> +					  lppllctl);
> +			mdelay(5);
> +			lppllctl &= ~TWL6040_HPLLSEL;
> +			twl6040_reg_write(twl6040, TWL6040_REG_LPPLLCTL,
> +					  lppllctl);
> +			hppllctl &= ~TWL6040_HPLLENA;
> +			twl6040_reg_write(twl6040, TWL6040_REG_HPPLLCTL,
> +					  hppllctl);
> +			break;
> +		default:
> +			dev_err(&twl6040_dev->dev,
> +				"freq_in %d not supported\n", freq_in);
> +			ret = -EINVAL;
> +			goto pll_out;
> +		}
> +
> +		twl6040->pll = TWL6040_LPPLL_ID;
> +		break;
> +	case TWL6040_HPPLL_ID:
> +		/* high-performance PLL can provide only 19.2 MHz */
> +		if (freq_out != 19200000) {
> +			dev_err(&twl6040_dev->dev,
> +				"freq_out %d not supported\n", freq_out);
> +			ret = -EINVAL;
> +			goto pll_out;
> +		}
> +
> +		hppllctl &= ~TWL6040_MCLK_MSK;
> +
> +		switch (freq_in) {
> +		case 12000000:
> +			/* PLL enabled, active mode */
> +			hppllctl |= TWL6040_MCLK_12000KHZ |
> +				    TWL6040_HPLLENA;
> +			break;
> +		case 19200000:
> +			/*
> +			 * PLL disabled
> +			 * (enable PLL if MCLK jitter quality
> +			 *  doesn't meet specification)
> +			 */
> +			hppllctl |= TWL6040_MCLK_19200KHZ;
> +			break;
> +		case 26000000:
> +			/* PLL enabled, active mode */
> +			hppllctl |= TWL6040_MCLK_26000KHZ |
> +				    TWL6040_HPLLENA;
> +			break;
> +		case 38400000:
> +			/* PLL enabled, active mode */
> +			hppllctl |= TWL6040_MCLK_38400KHZ |
> +				    TWL6040_HPLLENA;
> +			break;
> +		default:
> +			dev_err(&twl6040_dev->dev,
> +				"freq_in %d not supported\n", freq_in);
> +			ret = -EINVAL;
> +			goto pll_out;
> +		}
> +
> +		/* enable clock slicer to ensure input waveform is square */
> +		hppllctl |= TWL6040_HPLLSQRENA;
> +
> +		twl6040_reg_write(twl6040, TWL6040_REG_HPPLLCTL, hppllctl);
> +		udelay(500);
> +		lppllctl |= TWL6040_HPLLSEL;
> +		twl6040_reg_write(twl6040, TWL6040_REG_LPPLLCTL, lppllctl);
> +		lppllctl &= ~TWL6040_LPLLENA;
> +		twl6040_reg_write(twl6040, TWL6040_REG_LPPLLCTL, lppllctl);
> +
> +		twl6040->pll = TWL6040_HPPLL_ID;
> +		break;
> +	default:
> +		dev_err(&twl6040_dev->dev, "unknown pll id %d\n", id);
> +		ret = -EINVAL;
> +		goto pll_out;
> +	}
> +
> +	twl6040->sysclk = freq_out;
> +
> +pll_out:
> +	mutex_unlock(&twl6040->mutex);
> +	return ret;
> +}
> +EXPORT_SYMBOL(twl6040_set_pll);
> +
> +enum twl6040_pll_id twl6040_get_pll(struct twl6040 *twl6040)
> +{
> +	return twl6040->pll;
> +}
> +EXPORT_SYMBOL(twl6040_get_pll);
> +
> +unsigned int twl6040_get_sysclk(struct twl6040 *twl6040)
> +{
> +	return twl6040->sysclk;
> +}
> +EXPORT_SYMBOL(twl6040_get_sysclk);

these three above look like they should be done via clk framework ??

> +static int __devinit twl6040_probe(struct platform_device *pdev)
> +{
> +	struct twl4030_codec_data *pdata = pdev->dev.platform_data;
> +	struct twl6040 *twl6040;
> +	struct mfd_cell *cell = NULL;
> +	int ret, children = 0;
> +
> +	if (!pdata) {
> +		dev_err(&pdev->dev, "Platform data is missing\n");
> +		return -EINVAL;
> +	}
> +
> +	twl6040 = kzalloc(sizeof(struct twl6040), GFP_KERNEL);

sizeof(*twl6040) would allow to change the type without need to patch
this line. It's what's generally done on most drivers.

> +	if (!twl6040)
> +		return -ENOMEM;
> +
> +	platform_set_drvdata(pdev, twl6040);
> +
> +	twl6040_dev = pdev;

this is useless.

> +	twl6040->dev = &pdev->dev;
> +	twl6040->audpwron = pdata->audpwron_gpio;
> +	twl6040->irq = pdata->naudint_irq;
> +	twl6040->irq_base = pdata->irq_base;
> +
> +	mutex_init(&twl6040->mutex);
> +	mutex_init(&twl6040->io_mutex);
> +	init_completion(&twl6040->ready);
> +
> +	twl6040->rev = twl6040_reg_read(twl6040, TWL6040_REG_ASICREV);
> +
> +	if (gpio_is_valid(twl6040->audpwron)) {
> +		ret = gpio_request(twl6040->audpwron, "audpwron");
> +		if (ret)
> +			goto gpio1_err;
> +
> +		ret = gpio_direction_output(twl6040->audpwron, 0);
> +		if (ret)
> +			goto gpio2_err;
> +	}

what if gpio isn't valid ? Does it make sense to continue ? Is the GPIO
poweron pin a "must-have" or can you tie that pin to Vcc (or ground,
depending if it's active high or low) and have it working ?

> +	/* ERRATA: Automatic power-up is not possible in ES1.0 */
> +	if (twl6040_get_rev(twl6040) == TWL6040_REV_ES1_0)
> +		twl6040->audpwron = -EINVAL;
> +
> +	if (twl6040->irq) {
> +		/* codec interrupt */
> +		ret = twl6040_irq_init(twl6040);
> +		if (ret)
> +			goto gpio2_err;
> +
> +		ret = twl6040_request_irq(twl6040, TWL6040_IRQ_READY,

this is really wrong. Didn't we agree on using tradicional
request_threaded_irq() here ? Also, the IRQ number should be passed via
struct resource.

> +					  twl6040_naudint_handler, 0,
> +					  "twl6040_irq_ready", twl6040);
> +		if (ret) {
> +			dev_err(twl6040->dev, "READY IRQ request failed: %d\n",
> +				ret);
> +			goto irq_err;
> +		}
> +	}
> +
> +	/* dual-access registers controlled by I2C only */
> +	twl6040_set_bits(twl6040, TWL6040_REG_ACCCTL, TWL6040_I2CSEL);
> +
> +	if (pdata->audio) {
> +		cell = &twl6040->cells[children];
> +		cell->name = "twl6040-codec";
> +		cell->platform_data = pdata->audio;
> +		cell->pdata_size = sizeof(*pdata->audio);
> +		children++;
> +	}
> +
> +	if (pdata->vibra) {
> +		cell = &twl6040->cells[children];
> +		cell->name = "twl6040-vibra";
> +		cell->platform_data = pdata->vibra;
> +		cell->pdata_size = sizeof(*pdata->vibra);
> +		children++;
> +	}
> +
> +	if (children) {
> +		ret = mfd_add_devices(&pdev->dev, pdev->id, twl6040->cells,
> +				      children, NULL, 0);
> +		if (ret)
> +			goto mfd_err;
> +	} else {
> +		dev_err(&pdev->dev, "No platform data found for children\n");
> +		ret = -ENODEV;
> +		goto mfd_err;
> +	}
> +
> +	return 0;
> +
> +mfd_err:
> +	if (twl6040->irq)
> +		twl6040_free_irq(twl6040, TWL6040_IRQ_READY, twl6040);
> +irq_err:
> +	if (twl6040->irq)
> +		twl6040_irq_exit(twl6040);
> +gpio2_err:
> +	if (gpio_is_valid(twl6040->audpwron))
> +		gpio_free(twl6040->audpwron);
> +gpio1_err:
> +	platform_set_drvdata(pdev, NULL);
> +	kfree(twl6040);
> +	twl6040_dev = NULL;
> +	return ret;
> +}
> +
> +static int __devexit twl6040_remove(struct platform_device *pdev)
> +{
> +	struct twl6040 *twl6040 = platform_get_drvdata(pdev);
> +
> +	if (twl6040_is_powered(twl6040))
> +		twl6040_power(twl6040, 0);

disabling it twice, shouldn't be a problem, so this is also useless
check.

> +	if (gpio_is_valid(twl6040->audpwron))
> +		gpio_free(twl6040->audpwron);
> +
> +	twl6040_free_irq(twl6040, TWL6040_IRQ_READY, twl6040);

this has to be passed via struct resource.

> +	if (twl6040->irq)
> +		twl6040_irq_exit(twl6040);
> +
> +	mfd_remove_devices(&pdev->dev);
> +	platform_set_drvdata(pdev, NULL);
> +	kfree(twl6040);
> +	twl6040_dev = NULL;
> +
> +	return 0;
> +}
> +
> +static struct platform_driver twl6040_driver = {
> +	.probe		= twl6040_probe,
> +	.remove		= __devexit_p(twl6040_remove),
> +	.driver		= {
> +		.owner	= THIS_MODULE,
> +		.name	= "twl6040-audio",
> +	},
> +};
> +
> +static int __devinit twl6040_init(void)
> +{
> +	return platform_driver_register(&twl6040_driver);
> +}
> +module_init(twl6040_init);
> +
> +static void __devexit twl6040_exit(void)
> +{
> +	platform_driver_unregister(&twl6040_driver);
> +}
> +
> +module_exit(twl6040_exit);
> +
> +MODULE_DESCRIPTION("TWL6040 MFD");
> +MODULE_AUTHOR("Misael Lopez Cruz <misael.lopez@ti.com>");
> +MODULE_AUTHOR("Jorge Eduardo Candelaria <jorge.candelaria@ti.com>");
> +MODULE_LICENSE("GPL");
> +MODULE_ALIAS("platform:twl6040-audio");
> diff --git a/drivers/mfd/twl6040-irq.c b/drivers/mfd/twl6040-irq.c
> new file mode 100644
> index 0000000..ac776be
> --- /dev/null
> +++ b/drivers/mfd/twl6040-irq.c
> @@ -0,0 +1,205 @@
> +/*
> + * Interrupt controller support for TWL6040
> + *
> + * Author:     Misael Lopez Cruz <misael.lopez@ti.com>
> + *
> + * Copyright:   (C) 2011 Texas Instruments, Inc.
> + *
> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License version 2 as
> + * published by the Free Software Foundation.
> + *
> + * This program is distributed in the hope that it will be useful, but
> + * WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
> + * General Public License for more details.
> + *
> + * You should have received a copy of the GNU General Public License
> + * along with this program; if not, write to the Free Software
> + * Foundation, Inc., 51 Franklin St, Fifth Floor, Boston, MA
> + * 02110-1301 USA
> + *
> + */
> +
> +#include <linux/kernel.h>
> +#include <linux/module.h>
> +#include <linux/irq.h>
> +#include <linux/interrupt.h>
> +#include <linux/mfd/core.h>
> +#include <linux/mfd/twl6040-codec.h>
> +
> +struct twl6040_irq_data {
> +	int mask;
> +	int status;
> +};
> +
> +static struct twl6040_irq_data twl6040_irqs[] = {
> +	{
> +		.mask = TWL6040_THMSK,
> +		.status = TWL6040_THINT,
> +	},
> +	{
> +		.mask = TWL6040_PLUGMSK,
> +		.status = TWL6040_PLUGINT | TWL6040_UNPLUGINT,
> +	},
> +	{
> +		.mask = TWL6040_HOOKMSK,
> +		.status = TWL6040_HOOKINT,
> +	},
> +	{
> +		.mask = TWL6040_HFMSK,
> +		.status = TWL6040_HFINT,
> +	},
> +	{
> +		.mask = TWL6040_VIBMSK,
> +		.status = TWL6040_VIBINT,
> +	},
> +	{
> +		.mask = TWL6040_READYMSK,
> +		.status = TWL6040_READYINT,
> +	},
> +};
> +
> +static inline
> +struct twl6040_irq_data *irq_to_twl6040_irq(struct twl6040 *twl6040,
> +					    int irq)
> +{
> +	return &twl6040_irqs[irq - twl6040->irq_base];
> +}
> +
> +static void twl6040_irq_lock(struct irq_data *data)
> +{
> +	struct twl6040 *twl6040 = irq_data_get_irq_chip_data(data);
> +
> +	mutex_lock(&twl6040->irq_mutex);
> +}
> +
> +static void twl6040_irq_sync_unlock(struct irq_data *data)
> +{
> +	struct twl6040 *twl6040 = irq_data_get_irq_chip_data(data);
> +
> +	/* write back to hardware any change in irq mask */
> +	if (twl6040->irq_masks_cur != twl6040->irq_masks_cache) {
> +		twl6040->irq_masks_cache = twl6040->irq_masks_cur;
> +		twl6040_reg_write(twl6040, TWL6040_REG_INTMR,
> +				  twl6040->irq_masks_cur);
> +	}
> +
> +	mutex_unlock(&twl6040->irq_mutex);
> +}
> +
> +static void twl6040_irq_enable(struct irq_data *data)
> +{
> +	struct twl6040 *twl6040 = irq_data_get_irq_chip_data(data);
> +	struct twl6040_irq_data *irq_data = irq_to_twl6040_irq(twl6040,
> +							       data->irq);
> +
> +	twl6040->irq_masks_cur &= ~irq_data->mask;
> +}
> +
> +static void twl6040_irq_disable(struct irq_data *data)
> +{
> +	struct twl6040 *twl6040 = irq_data_get_irq_chip_data(data);
> +	struct twl6040_irq_data *irq_data = irq_to_twl6040_irq(twl6040,
> +							       data->irq);
> +
> +	twl6040->irq_masks_cur |= irq_data->mask;
> +}
> +
> +static struct irq_chip twl6040_irq_chip = {
> +	.name			= "twl6040",
> +	.irq_bus_lock		= twl6040_irq_lock,
> +	.irq_bus_sync_unlock	= twl6040_irq_sync_unlock,
> +	.irq_enable		= twl6040_irq_enable,
> +	.irq_disable		= twl6040_irq_disable,
> +};
> +
> +static irqreturn_t twl6040_irq_thread(int irq, void *data)
> +{
> +	struct twl6040 *twl6040 = data;
> +	u8 intid;
> +	int i;
> +
> +	intid = twl6040_reg_read(twl6040, TWL6040_REG_INTID);

what's available on this register ?? Are the status fields above bit
positions ? If so you can change this:

> +	/* apply masking and report (backwards to handle READYINT first) */
> +	for (i = ARRAY_SIZE(twl6040_irqs) - 1; i >= 0; i--) {
> +		if (twl6040->irq_masks_cur & twl6040_irqs[i].mask)
> +			intid &= ~twl6040_irqs[i].status;
> +		if (intid & twl6040_irqs[i].status)
> +			handle_nested_irq(twl6040->irq_base + i);
> +	}

into something like this:

	while (intid) {
		unsigned long pending = __ffs(intid);
		unsigned long irq;

		intid &= ~BIT(pending);
		irq = pending + twl6040->irq_base;
		handle_nested_irq(irq);
	}

and that twl6040_irq_data structure can go into the bin.

> +	/* ack unmasked irqs */
> +	twl6040_reg_write(twl6040, TWL6040_REG_INTID, intid);

I believe IRQ subsystem will handle this for you on your irq_ack() +
bus_sync_unlock() methods, right ? So you should provide them.

> +	return IRQ_HANDLED;
> +}
> +
> +int twl6040_irq_init(struct twl6040 *twl6040)
> +{
> +	int cur_irq, ret;
> +	u8 val;
> +
> +	mutex_init(&twl6040->irq_mutex);
> +
> +	/* mask the individual interrupt sources */
> +	twl6040->irq_masks_cur = TWL6040_ALLINT_MSK;
> +	twl6040->irq_masks_cache = TWL6040_ALLINT_MSK;
> +	twl6040_reg_write(twl6040, TWL6040_REG_INTMR, TWL6040_ALLINT_MSK);
> +
> +	if (!twl6040->irq) {
> +		dev_warn(twl6040->dev,
> +			 "no interrupt specified, no interrupts\n");
> +		twl6040->irq_base = 0;
> +		return 0;
> +	}
> +
> +	if (!twl6040->irq_base) {

base should be allocated here with irq_alloc_descs()

> +		dev_err(twl6040->dev,
> +			"no interrupt base specified, no interrupts\n");
> +		return 0;
> +	}
> +
> +	/* Register them with genirq */
> +	for (cur_irq = twl6040->irq_base;
> +	     cur_irq < twl6040->irq_base + ARRAY_SIZE(twl6040_irqs);
> +	     cur_irq++) {
> +		irq_set_chip_data(cur_irq, twl6040);
> +		irq_set_chip_and_handler(cur_irq, &twl6040_irq_chip,
> +					 handle_level_irq);
> +		irq_set_nested_thread(cur_irq, 1);
> +
> +		/* ARM needs us to explicitly flag the IRQ as valid
> +		 * and will set them noprobe when we do so. */

multiline comment style is wrong here.

> +#ifdef CONFIG_ARM
> +		set_irq_flags(cur_irq, IRQF_VALID);
> +#else
> +		irq_set_noprobe(cur_irq);
> +#endif
> +	}
> +
> +	ret = request_threaded_irq(twl6040->irq, NULL, twl6040_irq_thread,
> +				   IRQF_ONESHOT, "twl6040", twl6040);

I'm not sure you need IRQF_ONESHOT here, but that can be changed later.

> +	if (ret) {
> +		dev_err(twl6040->dev, "failed to request IRQ %d: %d\n",
> +			twl6040->irq, ret);
> +		return ret;
> +	}
> +
> +	/* reset interrupts */
> +	val = twl6040_reg_read(twl6040, TWL6040_REG_INTID);
> +
> +	/* interrupts cleared on write */
> +	twl6040_clear_bits(twl6040, TWL6040_REG_ACCCTL, TWL6040_INTCLRMODE);

you should do these two before request_threaded_irq(), you might have a
real IRQ which will be missed.

> +	return 0;
> +}
> +EXPORT_SYMBOL(twl6040_irq_init);

EXPORT_SYMBOL_GPL()

> +void twl6040_irq_exit(struct twl6040 *twl6040)
> +{
> +	if (twl6040->irq)

this check should always be true. You only call this on error path of
probe() and exit path.

> +		free_irq(twl6040->irq, twl6040);
> +}
> +EXPORT_SYMBOL(twl6040_irq_exit);

EXPORT_SYMBOL_GPL()

-- 
balbi

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 490 bytes --]