Hi Jean, On Thu, Oct 04, 2012 at 05:29:39PM +0200, Jean Delvare wrote: > > I'd rather drop the changelog altogether, we have revision control > systems (svn and git) for that. > Changelog dropped. > > This is a mask, would be good to make it visible in the name. OTOH the > masking is a no-op in practice so I'm not sure it's worth defining. > I removed it. I understand that byte mask shouldn't be keeped but bit mask is ok. Is it the right logic ? > I can't see the rationale for changing this variable name. temp was > just fine. Changing it makes the patch larger for no good reason. Same > later below. > The variable was changed to normalizing purpose. To keep things clear, I removed that diff. > > This is an actual code change. You just can't do that in a cleanup > patch, sorry. I don't even see the benefit, this makes the logic harder > to understand. > Indeed, it shouldn't be here. > This again is a code change. It subtly changes the behavior of the > driver if several errors are reported at the same time. You can't do > that in a cleanup patch! If you really want to do it, make it a > separate patch, so that you can properly describe the change and the > reasons why you think it is good. > Removed it. > Unrelated with this patch, but I think the above is wrong. The > datasheet says to write 1 to clear bits in the SMB_STS register, so the > above is clearing all bits _except_ SMBCOL_STS. Not good. OTOH we will > end up clearing all bits in sis630_transaction_end() anyway, so there's > no point in doing it already here. > Modified and splitted to a separate patch. > > Please leave "clear" as it was, it is much more readable. Remember you > do not have to care about the 80 column limit for strings. > > You can keep the string on a single line. > Good note taken. > > Spaces can be added around "+"s too. > > > Space before comma could be removed. > --> Cleanup patch v2 :) Best regards. -- Amaury Decrême -- To unsubscribe from this list: send the line "unsubscribe linux-i2c" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
