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

[Ladislav Michl <ladis@linux-mips.org>]

Jean,

I'll comment your mail first and then send separate patches (somehow
I can't sleep this night :))

On Thu, Apr 07, 2005 at 11:29:08PM +0200, Jean Delvare wrote:
> > * Move NULL argument checking from get/set date functions to
> >   ds1337_command function, so it is only at one place. Note that other
> >   drivers do not this checking at all and I think it is pointess,
> >   because you have to know that you are passing struct rtc_time
> >   anyway.
> 
> I am not certain these are the right things to do (moving the check or
> removing it). I am not a specialist of ioctl, but it looks to me that
> ds1337_command acts as a dispatcher, branching to various functions
> depending on the value of cmd. I can imagine that some functions take an
> argument, and some don't, so checking for NULL pointer in the dispatcher
> doesn't make much sense. Now it is correct that for now all (two)
> functions need a parameter, but what if later a function is added, which
> takes no parameter? You'd have to undo your change and move the check in
> each function again.
> 
> As for the check itself, the pointer somehow comes from user-space as I
> understand it, so you can't tell whether it's NULL or not - so checking
> makes full sense to me. If you take a look at the rtc8564 driver you'll
> see it *does* check for NULL pointers too.

You can't tell if memory it points to is valid. Okay, probably better
than nothing.

> > @@ -95,60 +96,38 @@
> >   */
> >  static int ds1337_get_datetime(struct i2c_client *client, struct
> >  rtc_time *dt) {
> > -	struct ds1337_data *data = i2c_get_clientdata(client);
> > -	int result;
> > -	u8 buf[7];
> > -	u8 val;
> > -	struct i2c_msg msg[2];
> > -	u8 offs = 0;
> > -
> > -	if (!dt) {
> > -		dev_dbg(&client->adapter->dev, "%s: EINVAL: dt=NULL\n",
> > -			__FUNCTION__);
> > -
> > -		return -EINVAL;
> > -	}
> > -
> > -	msg[0].addr = client->addr;
> > -	msg[0].flags = 0;
> > -	msg[0].len = 1;
> > -	msg[0].buf = &offs;
> > -
> > -	msg[1].addr = client->addr;
> > -	msg[1].flags = I2C_M_RD;
> > -	msg[1].len = sizeof(buf);
> > -	msg[1].buf = &buf[0];
> > +	unsigned char buf[7] = { 0, }, addr[1] = { 0 };
> > +	struct i2c_msg msgs[2] = {
> > +		{ client->addr, 0,        1, addr },
> > +		{ client->addr, I2C_M_RD, 7, buf  }
> > +	};
> > +	int result = i2c_transfer(client->adapter, msgs, 2);
> >  
> > -	result = client->adapter->algo->master_xfer(client->adapter,
> > -						    &msg[0], 2);
> 
> You are doing much more than just using i2c_transfer instead of
> master_xfer. You are also rewriting the way the message data is
> initialized. I see no reason to do that, as the previous code was
> correct as far as I can see.

Right, I just made it shorter. One more point for you, my way is not
struct i2c_msg change proof. I'll drop it.

> > -	if (result >= 0) {
> (...)
> > +	if (result < 0) {
> 
> By changing this you are making your patch much bigger and harder to
> review. Why do you do that?

Here you need to look at patched code. Now conditions in both
ds1337_get_datetime and ds1337_set_datetime look similar, so code is
IHMO easily readable. I'm fine with droping this change.

> > -		val = buf[2] & 0x3f;
> > -		dt->tm_hour = BCD_TO_BIN(val);
> (...)
> > +		dt->tm_hour = BCD2BIN(buf[2] & 0x3f);
> 
> No, James is correct. BCD2BIN (or BCD_TO_BIN for that matter) is a
> macro which evaluates its argument more than once. Using a temporary
> variable makes sense.

Agree.

> > +	unsigned char buf[8];
> >  	int result;
> > -	u8 buf[8];
> 
> Wow, what a useful change. Please please please... Focus on making your
> patch compact, have it do just the thing it is supposed (and advertised)
> to do. You know, I'll repeat it until you get it. No matter how many
> tries it takes.

Save your time I got it. buf is supposed to be char, that's what function
expects. I wrongly made it unsigned. u8, u16 etc. are used in case
when you for example need to generate say 8 bit bus access or need same 
width on all architectures. Neither is case here and using u8 makes no
sense. Anyway, will drop change. 

> >  	if (dt->tm_year >= 2000) {
> > -		val = dt->tm_year - 2000;
> >  		buf[6] |= (1 << 7);
> > -	} else {
> > -		val = dt->tm_year - 1900;
> > -	}
> > -	buf[7] = BIN_TO_BCD(val);
> > +		buf[7] = BIN2BCD(dt->tm_year - 2000);
> > +	} else
> > +		buf[7] = BIN2BCD(dt->tm_year - 1900);
> 
> Same as before, the use of a temporary variable makes full sense, don't
> change that. And you're again adding noise by dropping a pair of curly
> braces.

That's only because I read mail by jgarzik suggesting to remove such
braces few hours ago :) Also, i'll drop this change.

Best regards,
	ladis