Re: [PATCH v1 11/14] mtd: spi-nor: spansion: slightly rework control flow in late_init()

[<Tudor.Ambarus@microchip.com>]

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