Expanding Mailbox API

[Pavel Pisa]

Hello Mudit and Gedare,

I have returned from my mountain trip and digging through
e-mails flood, some oversight possible.

On Saturday 06 of August 2016 20:52:58 Gedare Bloom wrote:
> It would make sense to me to refactor this code. I can't recall if
> there was a compelling reason to have all of these different structs.

I have no strong opinion there.

The style has been introduced by Qiao Yang.
It is probably inspired mostly by U-boot code.
Linux kernel uses other style.

There are two structs for each request.
For example for BCM2835_MAILBOX_TAG_GET_BOARD_SERIAL operation
there is bcm2835_mbox_tag_get_board_serial which is internal
structure which requires correct alignment and padding for
passing to the VideoCore and minimized/VideoCore exchange
mechanism independent bcm2835_get_board_serial_entries structure
which includes only data interesting for user.

https://github.com/spark1729/rtems/commit/42442701293d4102f6987aae2c528d34c204f8d7

Form the amount of code perspective, it would be much easier
to build/serialize requests directly in the code VideoCore
access code (eliminate bcm2835_mbox_tag_get_board_serial
definition). But as a documentation for hardware/VideoCore
defined data format is definition of the structure quite
appropriate format.

The externally visible structure bcm2835_get_board_serial_entries
has not so much reasons to exists for simple services returning
one 32-bit numeric value. For the complex requests as video initialization,
the structure is appropriate because number of parameters is huge
and if there is need for some fields adjustment, function prototypes
adaptation gets complex (pthreads attributes style allows long term
source level compatibility better). Even for single value returning functions,
it is good practice to return success/fail result and separating
this information from actual data returned in referenced structure
can be better.

But for example for

int bcm2835_mailbox_get_clock_rate(
  bcm2835_get_clock_rate_entries *_entries );

use of prototype in the form

int bcm2835_mailbox_get_clock_rate( uint32_t clock_id,
                                    uint32_t *clock_rate_ptr);

could be much more straightformward and user friendly.
Question is if to left ptr to referenced location
for the return value as last argument or use it as the first
in this case. May it be for case get and set the last argument
is more logical (what the first and in/out value as the last).

This way next functions can be simplified

bcm2835_mailbox_get_pitch
bcm2835_mailbox_get_power_state
bcm2835_mailbox_set_power_state
bcm2835_mailbox_get_arm_memory
bcm2835_mailbox_get_vc_memory
bcm2835_mailbox_get_firmware_revision
bcm2835_mailbox_get_board_model
bcm2835_mailbox_get_board_revision
bcm2835_mailbox_get_board_serial
bcm2835_mailbox_get_clock_rate

The structures style should be kept for

bcm2835_mailbox_get_display_size
bcm2835_mailbox_init_frame_buffer

If style change is preferred, I change already included
functions as I find some time.

I would personally incline to commit proposed changes
as they are to move development and testing forward
and then do the simplification.

There are more things to clean up.
There is repeating functions calls sequence for each request
which should be hidden in some combined function.

  bcm2835_mailbox_buffer_flush_and_invalidate( &buffer, sizeof( &buffer ) );
  if ( bcm2835_mailbox_send_read_buffer( &buffer ) )
    return -1;
  if ( !bcm2835_mailbox_buffer_suceeded( &buffer.hdr ) )
    return -2;

As for the Mudit's work, it would worth to check if he can switch
to actual master (work seems to be based on Jul 19 master)
and check if all his tests works still with it.

Best wishes,

                 Pavel