From mboxrd@z Thu Jan 1 00:00:00 1970 From: Matthias Brugger Subject: Re: [PATCH v4 4/7] soc: mediatek: pwrap: update pwrap_init without slave programming Date: Fri, 13 Oct 2017 16:07:57 +0200 Message-ID: References: <604078485ca9ab04cea5bc531425d6035b27bc88.1505980364.git.sean.wang@mediatek.com> <1507887704.21840.32.camel@mtkswgap22> Mime-Version: 1.0 Content-Type: text/plain; charset=utf-8; format=flowed Content-Transfer-Encoding: 7bit Return-path: In-Reply-To: <1507887704.21840.32.camel@mtkswgap22> Content-Language: en-US Sender: linux-kernel-owner@vger.kernel.org To: Sean Wang Cc: robh+dt@kernel.org, mark.rutland@arm.com, devicetree@vger.kernel.org, linux-mediatek@lists.infradead.org, chen.zhong@mediatek.com, chenglin.xu@mediatek.com, linux-arm-kernel@lists.infradead.org, linux-kernel@vger.kernel.org List-Id: devicetree@vger.kernel.org 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() As from what I understand from the commit log, slv_program in the end enables encryption of the communication, right? Regards, Matthias