From owner-svn-src-head@FreeBSD.ORG Mon Jul 1 01:56:38 2013 Return-Path: Delivered-To: svn-src-head@FreeBSD.org Received: from mx1.freebsd.org (mx1.freebsd.org [IPv6:2001:1900:2254:206a::19:1]) by hub.freebsd.org (Postfix) with ESMTP id 60F92CB6; Mon, 1 Jul 2013 01:56:38 +0000 (UTC) (envelope-from brde@optusnet.com.au) Received: from mail108.syd.optusnet.com.au (mail108.syd.optusnet.com.au [211.29.132.59]) by mx1.freebsd.org (Postfix) with ESMTP id DDD6915F8; Mon, 1 Jul 2013 01:56:37 +0000 (UTC) Received: from c122-106-156-23.carlnfd1.nsw.optusnet.com.au (c122-106-156-23.carlnfd1.nsw.optusnet.com.au [122.106.156.23]) by mail108.syd.optusnet.com.au (Postfix) with ESMTPS id 39A861A3DAD; Mon, 1 Jul 2013 11:56:34 +1000 (EST) Date: Mon, 1 Jul 2013 11:56:25 +1000 (EST) From: Bruce Evans X-X-Sender: bde@besplex.bde.org To: Aleksandr Rybalko Subject: Re: svn commit: r252425 - head/sys/arm/arm In-Reply-To: <201306301952.r5UJqfwf010873@svn.freebsd.org> Message-ID: <20130701113808.F894@besplex.bde.org> References: <201306301952.r5UJqfwf010873@svn.freebsd.org> MIME-Version: 1.0 Content-Type: TEXT/PLAIN; charset=US-ASCII; format=flowed X-Optus-CM-Score: 0 X-Optus-CM-Analysis: v=2.0 cv=Q6eKePKa c=1 sm=1 a=tB-szSHScXMA:10 a=kj9zAlcOel0A:10 a=PO7r1zJSAAAA:8 a=JzwRw_2MAAAA:8 a=E11o_6pwCccA:10 a=i2sZn5OXAAAA:8 a=DWOTAInQcCBj81PfPZgA:9 a=CjuIK1q_8ugA:10 a=PObHtDWs1CoA:10 a=Eo4AgIMMjYkUubzM:21 a=2b1o89hOhl2EU1TR:21 a=ebeQFi2P/qHVC0Yw9JDJ4g==:117 Cc: svn-src-head@FreeBSD.org, svn-src-all@FreeBSD.org, src-committers@FreeBSD.org X-BeenThere: svn-src-head@freebsd.org X-Mailman-Version: 2.1.14 Precedence: list List-Id: SVN commit messages for the src tree for head/-current List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Mon, 01 Jul 2013 01:56:38 -0000 On Sun, 30 Jun 2013, Aleksandr Rybalko wrote: > Log: > Decrypt magic numbers - define names for fields of Generic Timer's CNTKCTL reg. > > Submitted by: Ruslan Bukin > > Modified: > head/sys/arm/arm/generic_timer.c > > Modified: head/sys/arm/arm/generic_timer.c > ============================================================================== > --- head/sys/arm/arm/generic_timer.c Sun Jun 30 19:36:17 2013 (r252424) > +++ head/sys/arm/arm/generic_timer.c Sun Jun 30 19:52:41 2013 (r252425) > @@ -66,7 +66,22 @@ __FBSDID("$FreeBSD$"); > #define GENERIC_TIMER_REG_CTRL 0 > #define GENERIC_TIMER_REG_TVAL 1 > > -#define CNTPSIRQ 29 > +#define GENERIC_TIMER_CNTKCTL_PL0PTEN (1 << 9) /* Physical timer registers > + access from PL0 */ > +#define GENERIC_TIMER_CNTKCTL_PL0VTEN (1 << 8) /* Virtual timer registers With names like these, the magic numbers are better. The prefix name GENERIC_TIMER is especially bad. GT would be good. > ... > +#define GENERIC_TIMER_CNTPSIRQ 29 Here the interesting part CNTPSIRQ is fairly abbreviated, but its prefix is not. > > struct arm_tmr_softc { > struct resource *irq_res; > @@ -167,7 +182,11 @@ disable_user_access(void) > uint32_t cntkctl; > > __asm volatile("mrc p15, 0, %0, c14, c1, 0" : "=r" (cntkctl)); > - cntkctl &= ~((3 << 8) | (7 << 0)); > + cntkctl &= ~(GENERIC_TIMER_CNTKCTL_PL0PTEN | > + GENERIC_TIMER_CNTKCTL_PL0VTEN | > + GENERIC_TIMER_CNTKCTL_EVNTEN | > + GENERIC_TIMER_CNTKCTL_PL0VCTEN | > + GENERIC_TIMER_CNTKCTL_PL0PCTEN); > __asm volatile("mcr p15, 0, %0, c14, c1, 0" : : "r" (cntkctl)); > isb(); > } Using these verbose names takes about 15 times as much code as before (the statement is only 5 times longer, but the definitions macros are about 10 times longer), and looks like even more becayse the statement is misformatted with non-KNF indentation which then requires extra line spitting with only 1 name per line. > @@ -270,7 +289,8 @@ arm_tmr_attach(device_t dev) > > rid = 0; > sc->irq_res = bus_alloc_resource(dev, SYS_RES_IRQ, &rid, > - CNTPSIRQ, CNTPSIRQ, 1, RF_SHAREABLE | RF_ACTIVE); > + GENERIC_TIMER_CNTPSIRQ, GENERIC_TIMER_CNTPSIRQ, > + 1, RF_SHAREABLE | RF_ACTIVE); > > arm_tmr_sc = sc; For full uglyness, expand all the prefixes and names: - SYS -> SYSTEM - RES -> RESOURCE - IRQ -> INTERRUPT_REQUEST_NUMBER - SYS_RES_IRQ -> SYSTEM_RESOURCE_INTERRUPT_REQUEST_NUMBER - RF -> RESOURCE_FLAG - RF_SHAREABLE_RESOURCE_FLAG_SHAREABLE - RF_ACTIVE -> RESOURCE_FLAG_ACTIVE - CNTPSIRQ -> COUNT_PRIVATELY_SOURCED_INTERRUPT_REQUEST_NUMBER (just guessing what PS means): sc->irq_res = bus_alloc_resource(dev, SYS_RES_INTERRUPT_REQUEST_NUMBER, &rid, GENERIC_TIMER_COUNT_PRIVATELY_SOURCED_INTERRUPT_REQUEST_NUMBER, GENERIC_TIMER_COUNT_PRIVATELY_SOURCED_INTERRUPT_REQUEST_NUMBER, 1, RESOURCE_FLAG_SHAREABLE | RESOURCE_FLAG_ACTIVE); Next, use non-KNF indentation: sc->irq_res = bus_alloc_resource(dev, SYS_RES_INTERRUPT_REQUEST_NUMBER, &rid, GENERIC_TIMER_COUNT_PRIVATELY_SOURCED_INTERRUPT_REQUEST_NUMBER, GENERIC_TIMER_COUNT_PRIVATELY_SOURCED_INTERRUPT_REQUEST_NUMBER, 1, RESOURCE_FLAG_SHAREABLE | RESOURCE_FLAG_ACTIVE); The names aren't even 80 characters long, so they actually fit on 1 line. Bruce