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

Laurent Pinchart
2024-03-20T21:00:23Z

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);

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

--
Regards,

Laurent Pinchart