On Sat, Jun 04, 2011 at 12:32:01PM -0400, Stijn Devriendt (sdevrien) wrote: > > From: anish singh [mailto:anish198519851985@xxxxxxxxx] > > > > I am no expert on HWMON but just want to > > add some points. > > @@ -454,7 +477,7 @@ static struct lm90_data *lm90_update_device(struct device *dev) > > > > if (data->flags & LM90_HAVE_LOCAL_EXT) { > > lm90_read16(client, LM90_REG_R_LOCAL_TEMP, > > - MAX6657_REG_R_LOCAL_TEMPL, > > + data->reg_local_ext, > > &data->temp11[4]); > > I don't think this variable reg_local_ext should exist as > > register address should be "# defined" and should not be > > part of lm90_data but i do see a case here where we are > > assuming MAX6657 is only having this LM90_HAVE_LOCAL_EXT > > flag set.So i think we should have some more branching here > > to detect the device and pass the corresponding register but as > > i said i am no expert. > > > > Only MAX6657 and SA56004 have the local temperature extension > register and unfortunately they reside at different offsets. > Therefore the probing will detect the right chip and, if supported, > use the correct register. > > > } else { > > if (lm90_read_reg(client, LM90_REG_R_LOCAL_TEMP, > > @@ -1372,6 +1400,11 @@ 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) { > > + data->reg_local_ext = lm90_params[data->kind].reg_local_ext; > > + BUG_ON(data->reg_local_ext == 0); > > + } > > + > > I think this BUG_ON is too harsh in probe.We generally use pr_err > > to print if something which is supposed to be set is not set.As BUG_ON > > will call kernel panic,right? > > The reason for adding the BUG_ON rather than the error was that it is > in fact a coding error when the flag is set without specifying the offset. > Such a condition should never make it into a running system and should be > caught during coding or review. > BUG_ON only does an oops, panic is optional depending on panic_on_oops being > set. > Maybe use WARN_ON instead ? Thanks, Guenter _______________________________________________ lm-sensors mailing list lm-sensors@xxxxxxxxxxxxxx http://lists.lm-sensors.org/mailman/listinfo/lm-sensors
