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

[kgunda]

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