Re: [PATCH v3 2/4] hwmon: (lm90) use macro defines for the status bit

[Wei Ni <wni@nvidia.com>]

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 .

>