On 07/17/2013 04:28 PM, Jean Delvare wrote: > 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. ok, I will update it in my next version. > >>>> (...) >>>> + 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. Ok, let's leave it as is. > >> 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. I will send out patches soon :) > >>>> (...) >>>> + 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.) Yes this is simpler, but I think in the future we may need to handle the status2, how to handle it ? Or we can define the status as bit[0~7]->status and bit[8~15]->status2 . > -- 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
