Re: [PATCH v2 4/7] bestcomm: core bestcomm support for Freescale MPC5200

On 10/15/07, Matt Sealey [off-list ref] wrote:

quoted

My nits:

Grant Likely wrote:

quoted

From: Sylvain Munaut <redacted>
+static int __devinit
+bcom_engine_init(void)

Why "bcom" and not "bestcomm"?

I can type 'bcom' twice as fast.  :-)  bcom is a suitable shortening;
I'm not concerned about it.

I prefer bcom as well. Much shorter and there is no ambiguity.
If you use theses, you are writing a 5200 driver. In that context you
should be able to figure out what 'bcom' stands for ... (and if not, maybe
you shouldn't be using them ;)

quoted

quoted

+     /* Disable COMM Bus Prefetch, apparently it's not reliable yet

*/

quoted

+     /* FIXME: This should be done on 5200 and not 5200B ... */
+     out_be16(&bcom_eng->regs->PtdCntrl,

in_be16(&bcom_eng->regs->PtdCntrl) | 1);

This really, really shouldn't even be here, could it be moved to a
platform
init, or switched on a PVR/SVR here?

I think I'd like to leave it here for getting this series merged; it
may not be good to have it here; but it's not dangerous either.  I'm
trying to keep churn on this series down to a minimum.

Please submit a patch to make this change once it's merged.

Mmmm.
I think it _does_ belong here. COMM bus prefetch _is_ a bestcomm engine
option.

But indeed as the comment says, it maybe should be enabled on 5200B.
Not a big concern for me at the moment tough.



   Sylvain