On Fri, 2017-10-13 at 16:07 +0200, Matthias Brugger wrote:

On 10/13/2017 11:41 AM, Sean Wang wrote:

quoted

On Tue, 2017-10-10 at 20:00 +0200, Matthias Brugger wrote:

quoted

On 09/21/2017 10:26 AM, sean.wang-NuS5LvNUpcJWk0Htik3J/w@public.gmane.org wrote:

quoted

From: Sean Wang <sean.wang-NuS5LvNUpcJWk0Htik3J/w@public.gmane.org>

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-NuS5LvNUpcJWk0Htik3J/w@public.gmane.org>
Signed-off-by: Chen Zhong <chen.zhong-NuS5LvNUpcJWk0Htik3J/w@public.gmane.org>
Signed-off-by: Sean Wang <sean.wang-NuS5LvNUpcJWk0Htik3J/w@public.gmane.org>
---
   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

--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html