<div dir="ltr"><div><div><div><div><div>Hi Gedare,<br><br></div>When I use uncrustify, it makes a lot of changes to the previous code as well. We can have another patch over this for the cosmetic changes. <br><br></div>I myself, did have doubts regarding the empty struct and having two structs that are basically identical, however previously it was implemented in the similar fashion so I went on with it. <br>For example : <i>bcm2835_mbox_tag_get_arm_memory</i> & <i>bcm2835_mbox_tag_get_vc_memory. </i>This is the same for a lot of the mailbox code. <br><br></div>I can change it if that is suggested. <br><br></div>Thanks. <br></div>Mudit<br></div><div class="gmail_extra"><br><div class="gmail_quote">On Thu, Jun 23, 2016 at 9:51 PM, Gedare Bloom <span dir="ltr"><<a href="mailto:gedare@rtems.org" target="_blank">gedare@rtems.org</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><span class="">On Mon, Jun 20, 2016 at 5:17 PM, Mudit Jain <<a href="mailto:muditjain18011995@gmail.com">muditjain18011995@gmail.com</a>> wrote:<br>
> From: muditj <<a href="mailto:spark1729@yahoo.com">spark1729@yahoo.com</a>><br>
><br>
> Added functions for retrieving firmware revision,<br>
> board model and board revision.<br>
> ---<br>
> c/src/lib/libbsp/arm/raspberrypi/include/vc.h | 19 +++++++<br>
> c/src/lib/libbsp/arm/raspberrypi/misc/vc.c | 60 ++++++++++++++++++++++<br>
> c/src/lib/libbsp/arm/raspberrypi/misc/vc_defines.h | 20 ++++++++<br>
> 3 files changed, 99 insertions(+)<br>
><br>
> diff --git a/c/src/lib/libbsp/arm/raspberrypi/include/vc.h b/c/src/lib/libbsp/arm/raspberrypi/include/vc.h<br>
> index 4e91fde..dbf9812 100644<br>
> --- a/c/src/lib/libbsp/arm/raspberrypi/include/vc.h<br>
> +++ b/c/src/lib/libbsp/arm/raspberrypi/include/vc.h<br>
> @@ -135,6 +135,25 @@ typedef struct<br>
><br>
> int<br>
> bcm2835_mailbox_get_vc_memory(bcm2835_get_vc_memory_entries* _entries);<br>
> +<br>
> +typedef struct<br>
> +{<br>
</span>Put { on previous line. Be consistent and consider using a tool to<br>
check style conformance:<br>
<a href="https://devel.rtems.org/wiki/Developer/Coding/Conventions#Tools" rel="noreferrer" target="_blank">https://devel.rtems.org/wiki/Developer/Coding/Conventions#Tools</a><br>
<div><div class="h5"><br>
> + uint32_t fw_rev;<br>
> +} bcm2835_mailbox_get_fw_rev_entries;<br>
> +<br>
> +int<br>
> +bcm2835_mailbox_get_firmware_revision(bcm2835_mailbox_get_fw_rev_entries* _entries);<br>
> +<br>
> +typedef struct<br>
> +{<br>
> + uint32_t spec;<br>
> +} bcm2835_get_board_spec_entries;<br>
> +<br>
> +int<br>
> +bcm2835_mailbox_get_board_model(bcm2835_get_board_spec_entries* _entries);<br>
> +<br>
> +int<br>
> +bcm2835_mailbox_get_board_revision(bcm2835_get_board_spec_entries* _entries);<br>
> /** @} */<br>
><br>
> #endif /* LIBBSP_ARM_RASPBERRYPI_VC_H */<br>
> diff --git a/c/src/lib/libbsp/arm/raspberrypi/misc/vc.c b/c/src/lib/libbsp/arm/raspberrypi/misc/vc.c<br>
> index 91f9174..d27668d 100644<br>
> --- a/c/src/lib/libbsp/arm/raspberrypi/misc/vc.c<br>
> +++ b/c/src/lib/libbsp/arm/raspberrypi/misc/vc.c<br>
> @@ -278,3 +278,63 @@ bcm2835_mailbox_get_vc_memory(bcm2835_get_vc_memory_entries* _entries)<br>
> return -2;<br>
> return 0;<br>
> }<br>
> +<br>
> +int<br>
> +bcm2835_mailbox_get_firmware_revision(bcm2835_mailbox_get_fw_rev_entries* _entries)<br>
> +{<br>
> + struct{<br>
> + bcm2835_mbox_buf_hdr hdr;<br>
> + bcm2835_mbox_tag_get_fw_rev get_fw_rev;<br>
> + uint32_t end_tag;<br>
> + }buffer BCM2835_MBOX_BUF_ALIGN_ATTRIBUTE;<br>
</div></div>Add some white space around brackets. Generally, this is a dense<br>
function to read. Also, not sure why there is locally defined struct<br>
here, should this struct be defined elsewhere? It looks like the exact<br>
same struct is used below with slightly different field names only.<br>
<span class=""><br>
> + BCM2835_MBOX_INIT_BUF(&buffer);<br>
> + BCM2835_MBOX_INIT_TAG_NO_REQ(&buffer.get_fw_rev,<br>
> + BCM2835_MAILBOX_TAG_FIRMWARE_REVISION);<br>
> + bcm2835_mailbox_buffer_flush_and_invalidate(&buffer, sizeof(&buffer));<br>
</span>does sizeof(&buffer) work as you expect? or do you want sizeof(buffer)?<br>
<span class=""><br>
> + if (bcm2835_mailbox_send_read_buffer(&buffer))<br>
> + return -1;<br>
> + _entries->fw_rev = buffer.get_fw_rev.body.resp.rev;<br>
> + if( !bcm2835_mailbox_buffer_suceeded(&buffer.hdr) )<br>
> + return -2;<br>
</span>It would be nice to have defined error codes.<br>
<br>
The same comments apply below.<br>
<div><div class="h5"><br>
> + return 0;<br>
> +}<br>
> +<br>
> +int<br>
> +bcm2835_mailbox_get_board_model(bcm2835_get_board_spec_entries* _entries)<br>
> +{<br>
> + struct{<br>
> + bcm2835_mbox_buf_hdr hdr;<br>
> + bcm2835_mbox_tag_get_board_spec get_board_model;<br>
> + uint32_t end_tag;<br>
> + }buffer BCM2835_MBOX_BUF_ALIGN_ATTRIBUTE;<br>
> + BCM2835_MBOX_INIT_BUF(&buffer);<br>
> + BCM2835_MBOX_INIT_TAG_NO_REQ(&buffer.get_board_model,<br>
> + BCM2835_MAILBOX_TAG_GET_BOARD_MODEL);<br>
> + bcm2835_mailbox_buffer_flush_and_invalidate(&buffer, sizeof(&buffer));<br>
> + if (bcm2835_mailbox_send_read_buffer(&buffer))<br>
> + return -1;<br>
> + _entries->spec = buffer.get_board_model.body.resp.spec;<br>
> + if( !bcm2835_mailbox_buffer_suceeded(&buffer.hdr) )<br>
> + return -2;<br>
> + return 0;<br>
> +}<br>
> +<br>
> +int<br>
> +bcm2835_mailbox_get_board_revision(bcm2835_get_board_spec_entries* _entries)<br>
> +{<br>
> + struct{<br>
> + bcm2835_mbox_buf_hdr hdr;<br>
> + bcm2835_mbox_tag_get_board_spec get_board_revision;<br>
> + uint32_t end_tag;<br>
> + }buffer BCM2835_MBOX_BUF_ALIGN_ATTRIBUTE;<br>
> + BCM2835_MBOX_INIT_BUF(&buffer);<br>
> + BCM2835_MBOX_INIT_TAG_NO_REQ(&buffer.get_board_revision,<br>
> + BCM2835_MAILBOX_TAG_GET_BOARD_VERSION);<br>
> + bcm2835_mailbox_buffer_flush_and_invalidate(&buffer, sizeof(&buffer));<br>
> + if (bcm2835_mailbox_send_read_buffer(&buffer))<br>
> + return -1;<br>
> + _entries->spec = buffer.get_board_revision.body.resp.spec;<br>
> + if( !bcm2835_mailbox_buffer_suceeded(&buffer.hdr) )<br>
> + return -2;<br>
> + return 0;<br>
> +}<br>
> diff --git a/c/src/lib/libbsp/arm/raspberrypi/misc/vc_defines.h b/c/src/lib/libbsp/arm/raspberrypi/misc/vc_defines.h<br>
> index f3d5a28..1b2bf92 100644<br>
> --- a/c/src/lib/libbsp/arm/raspberrypi/misc/vc_defines.h<br>
> +++ b/c/src/lib/libbsp/arm/raspberrypi/misc/vc_defines.h<br>
> @@ -135,10 +135,30 @@ typedef struct {<br>
><br>
> /* Video Core */<br>
> #define BCM2835_MAILBOX_TAG_FIRMWARE_REVISION 0x00000001<br>
> +typedef struct {<br>
> + bcm2835_mbox_tag_hdr tag_hdr;<br>
> + union {<br>
> + struct {<br>
> + } req;<br>
</div></div>an empty struct? what's the point for this?<br>
<span class=""><br>
> + struct {<br>
> + uint32_t rev;<br>
> + } resp;<br>
> + } body;<br>
> +} bcm2835_mbox_tag_get_fw_rev;<br>
><br>
> /* Hardware */<br>
> #define BCM2835_MAILBOX_TAG_GET_BOARD_MODEL 0x00010001<br>
> #define BCM2835_MAILBOX_TAG_GET_BOARD_VERSION 0x00010002<br>
> +typedef struct {<br>
> + bcm2835_mbox_tag_hdr tag_hdr;<br>
> + union {<br>
> + struct {<br>
> + } req;<br>
> + struct {<br>
> + uint32_t spec;<br>
> + } resp;<br>
> + } body;<br>
> +} bcm2835_mbox_tag_get_board_spec;<br>
><br>
</span>These two structs are basically identical. Do we care to distinguish them?<br>
<span class=""><br>
> #if (BSP_IS_RPI2 == 1)<br>
> #define BCM2836_MAILBOX_BOARD_V_2_B 0x4<br>
> --<br>
> 1.9.1<br>
><br>
</span>> _______________________________________________<br>
> devel mailing list<br>
> <a href="mailto:devel@rtems.org">devel@rtems.org</a><br>
> <a href="http://lists.rtems.org/mailman/listinfo/devel" rel="noreferrer" target="_blank">http://lists.rtems.org/mailman/listinfo/devel</a><br>
</blockquote></div><br></div>