Received-SPF: pass (google.com: domain of linux-kernel+bounces-161626-linux.lists.archive=gmail.com@vger.kernel.org designates 147.75.80.249 as permitted sender) client-ip=147.75.80.249;
Date: Mon, 29 Apr 2024 01:17:10 +0200
From: Andrew Lunn <andrew@lunn.ch>
To: =?iso-8859-1?Q?Ram=F3n?= Nordin Rodriguez <ramon.nordin.rodriguez@ferroamp.se>
Cc: Parthiban Veerasooran <Parthiban.Veerasooran@microchip.com>,
	davem@davemloft.net, edumazet@google.com, kuba@kernel.org,
	pabeni@redhat.com, horms@kernel.org, saeedm@nvidia.com,
	anthony.l.nguyen@intel.com, netdev@vger.kernel.org,
	linux-kernel@vger.kernel.org, corbet@lwn.net,
	linux-doc@vger.kernel.org, robh+dt@kernel.org,
	krzysztof.kozlowski+dt@linaro.org, conor+dt@kernel.org,
	devicetree@vger.kernel.org, horatiu.vultur@microchip.com,
	ruanjinjie@huawei.com, steen.hegelund@microchip.com,
	vladimir.oltean@nxp.com, UNGLinuxDriver@microchip.com,
	Thorsten.Kummermehr@microchip.com, Pier.Beruto@onsemi.com,
	Selvamani.Rajagopal@onsemi.com, Nicolas.Ferre@microchip.com,
	benjamin.bigler@bernformulastudent.ch
Subject: Re: [PATCH net-next v4 13/12] net: lan865x: optional hardware reset
Message-ID: <13c38696-25ed-418d-b85e-950736384a0b@lunn.ch>
References: <20240418125648.372526-1-Parthiban.Veerasooran@microchip.com>
 <Zi68sDje4wfgftyZ@builder>
Precedence: bulk
MIME-Version: 1.0
Content-Type: text/plain; charset=iso-8859-1
Content-Disposition: inline
Content-Transfer-Encoding: 8bit
In-Reply-To: <Zi68sDje4wfgftyZ@builder>

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