[lm-sensors] [PATCH] X1205 cleanup

[lm-sensors] [PATCH] X1205 cleanup

[Alessandro Zummo]

Content-Disposition: inline; filename=i2c-x1205-cleanup.patch

Removes unused functions and data structures.

Signed-off-by: Alessandro Zummo <a.zummo at towertech.it>
---
 drivers/i2c/chips/Kconfig |    5 +
 drivers/i2c/chips/x1205.c |  131 ++++------------------------------------------
 include/linux/x1205.h     |   31 ----------
 3 files changed, 15 insertions(+), 152 deletions(-)

--- linux-nslu2.orig/drivers/i2c/chips/x1205.c	2005-12-13 21:36:26.000000000 +0100
+++ linux-nslu2/drivers/i2c/chips/x1205.c	2005-12-13 21:36:34.000000000 +0100
@@ -1,5 +1,5 @@
 /*
- *  x1205.c - An i2c driver for the Xicor X1205 RTC
+ *  x1205.c - An i2c driver for the Xicor/Intersil X1205 RTC
  *  Copyright 2004 Karen Spearel
  *  Copyright 2005 Alessandro Zummo
  *
@@ -22,11 +22,9 @@
 #include <linux/string.h>
 #include <linux/bcd.h>
 #include <linux/rtc.h>
-#include <linux/list.h>
 
-#include <linux/x1205.h>
 
-#define DRV_VERSION "0.9.9"
+#define DRV_VERSION "1.0.0"
 
 /* Addresses to scan: none. This chip is located at
  * 0x6f and uses a two bytes register addressing.
@@ -101,8 +99,6 @@ I2C_CLIENT_MODULE_PARM(hctosys,
 static int x1205_attach(struct i2c_adapter *adapter);
 static int x1205_detach(struct i2c_client *client);
 static int x1205_probe(struct i2c_adapter *adapter, int address, int kind);
-static int x1205_command(struct i2c_client *client, unsigned int cmd,
-	void *arg);
 
 static struct i2c_driver x1205_driver = {
 	.owner		= THIS_MODULE,
@@ -112,35 +108,9 @@ static struct i2c_driver x1205_driver = 
 	.detach_client	= &x1205_detach,
 };
 
-struct x1205_data {
-	struct i2c_client client;
-	struct list_head list;
-	unsigned int epoch;
-};
-
 static const unsigned char days_in_mo[]  	{ 31, 28, 31, 30, 31, 30, 31, 31, 30, 31, 30, 31 };
 
-static LIST_HEAD(x1205_clients);
-
-/* Workaround until the I2C subsytem will allow to send
- * commands to a specific client. This function will send the command
- * to the first client.
- */
-int x1205_do_command(unsigned int cmd, void *arg)
-{
-	struct list_head *walk;
-	struct list_head *tmp;
-	struct x1205_data *data;
-
-	list_for_each_safe(walk, tmp, &x1205_clients) {
-		data = list_entry(walk, struct x1205_data, list);
-		return x1205_command(&data->client, cmd, arg);
-	}
-
-	return -ENODEV;
-}
-
 #define is_leap(year) \
 	((year) % 4 = 0 && ((year) % 100 != 0 || (year) % 400 = 0))
 
@@ -185,8 +155,6 @@ static int x1205_get_datetime(struct i2c
 		{ client->addr, I2C_M_RD, 8, buf },	/* read date */
 	};
 
-	struct x1205_data *data = i2c_get_clientdata(client);
-
 	/* read status register */
 	if ((i2c_transfer(client->adapter, &msgs[0], 2)) != 2) {
 		dev_err(&client->dev, "%s: read error\n", __FUNCTION__);
@@ -218,8 +186,8 @@ static int x1205_get_datetime(struct i2c
 	tm->tm_hour = BCD2BIN(buf[CCR_HOUR] & 0x3F); /* hr is 0-23 */
 	tm->tm_mday = BCD2BIN(buf[CCR_MDAY]);
 	tm->tm_mon = BCD2BIN(buf[CCR_MONTH]);
-	data->epoch = BCD2BIN(buf[CCR_Y2K]) * 100;
-	tm->tm_year = BCD2BIN(buf[CCR_YEAR]) + data->epoch - 1900;
+	tm->tm_year = BCD2BIN(buf[CCR_YEAR])
+			+ (BCD2BIN(buf[CCR_Y2K]) * 100) - 1900;
 	tm->tm_wday = buf[CCR_WDAY];
 
 	dev_dbg(&client->dev, "%s: tm is secs=%d, mins=%d, hours=%d, "
@@ -246,8 +214,6 @@ static int x1205_set_datetime(struct i2c
 
 	static const unsigned char diswe[3] = { 0, X1205_REG_SR, 0 };
 
-	struct x1205_data *data = i2c_get_clientdata(client);
-
 	/* check if all values in the tm struct are correct */
 	if ((err = x1205_validate_tm(tm)) < 0)
 		return err;
@@ -271,10 +237,10 @@ static int x1205_set_datetime(struct i2c
 		/* month, 0 - 11 */
 		buf[CCR_MONTH] = BIN2BCD(tm->tm_mon);
 
-		/* year, since 1900 */
-		buf[CCR_YEAR] = BIN2BCD(tm->tm_year + 1900 - data->epoch);
+		/* year, since the rtc epoch*/
+		buf[CCR_YEAR] = BIN2BCD(tm->tm_year % 100);
 		buf[CCR_WDAY] = tm->tm_wday & 0x07;
-		buf[CCR_Y2K] = BIN2BCD(data->epoch / 100);
+		buf[CCR_Y2K] = BIN2BCD(tm->tm_year / 100);
 	}
 
 	/* this sequence is required to unlock the chip */
@@ -387,8 +353,8 @@ static int x1205_hctosys(struct i2c_clie
 	struct rtc_time tm;
 	struct timespec tv;
 
-	err = x1205_command(client, X1205_CMD_GETDATETIME, &tm);
 
+	err = x1205_get_datetime(client, &tm, X1205_CCR_BASE);
 	if (err) {
 		dev_err(&client->dev,
 			"Unable to set the system clock\n");
@@ -538,28 +504,9 @@ static int x1205_attach(struct i2c_adapt
 	return i2c_probe(adapter, &addr_data, x1205_probe);
 }
 
-int x1205_direct_attach(int adapter_id,
-	struct i2c_client_address_data *address_data)
-{
-	int err;
-	struct i2c_adapter *adapter = i2c_get_adapter(adapter_id);
-
-	if (adapter) {
-		err = i2c_probe(adapter,
-			address_data, x1205_probe);
-
-		i2c_put_adapter(adapter);
-
-		return err;
-	}
-
-	return -ENODEV;
-}
-
 static int x1205_probe(struct i2c_adapter *adapter, int address, int kind)
 {
 	struct i2c_client *client;
-	struct x1205_data *data;
 
 	int err = 0;
 
@@ -570,23 +517,18 @@ static int x1205_probe(struct i2c_adapte
 		goto exit;
 	}
 
-	if (!(data = kzalloc(sizeof(struct x1205_data), GFP_KERNEL))) {
+	if (!(client = kzalloc(sizeof(struct i2c_client), GFP_KERNEL))) {
 		err = -ENOMEM;
 		goto exit;
 	}
 
 	/* Initialize our structures */
-	data->epoch = 2000;
-
-	client = &data->client;
 	client->addr = address;
 	client->driver = &x1205_driver;
 	client->adapter	= adapter;
 
 	strlcpy(client->name, "x1205", I2C_NAME_SIZE);
 
-	i2c_set_clientdata(client, data);
-
 	/* Verify the chip is really an X1205 */
 	if (kind < 0) {
 		if (x1205_validate_client(client) < 0) {
@@ -599,8 +541,6 @@ static int x1205_probe(struct i2c_adapte
 	if ((err = i2c_attach_client(client)))
 		goto exit_kfree;
 
-	list_add(&data->list, &x1205_clients);
-
 	dev_info(&client->dev, "chip found, driver version " DRV_VERSION "\n");
 
 	/* If requested, set the system time */
@@ -610,7 +550,7 @@ static int x1205_probe(struct i2c_adapte
 	return 0;
 
 exit_kfree:
-	kfree(data);
+	kfree(client);
 
 exit:
 	return err;
@@ -619,61 +559,17 @@ exit:
 static int x1205_detach(struct i2c_client *client)
 {
 	int err;
-	struct x1205_data *data = i2c_get_clientdata(client);
 
 	dev_dbg(&client->dev, "%s\n", __FUNCTION__);
 
 	if ((err = i2c_detach_client(client)))
 		return err;
 
-	list_del(&data->list);
-
-	kfree(data);
+	kfree(client);
 
 	return 0;
 }
 
-static int x1205_command(struct i2c_client *client, unsigned int cmd,
-	void *param)
-{
-	if (param = NULL)
-		return -EINVAL;
-
-	if (!capable(CAP_SYS_TIME))
-		return -EACCES;
-
-	dev_dbg(&client->dev, "%s: cmd=%d\n", __FUNCTION__, cmd);
-
-	switch (cmd) {
-	case X1205_CMD_GETDATETIME:
-		return x1205_get_datetime(client, param, X1205_CCR_BASE);
-
-	case X1205_CMD_SETTIME:
-		return x1205_set_datetime(client, param, 0,
-				X1205_CCR_BASE);
-
-	case X1205_CMD_SETDATETIME:
-		return x1205_set_datetime(client, param, 1,
-				X1205_CCR_BASE);
-
-	case X1205_CMD_GETALARM:
-		return x1205_get_datetime(client, param, X1205_ALM0_BASE);
-
-	case X1205_CMD_SETALARM:
-		return x1205_set_datetime(client, param, 1,
-				X1205_ALM0_BASE);
-
-	case X1205_CMD_GETDTRIM:
-		return x1205_get_dtrim(client, param);
-
-	case X1205_CMD_GETATRIM:
-		return x1205_get_atrim(client, param);
-
-	default:
-		return -EINVAL;
-	}
-}
-
 static int __init x1205_init(void)
 {
 	return i2c_add_driver(&x1205_driver);
@@ -687,12 +583,9 @@ static void __exit x1205_exit(void)
 MODULE_AUTHOR(
 	"Karen Spearel <kas11 at tampabay.rr.com>, "
 	"Alessandro Zummo <a.zummo at towertech.it>");
-MODULE_DESCRIPTION("Xicor X1205 RTC driver");
+MODULE_DESCRIPTION("Xicor/Intersil X1205 RTC driver");
 MODULE_LICENSE("GPL");
 MODULE_VERSION(DRV_VERSION);
 
-EXPORT_SYMBOL_GPL(x1205_do_command);
-EXPORT_SYMBOL_GPL(x1205_direct_attach);
-
 module_init(x1205_init);
 module_exit(x1205_exit);
--- linux-nslu2.orig/include/linux/x1205.h	2005-12-13 21:36:26.000000000 +0100
+++ /dev/null	1970-01-01 00:00:00.000000000 +0000
@@ -1,31 +0,0 @@
-/*
- *  x1205.h - defines for drivers/i2c/chips/x1205.c
- *  Copyright 2004 Karen Spearel
- *  Copyright 2005 Alessandro Zummo
- *
- *  This program is free software; you can redistribute it and/or modify
- *  it under the terms of the GNU General Public License as published by
- *  the Free Software Foundation; either version 2 of the License, or
- *  (at your option) any later version.
- */
-
-#ifndef __LINUX_X1205_H__
-#define __LINUX_X1205_H__
-
-/* commands */
-
-#define X1205_CMD_GETDATETIME	0
-#define X1205_CMD_SETTIME	1
-#define X1205_CMD_SETDATETIME	2
-#define X1205_CMD_GETALARM	3
-#define X1205_CMD_SETALARM	4
-#define X1205_CMD_GETDTRIM	5
-#define X1205_CMD_SETDTRIM	6
-#define X1205_CMD_GETATRIM	7
-#define X1205_CMD_SETATRIM	8
-
-extern int x1205_do_command(unsigned int cmd, void *arg);
-extern int x1205_direct_attach(int adapter_id,
-	struct i2c_client_address_data *address_data);
-
-#endif /* __LINUX_X1205_H__ */
--- linux-nslu2.orig/drivers/i2c/chips/Kconfig	2005-12-13 21:36:26.000000000 +0100
+++ linux-nslu2/drivers/i2c/chips/Kconfig	2005-12-13 21:38:34.000000000 +0100
@@ -127,10 +127,11 @@ config SENSORS_MAX6875
 	  will be called max6875.
 
 config RTC_X1205_I2C
-	tristate "Xicor X1205 RTC chip"
+	tristate "Xicor/Intersil X1205 RTC chip"
 	depends on I2C && EXPERIMENTAL
 	help
-	  If you say yes here you get support for the Xicor X1205 RTC chip.
+	  If you say yes here you get support for the
+	  Xicor/Intersil X1205 RTC chip.
 
 	  This driver can also be built as a module. If so, the module
 	  will be called x1205.

[lm-sensors] [PATCH] X1205 cleanup

[Jean Delvare]

Hi Alessandro,

Sorry for the late reply. I've been busy, and then had a holiday break.

> Removes unused functions and data structures.

Applying this patch adds the following warnings:

  CC [M]  drivers/i2c/chips/x1205.o
drivers/i2c/chips/x1205.c:204: warning: `x1205_set_datetime' defined but not used
drivers/i2c/chips/x1205.c:284: warning: `x1205_get_dtrim' defined but not used
drivers/i2c/chips/x1205.c:316: warning: `x1205_get_atrim' defined but not used

What is the point of your cleanup? As I understand it, the driver is no
more exporting any functionality to the rest of the kernel. As it has no
user-space interface either, this means that the only thing the driver
is useful for is running x1205_hctosys at runtime - and even this only
happens if the module parameter hctosys has been set, which isn't the
default.

So you're making your own driver useless? There must be something I am
missing, please explain.

Comments about the code itself:

> +		/* year, since the rtc epoch*/

Missing space before end of comment.

> +		buf[CCR_YEAR] = BIN2BCD(tm->tm_year % 100);
>  		buf[CCR_WDAY] = tm->tm_wday & 0x07;
> -		buf[CCR_Y2K] = BIN2BCD(data->epoch / 100);
> +		buf[CCR_Y2K] = BIN2BCD(tm->tm_year / 100);
>  	}

Isn't this actually changing what the code does? If so, this isn't
suitable for a cleanup patch and should be moved to a separate patch
with proper explanations.

> @@ -387,8 +353,8 @@ static int x1205_hctosys(struct i2c_clie
>  	struct rtc_time tm;
>  	struct timespec tv;
>  
> -	err = x1205_command(client, X1205_CMD_GETDATETIME, &tm);
>  
> +	err = x1205_get_datetime(client, &tm, X1205_CCR_BASE);
>  	if (err) {
>  		dev_err(&client->dev,
>  			"Unable to set the system clock\n");

This change will lead to two consecutive blank lines, which should be
avoided.

> --- linux-nslu2.orig/drivers/i2c/chips/Kconfig	2005-12-13 21:36:26.000000000 +0100
> +++ linux-nslu2/drivers/i2c/chips/Kconfig	2005-12-13 21:38:34.000000000 +0100
> @@ -127,10 +127,11 @@ config SENSORS_MAX6875
>  	  will be called max6875.
>  
>  config RTC_X1205_I2C
> -	tristate "Xicor X1205 RTC chip"
> +	tristate "Xicor/Intersil X1205 RTC chip"
>  	depends on I2C && EXPERIMENTAL
>  	help
> -	  If you say yes here you get support for the Xicor X1205 RTC chip.
> +	  If you say yes here you get support for the
> +	  Xicor/Intersil X1205 RTC chip.

"Xicor/Intelsil X1205" would easily fit at the end of the previous line.

Thanks,
-- 
Jean Delvare

[lm-sensors] [PATCH] X1205 cleanup

[Alessandro Zummo]

On Tue, 27 Dec 2005 12:13:56 +0100
Jean Delvare <khali at linux-fr.org> wrote:

> Applying this patch adds the following warnings:
> 
>   CC [M]  drivers/i2c/chips/x1205.o
> drivers/i2c/chips/x1205.c:204: warning: `x1205_set_datetime' defined but not used
> drivers/i2c/chips/x1205.c:284: warning: `x1205_get_dtrim' defined but not used
> drivers/i2c/chips/x1205.c:316: warning: `x1205_get_atrim' defined but not used
> 
> What is the point of your cleanup? As I understand it, the driver is no
> more exporting any functionality to the rest of the kernel. As it has no
> user-space interface either, this means that the only thing the driver
> is useful for is running x1205_hctosys at runtime - and even this only
> happens if the module parameter hctosys has been set, which isn't the
> default.

 This cleanup was in preparation for the new rtc subsystem I'm writing,
 so I thought it was better to leave those functions there.

> Missing space before end of comment.
> 
> > +		buf[CCR_YEAR] = BIN2BCD(tm->tm_year % 100);
> >  		buf[CCR_WDAY] = tm->tm_wday & 0x07;
> > -		buf[CCR_Y2K] = BIN2BCD(data->epoch / 100);
> > +		buf[CCR_Y2K] = BIN2BCD(tm->tm_year / 100);
> >  	}
> 
> Isn't this actually changing what the code does? If so, this isn't
> suitable for a cleanup patch and should be moved to a separate patch
> with proper explanations.

 you're right.. I'll fix it asap. Thanks!


-- 

 Best regards,

 Alessandro Zummo,
  Tower Technologies - Turin, Italy

  http://www.towertech.it

[lm-sensors] [PATCH] X1205 cleanup

[Jean Delvare]

Hi Alessandro,

> > What is the point of your cleanup? As I understand it, the driver is no
> > more exporting any functionality to the rest of the kernel. As it has no
> > user-space interface either, this means that the only thing the driver
> > is useful for is running x1205_hctosys at runtime - and even this only
> > happens if the module parameter hctosys has been set, which isn't the
> > default.
> 
> This cleanup was in preparation for the new rtc subsystem I'm writing,
> so I thought it was better to leave those functions there.

In that case, you are doing things in the wrong order. You should first
introduce your new RTC subsystem (which I think is a good idea,
although I lack time to actually look into your implementation), then
convert your driver directly to use it. Having an intermediate state
where your driver is mostly useless doesn't make much sense.

Thanks,
-- 
Jean Delvare