linux-kernel - Re: [PATCH v4 4/7] soc: mediatek: pwrap: update pwrap

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <1507887704.21840.32.camel@mtkswgap22>
Date:   Fri, 13 Oct 2017 17:41:44 +0800
From:   Sean Wang <sean.wang@...iatek.com>
To:     Matthias Brugger <matthias.bgg@...il.com>
CC:     <robh+dt@...nel.org>, <mark.rutland@....com>,
        <devicetree@...r.kernel.org>, <linux-mediatek@...ts.infradead.org>,
        <chen.zhong@...iatek.com>, <chenglin.xu@...iatek.com>,
        <linux-arm-kernel@...ts.infradead.org>,
        <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH v4 4/7] soc: mediatek: pwrap: update pwrap_init without
 slave programming

On Tue, 2017-10-10 at 20:00 +0200, Matthias Brugger wrote:
> 
> On 09/21/2017 10:26 AM, sean.wang@...iatek.com wrote:
> > From: Sean Wang <sean.wang@...iatek.com>
> > 
> > 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 <chenglin.xu@...iatek.com>
> > Signed-off-by: Chen Zhong <chen.zhong@...iatek.com>
> > Signed-off-by: Sean Wang <sean.wang@...iatek.com>
> > ---
> >   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