lkml.org 
[lkml]   [2013]   [Nov]   [27]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
Date
From
SubjectRe: [PATCH 1/3] mfd: tps6586x: add version detection
<snip>

> static int __remove_subdev(struct device *dev, void *unused)
> {
> platform_device_unregister(to_platform_device(dev));
> @@ -477,6 +486,7 @@ static int tps6586x_i2c_probe(struct i2c_client *client,
> {
> struct tps6586x_platform_data *pdata = dev_get_platdata(&client->dev);
> struct tps6586x *tps6586x;
> + const char *name = "";

How much memory space does this guarantee?

> int ret;
>
> if (!pdata && client->dev.of_node)
> @@ -487,20 +497,39 @@ static int tps6586x_i2c_probe(struct i2c_client *client,
> return -ENOTSUPP;
> }
>
> + tps6586x = devm_kzalloc(&client->dev, sizeof(*tps6586x), GFP_KERNEL);
> + if (tps6586x == NULL) {

Can you take this oppotunity to change to:
if (!tps6586x)

> + dev_err(&client->dev, "memory for tps6586x alloc failed\n");

I'm not keen on -ENOMEM prints, just return -ENOMEM and be done with it.

> + return -ENOMEM;
> + }
> +
> ret = i2c_smbus_read_byte_data(client, TPS6586X_VERSIONCRC);

If you're going to do this, please change 'ret' to 'version'.

> if (ret < 0) {
> dev_err(&client->dev, "Chip ID read failed: %d\n", ret);
> return -EIO;

Why are you returning an error here when you have a valid enum of:
TPS6586X_ANY = -1,

> }
>
> - 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");
> - return -ENOMEM;
> + tps6586x->version = (enum tps6586x_version)ret;
> + switch (ret) {
> + case TPS658621A:
> + name = "TPS658621A";
> + break;
> + case TPS658621CD:
> + name = "TPS658621C/D";
> + break;
> + case TPS658623:
> + name = "TPS658623";
> + break;
> + case TPS658643:
> + name = "TPS658643";
> + break;
> + default:
> + name = "TPS6586X";
> + break;
> }
>
> + dev_info(&client->dev, "Found %s, VERSIONCRC is %02x\n", name, ret);
> +

I'd suggest pulling this out of probe() and into a separate subroutine.

<snip>

--
Lee Jones
Linaro STMicroelectronics Landing Team Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/

\
 
 \ /
  Last update: 2013-11-27 14:41    [W:0.156 / U:0.996 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site