Re: [PATCH V1] regulator: DA9211 : support DA9213

[Mark Brown <broonie@kernel.org>]

[-- Attachment #1: Type: text/plain, Size: 1226 bytes --]

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.

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]