lkml.org 
[lkml]   [2005]   [Apr]   [7]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    Patch in this message
    /
    Date
    SubjectRe: [PATCH] i2c: new driver for ds1337 RTC
    From
    On Thu, Apr 07, 2005 at 11:59:05AM +0200, Jean Delvare wrote:
    > 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 and
    driver wasn't reviewed well enough before including. Therefore I'm still
    considering it as a new driver which can be easily reviewed as whole.

    > 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, 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.

    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... Driver's author should have probably
    last word anyway.

    > Once you realize that the time I need to review a patch increases in a
    > quadratic way (if not worse) relatively to the length of the patch, I am
    > sure you will see why it is important to send small patches doing just
    > one thing without noise.

    Ok, sending cleanup fixes only. Support for ds1339 will be in separate
    patch. Sorry I won't do more, because my time is also limited.

    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... Please let me know when/if you will
    accept support for DS1339.

    Best regards,
    ladis

    ===== drivers/i2c/chips/ds1337.c 1.1 vs edited =====
    --- 1.1/drivers/i2c/chips/ds1337.c 2005-03-31 22:58:08 +02:00
    +++ edited/drivers/i2c/chips/ds1337.c 2005-04-07 13:02:18 +02:00
    @@ -2,8 +2,9 @@
    * linux/drivers/i2c/chips/ds1337.c
    *
    * Copyright (C) 2005 James Chapman <jchapman@katalix.com>
    + * Copyright (C) 2005 Ladislav Michl <ladis@linux-mips.org>
    *
    - * based on linux/drivers/acron/char/pcf8583.c
    + * based on linux/drivers/acorn/char/pcf8583.c
    * Copyright (C) 2000 Russell King
    *
    * This program is free software; you can redistribute it and/or modify
    @@ -25,12 +26,13 @@
    #include <linux/list.h>

    /* Device registers */
    -#define DS1337_REG_HOUR 2
    -#define DS1337_REG_DAY 3
    -#define DS1337_REG_DATE 4
    -#define DS1337_REG_MONTH 5
    -#define DS1337_REG_CONTROL 14
    -#define DS1337_REG_STATUS 15
    +#define DS1337_REG_HOUR 0x02
    +#define DS1337_REG_DAY 0x03
    +#define DS1337_REG_DATE 0x04
    +#define DS1337_REG_MONTH 0x05
    +#define DS1337_REG_CONTROL 0x0e
    +#define DS1337_REG_STATUS 0x0f
    +#define DS1339_REG_CHARGE 0x10

    /* FIXME - how do we export these interface constants? */
    #define DS1337_GET_DATE 0
    @@ -93,116 +95,74 @@
    /*
    * Chip access functions
    */
    -static int ds1337_get_datetime(struct i2c_client *client, struct rtc_time *dt)
    +static int ds1337_get_datetime(struct i2c_client *client, struct rtc_time *tm)
    {
    - 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];
    -
    - result = client->adapter->algo->master_xfer(client->adapter,
    - &msg[0], 2);
    + 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);

    - dev_dbg(&client->adapter->dev,
    + dev_dbg(&client->dev,
    "%s: [%d] %02x %02x %02x %02x %02x %02x %02x\n",
    __FUNCTION__, result, buf[0], buf[1], buf[2], buf[3],
    buf[4], buf[5], buf[6]);

    - if (result >= 0) {
    - dt->tm_sec = BCD_TO_BIN(buf[0]);
    - dt->tm_min = BCD_TO_BIN(buf[1]);
    - val = buf[2] & 0x3f;
    - dt->tm_hour = BCD_TO_BIN(val);
    - dt->tm_wday = BCD_TO_BIN(buf[3]) - 1;
    - dt->tm_mday = BCD_TO_BIN(buf[4]);
    - val = buf[5] & 0x7f;
    - dt->tm_mon = BCD_TO_BIN(val);
    - dt->tm_year = 1900 + BCD_TO_BIN(buf[6]);
    + if (result < 0) {
    + dev_err(&client->dev, "error reading data! %d\n", result);
    + result = -EIO;
    + } else {
    + tm->tm_sec = BCD2BIN(buf[0]);
    + tm->tm_min = BCD2BIN(buf[1]);
    + tm->tm_hour = BCD2BIN(buf[2] & 0x3f);
    + tm->tm_wday = BCD2BIN(buf[3]);
    + tm->tm_mday = BCD2BIN(buf[4]);
    + tm->tm_mon = BCD2BIN(buf[5] & 0x7f) - 1;
    + tm->tm_year = BCD2BIN(buf[6]);
    if (buf[5] & 0x80)
    - dt->tm_year += 100;
    + tm->tm_year += 100;

    - dev_dbg(&client->adapter->dev, "%s: secs=%d, mins=%d, "
    - "hours=%d, mday=%d, mon=%d, year=%d, wday=%d\n",
    - __FUNCTION__, dt->tm_sec, dt->tm_min,
    - dt->tm_hour, dt->tm_mday,
    - dt->tm_mon, dt->tm_year, dt->tm_wday);
    - } else {
    - dev_err(&client->adapter->dev, "ds1337[%d]: error reading "
    - "data! %d\n", data->id, result);
    - result = -EIO;
    + dev_dbg(&client->dev, "%s: secs=%d, mins=%d, hours=%d, "
    + "mday=%d, mon=%d, year=%d, wday=%d\n", __FUNCTION__,
    + tm->tm_sec, tm->tm_min, tm->tm_hour,
    + tm->tm_mday, tm->tm_mon, tm->tm_year, tm->tm_wday);
    +
    + result = 0;
    }

    return result;
    }

    -static int ds1337_set_datetime(struct i2c_client *client, struct rtc_time *dt)
    +static int ds1337_set_datetime(struct i2c_client *client, struct rtc_time *tm)
    {
    - struct ds1337_data *data = i2c_get_clientdata(client);
    + unsigned char buf[8];
    int result;
    - u8 buf[8];
    - u8 val;
    - struct i2c_msg msg[1];
    -
    - if (!dt) {
    - dev_dbg(&client->adapter->dev, "%s: EINVAL: dt=NULL\n",
    - __FUNCTION__);

    - return -EINVAL;
    - }
    -
    - dev_dbg(&client->adapter->dev, "%s: secs=%d, mins=%d, hours=%d, "
    + dev_dbg(&client->dev, "%s: secs=%d, mins=%d, hours=%d, "
    "mday=%d, mon=%d, year=%d, wday=%d\n", __FUNCTION__,
    - dt->tm_sec, dt->tm_min, dt->tm_hour,
    - dt->tm_mday, dt->tm_mon, dt->tm_year, dt->tm_wday);
    + tm->tm_sec, tm->tm_min, tm->tm_hour,
    + tm->tm_mday, tm->tm_mon, tm->tm_year, tm->tm_wday);

    buf[0] = 0; /* reg offset */
    - buf[1] = BIN_TO_BCD(dt->tm_sec);
    - buf[2] = BIN_TO_BCD(dt->tm_min);
    - buf[3] = BIN_TO_BCD(dt->tm_hour) | (1 << 6);
    - buf[4] = BIN_TO_BCD(dt->tm_wday) + 1;
    - buf[5] = BIN_TO_BCD(dt->tm_mday);
    - buf[6] = BIN_TO_BCD(dt->tm_mon);
    - if (dt->tm_year >= 2000) {
    - val = dt->tm_year - 2000;
    + buf[1] = BIN2BCD(tm->tm_sec);
    + buf[2] = BIN2BCD(tm->tm_min);
    + buf[3] = BIN2BCD(tm->tm_hour) | (1 << 6);
    + buf[4] = BIN2BCD(tm->tm_wday);
    + buf[5] = BIN2BCD(tm->tm_mday);
    + buf[6] = BIN2BCD(tm->tm_mon + 1);
    + if (tm->tm_year >= 100) {
    + tm->tm_year -= 100;
    buf[6] |= (1 << 7);
    - } else {
    - val = dt->tm_year - 1900;
    }
    - buf[7] = BIN_TO_BCD(val);
    + buf[7] = BIN2BCD(tm->tm_year);

    - msg[0].addr = client->addr;
    - msg[0].flags = 0;
    - msg[0].len = sizeof(buf);
    - msg[0].buf = &buf[0];
    -
    - result = client->adapter->algo->master_xfer(client->adapter,
    - &msg[0], 1);
    + result = i2c_master_send(client, (char *)buf, sizeof(buf));
    if (result < 0) {
    - dev_err(&client->adapter->dev, "ds1337[%d]: error "
    - "writing data! %d\n", data->id, result);
    + dev_err(&client->dev, "error writing data! %d\n", result);
    result = -EIO;
    - } else {
    + } else
    result = 0;
    - }

    return result;
    }
    @@ -210,7 +170,7 @@
    static int ds1337_command(struct i2c_client *client, unsigned int cmd,
    void *arg)
    {
    - dev_dbg(&client->adapter->dev, "%s: cmd=%d\n", __FUNCTION__, cmd);
    + dev_dbg(&client->dev, "%s: cmd=%d\n", __FUNCTION__, cmd);

    switch (cmd) {
    case DS1337_GET_DATE:
    @@ -254,10 +214,10 @@
    */
    static int ds1337_detect(struct i2c_adapter *adapter, int address, int kind)
    {
    - struct i2c_client *new_client;
    + struct i2c_client *client;
    struct ds1337_data *data;
    + char *name;
    int err = 0;
    - const char *name = "";

    if (!i2c_check_functionality(adapter, I2C_FUNC_SMBUS_BYTE_DATA |
    I2C_FUNC_I2C))
    @@ -273,12 +233,12 @@
    /* The common I2C client data is placed right before the
    * DS1337-specific data.
    */
    - new_client = &data->client;
    - i2c_set_clientdata(new_client, data);
    - new_client->addr = address;
    - new_client->adapter = adapter;
    - new_client->driver = &ds1337_driver;
    - new_client->flags = 0;
    + client = &data->client;
    + i2c_set_clientdata(client, data);
    + client->addr = address;
    + client->adapter = adapter;
    + client->driver = &ds1337_driver;
    + client->flags = 0;

    /*
    * Now we do the remaining detection. A negative kind means that
    @@ -303,47 +263,52 @@
    u8 data;

    /* Check that status register bits 6-2 are zero */
    - if ((ds1337_read(new_client, DS1337_REG_STATUS, &data) < 0) ||
    + if ((ds1337_read(client, DS1337_REG_STATUS, &data) < 0) ||
    (data & 0x7c))
    goto exit_free;

    /* Check for a valid day register value */
    - if ((ds1337_read(new_client, DS1337_REG_DAY, &data) < 0) ||
    + if ((ds1337_read(client, DS1337_REG_DAY, &data) < 0) ||
    (data == 0) || (data & 0xf8))
    goto exit_free;

    /* Check for a valid date register value */
    - if ((ds1337_read(new_client, DS1337_REG_DATE, &data) < 0) ||
    + if ((ds1337_read(client, DS1337_REG_DATE, &data) < 0) ||
    (data == 0) || (data & 0xc0) || ((data & 0x0f) > 9) ||
    (data >= 0x32))
    goto exit_free;

    /* Check for a valid month register value */
    - if ((ds1337_read(new_client, DS1337_REG_MONTH, &data) < 0) ||
    + if ((ds1337_read(client, DS1337_REG_MONTH, &data) < 0) ||
    (data == 0) || (data & 0x60) || ((data & 0x0f) > 9) ||
    ((data >= 0x13) && (data <= 0x19)))
    goto exit_free;

    /* Check that control register bits 6-5 are zero */
    - if ((ds1337_read(new_client, DS1337_REG_CONTROL, &data) < 0) ||
    + if ((ds1337_read(client, DS1337_REG_CONTROL, &data) < 0) ||
    (data & 0x60))
    goto exit_free;

    kind = ds1337;
    }

    - if (kind == ds1337)
    + switch (kind) {
    + case ds1337:
    name = "ds1337";
    + break;
    + default:
    + name = "";
    + }

    /* We can fill in the remaining client fields */
    - strlcpy(new_client->name, name, I2C_NAME_SIZE);
    + strlcpy(client->name, name, I2C_NAME_SIZE);

    /* Tell the I2C layer a new client has arrived */
    - if ((err = i2c_attach_client(new_client)))
    + if ((err = i2c_attach_client(client)))
    goto exit_free;

    /* Initialize the DS1337 chip */
    - ds1337_init_client(new_client);
    + ds1337_init_client(client);

    /* Add client to local list */
    data->id = ds1337_id++;
    -
    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-07 13:32    [W:2.241 / U:0.028 seconds]
    ©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site