> -----Original Message-----
> From: Jakub Kicinski <k...@kernel.org>
> Sent: 2020年12月6日 3:58
> To: Joakim Zhang <qiangqing.zh...@nxp.com>
> Cc: peppe.cavall...@st.com; alexandre.tor...@st.com;
> joab...@synopsys.com; da...@davemloft.net; dl-linux-imx
> <linux-...@nxp.com>; netdev@vger.kernel.org
> Subject: Re: [PATCH] net: stmmac: implement .set_intf_mode() callback for
> imx8dxl
>
> On Thu, 3 Dec 2020 12:10:38 +0800 Joakim Zhang wrote:
> > From: Fugang Duan <fugang.d...@nxp.com>
> >
> > Implement .set_intf_mode() callback for imx8dxl.
> >
> > Signed-off-by: Fugang Duan <fugang.d...@nxp.com>
> > Signed-off-by: Joakim Zhang <qiangqing.zh...@nxp.com>
>
> Couple minor issues.
>
> > @@ -86,7 +88,37 @@ imx8dxl_set_intf_mode(struct
> plat_stmmacenet_data
> > *plat_dat) {
> > int ret = 0;
> >
> > - /* TBD: depends on imx8dxl scu interfaces to be upstreamed */
> > + struct imx_sc_ipc *ipc_handle;
> > + int val;
>
> Looks like you're gonna have a empty line in the middle of variable
> declarations?
>
> Please remove it and order the variable lines longest to shortest.
>
> > +
> > + ret = imx_scu_get_handle(&ipc_handle);
> > + if (ret)
> > + return ret;
> > +
> > + switch (plat_dat->interface) {
> > + case PHY_INTERFACE_MODE_MII:
> > + val = GPR_ENET_QOS_INTF_SEL_MII;
> > + break;
> > + case PHY_INTERFACE_MODE_RMII:
> > + val = GPR_ENET_QOS_INTF_SEL_RMII;
> > + break;
> > + case PHY_INTERFACE_MODE_RGMII:
> > + case PHY_INTERFACE_MODE_RGMII_ID:
> > + case PHY_INTERFACE_MODE_RGMII_RXID:
> > + case PHY_INTERFACE_MODE_RGMII_TXID:
> > + val = GPR_ENET_QOS_INTF_SEL_RGMII;
> > + break;
> > + default:
> > + pr_debug("imx dwmac doesn't support %d interface\n",
> > + plat_dat->interface);
> > + return -EINVAL;
> > + }
> > +
> > + ret = imx_sc_misc_set_control(ipc_handle, IMX_SC_R_ENET_1,
> > + IMX_SC_C_INTF_SEL, val >> 16);
> > + ret |= imx_sc_misc_set_control(ipc_handle, IMX_SC_R_ENET_1,
> > + IMX_SC_C_CLK_GEN_EN, 0x1);
> > return ret;
>
> These calls may return different errors AFAICT.
>
> You can't just errno values to gether the result will be meaningless.
>
> please use the normal flow, and return the result of the second call
> directly:
>
> ret = func1();
> if (ret)
> return ret;
>
> return func2();
>
> Please also CC the maintainers of the Ethernet PHY subsystem on v2, to make
> sure there is nothing wrong with the patch from their PoV.
Thanks Jakub for your kindly review, I will improve patch following your
comments.
Best Regards,
Joakim Zhang
> Thanks!
