Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S934104AbdGTGoX (ORCPT ); Thu, 20 Jul 2017 02:44:23 -0400 Received: from smtp.codeaurora.org ([198.145.29.96]:32826 "EHLO smtp.codeaurora.org" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S932982AbdGTGoV (ORCPT ); Thu, 20 Jul 2017 02:44:21 -0400 MIME-Version: 1.0 Content-Type: text/plain; charset=US-ASCII; format=flowed Content-Transfer-Encoding: 7bit Date: Thu, 20 Jul 2017 12:14:19 +0530 From: kgunda@codeaurora.org To: Stephen Boyd Cc: gregkh@linuxfoundation.org, Abhijeet Dharmapurikar , David Collins , linux-kernel@vger.kernel.org, linux-arm-msm@vger.kernel.org, linux-arm-msm-owner@vger.kernel.org Subject: Re: [PATCH V3 2/5] spmi: pmic-arb: rename pa_xx to pmic_arb_xx and other code cleanup In-Reply-To: <20170718224629.GB18179@codeaurora.org> References: <1500372632-10527-1-git-send-email-kgunda@codeaurora.org> <1500372632-10527-3-git-send-email-kgunda@codeaurora.org> <20170718224629.GB18179@codeaurora.org> Message-ID: User-Agent: Roundcube Webmail/1.2.5 Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 11938 Lines: 349 On 2017-07-19 04:16, Stephen Boyd wrote: > 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. > ok. will split it up next series. > 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. > Sure. will do it in next patch series. >> >> - 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? > will correct it next patch series. >> 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. ok ... I will modify it as per your current suggestion.