Re: [PATCH v3] rtc: ds1307: generalise ram size and offset

[Wolfram Sang <w.sang@pengutronix.de>]

[-- Attachment #1: Type: text/plain, Size: 2352 bytes --]

Austin,

> --- a/drivers/rtc/rtc-ds1307.c	2011-10-10 11:22:22.674690998 +1300
> +++ b/drivers/rtc/rtc-ds1307.c	2011-11-04 10:02:27.859155009 +1300
> @@ -104,6 +104,8 @@ enum ds_type {
>  
>  struct ds1307 {
>  	u8			offset; /* register's offset */
> +	u16			nvram_offset;
> +	u16			nvram_size;

I'd put them further down in the struct, at least after regs.


>  	u8			regs[11];
>  	enum ds_type		type;
>  	unsigned long		flags;
> @@ -119,26 +121,37 @@ struct ds1307 {
>  };
>  
>  struct chip_desc {
> -	unsigned		nvram56:1;
>  	unsigned		alarm:1;
> +	u8			offset;

I think the stuff related to 'offset' should be in a separate patch with
specific commit-msg.

> +	u16			nvram_offset;
> +	u16			nvram_size;
>  };
>  
>  static const struct chip_desc chips[last_ds_type] = {
>  	[ds_1307] = {
> -		.nvram56	= 1,
> +		.nvram_offset	= 8,
> +		.nvram_size	= 56, /* 56 bytes NVRAM */

Skip the comment, should be obvious.

>  	},
>  	[ds_1337] = {
>  		.alarm		= 1,
>  	},
>  	[ds_1338] = {
> -		.nvram56	= 1,
> +		.nvram_offset	= 8,
> +		.nvram_size	= 56, /* 56 bytes NVRAM */
>  	},
>  	[ds_1339] = {
>  		.alarm		= 1,
>  	},
> +	[ds_1388] = {
> +		.offset		= 1, /* seconds starts at 1 */
> +	},
>  	[ds_3231] = {
>  		.alarm		= 1,
>  	},
> +	[mcp7941x] = {
> +		.nvram_offset	= 0x20,
> +		.nvram_size	= 64, /* 64 bytes SRAM */

Minor: hex value for size, too? Comment should probably emphasize that
this is SRAM not NVRAM, the size is obvious again.

> +	},
>  };
>  
...

> @@ -638,7 +650,19 @@ static int __devinit ds1307_probe(struct
>  
>  	ds1307->client	= client;
>  	ds1307->type	= id->driver_data;
> -	ds1307->offset	= 0;
> +
> +	if (chip && chip->offset)
> +		ds1307->offset = chip->offset;
> +	else
> +		ds1307->offset = 0;
> +	if (chip && chip->nvram_size)
> +		ds1307->nvram_size = chip->nvram_size;
> +	else
> +		ds1307->nvram_size = 0;
> +	if (chip && chip->nvram_offset)
> +		ds1307->nvram_offset = chip->nvram_offset;
> +	else
> +		ds1307->nvram_offset = 0;

ds1307 is kzalloced, so you can simplify this. Then, you also need to
check only once if chip != NULL.

Regards,

   Wolfram

-- 
Pengutronix e.K.                           | Wolfram Sang                |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 198 bytes --]