[1/2] rtc: ds1307: refactor chip_desc table

Submitted by Wolfram Sang on Sept. 28, 2011, 8:18 p.m.

Details

Message ID 1317241107-11392-2-git-send-email-w.sang@pengutronix.de
State Superseded
Headers show

Commit Message

Wolfram Sang Sept. 28, 2011, 8:18 p.m.
The chip_desc table is suboptimal. Currently it requires an entry for every new
chip type, even if it is empty. This has already been forgotten for the ds1388
what will possibly cause an OOPS.  Refactor the code, so new entries are only
needed, when they chip type really needs a (non-empty) description. Also make
the table visually more appealing.

Signed-off-by: Wolfram Sang <w.sang@pengutronix.de>
---
 drivers/rtc/rtc-ds1307.c |   52 ++++++++++++++++++---------------------------
 1 files changed, 21 insertions(+), 31 deletions(-)

Comments

David Anders Sept. 28, 2011, 10:54 p.m.
tested with DS1307 and MCP79410

Tested-by: David Anders <danders.dev@gmail.com>

On Wed, Sep 28, 2011 at 3:18 PM, Wolfram Sang <w.sang@pengutronix.de> wrote:

> The chip_desc table is suboptimal. Currently it requires an entry for every
> new
> chip type, even if it is empty. This has already been forgotten for the
> ds1388
> what will possibly cause an OOPS.  Refactor the code, so new entries are
> only
> needed, when they chip type really needs a (non-empty) description. Also
> make
> the table visually more appealing.
>
> Signed-off-by: Wolfram Sang <w.sang@pengutronix.de>
> ---
>  drivers/rtc/rtc-ds1307.c |   52
> ++++++++++++++++++---------------------------
>  1 files changed, 21 insertions(+), 31 deletions(-)
>
> diff --git a/drivers/rtc/rtc-ds1307.c b/drivers/rtc/rtc-ds1307.c
> index 62b0763..6a64f4a 100644
> --- a/drivers/rtc/rtc-ds1307.c
> +++ b/drivers/rtc/rtc-ds1307.c
> @@ -36,6 +36,7 @@ enum ds_type {
>        m41t00,
>        mcp7941x,
>        rx_8025,
> +       last_ds_type /* always last */
>        // rs5c372 too?  different address...
>  };
>
> @@ -120,30 +121,23 @@ struct chip_desc {
>        unsigned                alarm:1;
>  };
>
> -static const struct chip_desc chips[] = {
> -[ds_1307] = {
> -       .nvram56        = 1,
> -},
> -[ds_1337] = {
> -       .alarm          = 1,
> -},
> -[ds_1338] = {
> -       .nvram56        = 1,
> -},
> -[ds_1339] = {
> -       .alarm          = 1,
> -},
> -[ds_1340] = {
> -},
> -[ds_3231] = {
> -       .alarm          = 1,
> -},
> -[m41t00] = {
> -},
> -[mcp7941x] = {
> -},
> -[rx_8025] = {
> -}, };
> +static const struct chip_desc chips[last_ds_type] = {
> +       [ds_1307] = {
> +               .nvram56        = 1,
> +       },
> +       [ds_1337] = {
> +               .alarm          = 1,
> +       },
> +       [ds_1338] = {
> +               .nvram56        = 1,
> +       },
> +       [ds_1339] = {
> +               .alarm          = 1,
> +       },
> +       [ds_3231] = {
> +               .alarm          = 1,
> +       },
> +};
>
>  static const struct i2c_device_id ds1307_id[] = {
>        { "ds1307", ds_1307 },
> @@ -653,7 +647,7 @@ static int __devinit ds1307_probe(struct i2c_client
> *client,
>        case ds_1339:
>        case ds_3231:
>                /* has IRQ? */
> -               if (ds1307->client->irq > 0 && chip->alarm) {
> +               if (ds1307->client->irq > 0 && chip && chip->alarm) {
>                        INIT_WORK(&ds1307->work, ds1307_work);
>                        want_irq = true;
>                }
> @@ -836,11 +830,7 @@ read_rtc:
>                }
>
>                break;
> -       case rx_8025:
> -       case ds_1337:
> -       case ds_1339:
> -       case ds_1388:
> -       case ds_3231:
> +       default:
>                break;
>        }
>
> @@ -894,7 +884,7 @@ read_rtc:
>                dev_dbg(&client->dev, "got IRQ %d\n", client->irq);
>        }
>
> -       if (chip->nvram56) {
> +       if (chip && chip->nvram56) {
>                err = sysfs_create_bin_file(&client->dev.kobj, &nvram);
>                if (err == 0) {
>                        set_bit(HAS_NVRAM, &ds1307->flags);
> --
> 1.7.2.5
>
>
Andrew Morton Feb. 1, 2012, 11:44 p.m.
(chasing down old patches whihc I wasn't cc'ed on)

On Wed, 28 Sep 2011 21:18:26 +0100
Wolfram Sang <w.sang@pengutronix.de> wrote:

> The chip_desc table is suboptimal. Currently it requires an entry for every new
> chip type, even if it is empty. This has already been forgotten for the ds1388
> what will possibly cause an OOPS.  Refactor the code, so new entries are only
> needed, when they chip type really needs a (non-empty) description. Also make
> the table visually more appealing.
> 
> ...
>
>  static const struct i2c_device_id ds1307_id[] = {
>  	{ "ds1307", ds_1307 },
> @@ -653,7 +647,7 @@ static int __devinit ds1307_probe(struct i2c_client *client,
>  	case ds_1339:
>  	case ds_3231:
>  		/* has IRQ? */
> -		if (ds1307->client->irq > 0 && chip->alarm) {
> +		if (ds1307->client->irq > 0 && chip && chip->alarm) {

But `chip' is initalised like this:
	
	const struct chip_desc *chip = &chips[id->driver_data];

and will never be NULL.
Wolfram Sang Feb. 2, 2012, 3:52 p.m.
> > -		if (ds1307->client->irq > 0 && chip->alarm) {
> > +		if (ds1307->client->irq > 0 && chip && chip->alarm) {
> 
> But `chip' is initalised like this:
> 	
> 	const struct chip_desc *chip = &chips[id->driver_data];
> 
> and will never be NULL.

Right, thanks. Will fix.

I will also wait for Austin's latest version to refactor the nvram
support, and push all that to my branch at

	git://git.pengutronix.de/git/wsa/linux-2.6.git ds1307

and also repost the whole series of ds1307-updates including collected
tags. I hope this will make it easier for you; if not, please say what
would :)

Regards,

   Wolfram

Patch hide | download patch | download mbox

diff --git a/drivers/rtc/rtc-ds1307.c b/drivers/rtc/rtc-ds1307.c
index 62b0763..6a64f4a 100644
--- a/drivers/rtc/rtc-ds1307.c
+++ b/drivers/rtc/rtc-ds1307.c
@@ -36,6 +36,7 @@  enum ds_type {
 	m41t00,
 	mcp7941x,
 	rx_8025,
+	last_ds_type /* always last */
 	// rs5c372 too?  different address...
 };
 
@@ -120,30 +121,23 @@  struct chip_desc {
 	unsigned		alarm:1;
 };
 
-static const struct chip_desc chips[] = {
-[ds_1307] = {
-	.nvram56	= 1,
-},
-[ds_1337] = {
-	.alarm		= 1,
-},
-[ds_1338] = {
-	.nvram56	= 1,
-},
-[ds_1339] = {
-	.alarm		= 1,
-},
-[ds_1340] = {
-},
-[ds_3231] = {
-	.alarm		= 1,
-},
-[m41t00] = {
-},
-[mcp7941x] = {
-},
-[rx_8025] = {
-}, };
+static const struct chip_desc chips[last_ds_type] = {
+	[ds_1307] = {
+		.nvram56	= 1,
+	},
+	[ds_1337] = {
+		.alarm		= 1,
+	},
+	[ds_1338] = {
+		.nvram56	= 1,
+	},
+	[ds_1339] = {
+		.alarm		= 1,
+	},
+	[ds_3231] = {
+		.alarm		= 1,
+	},
+};
 
 static const struct i2c_device_id ds1307_id[] = {
 	{ "ds1307", ds_1307 },
@@ -653,7 +647,7 @@  static int __devinit ds1307_probe(struct i2c_client *client,
 	case ds_1339:
 	case ds_3231:
 		/* has IRQ? */
-		if (ds1307->client->irq > 0 && chip->alarm) {
+		if (ds1307->client->irq > 0 && chip && chip->alarm) {
 			INIT_WORK(&ds1307->work, ds1307_work);
 			want_irq = true;
 		}
@@ -836,11 +830,7 @@  read_rtc:
 		}
 
 		break;
-	case rx_8025:
-	case ds_1337:
-	case ds_1339:
-	case ds_1388:
-	case ds_3231:
+	default:
 		break;
 	}
 
@@ -894,7 +884,7 @@  read_rtc:
 		dev_dbg(&client->dev, "got IRQ %d\n", client->irq);
 	}
 
-	if (chip->nvram56) {
+	if (chip && chip->nvram56) {
 		err = sysfs_create_bin_file(&client->dev.kobj, &nvram);
 		if (err == 0) {
 			set_bit(HAS_NVRAM, &ds1307->flags);