driver/mtd_spi_nor: fix erase with unaligned addresses#8400
driver/mtd_spi_nor: fix erase with unaligned addresses#8400MichelRottleuthner merged 1 commit intoRIOT-OS:masterfrom
Conversation
vincent-d
added
Type: bug
vincent-d
force-pushed
the
fix/mtd_erase
branch
from
b677085 to
e962db3
Compare
smlng
left a comment
smlng
left a comment
There was a problem hiding this comment.
minor nits, otherwise looks good
drivers/mtd_spi_nor/mtd_spi_nor.c
Outdated
| size -= total_size; | ||
| } | ||
| else if ((dev->flag & SPI_NOR_F_SECT_32K) && (size >= 32768) && ((addr & 0x7FFF) == 0)) { | ||
| /* 32 KiO sectors can be erased with sector erase command */ |
There was a problem hiding this comment.
from the command below I'd infer it should be /sector/block/, right?
| mtd_spi_cmd(dev, dev->opcode->wren); | ||
|
|
||
| if (size == total_size) { | ||
| mtd_spi_cmd(dev, dev->opcode->chip_erase); |
There was a problem hiding this comment.
you could move this special case above the loop, to avoid some unneeded ops, but I'm okay with leaving this in the loop
There was a problem hiding this comment.
Yes, but write enable would still be needed first, then I'm not sure there is a significant gain to move out of the loop, at least in terms of code duplication and readability
MichelRottleuthner
left a comment
MichelRottleuthner
left a comment
There was a problem hiding this comment.
Looks good to me. I'd just prefer defines over magic numbers.
drivers/mtd_spi_nor/mtd_spi_nor.c
Outdated
| mtd_spi_cmd(dev, dev->opcode->chip_erase); | ||
| size -= total_size; | ||
| } | ||
| else if ((dev->flag & SPI_NOR_F_SECT_32K) && (size >= 32768) && ((addr & 0x7FFF) == 0)) { |
There was a problem hiding this comment.
32768 has no ul suffix here like below. Why not use a define for 32786ul, 4096ul (and 0x7FFF)?
drivers/mtd_spi_nor/mtd_spi_nor.c
Outdated
| size -= total_size; | ||
| } | ||
| else if ((dev->flag & SPI_NOR_F_SECT_32K) && (size >= 32768) && ((addr & 0x7FFF) == 0)) { | ||
| /* 32 KiO blocks can be erased with block erase command */ |
vincent-d
force-pushed
the
fix/mtd_erase
branch
from
a7bb35b to
063a783
Compare
|
squashed @smlng could you ACK? |
3 participants
Contribution description
Erasing a 32k-block with an unaligned address leaded to erase to 32k-block which contained the address.
This is now fixed.
I'm wondering if we should not refactor a bit the-> see #8411mtdinterface to take into account the different erasable sizes on mtd nor flashes, this would be anyway in another PR!Issues/PRs references