| Message ID | 77384d24810d9a22fc04cad6f7468f54a9cbaafe.1386108712.git.stefan@agner.ch (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
> +int tps6586x_get_version(struct device *dev) > +{ > + struct tps6586x *tps6586x = dev_get_drvdata(dev); > + > + return tps6586x->version; > +} > +EXPORT_SYMBOL_GPL(tps6586x_get_version); I thought Mark suggested that this routine was converted to a 'static inline' and moved into the header? Did you not think this was a good idea?
Am 2013-12-04 09:10, schrieb Lee Jones: >> +int tps6586x_get_version(struct device *dev) >> +{ >> + struct tps6586x *tps6586x = dev_get_drvdata(dev); >> + >> + return tps6586x->version; >> +} >> +EXPORT_SYMBOL_GPL(tps6586x_get_version); > > I thought Mark suggested that this routine was converted to a 'static > inline' and moved into the header? Did you not think this was a good > idea? As I pointed out in the comment above, the struct tps6586x is in the C file, so I would need to move that too. That's why I did not made that change in the end. What do you think, should I still move (and move the struct too?)
On Wed, 04 Dec 2013, Stefan Agner wrote: > Am 2013-12-04 09:10, schrieb Lee Jones: > >> +int tps6586x_get_version(struct device *dev) > >> +{ > >> + struct tps6586x *tps6586x = dev_get_drvdata(dev); > >> + > >> + return tps6586x->version; > >> +} > >> +EXPORT_SYMBOL_GPL(tps6586x_get_version); > > > > I thought Mark suggested that this routine was converted to a 'static > > inline' and moved into the header? Did you not think this was a good > > idea? > As I pointed out in the comment above, the struct tps6586x is in the C > file, so I would need to move that too. That's why I did not made that > change in the end. What do you think, should I still move (and move the > struct too?) Why would the struct have to be moved if the function is inline?
Am 2013-12-04 11:07, schrieb Lee Jones: > On Wed, 04 Dec 2013, Stefan Agner wrote: > >> Am 2013-12-04 09:10, schrieb Lee Jones: >> >> +int tps6586x_get_version(struct device *dev) >> >> +{ >> >> + struct tps6586x *tps6586x = dev_get_drvdata(dev); >> >> + >> >> + return tps6586x->version; >> >> +} >> >> +EXPORT_SYMBOL_GPL(tps6586x_get_version); >> > >> > I thought Mark suggested that this routine was converted to a 'static >> > inline' and moved into the header? Did you not think this was a good >> > idea? >> As I pointed out in the comment above, the struct tps6586x is in the C >> file, so I would need to move that too. That's why I did not made that >> change in the end. What do you think, should I still move (and move the >> struct too?) > > Why would the struct have to be moved if the function is inline? True, the inline I could have done without moving the struct and the function. Would you like me to create another revision doing this? But moving the function needs moving of the struct tps6586x declaration... [Sorry, this time with answer all]
On Wed, 04 Dec 2013, Stefan Agner wrote: > Am 2013-12-04 11:07, schrieb Lee Jones: > > On Wed, 04 Dec 2013, Stefan Agner wrote: > > > >> Am 2013-12-04 09:10, schrieb Lee Jones: > >> >> +int tps6586x_get_version(struct device *dev) > >> >> +{ > >> >> + struct tps6586x *tps6586x = dev_get_drvdata(dev); > >> >> + > >> >> + return tps6586x->version; > >> >> +} > >> >> +EXPORT_SYMBOL_GPL(tps6586x_get_version); > >> > > >> > I thought Mark suggested that this routine was converted to a 'static > >> > inline' and moved into the header? Did you not think this was a good > >> > idea? > >> As I pointed out in the comment above, the struct tps6586x is in the C > >> file, so I would need to move that too. That's why I did not made that > >> change in the end. What do you think, should I still move (and move the > >> struct too?) > > > > Why would the struct have to be moved if the function is inline? > > True, the inline I could have done without moving the struct and the > function. Would you like me to create another revision doing this? > > But moving the function needs moving of the struct tps6586x > declaration... > > [Sorry, this time with answer all] Do you know what, it's really not that important. Patch applied.
On Wed, Dec 04, 2013 at 10:07:28AM +0000, Lee Jones wrote: > On Wed, 04 Dec 2013, Stefan Agner wrote: > > As I pointed out in the comment above, the struct tps6586x is in the C > > file, so I would need to move that too. That's why I did not made that > > change in the end. What do you think, should I still move (and move the > > struct too?) > Why would the struct have to be moved if the function is inline? If the function is in the header and trying to use a struct that's only defined in the C file then it's not going to build - keeping the struct in the C file only does seem like a worthwhile thing for encapsulation.
> > > As I pointed out in the comment above, the struct tps6586x is in the C > > > file, so I would need to move that too. That's why I did not made that > > > change in the end. What do you think, should I still move (and move the > > > struct too?) > > > Why would the struct have to be moved if the function is inline? > > If the function is in the header and trying to use a struct that's only > defined in the C file then it's not going to build - keeping the struct > in the C file only does seem like a worthwhile thing for encapsulation. Yes, I just carried out my own testing and found that out. Prior to this I was under the impression that inline functions were directly transposed into the location of the call, were perhaps it could make use of any resources declared within that context. It appears that impression was not correct. Every day continues to be a school day. :)
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..
On 12/05/2013 10:43 AM, Stefan Agner wrote: > Am 2013-12-05 18:06, schrieb Stephen Warren: > <snip> >>> @@ -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.. > > The value comes from the return value of i2c_smbus_read_byte_data. If > the value is below zero its an EIO error. > > I could add a variable "version", but for me it felt strange because we > check if version is below zero. This feels like its a wrong version > rather than a transmit error. So I would prefer ret over version. But I > agree, when one just reads the patch, its not obvious what exactly > happens. In my opinion, using a variable named "version" here would be preferable. Testing that against <0 is just the way the I2C API works, so the same argument could be applied to any I2C access.
Am 2013-12-05 18:06, schrieb Stephen Warren: <snip> >> @@ -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.. The value comes from the return value of i2c_smbus_read_byte_data. If the value is below zero its an EIO error. I could add a variable "version", but for me it felt strange because we check if version is below zero. This feels like its a wrong version rather than a transmit error. So I would prefer ret over version. But I agree, when one just reads the patch, its not obvious what exactly happens. In v2, I moved the i2c_smbus_read_byte_data function call after the allocation, so it was more obvious for the reader. But then, as Thierry Reding pointed out, not moving it is an optimization: In case reading fails, we don't allocate memory first.
Am 2013-12-05 18:40, schrieb Stephen Warren: > On 12/05/2013 10:43 AM, Stefan Agner wrote: >> Am 2013-12-05 18:06, schrieb Stephen Warren: >> <snip> >>>> @@ -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.. >> >> The value comes from the return value of i2c_smbus_read_byte_data. If >> the value is below zero its an EIO error. >> >> I could add a variable "version", but for me it felt strange because we >> check if version is below zero. This feels like its a wrong version >> rather than a transmit error. So I would prefer ret over version. But I >> agree, when one just reads the patch, its not obvious what exactly >> happens. > > In my opinion, using a variable named "version" here would be > preferable. Testing that against <0 is just the way the I2C API works, > so the same argument could be applied to any I2C access. Hm, I try the empiric way: $ grep -r -e i2c_smbus_read_byte_data | grep "ret =" | wc -l 139 $ grep -r -e i2c_smbus_read_byte_data | grep "version =" | wc -l 3 Ok, thats not fair at all, version is usage specific whilst ret is not. $ grep -r -e i2c_smbus_read_byte_data | grep " = " | wc -l 703 On the other hand is the additional variable. But I think the compiler will optimize that anyway, so this might not be an argument at all :-) I see your point... Should I create another patch revision? Lee, is the patch already merged?
> >> <snip> > >>>> @@ -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.. > >> > >> The value comes from the return value of i2c_smbus_read_byte_data. If > >> the value is below zero its an EIO error. > >> > >> I could add a variable "version", but for me it felt strange because we > >> check if version is below zero. This feels like its a wrong version > >> rather than a transmit error. So I would prefer ret over version. But I > >> agree, when one just reads the patch, its not obvious what exactly > >> happens. > > > > In my opinion, using a variable named "version" here would be > > preferable. Testing that against <0 is just the way the I2C API works, > > so the same argument could be applied to any I2C access. So, FWIW I agree with Stephen and have done from the start. Please see my original comment from the first submission: > > ret = i2c_smbus_read_byte_data(client, TPS6586X_VERSIONCRC); > If you're going to do this, please change 'ret' to 'version'. > Hm, I try the empiric way: > > $ grep -r -e i2c_smbus_read_byte_data | grep "ret =" | wc -l > 139 > $ grep -r -e i2c_smbus_read_byte_data | grep "version =" | wc -l > 3 > > Ok, thats not fair at all, version is usage specific whilst ret is not. > > $ grep -r -e i2c_smbus_read_byte_data | grep " = " | wc -l > 703 I not really that worried about what everyone else does. I'm more concerned with doing what we deem to be the correct thing here. > On the other hand is the additional variable. But I think the compiler > will optimize that anyway, so this might not be an argument at all :-) > > I see your point... Should I create another patch revision? Lee, is the > patch already merged? It isn't. Please submit another version as Stephen requests.
diff --git a/drivers/mfd/tps6586x.c b/drivers/mfd/tps6586x.c index ee61fd7..56a8422 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; + int 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); +int 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)); @@ -472,6 +481,31 @@ static void tps6586x_power_off(void) tps6586x_set_bits(tps6586x_dev, TPS6586X_SUPPLYENE, SLEEP_MODE_BIT); } +static void tps6586x_print_version(struct i2c_client *client, int version) +{ + const char *name; + + switch (version) { + 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, version); +} + static int tps6586x_i2c_probe(struct i2c_client *client, const struct i2c_device_id *id) { @@ -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; + tps6586x_print_version(client, tps6586x->version); tps6586x->client = client; tps6586x->dev = &client->dev; diff --git a/include/linux/mfd/tps6586x.h b/include/linux/mfd/tps6586x.h index 8799454..cbecec2 100644 --- a/include/linux/mfd/tps6586x.h +++ b/include/linux/mfd/tps6586x.h @@ -13,6 +13,12 @@ #define TPS6586X_SLEW_RATE_SET 0x08 #define TPS6586X_SLEW_RATE_MASK 0x07 +/* VERSION CRC */ +#define TPS658621A 0x15 +#define TPS658621CD 0x2c +#define TPS658623 0x1b +#define TPS658643 0x03 + enum { TPS6586X_ID_SYS, TPS6586X_ID_SM_0, @@ -97,5 +103,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 int 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> --- Since the struct tps6586x is inside the c file, I could not easily move the tps6586x_get_version function as inline to the header file. Changes since v2: - Avoid moving devm_kzalloc --- drivers/mfd/tps6586x.c | 43 ++++++++++++++++++++++++++++++++++++++----- include/linux/mfd/tps6586x.h | 7 +++++++ 2 files changed, 45 insertions(+), 5 deletions(-)