Hello Mark
Big Thanks, I check it on my board today-tomorrow.
But check some comments below.
Mark A. Greer wrote:
> On Tue, Nov 15, 2005 at 07:57:14PM -0700, Mark A. Greer wrote:
>
>>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
> + 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)))
> + 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)
--
Regards
Andrey Volkov
