Hi Jean,
thanks for all your suggestions, I'll go through them. I agree to most
of your suggestions. I don't comment on those and just prepare a new
patch. Some of them are mortifying, sorry for them. (Actually I put in
these to check how exact the reviews are. You passed ;-)
Jean Delvare wrote:
> > below is a patch adding support for the Epson RX8025 SA/NB RTC module.
>
> First of all: where is this chip found?
This is a driver I wrote for a custom SoC. Otherwise Google found one
or two mails by people having ported the rtc8564 driver to the rx8025
chip.
> Have you verified if this chip isn't compatible with other chips we
> have drivers for already?
I just compared with the rtc8564 driver as I didn't expect to match that
chip. In retrospect this proofs true. (I compared with the kconfig
symbols in drivers/i2c/chips that have [Rr][Tt][Cc] in their
description. Did I miss anything?)
> > - There exists a RTC driver interface in include/asm-arm/rtc.h by
> > Russell King. Should I use it even if it's ARM specific?
> > For now I used the ioctl interface directly.
>
> See with Alessandro Zummo, he is working on a unified RTC core. Maybe
> you can simplify your code by using it.
He answered my mail, too. I'll look into it.
> Don't initialize a static global to NULL explicitly, the compiler will
> do it for you.
I'm not fluent in the different C variants, so I usually prefer to
explicitly initialize static global variables with NULL, too.
> > + if(down_interruptible(&rx8025_lock))
> > + return -ERESTARTSYS;
> [...], what exactly are you protecting with this mutex?
rx8025_clients and rx8025_rtcclient. Maybe I can get rid of it when
using Alessandro's RTC core.
> > + /* according to the documention read-only-bits read as 0 */
> > +#define CHECK_ROBITS(reg, mask) \
> > + if (unlikely(result[2 + (reg)] & ~(mask))) { \
> > + dev_warn(&rx8025_rtcclient->dev, \
> > + "value for " #reg " out of spec: %x\n", \
> > + result[2 + (reg)]); \
> > + result[2 + RX8025_REG_SEC] &= (mask); \
> > + }
> > + CHECK_ROBITS(RX8025_REG_SEC, 0x7f);
> > + CHECK_ROBITS(RX8025_REG_MIN, 0x7f);
> > + CHECK_ROBITS(RX8025_REG_HOUR, 0x3f);
> > + CHECK_ROBITS(RX8025_REG_WDAY, 0x07);
> > + CHECK_ROBITS(RX8025_REG_MDAY, 0x3f);
> > + CHECK_ROBITS(RX8025_REG_MONTH, 0x1f);
>
> Since you end up checking all unused bits regardless of what the
> datasheet says, it would be more efficient - and more readble - to just
> mask the values you read.
While developping I observed that having some of these bits set results
in wrong transitions. So I want to warn about it. I put the masking in
the if body to have CHECK_ROBITS expand to one statement (without doing
do {...} while(0)). Maybe it's sensible to move that check to the
detection phase as you suggested.
> Use "client" instead of "new_client" please. Another legacy we try to
> get rid of.
That was one that astonished me, too. I just did that to be consistent.
> > + if (!i2c_check_functionality(adapter, I2C_FUNC_I2C)) {
> > + dev_dbg(&adapter->dev, "doesn't support full I2C\n");
> > + goto errout;
> > + }
>
> Given that all block transfers you do are standard I2C block transfers,
> you may consider switching to i2c_smbus_{read,write}_i2c_block_data for
> SMBus compatibility. This would also make your code more simple, I
> think.
Well I just found out the SMBus interface may match my needs when I
posted the driver. I'm not sure about the transfer in
rx8025_get_rtctime. In the spec this is called "Simplified read method"
so I have to check if the interface suits. (But OTOH the chips supports
the "Standard read method for I2C bus", too, so there should be no
problem.) The difference is that I don't have to sent "restart",
slave address and read bit as part of the request.
> > + new_client->flags = 0;
>
> Not needed, as you've kzalloc'd the client structure.
That's a relict, the driver was initially developped for 2.6.12.5.
kzalloc is newer.
> And feel free to add yourself in MAINTAINERS as the maintainer of this
> new driver if you want to take care of it in the future.
Yes, I want to, but I didn't hat the courage to do that when submitting
the first (non-trivial) patch.
Best regards and many thanks for your detailed review.
Uwe
--
Uwe Zeisberger
FS Forth-Systeme GmbH, A Digi International Company
Kueferstrasse 8, D-79206 Breisach, Germany
Phone: +49 (7667) 908 0 Fax: +49 (7667) 908 200
Web: www.fsforth.de, www.digi.com
