[PATCH 1/3] net: davicom: dm9000: switch to using gpiod API

[PATCH 1/3] net: davicom: dm9000: switch to using gpiod API

[Dmitry Torokhov]

This patch switches the driver away from legacy gpio/of_gpio API to
gpiod API, and removes use of of_get_named_gpio_flags() which I want to
make private to gpiolib.

Signed-off-by: Dmitry Torokhov <dmitry.torokhov@gmail.com>
---
 drivers/net/ethernet/davicom/dm9000.c | 26 ++++++++++++++------------
 1 file changed, 14 insertions(+), 12 deletions(-)

diff --git a/drivers/net/ethernet/davicom/dm9000.c b/drivers/net/ethernet/davicom/dm9000.c
index 77229e53b04e..c85a6ebd79fc 100644
--- a/drivers/net/ethernet/davicom/dm9000.c
+++ b/drivers/net/ethernet/davicom/dm9000.c
@@ -28,8 +28,7 @@
 #include <linux/irq.h>
 #include <linux/slab.h>
 #include <linux/regulator/consumer.h>
-#include <linux/gpio.h>
-#include <linux/of_gpio.h>
+#include <linux/gpio/consumer.h>
 
 #include <asm/delay.h>
 #include <asm/irq.h>
@@ -1421,8 +1420,7 @@ dm9000_probe(struct platform_device *pdev)
 	int iosize;
 	int i;
 	u32 id_val;
-	int reset_gpios;
-	enum of_gpio_flags flags;
+	struct gpio_desc *reset_gpio;
 	struct regulator *power;
 	bool inv_mac_addr = false;
 	u8 addr[ETH_ALEN];
@@ -1442,20 +1440,24 @@ dm9000_probe(struct platform_device *pdev)
 		dev_dbg(dev, "regulator enabled\n");
 	}
 
-	reset_gpios = of_get_named_gpio_flags(dev->of_node, "reset-gpios", 0,
-					      &flags);
-	if (gpio_is_valid(reset_gpios)) {
-		ret = devm_gpio_request_one(dev, reset_gpios, flags,
-					    "dm9000_reset");
+	reset_gpio = devm_gpiod_get_optional(dev, "reset", GPIOD_OUT_HIGH);
+	ret = PTR_ERR_OR_ZERO(reset_gpio);
+	if (ret) {
+		dev_err(dev, "failed to request reset gpio: %d\n", ret);
+		goto out_regulator_disable;
+	}
+
+	if (reset_gpio) {
+		ret = gpiod_set_consumer_name(reset_gpio, "dm9000_reset");
 		if (ret) {
-			dev_err(dev, "failed to request reset gpio %d: %d\n",
-				reset_gpios, ret);
+			dev_err(dev, "failed to set reset gpio name: %d\n",
+				ret);
 			goto out_regulator_disable;
 		}
 
 		/* According to manual PWRST# Low Period Min 1ms */
 		msleep(2);
-		gpio_set_value(reset_gpios, 1);
+		gpiod_set_value_cansleep(reset_gpio, 0);
 		/* Needs 3ms to read eeprom when PWRST is deasserted */
 		msleep(4);
 	}
-- 
2.37.2.789.g6183377224-goog

[PATCH 2/3] net: ks8851: switch to using gpiod API

[Dmitry Torokhov]

This patch switches the driver away from legacy gpio/of_gpio API to
gpiod API, and removes use of of_get_named_gpio_flags() which I want to
make private to gpiolib.

Signed-off-by: Dmitry Torokhov <dmitry.torokhov@gmail.com>
---
 drivers/net/ethernet/micrel/ks8851.h        |  2 +-
 drivers/net/ethernet/micrel/ks8851_common.c | 40 ++++++++++-----------
 2 files changed, 20 insertions(+), 22 deletions(-)

diff --git a/drivers/net/ethernet/micrel/ks8851.h b/drivers/net/ethernet/micrel/ks8851.h
index 6f34a61739b6..fecd43754cea 100644
--- a/drivers/net/ethernet/micrel/ks8851.h
+++ b/drivers/net/ethernet/micrel/ks8851.h
@@ -403,7 +403,7 @@ struct ks8851_net {
 	struct eeprom_93cx6	eeprom;
 	struct regulator	*vdd_reg;
 	struct regulator	*vdd_io;
-	int			gpio;
+	struct gpio_desc	*gpio;
 	struct mii_bus		*mii_bus;
 
 	void			(*lock)(struct ks8851_net *ks,
diff --git a/drivers/net/ethernet/micrel/ks8851_common.c b/drivers/net/ethernet/micrel/ks8851_common.c
index ec8457e61b45..cfbc900d4aeb 100644
--- a/drivers/net/ethernet/micrel/ks8851_common.c
+++ b/drivers/net/ethernet/micrel/ks8851_common.c
@@ -17,10 +17,9 @@
 #include <linux/cache.h>
 #include <linux/crc32.h>
 #include <linux/mii.h>
+#include <linux/gpio/consumer.h>
 #include <linux/regulator/consumer.h>
 
-#include <linux/gpio.h>
-#include <linux/of_gpio.h>
 #include <linux/of_mdio.h>
 #include <linux/of_net.h>
 
@@ -1117,24 +1116,23 @@ int ks8851_probe_common(struct net_device *netdev, struct device *dev,
 {
 	struct ks8851_net *ks = netdev_priv(netdev);
 	unsigned cider;
-	int gpio;
 	int ret;
 
 	ks->netdev = netdev;
 	ks->tx_space = 6144;
 
-	gpio = of_get_named_gpio_flags(dev->of_node, "reset-gpios", 0, NULL);
-	if (gpio == -EPROBE_DEFER)
-		return gpio;
-
-	ks->gpio = gpio;
-	if (gpio_is_valid(gpio)) {
-		ret = devm_gpio_request_one(dev, gpio,
-					    GPIOF_OUT_INIT_LOW, "ks8851_rst_n");
-		if (ret) {
-			dev_err(dev, "reset gpio request failed\n");
-			return ret;
-		}
+	ks->gpio = devm_gpiod_get_optional(dev, "reset", GPIOD_OUT_HIGH);
+	ret = PTR_ERR_OR_ZERO(ks->gpio);
+	if (ret) {
+		if (ret != -EPROBE_DEFER)
+			dev_err(dev, "reset gpio request failed: %d\n", ret);
+		return ret;
+	}
+
+	ret = gpiod_set_consumer_name(ks->gpio, "ks8851_rst_n");
+	if (ret) {
+		dev_err(dev, "failed to set reset gpio name: %d\n", ret);
+		return ret;
 	}
 
 	ks->vdd_io = devm_regulator_get(dev, "vdd-io");
@@ -1161,9 +1159,9 @@ int ks8851_probe_common(struct net_device *netdev, struct device *dev,
 		goto err_reg;
 	}
 
-	if (gpio_is_valid(gpio)) {
+	if (ks->gpio) {
 		usleep_range(10000, 11000);
-		gpio_set_value(gpio, 1);
+		gpiod_set_value_cansleep(ks->gpio, 0);
 	}
 
 	spin_lock_init(&ks->statelock);
@@ -1239,8 +1237,8 @@ int ks8851_probe_common(struct net_device *netdev, struct device *dev,
 err_id:
 	ks8851_unregister_mdiobus(ks);
 err_mdio:
-	if (gpio_is_valid(gpio))
-		gpio_set_value(gpio, 0);
+	if (ks->gpio)
+		gpiod_set_value_cansleep(ks->gpio, 1);
 	regulator_disable(ks->vdd_reg);
 err_reg:
 	regulator_disable(ks->vdd_io);
@@ -1259,8 +1257,8 @@ void ks8851_remove_common(struct device *dev)
 		dev_info(dev, "remove\n");
 
 	unregister_netdev(priv->netdev);
-	if (gpio_is_valid(priv->gpio))
-		gpio_set_value(priv->gpio, 0);
+	if (priv->gpio)
+		gpiod_set_value_cansleep(priv->gpio, 1);
 	regulator_disable(priv->vdd_reg);
 	regulator_disable(priv->vdd_io);
 }
-- 
2.37.2.789.g6183377224-goog

[PATCH 3/3] net: phy: spi_ks8895: switch to using gpiod API

[Dmitry Torokhov]

This patch switches the driver away from legacy gpio/of_gpio API to
gpiod API, and removes use of of_get_named_gpio_flags() which I want to
make private to gpiolib.

Signed-off-by: Dmitry Torokhov <dmitry.torokhov@gmail.com>
---
 drivers/net/phy/spi_ks8995.c | 69 ++++++++----------------------------
 1 file changed, 14 insertions(+), 55 deletions(-)

diff --git a/drivers/net/phy/spi_ks8995.c b/drivers/net/phy/spi_ks8995.c
index ff37f8ba6758..d4202d40d47a 100644
--- a/drivers/net/phy/spi_ks8995.c
+++ b/drivers/net/phy/spi_ks8995.c
@@ -17,7 +17,6 @@
 #include <linux/device.h>
 #include <linux/gpio/consumer.h>
 #include <linux/of.h>
-#include <linux/of_gpio.h>
 
 #include <linux/spi/spi.h>
 
@@ -137,15 +136,10 @@ static const struct ks8995_chip_params ks8995_chip[] = {
 	},
 };
 
-struct ks8995_pdata {
-	int reset_gpio;
-	enum of_gpio_flags reset_gpio_flags;
-};
-
 struct ks8995_switch {
 	struct spi_device	*spi;
 	struct mutex		lock;
-	struct ks8995_pdata	*pdata;
+	struct gpio_desc	*reset_gpio;
 	struct bin_attribute	regs_attr;
 	const struct ks8995_chip_params	*chip;
 	int			revision_id;
@@ -401,24 +395,6 @@ static int ks8995_get_revision(struct ks8995_switch *ks)
 	return err;
 }
 
-/* ks8995_parse_dt - setup platform data from devicetree
- * @ks: pointer to switch instance
- *
- * Parses supported DT properties and sets up platform data
- * accordingly.
- */
-static void ks8995_parse_dt(struct ks8995_switch *ks)
-{
-	struct device_node *np = ks->spi->dev.of_node;
-	struct ks8995_pdata *pdata = ks->pdata;
-
-	if (!np)
-		return;
-
-	pdata->reset_gpio = of_get_named_gpio_flags(np, "reset-gpios", 0,
-		&pdata->reset_gpio_flags);
-}
-
 static const struct bin_attribute ks8995_registers_attr = {
 	.attr = {
 		.name   = "registers",
@@ -449,38 +425,22 @@ static int ks8995_probe(struct spi_device *spi)
 	ks->spi = spi;
 	ks->chip = &ks8995_chip[variant];
 
-	if (ks->spi->dev.of_node) {
-		ks->pdata = devm_kzalloc(&spi->dev, sizeof(*ks->pdata),
-					 GFP_KERNEL);
-		if (!ks->pdata)
-			return -ENOMEM;
-
-		ks->pdata->reset_gpio = -1;
-
-		ks8995_parse_dt(ks);
+	ks->reset_gpio = devm_gpiod_get_optional(&spi->dev, "reset",
+						 GPIOD_OUT_HIGH);
+	err = PTR_ERR_OR_ZERO(ks->reset_gpio);
+	if (err) {
+		dev_err(&spi->dev,
+			"failed to get reset gpio: %d\n", err);
+		return err;
 	}
 
-	if (!ks->pdata)
-		ks->pdata = spi->dev.platform_data;
+	err = gpiod_set_consumer_name(ks->reset_gpio, "switch-reset");
+	if (err)
+		return err;
 
 	/* de-assert switch reset */
-	if (ks->pdata && gpio_is_valid(ks->pdata->reset_gpio)) {
-		unsigned long flags;
-
-		flags = (ks->pdata->reset_gpio_flags == OF_GPIO_ACTIVE_LOW ?
-			 GPIOF_ACTIVE_LOW : 0);
-
-		err = devm_gpio_request_one(&spi->dev,
-					    ks->pdata->reset_gpio,
-					    flags, "switch-reset");
-		if (err) {
-			dev_err(&spi->dev,
-				"failed to get reset-gpios: %d\n", err);
-			return -EIO;
-		}
-
-		gpiod_set_value(gpio_to_desc(ks->pdata->reset_gpio), 0);
-	}
+	/* FIXME: this likely requires a delay */
+	gpiod_set_value_cansleep(ks->reset_gpio, 0);
 
 	spi_set_drvdata(spi, ks);
 
@@ -524,8 +484,7 @@ static void ks8995_remove(struct spi_device *spi)
 	sysfs_remove_bin_file(&spi->dev.kobj, &ks->regs_attr);
 
 	/* assert reset */
-	if (ks->pdata && gpio_is_valid(ks->pdata->reset_gpio))
-		gpiod_set_value(gpio_to_desc(ks->pdata->reset_gpio), 1);
+	gpiod_set_value_cansleep(ks->reset_gpio, 1);
 }
 
 /* ------------------------------------------------------------------------ */
-- 
2.37.2.789.g6183377224-goog

Re: [PATCH 1/3] net: davicom: dm9000: switch to using gpiod API

[Linus Walleij]

On Tue, Sep 6, 2022 at 10:49 PM Dmitry Torokhov
<dmitry.torokhov@gmail.com> wrote:

> This patch switches the driver away from legacy gpio/of_gpio API to
> gpiod API, and removes use of of_get_named_gpio_flags() which I want to
> make private to gpiolib.
>
> Signed-off-by: Dmitry Torokhov <dmitry.torokhov@gmail.com>

Reviewed-by: Linus Walleij <linus.walleij@linaro.org>

I think net patches need [PATCH net-next] in the subject to get
applied.

Yours,
Linus Walleij

Re: [PATCH 2/3] net: ks8851: switch to using gpiod API

[Linus Walleij]

On Tue, Sep 6, 2022 at 10:49 PM Dmitry Torokhov
<dmitry.torokhov@gmail.com> wrote:

> This patch switches the driver away from legacy gpio/of_gpio API to
> gpiod API, and removes use of of_get_named_gpio_flags() which I want to
> make private to gpiolib.
>
> Signed-off-by: Dmitry Torokhov <dmitry.torokhov@gmail.com>

Reviewed-by: Linus Walleij <linus.walleij@linaro.org>

Yours,
Linus Walleij

Re: [PATCH 3/3] net: phy: spi_ks8895: switch to using gpiod API

[Linus Walleij]

On Tue, Sep 6, 2022 at 10:49 PM Dmitry Torokhov
<dmitry.torokhov@gmail.com> wrote:

> This patch switches the driver away from legacy gpio/of_gpio API to
> gpiod API, and removes use of of_get_named_gpio_flags() which I want to
> make private to gpiolib.
>
> Signed-off-by: Dmitry Torokhov <dmitry.torokhov@gmail.com>

Reviewed-by: Linus Walleij <linus.walleij@linaro.org>

Yours,
Linus Walleij

Re: [PATCH 1/3] net: davicom: dm9000: switch to using gpiod API

[Andrew Lunn]

On Wed, Sep 07, 2022 at 11:45:48PM +0200, Linus Walleij wrote:
> On Tue, Sep 6, 2022 at 10:49 PM Dmitry Torokhov
> <dmitry.torokhov@gmail.com> wrote:
> 
> > This patch switches the driver away from legacy gpio/of_gpio API to
> > gpiod API, and removes use of of_get_named_gpio_flags() which I want to
> > make private to gpiolib.
> >
> > Signed-off-by: Dmitry Torokhov <dmitry.torokhov@gmail.com>
> 
> Reviewed-by: Linus Walleij <linus.walleij@linaro.org>
> 
> I think net patches need [PATCH net-next] in the subject to get
> applied.

Correct.

https://docs.kernel.org/process/maintainer-netdev.html

For the odd drive by patch, the Maintainers often do accept patches
without it. So wait and see.

	Andrew

Re: [PATCH 1/3] net: davicom: dm9000: switch to using gpiod API

[Paolo Abeni]

On Thu, 2022-09-08 at 14:06 +0200, Andrew Lunn wrote:
> On Wed, Sep 07, 2022 at 11:45:48PM +0200, Linus Walleij wrote:
> > On Tue, Sep 6, 2022 at 10:49 PM Dmitry Torokhov
> > <dmitry.torokhov@gmail.com> wrote:
> > 
> > > This patch switches the driver away from legacy gpio/of_gpio API to
> > > gpiod API, and removes use of of_get_named_gpio_flags() which I want to
> > > make private to gpiolib.
> > > 
> > > Signed-off-by: Dmitry Torokhov <dmitry.torokhov@gmail.com>
> > 
> > Reviewed-by: Linus Walleij <linus.walleij@linaro.org>
> > 
> > I think net patches need [PATCH net-next] in the subject to get
> > applied.
> 
> Correct.
> 
> https://docs.kernel.org/process/maintainer-netdev.html
> 
> For the odd drive by patch, the Maintainers often do accept patches
> without it. So wait and see.

Due to a series of unfortunate coincidences the netdev backlog has
grown unusually long. Since the patches LGMT I'm going to apply them to
net-next without asking a repost.

@Dmitry: next time, please add the relevant tag, thanks!

Paolo

Re: [PATCH 1/3] net: davicom: dm9000: switch to using gpiod API

[patchwork-bot+netdevbpf]

Hello:

This series was applied to netdev/net-next.git (master)
by Paolo Abeni <pabeni@redhat.com>:

On Tue,  6 Sep 2022 13:49:20 -0700 you wrote:
> This patch switches the driver away from legacy gpio/of_gpio API to
> gpiod API, and removes use of of_get_named_gpio_flags() which I want to
> make private to gpiolib.
> 
> Signed-off-by: Dmitry Torokhov <dmitry.torokhov@gmail.com>
> ---
>  drivers/net/ethernet/davicom/dm9000.c | 26 ++++++++++++++------------
>  1 file changed, 14 insertions(+), 12 deletions(-)

Here is the summary with links:
  - [1/3] net: davicom: dm9000: switch to using gpiod API
    https://git.kernel.org/netdev/net-next/c/db49ca38579d
  - [2/3] net: ks8851: switch to using gpiod API
    https://git.kernel.org/netdev/net-next/c/7b77bb5c8130
  - [3/3] net: phy: spi_ks8895: switch to using gpiod API
    https://git.kernel.org/netdev/net-next/c/006534ec2804

You are awesome, thank you!
-- 
Deet-doot-dot, I am a bot.
https://korg.docs.kernel.org/patchwork/pwbot.html

Re: [PATCH 1/3] net: davicom: dm9000: switch to using gpiod API

[Paul Cercueil]

Hi Dmitry,

Le mar. 6 sept. 2022 à 13:49:20 -0700, Dmitry Torokhov 
<dmitry.torokhov@gmail.com> a écrit :
> This patch switches the driver away from legacy gpio/of_gpio API to
> gpiod API, and removes use of of_get_named_gpio_flags() which I want 
> to
> make private to gpiolib.
> 
> Signed-off-by: Dmitry Torokhov <dmitry.torokhov@gmail.com>
> ---
>  drivers/net/ethernet/davicom/dm9000.c | 26 ++++++++++++++------------
>  1 file changed, 14 insertions(+), 12 deletions(-)
> 
> diff --git a/drivers/net/ethernet/davicom/dm9000.c 
> b/drivers/net/ethernet/davicom/dm9000.c
> index 77229e53b04e..c85a6ebd79fc 100644
> --- a/drivers/net/ethernet/davicom/dm9000.c
> +++ b/drivers/net/ethernet/davicom/dm9000.c
> @@ -28,8 +28,7 @@
>  #include <linux/irq.h>
>  #include <linux/slab.h>
>  #include <linux/regulator/consumer.h>
> -#include <linux/gpio.h>
> -#include <linux/of_gpio.h>
> +#include <linux/gpio/consumer.h>
> 
>  #include <asm/delay.h>
>  #include <asm/irq.h>
> @@ -1421,8 +1420,7 @@ dm9000_probe(struct platform_device *pdev)
>  	int iosize;
>  	int i;
>  	u32 id_val;
> -	int reset_gpios;
> -	enum of_gpio_flags flags;
> +	struct gpio_desc *reset_gpio;
>  	struct regulator *power;
>  	bool inv_mac_addr = false;
>  	u8 addr[ETH_ALEN];
> @@ -1442,20 +1440,24 @@ dm9000_probe(struct platform_device *pdev)
>  		dev_dbg(dev, "regulator enabled\n");
>  	}
> 
> -	reset_gpios = of_get_named_gpio_flags(dev->of_node, "reset-gpios", 
> 0,
> -					      &flags);
> -	if (gpio_is_valid(reset_gpios)) {
> -		ret = devm_gpio_request_one(dev, reset_gpios, flags,
> -					    "dm9000_reset");
> +	reset_gpio = devm_gpiod_get_optional(dev, "reset", GPIOD_OUT_HIGH);
> +	ret = PTR_ERR_OR_ZERO(reset_gpio);
> +	if (ret) {
> +		dev_err(dev, "failed to request reset gpio: %d\n", ret);
> +		goto out_regulator_disable;
> +	}
> +
> +	if (reset_gpio) {
> +		ret = gpiod_set_consumer_name(reset_gpio, "dm9000_reset");
>  		if (ret) {
> -			dev_err(dev, "failed to request reset gpio %d: %d\n",
> -				reset_gpios, ret);
> +			dev_err(dev, "failed to set reset gpio name: %d\n",
> +				ret);
>  			goto out_regulator_disable;
>  		}
> 
>  		/* According to manual PWRST# Low Period Min 1ms */
>  		msleep(2);
> -		gpio_set_value(reset_gpios, 1);
> +		gpiod_set_value_cansleep(reset_gpio, 0);

Why is that 1 magically turned into a 0?

On my CI20 board I can't get the DM9000 chip to probe correctly with 
this patch (it fails to read the ID).
If I revert this patch then everything works fine.

Cheers,
-Paul

>  		/* Needs 3ms to read eeprom when PWRST is deasserted */
>  		msleep(4);
>  	}
> --
> 2.37.2.789.g6183377224-goog
> 

Re: [PATCH 1/3] net: davicom: dm9000: switch to using gpiod API

[Dmitry Torokhov]

Hi Paul,

On Fri, Nov 18, 2022 at 03:33:44PM +0000, Paul Cercueil wrote:
> Hi Dmitry,
> 
> Le mar. 6 sept. 2022 à 13:49:20 -0700, Dmitry Torokhov
> <dmitry.torokhov@gmail.com> a écrit :
> > This patch switches the driver away from legacy gpio/of_gpio API to
> > gpiod API, and removes use of of_get_named_gpio_flags() which I want to
> > make private to gpiolib.
> > 
> > Signed-off-by: Dmitry Torokhov <dmitry.torokhov@gmail.com>
> > ---
> >  drivers/net/ethernet/davicom/dm9000.c | 26 ++++++++++++++------------
> >  1 file changed, 14 insertions(+), 12 deletions(-)
> > 
> > diff --git a/drivers/net/ethernet/davicom/dm9000.c
> > b/drivers/net/ethernet/davicom/dm9000.c
> > index 77229e53b04e..c85a6ebd79fc 100644
> > --- a/drivers/net/ethernet/davicom/dm9000.c
> > +++ b/drivers/net/ethernet/davicom/dm9000.c
> > @@ -28,8 +28,7 @@
> >  #include <linux/irq.h>
> >  #include <linux/slab.h>
> >  #include <linux/regulator/consumer.h>
> > -#include <linux/gpio.h>
> > -#include <linux/of_gpio.h>
> > +#include <linux/gpio/consumer.h>
> > 
> >  #include <asm/delay.h>
> >  #include <asm/irq.h>
> > @@ -1421,8 +1420,7 @@ dm9000_probe(struct platform_device *pdev)
> >  	int iosize;
> >  	int i;
> >  	u32 id_val;
> > -	int reset_gpios;
> > -	enum of_gpio_flags flags;
> > +	struct gpio_desc *reset_gpio;
> >  	struct regulator *power;
> >  	bool inv_mac_addr = false;
> >  	u8 addr[ETH_ALEN];
> > @@ -1442,20 +1440,24 @@ dm9000_probe(struct platform_device *pdev)
> >  		dev_dbg(dev, "regulator enabled\n");
> >  	}
> > 
> > -	reset_gpios = of_get_named_gpio_flags(dev->of_node, "reset-gpios", 0,
> > -					      &flags);
> > -	if (gpio_is_valid(reset_gpios)) {
> > -		ret = devm_gpio_request_one(dev, reset_gpios, flags,
> > -					    "dm9000_reset");
> > +	reset_gpio = devm_gpiod_get_optional(dev, "reset", GPIOD_OUT_HIGH);
> > +	ret = PTR_ERR_OR_ZERO(reset_gpio);
> > +	if (ret) {
> > +		dev_err(dev, "failed to request reset gpio: %d\n", ret);
> > +		goto out_regulator_disable;
> > +	}
> > +
> > +	if (reset_gpio) {
> > +		ret = gpiod_set_consumer_name(reset_gpio, "dm9000_reset");
> >  		if (ret) {
> > -			dev_err(dev, "failed to request reset gpio %d: %d\n",
> > -				reset_gpios, ret);
> > +			dev_err(dev, "failed to set reset gpio name: %d\n",
> > +				ret);
> >  			goto out_regulator_disable;
> >  		}
> > 
> >  		/* According to manual PWRST# Low Period Min 1ms */
> >  		msleep(2);
> > -		gpio_set_value(reset_gpios, 1);
> > +		gpiod_set_value_cansleep(reset_gpio, 0);
> 
> Why is that 1 magically turned into a 0?

Because gpiod uses logical states (think active/inactive), not absolute
ones. Here we are deasserting the reset line.

> 
> On my CI20 board I can't get the DM9000 chip to probe correctly with this
> patch (it fails to read the ID).
> If I revert this patch then everything works fine.

Sorry, it is my fault of course: I missed that board has incorrect
annotation for the reset line. I will send out the patch below
(formatted properly of course):

diff --git a/arch/mips/boot/dts/ingenic/ci20.dts b/arch/mips/boot/dts/ingenic/ci20.dts
index 37c46720c719..f38c39572a9e 100644
--- a/arch/mips/boot/dts/ingenic/ci20.dts
+++ b/arch/mips/boot/dts/ingenic/ci20.dts
@@ -438,7 +438,7 @@ dm9000@6 {
 		ingenic,nemc-tAW = <50>;
 		ingenic,nemc-tSTRV = <100>;
 
-		reset-gpios = <&gpf 12 GPIO_ACTIVE_HIGH>;
+		reset-gpios = <&gpf 12 GPIO_ACTIVE_LOW>;
 		vcc-supply = <&eth0_power>;
 
 		interrupt-parent = <&gpe>;


Thanks.

-- 
Dmitry

Re: [PATCH 1/3] net: davicom: dm9000: switch to using gpiod API

[Paul Cercueil]

Le ven. 18 nov. 2022 à 07:58:21 -0800, Dmitry Torokhov 
<dmitry.torokhov@gmail.com> a écrit :
> Hi Paul,
> 
> On Fri, Nov 18, 2022 at 03:33:44PM +0000, Paul Cercueil wrote:
>>  Hi Dmitry,
>> 
>>  Le mar. 6 sept. 2022 à 13:49:20 -0700, Dmitry Torokhov
>>  <dmitry.torokhov@gmail.com> a écrit :
>>  > This patch switches the driver away from legacy gpio/of_gpio API 
>> to
>>  > gpiod API, and removes use of of_get_named_gpio_flags() which I 
>> want to
>>  > make private to gpiolib.
>>  >
>>  > Signed-off-by: Dmitry Torokhov <dmitry.torokhov@gmail.com>
>>  > ---
>>  >  drivers/net/ethernet/davicom/dm9000.c | 26 
>> ++++++++++++++------------
>>  >  1 file changed, 14 insertions(+), 12 deletions(-)
>>  >
>>  > diff --git a/drivers/net/ethernet/davicom/dm9000.c
>>  > b/drivers/net/ethernet/davicom/dm9000.c
>>  > index 77229e53b04e..c85a6ebd79fc 100644
>>  > --- a/drivers/net/ethernet/davicom/dm9000.c
>>  > +++ b/drivers/net/ethernet/davicom/dm9000.c
>>  > @@ -28,8 +28,7 @@
>>  >  #include <linux/irq.h>
>>  >  #include <linux/slab.h>
>>  >  #include <linux/regulator/consumer.h>
>>  > -#include <linux/gpio.h>
>>  > -#include <linux/of_gpio.h>
>>  > +#include <linux/gpio/consumer.h>
>>  >
>>  >  #include <asm/delay.h>
>>  >  #include <asm/irq.h>
>>  > @@ -1421,8 +1420,7 @@ dm9000_probe(struct platform_device *pdev)
>>  >  	int iosize;
>>  >  	int i;
>>  >  	u32 id_val;
>>  > -	int reset_gpios;
>>  > -	enum of_gpio_flags flags;
>>  > +	struct gpio_desc *reset_gpio;
>>  >  	struct regulator *power;
>>  >  	bool inv_mac_addr = false;
>>  >  	u8 addr[ETH_ALEN];
>>  > @@ -1442,20 +1440,24 @@ dm9000_probe(struct platform_device *pdev)
>>  >  		dev_dbg(dev, "regulator enabled\n");
>>  >  	}
>>  >
>>  > -	reset_gpios = of_get_named_gpio_flags(dev->of_node, 
>> "reset-gpios", 0,
>>  > -					      &flags);
>>  > -	if (gpio_is_valid(reset_gpios)) {
>>  > -		ret = devm_gpio_request_one(dev, reset_gpios, flags,
>>  > -					    "dm9000_reset");
>>  > +	reset_gpio = devm_gpiod_get_optional(dev, "reset", 
>> GPIOD_OUT_HIGH);
>>  > +	ret = PTR_ERR_OR_ZERO(reset_gpio);
>>  > +	if (ret) {
>>  > +		dev_err(dev, "failed to request reset gpio: %d\n", ret);
>>  > +		goto out_regulator_disable;
>>  > +	}
>>  > +
>>  > +	if (reset_gpio) {
>>  > +		ret = gpiod_set_consumer_name(reset_gpio, "dm9000_reset");
>>  >  		if (ret) {
>>  > -			dev_err(dev, "failed to request reset gpio %d: %d\n",
>>  > -				reset_gpios, ret);
>>  > +			dev_err(dev, "failed to set reset gpio name: %d\n",
>>  > +				ret);
>>  >  			goto out_regulator_disable;
>>  >  		}
>>  >
>>  >  		/* According to manual PWRST# Low Period Min 1ms */
>>  >  		msleep(2);
>>  > -		gpio_set_value(reset_gpios, 1);
>>  > +		gpiod_set_value_cansleep(reset_gpio, 0);
>> 
>>  Why is that 1 magically turned into a 0?
> 
> Because gpiod uses logical states (think active/inactive), not 
> absolute
> ones. Here we are deasserting the reset line.
> 
>> 
>>  On my CI20 board I can't get the DM9000 chip to probe correctly 
>> with this
>>  patch (it fails to read the ID).
>>  If I revert this patch then everything works fine.
> 
> Sorry, it is my fault of course: I missed that board has incorrect
> annotation for the reset line. I will send out the patch below
> (formatted properly of course):

So in *theory* you wouldn't fix it like that, because the driver should 
work with old Device Tree files, even if it had a broken property, as 
long as it used to work in the past.

The ci20.dts file however is always built into the kernel and I'm not 
aware of anybody doing things differently. As long as you make that 
explicit in your commit message I think Rob won't mind.

If he does, or if more boards are affected, an alternative is to switch 
the polarity of the GPIO in the driver, like so:

if (of_machine_is_compatible("mips,ci20") &&
    gpiod_is_active_low(reset_gpio)) {
	gpiod_toggle_active_low(reset_gpio);
}

Cheers,
-Paul

> diff --git a/arch/mips/boot/dts/ingenic/ci20.dts 
> b/arch/mips/boot/dts/ingenic/ci20.dts
> index 37c46720c719..f38c39572a9e 100644
> --- a/arch/mips/boot/dts/ingenic/ci20.dts
> +++ b/arch/mips/boot/dts/ingenic/ci20.dts
> @@ -438,7 +438,7 @@ dm9000@6 {
>  		ingenic,nemc-tAW = <50>;
>  		ingenic,nemc-tSTRV = <100>;
> 
> -		reset-gpios = <&gpf 12 GPIO_ACTIVE_HIGH>;
> +		reset-gpios = <&gpf 12 GPIO_ACTIVE_LOW>;
>  		vcc-supply = <&eth0_power>;
> 
>  		interrupt-parent = <&gpe>;
> 
> 
> Thanks.
> 
> --
> Dmitry

Re: [PATCH 1/3] net: davicom: dm9000: switch to using gpiod API

[Dmitry Torokhov]

On Fri, Nov 18, 2022 at 04:26:38PM +0000, Paul Cercueil wrote:
> 
> 
> Le ven. 18 nov. 2022 à 07:58:21 -0800, Dmitry Torokhov
> <dmitry.torokhov@gmail.com> a écrit :
> > Hi Paul,
> > 
> > On Fri, Nov 18, 2022 at 03:33:44PM +0000, Paul Cercueil wrote:
> > >  Hi Dmitry,
> > > 
> > >  Le mar. 6 sept. 2022 à 13:49:20 -0700, Dmitry Torokhov
> > >  <dmitry.torokhov@gmail.com> a écrit :
> > >  > This patch switches the driver away from legacy gpio/of_gpio API
> > > to
> > >  > gpiod API, and removes use of of_get_named_gpio_flags() which I
> > > want to
> > >  > make private to gpiolib.
> > >  >
> > >  > Signed-off-by: Dmitry Torokhov <dmitry.torokhov@gmail.com>
> > >  > ---
> > >  >  drivers/net/ethernet/davicom/dm9000.c | 26
> > > ++++++++++++++------------
> > >  >  1 file changed, 14 insertions(+), 12 deletions(-)
> > >  >
> > >  > diff --git a/drivers/net/ethernet/davicom/dm9000.c
> > >  > b/drivers/net/ethernet/davicom/dm9000.c
> > >  > index 77229e53b04e..c85a6ebd79fc 100644
> > >  > --- a/drivers/net/ethernet/davicom/dm9000.c
> > >  > +++ b/drivers/net/ethernet/davicom/dm9000.c
> > >  > @@ -28,8 +28,7 @@
> > >  >  #include <linux/irq.h>
> > >  >  #include <linux/slab.h>
> > >  >  #include <linux/regulator/consumer.h>
> > >  > -#include <linux/gpio.h>
> > >  > -#include <linux/of_gpio.h>
> > >  > +#include <linux/gpio/consumer.h>
> > >  >
> > >  >  #include <asm/delay.h>
> > >  >  #include <asm/irq.h>
> > >  > @@ -1421,8 +1420,7 @@ dm9000_probe(struct platform_device *pdev)
> > >  >  	int iosize;
> > >  >  	int i;
> > >  >  	u32 id_val;
> > >  > -	int reset_gpios;
> > >  > -	enum of_gpio_flags flags;
> > >  > +	struct gpio_desc *reset_gpio;
> > >  >  	struct regulator *power;
> > >  >  	bool inv_mac_addr = false;
> > >  >  	u8 addr[ETH_ALEN];
> > >  > @@ -1442,20 +1440,24 @@ dm9000_probe(struct platform_device *pdev)
> > >  >  		dev_dbg(dev, "regulator enabled\n");
> > >  >  	}
> > >  >
> > >  > -	reset_gpios = of_get_named_gpio_flags(dev->of_node,
> > > "reset-gpios", 0,
> > >  > -					      &flags);
> > >  > -	if (gpio_is_valid(reset_gpios)) {
> > >  > -		ret = devm_gpio_request_one(dev, reset_gpios, flags,
> > >  > -					    "dm9000_reset");
> > >  > +	reset_gpio = devm_gpiod_get_optional(dev, "reset",
> > > GPIOD_OUT_HIGH);
> > >  > +	ret = PTR_ERR_OR_ZERO(reset_gpio);
> > >  > +	if (ret) {
> > >  > +		dev_err(dev, "failed to request reset gpio: %d\n", ret);
> > >  > +		goto out_regulator_disable;
> > >  > +	}
> > >  > +
> > >  > +	if (reset_gpio) {
> > >  > +		ret = gpiod_set_consumer_name(reset_gpio, "dm9000_reset");
> > >  >  		if (ret) {
> > >  > -			dev_err(dev, "failed to request reset gpio %d: %d\n",
> > >  > -				reset_gpios, ret);
> > >  > +			dev_err(dev, "failed to set reset gpio name: %d\n",
> > >  > +				ret);
> > >  >  			goto out_regulator_disable;
> > >  >  		}
> > >  >
> > >  >  		/* According to manual PWRST# Low Period Min 1ms */
> > >  >  		msleep(2);
> > >  > -		gpio_set_value(reset_gpios, 1);
> > >  > +		gpiod_set_value_cansleep(reset_gpio, 0);
> > > 
> > >  Why is that 1 magically turned into a 0?
> > 
> > Because gpiod uses logical states (think active/inactive), not absolute
> > ones. Here we are deasserting the reset line.
> > 
> > > 
> > >  On my CI20 board I can't get the DM9000 chip to probe correctly
> > > with this
> > >  patch (it fails to read the ID).
> > >  If I revert this patch then everything works fine.
> > 
> > Sorry, it is my fault of course: I missed that board has incorrect
> > annotation for the reset line. I will send out the patch below
> > (formatted properly of course):
> 
> So in *theory* you wouldn't fix it like that, because the driver should work
> with old Device Tree files, even if it had a broken property, as long as it
> used to work in the past.
> 
> The ci20.dts file however is always built into the kernel and I'm not aware
> of anybody doing things differently. As long as you make that explicit in
> your commit message I think Rob won't mind.
> 
> If he does, or if more boards are affected, an alternative is to switch the
> polarity of the GPIO in the driver, like so:
> 
> if (of_machine_is_compatible("mips,ci20") &&
>    gpiod_is_active_low(reset_gpio)) {
> 	gpiod_toggle_active_low(reset_gpio);
> }

Right, we are typically hiding this kind of quirks in gpiolib-of instead
of polluting drivers, but yes, it is possible.

Thanks.

-- 
Dmitry

Re: [PATCH 1/3] net: davicom: dm9000: switch to using gpiod API

[Andrew Lunn]

> > Why is that 1 magically turned into a 0?
> 
> Because gpiod uses logical states (think active/inactive), not absolute
> ones. Here we are deasserting the reset line.

This is the same question/answer you had with me. Maybe it is worth
putting this into the commit message for other patches in your series
to prevent this question/answer again and again.

   Andrew

Re: [PATCH 1/3] net: davicom: dm9000: switch to using gpiod API

[Dmitry Torokhov]

On Fri, Nov 18, 2022 at 06:50:54PM +0100, Andrew Lunn wrote:
> > > Why is that 1 magically turned into a 0?
> > 
> > Because gpiod uses logical states (think active/inactive), not absolute
> > ones. Here we are deasserting the reset line.
> 
> This is the same question/answer you had with me. Maybe it is worth
> putting this into the commit message for other patches in your series
> to prevent this question/answer again and again.

Right... Actually I think I'll go and define that GPIO_STATE_ACTIVE/
GPIO_STATE_INACTIVE and try to get Linus and Bart to accept it as code
speaks louder than words ;)

Thanks.

-- 
Dmitry

Re: [PATCH 1/3] net: davicom: dm9000: switch to using gpiod API

[Linus Walleij]

On Fri, Nov 18, 2022 at 6:55 PM Dmitry Torokhov
<dmitry.torokhov@gmail.com> wrote:
> On Fri, Nov 18, 2022 at 06:50:54PM +0100, Andrew Lunn wrote:
> > > > Why is that 1 magically turned into a 0?
> > >
> > > Because gpiod uses logical states (think active/inactive), not absolute
> > > ones. Here we are deasserting the reset line.
> >
> > This is the same question/answer you had with me. Maybe it is worth
> > putting this into the commit message for other patches in your series
> > to prevent this question/answer again and again.
>
> Right... Actually I think I'll go and define that GPIO_STATE_ACTIVE/
> GPIO_STATE_INACTIVE and try to get Linus and Bart to accept it as code
> speaks louder than words ;)

What I have said about that is that it should be accompanied by some sed
or cocinelle script to change this everywhere in the kernel instead
of using 0/1 to the gpiod_set/direction etc functions. Then Torvalds
can run that toward the end of the merge window to just change this
everywhere at once and be done with it.

The reason I want it that way is that I am royally tired of changes that
begin in one tiny corner and then the change keeps confusing users
for years until it is finally fixed up 15 kernel revisions later.

Since that has created a support nightmare in the past, I am now
advocating an all-or-nothing approach with that type of change.

Yours,
Linus Walleij