Re: [PATCH v4 1/3] mfd: tps6586x: add version detection

[Stephen Warren <swarren@wwwdotorg.org>]

On 12/03/2013 03:18 PM, Stefan Agner wrote:
> Use the VERSIONCRC to determine the exact device version. According to
> the datasheet this register can be used as device identifier. The
> identification is needed since some tps6586x regulators use a different
> voltage table.

> diff --git a/drivers/mfd/tps6586x.c b/drivers/mfd/tps6586x.c

> @@ -493,13 +527,12 @@ static int tps6586x_i2c_probe(struct i2c_client *client,
>  		return -EIO;
>  	}
>  
> -	dev_info(&client->dev, "VERSIONCRC is %02x\n", ret);
> -
>  	tps6586x = devm_kzalloc(&client->dev, sizeof(*tps6586x), GFP_KERNEL);
> -	if (tps6586x == NULL) {
> -		dev_err(&client->dev, "memory for tps6586x alloc failed\n");
> +	if (!tps6586x)
>  		return -ENOMEM;
> -	}
> +
> +	tps6586x->version = ret;

I have to say, I dislike this version of the patch. Separating the
reading of the version register from the assignment to tps6586x->version
doesn't make any sense, especially given that the version value is
stored in a variable named "ret"; that name isn't remotely related to
what's stored there. What if someone comes along later and adds more
code that assigns to ret between where it's repurposed for the version
value and where it's assigned to tps6586x->version? It'd be extremely
difficult for a patch reviewer to spot that given the limited context in
a diff, and quite non-obvious to the person changing the code too..