[PATCH v2 2/4] boot: fdt: Clean up env_get_bootm_size()

[Marek Vasut]

On 3/20/24 10:00 PM, Laurent Pinchart wrote:
> On Wed, Mar 20, 2024 at 09:52:34PM +0100, Marek Vasut wrote:
>> On 3/18/24 5:18 PM, Laurent Pinchart wrote:
>>
>>>> @@ -142,7 +140,7 @@ phys_size_t env_get_bootm_size(void)
>>>>    
>>>>    	s = env_get("bootm_low");
>>>>    	if (s)
>>>> -		tmp = (phys_size_t)simple_strtoull(s, NULL, 16);
>>>> +		tmp = simple_strtoull(s, NULL, 16);
>>>>    	else
>>>>    		tmp = start;
>>>>    
>>>
>>> Maybe you could even drop the tmp variable completely by writing this
>>>
>>> 	if (s)
>>> 		size -= simple_strtoull(s, NULL, 16) - start;
>>>
>>> 	return size;
>>>
>>> I've never liked variables named tmp :-)
>>
>> No, let's not do this. With this FDT part, the code should be verbose
>> and as easy to understand at first glance as possible, no subtraction
>> assignments and other shenanigans please.
> 
> How about this ?
> 
> 	s = env_get("bootm_low");
> 	if (s) {
> 		phys_addr_t low_addr = simple_strtoull(s, NULL, 16);
>                  size -= low_addr - start;
> 	}
> 
> 	return size;
> 
> If you're going for readability, that's clearer than
> 
> 	return size - (tmp - start);

I do not share this opinion, the subtraction assignment makes this 
harder to read. If that makes for a more verbose code, so be it.

> and it also interestingly points out that tmp/low_addr should be a
> phys_addr_t, not a phys_size_t.

The original code already contains trivial 'tmp = start' assignment, so 
these two types should match, and they don't. Fixed in V4, thanks.