Rather than setting up the core, obsrv and chnls in probe by using
version specific conditionals, add a dedicated "get_core_resources"
version specific op and move the acquiring in there.
Signed-off-by: Abel Vesa <[email protected]>
---
drivers/spmi/spmi-pmic-arb.c | 113 +++++++++++++++++++++++++++----------------
1 file changed, 70 insertions(+), 43 deletions(-)
diff --git a/drivers/spmi/spmi-pmic-arb.c b/drivers/spmi/spmi-pmic-arb.c
index 38fed8a585fe..188252bfb95f 100644
--- a/drivers/spmi/spmi-pmic-arb.c
+++ b/drivers/spmi/spmi-pmic-arb.c
@@ -203,6 +203,7 @@ struct spmi_pmic_arb {
*/
struct pmic_arb_ver_ops {
const char *ver_str;
+ int (*get_core_resources)(struct platform_device *pdev, void __iomem *core);
int (*init_apid)(struct spmi_pmic_arb *pmic_arb, int index);
int (*ppid_to_apid)(struct spmi_pmic_arb *pmic_arb, u16 ppid);
/* spmi commands (read_cmd, write_cmd, cmd) functionality */
@@ -956,6 +957,19 @@ static int pmic_arb_init_apid_min_max(struct spmi_pmic_arb *pmic_arb)
return 0;
}
+static int pmic_arb_get_core_resources_v1(struct platform_device *pdev,
+ void __iomem *core)
+{
+ struct spmi_pmic_arb *pmic_arb = platform_get_drvdata(pdev);
+
+ pmic_arb->wr_base = core;
+ pmic_arb->rd_base = core;
+
+ pmic_arb->max_periphs = PMIC_ARB_MAX_PERIPHS;
+
+ return 0;
+}
+
static int pmic_arb_init_apid_v1(struct spmi_pmic_arb *pmic_arb, int index)
{
u32 *mapping_table;
@@ -1063,6 +1077,33 @@ static u16 pmic_arb_find_apid(struct spmi_pmic_arb *pmic_arb, u16 ppid)
return apid;
}
+static int pmic_arb_get_obsrvr_chnls_v2(struct platform_device *pdev)
+{
+ struct spmi_pmic_arb *pmic_arb = platform_get_drvdata(pdev);
+
+ pmic_arb->rd_base = devm_platform_ioremap_resource_byname(pdev, "obsrvr");
+ if (IS_ERR(pmic_arb->rd_base))
+ return PTR_ERR(pmic_arb->rd_base);
+
+ pmic_arb->wr_base = devm_platform_ioremap_resource_byname(pdev, "chnls");
+ if (IS_ERR(pmic_arb->wr_base))
+ return PTR_ERR(pmic_arb->wr_base);
+
+ return 0;
+}
+
+static int pmic_arb_get_core_resources_v2(struct platform_device *pdev,
+ void __iomem *core)
+{
+ struct spmi_pmic_arb *pmic_arb = platform_get_drvdata(pdev);
+
+ pmic_arb->core = core;
+
+ pmic_arb->max_periphs = PMIC_ARB_MAX_PERIPHS;
+
+ return pmic_arb_get_obsrvr_chnls_v2(pdev);
+}
+
static int pmic_arb_ppid_to_apid_v2(struct spmi_pmic_arb *pmic_arb, u16 ppid)
{
u16 apid_valid;
@@ -1246,6 +1287,18 @@ static int pmic_arb_offset_v5(struct spmi_pmic_arb *pmic_arb, u8 sid, u16 addr,
return offset;
}
+static int pmic_arb_get_core_resources_v7(struct platform_device *pdev,
+ void __iomem *core)
+{
+ struct spmi_pmic_arb *pmic_arb = platform_get_drvdata(pdev);
+
+ pmic_arb->core = core;
+
+ pmic_arb->max_periphs = PMIC_ARB_MAX_PERIPHS_V7;
+
+ return pmic_arb_get_obsrvr_chnls_v2(pdev);
+}
+
/*
* Only v7 supports 2 buses. Each bus will get a different apid count, read
* from different registers.
@@ -1469,6 +1522,7 @@ pmic_arb_apid_owner_v7(struct spmi_pmic_arb *pmic_arb, u16 n)
static const struct pmic_arb_ver_ops pmic_arb_v1 = {
.ver_str = "v1",
+ .get_core_resources = pmic_arb_get_core_resources_v1,
.init_apid = pmic_arb_init_apid_v1,
.ppid_to_apid = pmic_arb_ppid_to_apid_v1,
.non_data_cmd = pmic_arb_non_data_cmd_v1,
@@ -1484,6 +1538,7 @@ static const struct pmic_arb_ver_ops pmic_arb_v1 = {
static const struct pmic_arb_ver_ops pmic_arb_v2 = {
.ver_str = "v2",
+ .get_core_resources = pmic_arb_get_core_resources_v2,
.init_apid = pmic_arb_init_apid_v1,
.ppid_to_apid = pmic_arb_ppid_to_apid_v2,
.non_data_cmd = pmic_arb_non_data_cmd_v2,
@@ -1499,6 +1554,7 @@ static const struct pmic_arb_ver_ops pmic_arb_v2 = {
static const struct pmic_arb_ver_ops pmic_arb_v3 = {
.ver_str = "v3",
+ .get_core_resources = pmic_arb_get_core_resources_v2,
.init_apid = pmic_arb_init_apid_v1,
.ppid_to_apid = pmic_arb_ppid_to_apid_v2,
.non_data_cmd = pmic_arb_non_data_cmd_v2,
@@ -1514,6 +1570,7 @@ static const struct pmic_arb_ver_ops pmic_arb_v3 = {
static const struct pmic_arb_ver_ops pmic_arb_v5 = {
.ver_str = "v5",
+ .get_core_resources = pmic_arb_get_core_resources_v2,
.init_apid = pmic_arb_init_apid_v5,
.ppid_to_apid = pmic_arb_ppid_to_apid_v5,
.non_data_cmd = pmic_arb_non_data_cmd_v2,
@@ -1529,6 +1586,7 @@ static const struct pmic_arb_ver_ops pmic_arb_v5 = {
static const struct pmic_arb_ver_ops pmic_arb_v7 = {
.ver_str = "v7",
+ .get_core_resources = pmic_arb_get_core_resources_v7,
.init_apid = pmic_arb_init_apid_v7,
.ppid_to_apid = pmic_arb_ppid_to_apid_v5,
.non_data_cmd = pmic_arb_non_data_cmd_v2,
@@ -1565,16 +1623,6 @@ static int spmi_pmic_arb_probe(struct platform_device *pdev)
pmic_arb = spmi_controller_get_drvdata(ctrl);
pmic_arb->spmic = ctrl;
- /*
- * Please don't replace this with devm_platform_ioremap_resource() or
- * devm_ioremap_resource(). These both result in a call to
- * devm_request_mem_region() which prevents multiple mappings of this
- * register address range. SoCs with PMIC arbiter v7 may define two
- * arbiter devices, for the two physical SPMI interfaces, which share
- * some register address ranges (i.e. "core", "obsrvr", and "chnls").
- * Ensure that both devices probe successfully by calling devm_ioremap()
- * which does not result in a devm_request_mem_region() call.
- */
res = platform_get_resource_byname(pdev, IORESOURCE_MEM, "core");
core = devm_ioremap(&ctrl->dev, res->start, resource_size(res));
if (IS_ERR(core))
@@ -1584,44 +1632,23 @@ static int spmi_pmic_arb_probe(struct platform_device *pdev)
hw_ver = readl_relaxed(core + PMIC_ARB_VERSION);
- if (hw_ver < PMIC_ARB_VERSION_V2_MIN) {
+ if (hw_ver < PMIC_ARB_VERSION_V2_MIN)
pmic_arb->ver_ops = &pmic_arb_v1;
- pmic_arb->wr_base = core;
- pmic_arb->rd_base = core;
- } else {
- pmic_arb->core = core;
-
- if (hw_ver < PMIC_ARB_VERSION_V3_MIN)
- pmic_arb->ver_ops = &pmic_arb_v2;
- else if (hw_ver < PMIC_ARB_VERSION_V5_MIN)
- pmic_arb->ver_ops = &pmic_arb_v3;
- else if (hw_ver < PMIC_ARB_VERSION_V7_MIN)
- pmic_arb->ver_ops = &pmic_arb_v5;
- else
- pmic_arb->ver_ops = &pmic_arb_v7;
-
- res = platform_get_resource_byname(pdev, IORESOURCE_MEM,
- "obsrvr");
- pmic_arb->rd_base = devm_ioremap(&ctrl->dev, res->start,
- resource_size(res));
- if (IS_ERR(pmic_arb->rd_base))
- return PTR_ERR(pmic_arb->rd_base);
-
- res = platform_get_resource_byname(pdev, IORESOURCE_MEM,
- "chnls");
- pmic_arb->wr_base = devm_ioremap(&ctrl->dev, res->start,
- resource_size(res));
- if (IS_ERR(pmic_arb->wr_base))
- return PTR_ERR(pmic_arb->wr_base);
- }
+ else if (hw_ver < PMIC_ARB_VERSION_V3_MIN)
+ pmic_arb->ver_ops = &pmic_arb_v2;
+ else if (hw_ver < PMIC_ARB_VERSION_V5_MIN)
+ pmic_arb->ver_ops = &pmic_arb_v3;
+ else if (hw_ver < PMIC_ARB_VERSION_V7_MIN)
+ pmic_arb->ver_ops = &pmic_arb_v5;
+ else
+ pmic_arb->ver_ops = &pmic_arb_v7;
dev_info(&ctrl->dev, "PMIC arbiter version %s (0x%x)\n",
pmic_arb->ver_ops->ver_str, hw_ver);
- if (hw_ver < PMIC_ARB_VERSION_V7_MIN)
- pmic_arb->max_periphs = PMIC_ARB_MAX_PERIPHS;
- else
- pmic_arb->max_periphs = PMIC_ARB_MAX_PERIPHS_V7;
+ err = pmic_arb->ver_ops->get_core_resources(pdev, core);
+ if (err)
+ return err;
err = pmic_arb->ver_ops->init_apid(pmic_arb, 0);
if (err)
--
2.34.1
On 26/03/2024 17:28, Abel Vesa wrote:
> Rather than setting up the core, obsrv and chnls in probe by using
> version specific conditionals, add a dedicated "get_core_resources"
> version specific op and move the acquiring in there.
>
> Signed-off-by: Abel Vesa <[email protected]>
> ---
> drivers/spmi/spmi-pmic-arb.c | 113 +++++++++++++++++++++++++++----------------
> 1 file changed, 70 insertions(+), 43 deletions(-)
>
> diff --git a/drivers/spmi/spmi-pmic-arb.c b/drivers/spmi/spmi-pmic-arb.c
> index 38fed8a585fe..188252bfb95f 100644
> --- a/drivers/spmi/spmi-pmic-arb.c
> +++ b/drivers/spmi/spmi-pmic-arb.c
> @@ -203,6 +203,7 @@ struct spmi_pmic_arb {
> */
> struct pmic_arb_ver_ops {
> const char *ver_str;
> + int (*get_core_resources)(struct platform_device *pdev, void __iomem *core);
> int (*init_apid)(struct spmi_pmic_arb *pmic_arb, int index);
> int (*ppid_to_apid)(struct spmi_pmic_arb *pmic_arb, u16 ppid);
> /* spmi commands (read_cmd, write_cmd, cmd) functionality */
> @@ -956,6 +957,19 @@ static int pmic_arb_init_apid_min_max(struct spmi_pmic_arb *pmic_arb)
> return 0;
> }
>
> +static int pmic_arb_get_core_resources_v1(struct platform_device *pdev,
> + void __iomem *core)
> +{
> + struct spmi_pmic_arb *pmic_arb = platform_get_drvdata(pdev);
> +
> + pmic_arb->wr_base = core;
> + pmic_arb->rd_base = core;
> +
> + pmic_arb->max_periphs = PMIC_ARB_MAX_PERIPHS;
> +
> + return 0;
> +}
> +
> static int pmic_arb_init_apid_v1(struct spmi_pmic_arb *pmic_arb, int index)
> {
> u32 *mapping_table;
> @@ -1063,6 +1077,33 @@ static u16 pmic_arb_find_apid(struct spmi_pmic_arb *pmic_arb, u16 ppid)
> return apid;
> }
>
> +static int pmic_arb_get_obsrvr_chnls_v2(struct platform_device *pdev)
> +{
> + struct spmi_pmic_arb *pmic_arb = platform_get_drvdata(pdev);
> +
> + pmic_arb->rd_base = devm_platform_ioremap_resource_byname(pdev, "obsrvr");
> + if (IS_ERR(pmic_arb->rd_base))
> + return PTR_ERR(pmic_arb->rd_base);
> +
> + pmic_arb->wr_base = devm_platform_ioremap_resource_byname(pdev, "chnls");
> + if (IS_ERR(pmic_arb->wr_base))
> + return PTR_ERR(pmic_arb->wr_base);
> +
> + return 0;
> +}
> +
> +static int pmic_arb_get_core_resources_v2(struct platform_device *pdev,
> + void __iomem *core)
> +{
> + struct spmi_pmic_arb *pmic_arb = platform_get_drvdata(pdev);
> +
> + pmic_arb->core = core;
> +
> + pmic_arb->max_periphs = PMIC_ARB_MAX_PERIPHS;
> +
> + return pmic_arb_get_obsrvr_chnls_v2(pdev);
> +}
> +
> static int pmic_arb_ppid_to_apid_v2(struct spmi_pmic_arb *pmic_arb, u16 ppid)
> {
> u16 apid_valid;
> @@ -1246,6 +1287,18 @@ static int pmic_arb_offset_v5(struct spmi_pmic_arb *pmic_arb, u8 sid, u16 addr,
> return offset;
> }
>
> +static int pmic_arb_get_core_resources_v7(struct platform_device *pdev,
> + void __iomem *core)
> +{
> + struct spmi_pmic_arb *pmic_arb = platform_get_drvdata(pdev);
> +
> + pmic_arb->core = core;
> +
> + pmic_arb->max_periphs = PMIC_ARB_MAX_PERIPHS_V7;
> +
> + return pmic_arb_get_obsrvr_chnls_v2(pdev);
> +}
> +
> /*
> * Only v7 supports 2 buses. Each bus will get a different apid count, read
> * from different registers.
> @@ -1469,6 +1522,7 @@ pmic_arb_apid_owner_v7(struct spmi_pmic_arb *pmic_arb, u16 n)
>
> static const struct pmic_arb_ver_ops pmic_arb_v1 = {
> .ver_str = "v1",
> + .get_core_resources = pmic_arb_get_core_resources_v1,
> .init_apid = pmic_arb_init_apid_v1,
> .ppid_to_apid = pmic_arb_ppid_to_apid_v1,
> .non_data_cmd = pmic_arb_non_data_cmd_v1,
> @@ -1484,6 +1538,7 @@ static const struct pmic_arb_ver_ops pmic_arb_v1 = {
>
> static const struct pmic_arb_ver_ops pmic_arb_v2 = {
> .ver_str = "v2",
> + .get_core_resources = pmic_arb_get_core_resources_v2,
> .init_apid = pmic_arb_init_apid_v1,
> .ppid_to_apid = pmic_arb_ppid_to_apid_v2,
> .non_data_cmd = pmic_arb_non_data_cmd_v2,
> @@ -1499,6 +1554,7 @@ static const struct pmic_arb_ver_ops pmic_arb_v2 = {
>
> static const struct pmic_arb_ver_ops pmic_arb_v3 = {
> .ver_str = "v3",
> + .get_core_resources = pmic_arb_get_core_resources_v2,
> .init_apid = pmic_arb_init_apid_v1,
> .ppid_to_apid = pmic_arb_ppid_to_apid_v2,
> .non_data_cmd = pmic_arb_non_data_cmd_v2,
> @@ -1514,6 +1570,7 @@ static const struct pmic_arb_ver_ops pmic_arb_v3 = {
>
> static const struct pmic_arb_ver_ops pmic_arb_v5 = {
> .ver_str = "v5",
> + .get_core_resources = pmic_arb_get_core_resources_v2,
> .init_apid = pmic_arb_init_apid_v5,
> .ppid_to_apid = pmic_arb_ppid_to_apid_v5,
> .non_data_cmd = pmic_arb_non_data_cmd_v2,
> @@ -1529,6 +1586,7 @@ static const struct pmic_arb_ver_ops pmic_arb_v5 = {
>
> static const struct pmic_arb_ver_ops pmic_arb_v7 = {
> .ver_str = "v7",
> + .get_core_resources = pmic_arb_get_core_resources_v7,
> .init_apid = pmic_arb_init_apid_v7,
> .ppid_to_apid = pmic_arb_ppid_to_apid_v5,
> .non_data_cmd = pmic_arb_non_data_cmd_v2,
> @@ -1565,16 +1623,6 @@ static int spmi_pmic_arb_probe(struct platform_device *pdev)
> pmic_arb = spmi_controller_get_drvdata(ctrl);
> pmic_arb->spmic = ctrl;
>
> - /*
> - * Please don't replace this with devm_platform_ioremap_resource() or
> - * devm_ioremap_resource(). These both result in a call to
> - * devm_request_mem_region() which prevents multiple mappings of this
> - * register address range. SoCs with PMIC arbiter v7 may define two
> - * arbiter devices, for the two physical SPMI interfaces, which share
> - * some register address ranges (i.e. "core", "obsrvr", and "chnls").
> - * Ensure that both devices probe successfully by calling devm_ioremap()
> - * which does not result in a devm_request_mem_region() call.
> - */
Can you explain in the commit message why you remove this comment ?
> res = platform_get_resource_byname(pdev, IORESOURCE_MEM, "core");
> core = devm_ioremap(&ctrl->dev, res->start, resource_size(res));
> if (IS_ERR(core))
> @@ -1584,44 +1632,23 @@ static int spmi_pmic_arb_probe(struct platform_device *pdev)
>
> hw_ver = readl_relaxed(core + PMIC_ARB_VERSION);
>
> - if (hw_ver < PMIC_ARB_VERSION_V2_MIN) {
> + if (hw_ver < PMIC_ARB_VERSION_V2_MIN)
> pmic_arb->ver_ops = &pmic_arb_v1;
> - pmic_arb->wr_base = core;
> - pmic_arb->rd_base = core;
> - } else {
> - pmic_arb->core = core;
> -
> - if (hw_ver < PMIC_ARB_VERSION_V3_MIN)
> - pmic_arb->ver_ops = &pmic_arb_v2;
> - else if (hw_ver < PMIC_ARB_VERSION_V5_MIN)
> - pmic_arb->ver_ops = &pmic_arb_v3;
> - else if (hw_ver < PMIC_ARB_VERSION_V7_MIN)
> - pmic_arb->ver_ops = &pmic_arb_v5;
> - else
> - pmic_arb->ver_ops = &pmic_arb_v7;
> -
> - res = platform_get_resource_byname(pdev, IORESOURCE_MEM,
> - "obsrvr");
> - pmic_arb->rd_base = devm_ioremap(&ctrl->dev, res->start,
> - resource_size(res));
> - if (IS_ERR(pmic_arb->rd_base))
> - return PTR_ERR(pmic_arb->rd_base);
> -
> - res = platform_get_resource_byname(pdev, IORESOURCE_MEM,
> - "chnls");
> - pmic_arb->wr_base = devm_ioremap(&ctrl->dev, res->start,
> - resource_size(res));
> - if (IS_ERR(pmic_arb->wr_base))
> - return PTR_ERR(pmic_arb->wr_base);
> - }
> + else if (hw_ver < PMIC_ARB_VERSION_V3_MIN)
> + pmic_arb->ver_ops = &pmic_arb_v2;
> + else if (hw_ver < PMIC_ARB_VERSION_V5_MIN)
> + pmic_arb->ver_ops = &pmic_arb_v3;
> + else if (hw_ver < PMIC_ARB_VERSION_V7_MIN)
> + pmic_arb->ver_ops = &pmic_arb_v5;
> + else
> + pmic_arb->ver_ops = &pmic_arb_v7;
>
> dev_info(&ctrl->dev, "PMIC arbiter version %s (0x%x)\n",
> pmic_arb->ver_ops->ver_str, hw_ver);
>
> - if (hw_ver < PMIC_ARB_VERSION_V7_MIN)
> - pmic_arb->max_periphs = PMIC_ARB_MAX_PERIPHS;
> - else
> - pmic_arb->max_periphs = PMIC_ARB_MAX_PERIPHS_V7;
> + err = pmic_arb->ver_ops->get_core_resources(pdev, core);
> + if (err)
> + return err;
>
> err = pmic_arb->ver_ops->init_apid(pmic_arb, 0);
> if (err)
>
With that added:
Reviewed-by: Neil Armstrong <[email protected]>