linux-kernel - Re: [PATCH V1] regulator: DA9211 : support DA9213

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: <20140807100450.GE17528@sirena.org.uk>
Date:	Thu, 7 Aug 2014 11:04:50 +0100
From:	Mark Brown <broonie@...nel.org>
To:	James Ban <james.ban.opensource@...semi.com>
Cc:	Liam Girdwood <lgirdwood@...il.com>,
	Support Opensource <support.opensource@...semi.com>,
	LKML <linux-kernel@...r.kernel.org>,
	David Dajun Chen <david.chen@...semi.com>
Subject: Re: [PATCH V1] regulator: DA9211 : support DA9213

On Tue, Aug 05, 2014 at 09:56:45AM +0900, James Ban wrote:

This is mostly good, just one fairly small thing:

> +static const int da9211_current_limits[2][16] = {
> +	/* DA9211 current limits */
> +	{2000000, 2200000, 2400000, 2600000, 2800000, 3000000, 3200000, 3400000,
> +	3600000, 3800000, 4000000, 4200000, 4400000, 4600000, 4800000, 5000000},
> +	/* DA9213 current limits */
> +	{3000000, 3200000, 3400000, 3600000, 3800000, 4000000, 4200000, 4400000,
> +	4600000, 4800000, 5000000, 5200000, 5400000, 5600000, 5800000, 6000000},
>  };

It's going to be more robust to have two arrays here, C doesn't deal
with multiply nested arrays well and indexing by arbatrary number is
generally fragile.

> +	if (chip->chip_id == DA9213)
> +		row = 1;

switch statement, what happens when there's another derivative?

> +	if (!(chip->chip_id == DA9211 && data == DA9211_DEVICE_ID)
> +		&& !(chip->chip_id == DA9213 && data == DA9213_DEVICE_ID)) {
> +		dev_err(chip->dev, "Chip ID and configuration is mismatched\n");
> +		ret = -ENODEV;
> +		return ret;
> +	}

You could convert this to a switch statement and then set driver data
pointing to the right tables and so on so you don't have conditional
code elsewhere in the driver.

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