Hi Wei, On Wed, 17 Jul 2013 15:03:35 +0800, Wei Ni wrote: > On 07/16/2013 12:57 AM, Jean Delvare wrote: > > On Fri, 12 Jul 2013 15:48:05 +0800, Wei Ni wrote: > >> Add bit defines for the status register. > > > > Regarding the subject: for me these are constants, not macros. AFAIK > > the term "macro" refers to defines with parameters only. > > How about "Introduce status bits" I'd say "Define status bits" as this is exactly what you're doing ;-) That being said, your patch actually does more than this, as you are moving code around and to a separate function. The patch description should say that and explain why. > >> (...) > >> + if ((status & 0x7f) == 0 && (status2 & 0xfe) == 0) > >> + return false; > > > > It's a bit disappointing to not use the freshly introduced constants. > > That being said I agree it would make the code hard to read, so you can > > leave it as is. > > Sorry, I forgot it. > How about to define: > #define LM90_STATUS_MASK 0x7f > #define MAX6696_STATUS2_MASK 0xfe I wouldn't bother. I suspect that this code will have to be reworked soon anyway and these constants may no longer be needed then. > Or since Guenter is for vacation, I can just leave it as is, and wait > him back to talk about below issue. I do maintain the lm90 driver, so the decision is up to me. Guenter did a preliminary review of your patches and I am grateful for that, but I do not intend to wait for his return to continue with your patches. Otherwise he will have to do the same when he returns and I am gone, and this may end up delaying your patches by one kernel version. > >> (...) > >> + struct lm90_data *data = i2c_get_clientdata(client); > >> + u8 config, alarms; > >> + > >> + lm90_read_reg(client, LM90_REG_R_STATUS, &alarms); > > > > You end up reading LM90_REG_R_STATUS, which is not OK. This register > > contains self-clearing bits, so there is no guarantee that the second > > read will return the same value as the first read. You'll have to come > > up with a different approach that reads LM90_REG_R_STATUS only once. > > Oh, yes, this is a problem, I didn't noticed it. > How about to use this: > bool lm90_alarms_tripped(*client, *status); > bool lm90_alarms2_tripped(*client, *status2); > So we can read the status only once and pass it. This is a good idea but you only need status, not status2, so it can be made simpler: bool lm90_is_tripped(*client, *status); (handling both status and status 2 as you already do.) -- Jean Delvare -- To unsubscribe from this list: send the line "unsubscribe linux-tegra" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
