Date: Thu, 19 Feb 2015 10:29:52 -0700
From: Gilad Avidov <gavidov@codeaurora.org>
To: Josh Cartwright <joshc@eso.teric.us>
Cc: sdharia@codeaurora.org, mlocke@codeaurora.org,
        linux-arm-msm@vger.kernel.org, gregkh@linuxfoundation.org,
        linux-kernel@vger.kernel.org, iivanov@mm-sol.com, galak@codeaurora.org,
        agross@codeaurora.org
Subject: Re: [PATCH V3 2/2] spmi: pmic_arb: add support for hw version 2
Message-ID: <20150219102952.469e7754@gavidov-lnx.qualcomm.com>
In-Reply-To: <20150218153446.GA3485@kryptos>
References: <1423522272-24472-1-git-send-email-gavidov@codeaurora.org>
	<1423522272-24472-3-git-send-email-gavidov@codeaurora.org>
	<20150218153446.GA3485@kryptos>
Organization: Qualcomm Innovation Center, Inc.
MIME-Version: 1.0
Content-Type: text/plain; charset=US-ASCII
Content-Transfer-Encoding: 7bit
Sender: linux-kernel-owner@vger.kernel.org
Content-Length: 3674
Lines: 119

Hi Josh,

Thank you for the review.

On Wed, 18 Feb 2015 09:34:46 -0600
Josh Cartwright <joshc@eso.teric.us> wrote:

> Hey Gilad-
> 
> On Mon, Feb 09, 2015 at 03:51:12PM -0700, Gilad Avidov wrote:
> > Qualcomm PMIC Arbiter version-2 changes from version-1 are:
> > 
> > - Some different register offsets.
> > - New channel register space, one per PMIC peripheral (ppid).
> >   All tx traffic uses these channels.
> > - New observer register space. All rx trafic uses this space.
> > - Different command format for spmi command registers.
> > 
> > Acked-by: Sagar Dharia <sdharia@codeaurora.org>
> > Signed-off-by: Gilad Avidov <gavidov@codeaurora.org>
> [..]
> > +++ b/drivers/spmi/spmi-pmic-arb.c
> [..]
> > @@ -645,12 +795,65 @@ static int spmi_pmic_arb_probe(struct
> > platform_device *pdev) pa->spmic = ctrl;
> >  
> >  	res = platform_get_resource_byname(pdev, IORESOURCE_MEM,
> > "core");
> > -	pa->base = devm_ioremap_resource(&ctrl->dev, res);
> > -	if (IS_ERR(pa->base)) {
> > -		err = PTR_ERR(pa->base);
> > +	pa->rd_base = devm_ioremap_resource(&ctrl->dev, res);
> 
> This seems like an awkward way to do this, especially if you end up
> remapping it...

Agree. I think the following will fix it:
1. Assign the core reg base to a local variable.
2. For v1: assign that local to rd/wr base.
3. For v2: devm_unmap() that core.

> 
> > +	if (IS_ERR(pa->rd_base)) {
> > +		err = PTR_ERR(pa->rd_base);
> >  		goto err_put_ctrl;
> >  	}
> >  
> > +	hw_ver = readl_relaxed(pa->rd_base + PMIC_ARB_VERSION);
> > +	is_v1  = (hw_ver < PMIC_ARB_VERSION_V2_MIN);
> > +
> > +	dev_info(&ctrl->dev, "PMIC Arb Version-%d (0x%x)\n",
> > (is_v1 ? 1 : 2),
> > +		hw_ver);
> > +
> > +	if (is_v1) {
> > +		pa->ver_ops = &pmic_arb_v1;
> > +		pa->wr_base = pa->rd_base;
> > +	} else {
> > +		u8  chan;
> > +		u16 ppid;
> > +		u32 regval;
> > +
> > +		pa->ver_ops = &pmic_arb_v2;
> > +
> > +		pa->ppid_to_chan = devm_kzalloc(&ctrl->dev,
> > +					PPID_TO_CHAN_TABLE_SZ,
> > GFP_KERNEL);
> > +		if (!pa->ppid_to_chan) {
> > +			err = -ENOMEM;
> > +			goto err_put_ctrl;
> > +		}
> > +		/*
> > +		 * PMIC_ARB_REG_CHNL is a table in HW mapping
> > channel to ppid.
> > +		 * ppid_to_chan is an in-memory invert of that
> > table.
> > +		 */
> > +		for (chan = 0; chan < PMIC_ARB_MAX_CHNL; ++chan) {
> > +			regval = readl_relaxed(pa->rd_base +
> > +
> > PMIC_ARB_REG_CHNL(chan));
> > +			if (!regval)
> > +				continue;
> > +
> > +			ppid = (regval >> 8) & 0xFFF;
> > +			pa->ppid_to_chan[ppid] = chan;
> > +		}
> > +
> > +		res = platform_get_resource_byname(pdev,
> > IORESOURCE_MEM,
> > +						   "obsrvr");
> > +		pa->rd_base = devm_ioremap_resource(&ctrl->dev,
> > res);
> 
> ...here.  Especially because now you have some loose mapping hanging
> around for the lifetime of the device.  I'd suggest splitting the v1
> and v2 probe routines out into their own functions.
> 

75% of the probe code is common. By splitting to two probes that code
will be duplicated. Of course we can factor out the commonality, but
then will end up with a similar situation as here.

Splitting the probes will not rid us of needing the core reg space for
either version, since we need the core to access the version register.

>   Josh
> --
> To unsubscribe from this list: send the line "unsubscribe
> linux-arm-msm" in the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Thank you,
Gilad
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/