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@xxxxxxxxxxx> > 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
