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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-Id: <20070620.111616.15246884.nemoto@toshiba-tops.co.jp>
Date:	Wed, 20 Jun 2007 11:16:16 +0900 (JST)
From:	Atsushi Nemoto <anemo@....ocn.ne.jp>
To:	alessandro.zummo@...ertech.it
Cc:	rtc-linux@...glegroups.com, linux-kernel@...r.kernel.org,
	ab@...able.de, mgreer@...sta.com, khali@...ux-fr.org,
	david-b@...bell.net
Subject: Re: [rtc-linux] [PATCH 1/2] rtc: add rtc-m41txx driver

On Wed, 20 Jun 2007 00:28:28 +0200, Alessandro Zummo <alessandro.zummo@...ertech.it> wrote:
>   I'd prefer if you change the name in rtc-m41t80 and add
>  the names of the supported chips in the Kconfig entry.

OK, I will do.  I thought M41ST94 also can be supported but that was
wrong.  From datasheets, the driver can be used for M41T8[0123],
M41ST8[457] chips.

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

I will do.

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

I will do.

> > +++ 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@...sta.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

It is required by platform code to provide SQW setting.  Also
M41TXX_DRV_NAME and M41TXX_I2C_ADDR might be useful to declare
i2c_board_info, which would be something like:

	static struct m41txx_platform_data pdata = {
		.sqw_freq = M41TXX_SQW_32KHZ,
	};
	static struct i2c_board_info binfo __initdata = {
		I2C_BOARD_INFO(M41TXX_DRV_NAME, M41TXX_I2C_ADDR),
		.type = "m41t80",
		.platform_data = &pdata,
	};
	return i2c_register_board_info(0, &binfo, 1);

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

Thank you for review.
---
Atsushi Nemoto
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/