Paul Menzel has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/36642 )
Change subject: drivers/i2c/ptn3460: Provide chip driver for PTN3460 ......................................................................
Patch Set 1:
(17 comments)
https://review.coreboot.org/c/coreboot/+/36642/1/src/drivers/i2c/ptn3460/ptn... File src/drivers/i2c/ptn3460/ptn3460.h:
https://review.coreboot.org/c/coreboot/+/36642/1/src/drivers/i2c/ptn3460/ptn... PS1, Line 29: PTN_NO_ERROR PTN_SUCCESS?
https://review.coreboot.org/c/coreboot/+/36642/1/src/drivers/i2c/ptn3460/ptn... PS1, Line 34: #define PTN_ERROR 0x40000000 Better use an enum or extend `cb_err` in `src/include/types.h`.
https://review.coreboot.org/c/coreboot/+/36642/1/src/drivers/i2c/ptn3460/ptn... PS1, Line 66: This These
https://review.coreboot.org/c/coreboot/+/36642/1/src/drivers/i2c/ptn3460/ptn... PS1, Line 66: coded implemented
https://review.coreboot.org/c/coreboot/+/36642/1/src/drivers/i2c/ptn3460/ptn... File src/drivers/i2c/ptn3460/ptn3460.c:
https://review.coreboot.org/c/coreboot/+/36642/1/src/drivers/i2c/ptn3460/ptn... PS1, Line 23: functions function
https://review.coreboot.org/c/coreboot/+/36642/1/src/drivers/i2c/ptn3460/ptn... PS1, Line 23: /** \brief This functions selects one of 7 EDID-tables inside PTN3460 Please format it like below.
``` /** * \brief … * ```
https://review.coreboot.org/c/coreboot/+/36642/1/src/drivers/i2c/ptn3460/ptn... PS1, Line 26: * @param edid_num Number of EDID to emulate (0..6) Alignement of *Number* and *Pointer*?
https://review.coreboot.org/c/coreboot/+/36642/1/src/drivers/i2c/ptn3460/ptn... PS1, Line 27: * @return 0 on success or error code PTN_NO_ERROR is returned, isn’t it?
https://review.coreboot.org/c/coreboot/+/36642/1/src/drivers/i2c/ptn3460/ptn... PS1, Line 36: /* Enable emulation of the desired EDID table */ Could be removed, as the code is self-explanatory?
https://review.coreboot.org/c/coreboot/+/36642/1/src/drivers/i2c/ptn3460/ptn... PS1, Line 42: return PTN_NO_ERROR; One line?
return status ? (PTN_BUS_ERROR | status) : PTN_NO_ERROR
https://review.coreboot.org/c/coreboot/+/36642/1/src/drivers/i2c/ptn3460/ptn... PS1, Line 45: functions function
https://review.coreboot.org/c/coreboot/+/36642/1/src/drivers/i2c/ptn3460/ptn... PS1, Line 64: EDID-data EDID data
https://review.coreboot.org/c/coreboot/+/36642/1/src/drivers/i2c/ptn3460/ptn... PS1, Line 69: } How long does this loop need?
https://review.coreboot.org/c/coreboot/+/36642/1/src/drivers/i2c/ptn3460/ptn... PS1, Line 85: Not able Unable
https://review.coreboot.org/c/coreboot/+/36642/1/src/drivers/i2c/ptn3460/ptn... PS1, Line 95: Writing Write
https://review.coreboot.org/c/coreboot/+/36642/1/src/drivers/i2c/ptn3460/ptn... PS1, Line 105: status position?
https://review.coreboot.org/c/coreboot/+/36642/1/src/drivers/i2c/ptn3460/ptn... PS1, Line 110: *ptr++ = (uint8_t)status; /* fill config structure via ptr */ Is pointer arithmetic really needed?