linux-kernel - Re: [PATCHv5 5/11] REGULATOR: Regulator module of DA9052 device 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] [day] [month] [year] [list]

Message-ID: <20101221181949.GA3105@rakim.wolfsonmicro.main>
Date:	Tue, 21 Dec 2010 18:19:49 +0000
From:	Mark Brown <broonie@...nsource.wolfsonmicro.com>
To:	dd diasemi <dd.diasemi@...il.com>
Cc:	lrg@...mlogic.co.uk, linux-kernel@...r.kernel.org
Subject: Re: [PATCHv5 5/11] REGULATOR: Regulator module of DA9052 device
 driver

On Tue, Dec 21, 2010 at 07:10:12PM +0100, dd diasemi wrote:

> Linux Kernel Version: 2.6.34

Always submit drivers against current development kernels, see the git
trees referenced in MAINTAINERS - linux-next is usually a good
approximation.  Drivers generated against older kernels will frequently
not even build against current kernels.  I'm fairly sure this has been
mentioned to you on previous submissions.

I've *briefly* glanced through the code below but not thoroughly
reviewed due to the old kernel version.

> +struct regulator {
> +	struct device *dev;
> +	struct list_head list;
> +	int uA_load;
> +	int min_uV;
> +	int max_uV;
> +	int enabled;
> +	char *supply_name;
> +	struct device_attribute dev_attr;
> +	struct regulator_dev *rdev;
> +};

Why are you replicating this in your driver?  You should never even look
at the interenals of this structure...

> +	da9052_lock(priv->da9052);
> +	ret = priv->da9052->read(priv->da9052, &ssc_msg);
> +	if (ret) {
> +		da9052_unlock(priv->da9052);
> +		return -EIO;
> +	}
> +
> +	ssc_msg.data = (ssc_msg.data | da9052_regulators[id].en_bit_mask);
> +
> +	ret = priv->da9052->write(priv->da9052, &ssc_msg);
> +	if (ret) {
> +		da9052_unlock(priv->da9052);
> +		return -EIO;
> +	}
> +	da9052_unlock(priv->da9052);

This register manpulation looks like it really should be factored out
into the MFD driver - there's quite a few copies of essentially the same
code in the regulator driver alone.

> +			(DA9052_BUCK_PERI_STEP_ABOVE_3000);
> +	} else{

checkpatch.pl will spot style issues like this for you.

> +	priv->da9052 = da9052;
> +	for (i = 0; i < 14; i++) {

14 is a magic number...  should be something more meaningful.

> +/* PM Device name and static Major number macros */
> +#define DRIVER_NAME				"da9052-regulator"

This shouldn't be in a header where other code might see it.

> +/* PM Device Error Codes */
> +

Remove this empty section.
--
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/