uwe.poeche@siemens.com 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 2:
(18 comments)
https://review.coreboot.org/c/coreboot/+/36642/1/src/drivers/i2c/ptn3460/chi... File src/drivers/i2c/ptn3460/chip.h:
https://review.coreboot.org/c/coreboot/+/36642/1/src/drivers/i2c/ptn3460/chi... PS1, Line 1: struct
I guess not since here the structure is defined (which is empty as the parameters are handled differ […]
Ack
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?
Done
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`.
We had in mind to use this to combine status and information (via or) as return values. I think enum is not helpfull for this. Using types.h is also not helpfull because of not included special PTN return values.
https://review.coreboot.org/c/coreboot/+/36642/1/src/drivers/i2c/ptn3460/ptn... PS1, Line 66: This
These
Done
https://review.coreboot.org/c/coreboot/+/36642/1/src/drivers/i2c/ptn3460/ptn... PS1, Line 66: coded
implemented
Done
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
Done
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. […]
Done
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*?
Done
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?
Done
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?
Done
https://review.coreboot.org/c/coreboot/+/36642/1/src/drivers/i2c/ptn3460/ptn... PS1, Line 42: return PTN_NO_ERROR;
One line? […]
Done
https://review.coreboot.org/c/coreboot/+/36642/1/src/drivers/i2c/ptn3460/ptn... PS1, Line 45: functions
function
Done
https://review.coreboot.org/c/coreboot/+/36642/1/src/drivers/i2c/ptn3460/ptn... PS1, Line 64: EDID-data
EDID data
Done
https://review.coreboot.org/c/coreboot/+/36642/1/src/drivers/i2c/ptn3460/ptn... PS1, Line 69: }
We are writing 128 bytes byte-per-byte here. […]
Ack
https://review.coreboot.org/c/coreboot/+/36642/1/src/drivers/i2c/ptn3460/ptn... PS1, Line 85: Not able
Unable
Done
https://review.coreboot.org/c/coreboot/+/36642/1/src/drivers/i2c/ptn3460/ptn... PS1, Line 95: Writing
Write
Done
https://review.coreboot.org/c/coreboot/+/36642/1/src/drivers/i2c/ptn3460/ptn... PS1, Line 105: status
position?
You are right. The return value is the read value in lower 8 Bits with status in upper bit. So I changed to val.
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 */
We have this structure (struct ptn_3460_config cfg) which needs to be read byte-per-byte over I2C. […]
Ack