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

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(-)

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

(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.

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