[coreboot-gerrit] Change in coreboot[master]: drivers/i2c: Add new driver for RTC type RX6110 SA

[Werner Zeh (Code Review)]

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);
             : 	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


-- 
To view, visit https://review.coreboot.org/19625
To unsubscribe, visit https://review.coreboot.org/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I1290a10c2d5ad76a317c99c8b92a013309a605d6
Gerrit-PatchSet: 1
Gerrit-Project: coreboot
Gerrit-Branch: master
Gerrit-Owner: Mario Scheithauer <mario.scheithauer at siemens.com>
Gerrit-Reviewer: Paul Menzel <paulepanter at users.sourceforge.net>
Gerrit-Reviewer: Werner Zeh <werner.zeh at siemens.com>
Gerrit-Reviewer: build bot (Jenkins) <no-reply at coreboot.org>
Gerrit-HasComments: Yes