[flashrom-gerrit] [S] Change in flashrom[main]: how_to_add_new_chip: Add a section for feature bits and WRSR handling

[Hsuan-ting Chen (Code Review)]

Attention is currently required from: Anastasia Klimchuk, Thomas Heijligen.

Hsuan-ting Chen has posted comments on this change by Hsuan-ting Chen. ( https://review.coreboot.org/c/flashrom/+/82908?usp=email )

Change subject: how_to_add_new_chip: Add a section for feature bits and WRSR handling
......................................................................

Patch Set 2:

(10 comments)

Patchset:

PS2: 
Thanks for the suggestions!

File doc/contrib_howtos/how_to_add_new_chip.rst:

https://review.coreboot.org/c/flashrom/+/82908/comment/0085c63d_1e94e19b?usp... :
PS1, Line 105: Feature Bits
             : -------------
> This can be on the same level as "Properties", especially because you have future plans to gradually […]

Done

https://review.coreboot.org/c/flashrom/+/82908/comment/57b8609c_1770d64c?usp... :
PS1, Line 108: The ``.feature_bits`` field in a chip definition encodes various features using a bitmask.
             : Here are some of the selected feature bits that need to be highlighted:
> With this, we don't need an item in a list inside Properties. […]

Done

https://review.coreboot.org/c/flashrom/+/82908/comment/1a3333b7_a079cd5e?usp... :
PS1, Line 111: Write-Status-Register (WRSR) Handling
             : ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
> This header also goes on level up, use `-`

Done

https://review.coreboot.org/c/flashrom/+/82908/comment/c2b4d574_a09e6bf8?usp... :
PS1, Line 113: 
             : ``FEATURE_WRSR_EWSR``, ``FEATURE_WRSR_WREN``, and ``FEATURE_WRSR_EITHER``. These bits used for **SPI only**.
> I think this sentence is not needed. […]

Done

AFAIK it should be SPI only.

Move the "SPI only" hints into the next section instead.

https://review.coreboot.org/c/flashrom/+/82908/comment/98d08826_969aedc1?usp... :
PS1, Line 120: This
> Delete "This", start with "Indicates that"

Done

I noticed that it actually doesn't jump to a newline after EWSR, e.g.

```
FEATURE_WRSR_EWSR indicates that we need an Enable-Write-Status-Register
(EWSR) instruction which opens the status register for the immediately-followed
next WRSR instruction. Usually, the opcode is 0x50.
```

So I used the "indicates" with lowercase and remove the ":"

https://review.coreboot.org/c/flashrom/+/82908/comment/c85d39c7_8982b7d3?usp... :
PS1, Line 124: This
> same

Done

https://review.coreboot.org/c/flashrom/+/82908/comment/74fae8b8_7f28dc8b?usp... :
PS1, Line 125: priort
> typo, prior

Done

https://review.coreboot.org/c/flashrom/+/82908/comment/cef552ed_32a04ace?usp... :
PS1, Line 128: This
> same

Done

https://review.coreboot.org/c/flashrom/+/82908/comment/09a6fb9b_ea72a571?usp... :
PS1, Line 110: 
             : Write-Status-Register (WRSR) Handling
             : ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
             : 
             : ``FEATURE_WRSR_EWSR``, ``FEATURE_WRSR_WREN``, and ``FEATURE_WRSR_EITHER``. These bits used for **SPI only**.
             : 
             : The Write Status Register (WRSR) is used to configure various settings within the flash chip, including write protection and
             : other features. The way WRSR is accessed varies between SPI flash chips, leading to the need for these feature bits.
             : 
             : * ``FEATURE_WRSR_EWSR``:
             :   This indicates that we need an **Enable-Write-Status-Register** (EWSR) instruction which opens the status register for the
             :   immediately-followed next WRSR instruction. Usually, the opcode is **0x50**.
             : 
             : * ``FEATURE_WRSR_WREN``:
             :   This indicates that we need an **Write-Enable** (WREN) instruction to set the Write Enable Latch (WEL) bit. The WEL bit
             :   must be set priort to every WRSR command. Usually, the opcode is **0x06**.
             : 
             : * ``FEATURE_WRSR_EITHER``:
             :   This indicates that either EWSR or WREN is supported in this chip.
> Thank you for explaining! It's really good that I know your future plans. […]

Done