[PATCH v4 1/3] mfd: tps6586x: add version detection
From: Stephen Warren <hidden>
Date: 2013-12-05 17:07:00
Also in:
linux-tegra, lkml
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.
quoted hunk ↗ jump to hunk
diff --git a/drivers/mfd/tps6586x.c b/drivers/mfd/tps6586x.c
quoted hunk ↗ jump to hunk
@@ -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..