[PATCH] Adding functionalities to Mailbox RPi

[Mudit Jain]

Hi,

Any consensus regarding the issue ?
Do we go forward with the present implementation or a change is required ?

I have another change for getting and setting the clock rate which is by
the driver.
I can push the same along with the cosmetic changes done by uncrustify.

Thanks
Mudit


On Fri, Jun 24, 2016 at 3:56 PM, Pavel Pisa <ppisa4lists at pikron.com> wrote:

> Hello Mudit and Gedare,
>
> On Friday 24 of June 2016 12:02:36 Mudit Jain wrote:
> > 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 agree that uncrustify should be run before or after changes
> and result committed as separate patch.
> Suggestion if to run before or after Mudit's patch is a question.
> But may it be after would be simpler because actual patch version
> is tested and works.
>
> As for empty request struck, I am not sure about it.
> It declares that there are no data send to VC for given
> function and all data exchanges have defined "req"
> and "resp" fields in their buffer format structures
> now. The presence of the field (even empty) can make
> some access macros more generic. But I am not sure if this
> can be required in future. So removal of empty members
> can be done.
>
> As for two almost same structures, it is questionale as well.
> May it be, we should have three in the fact
> bcm2835_mbox_tag_get_fw_rev - .rev
> bcm2835_mbox_tag_get_board_model - .model
> bcm2835_mbox_tag_get_board_version - .version
>
> May it be actual two are ok
>
> bcm2835_mbox_tag_get_fw_rev - .rev
>   (this is of different/strange meaning 1461755178 = 0x57209d2a)
>   on my board and the number is confirmed from some other
>   source as valid firmware revision - may it be that is checksum?
>
> bcm2835_mbox_tag_get_board_spec - .spec
>   (on the other hand these are sane small documented numbers
>    0, 20 = 0x14)
>
> May be there should be only one structure
>
> bcm2835_mbox_tag_get_uint32 - .value
>
> which can be used directly or bcm2835_mbox_tag_get_fw_rev,
> bcm2835_mbox_tag_get_board_model, bcm2835_mbox_tag_get_board_version
> can be derived from that. It would be possible to have only
> single function in that case as well.
>
> But generally, I have no preference there and actual Mudit proposed
> solution is acceptable by me.
>
> Best wishes,
>
>              Pavel
>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.rtems.org/pipermail/devel/attachments/20160627/fef99458/attachment.html>