Re: [PATCH 2.6.39-rc7] RTC: rtc-ds1307 EEPROM support for ds1388 chip

[Andrew Morton <akpm@linux-foundation.org>]

On Tue, 14 Jun 2011 16:14:57 +1200
Austin Boyle <Austin.Boyle@aviatnet.com> wrote:

> On Mon, 2011-05-23 at 17:18 +1200, Austin Boyle wrote:
> > This is my patch which extends the NVRAM access functions in the DS1307
> > driver to support the 512 bytes of EEPROM on the DS1388 chip. Could you
> > please review the patch? If there are any problems with the way I have
> > implemented this or suggested improvements I will gladly modify. Thanks!
> 
> This is an updated version of the patch which prints the correct NVRAM
> size at init. Could someone please give me some feedback?
> 
> [PATCH 3.0-rc3] RTC: rtc-ds1307: EEPROM support for ds1388 chip
> 
> This patch adds support for different sized NVRAM, such as
> the 512 bytes of EEPROM on the DS1388 chip.

The patch is wordwrapped - please fix your email client.

The patch makes rather a mess of a decent-looking driver.  Please use
scripts/checkpatch.pl and review its report.

>
> ...
>
> @@ -540,18 +545,39 @@ ds1307_nvram_read(struct file *filp, str
>  	struct i2c_client	*client;
>  	struct ds1307		*ds1307;
>  	int			result;
> +	unsigned int		addr = 0;
>  
>  	client = kobj_to_i2c_client(kobj);
>  	ds1307 = i2c_get_clientdata(client);
>  
> -	if (unlikely(off >= NVRAM_SIZE))
> +	if (unlikely(off >= ds1307->nvram_size))
>  		return 0;
> -	if ((off + count) > NVRAM_SIZE)
> -		count = NVRAM_SIZE - off;
> +	if ((off + count) > ds1307->nvram_size)
> +		count = ds1307->nvram_size - off;
>  	if (unlikely(!count))
>  		return count;
>  
> -	result = ds1307->read_block_data(client, 8 + off, count, buf);
> +	if (ds1307->nvram_offset < 0xff) {	/* ds1307, ds1338 */
> +		result = ds1307->read_block_data(client, ds1307->nvram_offset + off,
> count, buf);
> +	} else	{	/* ds1388 */
> +		if ((off <= 0x00ff) && (off + count > 0x00ff))	{	/* read spans two
> blocks */
> +			client = ds1307->client[1];		/* first block */
> +			addr = (unsigned int)off;
> +			result = ds1307->read_block_data(client, addr, (0x100-addr), buf);

Ignores a possible error return value?

> +			client = ds1307->client[2];		/* second block */
> +			result += ds1307->read_block_data(client, 0x00, (count+addr-0x100),
> (buf+0x100-addr));
> +
> +		} else { 	/* read spans one blocks */
> +			if (off <= 0x00ff)	{
> +				client = ds1307->client[1];
> +				addr = (unsigned int)off;
> +			} else if (off <= 0x0200)	{
> +				client = ds1307->client[2];
> +				addr = (unsigned int)off - 0x100;
> +			}
> +			result = ds1307->read_block_data(client, addr, count, buf);
> +		}
> +	}
>  	if (result < 0)
>  		dev_err(&client->dev, "%s error %d\n", "nvram read", result);
>  	return result;
> @@ -565,18 +591,40 @@ ds1307_nvram_write(struct file *filp, st
>  	struct i2c_client	*client;
>  	struct ds1307		*ds1307;
>  	int			result;
> +	unsigned int		addr = 0;
>  
>  	client = kobj_to_i2c_client(kobj);
>  	ds1307 = i2c_get_clientdata(client);
>  
> -	if (unlikely(off >= NVRAM_SIZE))
> +	if (unlikely(off >= ds1307->nvram_size))
>  		return -EFBIG;
> -	if ((off + count) > NVRAM_SIZE)
> -		count = NVRAM_SIZE - off;
> +	if ((off + count) > ds1307->nvram_size)
> +		count = ds1307->nvram_size - off;
>  	if (unlikely(!count))
>  		return count;
>  
> -	result = ds1307->write_block_data(client, 8 + off, count, buf);
> +	if (ds1307->nvram_offset < 0xff) {	/* ds1307, ds1338 */
> +		result = ds1307->write_block_data(client, ds1307->nvram_offset + off,
> count, buf);
> +	} else	{	/* ds1388 */
> +		if ((off <= 0x00ff) && ((off + count) > 0x00ff))	{  /* read spans two
> blocks */
> +			client = ds1307->client[1];		/* first block */
> +			addr = (unsigned int)off;
> +			result = ds1307->write_block_data(client, addr, (0x100-addr), buf);

Error checking?

> +			client = ds1307->client[2];		/* second block */
> +			result = ds1307->write_block_data(client, 0x00, (addr+count-0x100),
> (buf+0x100-addr));
> +
> +		} else { /* Device address/offset translation */
> +			dev_dbg(&client->dev, "Write spans one block\n");
> +			if (off <= 0x00ff)	{

A single space before the {

> +				client = ds1307->client[1];
> +				addr = (unsigned int)off;
> +			} else if (off <= 0x0200)	{

ditto

> +				client = ds1307->client[2];
> +				addr = (unsigned int)off - 0x100;
> +			}
> +			result = ds1307->write_block_data(client, addr, count, buf);
> +		}
> +	}
>  	if (result < 0) {
>  		dev_err(&client->dev, "%s error %d\n", "nvram write", result);
>  		return result;
>
> ...
>
> @@ -866,16 +923,35 @@ read_rtc:
>  		dev_dbg(&client->dev, "got IRQ %d\n", client->irq);
>  	}
>  
> -	if (chip->nvram56) {
> +	if (chip->nvram) {
> +		nvram.size = ds1307->nvram_size;
>  		err = sysfs_create_bin_file(&client->dev.kobj, &nvram);
>  		if (err == 0) {
>  			set_bit(HAS_NVRAM, &ds1307->flags);
> -			dev_info(&client->dev, "56 bytes nvram\n");
> +			dev_info(&client->dev, "%d bytes nvram\n", nvram.size);
> +		}
> +	}
> +
> +	/* use dummy devices for multiple address chips */
> +	for (i = 1; i < ds1307->num_addrs; i++) {
> +		ds1307->client[i] = i2c_new_dummy(client->adapter, client->addr + i);
> +		if (!ds1307->client[i]) {
> +			dev_err(&client->dev, "address 0x%02x unavailable\n",
> +					client->addr + i);
> +			err = -EADDRINUSE;

EADDRINUSE is a networking error code.  Returning that from an
rtc-related operation might mislead the operator.


> +			goto err_clients;
>  		}
> +		i2c_set_clientdata(ds1307->client[i], ds1307);
>  	}
>  
>  	return 0;
>  
> +err_clients:
> +	for (i = 1; i < ds1307->num_addrs; i++)	{
> +		if (ds1307->client[i])	{
> +			i2c_unregister_device(ds1307->client[i]);
> +		}

We normally omit braces around a single statement.

The `if (ds1307->client[i])' test should be unneeded anyway - just
unregister client[1] though client[i - 1].

Why isn't client[0] used here btw?

> +	}
>  exit_irq:
>  	rtc_device_unregister(ds1307->rtc);
>  exit_free: