Hi Gedare,
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.
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.
For example : *bcm2835_mbox_tag_get_arm_memory* &
*bcm2835_mbox_tag_get_vc_memory.
*This is the same for a lot of the mailbox code.
I can change it if that is suggested.
Thanks.
Mudit
On Thu, Jun 23, 2016 at 9:51 PM, Gedare Bloom <ged...@rtems.org> wrote:
> On Mon, Jun 20, 2016 at 5:17 PM, Mudit Jain <muditjain18011...@gmail.com>
> wrote:
> > From: muditj <spark1...@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@rtems.org
> > http://lists.rtems.org/mailman/listinfo/devel
>
_______________________________________________
devel mailing list
devel@rtems.org
http://lists.rtems.org/mailman/listinfo/devel
