From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from EUR03-VE1-obe.outbound.protection.outlook.com (mail-ve1eur03on0627.outbound.protection.outlook.com [IPv6:2a01:111:f400:fe09::627]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-SHA384 (256/256 bits)) (No client certificate requested) by ml01.01.org (Postfix) with ESMTPS id 73B1A8212F for ; Fri, 17 Feb 2017 07:08:49 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=armh.onmicrosoft.com; s=selector1-arm-com; h=From:Date:Subject:Message-ID:Content-Type:MIME-Version; bh=Hcc1T4k+tlQLcMjLwQMc9pdTh0LRSzO4ZjwyIDvXfck=; b=fyzYbgjFgVnRKiUNtlkXnZRmMuHTy9hW0aWrfbeWvAM+yjmJUn7O+kcn0fmTWwh/eAlPycQ+2SRNwGNEqZPCjv9e+NFW1svUYkcamppNnezztyRneC08YzpkGAjSgnnkLZ/AfRuT2JKZ/jJ+YT8UrLv36PvF/4KpeQPYK+E0YkU= Received: from AM5PR0801MB1955.eurprd08.prod.outlook.com (10.168.157.151) by AM5PR0801MB1762.eurprd08.prod.outlook.com (10.169.247.16) with Microsoft SMTP Server (version=TLS1_2, cipher=TLS_ECDHE_RSA_WITH_AES_256_CBC_SHA384_P384) id 15.1.888.16; Fri, 17 Feb 2017 15:08:46 +0000 Received: from AM5PR0801MB1955.eurprd08.prod.outlook.com ([10.168.157.151]) by AM5PR0801MB1955.eurprd08.prod.outlook.com ([10.168.157.151]) with mapi id 15.01.0919.013; Fri, 17 Feb 2017 15:08:46 +0000 From: Alexei Fedorov To: "ryan.harkin@linaro.org" , Evan Lloyd CC: "edk2-devel@ml01.01.org" , Leif Lindholm , "ard.biesheuvel@linaro.org" Thread-Topic: [edk2] [PATCH 4/4] ArmPkg:Provide GetTriggerType/SetTriggerType functions Thread-Index: AQHSiRZOdLQueTUDWkyzRVEiPm2rYKFtIP+AgAAqUNI= Date: Fri, 17 Feb 2017 15:08:46 +0000 Message-ID: References: <20170209192623.262044-1-evan.lloyd@arm.com> <20170209192623.262044-5-evan.lloyd@arm.com> <20170213121552.GU16034@bivouac.eciton.net> , In-Reply-To: Accept-Language: en-GB, en-US X-MS-Has-Attach: X-MS-TNEF-Correlator: authentication-results: spf=none (sender IP is ) smtp.mailfrom=Alexei.Fedorov@arm.com; x-originating-ip: [25.166.127.4] x-ld-processed: f34e5979-57d9-4aaa-ad4d-b122a662184d,ExtAddr x-ms-office365-filtering-correlation-id: cc39cb61-60db-4510-1e6c-08d45746df02 x-ms-office365-filtering-ht: Tenant x-microsoft-antispam: UriScan:; BCL:0; PCL:0; RULEID:(22001)(48565401081); SRVR:AM5PR0801MB1762; x-microsoft-exchange-diagnostics: 1; AM5PR0801MB1762; 7:mnIh/mEvhF3J05/Jg8aiMqyK+PtfTetEoZELl+g1/k9I0cZoAfpUY+psLAWCtp82ISvP9puuAQgbOHaR0xHLJJCgPXTVYa6zSmxtZbRveVLZBF4YrCyijhcu+VojrZ+9HqT5O4hvPhgBKEPFjh4H95TVlAgFtN2qrUJtXLOG6ogmM+lOulNOxQAqZ0CKgKwCEExhj3NhLcA5b+ACVsCQWYzlZ2WEIEl2bpxIJYVL6CMgLNFCS8nw/Sz2A59DYfMhCJcatmee5dXHQVZCO4AFPd5OVhIh0tj44vCHecy4a3c06HzACO4DZHTYphohUVTKBCl64JGmL12HaYEQf+Hncg== x-microsoft-antispam-prvs: x-exchange-antispam-report-test: UriScan:(158342451672863)(180628864354917)(162533806227266); x-exchange-antispam-report-cfa-test: BCL:0; PCL:0; RULEID:(6040375)(601004)(2401047)(5005006)(8121501046)(10201501046)(3002001)(6055026)(6041248)(20161123560025)(20161123564025)(20161123555025)(20161123558025)(20161123562025)(6072148); SRVR:AM5PR0801MB1762; BCL:0; PCL:0; RULEID:; SRVR:AM5PR0801MB1762; x-forefront-prvs: 02213C82F8 x-forefront-antispam-report: SFV:NSPM; SFS:(10009020)(7916002)(39840400002)(39860400002)(39850400002)(39450400003)(39410400002)(24454002)(199003)(40434004)(189002)(86362001)(606005)(6436002)(54356999)(76176999)(68736007)(50986999)(6506006)(6116002)(105586002)(55016002)(54906002)(236005)(102836003)(3846002)(54896002)(9686003)(5890100001)(2900100001)(106116001)(99286003)(25786008)(5660300001)(6306002)(101416001)(2501003)(6246003)(106356001)(189998001)(53936002)(77096006)(93886004)(81156014)(81166006)(6636002)(8936002)(8676002)(229853002)(33656002)(7906003)(7736002)(122556002)(3280700002)(38730400002)(92566002)(74316002)(2950100002)(3660700001)(7696004)(66066001)(2906002)(53546006)(97736004)(4326007); DIR:OUT; SFP:1101; SCL:1; SRVR:AM5PR0801MB1762; H:AM5PR0801MB1955.eurprd08.prod.outlook.com; FPR:; SPF:None; PTR:InfoNoRecords; A:1; MX:1; LANG:en; received-spf: None (protection.outlook.com: arm.com does not designate permitted sender hosts) spamdiagnosticoutput: 1:99 spamdiagnosticmetadata: NSPM MIME-Version: 1.0 X-OriginatorOrg: arm.com X-MS-Exchange-CrossTenant-originalarrivaltime: 17 Feb 2017 15:08:46.2724 (UTC) X-MS-Exchange-CrossTenant-fromentityheader: Hosted X-MS-Exchange-CrossTenant-id: f34e5979-57d9-4aaa-ad4d-b122a662184d X-MS-Exchange-Transport-CrossTenantHeadersStamped: AM5PR0801MB1762 X-Content-Filtered-By: Mailman/MimeDel 2.1.21 Subject: Re: [PATCH 4/4] ArmPkg:Provide GetTriggerType/SetTriggerType functions X-BeenThere: edk2-devel@lists.01.org X-Mailman-Version: 2.1.21 Precedence: list List-Id: EDK II Development List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Fri, 17 Feb 2017 15:08:50 -0000 Content-Language: en-GB Content-Type: text/plain; charset="Windows-1252" Content-Transfer-Encoding: quoted-printable Please take a look at the following code in GenericWatchdogEntry(): Status =3D mInterruptProtocol->RegisterInterruptSource ( mInterruptProtocol, FixedPcdGet32 (PcdGenericWatchdogEl2Int= rNum), WatchdogInterruptHandler ); if (!EFI_ERROR (Status)) { Status =3D mInterruptProtocol->SetTriggerType ( mInterruptProtocol, FixedPcdGet32 (PcdGenericWatchdogEl2Int= rNum), EFI_HARDWARE_INTERRUPT2_TRIGGER_EDGE_RI= SING); Why can't we extend EFI_HARDWARE_INTERRUPT2_PROTOCOL RegisterInterruptSourc= e() function API to have an extra EFI_HARDWARE_INTERRUPT2_TRIGGER_TYPE Tri= ggerType parameter to set interrupt type & get rid of the second call? Alexei. ________________________________ From: edk2-devel on behalf of Ryan Harkin= Sent: 17 February 2017 12:30:13 To: Evan Lloyd Cc: edk2-devel@ml01.01.org; Leif Lindholm; ard.biesheuvel@linaro.org Subject: Re: [edk2] [PATCH 4/4] ArmPkg:Provide GetTriggerType/SetTriggerTyp= e functions On 17 February 2017 at 12:06, Evan Lloyd wrote: > Hi Ryan. > > > > From: Ryan Harkin [mailto:ryan.harkin@linaro.org] > Sent: 16 February 2017 20:42 > To: Evan Lloyd > Cc: ard.biesheuvel@linaro.org; Leif Lindholm; edk2-devel@ml01.01.org > Subject: RE: [PATCH 4/4] ArmPkg:Provide GetTriggerType/SetTriggerType > functions > > > > > On 16 Feb 2017 20:27, "Evan Lloyd" wrote: >> >> Hi Leif. >> We accept all the comments except that about ternaries. >> Response inline. >> > =85 > >> >> + >> >> + *TriggerType =3D (MmioBitFieldRead32 (RegAddress, BitNumber, >> >BitNumber) =3D=3D 0) >> >> + ? EFI_HARDWARE_INTERRUPT2_TRIGGER_LEVEL_HIGH >> >> + : EFI_HARDWARE_INTERRUPT2_TRIGGER_EDGE_RISING; >> >> + >> > >> >Ternaries are excellent when they increase code readability. >> >I am not convinced that is the case here. >> > >> >Consider: >> > >> > if (MmioBitFieldRead32 (RegAddress, BitNumber, BitNumber) =3D=3D 0) { >> > *TriggerType =3D EFI_HARDWARE_INTERRUPT2_TRIGGER_LEVEL_HIGH; >> > } else { >> > *TriggerType =3D EFI_HARDWARE_INTERRUPT2_TRIGGER_EDGE_RISING; >> > } >> > >> >? >> > >> >The versions generate identical code with gcc at -O1 and above. >> >> Firstly, I'm not sure why 5 lines of code is more readable than 3. >> My main point though is that the ternary expression clearly indicates >> that all we are doing is setting *TriggerType. >> The multiple assignment "if" requires examination to determine that ther= e >> is nothing else going on. (Because otherwise why wouldn't you use a >> ternary?) >> I'm about to submit v2 without this, in the hope that I've made the case= . >> > > I find your argument unconvincing and would prefer an "if" clause. > > That is fine, lots of people have irrational prejudices. ;-) > > Do you have a counter argument though? > I don't think I need a 3rd argument. Like Leif, I don't think that ternary adds clarity and think an '"if" statement would be easier to decipher. > Leif points out that =93Ternaries are excellent when they increase code > readability.=94 > > The debate would thus seem to be a subjective assessment of =93readabilit= y=94. > Indeed it is. > What criteria should we use to identify when a ternary is more readable, = and > when not? > > And how do we ensure consistency across all EDK2 maintainers? > None of that is down to me. Cheers, Ryan. > Evan > > > > =85 > > > > IMPORTANT NOTICE: The contents of this email and any attachments are > confidential and may also be privileged. If you are not the intended > recipient, please notify the sender immediately and do not disclose the > contents to any other person, use it for any purpose, or store or copy th= e > information in any medium. Thank you. _______________________________________________ edk2-devel mailing list edk2-devel@lists.01.org https://lists.01.org/mailman/listinfo/edk2-devel IMPORTANT NOTICE: The contents of this email and any attachments are confid= ential and may also be privileged. If you are not the intended recipient, p= lease notify the sender immediately and do not disclose the contents to any= other person, use it for any purpose, or store or copy the information in = any medium. Thank you.