lkml.org 
[lkml]   [2007]   [Jun]   [19]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
Date
From
SubjectRe: [rtc-linux] [PATCH 1/2] rtc: add rtc-m41txx driver
On Wed, 20 Jun 2007 00:59:26 +0900 (JST)
Atsushi Nemoto <anemo@mba.ocn.ne.jp> wrote:

>
> This driver supports M41T80, M41T81 and M41ST85. The old m41t00
> driver supports M41T00, M41T81 and M41T85(M41ST85). While the M41T00
> chip is now supported by rtc-ds1307 driver, this driver does not
> include support for the chip.

Hi,

I'd prefer if you change the name in rtc-m41t80 and add
the names of the supported chips in the Kconfig entry.

> +/* Sets the given date and time to the real time clock. */
> +static int m41txx_set_datetime(struct i2c_client *client, struct rtc_time *tm)
> +{
> + unsigned char wbuf[1 + M41TXX_REG_YEAR + 1];

would be easier to understand if you have some #defines
with the sizes you require for the various buffers.


> +#if defined(CONFIG_RTC_INTF_PROC) || defined(CONFIG_RTC_INTF_PROC_MODULE)
> +static int m41txx_rtc_proc(struct device *dev, struct seq_file *seq)
> +{
> + struct i2c_client *client = to_i2c_client(dev);
> + struct m41txx_data *clientdata = i2c_get_clientdata(client);
> + unsigned char reg;
> +
> + if (clientdata->chip->features & M41TXX_FEATURE_BL) {
> + reg = i2c_smbus_read_byte_data(client, M41TXX_REG_FLAGS);
> + seq_printf(seq, "battery\t\t: %s\n",
> + (reg & M41TXX_FLAGS_BATT_LOW) ? "exhausted" : "ok");
> + }
> + return 0;
> +}

the proc interface will go away sooner or later, would be better to expose
the battery status with a sysfs attribute.


> +++ b/include/linux/m41txx.h
> @@ -0,0 +1,40 @@
> +/*
> + * Definitions for the ST M41TXX family of i2c rtc chips.
> + *
> + * Based on m41t00.h by Mark A. Greer <mgreer@mvista.com>
> + *
> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License version 2 as
> + * published by the Free Software Foundation.

Unless this include is required by other files, please place
it in drivers/rtc calling it rtc-drivername.h

Other than the comments above, the driver
seems fine to me.

--

Best regards,

Alessandro Zummo,
Tower Technologies - Torino, Italy

http://www.towertech.it

-
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: 2007-06-20 00:31    [W:1.853 / U:0.060 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site