lkml.org 
[lkml]   [2005]   [Apr]   [7]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    Date
    SubjectRe: [PATCH] i2c: new driver for ds1337 RTC
    From
    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
    -
    To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
    the body of a message to majordomo@vger.kernel.org
    More majordomo info at http://vger.kernel.org/majordomo-info.html
    Please read the FAQ at http://www.tux.org/lkml/

    \
     
     \ /
      Last update: 2005-04-08 01:21    [W:2.060 / U:0.260 seconds]
    ©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site