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

Andrew Lunn
2024-04-28T23:17:36Z

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