Re: [PATCH 2/5] spmi: msm: Fix parsing FDT and reading ARB version

On Mon, 16 Jan 2023 at 06:03, Alexey Minnekhanov alexeymin@postmarketos.org wrote:

First of all, use dev_read_addr_name() instead of dev_read_addr_index() to avoid confusion: most dts files have their regs specified in the wrong order, so driver is reading config reg and using it instead of core reg. Using names instead of indexes helps to avoid such errors.

Second, same as Linux driver, use core reg to read version from [1]. This fixes reading incorrect arbiter version.

Third, print addresses in hex, so it can be visually compared to values in DTS more easily.

Signed-off-by: Alexey Minnekhanov alexeymin@postmarketos.org

drivers/spmi/spmi-msm.c | 16 +++++++++------- 1 file changed, 9 insertions(+), 7 deletions(-)

Apart from nit below, feel free to add:

Reviewed-by: Sumit Garg sumit.garg@linaro.org

diff --git a/drivers/spmi/spmi-msm.c b/drivers/spmi/spmi-msm.c index a9dcf5ab7f91..3df0f12c8b86 100644 --- a/drivers/spmi/spmi-msm.c +++ b/drivers/spmi/spmi-msm.c @@ -191,11 +191,12 @@ static int msm_spmi_probe(struct udevice *dev) u32 version; int i;

•   config_addr = dev_read_addr_index(dev, 0);
  

•   priv->spmi_core = dev_read_addr_index(dev, 1);
  

•   priv->spmi_obs = dev_read_addr_index(dev, 2);
  

•   /* DTS bindings: reg-names = "cnfg", "core", "obsrvr"; */
  

nit: this comment is redundant as there is already DT bindings documentation to look at.

-Sumit

•   config_addr = dev_read_addr_name(dev, "cnfg");
  

•   priv->spmi_core = dev_read_addr_name(dev, "core");
  

•   priv->spmi_obs = dev_read_addr_name(dev, "obsrvr");
  
  

•   hw_ver = readl(config_addr + PMIC_ARB_VERSION);
  

•   hw_ver = readl(priv->spmi_core + PMIC_ARB_VERSION);
  
    if (hw_ver < PMIC_ARB_VERSION_V3_MIN) {
            priv->arb_ver = V2;
  

@@ -218,9 +219,10 @@ static int msm_spmi_probe(struct udevice *dev) priv->spmi_obs == FDT_ADDR_T_NONE) return -EINVAL;

•   dev_dbg(dev, "priv->arb_chnl address (%llu)\n", priv->arb_chnl);
  

•   dev_dbg(dev, "priv->spmi_core address (%llu)\n", priv->spmi_core);
  

•   dev_dbg(dev, "priv->spmi_obs address (%llu)\n", priv->spmi_obs);
  

•   dev_dbg(dev, "priv->arb_chnl address (%llx)\n", priv->arb_chnl);
  

•   dev_dbg(dev, "priv->spmi_core address (%llx)\n", priv->spmi_core);
  

•   dev_dbg(dev, "priv->spmi_obs address (%llx)\n", priv->spmi_obs);
  

•   /* Scan peripherals connected to each SPMI channel */
    for (i = 0; i < SPMI_MAX_PERIPH; i++) {
            uint32_t periph = readl(priv->arb_chnl + ARB_CHANNEL_OFFSET(i));
  

-- 2.38.2