Mario Scheithauer 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 2:
(11 comments)
Thanks for the review!
https://review.coreboot.org/#/c/19625/1//COMMIT_MSG Commit Message:
PS1, Line 9: th
the
Done
PS1, Line 11: us
in the
Done
https://review.coreboot.org/#/c/19625/1/src/drivers/i2c/rx6110sa/chip.h File src/drivers/i2c/rx6110sa/chip.h:
PS1, Line 19: /* The day (of the week) is indicated by 7 bits, bit 0 to bit
Can you please add in the comment, that the weekday has to have the value 0
Done
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/i2c.h>
The whole driver references I2C transfaers.
is not used - removed
PS1, Line 18: #include <version.h>
You don't have to use soc/i2c.h as your driver completely relies on device/
changed with #include <device/device.h>
PS1, Line 81: data loss, s
"power loss event"?
Done
PS1, Line 85:
detected,
Done
PS1, Line 88:
detected,
Done
PS1, Line 95: b(I2C_BUS_NUM, I
In the other comments you have used lower case for bit descriptions. Want t
Done
PS1, Line 122:
Maybe better call it "RTC" here as the complete RTC is initialized now?
Done
PS1, Line 123: i2c_writeb(I2C_BUS_NUM, I2C_DEV_NUM, HOUR_REG, 1); : i2c_writeb(I2C_BUS_NUM, I2C_DEV_NUM, MINUTE_REG, 0); : i2c_writeb(I2C_BUS_NUM, I2C_DEV_NUM, SECOND_REG, 0);
In Line 107 you have written STOP_BIT to control register.
Done