: support for spmi_pmic_arb v3/v5 and bug fixes

From: Abhijeet Dharmapurikar <[email protected]>

The system crashes due to bad access when reading from an non configured peripheral and when writing to peripheral which is not owned by current ee. This patch verifies ownership to avoid crashing on write.

For reads, since the forward mapping table, data_channel->ppid, is towards the end of the block, we use the core size to figure the max number of ppids supported. The table starts at an offset of 0x800 within the block, so size - 0x800 will give us the area used by the table. Since each table is 4 bytes long (core_size - 0x800) / 4 will gives us the number of data_channel supported.
This new protection is functional on hw v2.

Signed-off-by Abhijeet Dharmapurikar <[email protected]>

Signed-off-by Kiran Gunda <[email protected]>

---
 drivers/spmi/spmi-pmic-arb.c | 84 +++++++++++++++++++++++++++++++++++++++++++-
 1 file changed, 83 insertions(+), 1 deletion(-)

drivers/spmi/spmi-pmic-arb.c +0 -0

index 5ec3a59..df463d4 100644

drivers/spmi/spmi-pmic-arb.c +1 -0

  * @mapping_table:	in-memory copy of PPID -> APID mapping table.
  * @domain:		irq domain object for PMIC IRQ domain
  * @spmic:		SPMI controller object

@@ -132,6 +133,7 @@ struct spmi_pmic_arb_dev {
 	u8			ee;
 	u16			min_apid;
 	u16			max_apid;
+	u16			max_periph;
 	u32			*mapping_table;
 	DECLARE_BITMAP(mapping_table_valid, PMIC_ARB_MAX_PERIPHS);
 	struct irq_domain	*domain;

drivers/spmi/spmi-pmic-arb.c +4 -0

 };
 
 /**
  * pmic_arb_ver: version dependent functionality.
  *
+ * @mode:	access rights to specified pmic peripheral.
  * @non_data_cmd:	on v1 issues an spmi non-data command.
  *			on v2 no HW support, returns -EOPNOTSUPP.
  * @offset:		on v1 offset of per-ee channel.

@@ -160,6 +164,8 @@ struct spmi_pmic_arb_dev {
  *			on v2 offset of SPMI_PIC_IRQ_CLEARn.
  */
 struct pmic_arb_ver_ops {
+	int (*mode)(struct spmi_pmic_arb_dev *dev, u8 sid, u16 addr,
+			mode_t *mode);
 	/* spmi commands (read_cmd, write_cmd, cmd) functionality */
 	int (*offset)(struct spmi_pmic_arb_dev *dev, u8 sid, u16 addr,
 		      u32 *offset);

@@ -313,11 +319,23 @@ static int pmic_arb_read_cmd(struct spmi_controller *ctrl, u8 opc, u8 sid,
 	u32 cmd;
 	int rc;
 	u32 offset;
+	mode_t mode;

drivers/spmi/spmi-pmic-arb.c +6 -0

+			"error: impermissible read from peripheral sid:%d addr:0x%x\n",
+			sid, addr);
+		return -EPERM;
+	}
+
 	if (bc >= PMIC_ARB_MAX_TRANS_BYTES) {
 		dev_err(&ctrl->dev,
 			"pmic-arb supports 1..%d bytes per trans, but:%zu requested",

@@ -364,11 +382,23 @@ static int pmic_arb_write_cmd(struct spmi_controller *ctrl, u8 opc, u8 sid,
 	u32 cmd;
 	int rc;
 	u32 offset;
+	mode_t mode;
 

drivers/spmi/spmi-pmic-arb.c +5 -0

+			"error: impermissible write to peripheral sid:%d addr:0x%x\n",
+			sid, addr);
+		return -EPERM;
+	}
+
 	if (bc >= PMIC_ARB_MAX_TRANS_BYTES) {
 		dev_err(&ctrl->dev,
 			"pmic-arb supports 1..%d bytes per trans, but:%zu requested",

drivers/spmi/spmi-pmic-arb.c +2 -0

+}
+
 /* v1 offset per ee */
 static int
 pmic_arb_offset_v1(struct spmi_pmic_arb_dev *pa, u8 sid, u16 addr, u32 *offset)

@@ -745,7 +782,11 @@ static u16 pmic_arb_find_chan(struct spmi_pmic_arb_dev *pa, u16 ppid)
 	 * PMIC_ARB_REG_CHNL is a table in HW mapping channel to ppid.
 	 * ppid_to_chan is an in-memory invert of that table.

drivers/spmi/spmi-pmic-arb.c +19 -0

+{
+	u16 ppid = (sid << 8) | (addr >> 8);
+	u16 chan;
+	u8 owner;
+
+	chan = pa->ppid_to_chan[ppid];
+	if (!(chan & PMIC_ARB_CHAN_VALID))
+		return -ENODEV;
+
+	*mode = 0;
+	*mode |= S_IRUSR;
+
+	chan &= ~PMIC_ARB_CHAN_VALID;
+	owner = pa->chan_to_owner[chan];
+	if (owner == pa->ee)
+		*mode |= S_IWUSR;
+	return 0;
+}
+
 /* v2 offset per ppid (chan) and per ee */
 static int
 pmic_arb_offset_v2(struct spmi_pmic_arb_dev *pa, u8 sid, u16 addr, u32 *offset)

drivers/spmi/spmi-pmic-arb.c +3 -0

 	core = devm_ioremap_resource(&ctrl->dev, res);
 	if (IS_ERR(core)) {
 		err = PTR_ERR(core);

@@ -899,6 +969,9 @@ static int spmi_pmic_arb_probe(struct platform_device *pdev)
 		pa->core = core;
 		pa->ver_ops = &pmic_arb_v2;
 
+		/* the apid to ppid table starts at PMIC_ARB_REG_CHNL(0) */
+		pa->max_periph =  (pa->core_size - PMIC_ARB_REG_CHNL(0)) / 4;
+
 		res = platform_get_resource_byname(pdev, IORESOURCE_MEM,
 						   "obsrvr");
 		pa->rd_base = devm_ioremap_resource(&ctrl->dev, res);

res = platform_get_resource_byname(pdev, IORESOURCE_MEM, "intr");

Signature · 2 lines

QUALCOMM INDIA, on behalf of Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, hosted by The Linux Foundation
--

Quoted 6 lines · Kiran Gunda

From: Abhijeet Dharmapurikar <[email protected]>

The system crashes due to bad access when reading from an non configured peripheral and when writing to peripheral which is not owned by current ee. This patch verifies ownership to avoid crashing on write.

What systems? As far as I know we don't have any bad accesses happening right now. If they are happening, we should fix the code that's accessing hardware that isn't owned by them.

Quoted 7 lines

For reads, since the forward mapping table, data_channel->ppid, is towards the end of the block, we use the core size to figure the max number of ppids supported. The table starts at an offset of 0x800 within the block, so size - 0x800 will give us the area used by the table. Since each table is 4 bytes long (core_size - 0x800) / 4 will gives us the number of data_channel supported.
This new protection is functional on hw v2.

Which brings us to the next question which is why do we need this patch at all? We aren't probing hardware to see what we have access to and then populating device structures based on that. Instead, we're just populating DT nodes that we've hardcoded in the dts files, so I'm a little lost on why we would have a node in there that we couldn't access. Please add such details to the commit text.

Quoted 8 lines

Signed-off-by Abhijeet Dharmapurikar <[email protected]>

Signed-off-by Kiran Gunda <[email protected]>

---
 drivers/spmi/spmi-pmic-arb.c | 84 
+++++++++++++++++++++++++++++++++++++++++++-
 1 file changed, 83 insertions(+), 1 deletion(-)

drivers/spmi/spmi-pmic-arb.c enum pmic_arb_cmd_op_code { · line 114

Quoted 9 lines

drivers/spmi/spmi-pmic-arb.c +1 -0

index 5ec3a59..df463d4 100644
@@ -111,6 +111,7 @@ enum pmic_arb_cmd_op_code {
  * @ee:			the current Execution Environment
  * @min_apid:		minimum APID (used for bounding IRQ search)
  * @max_apid:		maximum APID
+ * @max_periph:		maximum number of PMIC peripherals supported by HW.

Nitpick: Most of these lines don't end with a full-stop.

Quoted 10 lines

drivers/spmi/spmi-pmic-arb.c +1 -0

  * @mapping_table:	in-memory copy of PPID -> APID mapping table.
  * @domain:		irq domain object for PMIC IRQ domain
  * @spmic:		SPMI controller object

@@ -132,6 +133,7 @@ struct spmi_pmic_arb_dev {
 	u8			ee;
 	u16			min_apid;
 	u16			max_apid;
+	u16			max_periph;
 	u32			*mapping_table;
 	DECLARE_BITMAP(mapping_table_valid, PMIC_ARB_MAX_PERIPHS);

drivers/spmi/spmi-pmic-arb.c struct spmi_pmic_arb_dev { · line 145

Quoted 6 lines

drivers/spmi/spmi-pmic-arb.c +1 -0

 	struct irq_domain	*domain;

@@ -140,11 +142,13 @@ struct spmi_pmic_arb_dev {
 	const struct pmic_arb_ver_ops *ver_ops;
 	u16			*ppid_to_chan;
 	u16			last_channel;
+	u8			*chan_to_owner;

And we didn't document this one.

Quoted 29 lines

drivers/spmi/spmi-pmic-arb.c +4 -0

 };
 
 /**
  * pmic_arb_ver: version dependent functionality.
  *
+ * @mode:	access rights to specified pmic peripheral.
  * @non_data_cmd:	on v1 issues an spmi non-data command.
  *			on v2 no HW support, returns -EOPNOTSUPP.
  * @offset:		on v1 offset of per-ee channel.

@@ -160,6 +164,8 @@ struct spmi_pmic_arb_dev {
  *			on v2 offset of SPMI_PIC_IRQ_CLEARn.
  */
 struct pmic_arb_ver_ops {
+	int (*mode)(struct spmi_pmic_arb_dev *dev, u8 sid, u16 addr,
+			mode_t *mode);
 	/* spmi commands (read_cmd, write_cmd, cmd) functionality */
 	int (*offset)(struct spmi_pmic_arb_dev *dev, u8 sid, u16 addr,
 		      u32 *offset);

@@ -313,11 +319,23 @@ static int pmic_arb_read_cmd(struct 
 spmi_controller *ctrl, u8 opc, u8 sid,
 	u32 cmd;
 	int rc;
 	u32 offset;
+	mode_t mode;
 
 	rc = pmic_arb->ver_ops->offset(pmic_arb, sid, addr, &offset);
 	if (rc)
 		return rc;
 

drivers/spmi/spmi-pmic-arb.c static int pmic_arb_read_cmd(struct spmi_controller *ctrl, u8 opc, u8 sid, · line 332

Quoted 5 lines

drivers/spmi/spmi-pmic-arb.c +5 -0

+	rc = pmic_arb->ver_ops->mode(pmic_arb, sid, addr, &mode);
+	if (rc)
+		return rc;
+
+	if (!(mode & S_IRUSR)) {

Using mode_t for hardware access is odd. Perhaps just come up with some sort of READ/WRITE enum instead (if this sort of checking is even needed)?

drivers/spmi/spmi-pmic-arb.c static int pmic_arb_read_cmd(struct · line 334

Quoted 1 line

drivers/spmi/spmi-pmic-arb.c +1 -0

+		dev_err(&pmic_arb->spmic->dev,

The dev_err() just after uses ctrl->dev? Why not here?

Quoted 19 lines

drivers/spmi/spmi-pmic-arb.c +5 -0

+			"error: impermissible read from peripheral sid:%d addr:0x%x\n",
+			sid, addr);
+		return -EPERM;
+	}
+
 	if (bc >= PMIC_ARB_MAX_TRANS_BYTES) {
 		dev_err(&ctrl->dev,

drivers/spmi/spmi-pmic-arb.c +1 -0

 			"pmic-arb supports 1..%d bytes per trans, but:%zu requested",

@@ -364,11 +382,23 @@ static int pmic_arb_write_cmd(struct 
 spmi_controller *ctrl, u8 opc, u8 sid,
 	u32 cmd;
 	int rc;
 	u32 offset;
+	mode_t mode;
 
 	rc = pmic_arb->ver_ops->offset(pmic_arb, sid, addr, &offset);
 	if (rc)
 		return rc;
 

drivers/spmi/spmi-pmic-arb.c static int pmic_arb_write_cmd(struct spmi_controller *ctrl, u8 opc, u8 sid, · line 396

Quoted 6 lines

drivers/spmi/spmi-pmic-arb.c +6 -0

+	rc = pmic_arb->ver_ops->mode(pmic_arb, sid, addr, &mode);
+	if (rc)
+		return rc;
+
+	if (!(mode & S_IWUSR)) {
+		dev_err(&pmic_arb->spmic->dev,

The dev_err() just after uses ctrl->dev? Why not here?

Quoted 13 lines

drivers/spmi/spmi-pmic-arb.c +5 -0

+			"error: impermissible write to peripheral sid:%d addr:0x%x\n",
+			sid, addr);
+		return -EPERM;
+	}
+
 	if (bc >= PMIC_ARB_MAX_TRANS_BYTES) {
 		dev_err(&ctrl->dev,

drivers/spmi/spmi-pmic-arb.c +0 -0

 			"pmic-arb supports 1..%d bytes per trans, but:%zu requested",

@@ -727,6 +757,13 @@ static int qpnpint_irq_domain_map(struct 
 irq_domain *d,
 	return 0;
 }
 

drivers/spmi/spmi-pmic-arb.c static int qpnpint_irq_domain_map(struct irq_domain *d, · line 764

Quoted 6 lines

drivers/spmi/spmi-pmic-arb.c +5 -0

+static int
+pmic_arb_mode_v1(struct spmi_pmic_arb_dev *pa, u8 sid, u16 addr, 
 mode_t *mode)
+{
+	*mode = S_IRUSR | S_IWUSR;
+	return 0;

If mode was only positive then errors could be negative and no access could be 0. Then we could just return the mode from the function instead of passing a pointer around.

Quoted 14 lines

drivers/spmi/spmi-pmic-arb.c +2 -0

+}
+
 /* v1 offset per ee */

static int
pmic_arb_offset_v1(struct spmi_pmic_arb_dev *pa, u8 sid, u16 addr,
u32 *offset)

drivers/spmi/spmi-pmic-arb.c +2 -1

@@ -745,7 +782,11 @@ static u16 pmic_arb_find_chan(struct 
 spmi_pmic_arb_dev *pa, u16 ppid)
 	 * 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 = pa->last_channel; ; chan++) {
+	for (chan = pa->last_channel; chan < pa->max_periph; chan++) {
+		regval = readl_relaxed(pa->cnfg +

Quoted 6 lines

drivers/spmi/spmi-pmic-arb.c +3 -0

+				      SPMI_OWNERSHIP_TABLE_REG(chan));
+		pa->chan_to_owner[chan] = SPMI_OWNERSHIP_PERIPH2OWNER(regval);
+
 		offset = PMIC_ARB_REG_CHNL(chan);
 		if (offset >= pa->core_size)

break;

Seems like an unrelated change to the mapping logic?

drivers/spmi/spmi-pmic-arb.c static u16 pmic_arb_find_chan(struct · line 814

Quoted 8 lines

drivers/spmi/spmi-pmic-arb.c +2 -0

@@ -767,6 +808,27 @@ static u16 pmic_arb_find_chan(struct 
 spmi_pmic_arb_dev *pa, u16 ppid)
 }
 
 
+static int
+pmic_arb_mode_v2(struct spmi_pmic_arb_dev *pa, u8 sid, u16 addr, 
 mode_t *mode)

Probably spmi_pmic_arb_dev should be const here.

Quoted 26 lines

drivers/spmi/spmi-pmic-arb.c +19 -0

+{
+	u16 ppid = (sid << 8) | (addr >> 8);
+	u16 chan;
+	u8 owner;
+
+	chan = pa->ppid_to_chan[ppid];
+	if (!(chan & PMIC_ARB_CHAN_VALID))
+		return -ENODEV;
+
+	*mode = 0;
+	*mode |= S_IRUSR;
+
+	chan &= ~PMIC_ARB_CHAN_VALID;
+	owner = pa->chan_to_owner[chan];
+	if (owner == pa->ee)
+		*mode |= S_IWUSR;
+	return 0;
+}
+
 /* v2 offset per ppid (chan) and per ee */

static int
pmic_arb_offset_v2(struct spmi_pmic_arb_dev *pa, u8 sid, u16 addr,
u32 *offset)

drivers/spmi/spmi-pmic-arb.c +0 -0

@@ -879,6 +943,12 @@ static int spmi_pmic_arb_probe(struct 
 platform_device *pdev)
 

drivers/spmi/spmi-pmic-arb.c static int spmi_pmic_arb_probe(struct platform_device *pdev)

Quoted 5 lines

drivers/spmi/spmi-pmic-arb.c +2 -0

 	res = platform_get_resource_byname(pdev, IORESOURCE_MEM, "core");
 	pa->core_size = resource_size(res);
+	if (pa->core_size <= 0x800) {
+		dev_err(&pdev->dev, "core_size is smaller than 0x800. Failing 
 Probe\n");

Not sure why probe is capitalized.

drivers/spmi/spmi-pmic-arb.c static int spmi_pmic_arb_probe(struct platform_device *pdev) · line 951

Quoted 4 lines

drivers/spmi/spmi-pmic-arb.c +4 -0

+		err = -EINVAL;
+		goto err_put_ctrl;
+	}
+

We don't need these sorts of DT validation checks. Please remove.

Quoted 15 lines

drivers/spmi/spmi-pmic-arb.c +0 -0

 	core = devm_ioremap_resource(&ctrl->dev, res);

drivers/spmi/spmi-pmic-arb.c +3 -0

 	if (IS_ERR(core)) {
 		err = PTR_ERR(core);

@@ -899,6 +969,9 @@ static int spmi_pmic_arb_probe(struct 
 platform_device *pdev)
 		pa->core = core;
 		pa->ver_ops = &pmic_arb_v2;
 
+		/* the apid to ppid table starts at PMIC_ARB_REG_CHNL(0) */
+		pa->max_periph =  (pa->core_size - PMIC_ARB_REG_CHNL(0)) / 4;
+
 		res = platform_get_resource_byname(pdev, IORESOURCE_MEM,
 						   "obsrvr");

pa->rd_base = devm_ioremap_resource(&ctrl->dev, res);

Quoted 4 lines

res = platform_get_resource_byname(pdev, IORESOURCE_MEM, "intr");
--
QUALCOMM INDIA, on behalf of Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, hosted by The Linux Foundation --

P.S. Please put a newline in your signature so it doesn't exceed 80 columns.

Quoted 10 lines · Stephen Boyd

Quoted 8 lines · Kiran Gunda

From: Abhijeet Dharmapurikar <[email protected]>

The system crashes due to bad access when reading from an non configured
peripheral and when writing to peripheral which is not owned by current
ee. This patch verifies ownership to avoid crashing on write.

Quoted 4 lines

What systems? As far as I know we don't have any bad accesses happening right now. If they are happening, we should fix the code that's accessing hardware that isn't owned by them.

This change greatly improves the debugging effort for developers by printing
a very simple and clear error message when an invalid SPMI access occurs (due to bad DT configuration, bad bootloader SPMI permission configurations,
or other issues). Without this change, such accesses will cause XPU violations
that crash the system and require extensive effort to decode.

Quoted 9 lines

Quoted 8 lines

For reads, since the forward mapping table, data_channel->ppid, is towards the end of the block, we use the core size to figure the max number of ppids supported. The table starts at an offset of 0x800 within the block, so size - 0x800 will give us the area used by the table. Since each table is 4 bytes long (core_size - 0x800) / 4 will gives us the number of data_channel supported.
This new protection is functional on hw v2.

Quoted 8 lines

Which brings us to the next question which is why do we need this patch at all? We aren't probing hardware to see what we have access to and then populating device structures based on that. Instead, we're just populating DT nodes that we've hardcoded in the dts files, so I'm a little lost on why we would have a node in there that we couldn't access. Please add such details to the commit text.

invalid SPMI access occurs due to bad DT configuration, bad bootloader SPMI
permission configurations, or other issues. This change reduces the debugging
effort for developers by printing clear error message when an invalid SPMI
access occurs.

Quoted 15 lines · On 06/12, [email protected] wrote:

Quoted 10 lines · Stephen Boyd

Quoted 8 lines · Kiran Gunda

From: Abhijeet Dharmapurikar <[email protected]>

The system crashes due to bad access when reading from an non configured
peripheral and when writing to peripheral which is not owned by current
ee. This patch verifies ownership to avoid crashing on write.

Quoted 4 lines

What systems? As far as I know we don't have any bad accesses happening right now. If they are happening, we should fix the code that's accessing hardware that isn't owned by them.

Quoted 8 lines

This change greatly improves the debugging effort for developers by printing
a very simple and clear error message when an invalid SPMI access occurs (due to bad DT configuration, bad bootloader SPMI permission configurations,
or other issues). Without this change, such accesses will cause XPU violations
that crash the system and require extensive effort to decode.

Right, but they're easily detectable because we would know almost immediately that something isn't working when we integrate a change. If you update the DT and it stops working, the DT is bad. If you update the bootloader and it stops working, the bootloader is bad, etc.

Quoted 17 lines

Quoted 9 lines

Quoted 8 lines

For reads, since the forward mapping table, data_channel->ppid, is towards the end of the block, we use the core size to figure the max number of ppids supported. The table starts at an offset of 0x800 within the block, so size - 0x800 will give us the area used by the table. Since each table is 4 bytes long (core_size - 0x800) / 4 will gives us the number of data_channel supported.
This new protection is functional on hw v2.

Quoted 8 lines

Which brings us to the next question which is why do we need this patch at all? We aren't probing hardware to see what we have access to and then populating device structures based on that. Instead, we're just populating DT nodes that we've hardcoded in the dts files, so I'm a little lost on why we would have a node in there that we couldn't access. Please add such details to the commit text.

Quoted 7 lines

invalid SPMI access occurs due to bad DT configuration, bad bootloader SPMI
permission configurations, or other issues. This change reduces the debugging
effort for developers by printing clear error message when an invalid SPMI
access occurs.

Well we also take an overhead on every read/write. Sure things are slow so the overhead is negligible, but the permissions are on a peripheral id basis, so really we should look into _not_ populating devices that aren't accessible in the first place. Then we move the checks out of the read/write path and to a more logical place whereby we prevent a driver from attempting to even attach to read or write a register that is protected.

From: Abhijeet Dharmapurikar <[email protected]>

Usually *_dev best used for structures that embed a struct device in them. spmi_pmic_arb_dev doesn't embed one. It is simply a driver data structure. Use an appropriate name for it.

Also there are many places in the driver that left shift the bit to generate a bit mask. Replace it with the BIT() macro.

Signed-off-by Abhijeet Dharmapurikar <[email protected]>

Signed-off-by Kiran Gunda <[email protected]>

---
 drivers/spmi/spmi-pmic-arb.c | 164 +++++++++++++++++++++----------------------

 1 file changed, 82 insertions(+), 82 deletions(-)

drivers/spmi/spmi-pmic-arb.c +6 -6

index df463d4..7f918ea 100644
@@ -58,10 +58,10 @@
 
 /* Channel Status fields */
 enum pmic_arb_chnl_status {
-	PMIC_ARB_STATUS_DONE	= (1 << 0),
-	PMIC_ARB_STATUS_FAILURE	= (1 << 1),
-	PMIC_ARB_STATUS_DENIED	= (1 << 2),
-	PMIC_ARB_STATUS_DROPPED	= (1 << 3),
+	PMIC_ARB_STATUS_DONE	= BIT(0),
+	PMIC_ARB_STATUS_FAILURE	= BIT(1),
+	PMIC_ARB_STATUS_DENIED	= BIT(2),
+	PMIC_ARB_STATUS_DROPPED	= BIT(3),
 };
 
 /* Command register fields */

@@ -99,7 +99,7 @@ enum pmic_arb_cmd_op_code {
 struct pmic_arb_ver_ops;
 
 /**
- * spmi_pmic_arb_dev - SPMI PMIC Arbiter object
+ * spmi_pmic_arb - SPMI PMIC Arbiter object
  *
  * @rd_base:		on v1 "core", on v2 "observer" register base off DT.
  * @wr_base:		on v1 "core", on v2 "chnls"    register base off DT.

@@ -120,7 +120,7 @@ enum pmic_arb_cmd_op_code {
  * @ppid_to_chan	in-memory copy of PPID -> channel (APID) mapping table.
  *			v2 only.
  */
-struct spmi_pmic_arb_dev {
+struct spmi_pmic_arb {
 	void __iomem		*rd_base;
 	void __iomem		*wr_base;
 	void __iomem		*intr;

drivers/spmi/spmi-pmic-arb.c +1 -1

 			mode_t *mode);
 	/* spmi commands (read_cmd, write_cmd, cmd) functionality */
-	int (*offset)(struct spmi_pmic_arb_dev *dev, u8 sid, u16 addr,
+	int (*offset)(struct spmi_pmic_arb *dev, u8 sid, u16 addr,
 		      u32 *offset);
 	u32 (*fmt_cmd)(u8 opc, u8 sid, u16 addr, u8 bc);
 	int (*non_data_cmd)(struct spmi_controller *ctrl, u8 opc, u8 sid);

drivers/spmi/spmi-pmic-arb.c +2 -2

 }
 
-static inline void pmic_arb_set_rd_cmd(struct spmi_pmic_arb_dev *dev,
+static inline void pmic_arb_set_rd_cmd(struct spmi_pmic_arb *pa,
 				       u32 offset, u32 val)
 {
-	writel_relaxed(val, dev->rd_base + offset);
+	writel_relaxed(val, pa->rd_base + offset);
 }
 
 /**

drivers/spmi/spmi-pmic-arb.c +2 -1

 {
-	u32 data = __raw_readl(dev->rd_base + reg);
+	u32 data = __raw_readl(pa->rd_base + reg);
+
 	memcpy(buf, &data, (bc & 3) + 1);
 }
 

@@ -209,23 +210,24 @@ static void pa_read_data(struct spmi_pmic_arb_dev *dev, u8 *buf, u32 reg, u8 bc)
  * @buf:	buffer to write. length must be bc + 1.

drivers/spmi/spmi-pmic-arb.c +2 -2

 }
 

@@ -270,22 +272,22 @@ static int pmic_arb_wait_for_done(struct spmi_controller *ctrl,
 static int
 pmic_arb_non_data_cmd_v1(struct spmi_controller *ctrl, u8 opc, u8 sid)
 {
-	struct spmi_pmic_arb_dev *pmic_arb = spmi_controller_get_drvdata(ctrl);
+	struct spmi_pmic_arb *pa = spmi_controller_get_drvdata(ctrl);
 	unsigned long flags;
 	u32 cmd;
 	int rc;
 	u32 offset;
 
-	rc = pmic_arb->ver_ops->offset(pmic_arb, sid, 0, &offset);
+	rc = pa->ver_ops->offset(pa, sid, 0, &offset);
 	if (rc)
 		return rc;
 
 	cmd = ((opc | 0x40) << 27) | ((sid & 0xf) << 20);
 

drivers/spmi/spmi-pmic-arb.c +1 -1

 }

@@ -299,7 +301,7 @@ static int pmic_arb_wait_for_done(struct spmi_controller *ctrl,
 /* Non-data command */
 static int pmic_arb_cmd(struct spmi_controller *ctrl, u8 opc, u8 sid)
 {
-	struct spmi_pmic_arb_dev *pmic_arb = spmi_controller_get_drvdata(ctrl);
+	struct spmi_pmic_arb *pa = spmi_controller_get_drvdata(ctrl);
 
 	dev_dbg(&ctrl->dev, "cmd op:0x%x sid:%d\n", opc, sid);
 

drivers/spmi/spmi-pmic-arb.c +19 -22

 }
 
 static int pmic_arb_read_cmd(struct spmi_controller *ctrl, u8 opc, u8 sid,
 			     u16 addr, u8 *buf, size_t len)
 {
-	struct spmi_pmic_arb_dev *pmic_arb = spmi_controller_get_drvdata(ctrl);
+	struct spmi_pmic_arb *pa = spmi_controller_get_drvdata(ctrl);
 	unsigned long flags;
 	u8 bc = len - 1;
 	u32 cmd;

@@ -321,16 +323,16 @@ static int pmic_arb_read_cmd(struct spmi_controller *ctrl, u8 opc, u8 sid,
 	u32 offset;
 	mode_t mode;
 
-	rc = pmic_arb->ver_ops->offset(pmic_arb, sid, addr, &offset);
+	rc = pa->ver_ops->offset(pa, sid, addr, &offset);
 	if (rc)
 		return rc;
 
-	rc = pmic_arb->ver_ops->mode(pmic_arb, sid, addr, &mode);
+	rc = pa->ver_ops->mode(pa, sid, addr, &mode);
 	if (rc)
 		return rc;
 
 	if (!(mode & S_IRUSR)) {
-		dev_err(&pmic_arb->spmic->dev,
+		dev_err(&pa->spmic->dev,
 			"error: impermissible read from peripheral sid:%d addr:0x%x\n",
 			sid, addr);
 		return -EPERM;

@@ -353,30 +355,29 @@ static int pmic_arb_read_cmd(struct spmi_controller *ctrl, u8 opc, u8 sid,
 	else
 		return -EINVAL;
 
-	cmd = pmic_arb->ver_ops->fmt_cmd(opc, sid, addr, bc);
+	cmd = pa->ver_ops->fmt_cmd(opc, sid, addr, bc);
 
-	raw_spin_lock_irqsave(&pmic_arb->lock, flags);
-	pmic_arb_set_rd_cmd(pmic_arb, offset + PMIC_ARB_CMD, cmd);
-	rc = pmic_arb_wait_for_done(ctrl, pmic_arb->rd_base, sid, addr);
+	raw_spin_lock_irqsave(&pa->lock, flags);
+	pmic_arb_set_rd_cmd(pa, offset + PMIC_ARB_CMD, cmd);
+	rc = pmic_arb_wait_for_done(ctrl, pa->rd_base, sid, addr);
 	if (rc)
 		goto done;
 
-	pa_read_data(pmic_arb, buf, offset + PMIC_ARB_RDATA0,
+	pa_read_data(pa, buf, offset + PMIC_ARB_RDATA0,
 		     min_t(u8, bc, 3));
 
 	if (bc > 3)
-		pa_read_data(pmic_arb, buf + 4,
-				offset + PMIC_ARB_RDATA1, bc - 4);
+		pa_read_data(pa, buf + 4, offset + PMIC_ARB_RDATA1, bc - 4);
 
 done:
-	raw_spin_unlock_irqrestore(&pmic_arb->lock, flags);
+	raw_spin_unlock_irqrestore(&pa->lock, flags);
 	return rc;
 }
 
 static int pmic_arb_write_cmd(struct spmi_controller *ctrl, u8 opc, u8 sid,
 			      u16 addr, const u8 *buf, size_t len)
 {
-	struct spmi_pmic_arb_dev *pmic_arb = spmi_controller_get_drvdata(ctrl);
+	struct spmi_pmic_arb *pa = spmi_controller_get_drvdata(ctrl);
 	unsigned long flags;
 	u8 bc = len - 1;
 	u32 cmd;

@@ -384,16 +385,16 @@ static int pmic_arb_write_cmd(struct spmi_controller *ctrl, u8 opc, u8 sid,
 	u32 offset;
 	mode_t mode;
 
-	rc = pmic_arb->ver_ops->offset(pmic_arb, sid, addr, &offset);
+	rc = pa->ver_ops->offset(pa, sid, addr, &offset);
 	if (rc)
 		return rc;
 
-	rc = pmic_arb->ver_ops->mode(pmic_arb, sid, addr, &mode);
+	rc = pa->ver_ops->mode(pa, sid, addr, &mode);
 	if (rc)
 		return rc;
 
 	if (!(mode & S_IWUSR)) {
-		dev_err(&pmic_arb->spmic->dev,
+		dev_err(&pa->spmic->dev,
 			"error: impermissible write to peripheral sid:%d addr:0x%x\n",
 			sid, addr);
 		return -EPERM;

@@ -418,20 +419,18 @@ static int pmic_arb_write_cmd(struct spmi_controller *ctrl, u8 opc, u8 sid,
 	else
 		return -EINVAL;
 
-	cmd = pmic_arb->ver_ops->fmt_cmd(opc, sid, addr, bc);
+	cmd = pa->ver_ops->fmt_cmd(opc, sid, addr, bc);
 
 	/* Write data to FIFOs */
-	raw_spin_lock_irqsave(&pmic_arb->lock, flags);
-	pa_write_data(pmic_arb, buf, offset + PMIC_ARB_WDATA0,
-		      min_t(u8, bc, 3));
+	raw_spin_lock_irqsave(&pa->lock, flags);
+	pa_write_data(pa, buf, offset + PMIC_ARB_WDATA0, min_t(u8, bc, 3));
 	if (bc > 3)
-		pa_write_data(pmic_arb, buf + 4,
-				offset + PMIC_ARB_WDATA1, bc - 4);
+		pa_write_data(pa, buf + 4, offset + PMIC_ARB_WDATA1, bc - 4);
 

drivers/spmi/spmi-pmic-arb.c +14 -14

@@ -457,7 +456,7 @@ struct spmi_pmic_arb_qpnpint_type {
 static void qpnpint_spmi_write(struct irq_data *d, u8 reg, void *buf,
 			       size_t len)
 {
-	struct spmi_pmic_arb_dev *pa = irq_data_get_irq_chip_data(d);
+	struct spmi_pmic_arb *pa = irq_data_get_irq_chip_data(d);
 	u8 sid = d->hwirq >> 24;
 	u8 per = d->hwirq >> 16;
 

@@ -470,7 +469,7 @@ static void qpnpint_spmi_write(struct irq_data *d, u8 reg, void *buf,
 
 static void qpnpint_spmi_read(struct irq_data *d, u8 reg, void *buf, size_t len)
 {
-	struct spmi_pmic_arb_dev *pa = irq_data_get_irq_chip_data(d);
+	struct spmi_pmic_arb *pa = irq_data_get_irq_chip_data(d);
 	u8 sid = d->hwirq >> 24;
 	u8 per = d->hwirq >> 16;
 

@@ -481,7 +480,7 @@ static void qpnpint_spmi_read(struct irq_data *d, u8 reg, void *buf, size_t len)
 				    d->irq);
 }
 
-static void periph_interrupt(struct spmi_pmic_arb_dev *pa, u8 apid)
+static void periph_interrupt(struct spmi_pmic_arb *pa, u8 apid)
 {
 	unsigned int irq;
 	u32 status;

@@ -490,7 +489,7 @@ static void periph_interrupt(struct spmi_pmic_arb_dev *pa, u8 apid)
 	status = readl_relaxed(pa->intr + pa->ver_ops->irq_status(apid));
 	while (status) {
 		id = ffs(status) - 1;
-		status &= ~(1 << id);
+		status &= ~BIT(id);
 		irq = irq_find_mapping(pa->domain,
 				       pa->apid_to_ppid[apid] << 16
 				     | id << 8

@@ -501,7 +500,7 @@ static void periph_interrupt(struct spmi_pmic_arb_dev *pa, u8 apid)
 
 static void pmic_arb_chained_irq(struct irq_desc *desc)
 {
-	struct spmi_pmic_arb_dev *pa = irq_desc_get_handler_data(desc);
+	struct spmi_pmic_arb *pa = irq_desc_get_handler_data(desc);
 	struct irq_chip *chip = irq_desc_get_chip(desc);
 	void __iomem *intr = pa->intr;
 	int first = pa->min_apid >> 5;

@@ -516,7 +515,7 @@ static void pmic_arb_chained_irq(struct irq_desc *desc)
 				      pa->ver_ops->owner_acc_status(pa->ee, i));
 		while (status) {
 			id = ffs(status) - 1;
-			status &= ~(1 << id);
+			status &= ~BIT(id);
 			periph_interrupt(pa, id + i * 32);
 		}
 	}

@@ -526,23 +525,23 @@ static void pmic_arb_chained_irq(struct irq_desc *desc)
 
 static void qpnpint_irq_ack(struct irq_data *d)
 {
-	struct spmi_pmic_arb_dev *pa = irq_data_get_irq_chip_data(d);
+	struct spmi_pmic_arb *pa = irq_data_get_irq_chip_data(d);
 	u8 irq  = d->hwirq >> 8;
 	u8 apid = d->hwirq;
 	unsigned long flags;
 	u8 data;
 
 	raw_spin_lock_irqsave(&pa->lock, flags);
-	writel_relaxed(1 << irq, pa->intr + pa->ver_ops->irq_clear(apid));
+	writel_relaxed(BIT(irq), pa->intr + pa->ver_ops->irq_clear(apid));
 	raw_spin_unlock_irqrestore(&pa->lock, flags);
 
-	data = 1 << irq;
+	data = BIT(irq);
 	qpnpint_spmi_write(d, QPNPINT_REG_LATCHED_CLR, &data, 1);
 }
 
 static void qpnpint_irq_mask(struct irq_data *d)
 {
-	struct spmi_pmic_arb_dev *pa = irq_data_get_irq_chip_data(d);
+	struct spmi_pmic_arb *pa = irq_data_get_irq_chip_data(d);
 	u8 irq  = d->hwirq >> 8;
 	u8 apid = d->hwirq;
 	unsigned long flags;

@@ -558,13 +557,13 @@ static void qpnpint_irq_mask(struct irq_data *d)
 	}
 	raw_spin_unlock_irqrestore(&pa->lock, flags);
 
-	data = 1 << irq;
+	data = BIT(irq);
 	qpnpint_spmi_write(d, QPNPINT_REG_EN_CLR, &data, 1);
 }
 
 static void qpnpint_irq_unmask(struct irq_data *d)
 {
-	struct spmi_pmic_arb_dev *pa = irq_data_get_irq_chip_data(d);
+	struct spmi_pmic_arb *pa = irq_data_get_irq_chip_data(d);
 	u8 irq  = d->hwirq >> 8;
 	u8 apid = d->hwirq;
 	unsigned long flags;

@@ -579,7 +578,7 @@ static void qpnpint_irq_unmask(struct irq_data *d)
 	}
 	raw_spin_unlock_irqrestore(&pa->lock, flags);
 
-	data = 1 << irq;
+	data = BIT(irq);
 	qpnpint_spmi_write(d, QPNPINT_REG_EN_SET, &data, 1);
 }
 

@@ -590,7 +589,7 @@ static void qpnpint_irq_enable(struct irq_data *d)
 
 	qpnpint_irq_unmask(d);
 
-	data = 1 << irq;
+	data = BIT(irq);
 	qpnpint_spmi_write(d, QPNPINT_REG_LATCHED_CLR, &data, 1);
 }
 

Quoted 6 lines · Kiran Gunda

From: Abhijeet Dharmapurikar <[email protected]>

Usually *_dev best used for structures that embed a struct device in them. spmi_pmic_arb_dev doesn't embed one. It is simply a driver data structure. Use an appropriate name for it.

Quoted 2 lines

Also there are many places in the driver that left shift the bit to generate a bit mask. Replace it with the BIT() macro.

That would be a different patch because the subject doesn't even mention this.

Quoted 6 lines

Signed-off-by Abhijeet Dharmapurikar <[email protected]>

Signed-off-by Kiran Gunda <[email protected]>

---
 drivers/spmi/spmi-pmic-arb.c | 164 

+++++++++++++++++++++----------------------

Would also be nice if you ran scripts/objdiff on this so we can be confident the code didn't change.

Quoted 41 lines

 1 file changed, 82 insertions(+), 82 deletions(-)

drivers/spmi/spmi-pmic-arb.c +5 -5

index df463d4..7f918ea 100644
@@ -58,10 +58,10 @@
 
 /* Channel Status fields */
 enum pmic_arb_chnl_status {
-	PMIC_ARB_STATUS_DONE	= (1 << 0),
-	PMIC_ARB_STATUS_FAILURE	= (1 << 1),
-	PMIC_ARB_STATUS_DENIED	= (1 << 2),
-	PMIC_ARB_STATUS_DROPPED	= (1 << 3),
+	PMIC_ARB_STATUS_DONE	= BIT(0),
+	PMIC_ARB_STATUS_FAILURE	= BIT(1),
+	PMIC_ARB_STATUS_DENIED	= BIT(2),
+	PMIC_ARB_STATUS_DROPPED	= BIT(3),
 };
 
 /* Command register fields */

@@ -99,7 +99,7 @@ enum pmic_arb_cmd_op_code {
 struct pmic_arb_ver_ops;
 
 /**
- * spmi_pmic_arb_dev - SPMI PMIC Arbiter object
+ * spmi_pmic_arb - SPMI PMIC Arbiter object
  *
  * @rd_base:		on v1 "core", on v2 "observer" register base off DT.
  * @wr_base:		on v1 "core", on v2 "chnls"    register base off DT.

@@ -120,7 +120,7 @@ enum pmic_arb_cmd_op_code {
  * @ppid_to_chan	in-memory copy of PPID -> channel (APID) mapping 

table.
* v2 only.
*/

drivers/spmi/spmi-pmic-arb.c +1 -1

-struct spmi_pmic_arb_dev {
+struct spmi_pmic_arb {
 	void __iomem		*rd_base;
 	void __iomem		*wr_base;
 	void __iomem		*intr;

@@ -164,10 +164,10 @@ struct spmi_pmic_arb_dev {

drivers/spmi/spmi-pmic-arb.c struct spmi_pmic_arb_dev { · line 167

Quoted 5 lines

drivers/spmi/spmi-pmic-arb.c +1 -1

  *			on v2 offset of SPMI_PIC_IRQ_CLEARn.
  */
 struct pmic_arb_ver_ops {
-	int (*mode)(struct spmi_pmic_arb_dev *dev, u8 sid, u16 addr,
+	int (*mode)(struct spmi_pmic_arb *dev, u8 sid, u16 addr,

But we leave dev here? I'm losing faith that this patch is worthwhile.

Quoted 12 lines

drivers/spmi/spmi-pmic-arb.c +1 -1

 			mode_t *mode);
 	/* spmi commands (read_cmd, write_cmd, cmd) functionality */
-	int (*offset)(struct spmi_pmic_arb_dev *dev, u8 sid, u16 addr,
+	int (*offset)(struct spmi_pmic_arb *dev, u8 sid, u16 addr,
 		      u32 *offset);
 	u32 (*fmt_cmd)(u8 opc, u8 sid, u16 addr, u8 bc);
 	int (*non_data_cmd)(struct spmi_controller *ctrl, u8 opc, u8 sid);

@@ -178,16 +178,16 @@ struct pmic_arb_ver_ops {
 	u32 (*irq_clear)(u8 n);
 };
 

drivers/spmi/spmi-pmic-arb.c struct pmic_arb_ver_ops { · line 184

Quoted 6 lines

drivers/spmi/spmi-pmic-arb.c +2 -2

-static inline void pmic_arb_base_write(struct spmi_pmic_arb_dev *dev,
+static inline void pmic_arb_base_write(struct spmi_pmic_arb *pa,
 				       u32 offset, u32 val)
 {
-	writel_relaxed(val, dev->wr_base + offset);
+	writel_relaxed(val, pa->wr_base + offset);

"pa" is a little confusing with things like physical address and such. I would have gone for "arb", but the code is written already, so why change it now?

Quoted 10 lines

drivers/spmi/spmi-pmic-arb.c +2 -2

 }
 
-static inline void pmic_arb_set_rd_cmd(struct spmi_pmic_arb_dev *dev,
+static inline void pmic_arb_set_rd_cmd(struct spmi_pmic_arb *pa,
 				       u32 offset, u32 val)
 {
-	writel_relaxed(val, dev->rd_base + offset);
+	writel_relaxed(val, pa->rd_base + offset);
 }
 

drivers/spmi/spmi-pmic-arb.c static inline void pmic_arb_set_rd_cmd(struct · line 202

Quoted 10 lines

drivers/spmi/spmi-pmic-arb.c +1 -1

 /**

@@ -196,9 +196,10 @@ static inline void pmic_arb_set_rd_cmd(struct 
 spmi_pmic_arb_dev *dev,
  * @reg:	register's address
  * @buf:	output parameter, length must be bc + 1
  */
-static void pa_read_data(struct spmi_pmic_arb_dev *dev, u8 *buf, u32 
 reg, u8 bc)
+static void pa_read_data(struct spmi_pmic_arb *pa, u8 *buf, u32 reg, 
 u8 bc)

In fact, I would rename these pa_{read,write}_data() functions as pmic_arb_{read,write}_data() to be consistent. These are the only places "pa_" is used right now.

Quoted 18 lines

drivers/spmi/spmi-pmic-arb.c +2 -1

 {
-	u32 data = __raw_readl(dev->rd_base + reg);
+	u32 data = __raw_readl(pa->rd_base + reg);
+

drivers/spmi/spmi-pmic-arb.c +1 -1

 	memcpy(buf, &data, (bc & 3) + 1);
 }

@@ -209,23 +210,24 @@ static void pa_read_data(struct 
 spmi_pmic_arb_dev *dev, u8 *buf, u32 reg, u8 bc)
  * @buf:	buffer to write. length must be bc + 1.
  */
 static void
-pa_write_data(struct spmi_pmic_arb_dev *dev, const u8 *buf, u32 reg, 
 u8 bc)
+pa_write_data(struct spmi_pmic_arb *pa, const u8 *buf, u32 reg, u8 
 bc)
 {
 	u32 data = 0;

drivers/spmi/spmi-pmic-arb.c static void pa_read_data(struct spmi_pmic_arb_dev *dev, u8 *buf, u32 reg, u8 bc) · line 218

Quoted 4 lines

drivers/spmi/spmi-pmic-arb.c +2 -1

+
 	memcpy(&data, buf, (bc & 3) + 1);
-	__raw_writel(data, dev->wr_base + reg);
+	pmic_arb_base_write(pa, reg, data);

This is an unrelated change. Not sure what's going on with this diff but we most likely want to keep the __raw_writel() here. See how renames introduce bugs and why we don't value them?

Quoted 32 lines

drivers/spmi/spmi-pmic-arb.c +6 -6

 }
 

@@ -270,22 +272,22 @@ static int pmic_arb_wait_for_done(struct 
 spmi_controller *ctrl,
 static int
 pmic_arb_non_data_cmd_v1(struct spmi_controller *ctrl, u8 opc, u8 
 sid)
 {
-	struct spmi_pmic_arb_dev *pmic_arb = 
 spmi_controller_get_drvdata(ctrl);
+	struct spmi_pmic_arb *pa = spmi_controller_get_drvdata(ctrl);
 	unsigned long flags;
 	u32 cmd;
 	int rc;
 	u32 offset;
 
-	rc = pmic_arb->ver_ops->offset(pmic_arb, sid, 0, &offset);
+	rc = pa->ver_ops->offset(pa, sid, 0, &offset);
 	if (rc)
 		return rc;
 
 	cmd = ((opc | 0x40) << 27) | ((sid & 0xf) << 20);
 
-	raw_spin_lock_irqsave(&pmic_arb->lock, flags);
-	pmic_arb_base_write(pmic_arb, offset + PMIC_ARB_CMD, cmd);
-	rc = pmic_arb_wait_for_done(ctrl, pmic_arb->wr_base, sid, 0);
-	raw_spin_unlock_irqrestore(&pmic_arb->lock, flags);
+	raw_spin_lock_irqsave(&pa->lock, flags);
+	pmic_arb_base_write(pa, offset + PMIC_ARB_CMD, cmd);
+	rc = pmic_arb_wait_for_done(ctrl, pa->wr_base, sid, 0);
+	raw_spin_unlock_irqrestore(&pa->lock, flags);

Quoted 1 line

return rc;

Yeah pmic_arb sounds fine too. Not sure why we changed anything in this function.

Quoted 17 lines

}

drivers/spmi/spmi-pmic-arb.c +1 -1

@@ -299,7 +301,7 @@ static int pmic_arb_wait_for_done(struct 
 spmi_controller *ctrl,
 /* Non-data command */
 static int pmic_arb_cmd(struct spmi_controller *ctrl, u8 opc, u8 sid)
 {
-	struct spmi_pmic_arb_dev *pmic_arb = 
 spmi_controller_get_drvdata(ctrl);
+	struct spmi_pmic_arb *pa = spmi_controller_get_drvdata(ctrl);
 

drivers/spmi/spmi-pmic-arb.c +0 -0

 	dev_dbg(&ctrl->dev, "cmd op:0x%x sid:%d\n", opc, sid);

@@ -307,13 +309,13 @@ static int pmic_arb_cmd(struct spmi_controller 
 *ctrl, u8 opc, u8 sid)
 	if (opc < SPMI_CMD_RESET || opc > SPMI_CMD_WAKEUP)
 		return -EINVAL;
 

drivers/spmi/spmi-pmic-arb.c static int pmic_arb_cmd(struct spmi_controller *ctrl, u8 opc, u8 sid) · line 312

Quoted 2 lines

drivers/spmi/spmi-pmic-arb.c +1 -1

-	return pmic_arb->ver_ops->non_data_cmd(ctrl, opc, sid);
+	return pa->ver_ops->non_data_cmd(ctrl, opc, sid);

Same story...

Quoted 123 lines

drivers/spmi/spmi-pmic-arb.c +1 -1

 }
 
 static int pmic_arb_read_cmd(struct spmi_controller *ctrl, u8 opc, u8 
 sid,
 			     u16 addr, u8 *buf, size_t len)
 {
-	struct spmi_pmic_arb_dev *pmic_arb = 
 spmi_controller_get_drvdata(ctrl);
+	struct spmi_pmic_arb *pa = spmi_controller_get_drvdata(ctrl);

drivers/spmi/spmi-pmic-arb.c +3 -3

 	unsigned long flags;
 	u8 bc = len - 1;
 	u32 cmd;

@@ -321,16 +323,16 @@ static int pmic_arb_read_cmd(struct 
 spmi_controller *ctrl, u8 opc, u8 sid,
 	u32 offset;
 	mode_t mode;
 
-	rc = pmic_arb->ver_ops->offset(pmic_arb, sid, addr, &offset);
+	rc = pa->ver_ops->offset(pa, sid, addr, &offset);
 	if (rc)
 		return rc;
 
-	rc = pmic_arb->ver_ops->mode(pmic_arb, sid, addr, &mode);
+	rc = pa->ver_ops->mode(pa, sid, addr, &mode);
 	if (rc)
 		return rc;
 
 	if (!(mode & S_IRUSR)) {
-		dev_err(&pmic_arb->spmic->dev,
+		dev_err(&pa->spmic->dev,
 			"error: impermissible read from peripheral sid:%d addr:0x%x\n",
 			sid, addr);

drivers/spmi/spmi-pmic-arb.c +8 -9

 		return -EPERM;

@@ -353,30 +355,29 @@ static int pmic_arb_read_cmd(struct 
 spmi_controller *ctrl, u8 opc, u8 sid,
 	else
 		return -EINVAL;
 
-	cmd = pmic_arb->ver_ops->fmt_cmd(opc, sid, addr, bc);
+	cmd = pa->ver_ops->fmt_cmd(opc, sid, addr, bc);
 
-	raw_spin_lock_irqsave(&pmic_arb->lock, flags);
-	pmic_arb_set_rd_cmd(pmic_arb, offset + PMIC_ARB_CMD, cmd);
-	rc = pmic_arb_wait_for_done(ctrl, pmic_arb->rd_base, sid, addr);
+	raw_spin_lock_irqsave(&pa->lock, flags);
+	pmic_arb_set_rd_cmd(pa, offset + PMIC_ARB_CMD, cmd);
+	rc = pmic_arb_wait_for_done(ctrl, pa->rd_base, sid, addr);
 	if (rc)
 		goto done;
 
-	pa_read_data(pmic_arb, buf, offset + PMIC_ARB_RDATA0,
+	pa_read_data(pa, buf, offset + PMIC_ARB_RDATA0,
 		     min_t(u8, bc, 3));
 
 	if (bc > 3)
-		pa_read_data(pmic_arb, buf + 4,
-				offset + PMIC_ARB_RDATA1, bc - 4);
+		pa_read_data(pa, buf + 4, offset + PMIC_ARB_RDATA1, bc - 4);
 
 done:
-	raw_spin_unlock_irqrestore(&pmic_arb->lock, flags);
+	raw_spin_unlock_irqrestore(&pa->lock, flags);
 	return rc;
 }
 
 static int pmic_arb_write_cmd(struct spmi_controller *ctrl, u8 opc, 
 u8 sid,
 			      u16 addr, const u8 *buf, size_t len)
 {
-	struct spmi_pmic_arb_dev *pmic_arb = 
 spmi_controller_get_drvdata(ctrl);
+	struct spmi_pmic_arb *pa = spmi_controller_get_drvdata(ctrl);

drivers/spmi/spmi-pmic-arb.c +3 -3

 	unsigned long flags;
 	u8 bc = len - 1;
 	u32 cmd;

@@ -384,16 +385,16 @@ static int pmic_arb_write_cmd(struct 
 spmi_controller *ctrl, u8 opc, u8 sid,
 	u32 offset;
 	mode_t mode;
 
-	rc = pmic_arb->ver_ops->offset(pmic_arb, sid, addr, &offset);
+	rc = pa->ver_ops->offset(pa, sid, addr, &offset);
 	if (rc)
 		return rc;
 
-	rc = pmic_arb->ver_ops->mode(pmic_arb, sid, addr, &mode);
+	rc = pa->ver_ops->mode(pa, sid, addr, &mode);
 	if (rc)
 		return rc;
 
 	if (!(mode & S_IWUSR)) {
-		dev_err(&pmic_arb->spmic->dev,
+		dev_err(&pa->spmic->dev,
 			"error: impermissible write to peripheral sid:%d addr:0x%x\n",
 			sid, addr);

drivers/spmi/spmi-pmic-arb.c +7 -9

 		return -EPERM;

@@ -418,20 +419,18 @@ static int pmic_arb_write_cmd(struct 
 spmi_controller *ctrl, u8 opc, u8 sid,
 	else
 		return -EINVAL;
 
-	cmd = pmic_arb->ver_ops->fmt_cmd(opc, sid, addr, bc);
+	cmd = pa->ver_ops->fmt_cmd(opc, sid, addr, bc);
 
 	/* Write data to FIFOs */
-	raw_spin_lock_irqsave(&pmic_arb->lock, flags);
-	pa_write_data(pmic_arb, buf, offset + PMIC_ARB_WDATA0,
-		      min_t(u8, bc, 3));
+	raw_spin_lock_irqsave(&pa->lock, flags);
+	pa_write_data(pa, buf, offset + PMIC_ARB_WDATA0, min_t(u8, bc, 3));
 	if (bc > 3)
-		pa_write_data(pmic_arb, buf + 4,
-				offset + PMIC_ARB_WDATA1, bc - 4);
+		pa_write_data(pa, buf + 4, offset + PMIC_ARB_WDATA1, bc - 4);
 
 	/* Start the transaction */
-	pmic_arb_base_write(pmic_arb, offset + PMIC_ARB_CMD, cmd);
-	rc = pmic_arb_wait_for_done(ctrl, pmic_arb->wr_base, sid, addr);
-	raw_spin_unlock_irqrestore(&pmic_arb->lock, flags);
+	pmic_arb_base_write(pa, offset + PMIC_ARB_CMD, cmd);
+	rc = pmic_arb_wait_for_done(ctrl, pa->wr_base, sid, addr);
+	raw_spin_unlock_irqrestore(&pa->lock, flags);
 

Quoted 2 lines

drivers/spmi/spmi-pmic-arb.c +0 -0

 	return rc;

}

Same story for all this diff.

Quoted 124 lines

drivers/spmi/spmi-pmic-arb.c +2 -2

@@ -457,7 +456,7 @@ struct spmi_pmic_arb_qpnpint_type {
 static void qpnpint_spmi_write(struct irq_data *d, u8 reg, void *buf,
 			       size_t len)
 {
-	struct spmi_pmic_arb_dev *pa = irq_data_get_irq_chip_data(d);
+	struct spmi_pmic_arb *pa = irq_data_get_irq_chip_data(d);
 	u8 sid = d->hwirq >> 24;
 	u8 per = d->hwirq >> 16;
 

@@ -470,7 +469,7 @@ static void qpnpint_spmi_write(struct irq_data *d, 
 u8 reg, void *buf,
 
 static void qpnpint_spmi_read(struct irq_data *d, u8 reg, void *buf, 
 size_t len)
 {
-	struct spmi_pmic_arb_dev *pa = irq_data_get_irq_chip_data(d);
+	struct spmi_pmic_arb *pa = irq_data_get_irq_chip_data(d);
 	u8 sid = d->hwirq >> 24;

drivers/spmi/spmi-pmic-arb.c +1 -1

 	u8 per = d->hwirq >> 16;

@@ -481,7 +480,7 @@ static void qpnpint_spmi_read(struct irq_data *d, 
 u8 reg, void *buf, size_t len)
 				    d->irq);
 }
 
-static void periph_interrupt(struct spmi_pmic_arb_dev *pa, u8 apid)
+static void periph_interrupt(struct spmi_pmic_arb *pa, u8 apid)
 {
 	unsigned int irq;

drivers/spmi/spmi-pmic-arb.c +1 -1

 	u32 status;

@@ -490,7 +489,7 @@ static void periph_interrupt(struct 
 spmi_pmic_arb_dev *pa, u8 apid)
 	status = readl_relaxed(pa->intr + pa->ver_ops->irq_status(apid));
 	while (status) {
 		id = ffs(status) - 1;
-		status &= ~(1 << id);
+		status &= ~BIT(id);
 		irq = irq_find_mapping(pa->domain,
 				       pa->apid_to_ppid[apid] << 16

drivers/spmi/spmi-pmic-arb.c +1 -1

 				     | id << 8

@@ -501,7 +500,7 @@ static void periph_interrupt(struct 
 spmi_pmic_arb_dev *pa, u8 apid)
 
 static void pmic_arb_chained_irq(struct irq_desc *desc)
 {
-	struct spmi_pmic_arb_dev *pa = irq_desc_get_handler_data(desc);
+	struct spmi_pmic_arb *pa = irq_desc_get_handler_data(desc);
 	struct irq_chip *chip = irq_desc_get_chip(desc);
 	void __iomem *intr = pa->intr;

drivers/spmi/spmi-pmic-arb.c +1 -1

 	int first = pa->min_apid >> 5;

@@ -516,7 +515,7 @@ static void pmic_arb_chained_irq(struct irq_desc 
 *desc)
 				      pa->ver_ops->owner_acc_status(pa->ee, i));
 		while (status) {
 			id = ffs(status) - 1;
-			status &= ~(1 << id);
+			status &= ~BIT(id);
 			periph_interrupt(pa, id + i * 32);
 		}

drivers/spmi/spmi-pmic-arb.c +4 -4

 	}

@@ -526,23 +525,23 @@ static void pmic_arb_chained_irq(struct irq_desc 
 *desc)
 
 static void qpnpint_irq_ack(struct irq_data *d)
 {
-	struct spmi_pmic_arb_dev *pa = irq_data_get_irq_chip_data(d);
+	struct spmi_pmic_arb *pa = irq_data_get_irq_chip_data(d);
 	u8 irq  = d->hwirq >> 8;
 	u8 apid = d->hwirq;
 	unsigned long flags;
 	u8 data;
 
 	raw_spin_lock_irqsave(&pa->lock, flags);
-	writel_relaxed(1 << irq, pa->intr + pa->ver_ops->irq_clear(apid));
+	writel_relaxed(BIT(irq), pa->intr + pa->ver_ops->irq_clear(apid));
 	raw_spin_unlock_irqrestore(&pa->lock, flags);
 
-	data = 1 << irq;
+	data = BIT(irq);
 	qpnpint_spmi_write(d, QPNPINT_REG_LATCHED_CLR, &data, 1);
 }
 
 static void qpnpint_irq_mask(struct irq_data *d)
 {
-	struct spmi_pmic_arb_dev *pa = irq_data_get_irq_chip_data(d);
+	struct spmi_pmic_arb *pa = irq_data_get_irq_chip_data(d);
 	u8 irq  = d->hwirq >> 8;
 	u8 apid = d->hwirq;

drivers/spmi/spmi-pmic-arb.c +4 -4

 	unsigned long flags;

@@ -558,13 +557,13 @@ static void qpnpint_irq_mask(struct irq_data *d)
 	}
 	raw_spin_unlock_irqrestore(&pa->lock, flags);
 
-	data = 1 << irq;
+	data = BIT(irq);
 	qpnpint_spmi_write(d, QPNPINT_REG_EN_CLR, &data, 1);
 }
 
 static void qpnpint_irq_unmask(struct irq_data *d)
 {
-	struct spmi_pmic_arb_dev *pa = irq_data_get_irq_chip_data(d);
+	struct spmi_pmic_arb *pa = irq_data_get_irq_chip_data(d);
 	u8 irq  = d->hwirq >> 8;
 	u8 apid = d->hwirq;
 	unsigned long flags;

@@ -579,7 +578,7 @@ static void qpnpint_irq_unmask(struct irq_data *d)
 	}
 	raw_spin_unlock_irqrestore(&pa->lock, flags);
 
-	data = 1 << irq;
+	data = BIT(irq);
 	qpnpint_spmi_write(d, QPNPINT_REG_EN_SET, &data, 1);
 }
 

@@ -590,7 +589,7 @@ static void qpnpint_irq_enable(struct irq_data *d)
 
 	qpnpint_irq_unmask(d);
 
-	data = 1 << irq;
+	data = BIT(irq);
 	qpnpint_spmi_write(d, QPNPINT_REG_LATCHED_CLR, &data, 1);
 }
 

drivers/spmi/spmi-pmic-arb.c static int qpnpint_irq_set_type(struct irq_data · line 601

Quoted 6 lines

drivers/spmi/spmi-pmic-arb.c +1 -0

@@ -598,25 +597,26 @@ static int qpnpint_irq_set_type(struct irq_data 
 *d, unsigned int flow_type)
 {
 	struct spmi_pmic_arb_qpnpint_type type;
 	u8 irq = d->hwirq >> 8;
+	u8 bit_mask_irq = BIT(irq);

Why the local variable? Just do the ~BIT(irq) thing in place and let the compiler take care of the hoist?

Quoted 27 lines

drivers/spmi/spmi-pmic-arb.c +6 -6

 	qpnpint_spmi_read(d, QPNPINT_REG_SET_TYPE, &type, sizeof(type));
 
 	if (flow_type & (IRQF_TRIGGER_RISING | IRQF_TRIGGER_FALLING)) {
-		type.type |= 1 << irq;
+		type.type |= bit_mask_irq;
 		if (flow_type & IRQF_TRIGGER_RISING)
-			type.polarity_high |= 1 << irq;
+			type.polarity_high |= bit_mask_irq;
 		if (flow_type & IRQF_TRIGGER_FALLING)
-			type.polarity_low  |= 1 << irq;
+			type.polarity_low  |= bit_mask_irq;
 	} else {
 		if ((flow_type & (IRQF_TRIGGER_HIGH)) &&
 		    (flow_type & (IRQF_TRIGGER_LOW)))
 			return -EINVAL;
 
-		type.type &= ~(1 << irq); /* level trig */
+		type.type &= ~bit_mask_irq; /* level trig */
 		if (flow_type & IRQF_TRIGGER_HIGH)
-			type.polarity_high |= 1 << irq;
+			type.polarity_high |= bit_mask_irq;
 		else
-			type.polarity_low  |= 1 << irq;
+			type.polarity_low  |= bit_mask_irq;
 	}
 

drivers/spmi/spmi-pmic-arb.c struct spmi_pmic_arb_irq_spec {

Quoted 2 lines

drivers/spmi/spmi-pmic-arb.c +0 -0

 	qpnpint_spmi_write(d, QPNPINT_REG_SET_TYPE, &type, sizeof(type));

@@ -657,7 +657,7 @@ struct spmi_pmic_arb_irq_spec {

Overall I see little to no value with this patch. I suggest you drop the rename. The BIT() thing may be ok, but again, not sure there's any benefit.

Quoted 11 lines

Quoted 11 lines

drivers/spmi/spmi-pmic-arb.c +0 -0

@@ -209,23 +210,24 @@ static void pa_read_data(struct
 spmi_pmic_arb_dev *dev, u8 *buf, u32 reg, u8 bc)
 * @buf:	buffer to write. length must be bc + 1.
 */

static void
-pa_write_data(struct spmi_pmic_arb_dev *dev, const u8 *buf, u32 reg, u8 bc)
+pa_write_data(struct spmi_pmic_arb *pa, const u8 *buf, u32 reg, u8 bc)

{
	u32 data = 0;

Quoted 9 lines

drivers/spmi/spmi-pmic-arb.c static void pa_read_data(struct spmi_pmic_arb_dev *dev, u8 *buf, u32 reg, u8 bc) · line 218

Quoted 4 lines

+

	memcpy(&data, buf, (bc & 3) + 1);

drivers/spmi/spmi-pmic-arb.c +1 -1

-	__raw_writel(data, dev->wr_base + reg);
+	pmic_arb_base_write(pa, reg, data);

This is an unrelated change. Not sure what's going on with this diff but we most likely want to keep the __raw_writel() here. See how renames introduce bugs and why we don't value them?

Actually pmic_arb_base_write has the writel_relaxed inside it. that's why we removed the __raw_writel to use the common function. Anyways, we drop the renaming patch from this patch series.

Quoted 20 lines · On 06/01, [email protected] wrote:

Quoted 20 lines

Quoted 13 lines

drivers/spmi/spmi-pmic-arb.c +0 -0

@@ -209,23 +210,24 @@ static void pa_read_data(struct
 spmi_pmic_arb_dev *dev, u8 *buf, u32 reg, u8 bc)
 * @buf:	buffer to write. length must be bc + 1.
 */

static void
-pa_write_data(struct spmi_pmic_arb_dev *dev, const u8 *buf, u32 reg, u8 bc)
+pa_write_data(struct spmi_pmic_arb *pa, const u8 *buf, u32 reg, u8 bc)

{
	u32 data = 0;
+
	memcpy(&data, buf, (bc & 3) + 1);

drivers/spmi/spmi-pmic-arb.c static void pa_read_data(struct spmi_pmic_arb_dev *dev, u8 *buf, u32 reg, u8 bc) · line 218

Quoted 2 lines

drivers/spmi/spmi-pmic-arb.c +1 -1

-	__raw_writel(data, dev->wr_base + reg);
+	pmic_arb_base_write(pa, reg, data);

This is an unrelated change. Not sure what's going on with this diff but we most likely want to keep the __raw_writel() here. See how renames introduce bugs and why we don't value them?

Quoted 3 lines

Actually pmic_arb_base_write has the writel_relaxed inside it. that's why we removed the __raw_writel to use the common function. Anyways, we drop the renaming patch from this patch series.

__raw_writel() is there on purpose because we're reading bytes at a time and the CPU could be big-endian or little-endian. readl_relaxed() would do a byte swap which we don't want.

From: Abhijeet Dharmapurikar <[email protected]>

The driver currently uses "apid" and "chan" to mean apid. Remove the use of chan and use only apid.

On a SPMI bus there is allocation to manage up to 4K peripherals. However, in practice only few peripherals are instantiated and only few among the instantiated ones actually interrupt.

APID is CPU's way of keeping track of peripherals that could interrupt. There is a table that maps the 256 interrupting peripherals to a number between 0 and 255. This number is called APID. Information about that interrupting peripheral is stored in registers offset by its corresponding apid.

Signed-off-by Abhijeet Dharmapurikar <[email protected]>

Signed-off-by Kiran Gunda <[email protected]>

---
 drivers/spmi/spmi-pmic-arb.c | 68 ++++++++++++++++++++++----------------------
 1 file changed, 34 insertions(+), 34 deletions(-)

drivers/spmi/spmi-pmic-arb.c +0 -0

index 7f918ea..7201611 100644

drivers/spmi/spmi-pmic-arb.c +3 -3

  *			v2 only.
  */
 struct spmi_pmic_arb {

@@ -140,9 +140,9 @@ struct spmi_pmic_arb {
 	struct spmi_controller	*spmic;
 	u16			*apid_to_ppid;
 	const struct pmic_arb_ver_ops *ver_ops;
-	u16			*ppid_to_chan;
-	u16			last_channel;
-	u8			*chan_to_owner;
+	u16			*ppid_to_apid;
+	u16			last_apid;
+	u8			*apid_to_owner;
 };
 
 /**

@@ -772,22 +772,22 @@ static int qpnpint_irq_domain_map(struct irq_domain *d,
 	return 0;
 }
 

drivers/spmi/spmi-pmic-arb.c +5 -5

-	 * ppid_to_chan is an in-memory invert of that table.
+	 * ppid_to_apid is an in-memory invert of that table.
 	 */
-	for (chan = pa->last_channel; chan < pa->max_periph; chan++) {
+	for (apid = pa->last_apid; apid < pa->max_periph; apid++) {
 		regval = readl_relaxed(pa->cnfg +
-				      SPMI_OWNERSHIP_TABLE_REG(chan));
-		pa->chan_to_owner[chan] = SPMI_OWNERSHIP_PERIPH2OWNER(regval);
+				      SPMI_OWNERSHIP_TABLE_REG(apid));
+		pa->apid_to_owner[apid] = SPMI_OWNERSHIP_PERIPH2OWNER(regval);
 
-		offset = PMIC_ARB_REG_CHNL(chan);
+		offset = PMIC_ARB_REG_CHNL(apid);
 		if (offset >= pa->core_size)
 			break;
 

drivers/spmi/spmi-pmic-arb.c +3 -3

 		if (id == ppid) {
-			chan |= PMIC_ARB_CHAN_VALID;
+			apid |= PMIC_ARB_CHAN_VALID;
 			break;
 		}
 	}
-	pa->last_channel = chan & ~PMIC_ARB_CHAN_VALID;
+	pa->last_apid = apid & ~PMIC_ARB_CHAN_VALID;
 
-	return chan;
+	return apid;
 }
 
 

Quoted 4 lines · Kiran Gunda

From: Abhijeet Dharmapurikar <[email protected]>

The driver currently uses "apid" and "chan" to mean apid. Remove the use of chan and use only apid.

I'm not so sure. It currently uses "chan" to mean the offset to add to the "PMIC Arbiter channel registers" so that we can access the appropriate peripheral via the arbiter registers. I actually can't remember what APID or PPID stand for, so perhaps describing that as well would be helpful so we can navigate this acronym soup.

Quoted 10 lines

On a SPMI bus there is allocation to manage up to 4K peripherals. However, in practice only few peripherals are instantiated and only few among the instantiated ones actually interrupt.

APID is CPU's way of keeping track of peripherals that could interrupt. There is a table that maps the 256 interrupting peripherals to a number between 0 and 255. This number is called APID. Information about that interrupting peripheral is stored in registers offset by its corresponding apid.

That's all fine, but perhaps we shouldn't worry about "apid" being attached to interrupts? I mean, I can imagine some peripheral that doesn't interrupt, but we want to read/write it and that must be done with the "channel" or "apid" or really the "magic offset from the base of the channel registers" to do so. Probably APID is fine, as long as APID means "application processor peripheral id" or something along those lines.

Quoted 8 lines

Signed-off-by Abhijeet Dharmapurikar <[email protected]>

Signed-off-by Kiran Gunda <[email protected]>

---
 drivers/spmi/spmi-pmic-arb.c | 68 
++++++++++++++++++++++----------------------
 1 file changed, 34 insertions(+), 34 deletions(-)

Quoted 9 lines

drivers/spmi/spmi-pmic-arb.c +0 -1

index 7f918ea..7201611 100644
@@ -117,7 +117,7 @@ enum pmic_arb_cmd_op_code {
  * @spmic:		SPMI controller object
  * @apid_to_ppid:	in-memory copy of APID -> PPID mapping table.
  * @ver_ops:		version dependent operations.
- * @ppid_to_chan	in-memory copy of PPID -> channel (APID) mapping 

Quoted 3 lines

table.
+ * @ppid_to_apid in-memory copy of PPID -> channel (APID) mapping
table.

PPID->APID? No channel?

Quoted 30 lines

* v2 only.
*/
struct spmi_pmic_arb {

drivers/spmi/spmi-pmic-arb.c +5 -5

@@ -140,9 +140,9 @@ struct spmi_pmic_arb {
 	struct spmi_controller	*spmic;
 	u16			*apid_to_ppid;
 	const struct pmic_arb_ver_ops *ver_ops;
-	u16			*ppid_to_chan;
-	u16			last_channel;
-	u8			*chan_to_owner;
+	u16			*ppid_to_apid;
+	u16			last_apid;
+	u8			*apid_to_owner;
 };
 
 /**

@@ -772,22 +772,22 @@ static int qpnpint_irq_domain_map(struct 
 irq_domain *d,
 	return 0;
 }
 
-static u16 pmic_arb_find_chan(struct spmi_pmic_arb *pa, u16 ppid)
+static u16 pmic_arb_find_apid(struct spmi_pmic_arb *pa, u16 ppid)
 {
 	u32 regval, offset;
-	u16 chan;
+	u16 apid;
 	u16 id;
 

drivers/spmi/spmi-pmic-arb.c static int qpnpint_irq_domain_map(struct irq_domain *d, · line 782

Quoted 2 lines

drivers/spmi/spmi-pmic-arb.c +0 -0

 	/*
 	 * PMIC_ARB_REG_CHNL is a table in HW mapping channel to ppid.

Is this comment still relevant?

Quoted 21 lines

drivers/spmi/spmi-pmic-arb.c +5 -5

-	 * ppid_to_chan is an in-memory invert of that table.
+	 * ppid_to_apid is an in-memory invert of that table.
 	 */
-	for (chan = pa->last_channel; chan < pa->max_periph; chan++) {
+	for (apid = pa->last_apid; apid < pa->max_periph; apid++) {
 		regval = readl_relaxed(pa->cnfg +
-				      SPMI_OWNERSHIP_TABLE_REG(chan));
-		pa->chan_to_owner[chan] = SPMI_OWNERSHIP_PERIPH2OWNER(regval);
+				      SPMI_OWNERSHIP_TABLE_REG(apid));
+		pa->apid_to_owner[apid] = SPMI_OWNERSHIP_PERIPH2OWNER(regval);
 
-		offset = PMIC_ARB_REG_CHNL(chan);
+		offset = PMIC_ARB_REG_CHNL(apid);
 		if (offset >= pa->core_size)
 			break;

@@ -796,15 +796,15 @@ static u16 pmic_arb_find_chan(struct 
 spmi_pmic_arb *pa, u16 ppid)
 			continue;
 

drivers/spmi/spmi-pmic-arb.c static u16 pmic_arb_find_chan(struct spmi_pmic_arb *pa, u16 ppid) · line 799

Quoted 3 lines

drivers/spmi/spmi-pmic-arb.c +1 -1

 		id = (regval >> 8) & PMIC_ARB_PPID_MASK;
-		pa->ppid_to_chan[id] = chan | PMIC_ARB_CHAN_VALID;
+		pa->ppid_to_apid[id] = apid | PMIC_ARB_CHAN_VALID;

Why do we still call the flag PMIC_ARB_CHAN_VALID then? Shouldn't it be PMIC_ARB_APID_VALID?

Quoted 14 lines

drivers/spmi/spmi-pmic-arb.c +3 -3

 		if (id == ppid) {
-			chan |= PMIC_ARB_CHAN_VALID;
+			apid |= PMIC_ARB_CHAN_VALID;
 			break;
 		}
 	}
-	pa->last_channel = chan & ~PMIC_ARB_CHAN_VALID;
+	pa->last_apid = apid & ~PMIC_ARB_CHAN_VALID;
 
-	return chan;
+	return apid;
 }
 

drivers/spmi/spmi-pmic-arb.c +9 -39

-{
-	u16 ppid = spec->slave << 8 | spec->per;
-	u32 *mapping_table = pa->mapping_table;
-	int index = 0, i;
-	u32 data;
-
-	for (i = 0; i < SPMI_MAPPING_TABLE_TREE_DEPTH; ++i) {
-		if (!test_and_set_bit(index, pa->mapping_table_valid))
-			mapping_table[index] = readl_relaxed(pa->cnfg +
-						SPMI_MAPPING_TABLE_REG(index));
-
-		data = mapping_table[index];
-
-		if (ppid & BIT(SPMI_MAPPING_BIT_INDEX(data))) {
-			if (SPMI_MAPPING_BIT_IS_1_FLAG(data)) {
-				index = SPMI_MAPPING_BIT_IS_1_RESULT(data);
-			} else {
-				*apid = SPMI_MAPPING_BIT_IS_1_RESULT(data);
-				return 0;
-			}
-		} else {
-			if (SPMI_MAPPING_BIT_IS_0_FLAG(data)) {
-				index = SPMI_MAPPING_BIT_IS_0_RESULT(data);
-			} else {
-				*apid = SPMI_MAPPING_BIT_IS_0_RESULT(data);
-				return 0;
-			}
-		}
-	}
-
-	return -ENODEV;
-}
-
 static int qpnpint_irq_domain_dt_translate(struct irq_domain *d,
 					   struct device_node *controller,
 					   const u32 *intspec,

@@ -702,7 +668,7 @@ static int qpnpint_irq_domain_dt_translate(struct irq_domain *d,
 {
 	struct spmi_pmic_arb *pa = d->host_data;
 	struct spmi_pmic_arb_irq_spec spec;
-	int err;
+	int rc;
 	u8 apid;
 
 	dev_dbg(&pa->spmic->dev,

@@ -720,11 +686,14 @@ static int qpnpint_irq_domain_dt_translate(struct irq_domain *d,
 	spec.per   = intspec[1];
 	spec.irq   = intspec[2];
 
-	err = search_mapping_table(pa, &spec, &apid);
-	if (err)
-		return err;
-
-	pa->apid_to_ppid[apid] = spec.slave << 8 | spec.per;
+	rc = pa->ver_ops->ppid_to_apid(pa, intspec[0],
+			(intspec[1] << 8), &apid);
+	if (rc < 0) {
+		dev_err(&pa->spmic->dev,
+		"failed to xlate sid = 0x%x, periph = 0x%x, irq = %x rc = %d\n",
+		intspec[0], intspec[1], intspec[2], rc);
+		return rc;
+	}
 
 	/* Keep track of {max,min}_apid for bounding search during interrupt */
 	if (apid > pa->max_apid)