Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1752632AbdGRWqe (ORCPT ); Tue, 18 Jul 2017 18:46:34 -0400 Received: from smtp.codeaurora.org ([198.145.29.96]:40282 "EHLO smtp.codeaurora.org" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1752483AbdGRWqc (ORCPT ); Tue, 18 Jul 2017 18:46:32 -0400 DMARC-Filter: OpenDMARC Filter v1.3.2 smtp.codeaurora.org 8904B6072E Authentication-Results: pdx-caf-mail.web.codeaurora.org; dmarc=none (p=none dis=none) header.from=codeaurora.org Authentication-Results: pdx-caf-mail.web.codeaurora.org; spf=none smtp.mailfrom=sboyd@codeaurora.org Date: Tue, 18 Jul 2017 15:46:29 -0700 From: Stephen Boyd To: Kiran Gunda Cc: gregkh@linuxfoundation.org, Abhijeet Dharmapurikar , David Collins , linux-kernel@vger.kernel.org, linux-arm-msm@vger.kernel.org Subject: Re: [PATCH V3 2/5] spmi: pmic-arb: rename pa_xx to pmic_arb_xx and other code cleanup Message-ID: <20170718224629.GB18179@codeaurora.org> References: <1500372632-10527-1-git-send-email-kgunda@codeaurora.org> <1500372632-10527-3-git-send-email-kgunda@codeaurora.org> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <1500372632-10527-3-git-send-email-kgunda@codeaurora.org> User-Agent: Mutt/1.5.21 (2010-09-15) Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 11287 Lines: 327 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