[U-Boot] [PATCH 0/5] Create an API for safely accessing BMP header fields

[Nikita Kiryanov]

Hi Albert ARIBAUD,

On 02/04/2013 05:25 PM, Albert ARIBAUD wrote:
> Hi Nikita,
>
> On Mon, 04 Feb 2013 17:07:05 +0200, Nikita Kiryanov
> <nikita at compulab.co.il> wrote:
>
>> On 02/04/2013 04:19 PM, Albert ARIBAUD wrote:
>>> Hi Nikita,
>>>
>>> On Mon, 04 Feb 2013 14:37:06 +0200, Nikita Kiryanov
>>> <nikita at compulab.co.il> wrote:
>>>
>>>> Hi Albert,
>>>>
>>>> On 02/04/2013 01:57 PM, Albert ARIBAUD wrote:
>>>>>
>>>>> IIRC, you has submitted a fix so that BMP loads would result in
>>>>> correctly aligned fields and thus no need for accessors. Why this
>>>>> change of mind?
>>>>    >
>>>>
>>>> I did mention that I consider that fix as temporary. I believe this fix
>>>> is better because it addresses the issue everywhere and does so in a
>>>> more maintainable way (does not require the address fix to be duplicated
>>>> everywhere there is a problem).
>>>
>>> I actually like the new fix less, as it adds an API where there is no
>>> need of one -- it's not like the implementation of BMP is at risk of
>>> changing. What is the problem in fixing the load address at load time
>>> and then passing this fixed address around?
>>
>> The problem is that it makes the U-Boot display subsystems broken in
>> terms of what kind of programming interface they provide.
>
> I'm not sure I understand what this means. Once all places that
> construct BMPs in memory are fixed so that they don't misalign, and
> documentation is there to tell about the issue, what exactly remains
> broken?

See below...

>
>> The load address fix has to be applied in every code path that leads
>> to a BMP being loaded and read in memory. This means that anyone who
>> wants to implement some BMP related functionality needs to care about
>> this architecture/memory issue, and actively circumvent it.
>
> Yes; but it is only a matter of getting the properly aligned address
> from the non-aligned one, which a simple macro is enough to provide;

I would've been in favour of the address fix solution if it was confined
to lcd.c, because then it would've been an implementation detail of
U-Boot's display subsystem, but it's not going to be necessarily
confined there. We have display_draw_bitmap() in the API, so users can
write standalone programs that display BMPs. Boards are required to
load the BMP to memory on their own, so that's another place where
there might be probing of the BMP header. So new code paths that lead
to BMP header probing are to be expected, and this means that we would
be adding a new requirement to anyone who wants to use BMPs.

> the rest, namely accessing fields, does not have to be turned into an API,
> nor does it benefit from it.

I think an API has benefits, namely that it simplifies the code a bit,
and it separates the alignment concern (handled with the compilation
flag) from the reading-BMP-data concern (handled by using the API).

Besides, switching to an API requires
> searching every place where BMPs are used, just like fixing addressing
> does.
>
>> This isn't good design. If I'm manipulating BMPs I should only care
>> about my BMPs, not hardware issues.
>
> Except that you have to, because you're not simply manipulating BMPs,
> you're manipulating them in a context where alignment is required.

That's true, but it's not an argument against simplifying the code.

>
> Amicalement,
>


-- 
Regards,
Nikita.