Re: [PATCH 2/3] mfd: Add initial S5M8751 support

[Mark Brown <broonie@opensource.wolfsonmicro.com>]

On Wed, Jun 22, 2011 at 02:53:56PM +0900, Sangbeom Kim wrote:
> The WM8994 is a advanced PMIC with AUdio DAC
> Since it includes regulators, Battery charger, audio dac,
> it is represented as a multi-function device,
> though the functionality will be provided by the each driver.

I think you may have cut'n'pasted bits of this changelog from somewhere
else without doing all the updates you meant to :)

> +	if (event1 & S5M8751_MASK_PWRKEY1B)
> +		s5m8751_irq_call_handler(s5m8751, S5M8751_IRQ_PWRKEY1B);
> +
> +	if (event1 & S5M8751_MASK_PWRKEY2B)
> +		s5m8751_irq_call_handler(s5m8751, S5M8751_IRQ_PWRKEY2B);
> +
> +	if (event1 & S5M8751_MASK_PWRKEY3)
> +		s5m8751_irq_call_handler(s5m8751, S5M8751_IRQ_PWRKEY3);

This looks like the IRQ handler code doesn't need to understand the
specific events, it can just loop over the registers and fire off
interrupts based on the bit numbers.

As I said for the first patch this should all be using genirq.

> +static irqreturn_t s5m8751_irq(int irq, void *data)
> +{
> +	struct s5m8751 *s5m8751 = data;
> +
> +	disable_irq_nosync(irq);
> +	schedule_work(&s5m8751->irq_work);
> +
> +	return IRQ_HANDLED;
> +}

Use a threaded IRQ handler.

> +int s5m8751_device_init(struct s5m8751 *s5m8751, int irq,
> +			struct s5m8751_platform_data *pdata)
> +{
> +	int ret = -EINVAL;
> +	u8 chip_id;
> +
> +	if (pdata->init) {
> +		ret = pdata->init(s5m8751);
> +		if (ret != 0) {
> +			dev_err(s5m8751->dev,
> +			 "Platform init() failed: %d\n", ret);
> +			goto err;
> +		}
> +	}
> +
> +	s5m8751_reg_read(s5m8751, S5M8751_CHIP_ID, &chip_id);
> +	if (!chip_id)
> +		dev_info(s5m8751->dev, "Found S5M8751 device\n");
> +	else {
> +		dev_info(s5m8751->dev, "Didn't Find S5M8751 device\n");
> +		ret = -EINVAL;
> +		goto err;
> +	}

I'd really expect the init() callback to run after we've checked that
we can talk to the device.