Messages in this thread | | | Subject | RE: [lm-sensors] [PATCH] Add support for the Philips SA56004 temperature sensor. | Date | Mon, 23 May 2011 09:08:42 +0200 | From | "Stijn Devriendt (sdevrien)" <> |
| |
> -----Original Message----- > From: Guenter Roeck [mailto:guenter.roeck@ericsson.com] > > > if (!name) { /* identification failed */ > > @@ -1372,6 +1401,18 @@ static int lm90_probe(struct i2c_client > *new_client, > > /* Set maximum conversion rate */ > > data->max_convrate = lm90_params[data->kind].max_convrate; > > > > + if (data->flags & LM90_HAVE_LOCAL_EXT) { > > + if (lm90_params[data->kind].local_ext_offset > 0) > > + data->local_ext_offset = > > + lm90_params[data->kind].local_ext_offset; > > + else { > > + dev_err(&new_client->dev, > > + "Invalid temperature extension register. " > > + "Accuracy may be limited.\n"); > > + data->flags &= (~LM90_HAVE_LOCAL_EXT); > > + } > > Either { } in both branches of the if statement, or none. > ( ) around ~LM90_HAVE_LOCAL_EXT is unnecessary. > > I see it as BUG if LM90_HAVE_LOCAL_EXT is set but local_ext_offset isn't. > That should be found during coding (or code review), and not be exported > to the user. So, from my perspective, the check is unnecessary. I'll leave > that up to Jean to decide, though. > Do you think a BUG_ON() would be better suited here?
> In addition to the above, your patch generates several checkpatch errors > (trailing whitespace). Please fix. I recall letting checkpatch yell at me... I'll have another round of it to be sure.
Thanks, Stijn
| |