Re: [PATCH V3 2/2] spmi: pmic_arb: add support for hw version 2

[Josh Cartwright <joshc@eso.teric.us>]

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...

> +	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.

  Josh