Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1752376AbdFAQh4 (ORCPT ); Thu, 1 Jun 2017 12:37:56 -0400 Received: from smtp.codeaurora.org ([198.145.29.96]:46664 "EHLO smtp.codeaurora.org" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751620AbdFAQhx (ORCPT ); Thu, 1 Jun 2017 12:37:53 -0400 MIME-Version: 1.0 Content-Type: text/plain; charset=US-ASCII; format=flowed Content-Transfer-Encoding: 7bit Date: Thu, 01 Jun 2017 22:07:52 +0530 From: kgunda@codeaurora.org To: Stephen Boyd Cc: Abhijeet Dharmapurikar , Subbaraman Narayanamurthy , Greg Kroah-Hartman , David Collins , Christophe JAILLET , linux-kernel@vger.kernel.org, linux-arm-msm@vger.kernel.org, adharmap@quicinc.com, aghayal@qti.qualcomm.com, linux-arm-msm-owner@vger.kernel.org Subject: Re: [PATCH V1 03/15] spmi: pmic-arb: fix inconsistent use of apid and chan In-Reply-To: <20170531013146.GU20170@codeaurora.org> References: <1496147943-25822-1-git-send-email-kgunda@codeaurora.org> <1496147943-25822-4-git-send-email-kgunda@codeaurora.org> <20170531013146.GU20170@codeaurora.org> Message-ID: User-Agent: Roundcube Webmail/1.2.5 Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 4872 Lines: 140 On 2017-05-31 07:01, Stephen Boyd wrote: > On 05/30, Kiran Gunda wrote: >> From: Abhijeet Dharmapurikar >> >> The driver currently uses "apid" and "chan" to mean apid. Remove >> the use of chan and use only apid. > > I'm not so sure. It currently uses "chan" to mean the offset to > add to the "PMIC Arbiter channel registers" so that we can access > the appropriate peripheral via the arbiter registers. I actually > can't remember what APID or PPID stand for, so perhaps describing > that as well would be helpful so we can navigate this acronym > soup. > Yes. You are correct. Will describe the "apid" and "ppid" in the next version patch. >> >> On a SPMI bus there is allocation to manage up to 4K peripherals. >> However, in practice only few peripherals are instantiated >> and only few among the instantiated ones actually interrupt. >> >> APID is CPU's way of keeping track of peripherals that could >> interrupt. >> There is a table that maps the 256 interrupting peripherals to >> a number between 0 and 255. This number is called APID. Information >> about >> that interrupting peripheral is stored in registers offset by its >> corresponding apid. > > That's all fine, but perhaps we shouldn't worry about "apid" > being attached to interrupts? I mean, I can imagine some > peripheral that doesn't interrupt, but we want to read/write it > and that must be done with the "channel" or "apid" or really the > "magic offset from the base of the channel registers" to do so. > Probably APID is fine, as long as APID means "application > processor peripheral id" or something along those lines. > Yes. you are right. APID means "Application peripheral id". >> >> Signed-off-by: Abhijeet Dharmapurikar >> Signed-off-by: Kiran Gunda >> --- >> drivers/spmi/spmi-pmic-arb.c | 68 >> ++++++++++++++++++++++---------------------- >> 1 file changed, 34 insertions(+), 34 deletions(-) >> >> diff --git a/drivers/spmi/spmi-pmic-arb.c >> b/drivers/spmi/spmi-pmic-arb.c >> index 7f918ea..7201611 100644 >> --- a/drivers/spmi/spmi-pmic-arb.c >> +++ b/drivers/spmi/spmi-pmic-arb.c >> @@ -117,7 +117,7 @@ enum pmic_arb_cmd_op_code { >> * @spmic: SPMI controller object >> * @apid_to_ppid: in-memory copy of APID -> PPID mapping table. >> * @ver_ops: version dependent operations. >> - * @ppid_to_chan in-memory copy of PPID -> channel (APID) mapping >> table. >> + * @ppid_to_apid in-memory copy of PPID -> channel (APID) mapping >> table. > > PPID->APID? No channel? Sure. Will change it the next patch. > >> * v2 only. >> */ >> struct spmi_pmic_arb { >> @@ -140,9 +140,9 @@ struct spmi_pmic_arb { >> struct spmi_controller *spmic; >> u16 *apid_to_ppid; >> const struct pmic_arb_ver_ops *ver_ops; >> - u16 *ppid_to_chan; >> - u16 last_channel; >> - u8 *chan_to_owner; >> + u16 *ppid_to_apid; >> + u16 last_apid; >> + u8 *apid_to_owner; >> }; >> >> /** >> @@ -772,22 +772,22 @@ static int qpnpint_irq_domain_map(struct >> irq_domain *d, >> return 0; >> } >> >> -static u16 pmic_arb_find_chan(struct spmi_pmic_arb *pa, u16 ppid) >> +static u16 pmic_arb_find_apid(struct spmi_pmic_arb *pa, u16 ppid) >> { >> u32 regval, offset; >> - u16 chan; >> + u16 apid; >> u16 id; >> >> /* >> * PMIC_ARB_REG_CHNL is a table in HW mapping channel to ppid. > > Is this comment still relevant? We will change channel to apid in the next patch. > >> - * ppid_to_chan is an in-memory invert of that table. >> + * ppid_to_apid is an in-memory invert of that table. >> */ >> - for (chan = pa->last_channel; chan < pa->max_periph; chan++) { >> + for (apid = pa->last_apid; apid < pa->max_periph; apid++) { >> regval = readl_relaxed(pa->cnfg + >> - SPMI_OWNERSHIP_TABLE_REG(chan)); >> - pa->chan_to_owner[chan] = SPMI_OWNERSHIP_PERIPH2OWNER(regval); >> + SPMI_OWNERSHIP_TABLE_REG(apid)); >> + pa->apid_to_owner[apid] = SPMI_OWNERSHIP_PERIPH2OWNER(regval); >> >> - offset = PMIC_ARB_REG_CHNL(chan); >> + offset = PMIC_ARB_REG_CHNL(apid); >> if (offset >= pa->core_size) >> break; >> >> @@ -796,15 +796,15 @@ static u16 pmic_arb_find_chan(struct >> spmi_pmic_arb *pa, u16 ppid) >> continue; >> >> id = (regval >> 8) & PMIC_ARB_PPID_MASK; >> - pa->ppid_to_chan[id] = chan | PMIC_ARB_CHAN_VALID; >> + pa->ppid_to_apid[id] = apid | PMIC_ARB_CHAN_VALID; > > Why do we still call the flag PMIC_ARB_CHAN_VALID then? Shouldn't > it be PMIC_ARB_APID_VALID? > Yes. Agree. Will change it to PMIC_ARB_APID_VALID in the next patch. >> if (id == ppid) { >> - chan |= PMIC_ARB_CHAN_VALID; >> + apid |= PMIC_ARB_CHAN_VALID; >> break; >> } >> } >> - pa->last_channel = chan & ~PMIC_ARB_CHAN_VALID; >> + pa->last_apid = apid & ~PMIC_ARB_CHAN_VALID; >> >> - return chan; >> + return apid; >> } >> >>