[Top] [All Lists]

Re: [PATCH 1/2]: Add support for Dallas/Maxim DS1685/1687 RTC

To: Lars-Peter Clausen <>
Subject: Re: [PATCH 1/2]: Add support for Dallas/Maxim DS1685/1687 RTC
From: Kumba <>
Date: Thu, 17 Feb 2011 00:45:31 -0500
Cc: Linux MIPS List <>,
In-reply-to: <>
Original-recipient: rfc822;
References: <> <>
User-agent: Mozilla/5.0 (Windows; U; Windows NT 6.0; en-US; rv: Gecko/20101207 Lightning/1.0b2 Thunderbird/3.1.7
On 02/16/2011 18:23, Lars-Peter Clausen wrote:
I think you should really use readb(pdata->regs + REG) instead of the following
structs. Maybe add a helper function in the form of:
static uint8_t ds1685_read(struct ds1685_priv *ds1685, unsigned int reg) {
        return readb(pdata->regs + REG);

That should also help with the different paddings introduced in patch 2.

Lots of good feedback, thanks! Ralf already suggested using offsets instead of a struct. I'm tinkering now with getting this to work, as I have to have this done before I can address many of your other points.

I have determined the following formula specific to the SGI O2 to read the RTC registers:

readb(pdata->regs + RTC_<REGISTER> * 0x100);

is equivalent to


I'll assume writeb() changes are the same. The question is, how do I wire in the 0x100 padding value in such a way that I keep the IP32-specific bits out of generic code? Ralf mentioned using some field in platform_data, but I haven't quite learned the platform stuff (this is my first real attempt at a kernle driver).

Also, one thing I can quickly address, I put the ds1685.h file under include/linux/rtc because I saw that folder as already existing. I figured that's where rtc header files went. Right now, nothing outside of the driver uses it, but SGI O2 will need to eventually, as it uses the RTC to trigger a system poweroff by accessing a few of the extended control registers.

It currently uses similarly-duplicated #defines in a local header file, but I figured, if I can get this driver fully working, and other platforms could theoretically use the same trick, would not include/linux/rtc be the best place for the header? If there's a better place, please let me know!


Joshua Kinard

"The past tempts us, the present confuses us, the future frightens us. And our lives slip away, moment by moment, lost in that vast, terrible in-between."

--Emperor Turhan, Centauri Republic

<Prev in Thread] Current Thread [Next in Thread>