Re: [PATCH 2/3] ARM: i.mx53: Parse Reset GPIO pin in FEC driver from Devicetree

[Pavel Pisa <pisa@cmp.felk.cvut.cz>]

Hello Jean-Christophe and Rosta,

On Tuesday 05 of November 2013 17:15:16 Jean-Christophe PLAGNIOL-VILLARD 
wrote:
> On 15:42 Tue 05 Nov     , Rostislav Lisovy wrote:
> > Signed-off-by: Rostislav Lisovy <lisovy@gmail.com>
> >
> > diff --git a/drivers/net/fec_imx.c b/drivers/net/fec_imx.c
> > index 2f31352..6f883bf 100644
> > --- a/drivers/net/fec_imx.c
> > +++ b/drivers/net/fec_imx.c
> > @@ -671,6 +676,22 @@ static int fec_probe(struct device_d *dev)
> >
> >  	fec->regs = dev_request_mem_region(dev, 0);
> >
> > +#ifdef CONFIG_OFDEVICE
>
> use if (IS_ENABLED(CONFIG_OFDEVICE))
>
> so we can improve the code coverage
>
> > +	phy_reset = of_get_named_gpio(dev->device_node, "phy-reset-gpios", 0);

I have question if the missing definition of reset pin should/must lead
to whole ethernet interface removal/initialization abort.

I think, that there can be design where PHY reset is not controlled
by MCU pin (it is usually possible even reset device through MDIO)
and then it would be better to check if "phy-reset-gpios" is present
and if not then skip whole attempt to manipulate GPIO.
On the other hand if "phy-reset-gpios", it is required to configure it,
because default pin state can/should lead to PHY reset.

I.e.

if (IS_ENABLED(CONFIG_OFDEVICE))

  phy_reset = of_get_named_gpio(dev->device_node, "phy-reset-gpios", 0);

  if(phy_reset < 0) {
    print info ("\"phy-reset-gpios\" is not used for i.MX fec, skipping forced 
reset\n")
    
  } else {

> > +	if (!gpio_is_valid(phy_reset))
> > +		goto err_free;
> > +
> > +	ret = gpio_request(phy_reset, "phy-reset");
> > +	if (ret) {
> > +		pr_err("Can not request gpio %d (phy-reset): %d\n", phy_reset, ret);
> > +		goto err_free;
> > +	}
> > +
> > +	gpio_direction_output(phy_reset, 0);
>
> you need to check the return too
>
> > +	udelay(10);
> > +	gpio_set_value(phy_reset, 1);
  }
}
> > +
> >  	/* Reset chip. */
> >  	writel(FEC_ECNTRL_RESET, fec->regs + FEC_ECNTRL);
> >  	while(readl(fec->regs + FEC_ECNTRL) & 1) {

Best wishes,


                 Pavel


_______________________________________________
barebox mailing list
barebox@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/barebox