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

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] [thread-next>] [day] [month] [year] [list]

Date:	Wed, 11 Jan 2012 12:06:50 +0100
From:	Wolfram Sang <w.sang@...gutronix.de>
To:	Austin Boyle <Austin.Boyle@...atnet.com>
Cc:	rtc-linux@...glegroups.com, linux-kernel@...r.kernel.org
Subject: Re: [PATCH v3] rtc: ds1307: generalise ram size and offset

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/  |

Download attachment "signature.asc" of type "application/pgp-signature" (199 bytes)