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

["Grant Likely" <grant.likely@secretlab.ca>]

On 10/15/07, Matt Sealey <matt@genesi-usa.com> wrote:
>
> Grant Likely wrote:
> > On 10/15/07, Matt Sealey <matt@genesi-usa.com> wrote:
> >> My nits:
> >>
> >> Grant Likely wrote:
> >>> From: Sylvain Munaut <tnt@246tNt.com>
> >>> +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.

>
> >>> +     /* 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