Re: [rtc-linux] [PATCH 1/2] rtc: add rtc-m41txx driver

[Alessandro Zummo]

On Wed, 20 Jun 2007 00:59:26 +0900 (JST)
Atsushi Nemoto <anemo@xxxxxxxxxxxxx> 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@xxxxxxxxxx>
> + *
> + * 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@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/