[PATCH] Adding functionalities to Mailbox RPi

[Gedare Bloom]

On Mon, Jun 20, 2016 at 5:17 PM, Mudit Jain <muditjain18011995 at gmail.com> wrote:
> From: muditj <spark1729 at yahoo.com>
>
> Added functions for retrieving firmware revision,
> board model and board revision.
> ---
>  c/src/lib/libbsp/arm/raspberrypi/include/vc.h      | 19 +++++++
>  c/src/lib/libbsp/arm/raspberrypi/misc/vc.c         | 60 ++++++++++++++++++++++
>  c/src/lib/libbsp/arm/raspberrypi/misc/vc_defines.h | 20 ++++++++
>  3 files changed, 99 insertions(+)
>
> diff --git a/c/src/lib/libbsp/arm/raspberrypi/include/vc.h b/c/src/lib/libbsp/arm/raspberrypi/include/vc.h
> index 4e91fde..dbf9812 100644
> --- a/c/src/lib/libbsp/arm/raspberrypi/include/vc.h
> +++ b/c/src/lib/libbsp/arm/raspberrypi/include/vc.h
> @@ -135,6 +135,25 @@ typedef struct
>
>  int
>  bcm2835_mailbox_get_vc_memory(bcm2835_get_vc_memory_entries* _entries);
> +
> +typedef struct
> +{
Put { on previous line. Be consistent and consider using a tool to
check style conformance:
https://devel.rtems.org/wiki/Developer/Coding/Conventions#Tools

> +  uint32_t fw_rev;
> +} bcm2835_mailbox_get_fw_rev_entries;
> +
> +int
> +bcm2835_mailbox_get_firmware_revision(bcm2835_mailbox_get_fw_rev_entries* _entries);
> +
> +typedef struct
> +{
> +  uint32_t spec;
> +} bcm2835_get_board_spec_entries;
> +
> +int
> +bcm2835_mailbox_get_board_model(bcm2835_get_board_spec_entries* _entries);
> +
> +int
> +bcm2835_mailbox_get_board_revision(bcm2835_get_board_spec_entries* _entries);
>  /** @} */
>
>  #endif /* LIBBSP_ARM_RASPBERRYPI_VC_H */
> diff --git a/c/src/lib/libbsp/arm/raspberrypi/misc/vc.c b/c/src/lib/libbsp/arm/raspberrypi/misc/vc.c
> index 91f9174..d27668d 100644
> --- a/c/src/lib/libbsp/arm/raspberrypi/misc/vc.c
> +++ b/c/src/lib/libbsp/arm/raspberrypi/misc/vc.c
> @@ -278,3 +278,63 @@ bcm2835_mailbox_get_vc_memory(bcm2835_get_vc_memory_entries* _entries)
>      return -2;
>    return 0;
>  }
> +
> +int
> +bcm2835_mailbox_get_firmware_revision(bcm2835_mailbox_get_fw_rev_entries* _entries)
> +{
> +  struct{
> +    bcm2835_mbox_buf_hdr hdr;
> +    bcm2835_mbox_tag_get_fw_rev get_fw_rev;
> +    uint32_t end_tag;
> +  }buffer BCM2835_MBOX_BUF_ALIGN_ATTRIBUTE;
Add some white space around brackets. Generally, this is a dense
function to read. Also, not sure why there is locally defined struct
here, should this struct be defined elsewhere? It looks like the exact
same struct is used below with slightly different field names only.

> +  BCM2835_MBOX_INIT_BUF(&buffer);
> +  BCM2835_MBOX_INIT_TAG_NO_REQ(&buffer.get_fw_rev,
> +    BCM2835_MAILBOX_TAG_FIRMWARE_REVISION);
> +  bcm2835_mailbox_buffer_flush_and_invalidate(&buffer, sizeof(&buffer));
does sizeof(&buffer) work as you expect? or do you want sizeof(buffer)?

> +  if (bcm2835_mailbox_send_read_buffer(&buffer))
> +    return -1;
> +  _entries->fw_rev = buffer.get_fw_rev.body.resp.rev;
> +  if( !bcm2835_mailbox_buffer_suceeded(&buffer.hdr) )
> +    return -2;
It would be nice to have defined error codes.

The same comments apply below.

> +  return 0;
> +}
> +
> +int
> +bcm2835_mailbox_get_board_model(bcm2835_get_board_spec_entries* _entries)
> +{
> +  struct{
> +    bcm2835_mbox_buf_hdr hdr;
> +    bcm2835_mbox_tag_get_board_spec get_board_model;
> +    uint32_t end_tag;
> +  }buffer BCM2835_MBOX_BUF_ALIGN_ATTRIBUTE;
> +  BCM2835_MBOX_INIT_BUF(&buffer);
> +  BCM2835_MBOX_INIT_TAG_NO_REQ(&buffer.get_board_model,
> +    BCM2835_MAILBOX_TAG_GET_BOARD_MODEL);
> +  bcm2835_mailbox_buffer_flush_and_invalidate(&buffer, sizeof(&buffer));
> +  if (bcm2835_mailbox_send_read_buffer(&buffer))
> +    return -1;
> +  _entries->spec = buffer.get_board_model.body.resp.spec;
> +  if( !bcm2835_mailbox_buffer_suceeded(&buffer.hdr) )
> +    return -2;
> +  return 0;
> +}
> +
> +int
> +bcm2835_mailbox_get_board_revision(bcm2835_get_board_spec_entries* _entries)
> +{
> +  struct{
> +    bcm2835_mbox_buf_hdr hdr;
> +    bcm2835_mbox_tag_get_board_spec get_board_revision;
> +    uint32_t end_tag;
> +  }buffer BCM2835_MBOX_BUF_ALIGN_ATTRIBUTE;
> +  BCM2835_MBOX_INIT_BUF(&buffer);
> +  BCM2835_MBOX_INIT_TAG_NO_REQ(&buffer.get_board_revision,
> +    BCM2835_MAILBOX_TAG_GET_BOARD_VERSION);
> +  bcm2835_mailbox_buffer_flush_and_invalidate(&buffer, sizeof(&buffer));
> +  if (bcm2835_mailbox_send_read_buffer(&buffer))
> +    return -1;
> +  _entries->spec = buffer.get_board_revision.body.resp.spec;
> +  if( !bcm2835_mailbox_buffer_suceeded(&buffer.hdr) )
> +    return -2;
> +  return 0;
> +}
> diff --git a/c/src/lib/libbsp/arm/raspberrypi/misc/vc_defines.h b/c/src/lib/libbsp/arm/raspberrypi/misc/vc_defines.h
> index f3d5a28..1b2bf92 100644
> --- a/c/src/lib/libbsp/arm/raspberrypi/misc/vc_defines.h
> +++ b/c/src/lib/libbsp/arm/raspberrypi/misc/vc_defines.h
> @@ -135,10 +135,30 @@ typedef struct {
>
>  /* Video Core */
>  #define BCM2835_MAILBOX_TAG_FIRMWARE_REVISION   0x00000001
> +typedef struct {
> +  bcm2835_mbox_tag_hdr tag_hdr;
> +  union {
> +    struct {
> +    } req;
an empty struct? what's the point for this?

> +    struct {
> +      uint32_t rev;
> +    } resp;
> +  } body;
> +} bcm2835_mbox_tag_get_fw_rev;
>
>  /* Hardware */
>  #define BCM2835_MAILBOX_TAG_GET_BOARD_MODEL     0x00010001
>  #define BCM2835_MAILBOX_TAG_GET_BOARD_VERSION   0x00010002
> +typedef struct {
> +  bcm2835_mbox_tag_hdr tag_hdr;
> +  union {
> +    struct {
> +    } req;
> +    struct {
> +      uint32_t spec;
> +    } resp;
> +  } body;
> +} bcm2835_mbox_tag_get_board_spec;
>
These two structs are basically identical. Do we care to distinguish them?

>  #if (BSP_IS_RPI2 == 1)
>  #define BCM2836_MAILBOX_BOARD_V_2_B             0x4
> --
> 1.9.1
>
> _______________________________________________
> devel mailing list
> devel at rtems.org
> http://lists.rtems.org/mailman/listinfo/devel