Re: [PATCHv7 4/4] mt7601u: use linux/bitfield.h

[Arend Van Spriel <arend.vanspriel@broadcom.com>]

On 22-8-2016 12:52, Jakub Kicinski wrote:
> On Fri, 19 Aug 2016 21:02:02 +0200, Arend Van Spriel wrote:
>> On 19-8-2016 18:44, Jakub Kicinski wrote:
>>> Use the newly added linux/bitfield.h.
>>>
>>> Reviewed-by: Dinan Gunawardena <dinan.gunawardena@netronome.com>
>>> Signed-off-by: Jakub Kicinski <jakub.kicinski@netronome.com>
>>> ---

[snip]

>>> diff --git a/drivers/net/wireless/mediatek/mt7601u/eeprom.c b/drivers/net/wireless/mediatek/mt7601u/eeprom.c
>>> index 8d8ee0344f7b..da6faea092d6 100644
>>> --- a/drivers/net/wireless/mediatek/mt7601u/eeprom.c
>>> +++ b/drivers/net/wireless/mediatek/mt7601u/eeprom.c
>>> @@ -45,8 +45,8 @@ mt7601u_efuse_read(struct mt7601u_dev *dev, u16 addr, u8 *data,
>>>  	val = mt76_rr(dev, MT_EFUSE_CTRL);
>>>  	val &= ~(MT_EFUSE_CTRL_AIN |
>>>  		 MT_EFUSE_CTRL_MODE);
>>> -	val |= MT76_SET(MT_EFUSE_CTRL_AIN, addr & ~0xf) |
>>> -	       MT76_SET(MT_EFUSE_CTRL_MODE, mode) |
>>> +	val |= FIELD_PREP(MT_EFUSE_CTRL_AIN, addr & ~0xf) |
>>> +	       FIELD_PREP(MT_EFUSE_CTRL_MODE, mode) |
>>>  	       MT_EFUSE_CTRL_KICK;  
>>
>> MT_EFUSE_CTRL_KICK is probably a 1-bit field in MT_EFUSE_CTRL register.
>> It looks like you did not want to go all the way although you do give an
>> example in bitfield.h, ie. + *  #define REG_FIELD_B  BIT(7).
> 
> True, I just wanted to show in the examples that BIT() is OK to use.
> My feeling is that ORing in always set flags is acceptable, or at least
> it was my feeling when I wrote this code ;)

I find it tricky to have the user do the field clearing separately.
Especially if you allow the calling function to pass additional opaque
"flags". In my experience this is more error prone than anything else
when dealing with bit fields and as such it is unfortunate this aspect
is not addressed in your patches.

I would rather see:

	val = mt76_rr(dev, MT_EFUSE_CTRL);
	FIELD_SET(val, MT_EFUSE_CTRL_AIN, addr & ~0xf);
	FIELD_SET(val, MT_EFUSE_CTRL_MODE, mode);
	FIELD_SET(val, MT_EFUSE_CTRL_KICK, 1);
 	mt76_wr(dev, MT_EFUSE_CTRL, val);

in which FIELD_SET takes care of clearing the indicated field. I am
wondering how much these two solutions differ in terms of assembly
instructions.

Regards,
Arend