<div dir="ltr"><div><div><div>Hi,<br><br></div>Any consensus regarding the issue ? <br></div><div>Do we go forward with the present implementation or a change is required ?<br><br></div><div>I have another change for getting and setting the clock rate which is by the driver.<br></div><div>I can push the same along with the cosmetic changes done by uncrustify. <br><br></div><div>Thanks <br></div><div>Mudit <br></div><div><br></div></div></div><div class="gmail_extra"><br><div class="gmail_quote">On Fri, Jun 24, 2016 at 3:56 PM, Pavel Pisa <span dir="ltr"><<a href="mailto:ppisa4lists@pikron.com" target="_blank">ppisa4lists@pikron.com</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">Hello Mudit and Gedare,<br>
<span class=""><br>
On Friday 24 of June 2016 12:02:36 Mudit Jain wrote:<br>
> Hi Gedare,<br>
><br>
> When I use uncrustify, it makes a lot of changes to the previous code as<br>
> well. We can have another patch over this for the cosmetic changes.<br>
><br>
> I myself, did have doubts regarding the empty struct and having two structs<br>
> that are basically identical, however previously it was implemented in the<br>
> similar fashion so I went on with it.<br>
</span>> For example : *bcm2835_mbox_tag_get_arm_memory* &<br>
> *bcm2835_mbox_tag_get_vc_memory.<br>
> *This is the same for a lot of the mailbox code.<br>
<br>
I agree that uncrustify should be run before or after changes<br>
and result committed as separate patch.<br>
Suggestion if to run before or after Mudit's patch is a question.<br>
But may it be after would be simpler because actual patch version<br>
is tested and works.<br>
<br>
As for empty request struck, I am not sure about it.<br>
It declares that there are no data send to VC for given<br>
function and all data exchanges have defined "req"<br>
and "resp" fields in their buffer format structures<br>
now. The presence of the field (even empty) can make<br>
some access macros more generic. But I am not sure if this<br>
can be required in future. So removal of empty members<br>
can be done.<br>
<br>
As for two almost same structures, it is questionale as well.<br>
May it be, we should have three in the fact<br>
bcm2835_mbox_tag_get_fw_rev - .rev<br>
bcm2835_mbox_tag_get_board_model - .model<br>
bcm2835_mbox_tag_get_board_version - .version<br>
<br>
May it be actual two are ok<br>
<br>
bcm2835_mbox_tag_get_fw_rev - .rev<br>
  (this is of different/strange meaning 1461755178 = 0x57209d2a)<br>
  on my board and the number is confirmed from some other<br>
  source as valid firmware revision - may it be that is checksum?<br>
<br>
bcm2835_mbox_tag_get_board_spec - .spec<br>
  (on the other hand these are sane small documented numbers<br>
   0, 20 = 0x14)<br>
<br>
May be there should be only one structure<br>
<br>
bcm2835_mbox_tag_get_uint32 - .value<br>
<br>
which can be used directly or bcm2835_mbox_tag_get_fw_rev,<br>
bcm2835_mbox_tag_get_board_model, bcm2835_mbox_tag_get_board_version<br>
can be derived from that. It would be possible to have only<br>
single function in that case as well.<br>
<br>
But generally, I have no preference there and actual Mudit proposed<br>
solution is acceptable by me.<br>
<br>
Best wishes,<br>
<br>
             Pavel<br>
<br>
</blockquote></div><br></div>