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

[Kumba <kumba@gentoo.org>]

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

readb(pdata->regs.time.<REGISTER>);

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!

Thanks,

-- 
Joshua Kinard
Gentoo/MIPS
kumba@gentoo.org

"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