On 2018-06-13 12:50, Jarkko Nikula wrote:
> On 06/12/2018 03:26 PM, Peter Rosin wrote:
>> On 2018-06-12 08:09, Jarkko Nikula wrote:
>>> On 06/11/2018 06:20 PM, Andy Shevchenko wrote:
>>>> On Mon, 2018-06-11 at 17:22 +0300, Jarkko Nikula wrote:
>>>>> This is rather readability update than micro-optimization, or if not
>>>>> optimization at all. We take the input clock rate to a variable and
>>>>> pass
>>>>> that to SCL timing parameter calculation functions.
>>>>>
>>>>
>>>>> While at it, indent i2c_dw_scl_hcnt()/i2c_dw_scl_lcnt() argument list
>>>>> to
>>>>> the same alignment. Now first argument is off by one character.
>>>>
>>>> I think it still doesn't explain why instead of moving other lines, you
>>>> split the first one in each excerpt like this.
>>>>
>>> Indentation goes too far right, each following line needs 7 spaces and
>>> line length becomes over 80. I wouldn't like to go to that path.
>>
>> But the new line break does not make any sense *for this patch*. Just look
>> at it!
>>
>> The white space changes only makes sense when considering patch 6. And that
>> dependency is not mentioned in this commit message which makes any reviewer
>> go WFT when looking at this patch. I.e. in my view the white space changes
>> in this patch is just a preparation for patch 6. Which is a bit pointless,
>> why do you not just move the white space cleanup from patch 4 to patch 6?
>> Or do the cleanup here in patch 4, but do it in a way that makes sense by
>> itself (and then, if needed, redo it in patch 6).
>>
> I basically do here s/i2c_dw_clk_rate(dev)/ic_clk/ and while at it move
> the function call to its own line due the existing misalignment. I don't
> think it makes sense to split this tiny patch into two?
No, splitting into one extra patch is not what I'm after at all. What I was
after is that you'd have patch 4 like this
} else {
- hcnt = i2c_dw_scl_hcnt(i2c_dw_clk_rate(dev),
+ hcnt = i2c_dw_scl_hcnt(ic_clk,
4000, /* tHD;STA = tHIGH = 4.0 us */
and patch 6 like this.
} else {
- hcnt = i2c_dw_scl_hcnt(ic_clk,
+ dev->ss_hcnt =
+ i2c_dw_scl_hcnt(ic_clk,
4000, /* tHD;STA = tHIGH = 4.0 us */
> But what I'd like to do is to keep patch 6 as small as possible instead
> of moving this minor stuff there. I'm all fine dropping the whole
> misaligment fix, moving this patch before 6, changing commit log from v4
> [1], splitting this or whatever looks best for this minor uninteresting
> stuff :-)
The above doesn't alter neither size nor readability of patch 6 in any
significant way and has the benefit of not having that *really* strange
intermediate state to describe.
> 1. https://www.spinics.net/lists/linux-i2c/msg35230.html
Your description
"Instead of indenting the following lines we move the function
calls to own lines as this keeps the argument list aligned, line
length below 80 characters and no need to align with spaces."
is simply no justification for the strange intermediate state. It mainly
describes *what* you did (which is evident from the patch itself), not
*why*, and would only have made me go WTF even more had I not known about
patch 6. Sure, you talk about not wanting to align with spaces so that's
a why, but when was aligning with spaces *ever* a problem? And the line
length limit is common knowledge that need not be brought up, unless
perhaps if you break it...
FTR, the intermediate state looks like it is in desperate need of further
cleanup, which is never a good sign of a patch which describes itself as a
readability update.
But, as you say, this is not important. In the end all is fine. The
issue is that patch 4 does not make sense until you see patch 6, and
the description of patch 4 is still not helpful in v4.
Cheers,
Peter
