| Message ID | 20230116003315.2325575-3-alexeymin@postmarketos.org |
|---|---|
| State | Superseded |
| Delegated to: | Tom Rini |
| Headers | show |
| Series | spmi:msm: Several fixes | expand |
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. > > [1]: https://elixir.bootlin.com/linux/v6.1.6/source/drivers/spmi/spmi-pmic-arb.c#L1339 > > 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 >
On 2023-01-19 11:36, Sumit Garg wrote: > On Mon, 16 Jan 2023 at 06:03, Alexey Minnekhanov > <alexeymin@postmarketos.org> wrote: ... >> - 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 > Thanks, will remove it in v2. -- Alexey Minnekhanov
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"; */ + 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));
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 <alexeymin@postmarketos.org> --- drivers/spmi/spmi-msm.c | 16 +++++++++------- 1 file changed, 9 insertions(+), 7 deletions(-)