| Message ID | 20220202145853.4187726-12-michael@walle.cc |
|---|---|
| State | Changes Requested |
| Delegated to: | Ambarus Tudor |
| Headers | show |
| Series | mtd: spi-nor: move vendor specific code into vendor modules | expand |
On 2/2/22 16:58, Michael Walle wrote: > EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe > > Increase readability of the code. Instead of returning early if the > flash size is smaller or equal than 16MiB and then do the fixups for > larger flashes, do it within the condition. > mm, no, I'm not sure this improves readability, I see the two equivalent. The original version has the benefit of no indentation. Pratyush? > Signed-off-by: Michael Walle <michael@walle.cc> > --- > drivers/mtd/spi-nor/spansion.c | 13 ++++++------- > 1 file changed, 6 insertions(+), 7 deletions(-) > > diff --git a/drivers/mtd/spi-nor/spansion.c b/drivers/mtd/spi-nor/spansion.c > index 534196b1d3e7..dedc2de90cb8 100644 > --- a/drivers/mtd/spi-nor/spansion.c > +++ b/drivers/mtd/spi-nor/spansion.c > @@ -296,13 +296,12 @@ static const struct flash_info spansion_parts[] = { > > static void spansion_late_init(struct spi_nor *nor) > { > - if (nor->params->size <= SZ_16M) > - return; > - > - nor->flags |= SNOR_F_4B_OPCODES; > - /* No small sector erase for 4-byte command set */ > - nor->erase_opcode = SPINOR_OP_SE; > - nor->mtd.erasesize = nor->info->sector_size; > + if (nor->params->size > SZ_16M) { > + nor->flags |= SNOR_F_4B_OPCODES; > + /* No small sector erase for 4-byte command set */ > + nor->erase_opcode = SPINOR_OP_SE; > + nor->mtd.erasesize = nor->info->sector_size; > + } > } > > static const struct spi_nor_fixups spansion_fixups = { > -- > 2.30.2 >
Am 2022-02-10 04:26, schrieb Tudor.Ambarus@microchip.com: > On 2/2/22 16:58, Michael Walle wrote: >> EXTERNAL EMAIL: Do not click links or open attachments unless you know >> the content is safe >> >> Increase readability of the code. Instead of returning early if the >> flash size is smaller or equal than 16MiB and then do the fixups for >> larger flashes, do it within the condition. >> > > mm, no, I'm not sure this improves readability, I see the two > equivalent. > The original version has the benefit of no indentation. Pratyush? This is a preparation patch for 12/14, where the current version isn't working anyway. If that is not enough reason why this is bad IMHO, I'll give you two more. I'd agree with you if that function was called spansion_late_init_smaller_flashes() or something like that. But it is a generic function valid for all flashes. And if you read it you might get the impression there are only flashes smaller or equal than 16MiB. You have to look twice to notice it was the intention that the assignment afterwards are just for the smaller flashes (and you will need to notice that there aren't any assignments for all spansion flashes). There is no direct connection between the assignment and the condition. Whereas with if (condition) { some_action(); } It is clear that some_action() was intended to only execute if condition is true. Also - and that is worse IMHO - it might easily be missed as someone just add stuff to the end of the function which might goes unnoticed but it won't work for flashes >16MiB. -michael
On 2/10/22 10:16, Michael Walle wrote: > EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe > > Am 2022-02-10 04:26, schrieb Tudor.Ambarus@microchip.com: >> On 2/2/22 16:58, Michael Walle wrote: >>> EXTERNAL EMAIL: Do not click links or open attachments unless you know >>> the content is safe >>> >>> Increase readability of the code. Instead of returning early if the >>> flash size is smaller or equal than 16MiB and then do the fixups for >>> larger flashes, do it within the condition. >>> >> >> mm, no, I'm not sure this improves readability, I see the two >> equivalent. >> The original version has the benefit of no indentation. Pratyush? > > This is a preparation patch for 12/14, where the current version isn't > working anyway. If that is not enough reason why this is bad IMHO, I'll > give you two more. you can put the + if (nor->flags & SNOR_F_USE_CLSR) + nor->params->ready = spi_nor_sr_ready_and_clear; above the size check and get rid of the prerequisite requirement, no? But it will look ugly indeed. If these two are so tightly related, how about squashing them? > > I'd agree with you if that function was called > spansion_late_init_smaller_flashes() or something like that. But it is > a generic function valid for all flashes. And if you read it you might > get the impression there are only flashes smaller or equal than 16MiB. > You have to look twice to notice it was the intention that the > assignment afterwards are just for the smaller flashes (and you will > need to notice that there aren't any assignments for all spansion > flashes). There is no direct connection between the assignment and > the condition. Whereas with > if (condition) { > some_action(); > } > It is clear that some_action() was intended to only execute if > condition is true. > > Also - and that is worse IMHO - it might easily be missed as someone > just add stuff to the end of the function which might goes unnoticed > but it won't work for flashes >16MiB. > You definitely care about it if you wrote such a long email :). I find the first argument the strongest, these two are biased IMO. I'm waiting for v2 with this change included! :) Cheers, ta
On 10/02/22 03:26AM, Tudor.Ambarus@microchip.com wrote: > On 2/2/22 16:58, Michael Walle wrote: > > EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe > > > > Increase readability of the code. Instead of returning early if the > > flash size is smaller or equal than 16MiB and then do the fixups for > > larger flashes, do it within the condition. > > > > mm, no, I'm not sure this improves readability, I see the two equivalent. > The original version has the benefit of no indentation. Pratyush? I am fine with both to be honest. But Michael's reasoning does make some sense to me. So, Reviewed-by: Pratyush Yadav <p.yadav@ti.com>
diff --git a/drivers/mtd/spi-nor/spansion.c b/drivers/mtd/spi-nor/spansion.c index 534196b1d3e7..dedc2de90cb8 100644 --- a/drivers/mtd/spi-nor/spansion.c +++ b/drivers/mtd/spi-nor/spansion.c @@ -296,13 +296,12 @@ static const struct flash_info spansion_parts[] = { static void spansion_late_init(struct spi_nor *nor) { - if (nor->params->size <= SZ_16M) - return; - - nor->flags |= SNOR_F_4B_OPCODES; - /* No small sector erase for 4-byte command set */ - nor->erase_opcode = SPINOR_OP_SE; - nor->mtd.erasesize = nor->info->sector_size; + if (nor->params->size > SZ_16M) { + nor->flags |= SNOR_F_4B_OPCODES; + /* No small sector erase for 4-byte command set */ + nor->erase_opcode = SPINOR_OP_SE; + nor->mtd.erasesize = nor->info->sector_size; + } } static const struct spi_nor_fixups spansion_fixups = {
Increase readability of the code. Instead of returning early if the flash size is smaller or equal than 16MiB and then do the fixups for larger flashes, do it within the condition. Signed-off-by: Michael Walle <michael@walle.cc> --- drivers/mtd/spi-nor/spansion.c | 13 ++++++------- 1 file changed, 6 insertions(+), 7 deletions(-)