linux-kernel - Re: [PATCH] MFD : add MAX77686 mfd 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 for Android: free password hash cracker in your pocket

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

Message-ID: <20120501165825.GA3264@sirena.org.uk>
Date:	Tue, 1 May 2012 17:58:25 +0100
From:	Mark Brown <broonie@...nsource.wolfsonmicro.com>
To:	Jonghwa Lee <jonghwa3.lee@...sung.com>
Cc:	sameo@...ux.intel.com, linux-kernel@...r.kernel.org,
	cw00.choi@...sung.com, Chiwoong byun <woong.byun@...sung.com>,
	Kyungmin Park <kyungmin.park@...sung.com>,
	MyungJoo Ham <myungjoo.ham@...sung.com>
Subject: Re: [PATCH] MFD : add MAX77686 mfd driver

On Mon, Apr 30, 2012 at 05:53:42PM +0900, Jonghwa Lee wrote:

> +	if (irq_src & MAX77686_IRQSRC_RTC) {
> +#ifdef CONFIG_RTC_DRV_MAX77686
> +		ret = max77686_read_reg(max77686->rtc, MAX77686_RTC_INT,
> +					 &irq_reg[RTC_INT]);
> +#else
> +		ret = -ENODEV;
> +#endif

Why is this in an ifdef?  It's not really idiomatic to do this and if
the interrupt is never requested presumably everything should be fine.

> +int max77686_irq_resume(struct max77686_dev *max77686)
> +{
> +	if (max77686->irq && max77686->irq_base)
> +		max77686_irq_thread(max77686->irq_base, max77686);
> +	return 0;
> +}

Why is this needed?  I'd expect the parent IRQ controller to notice if
the device is asserting an interrupt when it resumes.

> +int max77686_read_reg(struct i2c_client *i2c, u8 reg, u8 *dest)
> +{
> +	struct max77686_dev *max77686 = i2c_get_clientdata(i2c);
> +	int ret;
> +
> +	mutex_lock(&max77686->iolock);
> +	ret = i2c_smbus_read_byte_data(i2c, reg);
> +	mutex_unlock(&max77686->iolock);
> +	if (ret < 0)
> +		return ret;

It would be much better to use regmap for the register I/O since this is
a PMIC and at least the regulator framework (possibly others soon) are
starting to abstract things out using it.  It also gets you things like
the debugfs dumps of the register map and so on easily.

> +	} else
> +		dev_info(max77686->dev, "device found\n");

This isn't relly adding much - can you log a chip revision or anything?

> +#ifdef CONFIG_RTC_DRV_MAX77686
> +	max77686->rtc = i2c_new_dummy(i2c->adapter, I2C_ADDR_RTC);
> +	i2c_set_clientdata(max77686->rtc, max77686);
> +#endif

Again, it's very odd that this is conditional.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/