Re: [U-Boot] [PATCH v4 2/4] armv8: ls2088aqds: The ls2088aqds board supports the I2C driver model.
++ Xiaowei Bao (he is taking over from Chuanhua)
> -Original Message-
> From: Wolfgang Denk
> Sent: Friday, August 23, 2019 3:05 PM
> To: Chuanhua Han
> Cc: albert.u.b...@aribaud.net; Prabhakar Kushwaha
> ; Priyanka Jain ;
> Rajesh Bhagat ; u-boot@lists.denx.de;
> lu...@denx.de; tr...@konsulko.com
> Subject: Re: [PATCH v4 2/4] armv8: ls2088aqds: The ls2088aqds board supports
> the I2C driver model.
>
> Dear Chuanhua Han,
>
> In message <20190726112403.32842-2-chuanhua@nxp.com> you wrote:
> > This patch is updating ls2088aqds board init code to support DM_I2C.
> ...
> > +struct reg_pair {
> > + uint addr;
> > + u8 *val;
> > +};
> > +
> > static void sgmii_configure_repeater(int serdes_port) {
> > struct mii_dev *bus;
> > uint8_t a = 0xf;
> > - int i, j, ret;
> > + int i, j, k, ret;
> > int dpmac_id = 0, dpmac, mii_bus = 0;
> > unsigned short value;
> > char dev[2][20] = {"LS2080A_QDS_MDIO0", "LS2080A_QDS_MDIO3"};
> @@
> > -104,10 +109,30 @@ static void sgmii_configure_repeater(int serdes_port)
> > uint8_t ch_b_eq[] = {0x1, 0x2, 0x3, 0x7};
> > uint8_t ch_b_ctl2[] = {0x81, 0x82, 0x83, 0x84};
> >
> > + u8 reg_val[6] = {0x18, 0x38, 0x4, 0x14, 0xb5, 0x20};
> > + struct reg_pair reg_pair[10] = {
> > + {6, _val[0]}, {4, _val[1]},
> > + {8, _val[2]}, {0xf, NULL},
> > + {0x11, NULL}, {0x16, NULL},
> > + {0x18, NULL}, {0x23, _val[3]},
> > + {0x2d, _val[4]}, {4, _val[5]},
> > + };
>
> Argh... this is pretty much unreadable. Why do you use a pointer for "val" in
> your struct reg_pair?
>
> Would it not be much simpler to use something like this:
>
> struct reg_pair {
> uintaddr;
> u8 val;
> }
> ...
> struct reg_pair reg_pair[] = {
> {0x06, 0x18},
> {0x04, 0x38},
> {0x08, 0x04},
> {0x0F, 0},
> {0x11, 0},
> {0x16, 0},
> {0x18, 0},
> {0x23, 0x14},
> {0x2D, 0xB5},
> {0x04, 0x20},
> }}
> ?
>
> Also, you should add some comments what all these magic numbers mean. As is,
> nobody is able to understand what you are doing here.
> This must be explained!
>
> Also, a comment should be added that (and why) some of the values get inserted
> dynamically later down in the code.
>
>
Dear Wolfgang,
Unfortunately this patch has been merged in main-line.
But I will ask (Xiaowei Bao) to send patch to fix it. I will make sure to get
fix in RC3 or RC4.
--pk
___
U-Boot mailing list
U-Boot@lists.denx.de
https://lists.denx.de/listinfo/u-boot
Re: [U-Boot] [PATCH v4 2/4] armv8: ls2088aqds: The ls2088aqds board supports the I2C driver model.
Dear Chuanhua Han,
In message <20190726112403.32842-2-chuanhua@nxp.com> you wrote:
> This patch is updating ls2088aqds board init code to support DM_I2C.
...
> +struct reg_pair {
> + uint addr;
> + u8 *val;
> +};
> +
> static void sgmii_configure_repeater(int serdes_port)
> {
> struct mii_dev *bus;
> uint8_t a = 0xf;
> - int i, j, ret;
> + int i, j, k, ret;
> int dpmac_id = 0, dpmac, mii_bus = 0;
> unsigned short value;
> char dev[2][20] = {"LS2080A_QDS_MDIO0", "LS2080A_QDS_MDIO3"};
> @@ -104,10 +109,30 @@ static void sgmii_configure_repeater(int serdes_port)
> uint8_t ch_b_eq[] = {0x1, 0x2, 0x3, 0x7};
> uint8_t ch_b_ctl2[] = {0x81, 0x82, 0x83, 0x84};
>
> + u8 reg_val[6] = {0x18, 0x38, 0x4, 0x14, 0xb5, 0x20};
> + struct reg_pair reg_pair[10] = {
> + {6, _val[0]}, {4, _val[1]},
> + {8, _val[2]}, {0xf, NULL},
> + {0x11, NULL}, {0x16, NULL},
> + {0x18, NULL}, {0x23, _val[3]},
> + {0x2d, _val[4]}, {4, _val[5]},
> + };
Argh... this is pretty much unreadable. Why do you use a pointer
for "val" in your struct reg_pair?
Would it not be much simpler to use something like this:
struct reg_pair {
uintaddr;
u8 val;
}
...
struct reg_pair reg_pair[] = {
{0x06, 0x18},
{0x04, 0x38},
{0x08, 0x04},
{0x0F, 0},
{0x11, 0},
{0x16, 0},
{0x18, 0},
{0x23, 0x14},
{0x2D, 0xB5},
{0x04, 0x20},
}}
?
Also, you should add some comments what all these magic numbers
mean. As is, nobody is able to understand what you are doing here.
This must be explained!
Also, a comment should be added that (and why) some of the values get
inserted dynamically later down in the code.
Best regards,
Wolfgang Denk
--
DENX Software Engineering GmbH, Managing Director: Wolfgang Denk
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: w...@denx.de
The easiest way to figure the cost of living is to take your income
and add ten percent.
___
U-Boot mailing list
U-Boot@lists.denx.de
https://lists.denx.de/listinfo/u-boot
Re: [U-Boot] [PATCH v4 2/4] armv8: ls2088aqds: The ls2088aqds board supports the I2C driver model.
> -Original Message- > From: Chuanhua Han > Sent: Friday, July 26, 2019 4:54 PM > To: albert.u.b...@aribaud.net; Prabhakar Kushwaha > ; Priyanka Jain ; > Rajesh Bhagat > Cc: u-boot@lists.denx.de; w...@denx.de; lu...@denx.de; > tr...@konsulko.com; Chuanhua Han > Subject: [PATCH v4 2/4] armv8: ls2088aqds: The ls2088aqds board supports > the I2C driver model. > > This patch is updating ls2088aqds board init code to support DM_I2C. > > Signed-off-by: Chuanhua Han > --- This patch has been applied to fsl-qoriq master, awaiting upstream. --pk ___ U-Boot mailing list U-Boot@lists.denx.de https://lists.denx.de/listinfo/u-boot
