Re: [PATCH 2.6.16-mm1 3/3] rtc: add support for m41t81 & m41t85 chips to m41t00 driver

[Jean Delvare <khali@linux-fr.org>]

Hi Mark,

Some more comments, sorry for being slow.

>  drivers/i2c/chips/m41t00.c |  294 ++++++++++++++++++++++++++++++++++-----------
>  include/linux/m41t00.h     |   50 +++++++

Who else is going to use this header file? It's rather rare to have a
hardware-specific header file under include/linux.

> diff -Nurp linux-2.6.16-mm1-cleanup/drivers/i2c/chips/m41t00.c linux-2.6.16-mm1-newsupp/drivers/i2c/chips/m41t00.c
> --- linux-2.6.16-mm1-cleanup/drivers/i2c/chips/m41t00.c	2006-03-27 16:00:35.000000000 -0700
> +++ linux-2.6.16-mm1-newsupp/drivers/i2c/chips/m41t00.c	2006-03-27 16:49:35.000000000 -0700
> (...)
>  static unsigned short ignore[] = { I2C_CLIENT_END };
> -static unsigned short normal_addr[] = { 0x68, I2C_CLIENT_END };
> +static unsigned short normal_addr[] = { 0, I2C_CLIENT_END };

Bad idea. If no platform data is found, the driver will attempt to
probe I2C address 0, which is a broadcast address. I can predict
interesting results ;)

So, if you don't want the driver to do anything in the absence of
platform data, you should define normal_addr the following way:

static unsigned short normal_addr[] = { I2C_CLIENT_END, I2C_CLIENT_END };

> +struct m41t00_chip_info {
> +	u16	type;
> +	u16	read_limit;
> +	u8	sec;		/* Offsets for chip regs */
> +	u8	min;
> +	u8	hour;
> +	u8	day;
> +	u8	mon;
> +	u8	year;
> +	u8	alarm_mon;
> +	u8	alarm_hour;
> +	u8	sqw;
> +	u32	sqw_freq;
> +};

u16 is probably overkill for .type and .read_limit.

> +static struct m41t00_chip_info m41t00_chip_info_tbl[] = {
> +	{ M41T00_TYPE_M41T00, 5, 0, 1, 2, 4, 5, 6, 0, 0, 0, 0 },
> +	{ M41T00_TYPE_M41T81, 1, 1, 2, 3, 5, 6, 7, 0xa, 0xc, 0x13, 0 },
> +	{ M41T00_TYPE_M41T85, 1, 1, 2, 3, 5, 6, 7, 0xa, 0xc, 0x13, 0 },
> +};

C99-style initialization, please? It'll take much more lines, granted,
but is also way more robust to future changes, and will be more
readable too.

May I ask why you define separate types for the M41T81 and the M41T85,
when you handle both exactly the same way?

> +		sec = buf[m41t00_chip->sec] & 0x7f;
> +		min = buf[m41t00_chip->min] & 0x7f;
> +		hour = buf[m41t00_chip->hour] & 0x3f;
> +		day = buf[m41t00_chip->day] & 0x3f;
> +		mon = buf[m41t00_chip->mon] & 0x1f;
> +		year = buf[m41t00_chip->year] & 0xff;

The year masking is a no-op, you could omit it.

> @@ -169,24 +246,100 @@ m41t00_probe(struct i2c_adapter *adap, i
>  {
>  	struct i2c_client *client;
>  	int rc;
> +	u8 rbuf[1], wbuf[2], msgbuf[1] = { 0 }; /* offset into rtc's regs */
> +	struct i2c_msg msgs[] = {
> +		{
> +			.addr	= 0,
> +			.flags	= 0,
> +			.len	= 1,
> +			.buf	= msgbuf,
> +		},
> +		{
> +			.addr	= 0,
> +			.flags	= I2C_M_RD,
> +			.len	= 1,
> +			.buf	= rbuf,
> +		},
> +	};

Why don't you initialize both .addr right away?

>  
>  	client = kzalloc(sizeof(struct i2c_client), GFP_KERNEL);
>  	if (!client)
>  		return -ENOMEM;
>  
> -	strlcpy(client->name, M41T00_DRV_NAME, I2C_NAME_SIZE);
> +	strlcpy(client->name, m41t00_driver.driver.name, I2C_NAME_SIZE);

The driver name may not be suitable here, the client name should
instead reflect what chip it is.

> +	if (m41t00_chip->type != M41T00_TYPE_M41T00) {
> +		/* If asked, set SQW frequency & enable */
> +		if (m41t00_chip->sqw_freq) {
> +			msgbuf[0] = m41t00_chip->alarm_mon;
> +			if ((rc = i2c_transfer(client->adapter, msgs, 2)) < 0)
> +				goto sqw_err;

You did not check whether the adapter you are attaching to supports this
kind of transaction! You need to call i2c_check_functionality, see how
other i2c chip drivers (for example ds1337) are doing. For i2c_transfer
and i2c_master_send, you want to check for I2C_FUNC_I2C.

> +
> +			wbuf[0] = m41t00_chip->alarm_mon;
> +			wbuf[1] = rbuf[0] & ~0x40;
> +			if ((rc = i2c_master_send(client, wbuf, 2)) < 0)
> +				goto sqw_err;
> +
> +			wbuf[0] = m41t00_chip->sqw;
> +			wbuf[1] = m41t00_chip->sqw_freq;
> +			if ((rc = i2c_master_send(client, wbuf, 2)) < 0)
> +				goto sqw_err;
> +
> +			wbuf[0] = m41t00_chip->alarm_mon;
> +			wbuf[1] = rbuf[0] | 0x40;
> +			if ((rc = i2c_master_send(client, wbuf, 2)) < 0)
> +				goto sqw_err;
> +		}
> +
> +		/* Make sure HT (Halt Update) bit is cleared */
> +		msgbuf[0] = m41t00_chip->alarm_hour;
> +		if ((rc = i2c_transfer(client->adapter, msgs, 2)) < 0)
> +			goto ht_err;
> +
> +		if (rbuf[0] & 0x40) {
> +			wbuf[0] = m41t00_chip->alarm_hour;
> +			wbuf[1] = rbuf[0] & ~0x40;
> +			if ((rc = i2c_master_send(client, wbuf, 2)) < 0)
> +				goto ht_err;
> +		}
> +	}
> +
> +	/* Make sure ST (stop) bit is cleared */
> +	msgbuf[0] = m41t00_chip->sec;
> +	if ((rc = i2c_transfer(client->adapter, msgs, 2)) < 0)
> +		goto st_err;
> +
> +	if (rbuf[0] & 0x80) {
> +		wbuf[0] = m41t00_chip->sec;
> +		wbuf[1] = rbuf[0] & ~0x80;
> +		if ((rc = i2c_master_send(client, wbuf, 2)) < 0)
> +			goto st_err;
>  	}

What you do here are basically SMBus Read Byte and SMBus Write Byte
transactions. The code would be much more simple if you were using the
i2c_smbus_read_byte_data and i2c_smbus_write_byte_data functions, which
take care of all the technical details.

> +st_err:
> +	dev_err(&client->dev, "m41t00_probe: Can't clear ST bit\n");
> +	goto attach_err;
> +ht_err:
> +	dev_err(&client->dev, "m41t00_probe: Can't clear HT bit\n");
> +	goto attach_err;
> +sqw_err:
> +	dev_err(&client->dev, "m41t00_probe: Can't set SQW Frequency\n");
> +attach_err:
> +	kfree(client);
> +	return rc;
>  }

Mrghh, this isn't exactly elegant... You should be able to make it
better if you switch to i2c_smbus_read/write_byte_data as suggested.

> diff -Nurp linux-2.6.16-mm1-cleanup/include/linux/m41t00.h linux-2.6.16-mm1-newsupp/include/linux/m41t00.h
> --- linux-2.6.16-mm1-cleanup/include/linux/m41t00.h	1969-12-31 17:00:00.000000000 -0700
> +++ linux-2.6.16-mm1-newsupp/include/linux/m41t00.h	2006-03-27 16:25:51.000000000 -0700
> @@ -0,0 +1,50 @@
> +/*
> + * Definitions for the ST M41T00 family of i2c rtc chips.
> + *
> + * Author: Mark A. Greer <mgreer@mvista.com>
> + *
> + * 2005 (c) MontaVista Software, Inc. This file is licensed under

We're in year 2006 now :)

> + * the terms of the GNU General Public License version 2. This program
> + * is licensed "as is" without any warranty of any kind, whether express
> + * or implied.
> + */
> +
> +#ifndef _M41T00_H
> +#define _M41T00_H
> +
> +#define	M41T00_DRV_NAME		"m41t00"

Why do you need to export this?

> +#define	M41T00_I2C_ADDR		0x68

Not used anywhere?

> +#define	M41T00_TYPE_M41T00	0
> +#define	M41T00_TYPE_M41T81	81
> +#define	M41T00_TYPE_M41T85	85

The i2c core has a facility for drivers supporting several types of
devices. Check for I2C_CLIENT_INSMOD_3() in your case. The advantage if
you go that way is that chip types can be set from userspace through
module parameters, which may be convenient for testing when adding
support for new platforms.

> +/* SQW output disabled, this is default value by power on*/
> +#define SQW_FREQ_DISABLE	(0)
> +
> +#define SQW_FREQ_32KHZ	(1<<4)		/* 32.768 KHz */
> +#define SQW_FREQ_8KHZ	(2<<4)		/* 8.192 KHz */
> +#define SQW_FREQ_4KHZ	(3<<4)		/* 4.096 KHz */
> +#define SQW_FREQ_2KHZ	(4<<4)		/* 2.048 KHz */
> +#define SQW_FREQ_1KHZ	(5<<4)		/* 1.024 KHz */
> +#define SQW_FREQ_512HZ	(6<<4)		/* 512 Hz */
> +#define SQW_FREQ_256HZ	(7<<4)		/* 256 Hz */
> +#define SQW_FREQ_128HZ	(8<<4)		/* 128 Hz */
> +#define SQW_FREQ_64HZ	(9<<4)		/* 64 Hz */
> +#define SQW_FREQ_32HZ	(10<<4)		/* 32 Hz */
> +#define SQW_FREQ_16HZ	(11<<4)		/* 16 Hz */
> +#define SQW_FREQ_8HZ	(12<<4)		/* 8 Hz */
> +#define SQW_FREQ_4HZ	(13<<4)		/* 4 Hz */
> +#define SQW_FREQ_2HZ	(14<<4)		/* 2 Hz */
> +#define SQW_FREQ_1HZ	(15<<4)		/* 1 Hz */

Not used anywhere either?

> +extern ulong m41t00_get_rtc_time(void);
> +extern int m41t00_set_rtc_time(ulong nowtime);

Hopefully you won't have to export these anymore after you move to the
new RTC subsystem. But that's a different story. In the meantime,
shouldn't you have EXPORT_SYMBOL_GPL() for these functions? Else
compiling m41t00 as a module won't work.

Thanks,
-- 
Jean Delvare
-
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/