From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1751498AbdJPGWb (ORCPT ); Mon, 16 Oct 2017 02:22:31 -0400 Received: from mailgw02.mediatek.com ([218.249.47.111]:41991 "EHLO mailgw02.mediatek.com" rhost-flags-OK-FAIL-OK-FAIL) by vger.kernel.org with ESMTP id S1750821AbdJPGW3 (ORCPT ); Mon, 16 Oct 2017 02:22:29 -0400 X-UUID: 3c4aa987d79e4abdb1efb955fbbec4f5-20171016 Message-ID: <1508134939.21840.39.camel@mtkswgap22> Subject: Re: [PATCH v4 4/7] soc: mediatek: pwrap: update pwrap_init without slave programming From: Sean Wang To: Matthias Brugger CC: , , , , , , , Date: Mon, 16 Oct 2017 14:22:19 +0800 In-Reply-To: References: <604078485ca9ab04cea5bc531425d6035b27bc88.1505980364.git.sean.wang@mediatek.com> <1507887704.21840.32.camel@mtkswgap22> Content-Type: text/plain; charset="UTF-8" X-Mailer: Evolution 3.2.3-0ubuntu6 Content-Transfer-Encoding: 7bit MIME-Version: 1.0 X-MTK: N Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On Fri, 2017-10-13 at 16:07 +0200, Matthias Brugger wrote: > > On 10/13/2017 11:41 AM, Sean Wang wrote: > > On Tue, 2017-10-10 at 20:00 +0200, Matthias Brugger wrote: > >> > >> On 09/21/2017 10:26 AM, sean.wang@mediatek.com wrote: > >>> From: Sean Wang > >>> > >>> pwrap initialization is highly associated with the base SoC, so > >>> update here for allowing pwrap_init without slave program which would be > >>> used to those PMICs without extra encryption on bus such as MT6380. > >>> > >>> Signed-off-by: Chenglin Xu > >>> Signed-off-by: Chen Zhong > >>> Signed-off-by: Sean Wang > >>> --- > >>> drivers/soc/mediatek/mtk-pmic-wrap.c | 91 +++++++++++++++++++++--------------- > >>> 1 file changed, 54 insertions(+), 37 deletions(-) > >>> > >>> diff --git a/drivers/soc/mediatek/mtk-pmic-wrap.c b/drivers/soc/mediatek/mtk-pmic-wrap.c > >>> index 27d7ccc..9c6d855 100644 > >>> --- a/drivers/soc/mediatek/mtk-pmic-wrap.c > >>> +++ b/drivers/soc/mediatek/mtk-pmic-wrap.c > >>> @@ -531,6 +531,7 @@ struct pmic_wrapper_type { > >>> u32 spi_w; > >>> u32 wdt_src; > >>> int has_bridge:1; > >>> + int slv_program:1; > >>> int (*init_reg_clock)(struct pmic_wrapper *wrp); > >>> int (*init_soc_specific)(struct pmic_wrapper *wrp); > >>> }; > >>> @@ -999,9 +1000,12 @@ static int pwrap_init(struct pmic_wrapper *wrp) > >>> } > >>> > >>> /* Reset SPI slave */ > >>> - ret = pwrap_reset_spislave(wrp); > >>> - if (ret) > >>> - return ret; > >>> + > >>> + if (wrp->master->slv_program) { > >>> + ret = pwrap_reset_spislave(wrp); > >>> + if (ret) > >>> + return ret; > >>> + } > >>> > >>> pwrap_writel(wrp, 1, PWRAP_WRAP_EN); > >>> > >>> @@ -1013,45 +1017,52 @@ static int pwrap_init(struct pmic_wrapper *wrp) > >>> if (ret) > >>> return ret; > >>> > >>> - /* Setup serial input delay */ > >>> - ret = pwrap_init_sidly(wrp); > >>> - if (ret) > >>> - return ret; > >>> + if (wrp->master->slv_program) { > >> > >> This if branch is really long and complex enough to put it into function apart. > >> > >> Thanks, > >> Matthias > >> > >> PD please take into account the comments I made on v3 of the series. > >> > > > > I'll try to breakdown the long logic into the short one and use a flag > > indicating the slave capability decides whether the functions is > > required being enabled for the slave instead of slv_program which is > > less meaningful. In this way, pmic_init will be more extensible when > > more different SoCs and target slaves with various flavors into the > > driver. And also take into accounts those suggestions you made in v3 in > > the next version. > > > > Sean > > > > I totally agree, but I wanted to underline that right now the if branch under > "if (wrp->master->slv_program)" is around 30 lines, so I think it would be a > good candidate to put it into it's own function. For example: > pwrap_init_encryption() understood. I'll try to turn the code block into a function. > As from what I understand from the commit log, slv_program in the end enables > encryption of the communication, right? > yes. slv_program in the end will enable the security feature over the bus such as the encryption and the signature for the data integrity when the pwrap slave can support it. > Regards, > Matthias