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

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

Grant Likely wrote:

quoted

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 hate acronyms and shortening for the sake of it.

My IDE highlights known symbols from includes and lets me tab complete them :D

After all once all these APIs are fixed and most of the drivers are implemented
(most of them are, already, anyway, and have been from TaskSomething to sdma_
to bcom_ changes and major API reworks), I wonder why we have to constantly
cut every function definition down to 4 characters rhp_bjz_ywh_moo_purr()

I'd level the same thing at bcom_eng (what's an Eng when it's at home? English?
Engraved? Surely Engine but.. come on :)

There's no real good need to shorten the names of things except when those
shortenings are also used in the documentation - after all, PSC is what Freescale
call a PSC, we wouldn't be making structures called mpc52xx_programmable_serial_controller,
that's redundant, I don't think calling it "bestcomm" (which is it's name) over
"bcom" (which isn't) works to anyone's advantage here.

bcom is used consistently within this file and its use is unambiguous.
 It doesn't need to be changed for this submission.

quoted

quoted

quoted

+     /* Disable COMM Bus Prefetch, apparently it's not reliable yet */
+     /* 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.

Why not just accept the churn, and remove those two lines, and someone will
submit a patch to make it work on the 5200 in the appropriate place later?

Simple; it's not my series.  I'm taking the viewpoint of only changing
what is critical to change to get the code in.  Those 2 lines may be
sub-optimal; but they are not *bad* or *dangerous* and they're easily
removed later.  I'm pushing this change with my maintainer hat on; not
as the device driver developer and as such only making necessary
changes.  My view is that it is *safe* and *good* to merge this driver
as is so that the FEC and other drivers can finally get unblocked.
Send me a patch to change it.

g.

-- 
Grant Likely, B.Sc., P.Eng.
Secret Lab Technologies Ltd.
grant.likely@secretlab.ca
(403) 399-0195