Skip to contentSkip to conversation

: spmi: pmic-arb: Fixup patches

PATCH In review Kiran Gunda 2 participants 8 messages Jul 18, 2017 – Jul 20, 2017

Subsystems (2)
Arm Qualcomm mailing list SPMI
lore
Share & tools
lore

Kiran Gunda

author
#0 lore raw

v3 cover : spmi: pmic-arb: Fixup patches

v3:
* spmi: pmic-arb: rename pa_xx to pmic_arb_xx and other code cleanup replaced "i" with "apid" in pmic_arb_find_apid finction.

* spmi: pmic-arb: use irq_chip callback to set spmi irq wakeup capability Added Stephen's reviewed-by tag.

v2:
* spmi: pmic-arb: remove the read/write access checks Added Stephen's reviewed-by tag.

* spmi: pmic-arb: rename pa_xx to pmic_arb_xx and other code cleanup Removed an extra line from the documentation.

* spmi: pmic-arb: replace the writel_relaxed with __raw_writel Added Stephen's reviewed-by tag.

* spmi: pmic-arb: return the value instead of passing by pointer Added Stephen's reviewed-by tag.
Modified the pmic_arb_offset_* function to return the offset instead of passed by a pointer.

* spmi: pmic-arb: use irq_chip callback to set spmi irq wakeup capability Simplified the qpnpint_irq_set_wake function by using the irq_set_irq_wake function instead of using enable_irq_wake/disable_irq_wake.

v1:

This patch series is to fix-up the comments given by Stephen Boyd on the below 11 pathces. These 11 patches were picked up by GregKH in to his tree and later reviewed by Stephen Boyd. As it is very difficult to revert the changes from his tree, we decided to let these patches go in to Greg's tree and provide the fix-up patches later.

Following are the 11 patches picked by Greg:

https://patchwork.kernel.org/patch/9754503/
https://patchwork.kernel.org/patch/9754501/
https://patchwork.kernel.org/patch/9754559/
https://patchwork.kernel.org/patch/9754511/
https://patchwork.kernel.org/patch/9754515/
https://patchwork.kernel.org/patch/9754553/
https://patchwork.kernel.org/patch/9754555/
https://patchwork.kernel.org/patch/9754521/
https://patchwork.kernel.org/patch/9754551
https://patchwork.kernel.org/patch/9754525
https://patchwork.kernel.org/patch/9754531/

Also this patch series is dependent on the below patch pushed by Stephen Boyd. Please take this series after taking this patch in to the tree.

https://patchwork.kernel.org/patch/9810723/

Kiran Gunda (5):
spmi: pmic-arb: remove the read/write access checks
spmi: pmic-arb: rename pa_xx to pmic_arb_xx and other code cleanup
spmi: pmic-arb: replace the writel_relaxed with __raw_writel
spmi: pmic-arb: return the value instead of passing by pointer
spmi: pmic-arb: use irq_chip callback to set spmi irq wakeup
capability

 drivers/spmi/spmi-pmic-arb.c | 563 +++++++++++++++++++------------------------
 1 file changed, 250 insertions(+), 313 deletions(-)
Signature · 2 lines
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
 a Linux Foundation Collaborative Project
author
#1 lore raw

PATCH v3 1/5 spmi: pmic-arb: remove the read/write access checks

The access mode checks for peripheral ownership for read/write permissions should not be required. Every peripheral enabled for this master is expected to have a read/write permissions. If there is any such invalid access due to wrong configuration in boot loader or device tree files, then it should be fixed in those locations. Hence, remove the access mode checks from the driver.

Signed-off-by Kiran Gunda <[email protected]>
Reviewed-by Stephen Boyd <[email protected]>
---
 drivers/spmi/spmi-pmic-arb.c | 75 +++-----------------------------------------
 1 file changed, 5 insertions(+), 70 deletions(-)
drivers/spmi/spmi-pmic-arb.c +5 -70
index f4b7a98..7531519 100644
@@ -128,7 +128,6 @@ struct apid_data {
  * @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.
  * @mapping_table:	in-memory copy of PPID -> APID mapping table.
  * @domain:		irq domain object for PMIC IRQ domain
  * @spmic:		SPMI controller object

@@ -148,7 +147,6 @@ struct spmi_pmic_arb {
 	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;

@@ -164,7 +162,6 @@ struct spmi_pmic_arb {
  *
  * @ver_str:		version string.
  * @ppid_to_apid:	finds the apid for a given ppid.
- * @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.

@@ -183,8 +180,6 @@ struct pmic_arb_ver_ops {
 	const char *ver_str;
 	int (*ppid_to_apid)(struct spmi_pmic_arb *pa, u8 sid, u16 addr,
 			u16 *apid);
-	int (*mode)(struct spmi_pmic_arb *dev, u8 sid, u16 addr,
-			mode_t *mode);
 	/* spmi commands (read_cmd, write_cmd, cmd) functionality */
 	int (*offset)(struct spmi_pmic_arb *dev, u8 sid, u16 addr,
 		      u32 *offset);

@@ -340,23 +335,11 @@ static int pmic_arb_read_cmd(struct spmi_controller *ctrl, u8 opc, u8 sid,
 	u32 cmd;
 	int rc;
 	u32 offset;
-	mode_t mode;
 
 	rc = pa->ver_ops->offset(pa, sid, addr, &offset);
 	if (rc)
 		return rc;
 
-	rc = pa->ver_ops->mode(pa, sid, addr, &mode);
-	if (rc)
-		return rc;
-
-	if (!(mode & S_IRUSR)) {
-		dev_err(&pa->spmic->dev,
-			"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",

@@ -402,23 +385,11 @@ static int pmic_arb_write_cmd(struct spmi_controller *ctrl, u8 opc, u8 sid,
 	u32 cmd;
 	int rc;
 	u32 offset;
-	mode_t mode;
 
 	rc = pa->ver_ops->offset(pa, sid, addr, &offset);
 	if (rc)
 		return rc;
 
-	rc = pa->ver_ops->mode(pa, sid, addr, &mode);
-	if (rc)
-		return rc;
-
-	if (!(mode & S_IWUSR)) {
-		dev_err(&pa->spmic->dev,
-			"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",

@@ -785,13 +756,6 @@ static int qpnpint_irq_domain_map(struct irq_domain *d,
 	return -ENODEV;
 }
 
-static int
-pmic_arb_mode_v1_v3(struct spmi_pmic_arb *pa, u8 sid, u16 addr, mode_t *mode)
-{
-	*mode = S_IRUSR | S_IWUSR;
-	return 0;
-}
-
 /* v1 offset per ee */
 static int
 pmic_arb_offset_v1(struct spmi_pmic_arb *pa, u8 sid, u16 addr, u32 *offset)

@@ -810,15 +774,15 @@ static u16 pmic_arb_find_apid(struct spmi_pmic_arb *pa, u16 ppid)
 	 * PMIC_ARB_REG_CHNL is a table in HW mapping channel to ppid.
 	 * ppid_to_apid is an in-memory invert of that table.
 	 */
-	for (apid = pa->last_apid; apid < pa->max_periph; apid++) {
-		regval = readl_relaxed(pa->cnfg +
-				      SPMI_OWNERSHIP_TABLE_REG(apid));
-		pa->apid_data[apid].owner = SPMI_OWNERSHIP_PERIPH2OWNER(regval);
-
+	for (apid = pa->last_apid; ; apid++) {
 		offset = PMIC_ARB_REG_CHNL(apid);
 		if (offset >= pa->core_size)
 			break;
 
+		regval = readl_relaxed(pa->cnfg +
+				      SPMI_OWNERSHIP_TABLE_REG(apid));
+		pa->apid_data[apid].owner = SPMI_OWNERSHIP_PERIPH2OWNER(regval);
+
 		regval = readl_relaxed(pa->core + offset);
 		if (!regval)
 			continue;

@@ -853,24 +817,6 @@ static u16 pmic_arb_find_apid(struct spmi_pmic_arb *pa, u16 ppid)
 	return 0;
 }
 
-static int
-pmic_arb_mode_v2(struct spmi_pmic_arb *pa, u8 sid, u16 addr, mode_t *mode)
-{
-	u16 apid;
-	int rc;
-
-	rc = pmic_arb_ppid_to_apid_v2(pa, sid, addr, &apid);
-	if (rc < 0)
-		return rc;
-
-	*mode = 0;
-	*mode |= S_IRUSR;
-
-	if (pa->ee == pa->apid_data[apid].owner)
-		*mode |= S_IWUSR;
-	return 0;
-}
-
 /* v2 offset per ppid and per ee */
 static int
 pmic_arb_offset_v2(struct spmi_pmic_arb *pa, u8 sid, u16 addr, u32 *offset)

@@ -944,7 +890,6 @@ static u32 pmic_arb_irq_clear_v2(u16 n)
 static const struct pmic_arb_ver_ops pmic_arb_v1 = {
 	.ver_str		= "v1",
 	.ppid_to_apid		= pmic_arb_ppid_to_apid_v1,
-	.mode			= pmic_arb_mode_v1_v3,
 	.non_data_cmd		= pmic_arb_non_data_cmd_v1,
 	.offset			= pmic_arb_offset_v1,
 	.fmt_cmd		= pmic_arb_fmt_cmd_v1,

@@ -957,7 +902,6 @@ static u32 pmic_arb_irq_clear_v2(u16 n)
 static const struct pmic_arb_ver_ops pmic_arb_v2 = {
 	.ver_str		= "v2",
 	.ppid_to_apid		= pmic_arb_ppid_to_apid_v2,
-	.mode			= pmic_arb_mode_v2,
 	.non_data_cmd		= pmic_arb_non_data_cmd_v2,
 	.offset			= pmic_arb_offset_v2,
 	.fmt_cmd		= pmic_arb_fmt_cmd_v2,

@@ -970,7 +914,6 @@ static u32 pmic_arb_irq_clear_v2(u16 n)
 static const struct pmic_arb_ver_ops pmic_arb_v3 = {
 	.ver_str		= "v3",
 	.ppid_to_apid		= pmic_arb_ppid_to_apid_v2,
-	.mode			= pmic_arb_mode_v1_v3,
 	.non_data_cmd		= pmic_arb_non_data_cmd_v2,
 	.offset			= pmic_arb_offset_v2,
 	.fmt_cmd		= pmic_arb_fmt_cmd_v2,

@@ -1003,11 +946,6 @@ static int spmi_pmic_arb_probe(struct platform_device *pdev)
 
 	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");
-		err = -EINVAL;
-		goto err_put_ctrl;
-	}
 
 	core = devm_ioremap_resource(&ctrl->dev, res);
 	if (IS_ERR(core)) {

@@ -1036,9 +974,6 @@ static int spmi_pmic_arb_probe(struct platform_device *pdev)
 		else
 			pa->ver_ops = &pmic_arb_v3;
 
-		/* 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);
Signature · 2 lines
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project
author
#2 lore raw

PATCH v3 2/5 spmi: pmic-arb: rename pa_xx to pmic_arb_xx and other code cleanup

In reply to Kiran Gunda

This patch cleans up the following.

- Rename the "pa" to "pmic_arb".
- Rename the spmi_pmic_arb *dev to spmi_pmic_arb *pmic_arb.
- Rename the pa_{read,write}_data() functions to pmic_arb_{read,write}_data().
- Rename channel to APID.
- Rename the HWIRQ_*() macros to hwirq_to_*().
- Clean up qpnpint_irq_set_type() and pmic_arb_find_apid() functions.

Signed-off-by Kiran Gunda <[email protected]>
---
 drivers/spmi/spmi-pmic-arb.c | 453 +++++++++++++++++++++----------------------
 1 file changed, 226 insertions(+), 227 deletions(-)
drivers/spmi/spmi-pmic-arb.c +226 -227
index 7531519..91b068b 100644
@@ -39,7 +39,7 @@
 #define PMIC_ARB_WDATA1			0x14
 #define PMIC_ARB_RDATA0			0x18
 #define PMIC_ARB_RDATA1			0x1C
-#define PMIC_ARB_REG_CHNL(N)		(0x800 + 0x4 * (N))
+#define PMIC_ARB_REG_APID(N)		(0x800 + 0x4 * (N))
 
 /* Mapping Table */
 #define SPMI_MAPPING_TABLE_REG(N)	(0x0B00 + (4 * (N)))

@@ -51,7 +51,7 @@
 
 #define SPMI_MAPPING_TABLE_TREE_DEPTH	16	/* Maximum of 16-bits */
 #define PMIC_ARB_MAX_PPID		BIT(12) /* PPID is 12bit */
-#define PMIC_ARB_CHAN_VALID		BIT(15)
+#define PMIC_ARB_APID_VALID		BIT(15)
 
 /* Ownership Table */
 #define SPMI_OWNERSHIP_TABLE_REG(N)	(0x0700 + (4 * (N)))

@@ -97,16 +97,16 @@ enum pmic_arb_cmd_op_code {
 /* interrupt enable bit */
 #define SPMI_PIC_ACC_ENABLE_BIT		BIT(0)
 
-#define HWIRQ(slave_id, periph_id, irq_id, apid) \
+#define spec_to_hwirq(slave_id, periph_id, irq_id, apid) \
 	((((slave_id) & 0xF)   << 28) | \
 	(((periph_id) & 0xFF)  << 20) | \
 	(((irq_id)    & 0x7)   << 16) | \
 	(((apid)      & 0x1FF) << 0))
 
-#define HWIRQ_SID(hwirq)  (((hwirq) >> 28) & 0xF)
-#define HWIRQ_PER(hwirq)  (((hwirq) >> 20) & 0xFF)
-#define HWIRQ_IRQ(hwirq)  (((hwirq) >> 16) & 0x7)
-#define HWIRQ_APID(hwirq) (((hwirq) >> 0)  & 0x1FF)
+#define hwirq_to_sid(hwirq)  (((hwirq) >> 28) & 0xF)
+#define hwirq_to_per(hwirq)  (((hwirq) >> 20) & 0xFF)
+#define hwirq_to_irq(hwirq)  (((hwirq) >> 16) & 0x7)
+#define hwirq_to_apid(hwirq) (((hwirq) >> 0)  & 0x1FF)
 
 struct pmic_arb_ver_ops;
 

@@ -132,7 +132,7 @@ struct apid_data {
  * @domain:		irq domain object for PMIC IRQ domain
  * @spmic:		SPMI controller object
  * @ver_ops:		version dependent operations.
- * @ppid_to_apid	in-memory copy of PPID -> channel (APID) mapping table.
+ * @ppid_to_apid	in-memory copy of PPID -> APID mapping table.
  */
 struct spmi_pmic_arb {
 	void __iomem		*rd_base;

@@ -178,10 +178,10 @@ struct spmi_pmic_arb {
  */
 struct pmic_arb_ver_ops {
 	const char *ver_str;
-	int (*ppid_to_apid)(struct spmi_pmic_arb *pa, u8 sid, u16 addr,
+	int (*ppid_to_apid)(struct spmi_pmic_arb *pmic_arb, u8 sid, u16 addr,
 			u16 *apid);
 	/* spmi commands (read_cmd, write_cmd, cmd) functionality */
-	int (*offset)(struct spmi_pmic_arb *dev, u8 sid, u16 addr,
+	int (*offset)(struct spmi_pmic_arb *pmic_arb, 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);

@@ -192,56 +192,57 @@ struct pmic_arb_ver_ops {
 	u32 (*irq_clear)(u16 n);
 };
 
-static inline void pmic_arb_base_write(struct spmi_pmic_arb *pa,
+static inline void pmic_arb_base_write(struct spmi_pmic_arb *pmic_arb,
 				       u32 offset, u32 val)
 {
-	writel_relaxed(val, pa->wr_base + offset);
+	writel_relaxed(val, pmic_arb->wr_base + offset);
 }
 
-static inline void pmic_arb_set_rd_cmd(struct spmi_pmic_arb *pa,
+static inline void pmic_arb_set_rd_cmd(struct spmi_pmic_arb *pmic_arb,
 				       u32 offset, u32 val)
 {
-	writel_relaxed(val, pa->rd_base + offset);
+	writel_relaxed(val, pmic_arb->rd_base + offset);
 }
 
 /**
- * pa_read_data: reads pmic-arb's register and copy 1..4 bytes to buf
+ * pmic_arb_read_data: reads pmic-arb's register and copy 1..4 bytes to buf
  * @bc:		byte count -1. range: 0..3
  * @reg:	register's address
  * @buf:	output parameter, length must be bc + 1
  */
-static void pa_read_data(struct spmi_pmic_arb *pa, u8 *buf, u32 reg, u8 bc)
+static void
+pmic_arb_read_data(struct spmi_pmic_arb *pmic_arb, u8 *buf, u32 reg, u8 bc)
 {
-	u32 data = __raw_readl(pa->rd_base + reg);
+	u32 data = __raw_readl(pmic_arb->rd_base + reg);
 
 	memcpy(buf, &data, (bc & 3) + 1);
 }
 
 /**
- * pa_write_data: write 1..4 bytes from buf to pmic-arb's register
+ * pmic_arb_write_data: write 1..4 bytes from buf to pmic-arb's register
  * @bc:		byte-count -1. range: 0..3.
  * @reg:	register's address.
  * @buf:	buffer to write. length must be bc + 1.
  */
-static void
-pa_write_data(struct spmi_pmic_arb *pa, const u8 *buf, u32 reg, u8 bc)
+static void pmic_arb_write_data(struct spmi_pmic_arb *pmic_arb, const u8 *buf,
+				u32 reg, u8 bc)
 {
 	u32 data = 0;
 
 	memcpy(&data, buf, (bc & 3) + 1);
-	pmic_arb_base_write(pa, reg, data);
+	pmic_arb_base_write(pmic_arb, reg, data);
 }
 
 static int pmic_arb_wait_for_done(struct spmi_controller *ctrl,
 				  void __iomem *base, u8 sid, u16 addr)
 {
-	struct spmi_pmic_arb *pa = spmi_controller_get_drvdata(ctrl);
+	struct spmi_pmic_arb *pmic_arb = spmi_controller_get_drvdata(ctrl);
 	u32 status = 0;
 	u32 timeout = PMIC_ARB_TIMEOUT_US;
 	u32 offset;
 	int rc;
 
-	rc = pa->ver_ops->offset(pa, sid, addr, &offset);
+	rc = pmic_arb->ver_ops->offset(pmic_arb, sid, addr, &offset);
 	if (rc)
 		return rc;
 

@@ -252,22 +253,19 @@ static int pmic_arb_wait_for_done(struct spmi_controller *ctrl,
 
 		if (status & PMIC_ARB_STATUS_DONE) {
 			if (status & PMIC_ARB_STATUS_DENIED) {
-				dev_err(&ctrl->dev,
-					"%s: transaction denied (0x%x)\n",
+				dev_err(&ctrl->dev, "%s: transaction denied (0x%x)\n",
 					__func__, status);
 				return -EPERM;
 			}
 
 			if (status & PMIC_ARB_STATUS_FAILURE) {
-				dev_err(&ctrl->dev,
-					"%s: transaction failed (0x%x)\n",
+				dev_err(&ctrl->dev, "%s: transaction failed (0x%x)\n",
 					__func__, status);
 				return -EIO;
 			}
 
 			if (status & PMIC_ARB_STATUS_DROPPED) {
-				dev_err(&ctrl->dev,
-					"%s: transaction dropped (0x%x)\n",
+				dev_err(&ctrl->dev, "%s: transaction dropped (0x%x)\n",
 					__func__, status);
 				return -EIO;
 			}

@@ -277,8 +275,7 @@ static int pmic_arb_wait_for_done(struct spmi_controller *ctrl,
 		udelay(1);
 	}
 
-	dev_err(&ctrl->dev,
-		"%s: timeout, status 0x%x\n",
+	dev_err(&ctrl->dev, "%s: timeout, status 0x%x\n",
 		__func__, status);
 	return -ETIMEDOUT;
 }

@@ -286,22 +283,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 *pa = spmi_controller_get_drvdata(ctrl);
+	struct spmi_pmic_arb *pmic_arb = spmi_controller_get_drvdata(ctrl);
 	unsigned long flags;
 	u32 cmd;
 	int rc;
 	u32 offset;
 
-	rc = pa->ver_ops->offset(pa, sid, 0, &offset);
+	rc = pmic_arb->ver_ops->offset(pmic_arb, sid, 0, &offset);
 	if (rc)
 		return rc;
 
 	cmd = ((opc | 0x40) << 27) | ((sid & 0xf) << 20);
 
-	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);
+	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);
 
 	return rc;
 }

@@ -315,7 +312,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 *pa = spmi_controller_get_drvdata(ctrl);
+	struct spmi_pmic_arb *pmic_arb = spmi_controller_get_drvdata(ctrl);
 
 	dev_dbg(&ctrl->dev, "cmd op:0x%x sid:%d\n", opc, sid);
 

@@ -323,26 +320,25 @@ static int pmic_arb_cmd(struct spmi_controller *ctrl, u8 opc, u8 sid)
 	if (opc < SPMI_CMD_RESET || opc > SPMI_CMD_WAKEUP)
 		return -EINVAL;
 
-	return pa->ver_ops->non_data_cmd(ctrl, opc, sid);
+	return pmic_arb->ver_ops->non_data_cmd(ctrl, opc, sid);
 }
 
 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 *pa = spmi_controller_get_drvdata(ctrl);
+	struct spmi_pmic_arb *pmic_arb = spmi_controller_get_drvdata(ctrl);
 	unsigned long flags;
 	u8 bc = len - 1;
 	u32 cmd;
 	int rc;
 	u32 offset;
 
-	rc = pa->ver_ops->offset(pa, sid, addr, &offset);
+	rc = pmic_arb->ver_ops->offset(pmic_arb, sid, addr, &offset);
 	if (rc)
 		return rc;
 
 	if (bc >= PMIC_ARB_MAX_TRANS_BYTES) {
-		dev_err(&ctrl->dev,
-			"pmic-arb supports 1..%d bytes per trans, but:%zu requested",
+		dev_err(&ctrl->dev, "pmic-arb supports 1..%d bytes per trans, but:%zu requested",
 			PMIC_ARB_MAX_TRANS_BYTES, len);
 		return  -EINVAL;
 	}

@@ -357,42 +353,42 @@ static int pmic_arb_read_cmd(struct spmi_controller *ctrl, u8 opc, u8 sid,
 	else
 		return -EINVAL;
 
-	cmd = pa->ver_ops->fmt_cmd(opc, sid, addr, bc);
+	cmd = pmic_arb->ver_ops->fmt_cmd(opc, sid, addr, bc);
 
-	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);
+	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);
 	if (rc)
 		goto done;
 
-	pa_read_data(pa, buf, offset + PMIC_ARB_RDATA0,
+	pmic_arb_read_data(pmic_arb, buf, offset + PMIC_ARB_RDATA0,
 		     min_t(u8, bc, 3));
 
 	if (bc > 3)
-		pa_read_data(pa, buf + 4, offset + PMIC_ARB_RDATA1, bc - 4);
+		pmic_arb_read_data(pmic_arb, buf + 4, offset + PMIC_ARB_RDATA1,
+					bc - 4);
 
 done:
-	raw_spin_unlock_irqrestore(&pa->lock, flags);
+	raw_spin_unlock_irqrestore(&pmic_arb->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 *pa = spmi_controller_get_drvdata(ctrl);
+	struct spmi_pmic_arb *pmic_arb = spmi_controller_get_drvdata(ctrl);
 	unsigned long flags;
 	u8 bc = len - 1;
 	u32 cmd;
 	int rc;
 	u32 offset;
 
-	rc = pa->ver_ops->offset(pa, sid, addr, &offset);
+	rc = pmic_arb->ver_ops->offset(pmic_arb, sid, addr, &offset);
 	if (rc)
 		return rc;
 
 	if (bc >= PMIC_ARB_MAX_TRANS_BYTES) {
-		dev_err(&ctrl->dev,
-			"pmic-arb supports 1..%d bytes per trans, but:%zu requested",
+		dev_err(&ctrl->dev, "pmic-arb supports 1..%d bytes per trans, but:%zu requested",
 			PMIC_ARB_MAX_TRANS_BYTES, len);
 		return  -EINVAL;
 	}

@@ -409,18 +405,20 @@ static int pmic_arb_write_cmd(struct spmi_controller *ctrl, u8 opc, u8 sid,
 	else
 		return -EINVAL;
 
-	cmd = pa->ver_ops->fmt_cmd(opc, sid, addr, bc);
+	cmd = pmic_arb->ver_ops->fmt_cmd(opc, sid, addr, bc);
 
 	/* Write data to FIFOs */
-	raw_spin_lock_irqsave(&pa->lock, flags);
-	pa_write_data(pa, buf, offset + PMIC_ARB_WDATA0, min_t(u8, bc, 3));
+	raw_spin_lock_irqsave(&pmic_arb->lock, flags);
+	pmic_arb_write_data(pmic_arb, buf, offset + PMIC_ARB_WDATA0,
+				min_t(u8, bc, 3));
 	if (bc > 3)
-		pa_write_data(pa, buf + 4, offset + PMIC_ARB_WDATA1, bc - 4);
+		pmic_arb_write_data(pmic_arb, buf + 4, offset + PMIC_ARB_WDATA1,
+					bc - 4);
 
 	/* Start the transaction */
-	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);
+	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);
 
 	return rc;
 }

@@ -446,67 +444,66 @@ 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 *pa = irq_data_get_irq_chip_data(d);
-	u8 sid = HWIRQ_SID(d->hwirq);
-	u8 per = HWIRQ_PER(d->hwirq);
+	struct spmi_pmic_arb *pmic_arb = irq_data_get_irq_chip_data(d);
+	u8 sid = hwirq_to_sid(d->hwirq);
+	u8 per = hwirq_to_per(d->hwirq);
 
-	if (pmic_arb_write_cmd(pa->spmic, SPMI_CMD_EXT_WRITEL, sid,
+	if (pmic_arb_write_cmd(pmic_arb->spmic, SPMI_CMD_EXT_WRITEL, sid,
 			       (per << 8) + reg, buf, len))
-		dev_err_ratelimited(&pa->spmic->dev,
-				"failed irqchip transaction on %x\n",
+		dev_err_ratelimited(&pmic_arb->spmic->dev, "failed irqchip transaction on %x\n",
 				    d->irq);
 }
 
 static void qpnpint_spmi_read(struct irq_data *d, u8 reg, void *buf, size_t len)
 {
-	struct spmi_pmic_arb *pa = irq_data_get_irq_chip_data(d);
-	u8 sid = HWIRQ_SID(d->hwirq);
-	u8 per = HWIRQ_PER(d->hwirq);
+	struct spmi_pmic_arb *pmic_arb = irq_data_get_irq_chip_data(d);
+	u8 sid = hwirq_to_sid(d->hwirq);
+	u8 per = hwirq_to_per(d->hwirq);
 
-	if (pmic_arb_read_cmd(pa->spmic, SPMI_CMD_EXT_READL, sid,
+	if (pmic_arb_read_cmd(pmic_arb->spmic, SPMI_CMD_EXT_READL, sid,
 			      (per << 8) + reg, buf, len))
-		dev_err_ratelimited(&pa->spmic->dev,
-				"failed irqchip transaction on %x\n",
+		dev_err_ratelimited(&pmic_arb->spmic->dev, "failed irqchip transaction on %x\n",
 				    d->irq);
 }
 
-static void cleanup_irq(struct spmi_pmic_arb *pa, u16 apid, int id)
+static void cleanup_irq(struct spmi_pmic_arb *pmic_arb, u16 apid, int id)
 {
-	u16 ppid = pa->apid_data[apid].ppid;
+	const struct pmic_arb_ver_ops *ver_ops = pmic_arb->ver_ops;
+	u16 ppid = pmic_arb->apid_data[apid].ppid;
 	u8 sid = ppid >> 8;
 	u8 per = ppid & 0xFF;
 	u8 irq_mask = BIT(id);
 
-	writel_relaxed(irq_mask, pa->intr + pa->ver_ops->irq_clear(apid));
+	writel_relaxed(irq_mask, pmic_arb->intr + ver_ops->irq_clear(apid));
 
-	if (pmic_arb_write_cmd(pa->spmic, SPMI_CMD_EXT_WRITEL, sid,
+	if (pmic_arb_write_cmd(pmic_arb->spmic, SPMI_CMD_EXT_WRITEL, sid,
 			(per << 8) + QPNPINT_REG_LATCHED_CLR, &irq_mask, 1))
-		dev_err_ratelimited(&pa->spmic->dev,
-				"failed to ack irq_mask = 0x%x for ppid = %x\n",
+		dev_err_ratelimited(&pmic_arb->spmic->dev, "failed to ack irq_mask = 0x%x for ppid = %x\n",
 				irq_mask, ppid);
 
-	if (pmic_arb_write_cmd(pa->spmic, SPMI_CMD_EXT_WRITEL, sid,
+	if (pmic_arb_write_cmd(pmic_arb->spmic, SPMI_CMD_EXT_WRITEL, sid,
 			       (per << 8) + QPNPINT_REG_EN_CLR, &irq_mask, 1))
-		dev_err_ratelimited(&pa->spmic->dev,
-				"failed to ack irq_mask = 0x%x for ppid = %x\n",
+		dev_err_ratelimited(&pmic_arb->spmic->dev, "failed to ack irq_mask = 0x%x for ppid = %x\n",
 				irq_mask, ppid);
 }
 
-static void periph_interrupt(struct spmi_pmic_arb *pa, u16 apid)
+static void periph_interrupt(struct spmi_pmic_arb *pmic_arb, u16 apid)
 {
+	const struct pmic_arb_ver_ops *ver_ops = pmic_arb->ver_ops;
+	u8 sid = (pmic_arb->apid_data[apid].ppid >> 8) & 0xF;
+	u8 per = pmic_arb->apid_data[apid].ppid & 0xFF;
 	unsigned int irq;
 	u32 status;
 	int id;
-	u8 sid = (pa->apid_data[apid].ppid >> 8) & 0xF;
-	u8 per = pa->apid_data[apid].ppid & 0xFF;
 
-	status = readl_relaxed(pa->intr + pa->ver_ops->irq_status(apid));
+	status = readl_relaxed(pmic_arb->intr + ver_ops->irq_status(apid));
 	while (status) {
 		id = ffs(status) - 1;
 		status &= ~BIT(id);
-		irq = irq_find_mapping(pa->domain, HWIRQ(sid, per, id, apid));
+		irq = irq_find_mapping(pmic_arb->domain,
+					spec_to_hwirq(sid, per, id, apid));
 		if (irq == 0) {
-			cleanup_irq(pa, apid, id);
+			cleanup_irq(pmic_arb, apid, id);
 			continue;
 		}
 		generic_handle_irq(irq);

@@ -515,11 +512,12 @@ static void periph_interrupt(struct spmi_pmic_arb *pa, u16 apid)
 
 static void pmic_arb_chained_irq(struct irq_desc *desc)
 {
-	struct spmi_pmic_arb *pa = irq_desc_get_handler_data(desc);
+	struct spmi_pmic_arb *pmic_arb = irq_desc_get_handler_data(desc);
+	const struct pmic_arb_ver_ops *ver_ops = pmic_arb->ver_ops;
 	struct irq_chip *chip = irq_desc_get_chip(desc);
-	void __iomem *intr = pa->intr;
-	int first = pa->min_apid >> 5;
-	int last = pa->max_apid >> 5;
+	void __iomem *intr = pmic_arb->intr;
+	int first = pmic_arb->min_apid >> 5;
+	int last = pmic_arb->max_apid >> 5;
 	u32 status, enable;
 	int i, id, apid;
 

@@ -527,15 +525,15 @@ static void pmic_arb_chained_irq(struct irq_desc *desc)
 
 	for (i = first; i <= last; ++i) {
 		status = readl_relaxed(intr +
-				      pa->ver_ops->owner_acc_status(pa->ee, i));
+				    ver_ops->owner_acc_status(pmic_arb->ee, i));
 		while (status) {
 			id = ffs(status) - 1;
 			status &= ~BIT(id);
 			apid = id + i * 32;
 			enable = readl_relaxed(intr +
-					pa->ver_ops->acc_enable(apid));
+					pmic_arb->ver_ops->acc_enable(apid));
 			if (enable & SPMI_PIC_ACC_ENABLE_BIT)
-				periph_interrupt(pa, apid);
+				periph_interrupt(pmic_arb, apid);
 		}
 	}
 

@@ -544,12 +542,13 @@ static void pmic_arb_chained_irq(struct irq_desc *desc)
 
 static void qpnpint_irq_ack(struct irq_data *d)
 {
-	struct spmi_pmic_arb *pa = irq_data_get_irq_chip_data(d);
-	u8 irq = HWIRQ_IRQ(d->hwirq);
-	u16 apid = HWIRQ_APID(d->hwirq);
+	struct spmi_pmic_arb *pmic_arb = irq_data_get_irq_chip_data(d);
+	const struct pmic_arb_ver_ops *ver_ops = pmic_arb->ver_ops;
+	u8 irq = hwirq_to_irq(d->hwirq);
+	u16 apid = hwirq_to_apid(d->hwirq);
 	u8 data;
 
-	writel_relaxed(BIT(irq), pa->intr + pa->ver_ops->irq_clear(apid));
+	writel_relaxed(BIT(irq), pmic_arb->intr + ver_ops->irq_clear(apid));
 
 	data = BIT(irq);
 	qpnpint_spmi_write(d, QPNPINT_REG_LATCHED_CLR, &data, 1);

@@ -557,7 +556,7 @@ static void qpnpint_irq_ack(struct irq_data *d)
 
 static void qpnpint_irq_mask(struct irq_data *d)
 {
-	u8 irq = HWIRQ_IRQ(d->hwirq);
+	u8 irq = hwirq_to_irq(d->hwirq);
 	u8 data = BIT(irq);
 
 	qpnpint_spmi_write(d, QPNPINT_REG_EN_CLR, &data, 1);

@@ -565,13 +564,13 @@ static void qpnpint_irq_mask(struct irq_data *d)
 
 static void qpnpint_irq_unmask(struct irq_data *d)
 {
-	struct spmi_pmic_arb *pa = irq_data_get_irq_chip_data(d);
-	u8 irq = HWIRQ_IRQ(d->hwirq);
-	u16 apid = HWIRQ_APID(d->hwirq);
+	struct spmi_pmic_arb *pmic_arb = irq_data_get_irq_chip_data(d);
+	u8 irq = hwirq_to_irq(d->hwirq);
+	u16 apid = hwirq_to_apid(d->hwirq);
 	u8 buf[2];
 
 	writel_relaxed(SPMI_PIC_ACC_ENABLE_BIT,
-		pa->intr + pa->ver_ops->acc_enable(apid));
+		pmic_arb->intr + pmic_arb->ver_ops->acc_enable(apid));
 
 	qpnpint_spmi_read(d, QPNPINT_REG_EN_SET, &buf[0], 1);
 	if (!(buf[0] & BIT(irq))) {

@@ -589,35 +588,35 @@ static void qpnpint_irq_unmask(struct irq_data *d)
 static int qpnpint_irq_set_type(struct irq_data *d, unsigned int flow_type)
 {
 	struct spmi_pmic_arb_qpnpint_type type;
-	u8 irq = HWIRQ_IRQ(d->hwirq);
-	u8 bit_mask_irq = BIT(irq);
+	u8 irq = hwirq_to_irq(d->hwirq);
 
 	qpnpint_spmi_read(d, QPNPINT_REG_SET_TYPE, &type, sizeof(type));
 
 	if (flow_type & (IRQF_TRIGGER_RISING | IRQF_TRIGGER_FALLING)) {
-		type.type |= bit_mask_irq;
+		type.type |= BIT(irq);
 		if (flow_type & IRQF_TRIGGER_RISING)
-			type.polarity_high |= bit_mask_irq;
+			type.polarity_high |= BIT(irq);
 		if (flow_type & IRQF_TRIGGER_FALLING)
-			type.polarity_low  |= bit_mask_irq;
+			type.polarity_low  |= BIT(irq);
+
+		qpnpint_spmi_write(d, QPNPINT_REG_SET_TYPE, &type,
+					sizeof(type));
+		irq_set_handler_locked(d, handle_edge_irq);
 	} else {
 		if ((flow_type & (IRQF_TRIGGER_HIGH)) &&
 		    (flow_type & (IRQF_TRIGGER_LOW)))
 			return -EINVAL;
 
-		type.type &= ~bit_mask_irq; /* level trig */
+		type.type &= ~BIT(irq); /* level trig */
 		if (flow_type & IRQF_TRIGGER_HIGH)
-			type.polarity_high |= bit_mask_irq;
+			type.polarity_high |= BIT(irq);
 		else
-			type.polarity_low  |= bit_mask_irq;
-	}
-
-	qpnpint_spmi_write(d, QPNPINT_REG_SET_TYPE, &type, sizeof(type));
+			type.polarity_low  |= BIT(irq);
 
-	if (flow_type & IRQ_TYPE_EDGE_BOTH)
-		irq_set_handler_locked(d, handle_edge_irq);
-	else
+		qpnpint_spmi_write(d, QPNPINT_REG_SET_TYPE, &type,
+					sizeof(type));
 		irq_set_handler_locked(d, handle_level_irq);
+	}
 
 	return 0;
 }

@@ -626,7 +625,7 @@ static int qpnpint_get_irqchip_state(struct irq_data *d,
 				     enum irqchip_irq_state which,
 				     bool *state)
 {
-	u8 irq = HWIRQ_IRQ(d->hwirq);
+	u8 irq = hwirq_to_irq(d->hwirq);
 	u8 status = 0;
 
 	if (which != IRQCHIP_STATE_LINE_LEVEL)

@@ -656,12 +655,11 @@ static int qpnpint_irq_domain_dt_translate(struct irq_domain *d,
 					   unsigned long *out_hwirq,
 					   unsigned int *out_type)
 {
-	struct spmi_pmic_arb *pa = d->host_data;
+	struct spmi_pmic_arb *pmic_arb = d->host_data;
 	int rc;
 	u16 apid;
 
-	dev_dbg(&pa->spmic->dev,
-		"intspec[0] 0x%1x intspec[1] 0x%02x intspec[2] 0x%02x\n",
+	dev_dbg(&pmic_arb->spmic->dev, "intspec[0] 0x%1x intspec[1] 0x%02x intspec[2] 0x%02x\n",
 		intspec[0], intspec[1], intspec[2]);
 
 	if (irq_domain_get_of_node(d) != controller)

@@ -671,25 +669,24 @@ static int qpnpint_irq_domain_dt_translate(struct irq_domain *d,
 	if (intspec[0] > 0xF || intspec[1] > 0xFF || intspec[2] > 0x7)
 		return -EINVAL;
 
-	rc = pa->ver_ops->ppid_to_apid(pa, intspec[0],
-			(intspec[1] << 8), &apid);
+	rc = pmic_arb->ver_ops->ppid_to_apid(pmic_arb, 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",
+		dev_err(&pmic_arb->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)
-		pa->max_apid = apid;
-	if (apid < pa->min_apid)
-		pa->min_apid = apid;
+	if (apid > pmic_arb->max_apid)
+		pmic_arb->max_apid = apid;
+	if (apid < pmic_arb->min_apid)
+		pmic_arb->min_apid = apid;
 
-	*out_hwirq = HWIRQ(intspec[0], intspec[1], intspec[2], apid);
+	*out_hwirq = spec_to_hwirq(intspec[0], intspec[1], intspec[2], apid);
 	*out_type  = intspec[3] & IRQ_TYPE_SENSE_MASK;
 
-	dev_dbg(&pa->spmic->dev, "out_hwirq = %lu\n", *out_hwirq);
+	dev_dbg(&pmic_arb->spmic->dev, "out_hwirq = %lu\n", *out_hwirq);
 
 	return 0;
 }

@@ -698,9 +695,9 @@ static int qpnpint_irq_domain_map(struct irq_domain *d,
 				  unsigned int virq,
 				  irq_hw_number_t hwirq)
 {
-	struct spmi_pmic_arb *pa = d->host_data;
+	struct spmi_pmic_arb *pmic_arb = d->host_data;
 
-	dev_dbg(&pa->spmic->dev, "virq = %u, hwirq = %lu\n", virq, hwirq);
+	dev_dbg(&pmic_arb->spmic->dev, "virq = %u, hwirq = %lu\n", virq, hwirq);
 
 	irq_set_chip_and_handler(virq, &pmic_arb_irqchip, handle_level_irq);
 	irq_set_chip_data(virq, d->host_data);

@@ -708,24 +705,24 @@ static int qpnpint_irq_domain_map(struct irq_domain *d,
 	return 0;
 }
 
-static int
-pmic_arb_ppid_to_apid_v1(struct spmi_pmic_arb *pa, u8 sid, u16 addr, u16 *apid)
+static int pmic_arb_ppid_to_apid_v1(struct spmi_pmic_arb *pmic_arb, u8 sid,
+				    u16 addr, u16 *apid)
 {
 	u16 ppid = sid << 8 | ((addr >> 8) & 0xFF);
-	u32 *mapping_table = pa->mapping_table;
+	u32 *mapping_table = pmic_arb->mapping_table;
 	int index = 0, i;
 	u16 apid_valid;
 	u32 data;
 
-	apid_valid = pa->ppid_to_apid[ppid];
-	if (apid_valid & PMIC_ARB_CHAN_VALID) {
-		*apid = (apid_valid & ~PMIC_ARB_CHAN_VALID);
+	apid_valid = pmic_arb->ppid_to_apid[ppid];
+	if (apid_valid & PMIC_ARB_APID_VALID) {
+		*apid = apid_valid & ~PMIC_ARB_APID_VALID;
 		return 0;
 	}
 
 	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 +
+		if (!test_and_set_bit(index, pmic_arb->mapping_table_valid))
+			mapping_table[index] = readl_relaxed(pmic_arb->cnfg +
 						SPMI_MAPPING_TABLE_REG(index));
 
 		data = mapping_table[index];

@@ -735,9 +732,9 @@ static int qpnpint_irq_domain_map(struct irq_domain *d,
 				index = SPMI_MAPPING_BIT_IS_1_RESULT(data);
 			} else {
 				*apid = SPMI_MAPPING_BIT_IS_1_RESULT(data);
-				pa->ppid_to_apid[ppid]
-					= *apid | PMIC_ARB_CHAN_VALID;
-				pa->apid_data[*apid].ppid = ppid;
+				pmic_arb->ppid_to_apid[ppid]
+					= *apid | PMIC_ARB_APID_VALID;
+				pmic_arb->apid_data[*apid].ppid = ppid;
 				return 0;
 			}
 		} else {

@@ -745,9 +742,9 @@ static int qpnpint_irq_domain_map(struct irq_domain *d,
 				index = SPMI_MAPPING_BIT_IS_0_RESULT(data);
 			} else {
 				*apid = SPMI_MAPPING_BIT_IS_0_RESULT(data);
-				pa->ppid_to_apid[ppid]
-					= *apid | PMIC_ARB_CHAN_VALID;
-				pa->apid_data[*apid].ppid = ppid;
+				pmic_arb->ppid_to_apid[ppid]
+					= *apid | PMIC_ARB_APID_VALID;
+				pmic_arb->apid_data[*apid].ppid = ppid;
 				return 0;
 			}
 		}

@@ -757,78 +754,77 @@ static int qpnpint_irq_domain_map(struct irq_domain *d,
 }
 
 /* v1 offset per ee */
-static int
-pmic_arb_offset_v1(struct spmi_pmic_arb *pa, u8 sid, u16 addr, u32 *offset)
+static int pmic_arb_offset_v1(struct spmi_pmic_arb *pmic_arb, u8 sid, u16 addr,
+				u32 *offset)
 {
-	*offset = 0x800 + 0x80 * pa->channel;
+	*offset = 0x800 + 0x80 * pmic_arb->channel;
 	return 0;
 }
 
-static u16 pmic_arb_find_apid(struct spmi_pmic_arb *pa, u16 ppid)
+static u16 pmic_arb_find_apid(struct spmi_pmic_arb *pmic_arb, u16 ppid)
 {
+	struct apid_data *apidd = &pmic_arb->apid_data[pmic_arb->last_apid];
 	u32 regval, offset;
-	u16 apid;
-	u16 id;
+	u16 id, apid;
 
 	/*
-	 * PMIC_ARB_REG_CHNL is a table in HW mapping channel to ppid.
+	 * PMIC_ARB_REG_APID is a table in HW mapping apid to ppid.
 	 * ppid_to_apid is an in-memory invert of that table.
 	 */
-	for (apid = pa->last_apid; ; apid++) {
-		offset = PMIC_ARB_REG_CHNL(apid);
-		if (offset >= pa->core_size)
+	for (apid = pmic_arb->last_apid; ; apid++, apidd++) {
+		offset = PMIC_ARB_REG_APID(apid);
+		if (offset >= pmic_arb->core_size)
 			break;
 
-		regval = readl_relaxed(pa->cnfg +
-				      SPMI_OWNERSHIP_TABLE_REG(apid));
-		pa->apid_data[apid].owner = SPMI_OWNERSHIP_PERIPH2OWNER(regval);
+		regval = readl_relaxed(pmic_arb->cnfg +
+					SPMI_OWNERSHIP_TABLE_REG(apid));
+		apidd->owner = SPMI_OWNERSHIP_PERIPH2OWNER(regval);
 
-		regval = readl_relaxed(pa->core + offset);
+		regval = readl_relaxed(pmic_arb->core + offset);
 		if (!regval)
 			continue;
 
 		id = (regval >> 8) & PMIC_ARB_PPID_MASK;
-		pa->ppid_to_apid[id] = apid | PMIC_ARB_CHAN_VALID;
-		pa->apid_data[apid].ppid = id;
+		pmic_arb->ppid_to_apid[id] = apid | PMIC_ARB_APID_VALID;
+		apidd->ppid = id;
 		if (id == ppid) {
-			apid |= PMIC_ARB_CHAN_VALID;
+			apid |= PMIC_ARB_APID_VALID;
 			break;
 		}
 	}
-	pa->last_apid = apid & ~PMIC_ARB_CHAN_VALID;
+	pmic_arb->last_apid = apid & ~PMIC_ARB_APID_VALID;
 
 	return apid;
 }
 
-
-static int
-pmic_arb_ppid_to_apid_v2(struct spmi_pmic_arb *pa, u8 sid, u16 addr, u16 *apid)
+static int pmic_arb_ppid_to_apid_v2(struct spmi_pmic_arb *pmic_arb, u8 sid,
+				    u16 addr, u16 *apid)
 {
 	u16 ppid = (sid << 8) | (addr >> 8);
 	u16 apid_valid;
 
-	apid_valid = pa->ppid_to_apid[ppid];
-	if (!(apid_valid & PMIC_ARB_CHAN_VALID))
-		apid_valid = pmic_arb_find_apid(pa, ppid);
-	if (!(apid_valid & PMIC_ARB_CHAN_VALID))
+	apid_valid = pmic_arb->ppid_to_apid[ppid];
+	if (!(apid_valid & PMIC_ARB_APID_VALID))
+		apid_valid = pmic_arb_find_apid(pmic_arb, ppid);
+	if (!(apid_valid & PMIC_ARB_APID_VALID))
 		return -ENODEV;
 
-	*apid = (apid_valid & ~PMIC_ARB_CHAN_VALID);
+	*apid = apid_valid & ~PMIC_ARB_APID_VALID;
 	return 0;
 }
 
 /* v2 offset per ppid and per ee */
-static int
-pmic_arb_offset_v2(struct spmi_pmic_arb *pa, u8 sid, u16 addr, u32 *offset)
+static int pmic_arb_offset_v2(struct spmi_pmic_arb *pmic_arb, u8 sid, u16 addr,
+				u32 *offset)
 {
 	u16 apid;
 	int rc;
 
-	rc = pmic_arb_ppid_to_apid_v2(pa, sid, addr, &apid);
+	rc = pmic_arb_ppid_to_apid_v2(pmic_arb, sid, addr, &apid);
 	if (rc < 0)
 		return rc;
 
-	*offset = 0x1000 * pa->ee + 0x8000 * apid;
+	*offset = 0x1000 * pmic_arb->ee + 0x8000 * apid;
 	return 0;
 }
 

@@ -930,22 +926,23 @@ static u32 pmic_arb_irq_clear_v2(u16 n)
 
 static int spmi_pmic_arb_probe(struct platform_device *pdev)
 {
-	struct spmi_pmic_arb *pa;
+	struct spmi_pmic_arb *pmic_arb;
 	struct spmi_controller *ctrl;
 	struct resource *res;
 	void __iomem *core;
+	u32 *mapping_table;
 	u32 channel, ee, hw_ver;
 	int err;
 
-	ctrl = spmi_controller_alloc(&pdev->dev, sizeof(*pa));
+	ctrl = spmi_controller_alloc(&pdev->dev, sizeof(*pmic_arb));
 	if (!ctrl)
 		return -ENOMEM;
 
-	pa = spmi_controller_get_drvdata(ctrl);
-	pa->spmic = ctrl;
+	pmic_arb = spmi_controller_get_drvdata(ctrl);
+	pmic_arb->spmic = ctrl;
 
 	res = platform_get_resource_byname(pdev, IORESOURCE_MEM, "core");
-	pa->core_size = resource_size(res);
+	pmic_arb->core_size = resource_size(res);
 
 	core = devm_ioremap_resource(&ctrl->dev, res);
 	if (IS_ERR(core)) {

@@ -953,9 +950,10 @@ static int spmi_pmic_arb_probe(struct platform_device *pdev)
 		goto err_put_ctrl;
 	}
 
-	pa->ppid_to_apid = devm_kcalloc(&ctrl->dev, PMIC_ARB_MAX_PPID,
-					sizeof(*pa->ppid_to_apid), GFP_KERNEL);
-	if (!pa->ppid_to_apid) {
+	pmic_arb->ppid_to_apid = devm_kcalloc(&ctrl->dev, PMIC_ARB_MAX_PPID,
+					      sizeof(*pmic_arb->ppid_to_apid),
+					      GFP_KERNEL);
+	if (!pmic_arb->ppid_to_apid) {
 		err = -ENOMEM;
 		goto err_put_ctrl;
 	}

@@ -963,54 +961,54 @@ 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) {
-		pa->ver_ops = &pmic_arb_v1;
-		pa->wr_base = core;
-		pa->rd_base = core;
+		pmic_arb->ver_ops = &pmic_arb_v1;
+		pmic_arb->wr_base = core;
+		pmic_arb->rd_base = core;
 	} else {
-		pa->core = core;
+		pmic_arb->core = core;
 
 		if (hw_ver < PMIC_ARB_VERSION_V3_MIN)
-			pa->ver_ops = &pmic_arb_v2;
+			pmic_arb->ver_ops = &pmic_arb_v2;
 		else
-			pa->ver_ops = &pmic_arb_v3;
+			pmic_arb->ver_ops = &pmic_arb_v3;
 
 		res = platform_get_resource_byname(pdev, IORESOURCE_MEM,
 						   "obsrvr");
-		pa->rd_base = devm_ioremap_resource(&ctrl->dev, res);
-		if (IS_ERR(pa->rd_base)) {
-			err = PTR_ERR(pa->rd_base);
+		pmic_arb->rd_base = devm_ioremap_resource(&ctrl->dev, res);
+		if (IS_ERR(pmic_arb->rd_base)) {
+			err = PTR_ERR(pmic_arb->rd_base);
 			goto err_put_ctrl;
 		}
 
 		res = platform_get_resource_byname(pdev, IORESOURCE_MEM,
 						   "chnls");
-		pa->wr_base = devm_ioremap_resource(&ctrl->dev, res);
-		if (IS_ERR(pa->wr_base)) {
-			err = PTR_ERR(pa->wr_base);
+		pmic_arb->wr_base = devm_ioremap_resource(&ctrl->dev, res);
+		if (IS_ERR(pmic_arb->wr_base)) {
+			err = PTR_ERR(pmic_arb->wr_base);
 			goto err_put_ctrl;
 		}
 	}
 
 	dev_info(&ctrl->dev, "PMIC arbiter version %s (0x%x)\n",
-		 pa->ver_ops->ver_str, hw_ver);
+		 pmic_arb->ver_ops->ver_str, hw_ver);
 
 	res = platform_get_resource_byname(pdev, IORESOURCE_MEM, "intr");
-	pa->intr = devm_ioremap_resource(&ctrl->dev, res);
-	if (IS_ERR(pa->intr)) {
-		err = PTR_ERR(pa->intr);
+	pmic_arb->intr = devm_ioremap_resource(&ctrl->dev, res);
+	if (IS_ERR(pmic_arb->intr)) {
+		err = PTR_ERR(pmic_arb->intr);
 		goto err_put_ctrl;
 	}
 
 	res = platform_get_resource_byname(pdev, IORESOURCE_MEM, "cnfg");
-	pa->cnfg = devm_ioremap_resource(&ctrl->dev, res);
-	if (IS_ERR(pa->cnfg)) {
-		err = PTR_ERR(pa->cnfg);
+	pmic_arb->cnfg = devm_ioremap_resource(&ctrl->dev, res);
+	if (IS_ERR(pmic_arb->cnfg)) {
+		err = PTR_ERR(pmic_arb->cnfg);
 		goto err_put_ctrl;
 	}
 
-	pa->irq = platform_get_irq_byname(pdev, "periph_irq");
-	if (pa->irq < 0) {
-		err = pa->irq;
+	pmic_arb->irq = platform_get_irq_byname(pdev, "periph_irq");
+	if (pmic_arb->irq < 0) {
+		err = pmic_arb->irq;
 		goto err_put_ctrl;
 	}
 

@@ -1027,7 +1025,7 @@ static int spmi_pmic_arb_probe(struct platform_device *pdev)
 		goto err_put_ctrl;
 	}
 
-	pa->channel = channel;
+	pmic_arb->channel = channel;
 
 	err = of_property_read_u32(pdev->dev.of_node, "qcom,ee", &ee);
 	if (err) {

@@ -1041,38 +1039,39 @@ static int spmi_pmic_arb_probe(struct platform_device *pdev)
 		goto err_put_ctrl;
 	}
 
-	pa->ee = ee;
-
-	pa->mapping_table = devm_kcalloc(&ctrl->dev, PMIC_ARB_MAX_PERIPHS - 1,
-					sizeof(*pa->mapping_table), GFP_KERNEL);
-	if (!pa->mapping_table) {
+	pmic_arb->ee = ee;
+	mapping_table = devm_kcalloc(&ctrl->dev, PMIC_ARB_MAX_PERIPHS - 1,
+					sizeof(*mapping_table), GFP_KERNEL);
+	if (!mapping_table) {
 		err = -ENOMEM;
 		goto err_put_ctrl;
 	}
 
+	pmic_arb->mapping_table = mapping_table;
 	/* Initialize max_apid/min_apid to the opposite bounds, during
 	 * the irq domain translation, we are sure to update these */
-	pa->max_apid = 0;
-	pa->min_apid = PMIC_ARB_MAX_PERIPHS - 1;
+	pmic_arb->max_apid = 0;
+	pmic_arb->min_apid = PMIC_ARB_MAX_PERIPHS - 1;
 
 	platform_set_drvdata(pdev, ctrl);
-	raw_spin_lock_init(&pa->lock);
+	raw_spin_lock_init(&pmic_arb->lock);
 
 	ctrl->cmd = pmic_arb_cmd;
 	ctrl->read_cmd = pmic_arb_read_cmd;
 	ctrl->write_cmd = pmic_arb_write_cmd;
 
 	dev_dbg(&pdev->dev, "adding irq domain\n");
-	pa->domain = irq_domain_add_tree(pdev->dev.of_node,
-					 &pmic_arb_irq_domain_ops, pa);
-	if (!pa->domain) {
+	pmic_arb->domain = irq_domain_add_tree(pdev->dev.of_node,
+					 &pmic_arb_irq_domain_ops, pmic_arb);
+	if (!pmic_arb->domain) {
 		dev_err(&pdev->dev, "unable to create irq_domain\n");
 		err = -ENOMEM;
 		goto err_put_ctrl;
 	}
 
-	irq_set_chained_handler_and_data(pa->irq, pmic_arb_chained_irq, pa);
-	enable_irq_wake(pa->irq);
+	irq_set_chained_handler_and_data(pmic_arb->irq, pmic_arb_chained_irq,
+					pmic_arb);
+	enable_irq_wake(pmic_arb->irq);
 
 	err = spmi_controller_add(ctrl);
 	if (err)

@@ -1081,8 +1080,8 @@ static int spmi_pmic_arb_probe(struct platform_device *pdev)
 	return 0;
 
 err_domain_remove:
-	irq_set_chained_handler_and_data(pa->irq, NULL, NULL);
-	irq_domain_remove(pa->domain);
+	irq_set_chained_handler_and_data(pmic_arb->irq, NULL, NULL);
+	irq_domain_remove(pmic_arb->domain);
 err_put_ctrl:
 	spmi_controller_put(ctrl);
 	return err;

@@ -1091,10 +1090,10 @@ static int spmi_pmic_arb_probe(struct platform_device *pdev)
 static int spmi_pmic_arb_remove(struct platform_device *pdev)
 {
 	struct spmi_controller *ctrl = platform_get_drvdata(pdev);
-	struct spmi_pmic_arb *pa = spmi_controller_get_drvdata(ctrl);
+	struct spmi_pmic_arb *pmic_arb = spmi_controller_get_drvdata(ctrl);
 	spmi_controller_remove(ctrl);
-	irq_set_chained_handler_and_data(pa->irq, NULL, NULL);
-	irq_domain_remove(pa->domain);
+	irq_set_chained_handler_and_data(pmic_arb->irq, NULL, NULL);
+	irq_domain_remove(pmic_arb->domain);
 	spmi_controller_put(ctrl);
 	return 0;
 }
Signature · 2 lines
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project
#3 lore raw

Re: [PATCH V3 2/5] spmi: pmic-arb: rename pa_xx to pmic_arb_xx and other code cleanup

Quoted 10 lines · Kiran Gunda

This patch cleans up the following.

- Rename the "pa" to "pmic_arb".
- Rename the spmi_pmic_arb *dev to spmi_pmic_arb *pmic_arb.
- Rename the pa_{read,write}_data() functions to pmic_arb_{read,write}_data().
- Rename channel to APID.
- Rename the HWIRQ_*() macros to hwirq_to_*().
- Clean up qpnpint_irq_set_type() and pmic_arb_find_apid() functions.

This patch still does too much at once. It's really hard to verify it. Please split it up by squashing this patch in, and then making another patch to do the qpnpint_irq_set_type() change and the pmic_arb_find_apid() changes. That's what I did to even look at this change sanely. The goal being to make a patch that does nothing besides rename things and can be verified to not actually change the generated code with scripts/objdiff.

More comments after this patch.

drivers/spmi/spmi-pmic-arb.c +34 -36
index 91b068b1c83d..2e34dd66feec 100644
@@ -513,7 +513,6 @@ static void periph_interrupt(struct spmi_pmic_arb *pmic_arb, u16 apid)
 static void pmic_arb_chained_irq(struct irq_desc *desc)
 {
 	struct spmi_pmic_arb *pmic_arb = irq_desc_get_handler_data(desc);
-	const struct pmic_arb_ver_ops *ver_ops = pmic_arb->ver_ops;
 	struct irq_chip *chip = irq_desc_get_chip(desc);
 	void __iomem *intr = pmic_arb->intr;
 	int first = pmic_arb->min_apid >> 5;

@@ -525,7 +524,7 @@ static void pmic_arb_chained_irq(struct irq_desc *desc)
 
 	for (i = first; i <= last; ++i) {
 		status = readl_relaxed(intr +
-				    ver_ops->owner_acc_status(pmic_arb->ee, i));
+				    pmic_arb->ver_ops->owner_acc_status(pmic_arb->ee, i));
 		while (status) {
 			id = ffs(status) - 1;
 			status &= ~BIT(id);

@@ -589,34 +588,34 @@ static int qpnpint_irq_set_type(struct irq_data *d, unsigned int flow_type)
 {
 	struct spmi_pmic_arb_qpnpint_type type;
 	u8 irq = hwirq_to_irq(d->hwirq);
+	u8 bit_mask_irq = BIT(irq);
 
 	qpnpint_spmi_read(d, QPNPINT_REG_SET_TYPE, &type, sizeof(type));
 
 	if (flow_type & (IRQF_TRIGGER_RISING | IRQF_TRIGGER_FALLING)) {
-		type.type |= BIT(irq);
+		type.type |= bit_mask_irq;
 		if (flow_type & IRQF_TRIGGER_RISING)
-			type.polarity_high |= BIT(irq);
+			type.polarity_high |= bit_mask_irq;
 		if (flow_type & IRQF_TRIGGER_FALLING)
-			type.polarity_low  |= BIT(irq);
-
-		qpnpint_spmi_write(d, QPNPINT_REG_SET_TYPE, &type,
-					sizeof(type));
-		irq_set_handler_locked(d, handle_edge_irq);
+			type.polarity_low  |= bit_mask_irq;
 	} else {
 		if ((flow_type & (IRQF_TRIGGER_HIGH)) &&
 		    (flow_type & (IRQF_TRIGGER_LOW)))
 			return -EINVAL;
 
-		type.type &= ~BIT(irq); /* level trig */
+		type.type &= ~bit_mask_irq; /* level trig */
 		if (flow_type & IRQF_TRIGGER_HIGH)
-			type.polarity_high |= BIT(irq);
+			type.polarity_high |= bit_mask_irq;
 		else
-			type.polarity_low  |= BIT(irq);
+			type.polarity_low  |= bit_mask_irq;
+	}
 
-		qpnpint_spmi_write(d, QPNPINT_REG_SET_TYPE, &type,
-					sizeof(type));
+	qpnpint_spmi_write(d, QPNPINT_REG_SET_TYPE, &type, sizeof(type));
+
+	if (flow_type & IRQ_TYPE_EDGE_BOTH)
+		irq_set_handler_locked(d, handle_edge_irq);
+	else
 		irq_set_handler_locked(d, handle_level_irq);
-	}
 
 	return 0;
 }

@@ -763,22 +762,22 @@ static int pmic_arb_offset_v1(struct spmi_pmic_arb *pmic_arb, u8 sid, u16 addr,
 
 static u16 pmic_arb_find_apid(struct spmi_pmic_arb *pmic_arb, u16 ppid)
 {
-	struct apid_data *apidd = &pmic_arb->apid_data[pmic_arb->last_apid];
 	u32 regval, offset;
-	u16 id, apid;
+	u16 apid;
+	u16 id;
 
 	/*
-	 * PMIC_ARB_REG_APID is a table in HW mapping apid to ppid.
+	 * PMIC_ARB_REG_CHNL is a table in HW mapping channel to ppid.
 	 * ppid_to_apid is an in-memory invert of that table.
 	 */
-	for (apid = pmic_arb->last_apid; ; apid++, apidd++) {
+	for (apid = pmic_arb->last_apid; ; apid++) {
 		offset = PMIC_ARB_REG_APID(apid);
 		if (offset >= pmic_arb->core_size)
 			break;
 
 		regval = readl_relaxed(pmic_arb->cnfg +
-					SPMI_OWNERSHIP_TABLE_REG(apid));
-		apidd->owner = SPMI_OWNERSHIP_PERIPH2OWNER(regval);
+				      SPMI_OWNERSHIP_TABLE_REG(apid));
+		pmic_arb->apid_data[apid].owner = SPMI_OWNERSHIP_PERIPH2OWNER(regval);
 
 		regval = readl_relaxed(pmic_arb->core + offset);
 		if (!regval)

@@ -786,7 +785,7 @@ static u16 pmic_arb_find_apid(struct spmi_pmic_arb *pmic_arb, u16 ppid)
 
 		id = (regval >> 8) & PMIC_ARB_PPID_MASK;
 		pmic_arb->ppid_to_apid[id] = apid | PMIC_ARB_APID_VALID;
-		apidd->ppid = id;
+		pmic_arb->apid_data[apid].ppid = id;
 		if (id == ppid) {
 			apid |= PMIC_ARB_APID_VALID;
 			break;

@@ -797,34 +796,35 @@ static u16 pmic_arb_find_apid(struct spmi_pmic_arb *pmic_arb, u16 ppid)
 	return apid;
 }
 
-static int pmic_arb_ppid_to_apid_v2(struct spmi_pmic_arb *pmic_arb, u8 sid,
-				    u16 addr, u16 *apid)
+
+static int
+pmic_arb_ppid_to_apid_v2(struct spmi_pmic_arb *pa, u8 sid, u16 addr, u16 *apid)
 {
 	u16 ppid = (sid << 8) | (addr >> 8);
 	u16 apid_valid;
 
-	apid_valid = pmic_arb->ppid_to_apid[ppid];
+	apid_valid = pa->ppid_to_apid[ppid];
 	if (!(apid_valid & PMIC_ARB_APID_VALID))
-		apid_valid = pmic_arb_find_apid(pmic_arb, ppid);
+		apid_valid = pmic_arb_find_apid(pa, ppid);
 	if (!(apid_valid & PMIC_ARB_APID_VALID))
 		return -ENODEV;
 
-	*apid = apid_valid & ~PMIC_ARB_APID_VALID;
+	*apid = (apid_valid & ~PMIC_ARB_APID_VALID);
 	return 0;
 }
 
 /* v2 offset per ppid and per ee */
-static int pmic_arb_offset_v2(struct spmi_pmic_arb *pmic_arb, u8 sid, u16 addr,
-				u32 *offset)
+static int
+pmic_arb_offset_v2(struct spmi_pmic_arb *pa, u8 sid, u16 addr, u32 *offset)
 {
 	u16 apid;
 	int rc;
 
-	rc = pmic_arb_ppid_to_apid_v2(pmic_arb, sid, addr, &apid);
+	rc = pmic_arb_ppid_to_apid_v2(pa, sid, addr, &apid);
 	if (rc < 0)
 		return rc;
 
-	*offset = 0x1000 * pmic_arb->ee + 0x8000 * apid;
+	*offset = 0x1000 * pa->ee + 0x8000 * apid;
 	return 0;
 }
 

@@ -930,7 +930,6 @@ static int spmi_pmic_arb_probe(struct platform_device *pdev)
 	struct spmi_controller *ctrl;
 	struct resource *res;
 	void __iomem *core;
-	u32 *mapping_table;
 	u32 channel, ee, hw_ver;
 	int err;
 

@@ -1040,14 +1039,13 @@ static int spmi_pmic_arb_probe(struct platform_device *pdev)
 	}
 
 	pmic_arb->ee = ee;
-	mapping_table = devm_kcalloc(&ctrl->dev, PMIC_ARB_MAX_PERIPHS - 1,
-					sizeof(*mapping_table), GFP_KERNEL);
-	if (!mapping_table) {
+	pmic_arb->mapping_table = devm_kcalloc(&ctrl->dev, PMIC_ARB_MAX_PERIPHS - 1,
+					sizeof(*pmic_arb->mapping_table), GFP_KERNEL);
+	if (!pmic_arb->mapping_table) {
 		err = -ENOMEM;
 		goto err_put_ctrl;
 	}
 
-	pmic_arb->mapping_table = mapping_table;
 	/* Initialize max_apid/min_apid to the opposite bounds, during
 	 * the irq domain translation, we are sure to update these */
 	pmic_arb->max_apid = 0;

Review

Quoted 11 lines
(quoted diff) +4 -3
-static void periph_interrupt(struct spmi_pmic_arb *pa, u16 apid)
+static void periph_interrupt(struct spmi_pmic_arb *pmic_arb, u16 apid)
 {
+	const struct pmic_arb_ver_ops *ver_ops = pmic_arb->ver_ops;
+	u8 sid = (pmic_arb->apid_data[apid].ppid >> 8) & 0xF;
+	u8 per = pmic_arb->apid_data[apid].ppid & 0xFF;
 	unsigned int irq;
 	u32 status;
 	int id;
-	u8 sid = (pa->apid_data[apid].ppid >> 8) & 0xF;
-	u8 per = pa->apid_data[apid].ppid & 0xFF;

Why did these two move? Should stay in the same place and take the rename.

Quoted 35 lines
(quoted diff) +9 -7
-	status = readl_relaxed(pa->intr + pa->ver_ops->irq_status(apid));
+	status = readl_relaxed(pmic_arb->intr + ver_ops->irq_status(apid));
 	while (status) {
 		id = ffs(status) - 1;
 		status &= ~BIT(id);
-		irq = irq_find_mapping(pa->domain, HWIRQ(sid, per, id, apid));
+		irq = irq_find_mapping(pmic_arb->domain,
+					spec_to_hwirq(sid, per, id, apid));
 		if (irq == 0) {
-			cleanup_irq(pa, apid, id);
+			cleanup_irq(pmic_arb, apid, id);
 			continue;
 		}
 		generic_handle_irq(irq);

@@ -515,11 +512,12 @@ static void periph_interrupt(struct spmi_pmic_arb *pa, u16 apid)
 
 static void pmic_arb_chained_irq(struct irq_desc *desc)
 {
-	struct spmi_pmic_arb *pa = irq_desc_get_handler_data(desc);
+	struct spmi_pmic_arb *pmic_arb = irq_desc_get_handler_data(desc);
+	const struct pmic_arb_ver_ops *ver_ops = pmic_arb->ver_ops;
 	struct irq_chip *chip = irq_desc_get_chip(desc);
-	void __iomem *intr = pa->intr;
-	int first = pa->min_apid >> 5;
-	int last = pa->max_apid >> 5;
+	void __iomem *intr = pmic_arb->intr;
+	int first = pmic_arb->min_apid >> 5;
+	int last = pmic_arb->max_apid >> 5;
 	u32 status, enable;
 	int i, id, apid;
 

@@ -527,15 +525,15 @@ static void pmic_arb_chained_irq(struct irq_desc *desc)
 
 	for (i = first; i <= last; ++i) {

Review static void pmic_arb_chained_irq(struct irq_desc *desc) · line 534

Quoted 10 lines
(quoted diff) +2 -2
 		status = readl_relaxed(intr +
-				      pa->ver_ops->owner_acc_status(pa->ee, i));
+				    ver_ops->owner_acc_status(pmic_arb->ee, i));
 		while (status) {
 			id = ffs(status) - 1;
 			status &= ~BIT(id);
 			apid = id + i * 32;
 			enable = readl_relaxed(intr +
-					pa->ver_ops->acc_enable(apid));
+					pmic_arb->ver_ops->acc_enable(apid));

This still uses pmic_arb->ver_ops instead of ver_ops?

Quoted 43 lines
(quoted diff) +11 -11
 			if (enable & SPMI_PIC_ACC_ENABLE_BIT)
-				periph_interrupt(pa, apid);
+				periph_interrupt(pmic_arb, apid);
 		}
 	}
 

@@ -589,35 +588,35 @@ static void qpnpint_irq_unmask(struct irq_data *d)
 static int qpnpint_irq_set_type(struct irq_data *d, unsigned int flow_type)
 {
 	struct spmi_pmic_arb_qpnpint_type type;
-	u8 irq = HWIRQ_IRQ(d->hwirq);
-	u8 bit_mask_irq = BIT(irq);
+	u8 irq = hwirq_to_irq(d->hwirq);
 
 	qpnpint_spmi_read(d, QPNPINT_REG_SET_TYPE, &type, sizeof(type));
 
 	if (flow_type & (IRQF_TRIGGER_RISING | IRQF_TRIGGER_FALLING)) {
-		type.type |= bit_mask_irq;
+		type.type |= BIT(irq);
 		if (flow_type & IRQF_TRIGGER_RISING)
-			type.polarity_high |= bit_mask_irq;
+			type.polarity_high |= BIT(irq);
 		if (flow_type & IRQF_TRIGGER_FALLING)
-			type.polarity_low  |= bit_mask_irq;
+			type.polarity_low  |= BIT(irq);
+
+		qpnpint_spmi_write(d, QPNPINT_REG_SET_TYPE, &type,
+					sizeof(type));
+		irq_set_handler_locked(d, handle_edge_irq);
 	} else {
 		if ((flow_type & (IRQF_TRIGGER_HIGH)) &&
 		    (flow_type & (IRQF_TRIGGER_LOW)))
 			return -EINVAL;
 
-		type.type &= ~bit_mask_irq; /* level trig */
+		type.type &= ~BIT(irq); /* level trig */
 		if (flow_type & IRQF_TRIGGER_HIGH)
-			type.polarity_high |= bit_mask_irq;
+			type.polarity_high |= BIT(irq);
 		else
-			type.polarity_low  |= bit_mask_irq;
-	}
-

Review static void qpnpint_irq_unmask(struct irq_data *d) · line 619

Quoted 10 lines
(quoted diff) +4 -4
-	qpnpint_spmi_write(d, QPNPINT_REG_SET_TYPE, &type, sizeof(type));
+			type.polarity_low  |= BIT(irq);
 
-	if (flow_type & IRQ_TYPE_EDGE_BOTH)
-		irq_set_handler_locked(d, handle_edge_irq);
-	else
+		qpnpint_spmi_write(d, QPNPINT_REG_SET_TYPE, &type,
+					sizeof(type));
 		irq_set_handler_locked(d, handle_level_irq);
+	}

Not sure I suggested this, but I would keep the REG_SET_TYPE call outside of the if statements to consolidate them, and also grow a local variable to hold the handler (aptly called handler) that then can be assigned with a single call to irq_set_handler_locked(). Please do this in a different patch.

Signature · 2 lines
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
a Linux Foundation Collaborative Project
author
#4 lore raw

Re: [PATCH V3 2/5] spmi: pmic-arb: rename pa_xx to pmic_arb_xx and other code cleanup

Quoted 12 lines · Stephen Boyd
Quoted 10 lines · Kiran Gunda

This patch cleans up the following.

- Rename the "pa" to "pmic_arb".
- Rename the spmi_pmic_arb *dev to spmi_pmic_arb *pmic_arb.
- Rename the pa_{read,write}_data() functions to pmic_arb_{read,write}_data().
- Rename channel to APID.
- Rename the HWIRQ_*() macros to hwirq_to_*().
- Clean up qpnpint_irq_set_type() and pmic_arb_find_apid() functions.

Quoted 8 lines

This patch still does too much at once. It's really hard to verify it. Please split it up by squashing this patch in, and then making another patch to do the qpnpint_irq_set_type() change and the pmic_arb_find_apid() changes. That's what I did to even look at this change sanely. The goal being to make a patch that does nothing besides rename things and can be verified to not actually change the generated code with scripts/objdiff.

ok. will split it up next series.

Quoted 196 lines

More comments after this patch.

drivers/spmi/spmi-pmic-arb.c +0 -1
index 91b068b1c83d..2e34dd66feec 100644
@@ -513,7 +513,6 @@ static void periph_interrupt(struct spmi_pmic_arb
 *pmic_arb, u16 apid)
 static void pmic_arb_chained_irq(struct irq_desc *desc)
 {
 	struct spmi_pmic_arb *pmic_arb = irq_desc_get_handler_data(desc);
-	const struct pmic_arb_ver_ops *ver_ops = pmic_arb->ver_ops;
 	struct irq_chip *chip = irq_desc_get_chip(desc);
 	void __iomem *intr = pmic_arb->intr;
drivers/spmi/spmi-pmic-arb.c +1 -1
 	int first = pmic_arb->min_apid >> 5;

@@ -525,7 +524,7 @@ static void pmic_arb_chained_irq(struct irq_desc 
 *desc)
 
 	for (i = first; i <= last; ++i) {
 		status = readl_relaxed(intr +
-				    ver_ops->owner_acc_status(pmic_arb->ee, i));
+				    pmic_arb->ver_ops->owner_acc_status(pmic_arb->ee, i));
 		while (status) {
 			id = ffs(status) - 1;
drivers/spmi/spmi-pmic-arb.c +13 -13
 			status &= ~BIT(id);

@@ -589,34 +588,34 @@ static int qpnpint_irq_set_type(struct irq_data
 *d, unsigned int flow_type)
 {
 	struct spmi_pmic_arb_qpnpint_type type;
 	u8 irq = hwirq_to_irq(d->hwirq);
+	u8 bit_mask_irq = BIT(irq);
 
 	qpnpint_spmi_read(d, QPNPINT_REG_SET_TYPE, &type, sizeof(type));
 
 	if (flow_type & (IRQF_TRIGGER_RISING | IRQF_TRIGGER_FALLING)) {
-		type.type |= BIT(irq);
+		type.type |= bit_mask_irq;
 		if (flow_type & IRQF_TRIGGER_RISING)
-			type.polarity_high |= BIT(irq);
+			type.polarity_high |= bit_mask_irq;
 		if (flow_type & IRQF_TRIGGER_FALLING)
-			type.polarity_low  |= BIT(irq);
-
-		qpnpint_spmi_write(d, QPNPINT_REG_SET_TYPE, &type,
-					sizeof(type));
-		irq_set_handler_locked(d, handle_edge_irq);
+			type.polarity_low  |= bit_mask_irq;
 	} else {
 		if ((flow_type & (IRQF_TRIGGER_HIGH)) &&
 		    (flow_type & (IRQF_TRIGGER_LOW)))
 			return -EINVAL;
 
-		type.type &= ~BIT(irq); /* level trig */
+		type.type &= ~bit_mask_irq; /* level trig */
 		if (flow_type & IRQF_TRIGGER_HIGH)
-			type.polarity_high |= BIT(irq);
+			type.polarity_high |= bit_mask_irq;
 		else
-			type.polarity_low  |= BIT(irq);
+			type.polarity_low  |= bit_mask_irq;
+	}
 
-		qpnpint_spmi_write(d, QPNPINT_REG_SET_TYPE, &type,
-					sizeof(type));
+	qpnpint_spmi_write(d, QPNPINT_REG_SET_TYPE, &type, sizeof(type));
+
+	if (flow_type & IRQ_TYPE_EDGE_BOTH)
+		irq_set_handler_locked(d, handle_edge_irq);
+	else
 		irq_set_handler_locked(d, handle_level_irq);
-	}
 
 	return 0;

}

drivers/spmi/spmi-pmic-arb.c +6 -6
@@ -763,22 +762,22 @@ static int pmic_arb_offset_v1(struct
 spmi_pmic_arb *pmic_arb, u8 sid, u16 addr,
 
 static u16 pmic_arb_find_apid(struct spmi_pmic_arb *pmic_arb, u16 
 ppid)
 {
-	struct apid_data *apidd = &pmic_arb->apid_data[pmic_arb->last_apid];
 	u32 regval, offset;
-	u16 id, apid;
+	u16 apid;
+	u16 id;
 
 	/*
-	 * PMIC_ARB_REG_APID is a table in HW mapping apid to ppid.
+	 * PMIC_ARB_REG_CHNL is a table in HW mapping channel to ppid.
 	 * ppid_to_apid is an in-memory invert of that table.
 	 */
-	for (apid = pmic_arb->last_apid; ; apid++, apidd++) {
+	for (apid = pmic_arb->last_apid; ; apid++) {
 		offset = PMIC_ARB_REG_APID(apid);
 		if (offset >= pmic_arb->core_size)
 			break;
 
 		regval = readl_relaxed(pmic_arb->cnfg +
-					SPMI_OWNERSHIP_TABLE_REG(apid));
-		apidd->owner = SPMI_OWNERSHIP_PERIPH2OWNER(regval);
+				      SPMI_OWNERSHIP_TABLE_REG(apid));
+		pmic_arb->apid_data[apid].owner = 
 SPMI_OWNERSHIP_PERIPH2OWNER(regval);
drivers/spmi/spmi-pmic-arb.c +1 -1
 		regval = readl_relaxed(pmic_arb->core + offset);
 		if (!regval)

@@ -786,7 +785,7 @@ static u16 pmic_arb_find_apid(struct spmi_pmic_arb
 *pmic_arb, u16 ppid)
 
 		id = (regval >> 8) & PMIC_ARB_PPID_MASK;
 		pmic_arb->ppid_to_apid[id] = apid | PMIC_ARB_APID_VALID;
-		apidd->ppid = id;
+		pmic_arb->apid_data[apid].ppid = id;
 		if (id == ppid) {
 			apid |= PMIC_ARB_APID_VALID;
drivers/spmi/spmi-pmic-arb.c +13 -14
 			break;

@@ -797,34 +796,35 @@ static u16 pmic_arb_find_apid(struct
 spmi_pmic_arb *pmic_arb, u16 ppid)
 	return apid;
 }
 
-static int pmic_arb_ppid_to_apid_v2(struct spmi_pmic_arb *pmic_arb, u8 
 sid,
-				    u16 addr, u16 *apid)
+
+static int
+pmic_arb_ppid_to_apid_v2(struct spmi_pmic_arb *pa, u8 sid, u16 addr, 
 u16 *apid)
 {
 	u16 ppid = (sid << 8) | (addr >> 8);
 	u16 apid_valid;
 
-	apid_valid = pmic_arb->ppid_to_apid[ppid];
+	apid_valid = pa->ppid_to_apid[ppid];
 	if (!(apid_valid & PMIC_ARB_APID_VALID))
-		apid_valid = pmic_arb_find_apid(pmic_arb, ppid);
+		apid_valid = pmic_arb_find_apid(pa, ppid);
 	if (!(apid_valid & PMIC_ARB_APID_VALID))
 		return -ENODEV;
 
-	*apid = apid_valid & ~PMIC_ARB_APID_VALID;
+	*apid = (apid_valid & ~PMIC_ARB_APID_VALID);
 	return 0;
 }
 
 /* v2 offset per ppid and per ee */
-static int pmic_arb_offset_v2(struct spmi_pmic_arb *pmic_arb, u8 sid, 
 u16 addr,
-				u32 *offset)
+static int
+pmic_arb_offset_v2(struct spmi_pmic_arb *pa, u8 sid, u16 addr, u32 
 *offset)
 {
 	u16 apid;
 	int rc;
 
-	rc = pmic_arb_ppid_to_apid_v2(pmic_arb, sid, addr, &apid);
+	rc = pmic_arb_ppid_to_apid_v2(pa, sid, addr, &apid);
 	if (rc < 0)
 		return rc;
 
-	*offset = 0x1000 * pmic_arb->ee + 0x8000 * apid;
+	*offset = 0x1000 * pa->ee + 0x8000 * apid;
 	return 0;
 }

@@ -930,7 +930,6 @@ static int spmi_pmic_arb_probe(struct 
 platform_device *pdev)
 	struct spmi_controller *ctrl;
 	struct resource *res;
 	void __iomem *core;
-	u32 *mapping_table;
 	u32 channel, ee, hw_ver;
 	int err;

@@ -1040,14 +1039,13 @@ static int spmi_pmic_arb_probe(struct
 platform_device *pdev)
 	}
 
 	pmic_arb->ee = ee;
-	mapping_table = devm_kcalloc(&ctrl->dev, PMIC_ARB_MAX_PERIPHS - 1,
-					sizeof(*mapping_table), GFP_KERNEL);
-	if (!mapping_table) {
+	pmic_arb->mapping_table = devm_kcalloc(&ctrl->dev, 
 PMIC_ARB_MAX_PERIPHS - 1,
+					sizeof(*pmic_arb->mapping_table), GFP_KERNEL);
+	if (!pmic_arb->mapping_table) {
 		err = -ENOMEM;
 		goto err_put_ctrl;
 	}
 
-	pmic_arb->mapping_table = mapping_table;
 	/* Initialize max_apid/min_apid to the opposite bounds, during
drivers/spmi/spmi-pmic-arb.c +0 -0
 	 * the irq domain translation, we are sure to update these */
 	pmic_arb->max_apid = 0;
Quoted 16 lines

Review

Quoted 12 lines
(quoted diff) +1 -1
-static void periph_interrupt(struct spmi_pmic_arb *pa, u16 apid)
+static void periph_interrupt(struct spmi_pmic_arb *pmic_arb, u16

apid)
{

(quoted diff) +3 -2
+	const struct pmic_arb_ver_ops *ver_ops = pmic_arb->ver_ops;
+	u8 sid = (pmic_arb->apid_data[apid].ppid >> 8) & 0xF;
+	u8 per = pmic_arb->apid_data[apid].ppid & 0xFF;
 	unsigned int irq;
 	u32 status;
 	int id;
-	u8 sid = (pa->apid_data[apid].ppid >> 8) & 0xF;
-	u8 per = pa->apid_data[apid].ppid & 0xFF;

Why did these two move? Should stay in the same place and take the rename.

Sure. will do it in next patch series.

Quoted 36 lines
Quoted 36 lines
(quoted diff) +9 -7
-	status = readl_relaxed(pa->intr + pa->ver_ops->irq_status(apid));
+	status = readl_relaxed(pmic_arb->intr + ver_ops->irq_status(apid));
 	while (status) {
 		id = ffs(status) - 1;
 		status &= ~BIT(id);
-		irq = irq_find_mapping(pa->domain, HWIRQ(sid, per, id, apid));
+		irq = irq_find_mapping(pmic_arb->domain,
+					spec_to_hwirq(sid, per, id, apid));
 		if (irq == 0) {
-			cleanup_irq(pa, apid, id);
+			cleanup_irq(pmic_arb, apid, id);
 			continue;
 		}
 		generic_handle_irq(irq);

@@ -515,11 +512,12 @@ static void periph_interrupt(struct 
 spmi_pmic_arb *pa, u16 apid)
 
 static void pmic_arb_chained_irq(struct irq_desc *desc)
 {
-	struct spmi_pmic_arb *pa = irq_desc_get_handler_data(desc);
+	struct spmi_pmic_arb *pmic_arb = irq_desc_get_handler_data(desc);
+	const struct pmic_arb_ver_ops *ver_ops = pmic_arb->ver_ops;
 	struct irq_chip *chip = irq_desc_get_chip(desc);
-	void __iomem *intr = pa->intr;
-	int first = pa->min_apid >> 5;
-	int last = pa->max_apid >> 5;
+	void __iomem *intr = pmic_arb->intr;
+	int first = pmic_arb->min_apid >> 5;
+	int last = pmic_arb->max_apid >> 5;
 	u32 status, enable;
 	int i, id, apid;

@@ -527,15 +525,15 @@ static void pmic_arb_chained_irq(struct irq_desc 
 *desc)
Quoted 14 lines

Review static void pmic_arb_chained_irq(struct irq_desc · line 535

Quoted 11 lines
(quoted diff) +2 -2
 	for (i = first; i <= last; ++i) {
 		status = readl_relaxed(intr +
-				      pa->ver_ops->owner_acc_status(pa->ee, i));
+				    ver_ops->owner_acc_status(pmic_arb->ee, i));
 		while (status) {
 			id = ffs(status) - 1;
 			status &= ~BIT(id);
 			apid = id + i * 32;
 			enable = readl_relaxed(intr +
-					pa->ver_ops->acc_enable(apid));
+					pmic_arb->ver_ops->acc_enable(apid));

This still uses pmic_arb->ver_ops instead of ver_ops?

will correct it next patch series.

Quoted 48 lines
Quoted 48 lines
(quoted diff) +12 -12
 			if (enable & SPMI_PIC_ACC_ENABLE_BIT)
-				periph_interrupt(pa, apid);
+				periph_interrupt(pmic_arb, apid);
 		}
 	}

@@ -589,35 +588,35 @@ static void qpnpint_irq_unmask(struct irq_data 
 *d)
 static int qpnpint_irq_set_type(struct irq_data *d, unsigned int 
 flow_type)
 {
 	struct spmi_pmic_arb_qpnpint_type type;
-	u8 irq = HWIRQ_IRQ(d->hwirq);
-	u8 bit_mask_irq = BIT(irq);
+	u8 irq = hwirq_to_irq(d->hwirq);
 
 	qpnpint_spmi_read(d, QPNPINT_REG_SET_TYPE, &type, sizeof(type));
 
 	if (flow_type & (IRQF_TRIGGER_RISING | IRQF_TRIGGER_FALLING)) {
-		type.type |= bit_mask_irq;
+		type.type |= BIT(irq);
 		if (flow_type & IRQF_TRIGGER_RISING)
-			type.polarity_high |= bit_mask_irq;
+			type.polarity_high |= BIT(irq);
 		if (flow_type & IRQF_TRIGGER_FALLING)
-			type.polarity_low  |= bit_mask_irq;
+			type.polarity_low  |= BIT(irq);
+
+		qpnpint_spmi_write(d, QPNPINT_REG_SET_TYPE, &type,
+					sizeof(type));
+		irq_set_handler_locked(d, handle_edge_irq);
 	} else {
 		if ((flow_type & (IRQF_TRIGGER_HIGH)) &&
 		    (flow_type & (IRQF_TRIGGER_LOW)))
 			return -EINVAL;
 
-		type.type &= ~bit_mask_irq; /* level trig */
+		type.type &= ~BIT(irq); /* level trig */
 		if (flow_type & IRQF_TRIGGER_HIGH)
-			type.polarity_high |= bit_mask_irq;
+			type.polarity_high |= BIT(irq);
 		else
-			type.polarity_low  |= bit_mask_irq;
-	}
-
-	qpnpint_spmi_write(d, QPNPINT_REG_SET_TYPE, &type, sizeof(type));
+			type.polarity_low  |= BIT(irq);
Quoted 13 lines

Review static void qpnpint_irq_unmask(struct irq_data · line 621

Quoted 7 lines
(quoted diff) +3 -3
-	if (flow_type & IRQ_TYPE_EDGE_BOTH)
-		irq_set_handler_locked(d, handle_edge_irq);
-	else
+		qpnpint_spmi_write(d, QPNPINT_REG_SET_TYPE, &type,
+					sizeof(type));
 		irq_set_handler_locked(d, handle_level_irq);
+	}

Not sure I suggested this, but I would keep the REG_SET_TYPE call outside of the if statements to consolidate them, and also grow a local variable to hold the handler (aptly called handler) that then can be assigned with a single call to irq_set_handler_locked(). Please do this in a different patch.

ok ... I will modify it as per your current suggestion.

author
#5 lore raw

PATCH v3 3/5 spmi: pmic-arb: replace the writel_relaxed with __raw_writel

In reply to Kiran Gunda

Replace the writel_relaxed with __raw_writel to avoid byte swapping in pmic_arb_write_data() function. That way the code is independent of the CPU endianness.

Signed-off-by Kiran Gunda <[email protected]>
Reviewed-by Stephen Boyd <[email protected]>
---
 drivers/spmi/spmi-pmic-arb.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)
drivers/spmi/spmi-pmic-arb.c +1 -1
index 91b068b..dee562e 100644
@@ -230,7 +230,7 @@ static void pmic_arb_write_data(struct spmi_pmic_arb *pmic_arb, const u8 *buf,
 	u32 data = 0;
 
 	memcpy(&data, buf, (bc & 3) + 1);
-	pmic_arb_base_write(pmic_arb, reg, data);
+	__raw_writel(data, pmic_arb->wr_base + reg);
 }
 
 static int pmic_arb_wait_for_done(struct spmi_controller *ctrl,
Signature · 2 lines
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project
author
#6 lore raw

PATCH v3 4/5 spmi: pmic-arb: return the value instead of passing by pointer

In reply to Kiran Gunda

Returning the output value from a function, when it is possible, is the better and cleaner way than passing it by the pointer. Hence, modify the ppid_to_apid mapping function to return apid instead of passing it by a pointer. While at it, pass the ppid as function parameter to ppid_to_apid mapping function instead of passing the sid and addr.

Signed-off-by Kiran Gunda <[email protected]>
Reviewed-by Stephen Boyd <[email protected]>
---
 drivers/spmi/spmi-pmic-arb.c | 82 +++++++++++++++++++++-----------------------
 1 file changed, 40 insertions(+), 42 deletions(-)
drivers/spmi/spmi-pmic-arb.c +40 -42
index dee562e..4795092 100644
@@ -178,11 +178,9 @@ struct spmi_pmic_arb {
  */
 struct pmic_arb_ver_ops {
 	const char *ver_str;
-	int (*ppid_to_apid)(struct spmi_pmic_arb *pmic_arb, u8 sid, u16 addr,
-			u16 *apid);
+	int (*ppid_to_apid)(struct spmi_pmic_arb *pmic_arb, u16 ppid);
 	/* spmi commands (read_cmd, write_cmd, cmd) functionality */
-	int (*offset)(struct spmi_pmic_arb *pmic_arb, u8 sid, u16 addr,
-		      u32 *offset);
+	int (*offset)(struct spmi_pmic_arb *pmic_arb, u8 sid, u16 addr);
 	u32 (*fmt_cmd)(u8 opc, u8 sid, u16 addr, u8 bc);
 	int (*non_data_cmd)(struct spmi_controller *ctrl, u8 opc, u8 sid);
 	/* Interrupts controller functionality (offset of PIC registers) */

@@ -242,10 +240,11 @@ static int pmic_arb_wait_for_done(struct spmi_controller *ctrl,
 	u32 offset;
 	int rc;
 
-	rc = pmic_arb->ver_ops->offset(pmic_arb, sid, addr, &offset);
-	if (rc)
+	rc = pmic_arb->ver_ops->offset(pmic_arb, sid, addr);
+	if (rc < 0)
 		return rc;
 
+	offset = rc;
 	offset += PMIC_ARB_STATUS;
 
 	while (timeout--) {

@@ -289,10 +288,11 @@ static int pmic_arb_wait_for_done(struct spmi_controller *ctrl,
 	int rc;
 	u32 offset;
 
-	rc = pmic_arb->ver_ops->offset(pmic_arb, sid, 0, &offset);
-	if (rc)
+	rc = pmic_arb->ver_ops->offset(pmic_arb, sid, 0);
+	if (rc < 0)
 		return rc;
 
+	offset = rc;
 	cmd = ((opc | 0x40) << 27) | ((sid & 0xf) << 20);
 
 	raw_spin_lock_irqsave(&pmic_arb->lock, flags);

@@ -333,10 +333,11 @@ static int pmic_arb_read_cmd(struct spmi_controller *ctrl, u8 opc, u8 sid,
 	int rc;
 	u32 offset;
 
-	rc = pmic_arb->ver_ops->offset(pmic_arb, sid, addr, &offset);
-	if (rc)
+	rc = pmic_arb->ver_ops->offset(pmic_arb, sid, addr);
+	if (rc < 0)
 		return rc;
 
+	offset = rc;
 	if (bc >= PMIC_ARB_MAX_TRANS_BYTES) {
 		dev_err(&ctrl->dev, "pmic-arb supports 1..%d bytes per trans, but:%zu requested",
 			PMIC_ARB_MAX_TRANS_BYTES, len);

@@ -383,10 +384,11 @@ static int pmic_arb_write_cmd(struct spmi_controller *ctrl, u8 opc, u8 sid,
 	int rc;
 	u32 offset;
 
-	rc = pmic_arb->ver_ops->offset(pmic_arb, sid, addr, &offset);
-	if (rc)
+	rc = pmic_arb->ver_ops->offset(pmic_arb, sid, addr);
+	if (rc < 0)
 		return rc;
 
+	offset = rc;
 	if (bc >= PMIC_ARB_MAX_TRANS_BYTES) {
 		dev_err(&ctrl->dev, "pmic-arb supports 1..%d bytes per trans, but:%zu requested",
 			PMIC_ARB_MAX_TRANS_BYTES, len);

@@ -656,8 +658,8 @@ static int qpnpint_irq_domain_dt_translate(struct irq_domain *d,
 					   unsigned int *out_type)
 {
 	struct spmi_pmic_arb *pmic_arb = d->host_data;
+	u16 apid, ppid;
 	int rc;
-	u16 apid;
 
 	dev_dbg(&pmic_arb->spmic->dev, "intspec[0] 0x%1x intspec[1] 0x%02x intspec[2] 0x%02x\n",
 		intspec[0], intspec[1], intspec[2]);

@@ -669,14 +671,15 @@ static int qpnpint_irq_domain_dt_translate(struct irq_domain *d,
 	if (intspec[0] > 0xF || intspec[1] > 0xFF || intspec[2] > 0x7)
 		return -EINVAL;
 
-	rc = pmic_arb->ver_ops->ppid_to_apid(pmic_arb, intspec[0],
-					    (intspec[1] << 8), &apid);
+	ppid = intspec[0] << 8 | intspec[1];
+	rc = pmic_arb->ver_ops->ppid_to_apid(pmic_arb, ppid);
 	if (rc < 0) {
 		dev_err(&pmic_arb->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;
 	}
 
+	apid = rc;
 	/* Keep track of {max,min}_apid for bounding search during interrupt */
 	if (apid > pmic_arb->max_apid)
 		pmic_arb->max_apid = apid;

@@ -705,19 +708,18 @@ static int qpnpint_irq_domain_map(struct irq_domain *d,
 	return 0;
 }
 
-static int pmic_arb_ppid_to_apid_v1(struct spmi_pmic_arb *pmic_arb, u8 sid,
-				    u16 addr, u16 *apid)
+static int pmic_arb_ppid_to_apid_v1(struct spmi_pmic_arb *pmic_arb, u16 ppid)
 {
-	u16 ppid = sid << 8 | ((addr >> 8) & 0xFF);
 	u32 *mapping_table = pmic_arb->mapping_table;
 	int index = 0, i;
 	u16 apid_valid;
+	u16 apid;
 	u32 data;
 
 	apid_valid = pmic_arb->ppid_to_apid[ppid];
 	if (apid_valid & PMIC_ARB_APID_VALID) {
-		*apid = apid_valid & ~PMIC_ARB_APID_VALID;
-		return 0;
+		apid = apid_valid & ~PMIC_ARB_APID_VALID;
+		return apid;
 	}
 
 	for (i = 0; i < SPMI_MAPPING_TABLE_TREE_DEPTH; ++i) {

@@ -731,21 +733,21 @@ static int pmic_arb_ppid_to_apid_v1(struct spmi_pmic_arb *pmic_arb, u8 sid,
 			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);
+				apid = SPMI_MAPPING_BIT_IS_1_RESULT(data);
 				pmic_arb->ppid_to_apid[ppid]
-					= *apid | PMIC_ARB_APID_VALID;
-				pmic_arb->apid_data[*apid].ppid = ppid;
-				return 0;
+					= apid | PMIC_ARB_APID_VALID;
+				pmic_arb->apid_data[apid].ppid = ppid;
+				return apid;
 			}
 		} 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);
+				apid = SPMI_MAPPING_BIT_IS_0_RESULT(data);
 				pmic_arb->ppid_to_apid[ppid]
-					= *apid | PMIC_ARB_APID_VALID;
-				pmic_arb->apid_data[*apid].ppid = ppid;
-				return 0;
+					= apid | PMIC_ARB_APID_VALID;
+				pmic_arb->apid_data[apid].ppid = ppid;
+				return apid;
 			}
 		}
 	}

@@ -754,11 +756,9 @@ static int pmic_arb_ppid_to_apid_v1(struct spmi_pmic_arb *pmic_arb, u8 sid,
 }
 
 /* v1 offset per ee */
-static int pmic_arb_offset_v1(struct spmi_pmic_arb *pmic_arb, u8 sid, u16 addr,
-				u32 *offset)
+static int pmic_arb_offset_v1(struct spmi_pmic_arb *pmic_arb, u8 sid, u16 addr)
 {
-	*offset = 0x800 + 0x80 * pmic_arb->channel;
-	return 0;
+	return 0x800 + 0x80 * pmic_arb->channel;
 }
 
 static u16 pmic_arb_find_apid(struct spmi_pmic_arb *pmic_arb, u16 ppid)

@@ -797,10 +797,8 @@ static u16 pmic_arb_find_apid(struct spmi_pmic_arb *pmic_arb, u16 ppid)
 	return apid;
 }
 
-static int pmic_arb_ppid_to_apid_v2(struct spmi_pmic_arb *pmic_arb, u8 sid,
-				    u16 addr, u16 *apid)
+static int pmic_arb_ppid_to_apid_v2(struct spmi_pmic_arb *pmic_arb, u16 ppid)
 {
-	u16 ppid = (sid << 8) | (addr >> 8);
 	u16 apid_valid;
 
 	apid_valid = pmic_arb->ppid_to_apid[ppid];

@@ -809,23 +807,23 @@ static int pmic_arb_ppid_to_apid_v2(struct spmi_pmic_arb *pmic_arb, u8 sid,
 	if (!(apid_valid & PMIC_ARB_APID_VALID))
 		return -ENODEV;
 
-	*apid = apid_valid & ~PMIC_ARB_APID_VALID;
-	return 0;
+	return apid_valid & ~PMIC_ARB_APID_VALID;
 }
 
 /* v2 offset per ppid and per ee */
-static int pmic_arb_offset_v2(struct spmi_pmic_arb *pmic_arb, u8 sid, u16 addr,
-				u32 *offset)
+static int pmic_arb_offset_v2(struct spmi_pmic_arb *pmic_arb, u8 sid, u16 addr)
 {
 	u16 apid;
+	u16 ppid;
 	int rc;
 
-	rc = pmic_arb_ppid_to_apid_v2(pmic_arb, sid, addr, &apid);
+	ppid = sid << 8 | ((addr >> 8) & 0xFF);
+	rc = pmic_arb_ppid_to_apid_v2(pmic_arb, ppid);
 	if (rc < 0)
 		return rc;
 
-	*offset = 0x1000 * pmic_arb->ee + 0x8000 * apid;
-	return 0;
+	apid = rc;
+	return 0x1000 * pmic_arb->ee + 0x8000 * apid;
 }
 
 static u32 pmic_arb_fmt_cmd_v1(u8 opc, u8 sid, u16 addr, u8 bc)
Signature · 2 lines
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project
author
#7 lore raw

PATCH v3 5/5 spmi: pmic-arb: use irq_chip callback to set spmi irq wakeup capability

In reply to Kiran Gunda

Currently the driver sets the pmic arbiter core interrupt as wakeup capable irrespective of the child irqs which causes the system to wakeup unnecessarily. To fix this, set the core interrupt as wakeup capable only if any of the child irqs request for it. Do this by marking it as wakeup capable in the irq_set_wake callback.

Signed-off-by Kiran Gunda <[email protected]>
Reviewed-by Stephen Boyd <[email protected]>
---
 drivers/spmi/spmi-pmic-arb.c | 13 +++++++++----
 1 file changed, 9 insertions(+), 4 deletions(-)
drivers/spmi/spmi-pmic-arb.c +9 -4
index 4795092..4824820 100644
@@ -623,6 +623,13 @@ static int qpnpint_irq_set_type(struct irq_data *d, unsigned int flow_type)
 	return 0;
 }
 
+static int qpnpint_irq_set_wake(struct irq_data *d, unsigned int on)
+{
+	struct spmi_pmic_arb *pmic_arb = irq_data_get_irq_chip_data(d);
+
+	return irq_set_irq_wake(pmic_arb->irq, on);
+}
+
 static int qpnpint_get_irqchip_state(struct irq_data *d,
 				     enum irqchip_irq_state which,
 				     bool *state)

@@ -645,9 +652,9 @@ static int qpnpint_get_irqchip_state(struct irq_data *d,
 	.irq_mask	= qpnpint_irq_mask,
 	.irq_unmask	= qpnpint_irq_unmask,
 	.irq_set_type	= qpnpint_irq_set_type,
+	.irq_set_wake	= qpnpint_irq_set_wake,
 	.irq_get_irqchip_state	= qpnpint_get_irqchip_state,
-	.flags		= IRQCHIP_MASK_ON_SUSPEND
-			| IRQCHIP_SKIP_SET_WAKE,
+	.flags		= IRQCHIP_MASK_ON_SUSPEND,
 };
 
 static int qpnpint_irq_domain_dt_translate(struct irq_domain *d,

@@ -1069,8 +1076,6 @@ static int spmi_pmic_arb_probe(struct platform_device *pdev)
 
 	irq_set_chained_handler_and_data(pmic_arb->irq, pmic_arb_chained_irq,
 					pmic_arb);
-	enable_irq_wake(pmic_arb->irq);
-
 	err = spmi_controller_add(ctrl);
 	if (err)
 		goto err_domain_remove;
Signature · 2 lines
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project