On Mon, 16 Jan 2023 at 06:03, Alexey Minnekhanov <alexey...@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. > > [1]: > https://elixir.bootlin.com/linux/v6.1.6/source/drivers/spmi/spmi-pmic-arb.c#L1339 > > Signed-off-by: Alexey Minnekhanov <alexey...@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.g...@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 >