Added support of ST m41t85 rtc chip

PATCH In review Andrey Volkov 5 participants 28 messages Nov 14, 2005 – Jan 26, 2006 3 messages not in the archive

lore

Share & tools

lore

Andrey Volkov

author

2005-11-14 13:50 UTC #0 lore raw

PATCH 1/1 Added support of ST m41t85 rtc chip

Hello Jean,

Possible too late to include in 2.6.15,
but better later then never :).

Comments?

Signature · 4 lines

Regards
Andrey Volkov

P.S. Please CC to me directly, I'm not in [email protected] list.

Added support of ST M41T85 RTC

Signed-off-by Andrey Volkov <[email protected]>

---

 drivers/i2c/chips/Kconfig  |   88 ++++++++++++++
 drivers/i2c/chips/Makefile |    1 
 drivers/i2c/chips/m41t85.c |  285 ++++++++++++++++++++++++++++++++++++++++++++
 include/linux/i2c-id.h     |    1 
 4 files changed, 375 insertions(+), 0 deletions(-)

drivers/i2c/chips/Kconfig +88 -0

index f9fae28..6593fc5 100644
@@ -111,6 +111,94 @@ config SENSORS_M41T00
 	  This driver can also be built as a module.  If so, the module
 	  will be called m41t00.
 
+config SENSORS_M41T85
+	tristate "ST M41T85 RTC chip"
+	depends on I2C
+	help
+	  If you say yes here you get support for the ST M41T85 RTC chip.
+
+	  This driver can also be built as a module.  If so, the module
+	  will be called m41t85.
+
+config SENSORS_M41T85_SQW_FRQ_ENABLE
+	depends on SENSORS_M41T85
+	bool "Square Wave Output"
+	help
+	  If you say yes here, then m41t85 will generate clocks on it 
+	  SQW pin at frequency which you are select below.
+
+choice 
+	prompt "SQW frequincy"
+	depends on SENSORS_M41T85_SQW_FRQ_ENABLE
+	default SENSORS_M41T85_SQW_FRQ_32KHZ
+
+config SENSORS_M41T85_SQW_FRQ_32KHZ
+	bool "32.768 kHz"
+
+config SENSORS_M41T85_SQW_FRQ_8KHZ
+	bool "8 kHz"
+
+config SENSORS_M41T85_SQW_FRQ_4KHZ
+	bool "4 kHz"
+
+config SENSORS_M41T85_SQW_FRQ_2KHZ
+	bool "2 kHz"
+
+config SENSORS_M41T85_SQW_FRQ_1KHZ
+	bool "1 kHz"
+
+config SENSORS_M41T85_SQW_FRQ_512HZ
+	bool "512 Hz"
+
+config SENSORS_M41T85_SQW_FRQ_256HZ
+	bool "256 Hz"
+
+config SENSORS_M41T85_SQW_FRQ_128HZ
+	bool "128 Hz"
+
+config SENSORS_M41T85_SQW_FRQ_64HZ
+	bool "64 Hz"
+
+config SENSORS_M41T85_SQW_FRQ_32HZ
+	bool "32 Hz"
+
+config SENSORS_M41T85_SQW_FRQ_16HZ
+	bool "16 Hz"
+
+config SENSORS_M41T85_SQW_FRQ_8HZ
+	bool "8 Hz"
+
+config SENSORS_M41T85_SQW_FRQ_4HZ
+	bool "4 Hz"
+
+config SENSORS_M41T85_SQW_FRQ_2HZ
+	bool "2 Hz"
+
+config SENSORS_M41T85_SQW_FRQ_1HZ
+	bool "1 Hz"
+
+endchoice
+
+config SENSORS_M41T85_SQW_FRQ
+	int
+	depends on SENSORS_M41T85_SQW_FRQ_ENABLE
+	default "1" if SENSORS_M41T85_SQW_FRQ_32KHZ
+	default "2" if SENSORS_M41T85_SQW_FRQ_8KHZ
+	default "3" if SENSORS_M41T85_SQW_FRQ_4KHZ
+	default "4" if SENSORS_M41T85_SQW_FRQ_2KHZ
+	default "5" if SENSORS_M41T85_SQW_FRQ_1KHZ
+	default "6" if SENSORS_M41T85_SQW_FRQ_512HZ
+	default "7" if SENSORS_M41T85_SQW_FRQ_256HZ
+	default "8" if SENSORS_M41T85_SQW_FRQ_128HZ
+	default "9" if SENSORS_M41T85_SQW_FRQ_64HZ
+	default "10" if SENSORS_M41T85_SQW_FRQ_32HZ
+	default "11" if SENSORS_M41T85_SQW_FRQ_16HZ
+	default "12" if SENSORS_M41T85_SQW_FRQ_8HZ
+	default "13" if SENSORS_M41T85_SQW_FRQ_4HZ
+	default "14" if SENSORS_M41T85_SQW_FRQ_2HZ
+	default "15" if SENSORS_M41T85_SQW_FRQ_1HZ
+
+
 config SENSORS_MAX6875
 	tristate "Maxim MAX6875 Power supply supervisor"
 	depends on I2C && EXPERIMENTAL

drivers/i2c/chips/Makefile +1 -0

index 46178b5..5637881 100644
@@ -14,6 +14,7 @@ obj-$(CONFIG_SENSORS_RTC8564)	+= rtc8564
 obj-$(CONFIG_ISP1301_OMAP)	+= isp1301_omap.o
 obj-$(CONFIG_TPS65010)		+= tps65010.o
 obj-$(CONFIG_RTC_X1205_I2C)	+= x1205.o
+obj-$(CONFIG_SENSORS_M41T85)	+= m41t85.o
 
 ifeq ($(CONFIG_I2C_DEBUG_CHIP),y)
 EXTRA_CFLAGS += -DDEBUG

drivers/i2c/chips/m41t85.c new file +285 -0

285 lines not on this page · raw message

include/linux/i2c-id.h +1 -0

index 5c05f52..ac8ffcd 100644
@@ -108,6 +108,7 @@
 #define I2C_DRIVERID_SAA7127	72	/* saa7124 video encoder	*/
 #define I2C_DRIVERID_SAA711X	73	/* saa711x video encoders	*/
 #define I2C_DRIVERID_AKITAIOEXP	74	/* IO Expander on Sharp SL-C1000 */
+#define I2C_DRIVERID_STM41T85	75	/* real time clock		*/
 
 #define I2C_DRIVERID_EXP0	0xF0	/* experimental use id's	*/
 #define I2C_DRIVERID_EXP1	0xF1

Jean Delvare

2005-11-15 20:51 UTC #3 lore raw

In reply to Andrey Volkov

Hi Andrey,

Quoted 2 lines

Possible too late to include in 2.6.15,
but better later then never :).

You must be kidding. It might be too late for 2.6._16_. Reviewing takes time, reworking afterwards takes time, then you get some testing in -mm and it takes time again.

Quoted 1 line

Comments?

Sure, although I don't really have the time right now for a complete review. And I'd rather not review the code if it finally isn't used.

First, a question. Can't you merge the M41T85 support into the m41t00 driver?

Mark, care to comment on that possibility, and/or on the code itself?

drivers/i2c/chips/Kconfig config SENSORS_M41T00 · line 116

Quoted 3 lines

drivers/i2c/chips/Kconfig +3 -0

+config SENSORS_M41T85
+	tristate "ST M41T85 RTC chip"
+	depends on I2C

I would pretty much likt it named RTC_M41T85 or RTC_M41T85_I2C instead. It's not a sensor in any way. And it must depend on EXPERIMENTAL to start with.

drivers/i2c/chips/Kconfig config SENSORS_M41T00 · line 125

Quoted 3 lines

drivers/i2c/chips/Kconfig +3 -0

+config SENSORS_M41T85_SQW_FRQ_ENABLE
+	depends on SENSORS_M41T85
+	bool "Square Wave Output"

What a mess. Please just have a sysfs file for that, it's more flexible and less intrusive.

Quoted 3 lines

+/*
+ * drivers/i2c/chips/m41t85.c
+ *

Redundant, you know where the file is if you found it.

Quoted 3 lines

+ * HISTORY:
+ * 2005-10-12 Created
+ */

History doesn't belong to the source code.

Quoted 7 lines

+/*
+ * This i2c client/driver wedges between the drivers/char/genrtc.c RTC
+ * interface and the SMBus interface of the i2c subsystem.
+ * It would be more efficient to use i2c msgs/i2c_transfer directly but, as
+ * recommened in .../Documentation/i2c/writing-clients section
+ * "Sending and receiving", using SMBus level communication is preferred.
+ */

The i2c_transfer interface wouldn't exist if nobody was ever supposed to use it. Using the SMBus compatible interface makes it possible to use your chip in conjunction with more I2C or SMBus masters. But maybe you just need a specific driver for a specific case, and you just don't care about the other cases. In that case, go with i2c_transfer for smaller and faster code.

Also, use <file:Documentation/...> for referencing document files.

Quoted 1 line

+#define M41T85_DRV_NAME "m41t85"

Please define m41t85_driver.name directly, and use references to it where needed. This avoids duplicating string constants.

drivers/i2c/chips/m41t85.c line 142

Quoted 5 lines

drivers/i2c/chips/m41t85.c new file +5 -0

+static void
+m41t85_set_tlet(ulong arg)
+{
+	struct rtc_time	tm;
+	ulong	nowtime = *(ulong *)arg;

What's the idea behind this cast?

drivers/i2c/chips/m41t85.c line 208

Quoted 1 line

drivers/i2c/chips/m41t85.c new file +1 -0
+	strncpy(client->name, M41T85_DRV_NAME, I2C_NAME_SIZE);

Please use strlcpy instead.

drivers/i2c/chips/m41t85.c line 278

Quoted 6 lines

drivers/i2c/chips/m41t85.c new file +6 -0

+static void __exit
+m41t85_exit(void)
+{
+	i2c_del_driver(&m41t85_driver);
+	return;
+}

Drop this "return" statement, it is useless.

Thanks,

Signature · 1 line

Jean Delvare

Andrey Volkov

author

2005-11-15 21:24 UTC #2 lore raw

In reply to Andrew Morton

Quoted 7 lines · Andrew Morton

Quoted 5 lines · Andrey Volkov
...
Added support of ST M41T85 RTC
...

Quoted 7 lines

drivers/i2c/chips/m41t85.c line 80
Quoted 2 lines
drivers/i2c/chips/m41t85.c new file +2 -0
+ulong
+m41t85_get_rtc_time(void)
Does this need to have global scope?
It appears to have no callers.

I use this function(s) in platform driver (which still in dev stage) of our board as platform get_rtc_time/set_rtc_time by same way as Mark in the katana does.

May be better create special header in include/linux? (And convert m41t80.c and arch/ppc/katana.c too)

Quoted 13 lines

drivers/i2c/chips/m41t85.c new file +13 -0

+static void
+m41t85_set_tlet(ulong arg)
+{
+	struct rtc_time	tm;
+	ulong	nowtime = *(ulong *)arg;
+
+	to_tm(nowtime, &tm);
+	tm.tm_year = (tm.tm_year - 1900) % 100;
+
+	tm.tm_sec = BIN2BCD(tm.tm_sec);
+	tm.tm_min = BIN2BCD(tm.tm_min);
+	tm.tm_hour = BIN2BCD(tm.tm_hour);
+	tm.tm_mon = BIN2BCD(tm.tm_mon);

Quoted 8 lines

drivers/i2c/chips/m41t85.c line 154
Quoted 4 lines
drivers/i2c/chips/m41t85.c new file +4 -0
+	tm.tm_mday = BIN2BCD(tm.tm_mday);
+	tm.tm_year = BIN2BCD(tm.tm_year);
+
+	down(&m41t85_mutex);
Cannot do down() in a tasklet handler! Enable CONFIG_DEBUG_PREEMPT and CONFIG_DEBUG_SPINLOCK_SLEEP, retest.

Oops, you're right. It's copy-paste bug from m41t00.c (which then buggy too).

Quoted 4 lines

schedule_work() might be an appropriate fix.

Quoted 15 lines

drivers/i2c/chips/m41t85.c line 188
Quoted 12 lines
drivers/i2c/chips/m41t85.c new file +12 -0
+int
+m41t85_set_rtc_time(ulong nowtime)
+{
+	new_time = nowtime;
+
+	if (in_interrupt())
+		tasklet_schedule(&m41t85_tasklet);
+	else
+		m41t85_set_tlet((ulong)&new_time);
+
+	return 0;
+}
hm, this function isn't referenced from within this patch either.

Same as above.

Quoted 8 lines

drivers/i2c/chips/m41t85.c line 218
Quoted 3 lines
drivers/i2c/chips/m41t85.c new file +1 -0
+	#if defined (CONFIG_SENSORS_M41T85_SQW_FRQ)
#if's normally start in column zero.

Quoted 4 lines

drivers/i2c/chips/m41t85.c line 221
Quoted 1 line
drivers/i2c/chips/m41t85.c new file +1 -0
+		ret = i2c_smbus_write_byte_data(client, RTC_SQW_ADDR, CONFIG_SENSORS_M41T85_SQW_FRQ);
My, what large xterms you have ;)

Tabs=4 and 1280 full screened :). Ok I fix it to 80 columns.

--
Regards
Andrey Volkov

Andrey Volkov

author

2005-11-15 21:49 UTC #4 lore raw

In reply to Jean Delvare

Quoted 11 lines · Jean Delvare

Hi Andrey,
Quoted 2 lines
Possible too late to include in 2.6.15,
but better later then never :).
You must be kidding. It might be too late for 2.6._16_. Reviewing takes time, reworking afterwards takes time, then you get some testing in -mm and it takes time again.

:((.

Quoted 6 lines

Quoted 2 lines
Comments?
Sure, although I don't really have the time right now for a complete review. And I'd rather not review the code if it finally isn't used.

(see my prev. reply to Andrew and you)

Quoted 3 lines

First, a question. Can't you merge the M41T85 support into the m41t00 driver?

It was first thing what I try, but this chips are very similar only at first glance. m41t85 have _really_ extended sets of regs and result was very littered by #if/#else file.

Quoted 3 lines

Mark, care to comment on that possibility, and/or on the code itself?

And, please, remove unnecessary PPC dependence from Kconfig.

Quoted 7 lines

drivers/i2c/chips/Kconfig config SENSORS_M41T00 · line 125
Quoted 3 lines
drivers/i2c/chips/Kconfig +3 -0
+config SENSORS_M41T85_SQW_FRQ_ENABLE
+	depends on SENSORS_M41T85
+	bool "Square Wave Output"
What a mess. Please just have a sysfs file for that, it's more flexible and less intrusive.

I agree, it's look messed, but if sombody use SQW, then must exist some startup constant for some custom board. Changing this frq may exist only as option.

--
Regards
Andrey Volkov

Mark A. Greer

2005-11-16 02:56 UTC #12 lore raw

In reply to Jean Delvare

Hi Jean,

On Tue, Nov 15, 2005 at 09:52:26PM +0100, Jean Delvare wrote:

<snip>

Quoted 4 lines

First, a question. Can't you merge the M41T85 support into the m41t00 driver?
Mark, care to comment on that possibility, and/or on the code itself?

Sure.

I wrestled with the ST website for the m41t85 datasheet but lost so I I can only guess from the patch. The drivers do look very similar. It looks like the m41t85 is basically a m41t00 with an alarm (watchdog timer never used AFAICT). Also there are some differences in register offsets and [maybe] some minor differences within the registers but nothing that serious.

I think we can combine the two into an m41txx.c and pass the exact type in via platform_data--that would be the correct mechanism, right? The platform_data could also be used to seed the correct SQW freq and eliminate all the Kconfig noise.

Comments?

As for Jean's and Andrew's comments about the driver, they seem valid to me and should be addressed. In Andrey's defense, many of them are my fault. Once there is a consensus on the merging m41t00 & m41t85 question, I'll try to get a fixed up patch within a couple weeks.

Mark

Mark A. Greer

2005-11-16 03:14 UTC #5 lore raw

In reply to Andrey Volkov

On Wed, Nov 16, 2005 at 12:48:59AM +0300, Andrey Volkov wrote:

<snip>

Quoted 4 lines

Quoted 3 lines
Mark, care to comment on that possibility, and/or on the code itself?
And, please, remove unnecessary PPC dependence from Kconfig.

When I originally submitted the m41t00 patch, I made it clear that it was PPC only and gave the reason why:

http://archives.andrew.net.au/lm-sensors/msg29280.html

What processor arch did you test your m41t85 driver on? AFAICT, PPC is the only arch that uses that exact definition of get/set_rtc_time().

Mark

Andrey Volkov

author

2005-11-16 14:45 UTC #13 lore raw

In reply to Mark A. Greer

Quoted 12 lines · Mark A. Greer

Hi Jean,
On Tue, Nov 15, 2005 at 09:52:26PM +0100, Jean Delvare wrote:
<snip>
Quoted 4 lines
First, a question. Can't you merge the M41T85 support into the m41t00 driver?
Mark, care to comment on that possibility, and/or on the code itself?

Quoted 6 lines

Sure.
I wrestled with the ST website for the m41t85 datasheet but lost so I I can only guess from the patch. The drivers do look very similar. It looks like the m41t85 is basically a m41t00 with an alarm (watchdog timer never used AFAICT).

http://www.st.com/stonline/products/literature/ds/7531/m41st85w.pdf
(I agree ST's (and Maxim's too) site is for strength of mind men :) ). Also added onchip clock, battery and poweroff time
(HT == Halt Timestamp).

Quoted 3 lines

Also there are some differences in register
offsets and [maybe] some minor differences within the registers but nothing that serious.

Mainly you're right, but, as I wrote before, due to _many_ little differences I get #if/#else.. noise (half file, if be more precisely, was under #if/#else) in unified file. But, if this noise will be acceptable, then yes, I agree to merge this drivers, as minimum, for better administration.

<snip>

Signature · 2 lines

Regards
Andrey Volkov

Andrey Volkov

author

2005-11-16 14:50 UTC #6 lore raw

In reply to Mark A. Greer

Quoted 6 lines · Mark A. Greer

When I originally submitted the m41t00 patch, I made it clear that it was PPC only and gave the reason why:
http://archives.andrew.net.au/lm-sensors/msg29280.html
What processor arch did you test your m41t85 driver on?

PPC32 :) (MPC5200).

Quoted 2 lines

AFAICT, PPC is the only arch that uses that exact definition of get/set_rtc_time().

But chip (and driver) itself may be used in any i2c net with any chip as master with slightly modification of platform driver. Am I right?.

Signature · 2 lines

Regards
Andrey Volkov

Jean Delvare

2005-11-16 15:34 UTC #14 lore raw

In reply to Andrey Volkov

Hi Andrey,

Quoted 7 lines · Andrey Volkov

Quoted 4 lines
I wrestled with the ST website for the m41t85 datasheet but lost so I I can only guess from the patch. The drivers do look very similar. It looks like the m41t85 is basically a m41t00 with an alarm (watchdog timer never used AFAICT).
http://www.st.com/stonline/products/literature/ds/7531/m41st85w.pdf
(I agree ST's (and Maxim's too) site is for strength of mind men :) ).

We're sliding off-topic, but I find Maxim's website quite good.

Quoted 9 lines

Quoted 3 lines
Also there are some differences in register
offsets and [maybe] some minor differences within the registers but nothing that serious.
Mainly you're right, but, as I wrote before, due to _many_ little differences I get #if/#else.. noise (half file, if be more precisely, was under #if/#else) in unified file. But, if this noise will be acceptable, then yes, I agree to merge this drivers, as minimum, for better administration.

This simply means you did it the wrong way. The use of #if/#else/#endif suggests that you made the distinction between chips at compilation time, and the generated driver binary ended up supporting only one of the two chips. This doesn't make any sense. Same is true for your output clock frequency, making it a kernel configuration option means that the decision happens at compilation time, which is very restrictive.

Think of what will happen when a Linux distribution has to build a kernel for their next release. How are they going to build a kernel suitable for all systems? With your approach, they just won't be able to. They'd have to build one kernel with m41t00 support, and one with m41t85 support. Even worse, they would need one separate build for each variant of the m41t85 driver, and there are over a dozen of these (one without clock output, and one more for each allowed clock frequency). That's obviously insane.

This is why you want to move the decision at the run time level rather than the compilation time level whenever possible. The clock output option can't be a configuration option. I previously suggested a sysfs file, because this gives the greatest flexibility. If you don't like the idea for whatever reason, you may go for a module parameter instead.

Same reasonment holds for the m41t00 vs. m41t85 choice. You can't decide at compilation time. If we go for a common driver, it has to support both devices at the same time. Mark suggested to use platform-specific data. I'm not familiar with this, but it sounds reasonable.

I don't know for sure at this point whether having a single driver is the right choice, I'll let you and Mark check it out and decide. But the right way to determine this is definitely not through the use of #if/#endif preprocessing stuff.

Thanks,
--
Jean Delvare

Andrey Volkov

author

2005-11-16 16:43 UTC #15 lore raw

Hi Jean

Quoted 9 lines · Jean Delvare

Hi Andrey,
Quoted 5 lines · Andrey Volkov
Quoted 4 lines
I wrestled with the ST website for the m41t85 datasheet but lost so I I can only guess from the patch. The drivers do look very similar. It looks like the m41t85 is basically a m41t00 with an alarm (watchdog timer never used AFAICT).

Quoted 5 lines

Quoted 2 lines
http://www.st.com/stonline/products/literature/ds/7531/m41st85w.pdf
(I agree ST's (and Maxim's too) site is for strength of mind men :) ).
We're sliding off-topic, but I find Maxim's website quite good.

Try select chip by interface :) (Timekeeping & RTC as ex). But Maxim site is better, it's not disputed.

Quoted 12 lines

Quoted 11 lines
Quoted 5 lines
Also there are some differences in register
offsets and [maybe] some minor differences within the registers but nothing that serious.
Mainly you're right, but, as I wrote before, due to _many_ little differences I get #if/#else.. noise (half file, if be more precisely, was under #if/#else) in unified file. But, if this noise will be acceptable, then yes, I agree to merge this drivers, as minimum, for better administration.

<snip>

Quoted 4 lines

Same reasonment holds for the m41t00 vs. m41t85 choice. You can't decide at compilation time. If we go for a common driver, it has to support both devices at the same time. Mark suggested to use platform-specific data. I'm not familiar with this, but it sounds reasonable.

It's pity, but all chips of m41xx family (approx. 20 members) have same i2c slave address (0x68) and have not some hwd model specific id registers. Hence I couldn't made any clue about chip specific regs/pins/... at run-time (and couldn't use two or more chips in one i2c subnet :)). And the situation worsens, as various chips have various time regs offsets, as ex. REG_SEC of m41t00 have offset 0, but in m41t85 _SAME_ REG_SEC have offset 1, etc (God, who in ST was so, hm, ...., so made such decision?).

So I coldn't see any suitable run-time method of detection which function driver could use for current "unknown" m41xx chip (from driver probe point of view) whithout Kconfig/module parameter prompting.

Quoted 5 lines

I don't know for sure at this point whether having a single driver is the right choice, I'll let you and Mark check it out and decide. But the right way to determine this is definitely not through the use of #if/#endif preprocessing stuff.

IMHO - single driver for all m41xx will be nice thing, but I'm not sure that it will not be monster- alike when it be released.

Signature · 2 lines

Regards
Andrey Volkov

Andy Isaacson

2005-11-16 18:55 UTC #7 lore raw

In reply to Mark A. Greer

Quoted 9 lines · Mark A. Greer

Quoted 3 lines · Andrey Volkov
Quoted 2 lines
Mark, care to comment on that possibility, and/or on the code itself?
And, please, remove unnecessary PPC dependence from Kconfig.
When I originally submitted the m41t00 patch, I made it clear that it was PPC only and gave the reason why:
http://archives.andrew.net.au/lm-sensors/msg29280.html

I have a MIPS platform with M41T81 (SiByte SWARM, arch/mips/sibyte/swarm/rtc_m41t81.c) that I would dearly love to decruftify. (Don't pay too much attention to the kernel.org trees for MIPS, you need to pull the git repo on linux-mips.org for the real stuff.)

So I'm definitely interested in this work.

-andy

Mark A. Greer

2005-11-16 21:24 UTC #18 lore raw

In reply to Jean Delvare

On Wed, Nov 16, 2005 at 04:19:58PM +0100, Jean Delvare wrote:

<snip>

Quoted 10 lines

This is why you want to move the decision at the run time level rather than the compilation time level whenever possible. The clock output option can't be a configuration option. I previously suggested a sysfs file, because this gives the greatest flexibility. If you don't like the idea for whatever reason, you may go for a module parameter instead.
Same reasonment holds for the m41t00 vs. m41t85 choice. You can't decide at compilation time. If we go for a common driver, it has to support both devices at the same time. Mark suggested to use platform-specific data. I'm not familiar with this, but it sounds reasonable.

I don't know the entire history behind platform_data but my understanding is that it was designed to provide a mechanism for platform-specific code to pass info to drivers.

I *believe* that this would be a proper use of platform_data but I'm hoping someone out in lkml-land who knows better than I will confirm that.

Quoted 4 lines

I don't know for sure at this point whether having a single driver is the right choice, I'll let you and Mark check it out and decide. But the right way to determine this is definitely not through the use of #if/#endif preprocessing stuff.

I agree. We can and absolutely should do this at run-time if a merged driver is feasible. I'll dig thru the datasheets today and start prototyping some code if it looks like we can merge the code.

Mark

Mark A. Greer

2005-11-16 21:35 UTC #16 lore raw

In reply to Andrey Volkov

On Wed, Nov 16, 2005 at 07:43:29PM +0300, Andrey Volkov wrote:

<snip>

Quoted 8 lines

Quoted 4 lines
Same reasonment holds for the m41t00 vs. m41t85 choice. You can't decide at compilation time. If we go for a common driver, it has to support both devices at the same time. Mark suggested to use platform-specific data. I'm not familiar with this, but it sounds reasonable.
It's pity, but all chips of m41xx family (approx. 20 members) have same i2c slave address (0x68) and have not some hwd model specific id registers. Hence I couldn't made any clue about chip specific regs/pins/... at run-time

The hope is that your platform determines the type of rtc chip and then passes that into the m41txx driver. If your platform can have different rtc chips and no way to determine which one is there at runtime, we have a problem.

Quoted 5 lines

(and couldn't use two or more chips in one i2c subnet :)). And the situation worsens, as various chips have various time regs offsets, as ex. REG_SEC of m41t00 have offset 0, but in m41t85 _SAME_ REG_SEC have offset 1, etc (God, who in ST was so, hm, ...., so made such decision?).

I'll peruse the datasheets to see how feasible it is to merge. At this point, I think its possible but that may be my ignorance talking.

Quoted 3 lines

So I coldn't see any suitable run-time method of detection which function driver could use for current "unknown" m41xx chip (from driver probe point of view) whithout Kconfig/module parameter prompting.

See above.

Quoted 6 lines

Quoted 4 lines
I don't know for sure at this point whether having a single driver is the right choice, I'll let you and Mark check it out and decide. But the right way to determine this is definitely not through the use of #if/#endif preprocessing stuff.
IMHO - single driver for all m41xx will be nice thing, but I'm not sure that it will not be monster- alike when it be released.

Understood. I'll see what I can come up with. If it looks reasonable, I'll run it past everyone.

Mark

Mark A. Greer

2005-11-16 22:23 UTC #8 lore raw

In reply to Andy Isaacson

Quoted 11 lines · Andy Isaacson

Quoted 9 lines · Mark A. Greer
Quoted 3 lines · Andrey Volkov
Quoted 2 lines
Mark, care to comment on that possibility, and/or on the code itself?
And, please, remove unnecessary PPC dependence from Kconfig.
When I originally submitted the m41t00 patch, I made it clear that it was PPC only and gave the reason why:
http://archives.andrew.net.au/lm-sensors/msg29280.html

Quoted 7 lines

I have a MIPS platform with M41T81 (SiByte SWARM, arch/mips/sibyte/swarm/rtc_m41t81.c) that I would dearly love to decruftify. (Don't pay too much attention to the kernel.org trees for MIPS, you need to pull the git repo on linux-mips.org for the real stuff.)
So I'm definitely interested in this work.

Thanks Andy. I'll check out the m41t81 datasheet and see how well it blends with the others. Also, ppc and mips will have different interfaces but it should be acceptable to have two sets of interface routines with #ifdef CONFIG_PPC/MIPS around them.

I'm off to read datasheets now... :)

Mark

Jean Delvare

2005-11-17 09:34 UTC #17 lore raw

In reply to Andrey Volkov

Hi Andrey,

Quoted 8 lines · Andrey Volkov

It's pity, but all chips of m41xx family (approx. 20 members) have same i2c slave address (0x68) and have not some hwd model specific id registers. Hence I couldn't made any clue about chip specific regs/pins/... at run-time (and couldn't use two or more chips in one i2c subnet :)). And the situation worsens, as various chips have various time regs offsets, as ex. REG_SEC of m41t00 have offset 0, but in m41t85 _SAME_ REG_SEC have offset 1, etc (God, who in ST was so, hm, ...., so made such decision?).

I wholeheartedly second your views, it would be way way better if all I2C chips had an ID register. However, having a separate driver for every chip doesn't actually solve the undifferenciability issue. You can't prevent the user from loading the wrong driver. Also, the problem isn't specific to the m41txx family of chips, many other families of chips suffer the same.

The main problem here is that i2c-core currently lacks a mechanism which would let us explicitely attach a given driver (and chip type within that driver) to an arbitrary adapter, address pair. I will be working on implementing this with the help of others in the next few weeks. RTC drivers and media/video drivers should enjoy this new interface.

Thanks,
--
Jean Delvare

Mark A. Greer

2005-11-18 20:35 UTC #9 lore raw

In reply to Mark A. Greer

Quoted 1 line · Mark A. Greer

I'm off to read datasheets now... :)

Well, the people who sign my paychecks wanted me to do something else for the last couple days so I haven't looked at this yet. And now I'm leaving soon and will be taking next week off so I won't even start on this until the week of Nov 28. Sorry...

Mark

Andrey Volkov

author

2005-11-21 12:35 UTC #10 lore raw

Quoted 6 lines · Mark A. Greer

Quoted 1 line · Mark A. Greer
I'm off to read datasheets now... :)

Quoted 6 lines

Well, the people who sign my paychecks wanted me to do something else for the last couple days so I haven't looked at this yet. And now I'm leaving soon and will be taking next week off so I won't even start on this until the week of Nov 28. Sorry...
Mark

Ok, it's not a problem to wait for me (since anyway we couldn't release it before 2.6.15).

Mark, may I help you?

Signature · 2 lines

Regards
Andrey Volkov

Mark A. Greer

2005-12-06 21:17 UTC #11 lore raw

Quoted 14 lines · Andrey Volkov

Quoted 11 lines · Mark A. Greer
Quoted 1 line · Mark A. Greer
I'm off to read datasheets now... :)
Well, the people who sign my paychecks wanted me to do something else for the last couple days so I haven't looked at this yet. And now I'm leaving soon and will be taking next week off so I won't even start on this until the week of Nov 28. Sorry...

Quoted 6 lines

Quoted 1 line
Mark
Ok, it's not a problem to wait for me (since anyway we couldn't release it before 2.6.15).
Mark, may I help you?

Sorry for ignoring you Andrey, I've been totally consumed on other things until now.

Thanks for the offer but I doubt its a big enough project to warrant two people. I have a couple patches to a different driver to make & submit then I'll work on this. I hope to have a patch for this sometime next week sometime. I would certainly appreciate a thorough review of the patch, though.

Mark

Mark A. Greer

2005-12-19 21:03 UTC #19 lore raw

RFC 1/1 i2c: Combined ST m41txx i2c rtc chip driver (was: [PATCH 1/1] Added support of ST m41t85 rtc chip)

In reply to Mark A. Greer

Quoted 11 lines · Mark A. Greer

I think we can combine the two into an m41txx.c and pass the exact type in via platform_data--that would be the correct mechanism, right? The platform_data could also be used to seed the correct SQW freq and eliminate all the Kconfig noise.
Comments?
As for Jean's and Andrew's comments about the driver, they seem valid to me and should be addressed. In Andrey's defense, many of them are my fault. Once there is a consensus on the merging m41t00 & m41t85 question, I'll try to get a fixed up patch within a couple weeks.

I've made what I think should be close to an acceptable driver that supports the m41t00, m41t81, and m41t85 i2c rtc chips. I only have hw to test the m41t00 on a ppc platform, though.

It was suggested to me a while back to add retries when reading & writing the rtc regs. Personally, I think its overkill but I added the code anyway. You will see in m41txx_get_rtc_time() that 3 tries are made to get a good read of all the regs then up to 10 tries are made to make sure that the time didn't change while we were doing the reads. Something similar is done in m41txx_set().

Andrey, would you please apply and test this patch on your 5200?

Andy, I apologize but I didn't take the time to look at the mips tree in depth. I did do a quick scan and I think that you should be able to set 'rtc_get_time' to 'm41txx_get_rtc_time' and 'rtc_set_time' to 'm41txx_set_rtc_time', and it'll work. You'll have to hook up the proper driver for your host i2c ctlr, though.

Jean, Andrew, I think I addressed your coding style, etc. issues with the driver. If not, just point out the problems an I'll fix them.

The patch is against the 2.6.15-rc5-mm3 tree. Comments welcome.

[I will send a second patch that contains the changes I made to the ppc katana platform that has the m41t00 that I tested with.]

Thanks,

Mark

---
 drivers/i2c/chips/Kconfig  |   19 +-
 drivers/i2c/chips/Makefile |    2 
 drivers/i2c/chips/m41t00.c |  240 --------------------------------
 drivers/i2c/chips/m41txx.c |  336 +++++++++++++++++++++++++++++++++++++++++++++
 include/linux/i2c-id.h     |    2 
 include/linux/m41txx.h     |   28 +++
 6 files changed, 376 insertions(+), 251 deletions(-)

---

drivers/i2c/chips/Kconfig +10 -9

diff -Nurp linux-2.6.15-rc5-mm3.orig/drivers/i2c/chips/Kconfig linux-2.6.15-rc5-mm3-m41txx/drivers/i2c/chips/Kconfig
@@ -102,15 +102,6 @@ config TPS65010
 	  This driver can also be built as a module.  If so, the module
 	  will be called tps65010.
 
-config SENSORS_M41T00
-	tristate "ST M41T00 RTC chip"
-	depends on I2C && PPC32
-	help
-	  If you say yes here you get support for the ST M41T00 RTC chip.
-
-	  This driver can also be built as a module.  If so, the module
-	  will be called m41t00.
-
 config SENSORS_MAX6875
 	tristate "Maxim MAX6875 Power supply supervisor"
 	depends on I2C && EXPERIMENTAL

@@ -135,4 +126,14 @@ config RTC_X1205_I2C
 	  This driver can also be built as a module. If so, the module
 	  will be called x1205.
 
+config RTC_M41TXX_I2C
+	tristate "ST M41TXX Family of I2C RTC chips"
+	depends on I2C
+	help
+	  If you say yes here you get support for the ST M41TXX family
+	  of I2C RTC chips.
+
+	  This driver can also be built as a module.  If so, the module
+	  will be called m41t00.
+
 endmenu

drivers/i2c/chips/m41t00.c +0 ~~-240~~

240 lines not on this page · raw message

drivers/i2c/chips/m41txx.c +336 -0

336 lines not on this page · raw message

drivers/i2c/chips/Makefile +1 -1

diff -Nurp linux-2.6.15-rc5-mm3.orig/drivers/i2c/chips/Makefile linux-2.6.15-rc5-mm3-m41txx/drivers/i2c/chips/Makefile
@@ -6,7 +6,6 @@ obj-$(CONFIG_SENSORS_DS1337)	+= ds1337.o
 obj-$(CONFIG_SENSORS_DS1374)	+= ds1374.o
 obj-$(CONFIG_SENSORS_EEPROM)	+= eeprom.o
 obj-$(CONFIG_SENSORS_MAX6875)	+= max6875.o
-obj-$(CONFIG_SENSORS_M41T00)	+= m41t00.o
 obj-$(CONFIG_SENSORS_PCA9539)	+= pca9539.o
 obj-$(CONFIG_SENSORS_PCF8574)	+= pcf8574.o
 obj-$(CONFIG_SENSORS_PCF8591)	+= pcf8591.o

@@ -14,6 +13,7 @@ obj-$(CONFIG_SENSORS_RTC8564)	+= rtc8564
 obj-$(CONFIG_ISP1301_OMAP)	+= isp1301_omap.o
 obj-$(CONFIG_TPS65010)		+= tps65010.o
 obj-$(CONFIG_RTC_X1205_I2C)	+= x1205.o
+obj-$(CONFIG_RTC_M41TXX_I2C)	+= m41txx.o
 
 ifeq ($(CONFIG_I2C_DEBUG_CHIP),y)
 EXTRA_CFLAGS += -DDEBUG

include/linux/i2c-id.h +1 -1

diff -Nurp linux-2.6.15-rc5-mm3.orig/include/linux/i2c-id.h linux-2.6.15-rc5-mm3-m41txx/include/linux/i2c-id.h
@@ -85,7 +85,7 @@
 #define I2C_DRIVERID_SAA7114	49	/* video decoder		*/
 #define I2C_DRIVERID_ZR36120	50     /* Zoran 36120 video encoder	*/
 #define I2C_DRIVERID_24LC32A	51	/* Microchip 24LC32A 32k EEPROM	*/
-#define I2C_DRIVERID_STM41T00	52	/* real time clock		*/
+#define I2C_DRIVERID_STM41TXX	52	/* real time clock		*/
 #define I2C_DRIVERID_UDA1342	53	/* UDA1342 audio codec		*/
 #define I2C_DRIVERID_ADV7170	54	/* video encoder		*/
 #define I2C_DRIVERID_RADEON	55	/* I2C bus on Radeon boards	*/

include/linux/m41txx.h +28 -0

diff -Nurp linux-2.6.15-rc5-mm3.orig/include/linux/m41txx.h linux-2.6.15-rc5-mm3-m41txx/include/linux/m41txx.h
@@ -0,0 +1,28 @@
+/*
+ * Definitions for the ST M41Txx family of i2c rtc chips.
+ *
+ * Author: Mark A. Greer <mgreer at mvista.com>
+ *
+ * 2005 (c) MontaVista Software, Inc. This file is licensed under
+ * the terms of the GNU General Public License version 2. This program
+ * is licensed "as is" without any warranty of any kind, whether express
+ * or implied.
+ */
+
+#ifndef _M41TXX_H
+#define _M41TXX_H
+
+#define	M41TXX_DRV_NAME		"m41txx rtc"
+#define	M41TXX_I2C_ADDR		0x68
+
+#define	M41TXX_TYPE_M41T00	0
+#define	M41TXX_TYPE_M41T81	81
+#define	M41TXX_TYPE_M41T85	85
+
+struct m41txx_platform_data {
+	u16	type;
+	u16	i2c_addr;
+	u32	sqw_freq;
+};
+
+#endif /* _M41TXX_H */

Mark A. Greer

2005-12-19 21:06 UTC #20 lore raw

Re: [RFC] i2c: Combined ST m41txx i2c rtc chip driver (was: [PATCH 1/1] Added support of ST m41t85 rtc chip)

Quoted 2 lines · Mark A. Greer

[I will send a second patch that contains the changes I made to the ppc katana platform that has the m41t00 that I tested with.]

FYI, here are the code changes with an updated defconfig for the platform.

Mark

---
 configs/katana_defconfig |   99 +++++++++++++++++++++++++++++++----------------
 platforms/katana.c       |   37 +++++++++++++++--
 2 files changed, 99 insertions(+), 37 deletions(-)

---

arch/ppc/configs/katana_defconfig +67 ~~-32~~

diff -Nurp linux-2.6.15-rc5-mm3.orig/arch/ppc/configs/katana_defconfig linux-2.6.15-rc5-mm3-m41txx/arch/ppc/configs/katana_defconfig
@@ -1,17 +1,17 @@
 #
 # Automatically generated make config: don't edit
-# Linux kernel version: 2.6.13-mm1
-# Thu Sep  1 17:16:03 2005
+# Linux kernel version: 2.6.15-rc5-mm1
+# Fri Dec 16 16:12:10 2005
 #
 CONFIG_MMU=y
 CONFIG_GENERIC_HARDIRQS=y
 CONFIG_RWSEM_XCHGADD_ALGORITHM=y
 CONFIG_GENERIC_CALIBRATE_DELAY=y
-CONFIG_HAVE_DEC_LOCK=y
 CONFIG_PPC=y
 CONFIG_PPC32=y
 CONFIG_GENERIC_NVRAM=y
 CONFIG_SCHED_NO_NO_OMIT_FRAME_POINTER=y
+CONFIG_ARCH_MAY_HAVE_PC_FDC=y
 
 #
 # Code maturity level options

@@ -27,20 +27,21 @@ CONFIG_INIT_ENV_ARG_LIMIT2
 CONFIG_LOCALVERSION=""
 CONFIG_LOCALVERSION_AUTO=y
 CONFIG_SWAP=y
+CONFIG_SWAP_PREFETCH=y
 CONFIG_SYSVIPC=y
 # CONFIG_POSIX_MQUEUE is not set
 # CONFIG_BSD_PROCESS_ACCT is not set
 CONFIG_SYSCTL=y
 # CONFIG_AUDIT is not set
-# CONFIG_HOTPLUG is not set
-CONFIG_KOBJECT_UEVENT=y
 # CONFIG_IKCONFIG is not set
 CONFIG_INITRAMFS_SOURCE=""
 # CONFIG_EMBEDDED is not set
 CONFIG_KALLSYMS=y
 # CONFIG_KALLSYMS_EXTRA_PASS is not set
+CONFIG_HOTPLUG=y
 CONFIG_PRINTK=y
 CONFIG_BUG=y
+CONFIG_ELF_CORE=y
 CONFIG_BASE_FULL=y
 CONFIG_FUTEX=y
 CONFIG_EPOLL=y

@@ -49,8 +50,11 @@ CONFIG_CC_ALIGN_FUNCTIONS=0
 CONFIG_CC_ALIGN_LABELS=0
 CONFIG_CC_ALIGN_LOOPS=0
 CONFIG_CC_ALIGN_JUMPS=0
+CONFIG_SLAB=y
 # CONFIG_TINY_SHMEM is not set
 CONFIG_BASE_SMALL=0
+# CONFIG_SLOB is not set
+CONFIG_OBSOLETE_INTERMODULE=y
 
 #
 # Loadable module support

@@ -64,6 +68,25 @@ CONFIG_OBSOLETE_MODPARM=y
 CONFIG_KMOD=y
 
 #
+# Block layer
+#
+# CONFIG_LBD is not set
+# CONFIG_BLK_DEV_IO_TRACE is not set
+
+#
+# IO Schedulers
+#
+CONFIG_IOSCHED_NOOP=y
+CONFIG_IOSCHED_AS=y
+CONFIG_IOSCHED_DEADLINE=y
+CONFIG_IOSCHED_CFQ=y
+CONFIG_DEFAULT_AS=y
+# CONFIG_DEFAULT_DEADLINE is not set
+# CONFIG_DEFAULT_CFQ is not set
+# CONFIG_DEFAULT_NOOP is not set
+CONFIG_DEFAULT_IOSCHED="anticipatory"
+
+#
 # Processor
 #
 CONFIG_6xx=y

@@ -84,11 +107,6 @@ CONFIG_PPC_STD_MMU=y
 CONFIG_NOT_COHERENT_CACHE=y
 
 #
-# Performance-monitoring counters support
-#
-# CONFIG_PERFCTR is not set
-
-#
 # Platform options
 #
 # CONFIG_PPC_MULTIPLATFORM is not set

@@ -144,11 +162,13 @@ CONFIG_FLATMEM_MANUAL=y
 CONFIG_FLATMEM=y
 CONFIG_FLAT_NODE_MEM_MAP=y
 # CONFIG_SPARSEMEM_STATIC is not set
+CONFIG_SPLIT_PTLOCK_CPUS=4
 CONFIG_BINFMT_ELF=y
 CONFIG_BINFMT_MISC=y
 CONFIG_CMDLINE_BOOL=y
 CONFIG_CMDLINE="console=ttyMM0 ip=on"
 # CONFIG_PM is not set
+# CONFIG_SOFTWARE_SUSPEND is not set
 CONFIG_SECCOMP=y
 CONFIG_ISA_DMA_API=y

@@ -156,6 +176,8 @@ CONFIG_ISA_DMA_API=y
 # Bus options
 #
 CONFIG_GENERIC_ISA_DMA=y
+# CONFIG_PPC_I8259 is not set
+CONFIG_PPC_INDIRECT_PCI=y
 CONFIG_PCI=y
 CONFIG_PCI_DOMAINS=y
 CONFIG_PCI_LEGACY_PROC=y

@@ -241,14 +263,16 @@ CONFIG_TCP_CONG_BIC=y
 # CONFIG_NET_DIVERT is not set
 # CONFIG_ECONET is not set
 # CONFIG_WAN_ROUTER is not set
+
+#
+# QoS and/or fair queueing
+#
 # CONFIG_NET_SCHED is not set
-# CONFIG_NET_CLS_ROUTE is not set
 
 #
 # Network testing
 #
 # CONFIG_NET_PKTGEN is not set
-# CONFIG_NETFILTER_NETLINK is not set
 # CONFIG_HAMRADIO is not set
 # CONFIG_IRDA is not set
 # CONFIG_BT is not set

@@ -266,6 +290,11 @@ CONFIG_PREVENT_FIRMWARE_BUILD=y
 # CONFIG_FW_LOADER is not set
 
 #
+# Connector - unified userspace <-> kernelspace linker
+#
+# CONFIG_CONNECTOR is not set
+
+#
 # Memory Technology Devices (MTD)
 #
 CONFIG_MTD=y

@@ -283,6 +312,7 @@ CONFIG_MTD_BLOCK=y
 # CONFIG_FTL is not set
 # CONFIG_NFTL is not set
 # CONFIG_INFTL is not set
+# CONFIG_RFD_FTL is not set
 
 #
 # RAM/ROM/Flash chip drivers

@@ -347,6 +377,11 @@ CONFIG_MTD_PHRAM=y
 # CONFIG_MTD_NAND is not set
 
 #
+# OneNAND Flash Device Drivers
+#
+# CONFIG_MTD_ONENAND is not set
+
+#
 # Parallel port support
 #
 # CONFIG_PARPORT is not set

@@ -372,16 +407,7 @@ CONFIG_BLK_DEV_RAM=y
 CONFIG_BLK_DEV_RAM_COUNT
 CONFIG_BLK_DEV_RAM_SIZE@96
 CONFIG_BLK_DEV_INITRD=y
-# CONFIG_LBD is not set
 # CONFIG_CDROM_PKTCDVD is not set
-
-#
-# IO Schedulers
-#
-CONFIG_IOSCHED_NOOP=y
-CONFIG_IOSCHED_AS=y
-CONFIG_IOSCHED_DEADLINE=y
-CONFIG_IOSCHED_CFQ=y
 # CONFIG_ATA_OVER_ETH is not set
 
 #

@@ -418,6 +444,7 @@ CONFIG_IOSCHED_CFQ=y
 #
 # Macintosh device drivers
 #
+# CONFIG_WINDFARM is not set
 
 #
 # Network device support

@@ -445,6 +472,7 @@ CONFIG_NET_ETHERNET=y
 CONFIG_MII=y
 # CONFIG_HAPPYMEAL is not set
 # CONFIG_SUNGEM is not set
+# CONFIG_CASSINI is not set
 # CONFIG_NET_VENDOR_3COM is not set
 
 #

@@ -599,7 +627,6 @@ CONFIG_SERIAL_MPSC=y
 CONFIG_SERIAL_MPSC_CONSOLE=y
 CONFIG_SERIAL_CORE=y
 CONFIG_SERIAL_CORE_CONSOLE=y
-# CONFIG_SERIAL_JSM is not set
 CONFIG_UNIX98_PTYS=y
 CONFIG_LEGACY_PTYS=y
 CONFIG_LEGACY_PTY_COUNT%6

@@ -631,6 +658,7 @@ CONFIG_GEN_RTC=y
 # TPM devices
 #
 # CONFIG_TCG_TPM is not set
+# CONFIG_TELCLOCK is not set
 
 #
 # I2C support

@@ -682,14 +710,20 @@ CONFIG_I2C_MV64XXX=y
 # CONFIG_SENSORS_PCA9539 is not set
 # CONFIG_SENSORS_PCF8591 is not set
 # CONFIG_SENSORS_RTC8564 is not set
-CONFIG_SENSORS_M41T00=y
 # CONFIG_SENSORS_MAX6875 is not set
+# CONFIG_RTC_X1205_I2C is not set
+CONFIG_RTC_M41TXX_I2C=y
 # CONFIG_I2C_DEBUG_CORE is not set
 # CONFIG_I2C_DEBUG_ALGO is not set
 # CONFIG_I2C_DEBUG_BUS is not set
 # CONFIG_I2C_DEBUG_CHIP is not set
 
 #
+# SPI support
+#
+# CONFIG_SPI_MASTER is not set
+
+#
 # Dallas's 1-wire bus
 #
 # CONFIG_W1 is not set

@@ -728,6 +762,7 @@ CONFIG_HWMON=y
 # CONFIG_SENSORS_SMSC47M1 is not set
 # CONFIG_SENSORS_SMSC47B397 is not set
 # CONFIG_SENSORS_VIA686A is not set
+# CONFIG_SENSORS_VT8231 is not set
 # CONFIG_SENSORS_W83781D is not set
 # CONFIG_SENSORS_W83792D is not set
 # CONFIG_SENSORS_W83L785TS is not set

@@ -782,6 +817,10 @@ CONFIG_USB_ARCH_HAS_OHCI=y
 # CONFIG_USB is not set
 
 #
+# NOTE: USB_STORAGE enables SCSI, and 'SCSI disk support'
+#
+
+#
 # USB Gadget Support
 #
 # CONFIG_USB_GADGET is not set

@@ -801,6 +840,10 @@ CONFIG_USB_ARCH_HAS_OHCI=y
 #
 
 #
+# EDAC - error detection and reporting (RAS)
+#
+
+#
 # Distributed Lock Manager
 #
 # CONFIG_DLM is not set

@@ -816,10 +859,6 @@ CONFIG_EXT2_FS=y
 # CONFIG_REISERFS_FS is not set
 # CONFIG_JFS_FS is not set
 # CONFIG_FS_POSIX_ACL is not set
-
-#
-# XFS support
-#
 # CONFIG_XFS_FS is not set
 # CONFIG_OCFS2_FS is not set
 # CONFIG_MINIX_FS is not set

@@ -853,8 +892,8 @@ CONFIG_SYSFS=y
 CONFIG_TMPFS=y
 # CONFIG_HUGETLB_PAGE is not set
 CONFIG_RAMFS=y
-# CONFIG_CONFIGFS_FS is not set
 # CONFIG_RELAYFS_FS is not set
+# CONFIG_CONFIGFS_FS is not set
 
 #
 # Miscellaneous filesystems

@@ -917,10 +956,6 @@ CONFIG_MSDOS_PARTITION=y
 # CONFIG_CRC16 is not set
 CONFIG_CRC32=y
 # CONFIG_LIBCRC32C is not set
-
-#
-# Profiling support
-#
 # CONFIG_PROFILING is not set
 
 #

arch/ppc/platforms/katana.c +32 -5

diff -Nurp linux-2.6.15-rc5-mm3.orig/arch/ppc/platforms/katana.c linux-2.6.15-rc5-mm3-m41txx/arch/ppc/platforms/katana.c
@@ -30,6 +30,7 @@
 #include <linux/mtd/physmap.h>
 #include <linux/mv643xx.h>
 #include <linux/platform_device.h>
+#include <linux/m41txx.h>
 #ifdef CONFIG_BOOTIMG
 #include <linux/bootimg.h>
 #endif

@@ -845,17 +846,43 @@ katana_find_end_of_memory(void)
 	return bdp->bi_memsize;
 }
 
-#if defined(CONFIG_I2C_MV64XXX) && defined(CONFIG_SENSORS_M41T00)
-extern ulong	m41t00_get_rtc_time(void);
-extern int	m41t00_set_rtc_time(ulong);
+#if defined(CONFIG_I2C_MV64XXX) && defined(CONFIG_RTC_M41TXX_I2C)
+static struct m41txx_platform_data katana_m41txx_pdata = {
+	.type = M41TXX_TYPE_M41T00,
+	.i2c_addr = M41TXX_I2C_ADDR,
+};
+
+static struct platform_device katana_m41txx_pdev = {
+	.name		= M41TXX_DRV_NAME,
+	.id		= 0,
+	.num_resources	= 0,
+	.dev = {
+		.platform_data = &katana_m41txx_pdata,
+	},
+};
+
+static int __init
+katana_add_pdata(void)
+{
+	int rc;
+
+	if ((rc = platform_device_register(&katana_m41txx_pdev)))
+		platform_device_unregister(&katana_m41txx_pdev);
+
+	return rc;
+}
+arch_initcall(katana_add_pdata);
+
+extern ulong	m41txx_get_rtc_time(void);
+extern int	m41txx_set_rtc_time(ulong);
 
 static int __init
 katana_rtc_hookup(void)
 {
 	struct timespec	tv;
 
-	ppc_md.get_rtc_time = m41t00_get_rtc_time;
-	ppc_md.set_rtc_time = m41t00_set_rtc_time;
+	ppc_md.get_rtc_time = m41txx_get_rtc_time;
+	ppc_md.set_rtc_time = m41txx_set_rtc_time;
 
 	tv.tv_nsec = 0;
 	tv.tv_sec = (ppc_md.get_rtc_time)();

Andrey Volkov

author

2005-12-20 10:05 UTC #21 lore raw

Re: [RFC] i2c: Combined ST m41txx i2c rtc chip driver

In reply to Mark A. Greer

Hello Mark

Big Thanks, I check it on my board today-tomorrow. But check some comments below.

Quoted 60 lines · Mark A. Greer

Quoted 11 lines · Mark A. Greer
I think we can combine the two into an m41txx.c and pass the exact type in via platform_data--that would be the correct mechanism, right? The platform_data could also be used to seed the correct SQW freq and eliminate all the Kconfig noise.
Comments?
As for Jean's and Andrew's comments about the driver, they seem valid to me and should be addressed. In Andrey's defense, many of them are my fault. Once there is a consensus on the merging m41t00 & m41t85 question, I'll try to get a fixed up patch within a couple weeks.
I've made what I think should be close to an acceptable driver that supports the m41t00, m41t81, and m41t85 i2c rtc chips. I only have hw to test the m41t00 on a ppc platform, though.
It was suggested to me a while back to add retries when reading & writing the rtc regs. Personally, I think its overkill but I added the code anyway. You will see in m41txx_get_rtc_time() that 3 tries are made to get a good read of all the regs then up to 10 tries are made to make sure that the time didn't change while we were doing the reads. Something similar is done in m41txx_set().
Andrey, would you please apply and test this patch on your 5200?
Andy, I apologize but I didn't take the time to look at the mips tree in depth. I did do a quick scan and I think that you should be able to set 'rtc_get_time' to 'm41txx_get_rtc_time' and 'rtc_set_time' to 'm41txx_set_rtc_time', and it'll work. You'll have to hook up the proper driver for your host i2c ctlr, though.
Jean, Andrew, I think I addressed your coding style, etc. issues with the driver. If not, just point out the problems an I'll fix them.
The patch is against the 2.6.15-rc5-mm3 tree. Comments welcome.
[I will send a second patch that contains the changes I made to the ppc katana platform that has the m41t00 that I tested with.]
Thanks,
Mark
drivers/i2c/chips/m41txx.c +14 -0
+	down(&m41txx_mutex);
+	do {
+		retries = M41TXX_MAX_RETRIES;
+
+		do {
+			if (((sec = i2c_smbus_read_byte_data(save_client,
+						m41txx_chip->sec)) >= 0)
+				&& ((min = i2c_smbus_read_byte_data(save_client,
+						m41txx_chip->min)) >= 0)
+				&& ((hour= i2c_smbus_read_byte_data(save_client,
+						m41txx_chip->hour)) >= 0)
+				&& ((day = i2c_smbus_read_byte_data(save_client,
+						m41txx_chip->day)) >= 0)
+				&& ((mon = i2c_smbus_read_byte_data(save_client,

drivers/i2c/chips/m41txx.c line 102

Quoted 10 lines

drivers/i2c/chips/m41txx.c +10 -0

+						m41txx_chip->mon)) >= 0)
+				&& ((year= i2c_smbus_read_byte_data(save_client,
+						m41txx_chip->year)) >= 0))
+				break;
+		} while (--retries > 0);
+
+		if ((retries = 0) || ((sec = sec1) && (min = min1)
+				&& (hour = hour1) && (day = day1)
+				&& (mon = mon1) && (year = year1)))
+			break;

I think this code is overburdened (I forgot to point on it last time, sorry) and may be wrong for m41t8x, since when you send i2c stop condition (in read_byte_data), you release time registers of m41t8x, and as a consequence, in the worst case, you must compare/read it an undetermined number of times, but not 3 times (however, for m41t00 this code is correct).

I think i2c_master_recv here and i2c_master_send above in m41txx_set will be more appropriate, since for m41t00 it will have no meaning when you send STOP (250ms stall), but for m41t8x you could drop this while-loop completely.

Hint: quotation from m41st85w.pdf p13/34 "The system-to-user transfer of clock data will be halted whenever the address being read is a clock address (00h to 07h). The update will resume either due to a Stop Condition or when the pointer increments to a non-clock or RAM address."

Also, please, change _obsoleted_ BCD_TO_BIN to BCD2BIN (see include/linux/bcd.h)

Signature · 2 lines

Regards
Andrey Volkov

Mark A. Greer

2005-12-21 21:25 UTC #22 lore raw

Re: [RFC] i2c: Combined ST m41txx i2c rtc chip driver

Hi Andrey,

Quoted 7 lines · Andrey Volkov

Hello Mark
Big Thanks, I check it on my board today-tomorrow. But check some comments below.
Quoted 1 line · Mark A. Greer
On Tue, Nov 15, 2005 at 07:57:14PM -0700, Mark A. Greer wrote:

<snip>

Quoted 23 lines

drivers/i2c/chips/m41txx.c +23 -0

+	down(&m41txx_mutex);
+	do {
+		retries = M41TXX_MAX_RETRIES;
+
+		do {
+			if (((sec = i2c_smbus_read_byte_data(save_client,
+						m41txx_chip->sec)) >= 0)
+				&& ((min = i2c_smbus_read_byte_data(save_client,
+						m41txx_chip->min)) >= 0)
+				&& ((hour= i2c_smbus_read_byte_data(save_client,
+						m41txx_chip->hour)) >= 0)
+				&& ((day = i2c_smbus_read_byte_data(save_client,
+						m41txx_chip->day)) >= 0)
+				&& ((mon = i2c_smbus_read_byte_data(save_client,
+						m41txx_chip->mon)) >= 0)
+				&& ((year= i2c_smbus_read_byte_data(save_client,
+						m41txx_chip->year)) >= 0))
+				break;
+		} while (--retries > 0);
+
+		if ((retries = 0) || ((sec = sec1) && (min = min1)
+				&& (hour = hour1) && (day = day1)
+				&& (mon = mon1) && (year = year1)))

Quoted 8 lines

Review
Quoted 1 line
(quoted diff) +1 -0
+			break;
I think this code is overburdened (I forgot to point on it last time, sorry) and may be wrong for m41t8x, since when you send i2c stop condition (in read_byte_data), you release time registers of m41t8x, and as a consequence, in the worst case, you must compare/read it an undetermined number of times, but not 3 times (however, for m41t00 this code is correct).

The 3 tries isn't to make sure that the registers didn't change, its to make sure we actually successfully read all of the registers. There are 10 tries to make sure the registers didn't change. I doubt it will ever take more than 2 or 3 so I don't see a problem with a limit of 10.

I *think* I understand you point, though. You would prefer I not use the smbus calls, correct? If so, I disagree. I think its better to use the smbus calls b/c they're the most generic (read: will work with the most i2c host ctlr drivers). Perhaps Jean or someone else can make an executive decision on this.

Quoted 4 lines

I think i2c_master_recv here and i2c_master_send above in m41txx_set will be more appropriate, since for m41t00 it will have no meaning when you send STOP (250ms stall), but for m41t8x you could drop this while-loop completely.

I understand but its an issue of being more generic.

<snip>

Quoted 2 lines

Also, please, change _obsoleted_ BCD_TO_BIN to BCD2BIN (see include/linux/bcd.h)

I figured one of those was deprecated but didn't know which one. I'll change them.

Mark

Andrey Volkov

author

2006-01-11 19:03 UTC #26 lore raw

Re: [RFC] i2c: Combined ST m41txx i2c rtc chip driver

In reply to a message not in the archive

Hi Mark,

Quoted 21 lines · Mark A. Greer

Quoted 12 lines · Andrey Volkov
Quoted 9 lines · Mark A. Greer
Quoted 4 lines · Andrey Volkov
Hello Mark
Big Thanks, I check it on my board today-tomorrow. But check some comments below.
Quoted 5 lines
Quoted 2 lines
Did you have a chance to test it?
I testing it now (holidays, sorry).
Aprrox 3-4 hours delay.

Quoted 7 lines

No problem. Just curious if it works for you.
Please let me know.
Thanks,
Mark

Attached patch fix some misspellings and, mainly, _reset_ HT flag. If this flag is set, then STM418xx doesn't run in spite of ST flag state.
With this patch my board work ok.

P.S. Jean, "i2c_master_send vs i2c_smbus_write_byte_data" problem still open.
Could you made executive decision about it?

Signature · 6 lines

Regards
Andrey Volkov
-------------- next part --------------
An embedded and charset-unspecified text was scrubbed...
Name: 01-stm_combind_fixes.diff
Url: http://lists.lm-sensors.org/pipermail/lm-sensors/attachments/20060111/662a70d3/01-stm_combind_fixes.pl

Mark A. Greer

2006-01-18 22:06 UTC #27 lore raw

Re: [RFC] i2c: Combined ST m41txx i2c rtc chip driver

On Wed, Jan 11, 2006 at 10:03:24PM +0300, Andrey Volkov wrote:
<snip>

Quoted 3 lines

P.S. Jean, "i2c_master_send vs i2c_smbus_write_byte_data" problem still open.
Could you made executive decision about it?

Hi Andrey,

I'm taking the roaring silence to mean nobody really cares so I don't really have a problem going your way. I'll submit a patch and if it gets pooh-pooh'd, we can always change it back.

Stay tuned.

Mark

Jean Delvare

2006-01-19 07:24 UTC #28 lore raw

Re: [RFC] i2c: Combined ST m41txx i2c rtc chip driver

Hi Mark, Andrey,

Quoted 9 lines · Andrey Volkov

<snip>
Quoted 3 lines
P.S. Jean, "i2c_master_send vs i2c_smbus_write_byte_data" problem still open.
Could you made executive decision about it?
I'm taking the roaring silence to mean nobody really cares so I don't really have a problem going your way. I'll submit a patch and if it gets pooh-pooh'd, we can always change it back.

Sorry for the very late answer, I hadn't realized you were waiting for me.

The I2C vs. SMBus interface choice is yours (as long as the device supports both). I2C brings performance, SMBus gives you compatibility. Depending on the device you are writing a driver for, compatibility may or may not matter. For example, for a hardware monitoring chip driver, we always go for SMBus, because these chips are almost always connected to non-I2C SMBus masters. But RTC drivers are typically not used on mainstream PC motherboards so the compatibility may not be needed.

So, as Mark says, you should just go with the code you have right now. If it is SMBus and someone finds it too slow, that person can always add I2C access afterwards. OTOH if it is I2C and someone ever needs compatibility with a non-I2C master, that person gets to add the code. Note that it is perfectly acceptable to have *both* I2C and SMBus access methods in the same driver, if compatibility is wanted but the performance gain using I2C is significant. Of course it means more code in the driver, but as long as both access methods work and are properly maintained, it's fine with me.

Thanks,

Signature · 1 line

Jean Delvare

Mark A. Greer

2006-01-26 02:01 UTC #29 lore raw

Re: [RFC] i2c: Combined ST m41txx i2c rtc chip driver

Quoted 23 lines · Jean Delvare

Hi Mark, Andrey,
Quoted 9 lines · Andrey Volkov
<snip>
Quoted 3 lines
P.S. Jean, "i2c_master_send vs i2c_smbus_write_byte_data" problem still open.
Could you made executive decision about it?
I'm taking the roaring silence to mean nobody really cares so I don't really have a problem going your way. I'll submit a patch and if it gets pooh-pooh'd, we can always change it back.
Sorry for the very late answer, I hadn't realized you were waiting for me.
The I2C vs. SMBus interface choice is yours (as long as the device supports both). I2C brings performance, SMBus gives you compatibility. Depending on the device you are writing a driver for, compatibility may or may not matter. For example, for a hardware monitoring chip driver, we always go for SMBus, because these chips are almost always connected to non-I2C SMBus masters. But RTC drivers are typically not used on mainstream PC motherboards so the compatibility may not be needed.

Quoted 9 lines

So, as Mark says, you should just go with the code you have right now. If it is SMBus and someone finds it too slow, that person can always add I2C access afterwards. OTOH if it is I2C and someone ever needs compatibility with a non-I2C master, that person gets to add the code. Note that it is perfectly acceptable to have *both* I2C and SMBus access methods in the same driver, if compatibility is wanted but the performance gain using I2C is significant. Of course it means more code in the driver, but as long as both access methods work and are properly maintained, it's fine with me.

Hi Jean. Thanks for the feedback.

I decided to go with i2c b/c it is quite a bit more efficient and it eliminates the need to worry about values rolling over while reading the m41t8x's chips (still need to retry on the m41t00).

Andrey, et. al., how does this look & work?
Diff'd against linux-2.6.16-rc1-mm2.

Mark
---

drivers/i2c/chips/Kconfig +10 -9

diff -Nurp linux-2.6.16-rc1-mm2/drivers/i2c/chips/Kconfig linux-2.6.16-rc1-mm2-m41txx/drivers/i2c/chips/Kconfig
@@ -102,15 +102,6 @@ config TPS65010
 	  This driver can also be built as a module.  If so, the module
 	  will be called tps65010.
 
-config SENSORS_M41T00
-	tristate "ST M41T00 RTC chip"
-	depends on I2C && PPC32
-	help
-	  If you say yes here you get support for the ST M41T00 RTC chip.
-
-	  This driver can also be built as a module.  If so, the module
-	  will be called m41t00.
-
 config SENSORS_MAX6875
 	tristate "Maxim MAX6875 Power supply supervisor"
 	depends on I2C && EXPERIMENTAL

@@ -135,4 +126,14 @@ config RTC_X1205_I2C
 	  This driver can also be built as a module. If so, the module
 	  will be called x1205.
 
+config RTC_M41TXX_I2C
+	tristate "ST M41TXX Family of I2C RTC chips"
+	depends on I2C
+	help
+	  If you say yes here you get support for the ST M41TXX family
+	  of I2C RTC chips.
+
+	  This driver can also be built as a module.  If so, the module
+	  will be called m41txx.
+
 endmenu

drivers/i2c/chips/m41t00.c +0 ~~-241~~

241 lines not on this page · raw message

drivers/i2c/chips/m41txx.c +365 -0

365 lines not on this page · raw message

drivers/i2c/chips/Makefile +1 -1

diff -Nurp linux-2.6.16-rc1-mm2/drivers/i2c/chips/Makefile linux-2.6.16-rc1-mm2-m41txx/drivers/i2c/chips/Makefile
@@ -6,7 +6,6 @@ obj-$(CONFIG_SENSORS_DS1337)	+= ds1337.o
 obj-$(CONFIG_SENSORS_DS1374)	+= ds1374.o
 obj-$(CONFIG_SENSORS_EEPROM)	+= eeprom.o
 obj-$(CONFIG_SENSORS_MAX6875)	+= max6875.o
-obj-$(CONFIG_SENSORS_M41T00)	+= m41t00.o
 obj-$(CONFIG_SENSORS_PCA9539)	+= pca9539.o
 obj-$(CONFIG_SENSORS_PCF8574)	+= pcf8574.o
 obj-$(CONFIG_SENSORS_PCF8591)	+= pcf8591.o

@@ -14,6 +13,7 @@ obj-$(CONFIG_SENSORS_RTC8564)	+= rtc8564
 obj-$(CONFIG_ISP1301_OMAP)	+= isp1301_omap.o
 obj-$(CONFIG_TPS65010)		+= tps65010.o
 obj-$(CONFIG_RTC_X1205_I2C)	+= x1205.o
+obj-$(CONFIG_RTC_M41TXX_I2C)	+= m41txx.o
 
 ifeq ($(CONFIG_I2C_DEBUG_CHIP),y)
 EXTRA_CFLAGS += -DDEBUG

include/linux/i2c-id.h +1 -1

diff -Nurp linux-2.6.16-rc1-mm2/include/linux/i2c-id.h linux-2.6.16-rc1-mm2-m41txx/include/linux/i2c-id.h
@@ -79,7 +79,7 @@
 #define I2C_DRIVERID_SAA7114	49	/* video decoder		*/
 #define I2C_DRIVERID_ZR36120	50     /* Zoran 36120 video encoder	*/
 #define I2C_DRIVERID_24LC32A	51	/* Microchip 24LC32A 32k EEPROM	*/
-#define I2C_DRIVERID_STM41T00	52	/* real time clock		*/
+#define I2C_DRIVERID_STM41TXX	52	/* real time clock		*/
 #define I2C_DRIVERID_UDA1342	53	/* UDA1342 audio codec		*/
 #define I2C_DRIVERID_ADV7170	54	/* video encoder		*/
 #define I2C_DRIVERID_RADEON	55	/* I2C bus on Radeon boards	*/

include/linux/m41txx.h +47 -0

47 lines not on this page · raw message

Mark A. Greer

2006-01-26 20:50 UTC #30 lore raw

Re: [RFC] i2c: Combined ST m41txx i2c rtc chip driver

Quoted 2 lines · Mark A. Greer

Andrey, et. al., how does this look & work?
Diff'd against linux-2.6.16-rc1-mm2.

Oops, had a minor mess up in the previous patch. This one replaces that one.

Mark
---

drivers/i2c/chips/Kconfig +10 -9

diff -Nurp linux-2.6.16-rc1-mm2/drivers/i2c/chips/Kconfig linux-2.6.16-rc1-mm2-m41txx/drivers/i2c/chips/Kconfig
@@ -102,15 +102,6 @@ config TPS65010
 	  This driver can also be built as a module.  If so, the module
 	  will be called tps65010.
 
-config SENSORS_M41T00
-	tristate "ST M41T00 RTC chip"
-	depends on I2C && PPC32
-	help
-	  If you say yes here you get support for the ST M41T00 RTC chip.
-
-	  This driver can also be built as a module.  If so, the module
-	  will be called m41t00.
-
 config SENSORS_MAX6875
 	tristate "Maxim MAX6875 Power supply supervisor"
 	depends on I2C && EXPERIMENTAL

@@ -135,4 +126,14 @@ config RTC_X1205_I2C
 	  This driver can also be built as a module. If so, the module
 	  will be called x1205.
 
+config RTC_M41TXX_I2C
+	tristate "ST M41TXX Family of I2C RTC chips"
+	depends on I2C
+	help
+	  If you say yes here you get support for the ST M41TXX family
+	  of I2C RTC chips.
+
+	  This driver can also be built as a module.  If so, the module
+	  will be called m41txx.
+
 endmenu

drivers/i2c/chips/m41t00.c +0 ~~-241~~

241 lines not on this page · raw message

drivers/i2c/chips/m41txx.c +365 -0

365 lines not on this page · raw message

drivers/i2c/chips/Makefile +1 -1

diff -Nurp linux-2.6.16-rc1-mm2/drivers/i2c/chips/Makefile linux-2.6.16-rc1-mm2-m41txx/drivers/i2c/chips/Makefile
@@ -6,7 +6,6 @@ obj-$(CONFIG_SENSORS_DS1337)	+= ds1337.o
 obj-$(CONFIG_SENSORS_DS1374)	+= ds1374.o
 obj-$(CONFIG_SENSORS_EEPROM)	+= eeprom.o
 obj-$(CONFIG_SENSORS_MAX6875)	+= max6875.o
-obj-$(CONFIG_SENSORS_M41T00)	+= m41t00.o
 obj-$(CONFIG_SENSORS_PCA9539)	+= pca9539.o
 obj-$(CONFIG_SENSORS_PCF8574)	+= pcf8574.o
 obj-$(CONFIG_SENSORS_PCF8591)	+= pcf8591.o

@@ -14,6 +13,7 @@ obj-$(CONFIG_SENSORS_RTC8564)	+= rtc8564
 obj-$(CONFIG_ISP1301_OMAP)	+= isp1301_omap.o
 obj-$(CONFIG_TPS65010)		+= tps65010.o
 obj-$(CONFIG_RTC_X1205_I2C)	+= x1205.o
+obj-$(CONFIG_RTC_M41TXX_I2C)	+= m41txx.o
 
 ifeq ($(CONFIG_I2C_DEBUG_CHIP),y)
 EXTRA_CFLAGS += -DDEBUG

include/linux/i2c-id.h +1 -1

diff -Nurp linux-2.6.16-rc1-mm2/include/linux/i2c-id.h linux-2.6.16-rc1-mm2-m41txx/include/linux/i2c-id.h
@@ -79,7 +79,7 @@
 #define I2C_DRIVERID_SAA7114	49	/* video decoder		*/
 #define I2C_DRIVERID_ZR36120	50     /* Zoran 36120 video encoder	*/
 #define I2C_DRIVERID_24LC32A	51	/* Microchip 24LC32A 32k EEPROM	*/
-#define I2C_DRIVERID_STM41T00	52	/* real time clock		*/
+#define I2C_DRIVERID_STM41TXX	52	/* real time clock		*/
 #define I2C_DRIVERID_UDA1342	53	/* UDA1342 audio codec		*/
 #define I2C_DRIVERID_ADV7170	54	/* video encoder		*/
 #define I2C_DRIVERID_RADEON	55	/* I2C bus on Radeon boards	*/

include/linux/m41txx.h +47 -0

47 lines not on this page · raw message