Re: [PATCH 1/2] mfd: add TI TPS80031 mfd core driver

[Mark Brown]

On Mon, Nov 05, 2012 at 03:14:17PM +0530, Laxman Dewangan wrote:

Looks pretty good, a few smaller issues though.

> +static bool is_volatile_reg_id0(struct device *dev, unsigned int reg)
> +{
> +	return false;
> +}

This is the default for all registers, you should not need to provide
a function like this.

> +	dev_info(&client->dev, "Jtag version 0x%02x and Eprom version 0x%02x\n",
> +					jtag_ver, ep_ver);

EPROM is an acronym.  For TI devices I think normally the ES revision
is referred to as just that, otherwise JTAG is an acronym too.

> +	if (client->irq) {
> +		ret = tps80031_irq_init(tps80031, client->irq, pdata->irq_base);
> +		if (ret) {
> +			dev_err(&client->dev, "IRQ init failed: %d\n", ret);
> +			goto fail_client_reg;
> +		}
> +	}

Since for current kernel versions we're using irqdomains we can set up
all the interrupts without an irq.  It can make life easier to just
always set up the interrupt controller so drivers don't need to worry
about it.

> +	ret = mfd_add_devices(tps80031->dev, -1,
> +			tps80031_cell, ARRAY_SIZE(tps80031_cell),
> +			NULL, tps80031->irq_base,
> +			regmap_irq_get_domain(tps80031->irq_data));

If you're supplying a domain you shouldn't need to provide an irq_base,
the domain fulfuls the same role better.

> +static int __devexit tps80031_remove(struct i2c_client *client)
> +{
> +	struct tps80031 *tps80031 = i2c_get_clientdata(client);
> +	int i;
> +
> +	mfd_remove_devices(tps80031->dev);
> +
> +	if (client->irq)
> +		free_irq(client->irq, tps80031);

regmap_irq_exit()

> +#ifdef CONFIG_PM_SLEEP
> +static int tps80031_suspend(struct device *dev)
> +{
> +	struct i2c_client *client = to_i2c_client(dev);
> +
> +	if (client->irq)
> +		disable_irq(client->irq);

The interrupt core should take care of this for you if it's important,
and note that people often use things like RTC alarms to wake the
system.

Attachment: signature.asc
Description: Digital signature