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 <[email protected]>
---
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
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 <[email protected]>
> ---
> 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 [email protected]:
> 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 [email protected]:
>> 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, [email protected] 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 <[email protected]>
--
Regards,
Pratyush Yadav
Texas Instruments Inc.