Re: [PATCH] i2c: new driver for ds1337 RTC

["Jean Delvare" <khali@linux-fr.org>]

Hi Ladislav,

> > Please slice this into separarte patches. Adding support for the DS1339
> > is one thing, bug fixes are another. I can't review patches which do
> > that many different things at once.
>
> I have objection ;-) Nothing in kernel seems to use that driver (...)

How would you know? It has a user-space interface
(ds1337_driver.command), which anyone might be using.

> (...) and driver wasn't reviewed well enough before including.

I did review this driver. Thanks for the compliment, I really appreciate.

The driver went through several review and update steps between James
Chapman and I, part of which can be seen here (there were a couple of
additional private mails IIRC):
  http://archives.andrew.net.au/lm-sensors/msg29709.html
It got as good a review as one would ask for, or so I believe.

> Therefore I'm still considering it as a new driver which can be easily
> reviewed as whole.

Reviewing small, individual patches is always easier than reviewing one
big patch doing several things at once, it has nothing to do with the
age of the code. And at any rate you shouldn't consider anything is
easy when you are not the one doing it.

> > As a side note, please avoid noise in your patch. Converting constants
> > from decimal to hexadecimal or renaming variables makes my review job
> > way harder, as it distracts me from the real point of your patch.
>
> Hexadecimal constant are there to match datasheet, (...)

And anyone may argue that decimal values are easier to read, or whatever,
and send a revert patch next week? James did it that way, values are
correct. I see no point in changing them.

> (...) local variables should be short and with new_client is was
> reaching 80 column limit, that's coding style and that's why I made
> that change.

You changed other variable names with no reason (unless you are going to
argue that "tm" is shorter than "dt", of course).

I *do* agree that "new_client" is a poor variable name in this context,
and "client" carries the very same amount of information. That's not
specific to this driver though, almost all i2c chip driver use that name.

> Please consider that I would suggest these changes to driver author
> before his patch went in tree in case I would read all that mailing
> lists around. I'm lazy, my bad...

Sure you would have been welcome to review James Chapman's original
patch. But you didn't, so the fact that you would have objected if you
did is irrelevant. The code is in the kernel tree now.

You are very welcome to review future patches, though.

> Driver's author should have probably last word anyway.

Not necessarily. I had similarly hairsplitting objections to James'
driver when he first posted his patch, and we reached a state of
agreement on everything quite easily. He is a very pleasant person to
work with.

> Here is patch which does cleanup/fixes only. It is still hard to review,
> but that's living. I won't split it to smaller parts just because I had
> to touch each single line in get/set date functions. Sorry if it sounds
> impolite, but now I can't spend more time on it. Perhaps later, if
> anyone don't buy it as is meanwhile...

I'm not sure if it qualifies as impolite, but I can tell you won't get
anywhere with such an attitude. Trust me, I've been there before, and
changed.

Cleanups and fixes belong to separate patches. Your current patch, even
without the DS1339 support, is a mess, mixing useful cleanups, useless
cleanups and actual fixes - unfortunately undocumented so we can't even
say what exactly you are trying to fix.

> Please let me know when/if you will accept support for DS1339.

After this patch has been split, reviewed and modified enough to be
accepted. Your time might be limited, this won't change a thing, sorry.
Patches are accepted when we think they are good, and not before. You
can't tell that the original ds1337 driver wasn't properly reviewed
and at the same time ask me to accept this random patch of yours while I
an not able to review it in its current state.

Thanks,
--
Jean Delvare