* Re: [PATCH v3 1/9] davinci: EMAC support for Omapl138-Hawkboard
[not found] ` <1287076899-4365-1-git-send-email-vm.rod25-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
@ 2010-10-14 17:16 ` Sergei Shtylyov
2010-10-14 21:44 ` Victor Rodriguez
0 siblings, 1 reply; 5+ messages in thread
From: Sergei Shtylyov @ 2010-10-14 17:16 UTC (permalink / raw)
To: vm.rod25-Re5JQEeQqe8AvxtiuMwx3w
Cc: davinci-linux-open-source-VycZQUHpC/PFrsHnngEfi1aTQe2KTcn/,
alsa-devel-K7yf7f+aM1XWsZ/bQMPhNw
Hello.
On 10/14/10 21:21, vm.rod25-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org wrote:
> From: Victor Rodriguez<vm.rod25-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
> This patch adds EMAC support for the Hawkboard-L138 system
> Signed-off-by: Victor Rodriguez<victor.rodriguez-Ut8ZLSGP1ULQT0dZR+AlfA@public.gmane.org>
> ---
> arch/arm/mach-davinci/board-omapl138-hawk.c | 49 +++++++++++++++++++++++++++
> 1 files changed, 49 insertions(+), 0 deletions(-)
> diff --git a/arch/arm/mach-davinci/board-omapl138-hawk.c b/arch/arm/mach-davinci/board-omapl138-hawk.c
> index c472dd8..3ae5178 100644
> --- a/arch/arm/mach-davinci/board-omapl138-hawk.c
> +++ b/arch/arm/mach-davinci/board-omapl138-hawk.c
> @@ -19,6 +19,53 @@
>
> #include<mach/cp_intc.h>
> #include<mach/da8xx.h>
> +#include<mach/mux.h>
> +
> +#define HAWKBOARD_PHY_ID "0:07"
> +
> +static short omapl138_hawk_mii_pins[] __initdata = {
> + DA850_MII_TXEN, DA850_MII_TXCLK, DA850_MII_COL, DA850_MII_TXD_3,
> + DA850_MII_TXD_2, DA850_MII_TXD_1, DA850_MII_TXD_0, DA850_MII_RXER,
> + DA850_MII_CRS, DA850_MII_RXCLK, DA850_MII_RXDV, DA850_MII_RXD_3,
> + DA850_MII_RXD_2, DA850_MII_RXD_1, DA850_MII_RXD_0, DA850_MDIO_CLK,
> + DA850_MDIO_D,
> + -1
> +};
> +
> +static int __init omapl138_hawk_config_emac(void)
> +{
> + void __iomem *cfgchip3;
> + int ret;
> + u32 val;
> + struct davinci_soc_info *soc_info =&davinci_soc_info;
> +
> + if (!machine_is_omapl138_hawkboard())
> + return 0;
> +
> + cfgchip3 = DA8XSYSCFG0_VIRT(DA8XX_CFGCHIP3_REG);
Could be initializer...
> + val = __raw_readl(cfgchip3);
> +
> + val &= ~BIT(8);
> + ret = davinci_cfg_reg_list(omapl138_hawk_mii_pins);
> + pr_info("EMAC: MII PHY configured\n");
I think this pr_info() should follow __raw_writel() call.
> + if (ret)
> + pr_warning("%s: "
> + "cpgmac/mii mux setup failed: %d\n", __func__, ret);
You should return here.
> +
> + /* configure the CFGCHIP3 register for MII */
> + __raw_writel(val, cfgchip3);
> +
> + soc_info->emac_pdata->phy_id = HAWKBOARD_PHY_ID;
> +
> + ret = da8xx_register_emac();
> + if (ret)
> + pr_warning("%s: "
> + "emac registration failed: %d\n", __func__, ret);
> + return 0;
Why this function returns anything at all if it'a always 0, and the result
is ignored?
> @@ -30,6 +77,8 @@ static __init void omapl138_hawk_init(void)
>
> davinci_serial_init(&omapl138_hawk_uart_config);
>
> + ret = omapl138_hawk_config_emac();
Why assign 'ret', if you're ignoring the result?
> +
> ret = da8xx_register_watchdog();
> if (ret)
> pr_warning("omapl138_hawk_init: "
WBR, Sergei
^ permalink raw reply [flat|nested] 5+ messages in thread
* [PATCH v3 1/9] davinci: EMAC support for Omapl138-Hawkboard
@ 2010-10-14 17:21 vm.rod25
[not found] ` <1287076899-4365-1-git-send-email-vm.rod25-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
0 siblings, 1 reply; 5+ messages in thread
From: vm.rod25 @ 2010-10-14 17:21 UTC (permalink / raw)
To: davinci-linux-open-source
Cc: nsekhar, alsa-devel, khasim, caglarakyuz, sshtylyov
From: Victor Rodriguez <vm.rod25@gmail.com>
This patch adds EMAC support for the Hawkboard-L138 system
Signed-off-by: Victor Rodriguez <victor.rodriguez@sasken.com>
---
arch/arm/mach-davinci/board-omapl138-hawk.c | 49 +++++++++++++++++++++++++++
1 files changed, 49 insertions(+), 0 deletions(-)
diff --git a/arch/arm/mach-davinci/board-omapl138-hawk.c b/arch/arm/mach-davinci/board-omapl138-hawk.c
index c472dd8..3ae5178 100644
--- a/arch/arm/mach-davinci/board-omapl138-hawk.c
+++ b/arch/arm/mach-davinci/board-omapl138-hawk.c
@@ -19,6 +19,53 @@
#include <mach/cp_intc.h>
#include <mach/da8xx.h>
+#include <mach/mux.h>
+
+#define HAWKBOARD_PHY_ID "0:07"
+
+static short omapl138_hawk_mii_pins[] __initdata = {
+ DA850_MII_TXEN, DA850_MII_TXCLK, DA850_MII_COL, DA850_MII_TXD_3,
+ DA850_MII_TXD_2, DA850_MII_TXD_1, DA850_MII_TXD_0, DA850_MII_RXER,
+ DA850_MII_CRS, DA850_MII_RXCLK, DA850_MII_RXDV, DA850_MII_RXD_3,
+ DA850_MII_RXD_2, DA850_MII_RXD_1, DA850_MII_RXD_0, DA850_MDIO_CLK,
+ DA850_MDIO_D,
+ -1
+};
+
+static int __init omapl138_hawk_config_emac(void)
+{
+ void __iomem *cfgchip3;
+ int ret;
+ u32 val;
+ struct davinci_soc_info *soc_info = &davinci_soc_info;
+
+ if (!machine_is_omapl138_hawkboard())
+ return 0;
+
+ cfgchip3 = DA8XX_SYSCFG0_VIRT(DA8XX_CFGCHIP3_REG);
+
+ val = __raw_readl(cfgchip3);
+
+ val &= ~BIT(8);
+ ret = davinci_cfg_reg_list(omapl138_hawk_mii_pins);
+ pr_info("EMAC: MII PHY configured\n");
+
+ if (ret)
+ pr_warning("%s: "
+ "cpgmac/mii mux setup failed: %d\n", __func__, ret);
+
+ /* configure the CFGCHIP3 register for MII */
+ __raw_writel(val, cfgchip3);
+
+ soc_info->emac_pdata->phy_id = HAWKBOARD_PHY_ID;
+
+ ret = da8xx_register_emac();
+ if (ret)
+ pr_warning("%s: "
+ "emac registration failed: %d\n", __func__, ret);
+ return 0;
+}
+
static struct davinci_uart_config omapl138_hawk_uart_config __initdata = {
.enabled_uarts = 0x7,
@@ -30,6 +77,8 @@ static __init void omapl138_hawk_init(void)
davinci_serial_init(&omapl138_hawk_uart_config);
+ ret = omapl138_hawk_config_emac();
+
ret = da8xx_register_watchdog();
if (ret)
pr_warning("omapl138_hawk_init: "
--
1.6.0.5
^ permalink raw reply related [flat|nested] 5+ messages in thread
* Re: [PATCH v3 1/9] davinci: EMAC support for Omapl138-Hawkboard
2010-10-14 17:16 ` Sergei Shtylyov
@ 2010-10-14 21:44 ` Victor Rodriguez
[not found] ` <AANLkTikaDB1Cf9T5W-ogbQgEvGpmhYjJ8YHa3UftFTjL-JsoAwUIsXosN+BqQ9rBEUg@public.gmane.org>
0 siblings, 1 reply; 5+ messages in thread
From: Victor Rodriguez @ 2010-10-14 21:44 UTC (permalink / raw)
To: Sergei Shtylyov
Cc: nsekhar, davinci-linux-open-source, khasim, alsa-devel,
caglarakyuz
On Thu, Oct 14, 2010 at 12:16 PM, Sergei Shtylyov <sshtylyov@mvista.com> wrote:
> Hello.
>
> On 10/14/10 21:21, vm.rod25@gmail.com wrote:
>
>> From: Victor Rodriguez<vm.rod25@gmail.com>
>
>> This patch adds EMAC support for the Hawkboard-L138 system
>
>> Signed-off-by: Victor Rodriguez<victor.rodriguez@sasken.com>
>> ---
>> arch/arm/mach-davinci/board-omapl138-hawk.c | 49
>> +++++++++++++++++++++++++++
>> 1 files changed, 49 insertions(+), 0 deletions(-)
>
>> diff --git a/arch/arm/mach-davinci/board-omapl138-hawk.c
>> b/arch/arm/mach-davinci/board-omapl138-hawk.c
>> index c472dd8..3ae5178 100644
>> --- a/arch/arm/mach-davinci/board-omapl138-hawk.c
>> +++ b/arch/arm/mach-davinci/board-omapl138-hawk.c
>> @@ -19,6 +19,53 @@
>>
>> #include<mach/cp_intc.h>
>> #include<mach/da8xx.h>
>> +#include<mach/mux.h>
>> +
>> +#define HAWKBOARD_PHY_ID "0:07"
>> +
>> +static short omapl138_hawk_mii_pins[] __initdata = {
>> + DA850_MII_TXEN, DA850_MII_TXCLK, DA850_MII_COL, DA850_MII_TXD_3,
>> + DA850_MII_TXD_2, DA850_MII_TXD_1, DA850_MII_TXD_0, DA850_MII_RXER,
>> + DA850_MII_CRS, DA850_MII_RXCLK, DA850_MII_RXDV, DA850_MII_RXD_3,
>> + DA850_MII_RXD_2, DA850_MII_RXD_1, DA850_MII_RXD_0, DA850_MDIO_CLK,
>> + DA850_MDIO_D,
>> + -1
>> +};
>> +
>> +static int __init omapl138_hawk_config_emac(void)
>> +{
>> + void __iomem *cfgchip3;
>> + int ret;
>> + u32 val;
>> + struct davinci_soc_info *soc_info =&davinci_soc_info;
>> +
>> + if (!machine_is_omapl138_hawkboard())
>> + return 0;
>> +
>> + cfgchip3 = DA8XSYSCFG0_VIRT(DA8XX_CFGCHIP3_REG);
>
> Could be initializer...
I do not understand this
>> + val = __raw_readl(cfgchip3);
>> +
>> + val &= ~BIT(8);
>> + ret = davinci_cfg_reg_list(omapl138_hawk_mii_pins);
>> + pr_info("EMAC: MII PHY configured\n");
>
> I think this pr_info() should follow __raw_writel() call.
Ok
>> + if (ret)
>> + pr_warning("%s: "
>> + "cpgmac/mii mux setup failed: %d\n", __func__,
>> ret);
>
> You should return here.
>
>> +
>> + /* configure the CFGCHIP3 register for MII */
>> + __raw_writel(val, cfgchip3);
>> +
>> + soc_info->emac_pdata->phy_id = HAWKBOARD_PHY_ID;
>> +
>> + ret = da8xx_register_emac();
>> + if (ret)
>> + pr_warning("%s: "
>> + "emac registration failed: %d\n", __func__, ret);
>> + return 0;
>
> Why this function returns anything at all if it'a always 0, and the result
> is ignored?
>
>> @@ -30,6 +77,8 @@ static __init void omapl138_hawk_init(void)
>>
>> davinci_serial_init(&omapl138_hawk_uart_config);
>>
>> + ret = omapl138_hawk_config_emac();
>
> Why assign 'ret', if you're ignoring the result?
Sorry my mistake I forgot to check this, please check the nest patch
maybe it is a better implementation
---
arch/arm/mach-davinci/board-omapl138-hawk.c | 49 +++++++++++++++++++++++++++
1 files changed, 49 insertions(+), 0 deletions(-)
diff --git a/arch/arm/mach-davinci/board-omapl138-hawk.c
b/arch/arm/mach-davinci/board-omapl138-hawk.c
index c472dd8..35bcea1 100644
--- a/arch/arm/mach-davinci/board-omapl138-hawk.c
+++ b/arch/arm/mach-davinci/board-omapl138-hawk.c
@@ -19,6 +19,53 @@
#include <mach/cp_intc.h>
#include <mach/da8xx.h>
+#include <mach/mux.h>
+
+#define HAWKBOARD_PHY_ID "0:07"
+
+static short omapl138_hawk_mii_pins[] __initdata = {
+ DA850_MII_TXEN, DA850_MII_TXCLK, DA850_MII_COL, DA850_MII_TXD_3,
+ DA850_MII_TXD_2, DA850_MII_TXD_1, DA850_MII_TXD_0, DA850_MII_RXER,
+ DA850_MII_CRS, DA850_MII_RXCLK, DA850_MII_RXDV, DA850_MII_RXD_3,
+ DA850_MII_RXD_2, DA850_MII_RXD_1, DA850_MII_RXD_0, DA850_MDIO_CLK,
+ DA850_MDIO_D,
+ -1
+};
+
+static __init void omapl138_hawk_config_emac(void)
+{
+ void __iomem *cfgchip3;
+ int ret;
+ u32 val;
+ struct davinci_soc_info *soc_info = &davinci_soc_info;
+
+ if (!machine_is_omapl138_hawkboard())
+ return;
+
+ cfgchip3 = DA8XX_SYSCFG0_VIRT(DA8XX_CFGCHIP3_REG);
+
+ val = __raw_readl(cfgchip3);
+
+ val &= ~BIT(8);
+ ret = davinci_cfg_reg_list(omapl138_hawk_mii_pins);
+ if (ret)
+ pr_warning("%s: "
+ "cpgmac/mii mux setup failed: %d\n", __func__, ret);
+ return;
+
+ /* configure the CFGCHIP3 register for MII */
+ __raw_writel(val, cfgchip3);
+ pr_info("EMAC: MII PHY configured\n");
+
+ soc_info->emac_pdata->phy_id = HAWKBOARD_PHY_ID;
+
+ ret = da8xx_register_emac();
+ if (ret)
+ pr_warning("%s: "
+ "emac registration failed: %d\n", __func__, ret);
+ return;
+}
+
static struct davinci_uart_config omapl138_hawk_uart_config __initdata = {
.enabled_uarts = 0x7,
@@ -30,6 +77,8 @@ static __init void omapl138_hawk_init(void)
davinci_serial_init(&omapl138_hawk_uart_config);
+ omapl138_hawk_config_emac();
+
ret = da8xx_register_watchdog();
if (ret)
pr_warning("omapl138_hawk_init: "
--
1.6.0.5
Thanks for all
Regards
Victor Rodriguez
>> +
>> ret = da8xx_register_watchdog();
>> if (ret)
>> pr_warning("omapl138_hawk_init: "
>
> WBR, Sergei
>
^ permalink raw reply related [flat|nested] 5+ messages in thread
* Re: [PATCH v3 1/9] davinci: EMAC support for Omapl138-Hawkboard
[not found] ` <AANLkTikaDB1Cf9T5W-ogbQgEvGpmhYjJ8YHa3UftFTjL-JsoAwUIsXosN+BqQ9rBEUg@public.gmane.org>
@ 2010-10-15 16:25 ` Sergei Shtylyov
2010-10-18 23:58 ` Victor Rodriguez
0 siblings, 1 reply; 5+ messages in thread
From: Sergei Shtylyov @ 2010-10-15 16:25 UTC (permalink / raw)
To: Victor Rodriguez
Cc: davinci-linux-open-source-VycZQUHpC/PFrsHnngEfi1aTQe2KTcn/,
alsa-devel-K7yf7f+aM1XWsZ/bQMPhNw
Hello.
On 10/15/10 01:44, Victor Rodriguez wrote:
>>> From: Victor Rodriguez<vm.rod25-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
>>> This patch adds EMAC support for the Hawkboard-L138 system
>>> Signed-off-by: Victor Rodriguez<victor.rodriguez-Ut8ZLSGP1ULQT0dZR+AlfA@public.gmane.org>
[...]
>>> diff --git a/arch/arm/mach-davinci/board-omapl138-hawk.c
>>> b/arch/arm/mach-davinci/board-omapl138-hawk.c
>>> index c472dd8..3ae5178 100644
>>> --- a/arch/arm/mach-davinci/board-omapl138-hawk.c
>>> +++ b/arch/arm/mach-davinci/board-omapl138-hawk.c
>>> @@ -19,6 +19,53 @@
>>>
>>> #include<mach/cp_intc.h>
>>> #include<mach/da8xx.h>
>>> +#include<mach/mux.h>
>>> +
>>> +#define HAWKBOARD_PHY_ID "0:07"
>>> +
>>> +static short omapl138_hawk_mii_pins[] __initdata = {
>>> + DA850_MII_TXEN, DA850_MII_TXCLK, DA850_MII_COL, DA850_MII_TXD_3,
>>> + DA850_MII_TXD_2, DA850_MII_TXD_1, DA850_MII_TXD_0, DA850_MII_RXER,
>>> + DA850_MII_CRS, DA850_MII_RXCLK, DA850_MII_RXDV, DA850_MII_RXD_3,
>>> + DA850_MII_RXD_2, DA850_MII_RXD_1, DA850_MII_RXD_0, DA850_MDIO_CLK,
>>> + DA850_MDIO_D,
>>> + -1
>>> +};
>>> +
>>> +static int __init omapl138_hawk_config_emac(void)
>>> +{
>>> + void __iomem *cfgchip3;
>>> + int ret;
>>> + u32 val;
>>> + struct davinci_soc_info *soc_info =&davinci_soc_info;
>>> +
>>> + if (!machine_is_omapl138_hawkboard())
>>> + return 0;
>>> +
>>> + cfgchip3 = DA8XSYSCFG0_VIRT(DA8XX_CFGCHIP3_REG);
>> Could be initializer...
> I do not understand this
I mean you could assign 'cfgchip3' right when you declare it.
>>> + val = __raw_readl(cfgchip3);
>>> +
>>> + val&= ~BIT(8);
>>> + ret = davinci_cfg_reg_list(omapl138_hawk_mii_pins);
>>> + pr_info("EMAC: MII PHY configured\n");
>> I think this pr_info() should follow __raw_writel() call.
> Ok
>>> + if (ret)
>>> + pr_warning("%s: "
>>> + "cpgmac/mii mux setup failed: %d\n", __func__,
>>> ret);
>> You should return here.
>>> +
>>> + /* configure the CFGCHIP3 register for MII */
>>> + __raw_writel(val, cfgchip3);
>>> +
>>> + soc_info->emac_pdata->phy_id = HAWKBOARD_PHY_ID;
>>> +
>>> + ret = da8xx_register_emac();
>>> + if (ret)
>>> + pr_warning("%s: "
>>> + "emac registration failed: %d\n", __func__, ret);
>>> + return 0;
>> Why this function returns anything at all if it'a always 0, and the result
>> is ignored?
>>> @@ -30,6 +77,8 @@ static __init void omapl138_hawk_init(void)
>>>
>>> davinci_serial_init(&omapl138_hawk_uart_config);
>>> + ret = omapl138_hawk_config_emac();
>>
>> Why assign 'ret', if you're ignoring the result?
> Sorry my mistake I forgot to check this, please check the nest patch
> maybe it is a better implementation
[...]
> diff --git a/arch/arm/mach-davinci/board-omapl138-hawk.c
> b/arch/arm/mach-davinci/board-omapl138-hawk.c
> index c472dd8..35bcea1 100644
> --- a/arch/arm/mach-davinci/board-omapl138-hawk.c
> +++ b/arch/arm/mach-davinci/board-omapl138-hawk.c
> @@ -19,6 +19,53 @@
>
> #include<mach/cp_intc.h>
> #include<mach/da8xx.h>
> +#include<mach/mux.h>
> +
> +#define HAWKBOARD_PHY_ID "0:07"
> +
> +static short omapl138_hawk_mii_pins[] __initdata = {
> + DA850_MII_TXEN, DA850_MII_TXCLK, DA850_MII_COL, DA850_MII_TXD_3,
> + DA850_MII_TXD_2, DA850_MII_TXD_1, DA850_MII_TXD_0, DA850_MII_RXER,
> + DA850_MII_CRS, DA850_MII_RXCLK, DA850_MII_RXDV, DA850_MII_RXD_3,
> + DA850_MII_RXD_2, DA850_MII_RXD_1, DA850_MII_RXD_0, DA850_MDIO_CLK,
> + DA850_MDIO_D,
> + -1
> +};
> +
> +static __init void omapl138_hawk_config_emac(void)
> +{
> + void __iomem *cfgchip3;
> + int ret;
> + u32 val;
> + struct davinci_soc_info *soc_info =&davinci_soc_info;
> +
> + if (!machine_is_omapl138_hawkboard())
> + return;
> +
> + cfgchip3 = DA8XX_SYSCFG0_VIRT(DA8XX_CFGCHIP3_REG);
> +
> + val = __raw_readl(cfgchip3);
> +
> + val&= ~BIT(8);
> + ret = davinci_cfg_reg_list(omapl138_hawk_mii_pins);
> + if (ret)
> + pr_warning("%s: "
> + "cpgmac/mii mux setup failed: %d\n", __func__, ret);
> + return;
Should enclose these two statements in {} and fix indentation.
> +
> + /* configure the CFGCHIP3 register for MII */
> + __raw_writel(val, cfgchip3);
> + pr_info("EMAC: MII PHY configured\n");
> +
> + soc_info->emac_pdata->phy_id = HAWKBOARD_PHY_ID;
> +
> + ret = da8xx_register_emac();
> + if (ret)
> + pr_warning("%s: "
> + "emac registration failed: %d\n", __func__, ret);
> + return;
'return' is useless here, and it should've been enclosed into {}...
WBR. Sergei
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH v3 1/9] davinci: EMAC support for Omapl138-Hawkboard
2010-10-15 16:25 ` Sergei Shtylyov
@ 2010-10-18 23:58 ` Victor Rodriguez
0 siblings, 0 replies; 5+ messages in thread
From: Victor Rodriguez @ 2010-10-18 23:58 UTC (permalink / raw)
To: Sergei Shtylyov
Cc: nsekhar, davinci-linux-open-source, khasim, alsa-devel,
caglarakyuz
On Fri, Oct 15, 2010 at 11:25 AM, Sergei Shtylyov <sshtylyov@mvista.com> wrote:
> Hello.
>
> On 10/15/10 01:44, Victor Rodriguez wrote:
>
>>>> From: Victor Rodriguez<vm.rod25@gmail.com>
>
>>>> This patch adds EMAC support for the Hawkboard-L138 system
>
>>>> Signed-off-by: Victor Rodriguez<victor.rodriguez@sasken.com>
>
> [...]
>
>>>> diff --git a/arch/arm/mach-davinci/board-omapl138-hawk.c
>>>> b/arch/arm/mach-davinci/board-omapl138-hawk.c
>>>> index c472dd8..3ae5178 100644
>>>> --- a/arch/arm/mach-davinci/board-omapl138-hawk.c
>>>> +++ b/arch/arm/mach-davinci/board-omapl138-hawk.c
>>>> @@ -19,6 +19,53 @@
>>>>
>>>> #include<mach/cp_intc.h>
>>>> #include<mach/da8xx.h>
>>>> +#include<mach/mux.h>
>>>> +
>>>> +#define HAWKBOARD_PHY_ID "0:07"
>>>> +
>>>> +static short omapl138_hawk_mii_pins[] __initdata = {
>>>> + DA850_MII_TXEN, DA850_MII_TXCLK, DA850_MII_COL, DA850_MII_TXD_3,
>>>> + DA850_MII_TXD_2, DA850_MII_TXD_1, DA850_MII_TXD_0,
>>>> DA850_MII_RXER,
>>>> + DA850_MII_CRS, DA850_MII_RXCLK, DA850_MII_RXDV, DA850_MII_RXD_3,
>>>> + DA850_MII_RXD_2, DA850_MII_RXD_1, DA850_MII_RXD_0,
>>>> DA850_MDIO_CLK,
>>>> + DA850_MDIO_D,
>>>> + -1
>>>> +};
>>>> +
>>>> +static int __init omapl138_hawk_config_emac(void)
>>>> +{
>>>> + void __iomem *cfgchip3;
>>>> + int ret;
>>>> + u32 val;
>>>> + struct davinci_soc_info *soc_info =&davinci_soc_info;
>>>> +
>>>> + if (!machine_is_omapl138_hawkboard())
>>>> + return 0;
>>>> +
>>>> + cfgchip3 = DA8XSYSCFG0_VIRT(DA8XX_CFGCHIP3_REG);
>
>>> Could be initializer...
>
>> I do not understand this
>
> I mean you could assign 'cfgchip3' right when you declare it.
>
>>>> + val = __raw_readl(cfgchip3);
>>>> +
>>>> + val&= ~BIT(8);
>>>> + ret = davinci_cfg_reg_list(omapl138_hawk_mii_pins);
>>>> + pr_info("EMAC: MII PHY configured\n");
>
>>> I think this pr_info() should follow __raw_writel() call.
>
>> Ok
>
>>>> + if (ret)
>>>> + pr_warning("%s: "
>>>> + "cpgmac/mii mux setup failed: %d\n", __func__,
>>>> ret);
>
>>> You should return here.
>
>>>> +
>>>> + /* configure the CFGCHIP3 register for MII */
>>>> + __raw_writel(val, cfgchip3);
>>>> +
>>>> + soc_info->emac_pdata->phy_id = HAWKBOARD_PHY_ID;
>>>> +
>>>> + ret = da8xx_register_emac();
>>>> + if (ret)
>>>> + pr_warning("%s: "
>>>> + "emac registration failed: %d\n", __func__,
>>>> ret);
>>>> + return 0;
>
>>> Why this function returns anything at all if it'a always 0, and the
>>> result
>>> is ignored?
>
>>>> @@ -30,6 +77,8 @@ static __init void omapl138_hawk_init(void)
>>>>
>>>> davinci_serial_init(&omapl138_hawk_uart_config);
>
>>>> + ret = omapl138_hawk_config_emac();
>>>
>>> Why assign 'ret', if you're ignoring the result?
>
>> Sorry my mistake I forgot to check this, please check the nest patch
>> maybe it is a better implementation
>
> [...]
>
>> diff --git a/arch/arm/mach-davinci/board-omapl138-hawk.c
>> b/arch/arm/mach-davinci/board-omapl138-hawk.c
>> index c472dd8..35bcea1 100644
>> --- a/arch/arm/mach-davinci/board-omapl138-hawk.c
>> +++ b/arch/arm/mach-davinci/board-omapl138-hawk.c
>> @@ -19,6 +19,53 @@
>>
>> #include<mach/cp_intc.h>
>> #include<mach/da8xx.h>
>> +#include<mach/mux.h>
>> +
>> +#define HAWKBOARD_PHY_ID "0:07"
>> +
>> +static short omapl138_hawk_mii_pins[] __initdata = {
>> + DA850_MII_TXEN, DA850_MII_TXCLK, DA850_MII_COL, DA850_MII_TXD_3,
>> + DA850_MII_TXD_2, DA850_MII_TXD_1, DA850_MII_TXD_0, DA850_MII_RXER,
>> + DA850_MII_CRS, DA850_MII_RXCLK, DA850_MII_RXDV, DA850_MII_RXD_3,
>> + DA850_MII_RXD_2, DA850_MII_RXD_1, DA850_MII_RXD_0, DA850_MDIO_CLK,
>> + DA850_MDIO_D,
>> + -1
>> +};
>> +
>> +static __init void omapl138_hawk_config_emac(void)
>> +{
>> + void __iomem *cfgchip3;
>> + int ret;
>> + u32 val;
>> + struct davinci_soc_info *soc_info =&davinci_soc_info;
>> +
>> + if (!machine_is_omapl138_hawkboard())
>> + return;
>> +
>> + cfgchip3 = DA8XX_SYSCFG0_VIRT(DA8XX_CFGCHIP3_REG);
>> +
>> + val = __raw_readl(cfgchip3);
>> +
>> + val&= ~BIT(8);
>> + ret = davinci_cfg_reg_list(omapl138_hawk_mii_pins);
>> + if (ret)
>> + pr_warning("%s: "
>> + "cpgmac/mii mux setup failed: %d\n", __func__,
>> ret);
>> + return;
>
> Should enclose these two statements in {} and fix indentation.
>
>> +
>> + /* configure the CFGCHIP3 register for MII */
>> + __raw_writel(val, cfgchip3);
>> + pr_info("EMAC: MII PHY configured\n");
>> +
>> + soc_info->emac_pdata->phy_id = HAWKBOARD_PHY_ID;
>> +
>> + ret = da8xx_register_emac();
>> + if (ret)
>> + pr_warning("%s: "
>> + "emac registration failed: %d\n", __func__, ret);
>> + return;
>
> 'return' is useless here, and it should've been enclosed into {}...
>
> WBR. Sergei
>
Hi Sergei
Thanks a lot for your feed back I have already fixed
Regards
Victor Rodriguez
^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2010-10-18 23:58 UTC | newest]
Thread overview: 5+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2010-10-14 17:21 [PATCH v3 1/9] davinci: EMAC support for Omapl138-Hawkboard vm.rod25
[not found] ` <1287076899-4365-1-git-send-email-vm.rod25-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
2010-10-14 17:16 ` Sergei Shtylyov
2010-10-14 21:44 ` Victor Rodriguez
[not found] ` <AANLkTikaDB1Cf9T5W-ogbQgEvGpmhYjJ8YHa3UftFTjL-JsoAwUIsXosN+BqQ9rBEUg@public.gmane.org>
2010-10-15 16:25 ` Sergei Shtylyov
2010-10-18 23:58 ` Victor Rodriguez
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.