Re: [PATCH V1 03/15] spmi: pmic-arb: fix inconsistent use of apid and chan

[kgunda@codeaurora.org]

On 2017-05-31 07:01, Stephen Boyd wrote:
> On 05/30, Kiran Gunda wrote:
>> From: Abhijeet Dharmapurikar <adharmap@codeaurora.org>
>> 
>> 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 <adharmap@codeaurora.org>
>> Signed-off-by: Kiran Gunda <kgunda@codeaurora.org>
>> ---
>>  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;
>>  }
>> 
>>