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

[Stephen Boyd]

On 07/18, Kiran Gunda wrote:
> 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.

diff --git a/drivers/spmi/spmi-pmic-arb.c b/drivers/spmi/spmi-pmic-arb.c
index 91b068b1c83d..2e34dd66feec 100644
--- a/drivers/spmi/spmi-pmic-arb.c
+++ b/drivers/spmi/spmi-pmic-arb.c
@@ -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;

> -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.

>  
> -	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));

This still uses pmic_arb->ver_ops instead of ver_ops?

>  			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);
>  
> -	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.

-- 
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
a Linux Foundation Collaborative Project