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  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]
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)

Powered by blists - more mailing lists