Werner Zeh has posted comments on this change. ( https://review.coreboot.org/19625 )
Change subject: drivers/i2c: Add new driver for RTC type RX6110 SA ......................................................................
Patch Set 1:
(11 comments)
https://review.coreboot.org/#/c/19625/1/src/drivers/i2c/rx6110sa/chip.h File src/drivers/i2c/rx6110sa/chip.h:
PS1, Line 19: unsigned char user_weekday; /* User day of the week to set */ Can you please add in the comment, that the weekday has to have the value 0..6 as the weekday register in the RTC is encoded in single set bits.
https://review.coreboot.org/#/c/19625/1/src/drivers/i2c/rx6110sa/rx6110sa.c File src/drivers/i2c/rx6110sa/rx6110sa.c:
PS1, Line 16: #include <device/smbus.h> The whole driver references I2C transfaers. Do we need smbus.h here at all?
PS1, Line 18: #include <soc/i2c.h> You don't have to use soc/i2c.h as your driver completely relies on device/i2c.h. Or did I missed something?
PS1, Line 81: error status "power loss event"?
PS1, Line 85: detect detected,
PS1, Line 88: detect detected,
PS1, Line 95: F lower case
PS1, Line 95: Timer Enable Bit In the other comments you have used lower case for bit descriptions. Want to sync that?
PS1, Line 122: clock Maybe better call it "RTC" here as the complete RTC is initialized now?
PS1, Line 123: i2c_readb(I2C_BUS_NUM, I2C_DEV_NUM, CTRL_REG, ®); : reg &= ~STOP_BIT; : i2c_writeb(I2C_BUS_NUM, I2C_DEV_NUM, CTRL_REG, reg); In Line 107 you have written STOP_BIT to control register. With that write access the only set bit in that register is bit 6. To start the RTC now you don't have to read the control register back again as the value is known. Just write 0x0 to it. Or did I missed something?
https://review.coreboot.org/#/c/19625/1/src/drivers/i2c/rx6110sa/rx6110sa.h File src/drivers/i2c/rx6110sa/rx6110sa.h:
PS1, Line 54: V lower case