| Message ID | ef881d39f78b75b4badeebb5be0264edc3906d86.1385508112.git.stefan@agner.ch (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
<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>
Oh, and it's usually a good idea to CC all of the maintainers. This patch nearly slipped through the gaps.
Am 2013-11-27 14:09, schrieb Lee Jones: > <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? > Well, its just an empty string (so, 1). Actually I could assign NULL here, since we do have a default in the case later on. > >> 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, > Hm, when the device is not answering on that request, the probe method should fail I would say. This means that the device is missing most likely. However, I should set the device version to TPS6586X_ANY if I happen to end up in the default case. >> } >> >> - 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> Since I will alter the version when I end up in the default case in my next patch, would you still do a separate subroutine? I think its somewhat heavily coupled to the probe function. Sorry missing you on the CC, will do next time :-) -- Stefan
> >> 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, > > > Hm, when the device is not answering on that request, the probe method > should fail I would say. This means that the device is missing most > likely. However, I should set the device version to TPS6586X_ANY if I > happen to end up in the default case. I would say that returning an error is the sound thing to do, but I'm missing the point of TPS6586X_ANY, as it doesn't appear to be used in this context. > >> } > >> > >> - 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> > Since I will alter the version when I end up in the default case in my > next patch, would you still do a separate subroutine? I think its > somewhat heavily coupled to the probe function. > > Sorry missing you on the CC, will do next time :-) It's not that it's unrelated, it's just a whole bulk of code which is essentially featureless. 17 lines of code just to have the name of the device in the bootlog. For that reason I'd like to see this abstracted from (the useful stuff in) probe() please.
Re-adding the list. Please remember to 'reply to all' when discussing patches. > > >> Hm, when the device is not answering on that request, the probe method > > >> should fail I would say. This means that the device is missing most > > >> likely. However, I should set the device version to TPS6586X_ANY if I > > >> happen to end up in the default case. > > > > > > I would say that returning an error is the sound thing to do, but I'm > > > missing the point of TPS6586X_ANY, as it doesn't appear to be used in > > > this context. > > > > > > > Yes, its mainly used in the regulator code later on. I do assign voltage > > table to TPS6586X_ANY, if its the appropriate table for any version of > > the device. > > <snip> > > > Ok, will change this. Setting the version to TPS6586X_ANY in the default > > case is anyway not a good idea since it would suppress unknown versions. > > Perhaps I should suggest to make TPS6586X_ANY a positive number then, > as a negative value to me indicates more of an error than a generic > parameter.
Am 2013-11-27 15:36, schrieb Lee Jones: <snip> >> Perhaps I should suggest to make TPS6586X_ANY a positive number then, >> as a negative value to me indicates more of an error than a generic >> parameter. I see, its especially confusing since the version is filled using the i2c_smbus_read_byte_data functions return value. The version field is a 8-Bit value according to the data sheet, I could use 0x100 as TPS6586X_ANY identifier.
On Wed, 27 Nov 2013, Stefan Agner wrote: > Am 2013-11-27 15:36, schrieb Lee Jones: > <snip> > >> Perhaps I should suggest to make TPS6586X_ANY a positive number then, > >> as a negative value to me indicates more of an error than a generic > >> parameter. > I see, its especially confusing since the version is filled using the > i2c_smbus_read_byte_data functions return value. The version field is a > 8-Bit value according to the data sheet, I could use 0x100 as > TPS6586X_ANY identifier. How far are we away from using 0xFF? I'd be happy to use that and change it _if_ we ever get close. If it's likely that it'll be used, then sure 0x100 sounds okay too.
Am 2013-11-27 16:30, schrieb Lee Jones: > On Wed, 27 Nov 2013, Stefan Agner wrote: > >> Am 2013-11-27 15:36, schrieb Lee Jones: >> <snip> >> >> Perhaps I should suggest to make TPS6586X_ANY a positive number then, >> >> as a negative value to me indicates more of an error than a generic >> >> parameter. >> I see, its especially confusing since the version is filled using the >> i2c_smbus_read_byte_data functions return value. The version field is a >> 8-Bit value according to the data sheet, I could use 0x100 as >> TPS6586X_ANY identifier. > > How far are we away from using 0xFF? > > I'd be happy to use that and change it _if_ we ever get close. > > If it's likely that it'll be used, then sure 0x100 sounds okay too. Yes, I thought about 0xFF too. The latest device we support is TPS658643 (according to data sheet release dates), which has the smallest version number (03). Since it seems to be a CRC (hence VERSIONCRC) the number is quite random. Also, 0xFF sounds like a bitmask which can mask all versions, but the versions can't be used bitwise... So I would prefer to go with 0x100.
On Wed, 27 Nov 2013, Stefan Agner wrote: > Am 2013-11-27 16:30, schrieb Lee Jones: > > On Wed, 27 Nov 2013, Stefan Agner wrote: > > > >> Am 2013-11-27 15:36, schrieb Lee Jones: > >> <snip> > >> >> Perhaps I should suggest to make TPS6586X_ANY a positive number then, > >> >> as a negative value to me indicates more of an error than a generic > >> >> parameter. > >> I see, its especially confusing since the version is filled using the > >> i2c_smbus_read_byte_data functions return value. The version field is a > >> 8-Bit value according to the data sheet, I could use 0x100 as > >> TPS6586X_ANY identifier. > > > > How far are we away from using 0xFF? > > > > I'd be happy to use that and change it _if_ we ever get close. > > > > If it's likely that it'll be used, then sure 0x100 sounds okay too. > > Yes, I thought about 0xFF too. The latest device we support is TPS658643 > (according to data sheet release dates), which has the smallest version > number (03). Since it seems to be a CRC (hence VERSIONCRC) the number is > quite random. Also, 0xFF sounds like a bitmask which can mask all > versions, but the versions can't be used bitwise... So I would prefer to > go with 0x100. Deal!
On 11/26/2013 04:45 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 > @@ -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 = ""; There's no need to assign any value here; the switch below always assigns something over the top of the variable, so it doesn't need to be pre-initialized. > + 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; Why not make the version variable an integer of some kind. Then, the cast wouldn't be required. The values like TPS658621A can be #defines or const u32. > + switch (ret) { That should switch on tps6586x->version since that's been assigned; it'll make the purpose a bit clearer. > + default: > + name = "TPS6586X"; > + break; > } I hope the rest of the code deals with unknown values of tps6586x->version OK.
Am 2013-11-27 17:58, schrieb Stephen Warren: <snip> >> + 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; > > Why not make the version variable an integer of some kind. Then, the > cast wouldn't be required. The values like TPS658621A can be #defines or > const u32. > Yep this would work too. Whatever is preferred, maybe define would be more consistent since SLEW_RATE are defines too (both, the VERSIONCRC and SLEW_RATE values are possible content of registers). >> + switch (ret) { > > That should switch on tps6586x->version since that's been assigned; > it'll make the purpose a bit clearer. > >> + default: >> + name = "TPS6586X"; >> + break; >> } > > I hope the rest of the code deals with unknown values of > tps6586x->version OK. > Well, the tps6586x->version is not completly unknown, its something valid which is returned by i2c_smbus_read_byte_data. According to the data sheet this should be a 8-Bit value. However, the patch should handle every version, since I tried to make the change backward compatible: The driver now and then supports every device version, just the default voltage table are applied if there are no specific tables available.
diff --git a/drivers/mfd/tps6586x.c b/drivers/mfd/tps6586x.c index ee61fd7..a3ba9c1 100644 --- a/drivers/mfd/tps6586x.c +++ b/drivers/mfd/tps6586x.c @@ -124,6 +124,7 @@ struct tps6586x { struct device *dev; struct i2c_client *client; struct regmap *regmap; + enum tps6586x_version version; int irq; struct irq_chip irq_chip; @@ -208,6 +209,14 @@ int tps6586x_irq_get_virq(struct device *dev, int irq) } EXPORT_SYMBOL_GPL(tps6586x_irq_get_virq); +enum tps6586x_version tps6586x_get_version(struct device *dev) +{ + struct tps6586x *tps6586x = dev_get_drvdata(dev); + + return tps6586x->version; +} +EXPORT_SYMBOL_GPL(tps6586x_get_version); + 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 = ""; 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) { + dev_err(&client->dev, "memory for tps6586x alloc failed\n"); + return -ENOMEM; + } + ret = i2c_smbus_read_byte_data(client, TPS6586X_VERSIONCRC); if (ret < 0) { dev_err(&client->dev, "Chip ID read failed: %d\n", ret); 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"); - 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); + tps6586x->client = client; tps6586x->dev = &client->dev; i2c_set_clientdata(client, tps6586x); diff --git a/include/linux/mfd/tps6586x.h b/include/linux/mfd/tps6586x.h index 8799454..40a36a0 100644 --- a/include/linux/mfd/tps6586x.h +++ b/include/linux/mfd/tps6586x.h @@ -62,6 +62,14 @@ enum { TPS6586X_INT_RTC_ALM2, }; +enum tps6586x_version { + TPS658621A = 0x15, + TPS658621CD = 0x2c, + TPS658623 = 0x1b, + TPS658643 = 0x03, + TPS6586X_ANY = -1, +}; + struct tps6586x_settings { int slew_rate; }; @@ -97,5 +105,6 @@ extern int tps6586x_clr_bits(struct device *dev, int reg, uint8_t bit_mask); extern int tps6586x_update(struct device *dev, int reg, uint8_t val, uint8_t mask); extern int tps6586x_irq_get_virq(struct device *dev, int irq); +extern enum tps6586x_version tps6586x_get_version(struct device *dev); #endif /*__LINUX_MFD_TPS6586X_H */
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. Signed-off-by: Stefan Agner <stefan@agner.ch> --- drivers/mfd/tps6586x.c | 41 +++++++++++++++++++++++++++++++++++------ include/linux/mfd/tps6586x.h | 9 +++++++++ 2 files changed, 44 insertions(+), 6 deletions(-)