Re: [PATCH net-next v4 13/12] net: lan865x: optional hardware reset

[Andrew Lunn <andrew@lunn.ch>]

On Sun, Apr 28, 2024 at 11:16:32PM +0200, Ramón Nordin Rodriguez wrote:
> >From c65e42982684d5fd8b2294eb6acf755aa0fcab83 Mon Sep 17 00:00:00 2001
> From: =?UTF-8?q?Ram=C3=B3n=20Nordin=20Rodriguez?=
>  <ramon.nordin.rodriguez@ferroamp.se>
> Date: Sun, 28 Apr 2024 22:25:12 +0200
> Subject: [PATCH net-next v4 13/12] net: lan865x: optional hardware reset
> MIME-Version: 1.0
> Content-Type: text/plain; charset=UTF-8
> Content-Transfer-Encoding: 8bit

You sent this patch in an odd way. We don't normally see headers like
this. I've been using b4 recently for patch management:

https://b4.docs.kernel.org/en/latest/contributor/prep.html

Using `b4 send` is a good idea. Otherwise git format-patch; git send-email

> index 9abefa8b9d9f..bed9033574b2 100644
> --- a/drivers/net/ethernet/microchip/lan865x/lan865x.c
> +++ b/drivers/net/ethernet/microchip/lan865x/lan865x.c
> @@ -9,6 +9,7 @@
>  #include <linux/kernel.h>
>  #include <linux/phy.h>
>  #include <linux/oa_tc6.h>
> +#include <linux/gpio/driver.h>

This is not a gpio driver, it is a gpio consumer. So you should be
using linux/gpio/consumer.h. Also, i _think_ the includes are sorted,
so it probably should go earlier.

>  
>  #define DRV_NAME			"lan865x"
>  
> @@ -33,6 +34,7 @@
>  
>  struct lan865x_priv {
>  	struct work_struct multicast_work;
> +	struct gpio_desc *reset_gpio;
>  	struct net_device *netdev;
>  	struct spi_device *spi;
>  	struct oa_tc6 *tc6;
> @@ -283,6 +285,24 @@ static int lan865x_set_zarfe(struct lan865x_priv *priv)
>  	return oa_tc6_write_register(priv->tc6, OA_TC6_REG_CONFIG0, regval);
>  }
>  
> +static int lan865x_probe_reset_gpio(struct lan865x_priv *priv)
> +{
> +	priv->reset_gpio = devm_gpiod_get_optional(&priv->spi->dev, "reset",
> +						   GPIOD_OUT_HIGH);
> +	if (IS_ERR(priv->reset_gpio))
> +		return PTR_ERR(priv->reset_gpio);
> +
> +	return 0;
> +}
> +
> +static void lan865x_hw_reset(struct lan865x_priv *priv)
> +{
> +	gpiod_set_value_cansleep(priv->reset_gpio, 1);
> +	// section 9.6.3 RESET_N Timing specifies a minimum hold of 5us
> +	usleep_range(5, 10);
> +	gpiod_set_value_cansleep(priv->reset_gpio, 0);
> +}

Do you see a need to do a reset at any time other than probe? If not,
i would probably combine these two functions into one. Also, since you
pass GPIOD_OUT_HIGH, you have already put it into reset. So setting
the gpio to 1 is pointless.

Does the datasheet say anything about how long you should wait after
releasing the reset?

> +
>  static int lan865x_probe(struct spi_device *spi)
>  {
>  	struct net_device *netdev;
> @@ -297,6 +317,14 @@ static int lan865x_probe(struct spi_device *spi)
>  	priv->netdev = netdev;
>  	priv->spi = spi;
>  	spi_set_drvdata(spi, priv);
> +	if (lan865x_probe_reset_gpio(priv)) {
> +		dev_err(&spi->dev, "failed to probe reset pin");
> +		ret = -ENODEV;

It is normal that a function like lan865x_probe_reset_gpio() would
return an error code. You should then return that error code, rather
than replace it with ENODEV.

     Andrew