[PATCH] Add support for pt7c4338 (rtc device) in rtc-ds1307 driver

[PATCH] Add support for pt7c4338 (rtc device) in rtc-ds1307 driver

[Priyanka Jain]

PT7C4338 chip is being manufactured by Pericom Technology Inc.
It is a serial real-time clock which provides:
1)Low-power clock/calendar.
2)Programmable square-wave output.
It has 56 bytes of nonvolatile RAM.
Its register set is same as that of rtc device: DS1307.

Signed-off-by: Priyanka Jain <Priyanka.Jain@freescale.com>
---
 Changes :
	 This patch will supersede patch:
		"RTC driver(Linux) for PT7C4338 chip"
	 Incorporting Wolfram Sang's comments to reuse ds1307 driver.

 drivers/rtc/Kconfig      |    6 +++---
 drivers/rtc/rtc-ds1307.c |    7 +++++++
 2 files changed, 10 insertions(+), 3 deletions(-)

diff --git a/drivers/rtc/Kconfig b/drivers/rtc/Kconfig
index b8f4e9e..c6045dd 100644
--- a/drivers/rtc/Kconfig
+++ b/drivers/rtc/Kconfig
@@ -126,13 +126,13 @@ comment "I2C RTC drivers"
 if I2C
 
 config RTC_DRV_DS1307
-	tristate "Dallas/Maxim DS1307/37/38/39/40, ST M41T00, EPSON RX-8025"
+	tristate "Dallas/Maxim DS1307/37/38/39/40, ST M41T00, EPSON RX-8025, PT7C4338"
 	help
 	  If you say yes here you get support for various compatible RTC
 	  chips (often with battery backup) connected with I2C. This driver
 	  should handle DS1307, DS1337, DS1338, DS1339, DS1340, ST M41T00,
-	  EPSON RX-8025 and probably other chips. In some cases the RTC
-	  must already have been initialized (by manufacturing or a
+	  EPSON RX-8025, PT7C4338 and probably other chips. In some cases
+	  the RTC must already have been initialized (by manufacturing or a
 	  bootloader).
 
 	  The first seven registers on these chips hold an RTC, and other
diff --git a/drivers/rtc/rtc-ds1307.c b/drivers/rtc/rtc-ds1307.c
index 4724ba3..8436f16 100644
--- a/drivers/rtc/rtc-ds1307.c
+++ b/drivers/rtc/rtc-ds1307.c
@@ -4,6 +4,8 @@
  *  Copyright (C) 2005 James Chapman (ds1337 core)
  *  Copyright (C) 2006 David Brownell
  *  Copyright (C) 2009 Matthias Fuchs (rx8025 support)
+ *  Copyright (C) 2011 Priyanka Jain (Priyanka.Jain@freescale.com)
+ *                                   (pt7c4338 support)
  *
  * This program is free software; you can redistribute it and/or modify
  * it under the terms of the GNU General Public License version 2 as
@@ -34,6 +36,7 @@ enum ds_type {
 	ds_1388,
 	ds_3231,
 	m41t00,
+	pt7c4338,
 	rx_8025,
 	// rs5c372 too?  different address...
 };
@@ -137,6 +140,8 @@ static const struct chip_desc chips[] = {
 },
 [m41t00] = {
 },
+[pt7c4338] = {
+},
 [rx_8025] = {
 }, };
 
@@ -149,6 +154,7 @@ static const struct i2c_device_id ds1307_id[] = {
 	{ "ds1340", ds_1340 },
 	{ "ds3231", ds_3231 },
 	{ "m41t00", m41t00 },
+	{ "pt7c4338", pt7c4338 },
 	{ "rx8025", rx_8025 },
 	{ }
 };
@@ -769,6 +775,7 @@ read_rtc:
 	switch (ds1307->type) {
 	case ds_1307:
 	case m41t00:
+	case pt7c4338:
 		/* clock halted?  turn it on, so clock can tick. */
 		if (tmp & DS1307_BIT_CH) {
 			i2c_smbus_write_byte_data(client, DS1307_REG_SECS, 0);
-- 
1.6.5.6

Re: [PATCH] Add support for pt7c4338 (rtc device) in rtc-ds1307 driver

[Wolfram Sang]

[-- Attachment #1: Type: text/plain, Size: 690 bytes --]

On Thu, May 26, 2011 at 12:33:29PM +0530, Priyanka Jain wrote:

> PT7C4338 chip is being manufactured by Pericom Technology Inc.
> It is a serial real-time clock which provides:
> 1)Low-power clock/calendar.
> 2)Programmable square-wave output.
> It has 56 bytes of nonvolatile RAM.
> Its register set is same as that of rtc device: DS1307.
> 
> Signed-off-by: Priyanka Jain <Priyanka.Jain@freescale.com>

So it is identical to ds1307? Then why not name your platform_device
simply 'ds1307'?

Regards,

   Wolfram

-- 
Pengutronix e.K.                           | Wolfram Sang                |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 198 bytes --]

RE: [PATCH] Add support for pt7c4338 (rtc device) in rtc-ds1307 driver

[Jain Priyanka-B32167]

> -----Original Message-----
> From: linuxppc-dev-bounces+priyanka.jain=3Dfreescale.com@lists.ozlabs.org
> [mailto:linuxppc-dev-
> bounces+priyanka.jain=3Dfreescale.com@lists.ozlabs.org] On Behalf Of
> Wolfram Sang
> Sent: Thursday, May 26, 2011 2:40 PM
> To: Jain Priyanka-B32167
> Cc: a.zummo@towertech.it; akpm@linux-foundation.org; linuxppc-
> dev@lists.ozlabs.org; rtc-linux@googlegroups.com; p_gortmaker@yahoo.com
> Subject: Re: [PATCH] Add support for pt7c4338 (rtc device) in rtc-ds1307
> driver
>=20
> On Thu, May 26, 2011 at 12:33:29PM +0530, Priyanka Jain wrote:
>=20
> > PT7C4338 chip is being manufactured by Pericom Technology Inc.
> > It is a serial real-time clock which provides:
> > 1)Low-power clock/calendar.
> > 2)Programmable square-wave output.
> > It has 56 bytes of nonvolatile RAM.
> > Its register set is same as that of rtc device: DS1307.
> >
> > Signed-off-by: Priyanka Jain <Priyanka.Jain@freescale.com>
>=20
> So it is identical to ds1307? Then why not name your platform_device
> simply 'ds1307'?
Yes, It is possible to directly use platform device 'ds1307' in device tree=
.=20
But I have one query of how to capture the information that pericom, pt7c43=
38 device is compatible with dallas, ds1307. Can it be done in somewhere in=
 some document or some code. Actually for pt7c4338 device driver, I actuall=
y started by asking if there is any already existing rtc device driver whic=
h is compatible with pericom pt7c4338 device on mailing list and as there w=
as no answer or help then, I actually ended up writing device driver for th=
at and then on the suggestion using ds1307 device driver for this.

Thanks
Priyanka
>=20
> Regards,
>=20
>    Wolfram
>=20
> --
> Pengutronix e.K.                           | Wolfram Sang
> |
> Industrial Linux Solutions                 | http://www.pengutronix.de/
> |

Re: [PATCH] Add support for pt7c4338 (rtc device) in rtc-ds1307 driver

[Wolfram Sang]

[-- Attachment #1: Type: text/plain, Size: 1455 bytes --]

Hi, Priyanka,

> Yes, It is possible to directly use platform device 'ds1307' in device
> tree. 

Great, thanks for testing.

> But I have one query of how to capture the information that pericom,
> pt7c4338 device is compatible with dallas, ds1307. Can it be done in
> somewhere in some document or some code. Actually for pt7c4338 device

Yes, it should definately be documented in the source.

> driver, I actually started by asking if there is any already existing
> rtc device driver which is compatible with pericom pt7c4338 device on
> mailing list and as there was no answer or help then, I actually ended
> up writing device driver for that 

The first place where this should be mentioned is the datasheet of the
pt-chip, so you might ask the producer to add this information (don't
expect much to happen, though). For such generic devices, it is then
useful to look for similar register sets in existing drivers to find a
duplicate. Please don't forget that such lists are voluntary, there is
no guarantee that mails get replied to.

> and then on the suggestion using ds1307 device driver for this.

IIRC I asked you explicitly for the differences between the chips. If
there are none, you can use the driver directly, right? :)

Regards,

   Wolfram

-- 
Pengutronix e.K.                           | Wolfram Sang                |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 198 bytes --]

Re: [rtc-linux] Re: [PATCH] Add support for pt7c4338 (rtc device) in rtc-ds1307 driver

[Tabi Timur-B04825]

On Mon, May 30, 2011 at 3:24 AM, Wolfram Sang <w.sang@pengutronix.de> wrote=
:

> The first place where this should be mentioned is the datasheet of the
> pt-chip, so you might ask the producer to add this information (don't
> expect much to happen, though).

It's true that the data sheet does not mention that it's identical to
the DS1307, but that's still no excuse for not noticing it and writing
a whole driver for it.  :-(

> IIRC I asked you explicitly for the differences between the chips. If
> there are none, you can use the driver directly, right? :)

Yes.  The device tree node for the PT7C4338 device should just say

   /* The board has a PT7C4338, which is compatible with the DS1307 */
   compatible =3D "dallas,ds1307";

And that's it.

--=20
Timur Tabi
Linux kernel developer at Freescale=

Re: [rtc-linux] Re: [PATCH] Add support for pt7c4338 (rtc device) in rtc-ds1307 driver

[Wolfram Sang]

[-- Attachment #1: Type: text/plain, Size: 618 bytes --]


> > IIRC I asked you explicitly for the differences between the chips. If
> > there are none, you can use the driver directly, right? :)
> 
> Yes.  The device tree node for the PT7C4338 device should just say
> 
>    /* The board has a PT7C4338, which is compatible with the DS1307 */
>    compatible = "dallas,ds1307";
> 
> And that's it.

I'd also suggest to add a comment to the id_table in rtc-ds1307.c, so
the chip name can be grepped for.

-- 
Pengutronix e.K.                           | Wolfram Sang                |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 198 bytes --]

Re: [rtc-linux] Re: [PATCH] Add support for pt7c4338 (rtc device) in rtc-ds1307 driver

[Anton Vorontsov]

On Mon, May 30, 2011 at 02:29:58PM +0000, Tabi Timur-B04825 wrote:
> On Mon, May 30, 2011 at 3:24 AM, Wolfram Sang <w.sang@pengutronix.de> wrote:
> 
> > The first place where this should be mentioned is the datasheet of the
> > pt-chip, so you might ask the producer to add this information (don't
> > expect much to happen, though).
> 
> It's true that the data sheet does not mention that it's identical to
> the DS1307, but that's still no excuse for not noticing it and writing
> a whole driver for it.  :-(
> 
> > IIRC I asked you explicitly for the differences between the chips. If
> > there are none, you can use the driver directly, right? :)
> 
> Yes.  The device tree node for the PT7C4338 device should just say
> 
>    /* The board has a PT7C4338, which is compatible with the DS1307 */
>    compatible = "dallas,ds1307";

While it seems to be 100% compatible, there could be chip-specific
bugs or some interesting features that are hidden behind "reserved"
bits and registers.

So I think device tree should not lie about the chip model. Doing
'compatible = "pericom,pt7c4338", "dallas,ds1307"' is perfectly fine.

Note that today the several compatible entries approach gives you
almost nothing, as you will need to add pt7c4338 entry into the driver
anyway.

I tried to improve this, i.e. make linux do OF-matching on the most
generic compatible entry (the last one):

http://www.mail-archive.com/linuxppc-dev@lists.ozlabs.org/msg21196.html

It was received coldly though:

http://www.mail-archive.com/linuxppc-dev@lists.ozlabs.org/msg22041.html
http://www.mail-archive.com/linuxppc-dev@lists.ozlabs.org/msg21273.html

-- 
Anton Vorontsov
Email: cbouatmailru@gmail.com

Re: [rtc-linux] Re: [PATCH] Add support for pt7c4338 (rtc device) in rtc-ds1307 driver

[Grant Likely]

On Mon, May 30, 2011 at 08:57:45PM +0400, Anton Vorontsov wrote:
> On Mon, May 30, 2011 at 02:29:58PM +0000, Tabi Timur-B04825 wrote:
> > On Mon, May 30, 2011 at 3:24 AM, Wolfram Sang <w.sang@pengutronix.de> wrote:
> > 
> > > The first place where this should be mentioned is the datasheet of the
> > > pt-chip, so you might ask the producer to add this information (don't
> > > expect much to happen, though).
> > 
> > It's true that the data sheet does not mention that it's identical to
> > the DS1307, but that's still no excuse for not noticing it and writing
> > a whole driver for it.  :-(
> > 
> > > IIRC I asked you explicitly for the differences between the chips. If
> > > there are none, you can use the driver directly, right? :)
> > 
> > Yes.  The device tree node for the PT7C4338 device should just say
> > 
> >    /* The board has a PT7C4338, which is compatible with the DS1307 */
> >    compatible = "dallas,ds1307";
> 
> While it seems to be 100% compatible, there could be chip-specific
> bugs or some interesting features that are hidden behind "reserved"
> bits and registers.
> 
> So I think device tree should not lie about the chip model. Doing
> 'compatible = "pericom,pt7c4338", "dallas,ds1307"' is perfectly fine.

Correct.  It's fine (and encouraged) to claim compatibility, but the
node should always specify the exact part in the compatible list.

> 
> Note that today the several compatible entries approach gives you
> almost nothing, as you will need to add pt7c4338 entry into the driver
> anyway.
> 
> I tried to improve this, i.e. make linux do OF-matching on the most
> generic compatible entry (the last one):
> 
> http://www.mail-archive.com/linuxppc-dev@lists.ozlabs.org/msg21196.html
> 
> It was received coldly though:
> 
> http://www.mail-archive.com/linuxppc-dev@lists.ozlabs.org/msg22041.html
> http://www.mail-archive.com/linuxppc-dev@lists.ozlabs.org/msg21273.html

On that note, device-tree-style of_match_table binding now works for
i2c devices, so the problems you were having with that thread should
now be solved.

The of_find_i2c_driver() approach was only ever a heuristic to get
things working in the short term.  of_match_table is is better in the
long run.

g.