lkml.org 
[lkml]   [2012]   [Jan]   [11]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
Date
From
SubjectRe: [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/ |[unhandled content-type:application/pgp-signature]
\
 
 \ /
  Last update: 2012-01-11 12:09    [from the cache]
©2003-2011 Jasper Spaans