Re: [PATCH 0/2] Add support for the S-35390A RTC chip.

Re: [PATCH 0/2] Add support for the S-35390A RTC chip.

[Jean Delvare]

Hi Byron,

On Sat, 15 Dec 2007 20:42:54 +0000, Byron Bradley wrote:
> These patches provide support the S-35390A RTC chip as used in the
> QNAP TS-109/TS-209 NAS devices. Along with a change from Andrew Morton
> this has already been accepted into the -mm tree but it was suggested
> that I send it to this list for an I2C review.
> 
> This chip is special in that the first 4-bits of the address are fixed
> to 0xc and the next 3-bits are used as a command so it takes up 8 addresses
> on the I2C bus.

What a weird design :(

> 0001 Add support for the S-35390A RTC chip.
> 0002 rtc-add-support-for-the-s-35390a-rtc-chip-fix
>         Written by Andrew Morton this replaces my bit-reversing function
>         with bitrev8().

Given that your original patch isn't merged in git yet, the preferred
approach is to merge the fixes in your patch before you resubmit it.
This makes further reviews easier.

-- 
Jean Delvare

Re: [PATCH 1/2] Add support for the S-35390A RTC chip.

[Jean Delvare]

On Sat, 15 Dec 2007 20:42:55 +0000, Byron Bradley wrote:
> This adds basic get/set time support for the Seiko Instruments
> S-35390A. This chip communicates using I2C and is used on the
> QNAP TS-109/TS-209 NAS devices.

My review of the i2c side of things (mainly):

> Signed-off-by: Byron Bradley <byron.bbradley-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
> Tested-by: Tim Ellis <timtimred-f/KTTADhmRsdnm+yROfE0A@public.gmane.org>
> ---
>  drivers/rtc/Kconfig       |    9 ++
>  drivers/rtc/Makefile      |    1 +
>  drivers/rtc/rtc-s35390a.c |  302 +++++++++++++++++++++++++++++++++++++++++++++
>  3 files changed, 312 insertions(+), 0 deletions(-)
>  create mode 100644 drivers/rtc/rtc-s35390a.c
> 
> diff --git a/drivers/rtc/Kconfig b/drivers/rtc/Kconfig
> index 1e6715e..6c0fdf9 100644
> --- a/drivers/rtc/Kconfig
> +++ b/drivers/rtc/Kconfig
> @@ -246,6 +246,15 @@ config RTC_DRV_TWL92330
>  	  platforms.  The support is integrated with the rest of
>  	  the Menelaus driver; it's not separate module.
>  
> +config RTC_DRV_S35390A
> +	tristate "Seiko Instruments S-35390A"
> +	help
> +	  If you say yes here you will get support for the Seiko
> +	  Instruments S-35390A.
> +
> +	  This driver can also be built as a module. If so the module
> +	  will be called rtc-s35390a.
> +
>  endif # I2C
>  
>  comment "SPI RTC drivers"
> diff --git a/drivers/rtc/Makefile b/drivers/rtc/Makefile
> index 465db4d..8d6218f 100644
> --- a/drivers/rtc/Makefile
> +++ b/drivers/rtc/Makefile
> @@ -41,6 +41,7 @@ obj-$(CONFIG_RTC_DRV_PL031)	+= rtc-pl031.o
>  obj-$(CONFIG_RTC_DRV_RS5C313)	+= rtc-rs5c313.o
>  obj-$(CONFIG_RTC_DRV_RS5C348)	+= rtc-rs5c348.o
>  obj-$(CONFIG_RTC_DRV_RS5C372)	+= rtc-rs5c372.o
> +obj-$(CONFIG_RTC_DRV_S35390A)	+= rtc-s35390a.o
>  obj-$(CONFIG_RTC_DRV_S3C)	+= rtc-s3c.o
>  obj-$(CONFIG_RTC_DRV_SA1100)	+= rtc-sa1100.o
>  obj-$(CONFIG_RTC_DRV_SH)	+= rtc-sh.o
> diff --git a/drivers/rtc/rtc-s35390a.c b/drivers/rtc/rtc-s35390a.c
> new file mode 100644
> index 0000000..29a95b6
> --- /dev/null
> +++ b/drivers/rtc/rtc-s35390a.c
> @@ -0,0 +1,302 @@
> +/*
> + * Seiko Instruments S-35390A RTC Driver
> + *
> + * Copyright (c) 2007 Byron Bradley
> + *
> + * This program is free software; you can redistribute it and/or
> + * modify it under the terms of the GNU General Public License
> + * as published by the Free Software Foundation; either version
> + * 2 of the License, or (at your option) any later version.
> + */
> +
> +#include <linux/module.h>
> +#include <linux/rtc.h>
> +#include <linux/i2c.h>
> +#include <linux/bcd.h>

You also need to include <linux/slab.h> for kzalloc and kfree.

> +
> +#define S35390A_CMD_STATUS1	0
> +#define S35390A_CMD_STATUS2	1
> +#define S35390A_CMD_TIME1	2
> +
> +#define S35390A_BYTE_YEAR	0
> +#define S35390A_BYTE_MONTH	1
> +#define S35390A_BYTE_DAY	2
> +#define S35390A_BYTE_WDAY	3
> +#define S35390A_BYTE_HOURS	4
> +#define S35390A_BYTE_MINS	5
> +#define S35390A_BYTE_SECS	6
> +
> +#define S35390A_FLAG_POC	0x01
> +#define S35390A_FLAG_BLD	0x02
> +#define S35390A_FLAG_24H	0x40
> +#define S35390A_FLAG_RESET	0x80
> +#define S35390A_FLAG_TEST	0x01
> +
> +struct s35390a {
> +	struct i2c_client *client;
> +	struct rtc_device *rtc;
> +	int twentyfourhour;
> +};
> +
> +static int s35390a_set_reg(struct s35390a *s35390a, int reg, char *buf, int len)
> +{
> +	struct i2c_client *client = s35390a->client;
> +	struct i2c_msg msg[] = {
> +		{ client->addr | reg, 0, len, buf },
> +	};
> +
> +	/* Only write to the writable bits in the status1 register */
> +	if (reg == S35390A_CMD_STATUS1)
> +		buf[0] &= 0xf;

This would be more efficiently handled in the caller. After all there's
only one place where you write to this register.

> +
> +	if ((i2c_transfer(client->adapter, msg, 1)) != 1)
> +		return -EIO;
> +
> +	return 0;
> +}
> +
> +static int s35390a_get_reg(struct s35390a *s35390a, int reg, char *buf, int len)
> +{
> +	struct i2c_client *client = s35390a->client;
> +	struct i2c_msg msg[] = {
> +		{ client->addr | reg, I2C_M_RD, len, buf },
> +	};
> +
> +	if ((i2c_transfer(client->adapter, msg, 1)) != 1)
> +		return -EIO;
> +
> +	return 0;
> +}
> +
> +static int s35390a_reset(struct s35390a *s35390a)
> +{
> +	char buf[1];
> +
> +	if (s35390a_get_reg(s35390a, S35390A_CMD_STATUS1, buf, sizeof(buf)) < 0)
> +		return -EIO;
> +
> +	if (!(buf[0] & (S35390A_FLAG_POC | S35390A_FLAG_BLD)))
> +		return 0;

This will return if _either_ flag is set, is it what you want?

> +
> +	buf[0] |= S35390A_FLAG_RESET;
> +	return s35390a_set_reg(s35390a, S35390A_CMD_STATUS1, buf, sizeof(buf));

There's something wrong here. You set S35390A_FLAG_RESET which is 0x80,
but then in s35390a_set_reg() you mask the value with 0xf, which resets
the RESET bit to 0, before you write it to the chip.

> +}
> +
> +static int s35390a_disable_test_mode(struct s35390a *s35390a)
> +{
> +	char buf[1];
> +
> +	if (s35390a_get_reg(s35390a, S35390A_CMD_STATUS2, buf, sizeof(buf)) < 0)
> +		return -EIO;
> +
> +	if (!(buf[0] & S35390A_FLAG_TEST))
> +		return 0;
> +
> +	buf[0] &= ~S35390A_FLAG_TEST;
> +	return s35390a_set_reg(s35390a, S35390A_CMD_STATUS2, buf, sizeof(buf));
> +}
> +
> +static char s35390a_hr2reg(struct s35390a *s35390a, int hour)
> +{
> +	if (s35390a->twentyfourhour)
> +		return BIN2BCD(hour);
> +
> +	if (hour < 12)
> +		return BIN2BCD(hour);
> +
> +	return 0x40 | BIN2BCD(hour - 12);
> +}
> +
> +static int s35390a_reg2hr(struct s35390a *s35390a, char reg)
> +{
> +	unsigned hour;
> +
> +	if (s35390a->twentyfourhour)
> +		return BCD2BIN(reg & 0x3f);
> +
> +	hour = BCD2BIN(reg & 0x3f);
> +	if (reg & 0x40)
> +		hour += 12;
> +
> +	return hour;
> +}
> +
> +static inline char reverse(char x)
> +{
> +	x = ((x >> 1) & 0x55) | ((x << 1) & 0xaa);
> +	x = ((x >> 2) & 0x33) | ((x << 2) & 0xcc);
> +	return (x >> 4) | (x << 4);
> +}
> +
> +static int s35390a_set_datetime(struct i2c_client *client, struct rtc_time *tm)
> +{
> +	struct s35390a	*s35390a = i2c_get_clientdata(client);
> +	int i, err;
> +	char buf[7];
> +
> +	dev_dbg(&client->dev, "%s: tm is secs=%d, mins=%d, hours=%d mday=%d, "
> +		"mon=%d, year=%d, wday=%d\n", __FUNCTION__, tm->tm_sec,
> +		tm->tm_min, tm->tm_hour, tm->tm_mday, tm->tm_mon, tm->tm_year,
> +		tm->tm_wday);
> +
> +	buf[S35390A_BYTE_YEAR] = BIN2BCD(tm->tm_year - 100);
> +	buf[S35390A_BYTE_MONTH] = BIN2BCD(tm->tm_mon + 1);
> +	buf[S35390A_BYTE_DAY] = BIN2BCD(tm->tm_mday);
> +	buf[S35390A_BYTE_WDAY] = BIN2BCD(tm->tm_wday);
> +	buf[S35390A_BYTE_HOURS] = s35390a_hr2reg(s35390a, tm->tm_hour);
> +	buf[S35390A_BYTE_MINS] = BIN2BCD(tm->tm_min);
> +	buf[S35390A_BYTE_SECS] = BIN2BCD(tm->tm_sec);
> +
> +	/* This chip expects the bits of each byte to be in reverse order */
> +	for (i = 0; i < 7; ++i)
> +		buf[i] = reverse(buf[i]);
> +
> +	err = s35390a_set_reg(s35390a, S35390A_CMD_TIME1, buf, sizeof(buf));
> +
> +	return err;
> +}
> +
> +static int s35390a_get_datetime(struct i2c_client *client, struct rtc_time *tm)
> +{
> +	struct s35390a *s35390a = i2c_get_clientdata(client);
> +	char buf[7];
> +	int i, err;
> +
> +	err = s35390a_get_reg(s35390a, S35390A_CMD_TIME1, buf, sizeof(buf));
> +	if (err < 0)
> +		return err;
> +
> +	/* This chip returns the bits of each byte in reverse order */
> +	for (i = 0; i < 7; ++i)
> +		buf[i] = reverse(buf[i]);
> +
> +	tm->tm_sec = BCD2BIN(buf[S35390A_BYTE_SECS]);
> +	tm->tm_min = BCD2BIN(buf[S35390A_BYTE_MINS]);
> +	tm->tm_hour = s35390a_reg2hr(s35390a, buf[S35390A_BYTE_HOURS]);
> +	tm->tm_wday = BCD2BIN(buf[S35390A_BYTE_WDAY]);
> +	tm->tm_mday = BCD2BIN(buf[S35390A_BYTE_DAY]);
> +	tm->tm_mon = BCD2BIN(buf[S35390A_BYTE_MONTH]) - 1;
> +	tm->tm_year = BCD2BIN(buf[S35390A_BYTE_YEAR]) + 100;
> +
> +	dev_dbg(&client->dev, "%s: tm is secs=%d, mins=%d, hours=%d, mday=%d, "
> +		"mon=%d, year=%d, wday=%d\n", __FUNCTION__, tm->tm_sec,
> +		tm->tm_min, tm->tm_hour, tm->tm_mday, tm->tm_mon, tm->tm_year,
> +		tm->tm_wday);
> +
> +	return rtc_valid_tm(tm);
> +}
> +
> +static int s35390a_rtc_read_time(struct device *dev, struct rtc_time *tm)
> +{
> +	return s35390a_get_datetime(to_i2c_client(dev), tm);
> +}
> +
> +static int s35390a_rtc_set_time(struct device *dev, struct rtc_time *tm)
> +{
> +	return s35390a_set_datetime(to_i2c_client(dev), tm);
> +}
> +
> +static const struct rtc_class_ops s35390a_rtc_ops = {
> +	.read_time	= s35390a_rtc_read_time,
> +	.set_time	= s35390a_rtc_set_time,
> +};
> +
> +static struct i2c_driver s35390a_driver;
> +
> +static int s35390a_probe(struct i2c_client *client)
> +{
> +	int err = 0;

Useless initialization.

> +	struct s35390a *s35390a;
> +	struct rtc_time tm;
> +	char buf[1];
> +
> +	if (!i2c_check_functionality(client->adapter, I2C_FUNC_I2C)) {
> +		err = -ENODEV;
> +		goto exit;
> +	}
> +
> +	s35390a = kzalloc(sizeof(struct s35390a), GFP_KERNEL);
> +	if (!s35390a) {
> +		err = -ENOMEM;
> +		goto exit;
> +	}
> +
> +	s35390a->client = client;
> +	i2c_set_clientdata(client, s35390a);
> +
> +	err = s35390a_disable_test_mode(s35390a);
> +	if (err < 0) {
> +		dev_err(&client->dev, "error disabling test mode\n");
> +		goto exit_kfree;
> +	}
> +
> +	err = s35390a_reset(s35390a);
> +	if (err < 0) {
> +		dev_err(&client->dev, "error resetting chip\n");
> +		goto exit_kfree;
> +	}
> +
> +	err = s35390a_get_reg(s35390a, S35390A_CMD_STATUS1, buf, sizeof(buf));
> +	if (err < 0) {
> +		dev_err(&client->dev, "error checking 12/24 hour mode\n");
> +		goto exit_kfree;
> +	}
> +	if (buf[0] & S35390A_FLAG_24H)
> +		s35390a->twentyfourhour = 1;
> +	else
> +		s35390a->twentyfourhour = 0;

Wouldn't it be more efficient to just force 24h mode here?

> +
> +	if (s35390a_get_datetime(client, &tm) < 0)
> +		dev_warn(&client->dev, "clock needs to be set\n");
> +
> +	dev_info(&client->dev, "S35390A found\n");

rtc_device_register already prints a message so this is somewhat
redundant.

> +
> +	s35390a->rtc = rtc_device_register(s35390a_driver.driver.name,
> +				&client->dev, &s35390a_rtc_ops, THIS_MODULE);
> +
> +	if (IS_ERR(s35390a->rtc)) {
> +		err = PTR_ERR(s35390a->rtc);
> +		goto exit_kfree;
> +	}
> +	return 0;
> +
> +exit_kfree:

I suggest adding:
	i2c_set_clientdata(client, NULL);
so as to not leave a dangling pointer behind.

> +	kfree(s35390a);
> +
> +exit:
> +	return err;
> +}
> +
> +static int s35390a_remove(struct i2c_client *client)
> +{
> +	struct s35390a *s35390a = i2c_get_clientdata(client);
> +
> +	rtc_device_unregister(s35390a->rtc);

Same here.

> +	kfree(s35390a);
> +	return 0;
> +}
> +
> +static struct i2c_driver s35390a_driver = {
> +	.driver		= {
> +		.name	= "rtc-s35390a",
> +	},
> +	.probe		= s35390a_probe,
> +	.remove		= s35390a_remove,
> +};
> +
> +static int __init s35390a_rtc_init(void)
> +{
> +	return i2c_add_driver(&s35390a_driver);
> +}
> +
> +static void __exit s35390a_rtc_exit(void)
> +{
> +	i2c_del_driver(&s35390a_driver);
> +}
> +
> +MODULE_AUTHOR("Byron Bradley <byron.bbradley-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>");
> +MODULE_DESCRIPTION("S35390A RTC driver");
> +MODULE_LICENSE("GPL");
> +
> +module_init(s35390a_rtc_init);
> +module_exit(s35390a_rtc_exit);

No other comment, that's pretty clean code overall.

Note: due to the particular way this device is accessed, I don't think
that we want to use SMBus-level functions (as had been suggested in a
different thread) unless we really have to for compatibility purposes.
Your code is quite nice the way it is and I suspect that using
SMBus-level functions would make it much more complex.

-- 
Jean Delvare

Re: [PATCH 1/2] Add support for the S-35390A RTC chip.

[David Brownell]

On Friday 04 January 2008, Jean Delvare wrote:
> > +     if (buf[0] & S35390A_FLAG_24H)
> > +             s35390a->twentyfourhour = 1;
> > +     else
> > +             s35390a->twentyfourhour = 0;
> 
> Wouldn't it be more efficient to just force 24h mode here?

Maybe, but that would make trouble on multi-master systems where
the other masters assume this stays initialized.  Also, many
RTCs can't change that mode easily ... so it should be done only
when (re)initializing after e.g. an oscillator failure or loss
of the backup power supply.

- Dave
 

Re: [PATCH 1/2] Add support for the S-35390A RTC chip.

[Byron Bradley]

On Jan 4, 2008 8:06 PM, Jean Delvare <khali-PUYAD+kWke1g9hUCZPvPmw@public.gmane.org> wrote:
> On Sat, 15 Dec 2007 20:42:55 +0000, Byron Bradley wrote:
> > This adds basic get/set time support for the Seiko Instruments
> > S-35390A. This chip communicates using I2C and is used on the
> > QNAP TS-109/TS-209 NAS devices.
>
> My review of the i2c side of things (mainly):
>
>
> > Signed-off-by: Byron Bradley <byron.bbradley-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
> > Tested-by: Tim Ellis <timtimred-f/KTTADhmRsdnm+yROfE0A@public.gmane.org>
> > ---
> >  drivers/rtc/Kconfig       |    9 ++
> >  drivers/rtc/Makefile      |    1 +
> >  drivers/rtc/rtc-s35390a.c |  302 +++++++++++++++++++++++++++++++++++++++++++++
> >  3 files changed, 312 insertions(+), 0 deletions(-)
> >  create mode 100644 drivers/rtc/rtc-s35390a.c
> >
> > diff --git a/drivers/rtc/Kconfig b/drivers/rtc/Kconfig
> > index 1e6715e..6c0fdf9 100644
> > --- a/drivers/rtc/Kconfig
> > +++ b/drivers/rtc/Kconfig
> > @@ -246,6 +246,15 @@ config RTC_DRV_TWL92330
> >         platforms.  The support is integrated with the rest of
> >         the Menelaus driver; it's not separate module.
> >
> > +config RTC_DRV_S35390A
> > +     tristate "Seiko Instruments S-35390A"
> > +     help
> > +       If you say yes here you will get support for the Seiko
> > +       Instruments S-35390A.
> > +
> > +       This driver can also be built as a module. If so the module
> > +       will be called rtc-s35390a.
> > +
> >  endif # I2C
> >
> >  comment "SPI RTC drivers"
> > diff --git a/drivers/rtc/Makefile b/drivers/rtc/Makefile
> > index 465db4d..8d6218f 100644
> > --- a/drivers/rtc/Makefile
> > +++ b/drivers/rtc/Makefile
> > @@ -41,6 +41,7 @@ obj-$(CONFIG_RTC_DRV_PL031) += rtc-pl031.o
> >  obj-$(CONFIG_RTC_DRV_RS5C313)        += rtc-rs5c313.o
> >  obj-$(CONFIG_RTC_DRV_RS5C348)        += rtc-rs5c348.o
> >  obj-$(CONFIG_RTC_DRV_RS5C372)        += rtc-rs5c372.o
> > +obj-$(CONFIG_RTC_DRV_S35390A)        += rtc-s35390a.o
> >  obj-$(CONFIG_RTC_DRV_S3C)    += rtc-s3c.o
> >  obj-$(CONFIG_RTC_DRV_SA1100) += rtc-sa1100.o
> >  obj-$(CONFIG_RTC_DRV_SH)     += rtc-sh.o
> > diff --git a/drivers/rtc/rtc-s35390a.c b/drivers/rtc/rtc-s35390a.c
> > new file mode 100644
> > index 0000000..29a95b6
> > --- /dev/null
> > +++ b/drivers/rtc/rtc-s35390a.c
> > @@ -0,0 +1,302 @@
> > +/*
> > + * Seiko Instruments S-35390A RTC Driver
> > + *
> > + * Copyright (c) 2007 Byron Bradley
> > + *
> > + * This program is free software; you can redistribute it and/or
> > + * modify it under the terms of the GNU General Public License
> > + * as published by the Free Software Foundation; either version
> > + * 2 of the License, or (at your option) any later version.
> > + */
> > +
> > +#include <linux/module.h>
> > +#include <linux/rtc.h>
> > +#include <linux/i2c.h>
> > +#include <linux/bcd.h>
>
> You also need to include <linux/slab.h> for kzalloc and kfree.
>
>
> > +
> > +#define S35390A_CMD_STATUS1  0
> > +#define S35390A_CMD_STATUS2  1
> > +#define S35390A_CMD_TIME1    2
> > +
> > +#define S35390A_BYTE_YEAR    0
> > +#define S35390A_BYTE_MONTH   1
> > +#define S35390A_BYTE_DAY     2
> > +#define S35390A_BYTE_WDAY    3
> > +#define S35390A_BYTE_HOURS   4
> > +#define S35390A_BYTE_MINS    5
> > +#define S35390A_BYTE_SECS    6
> > +
> > +#define S35390A_FLAG_POC     0x01
> > +#define S35390A_FLAG_BLD     0x02
> > +#define S35390A_FLAG_24H     0x40
> > +#define S35390A_FLAG_RESET   0x80
> > +#define S35390A_FLAG_TEST    0x01
> > +
> > +struct s35390a {
> > +     struct i2c_client *client;
> > +     struct rtc_device *rtc;
> > +     int twentyfourhour;
> > +};
> > +
> > +static int s35390a_set_reg(struct s35390a *s35390a, int reg, char *buf, int len)
> > +{
> > +     struct i2c_client *client = s35390a->client;
> > +     struct i2c_msg msg[] = {
> > +             { client->addr | reg, 0, len, buf },
> > +     };
> > +
> > +     /* Only write to the writable bits in the status1 register */
> > +     if (reg == S35390A_CMD_STATUS1)
> > +             buf[0] &= 0xf;
>
> This would be more efficiently handled in the caller. After all there's
> only one place where you write to this register.
>
> > +
> > +     if ((i2c_transfer(client->adapter, msg, 1)) != 1)
> > +             return -EIO;
> > +
> > +     return 0;
> > +}
> > +
> > +static int s35390a_get_reg(struct s35390a *s35390a, int reg, char *buf, int len)
> > +{
> > +     struct i2c_client *client = s35390a->client;
> > +     struct i2c_msg msg[] = {
> > +             { client->addr | reg, I2C_M_RD, len, buf },
> > +     };
> > +
> > +     if ((i2c_transfer(client->adapter, msg, 1)) != 1)
> > +             return -EIO;
> > +
> > +     return 0;
> > +}
> > +
> > +static int s35390a_reset(struct s35390a *s35390a)
> > +{
> > +     char buf[1];
> > +
> > +     if (s35390a_get_reg(s35390a, S35390A_CMD_STATUS1, buf, sizeof(buf)) < 0)
> > +             return -EIO;
> > +
> > +     if (!(buf[0] & (S35390A_FLAG_POC | S35390A_FLAG_BLD)))
> > +             return 0;
>
> This will return if _either_ flag is set, is it what you want?

The ! means that this will return if neither flag is set right? POC
means that the device is newly powered and the register state may be
invalid. BLD is set when the power drops below minimum so again the
registers could be invalid. In either case the device should be reset

> > +
> > +     buf[0] |= S35390A_FLAG_RESET;
> > +     return s35390a_set_reg(s35390a, S35390A_CMD_STATUS1, buf, sizeof(buf));
>
> There's something wrong here. You set S35390A_FLAG_RESET which is 0x80,
> but then in s35390a_set_reg() you mask the value with 0xf, which resets
> the RESET bit to 0, before you write it to the chip.

Yes, the mask should be 0xf0, it was written before I realised the
bit-order was reversed.

> > +}
> > +
> > +static int s35390a_disable_test_mode(struct s35390a *s35390a)
> > +{
> > +     char buf[1];
> > +
> > +     if (s35390a_get_reg(s35390a, S35390A_CMD_STATUS2, buf, sizeof(buf)) < 0)
> > +             return -EIO;
> > +
> > +     if (!(buf[0] & S35390A_FLAG_TEST))
> > +             return 0;
> > +
> > +     buf[0] &= ~S35390A_FLAG_TEST;
> > +     return s35390a_set_reg(s35390a, S35390A_CMD_STATUS2, buf, sizeof(buf));
> > +}
> > +
> > +static char s35390a_hr2reg(struct s35390a *s35390a, int hour)
> > +{
> > +     if (s35390a->twentyfourhour)
> > +             return BIN2BCD(hour);
> > +
> > +     if (hour < 12)
> > +             return BIN2BCD(hour);
> > +
> > +     return 0x40 | BIN2BCD(hour - 12);
> > +}
> > +
> > +static int s35390a_reg2hr(struct s35390a *s35390a, char reg)
> > +{
> > +     unsigned hour;
> > +
> > +     if (s35390a->twentyfourhour)
> > +             return BCD2BIN(reg & 0x3f);
> > +
> > +     hour = BCD2BIN(reg & 0x3f);
> > +     if (reg & 0x40)
> > +             hour += 12;
> > +
> > +     return hour;
> > +}
> > +
> > +static inline char reverse(char x)
> > +{
> > +     x = ((x >> 1) & 0x55) | ((x << 1) & 0xaa);
> > +     x = ((x >> 2) & 0x33) | ((x << 2) & 0xcc);
> > +     return (x >> 4) | (x << 4);
> > +}
> > +
> > +static int s35390a_set_datetime(struct i2c_client *client, struct rtc_time *tm)
> > +{
> > +     struct s35390a  *s35390a = i2c_get_clientdata(client);
> > +     int i, err;
> > +     char buf[7];
> > +
> > +     dev_dbg(&client->dev, "%s: tm is secs=%d, mins=%d, hours=%d mday=%d, "
> > +             "mon=%d, year=%d, wday=%d\n", __FUNCTION__, tm->tm_sec,
> > +             tm->tm_min, tm->tm_hour, tm->tm_mday, tm->tm_mon, tm->tm_year,
> > +             tm->tm_wday);
> > +
> > +     buf[S35390A_BYTE_YEAR] = BIN2BCD(tm->tm_year - 100);
> > +     buf[S35390A_BYTE_MONTH] = BIN2BCD(tm->tm_mon + 1);
> > +     buf[S35390A_BYTE_DAY] = BIN2BCD(tm->tm_mday);
> > +     buf[S35390A_BYTE_WDAY] = BIN2BCD(tm->tm_wday);
> > +     buf[S35390A_BYTE_HOURS] = s35390a_hr2reg(s35390a, tm->tm_hour);
> > +     buf[S35390A_BYTE_MINS] = BIN2BCD(tm->tm_min);
> > +     buf[S35390A_BYTE_SECS] = BIN2BCD(tm->tm_sec);
> > +
> > +     /* This chip expects the bits of each byte to be in reverse order */
> > +     for (i = 0; i < 7; ++i)
> > +             buf[i] = reverse(buf[i]);
> > +
> > +     err = s35390a_set_reg(s35390a, S35390A_CMD_TIME1, buf, sizeof(buf));
> > +
> > +     return err;
> > +}
> > +
> > +static int s35390a_get_datetime(struct i2c_client *client, struct rtc_time *tm)
> > +{
> > +     struct s35390a *s35390a = i2c_get_clientdata(client);
> > +     char buf[7];
> > +     int i, err;
> > +
> > +     err = s35390a_get_reg(s35390a, S35390A_CMD_TIME1, buf, sizeof(buf));
> > +     if (err < 0)
> > +             return err;
> > +
> > +     /* This chip returns the bits of each byte in reverse order */
> > +     for (i = 0; i < 7; ++i)
> > +             buf[i] = reverse(buf[i]);
> > +
> > +     tm->tm_sec = BCD2BIN(buf[S35390A_BYTE_SECS]);
> > +     tm->tm_min = BCD2BIN(buf[S35390A_BYTE_MINS]);
> > +     tm->tm_hour = s35390a_reg2hr(s35390a, buf[S35390A_BYTE_HOURS]);
> > +     tm->tm_wday = BCD2BIN(buf[S35390A_BYTE_WDAY]);
> > +     tm->tm_mday = BCD2BIN(buf[S35390A_BYTE_DAY]);
> > +     tm->tm_mon = BCD2BIN(buf[S35390A_BYTE_MONTH]) - 1;
> > +     tm->tm_year = BCD2BIN(buf[S35390A_BYTE_YEAR]) + 100;
> > +
> > +     dev_dbg(&client->dev, "%s: tm is secs=%d, mins=%d, hours=%d, mday=%d, "
> > +             "mon=%d, year=%d, wday=%d\n", __FUNCTION__, tm->tm_sec,
> > +             tm->tm_min, tm->tm_hour, tm->tm_mday, tm->tm_mon, tm->tm_year,
> > +             tm->tm_wday);
> > +
> > +     return rtc_valid_tm(tm);
> > +}
> > +
> > +static int s35390a_rtc_read_time(struct device *dev, struct rtc_time *tm)
> > +{
> > +     return s35390a_get_datetime(to_i2c_client(dev), tm);
> > +}
> > +
> > +static int s35390a_rtc_set_time(struct device *dev, struct rtc_time *tm)
> > +{
> > +     return s35390a_set_datetime(to_i2c_client(dev), tm);
> > +}
> > +
> > +static const struct rtc_class_ops s35390a_rtc_ops = {
> > +     .read_time      = s35390a_rtc_read_time,
> > +     .set_time       = s35390a_rtc_set_time,
> > +};
> > +
> > +static struct i2c_driver s35390a_driver;
> > +
> > +static int s35390a_probe(struct i2c_client *client)
> > +{
> > +     int err = 0;
>
> Useless initialization.
>
>
> > +     struct s35390a *s35390a;
> > +     struct rtc_time tm;
> > +     char buf[1];
> > +
> > +     if (!i2c_check_functionality(client->adapter, I2C_FUNC_I2C)) {
> > +             err = -ENODEV;
> > +             goto exit;
> > +     }
> > +
> > +     s35390a = kzalloc(sizeof(struct s35390a), GFP_KERNEL);
> > +     if (!s35390a) {
> > +             err = -ENOMEM;
> > +             goto exit;
> > +     }
> > +
> > +     s35390a->client = client;
> > +     i2c_set_clientdata(client, s35390a);
> > +
> > +     err = s35390a_disable_test_mode(s35390a);
> > +     if (err < 0) {
> > +             dev_err(&client->dev, "error disabling test mode\n");
> > +             goto exit_kfree;
> > +     }
> > +
> > +     err = s35390a_reset(s35390a);
> > +     if (err < 0) {
> > +             dev_err(&client->dev, "error resetting chip\n");
> > +             goto exit_kfree;
> > +     }
> > +
> > +     err = s35390a_get_reg(s35390a, S35390A_CMD_STATUS1, buf, sizeof(buf));
> > +     if (err < 0) {
> > +             dev_err(&client->dev, "error checking 12/24 hour mode\n");
> > +             goto exit_kfree;
> > +     }
> > +     if (buf[0] & S35390A_FLAG_24H)
> > +             s35390a->twentyfourhour = 1;
> > +     else
> > +             s35390a->twentyfourhour = 0;
>
> Wouldn't it be more efficient to just force 24h mode here?

I did think about this but of the RTC drivers I was looking at they
all supported both modes. As David Brownell has already pointed out
changing this value is difficult because the time data would become
invalid. I could however switch to 24 hour mode whenever the chip is
reset, this would simplify the runtime a little.

> > +
> > +     if (s35390a_get_datetime(client, &tm) < 0)
> > +             dev_warn(&client->dev, "clock needs to be set\n");
> > +
> > +     dev_info(&client->dev, "S35390A found\n");
>
> rtc_device_register already prints a message so this is somewhat
> redundant.
>
> > +
> > +     s35390a->rtc = rtc_device_register(s35390a_driver.driver.name,
> > +                             &client->dev, &s35390a_rtc_ops, THIS_MODULE);
> > +
> > +     if (IS_ERR(s35390a->rtc)) {
> > +             err = PTR_ERR(s35390a->rtc);
> > +             goto exit_kfree;
> > +     }
> > +     return 0;
> > +
> > +exit_kfree:
>
> I suggest adding:
>         i2c_set_clientdata(client, NULL);
> so as to not leave a dangling pointer behind.
>
> > +     kfree(s35390a);
> > +
> > +exit:
> > +     return err;
> > +}
> > +
> > +static int s35390a_remove(struct i2c_client *client)
> > +{
> > +     struct s35390a *s35390a = i2c_get_clientdata(client);
> > +
> > +     rtc_device_unregister(s35390a->rtc);
>
> Same here.
>
> > +     kfree(s35390a);
> > +     return 0;
> > +}
> > +
> > +static struct i2c_driver s35390a_driver = {
> > +     .driver         = {
> > +             .name   = "rtc-s35390a",
> > +     },
> > +     .probe          = s35390a_probe,
> > +     .remove         = s35390a_remove,
> > +};
> > +
> > +static int __init s35390a_rtc_init(void)
> > +{
> > +     return i2c_add_driver(&s35390a_driver);
> > +}
> > +
> > +static void __exit s35390a_rtc_exit(void)
> > +{
> > +     i2c_del_driver(&s35390a_driver);
> > +}
> > +
> > +MODULE_AUTHOR("Byron Bradley <byron.bbradley-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>");
> > +MODULE_DESCRIPTION("S35390A RTC driver");
> > +MODULE_LICENSE("GPL");
> > +
> > +module_init(s35390a_rtc_init);
> > +module_exit(s35390a_rtc_exit);
>
> No other comment, that's pretty clean code overall.

Thanks for the comments, I'll resend as a single patch with the
changes and i2c_new_dummy() functionality.

> Note: due to the particular way this device is accessed, I don't think
> that we want to use SMBus-level functions (as had been suggested in a
> different thread) unless we really have to for compatibility purposes.
> Your code is quite nice the way it is and I suspect that using
> SMBus-level functions would make it much more complex.

Yes I started trying to change to the smbus functions but this is
difficult because it doesn't use the command part so when writing
multiple bytes the first data byte would have to be the command. I
never tried it but I'm not sure if the block read command would work
either because it requires a command and I don't think the chip would
like the extra data. I've noticed in testing it can be quite fussy, if
you try to write to a read only register it will lock up even though
the data sheet says it will ignore it.

-- 
Byron Bradley