On Mon, 22 Aug 2016 21:19:41 +0200, Arend Van Spriel wrote: > 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@xxxxxxxxxxxxx> > >>> Signed-off-by: Jakub Kicinski <jakub.kicinski@xxxxxxxxxxxxx> > >>> --- > > [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 don't think anything in this set prevents people from adding a FIELD_SET() wrapper. Quite the opposite and I'd encourage it. > 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. Yes, I agree this is a good solution as well. The reason I didn't go with this approach (apart from modifying an argument of a macro ;)) is the experience with rt2x00 which followed this design and I wasn't particularly fond of the resulting code. I find PREP macro easier to read (less parameters). It's also often convenient to not have to zero the variable for pure write and be able to combine the values in a parameter list: + mt7601u_wr(dev, MT_TXOP_CTRL_CFG, + FIELD_PREP(MT_TXOP_TRUN_EN, 0x3f) | + FIELD_PREP(MT_TXOP_EXT_CCA_DLY, 0x58)); or: + reg = cpu_to_le32(FIELD_PREP(MT_TXD_INFO_TYPE, DMA_PACKET) | + FIELD_PREP(MT_TXD_INFO_D_PORT, CPU_TX_PORT) | + FIELD_PREP(MT_TXD_INFO_LEN, len)); So each approach has it's advantages and I think they complement each other. Please note that I'm just using mt7601u as an example, I really don't need SET in the new code I'm trying to use these macros for now [1]. [1] https://patchwork.ozlabs.org/patch/628779/
