T Michael Turney has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/30003 )
Change subject: sdm845: Add GPIO IRQ APIs ......................................................................
Patch Set 15:
(6 comments)
https://review.coreboot.org/#/c/30003/9/src/soc/qualcomm/sdm845/gpio.c File src/soc/qualcomm/sdm845/gpio.c:
https://review.coreboot.org/#/c/30003/9/src/soc/qualcomm/sdm845/gpio.c@92 PS9, Line 92: &
Is this supposed to be a << instead of a & ?
Ack
https://review.coreboot.org/#/c/30003/9/src/soc/qualcomm/sdm845/gpio.c@94 PS9, Line 94: write32(®s->intr_cfg, reg_val);
If this does need to be a read-modify-write operation, then please use clrsetbits_le32().
Ack
https://review.coreboot.org/#/c/30003/11/src/soc/qualcomm/sdm845/gpio.c File src/soc/qualcomm/sdm845/gpio.c:
https://review.coreboot.org/#/c/30003/11/src/soc/qualcomm/sdm845/gpio.c@90 PS11, Line 90: clrsetbits_le32(®s->intr_cfg, GPIO_INTR_RAW_STATUS_ENABLE
Please combine into a single access: […]
Ack
https://review.coreboot.org/#/c/30003/12/src/soc/qualcomm/sdm845/gpio.c File src/soc/qualcomm/sdm845/gpio.c:
https://review.coreboot.org/#/c/30003/12/src/soc/qualcomm/sdm845/gpio.c@91 PS12, Line 91: << GPIO_INTR_RAW_STATUS_EN_SHIFT);
line over 80 characters
Ack
https://review.coreboot.org/#/c/30003/9/src/soc/qualcomm/sdm845/include/soc/... File src/soc/qualcomm/sdm845/include/soc/gpio.h:
https://review.coreboot.org/#/c/30003/9/src/soc/qualcomm/sdm845/include/soc/... PS9, Line 382: IRQ_TYPE_LEVEL
Since the exact value of this is important, please write = 0 (and = 1, = 2, etc. […]
Ack
https://review.coreboot.org/#/c/30003/1/src/soc/qualcomm/sdm845/include/soc/... File src/soc/qualcomm/sdm845/include/soc/gpio.h:
https://review.coreboot.org/#/c/30003/1/src/soc/qualcomm/sdm845/include/soc/... PS1, Line 77: #define GPIO_ENABLE 1
Well, you need to update all use sites accordingly when you change it, of course.
Ack
-
T Michael Turney (Code Review)