Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1751675AbdFFLKO (ORCPT ); Tue, 6 Jun 2017 07:10:14 -0400 Received: from smtp.codeaurora.org ([198.145.29.96]:36178 "EHLO smtp.codeaurora.org" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751607AbdFFLKM (ORCPT ); Tue, 6 Jun 2017 07:10:12 -0400 MIME-Version: 1.0 Content-Type: text/plain; charset=US-ASCII; format=flowed Content-Transfer-Encoding: 7bit Date: Tue, 06 Jun 2017 16:40:05 +0530 From: kgunda@codeaurora.org To: Stephen Boyd Cc: Abhijeet Dharmapurikar , David Collins , Christophe JAILLET , Subbaraman Narayanamurthy , linux-kernel@vger.kernel.org, linux-arm-msm@vger.kernel.org, adharmap@quicinc.com, aghayal@qti.qualcomm.com, linux-arm-msm-owner@vger.kernel.org Subject: Re: [PATCH V1 10/15] spmi: pmic_arb: add support for PMIC bus arbiter v3 In-Reply-To: <20170531221834.GJ20170@codeaurora.org> References: <1496147943-25822-1-git-send-email-kgunda@codeaurora.org> <1496147943-25822-11-git-send-email-kgunda@codeaurora.org> <20170531221834.GJ20170@codeaurora.org> Message-ID: <0f4c81471de412d946d6fac1c7178adc@codeaurora.org> 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: 3055 Lines: 88 On 2017-06-01 03:48, Stephen Boyd wrote: > On 05/30, Kiran Gunda wrote: >> >> /* PMIC Arbiter channel registers offsets */ >> @@ -96,6 +97,17 @@ 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) \ >> + ((((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) > > How about lowercase and hwirq_to_*() macros? Then it's more > function like style. And spec_to_hwirq() or something? > Sure. Will change it in the follow up patch. >> + >> struct pmic_arb_ver_ops; >> >> struct apid_data { >> @@ -151,7 +163,9 @@ struct spmi_pmic_arb { >> /** >> * pmic_arb_ver: version dependent functionality. >> * >> - * @mode: access rights to specified pmic peripheral. >> + * @ver_str: version string. >> + * @ppid_to_apid: finds the apid for a given ppid. >> + * @mode: access rights to specified pmic peripheral. > > mode didn't need to change tabbing. Not sure why it's appearing > in the diff. > When adding the new field description added extra tab for mode for alignment. That's why it is showing up. >> * @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. >> @@ -177,10 +192,10 @@ struct pmic_arb_ver_ops { >> 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) */ >> - u32 (*owner_acc_status)(u8 m, u8 n); >> - u32 (*acc_enable)(u8 n); >> - u32 (*irq_status)(u8 n); >> - u32 (*irq_clear)(u8 n); >> + u32 (*owner_acc_status)(u8 m, u16 n); >> + u32 (*acc_enable)(u16 n); >> + u32 (*irq_status)(u16 n); >> + u32 (*irq_clear)(u16 n); >> }; >> >> static inline void pmic_arb_base_write(struct spmi_pmic_arb *pa, >> @@ -776,7 +787,7 @@ static int qpnpint_irq_domain_map(struct >> irq_domain *d, >> } >> >> static int >> -pmic_arb_mode_v1(struct spmi_pmic_arb *pa, u8 sid, u16 addr, mode_t >> *mode) >> +pmic_arb_mode_v1_v3(struct spmi_pmic_arb *pa, u8 sid, u16 addr, >> mode_t *mode) > > Nice to know that the mode became useless in v3 and beyond! Sigh. > >> { >> *mode = S_IRUSR | S_IWUSR; >> return 0; >> @@ -987,21 +1017,21 @@ static int spmi_pmic_arb_probe(struct >> platform_device *pdev) >> >> /* the apid to ppid table starts at PMIC_ARB_REG_CHNL(0) */ >> - pa->max_periph = (pa->core_size - PMIC_ARB_REG_CHNL(0)) / 4; >> + pa->max_periph = (pa->core_size - PMIC_ARB_REG_CHNL(0)) / 4; > > This looks unrelated? > Yes. Just removed the extra space after '='. >> >> res = platform_get_resource_byname(pdev, IORESOURCE_MEM, >> "obsrvr");