Re: [PATCH] i2c: iproc: Change driver to use 'BIT' macro

[Ray Jui <ray.jui@broadcom.com>]

On 4/17/2019 11:21 PM, Peter Rosin wrote:
> On 2019-04-18 01:48, Ray Jui wrote:
>>
>>
>> On 4/14/2019 11:56 PM, Peter Rosin wrote:
>>> On 2019-04-13 00:59, Peter Rosin wrote:
>>>> On 2019-04-03 23:05, Ray Jui wrote:
>>>>> Change the iProc I2C driver to use the 'BIT' macro from all '1 << XXX'
>>>>> bit operations to get rid of compiler warning and improve readability of
>>>>> the code
>>>>
>>>> All? I see lots more '1 << XXX_SHIFT' matches. I might be behind though?
>>>
>>> I verified that, and yes indeed, I was behind. That said, see below...
>>>
>>
>> Right. Previous change that this change depends on is already queued in
>> i2c/for-next.
>>
>>>> Anyway, if you are cleaning up, I'm just flagging that BIT(XXX_SHIFT) looks
>>>> a bit clunky to me. You might consider renaming all those single-bit
>>>> XXX_SHIFT macros to simple be
>>>>
>>>> #define XXX BIT(<xxx>)
>>>>
>>>> instead of
>>>>
>>>> #define XXX_SHIFT <xxx>
>>>>
>>>> but that triggers more churn, so is obviously more error prone. You might
>>>> not dare it?
>>>>
>>
>> With the current code, I don't see how that is cleaner. With XXX_SHIFT
>> specified, it makes it very clear to the user that the define a for a
>> bit location within a register. You can argue and say it makes the
>> define longer, but not less clear.
>>
>>>> Cheers,
>>>> Peter
>>>>
>>>>> Signed-off-by: Ray Jui <ray.jui@broadcom.com>
>>>>> ---
>>>>>  drivers/i2c/busses/i2c-bcm-iproc.c | 6 +++---
>>>>>  1 file changed, 3 insertions(+), 3 deletions(-)
>>>>>
>>>>> diff --git a/drivers/i2c/busses/i2c-bcm-iproc.c b/drivers/i2c/busses/i2c-bcm-iproc.c
>>>>> index 562942d0c05c..a845b8decac8 100644
>>>>> --- a/drivers/i2c/busses/i2c-bcm-iproc.c
>>>>> +++ b/drivers/i2c/busses/i2c-bcm-iproc.c
>>>>> @@ -717,7 +717,7 @@ static int bcm_iproc_i2c_xfer_single_msg(struct bcm_iproc_i2c_dev *iproc_i2c,
>>>>>  
>>>>>  			/* mark the last byte */
>>>>>  			if (i == msg->len - 1)
>>>>> -				val |= 1 << M_TX_WR_STATUS_SHIFT;
>>>>> +				val |= BIT(M_TX_WR_STATUS_SHIFT);
>>>>>  
>>>>>  			iproc_i2c_wr_reg(iproc_i2c, M_TX_OFFSET, val);
>>>>>  		}
>>>>> @@ -844,7 +844,7 @@ static int bcm_iproc_i2c_cfg_speed(struct bcm_iproc_i2c_dev *iproc_i2c)
>>>>>  
>>>>>  	iproc_i2c->bus_speed = bus_speed;
>>>>>  	val = iproc_i2c_rd_reg(iproc_i2c, TIM_CFG_OFFSET);
>>>>> -	val &= ~(1 << TIM_CFG_MODE_400_SHIFT);
>>>>> +	val &= ~BIT(TIM_CFG_MODE_400_SHIFT);
>>>>>  	val |= (bus_speed == 400000) << TIM_CFG_MODE_400_SHIFT;
>>>
>>> These two statements now no longer "match". One uses BIT and the other open
>>> codes the shift. I think that's bad. Losing the _SHIFT suffix and including
>>> BIT in the macro expansion, as suggested above, yields:
>>>
>>> 	val &= ~TIM_CFG_MODE_400;
>>> 	if (bus_speed == 400000)
>>> 		val |= TIM_CFG_MODE_400;
>>>
>>> which is perhaps one more line, but also more readable IMO.
>>>
>>
>> A single line with evaluation embedded is nice and clean to me. I guess
>> this is subjective.
> 
> The "problem" I had when I looked at the driver was not any one specific
> instance. It was just that, for my taste, the code had too many shifts
> etc inline with the real code. Replacing 1 << xyz_SHIFT with BIT(xyz_SHIFT)
> is not a real improvement, they are just about equal to me, it's just that
> there are still too many mechanical things happening that could easily be
> tucked away with the suggested approach.
> 

Right, for your taste. Like I said, I feel this is very subjective. To
me, and many other I2C driver owners (I just checked how many other I2C
drivers also appear to prefer to use XXX_SHIFT and there are a lot of
them), using XXX_SHIFT makes it more clear that the define is intended
to be used for bit shift operation.

>> I'll leave the decision to Wolfram. If he also prefers the above change
>> to be made, sure. Otherwise, I'll leave it as it is.
> 
> But if you see no value in my suggestion and/or don't what to take the
> cleanup one step further, then just leave it as-is.
> 

Again, this is subjective. Personally I do not feel this is "cleanup one
step further". To me, this change will make the code less clear on the
intended operation.

>>> But all this is of course in deep nit-pick-territory...
>>>
>>> Cheers,
>>> Peter
>>>
>>
>> Thanks,
>>
>> Ray
>>
>