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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20121105103130.GB1385@opensource.wolfsonmicro.com>
Date:	Mon, 5 Nov 2012 11:31:30 +0100
From:	Mark Brown <broonie@...nsource.wolfsonmicro.com>
To:	Laxman Dewangan <ldewangan@...dia.com>
Cc:	sameo@...ux.intel.com, lrg@...com, linux-kernel@...r.kernel.org
Subject: Re: [PATCH 1/2] mfd: add TI TPS80031 mfd core driver

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.

Download attachment "signature.asc" of type "application/pgp-signature" (837 bytes)