LinuxLists.cc - [PATCH 1/3] net: davicom: dm9000: switch to using gpiod API

2022-09-06 21:05:18

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

2022-09-06 21:41:40

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

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 <[email protected]>
---
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

2022-09-07 22:13:24

by Linus Walleij

[permalink] [raw]

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

On Tue, Sep 6, 2022 at 10:49 PM Dmitry Torokhov
<[email protected]> 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 <[email protected]>

Reviewed-by: Linus Walleij <[email protected]>

Yours,
Linus Walleij

2022-09-07 22:21:30

by Linus Walleij

[permalink] [raw]

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

2022-09-08 12:10:23

by Andrew Lunn

[permalink] [raw]

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

On Wed, Sep 07, 2022 at 11:45:48PM +0200, Linus Walleij wrote:
> On Tue, Sep 6, 2022 at 10:49 PM Dmitry Torokhov
> <[email protected]> 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 <[email protected]>
>
> Reviewed-by: Linus Walleij <[email protected]>
>
> 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

2022-09-15 10:36:23

by Paolo Abeni

[permalink] [raw]

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

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
> > <[email protected]> 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 <[email protected]>
> >
> > Reviewed-by: Linus Walleij <[email protected]>
> >
> > 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

2022-09-15 10:53:33

by patchwork-bot+netdevbpf

[permalink] [raw]

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

Hello:

This series was applied to netdev/net-next.git (master)
by Paolo Abeni <[email protected]>:

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 <[email protected]>
> ---
> 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

2022-11-18 16:09:23

by Dmitry Torokhov

[permalink] [raw]

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

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
> <[email protected]> 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 <[email protected]>
> > ---
> > 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

2022-11-18 16:32:49

by Paul Cercueil

[permalink] [raw]

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

Hi Dmitry,

Le mar. 6 sept. 2022 ? 13:49:20 -0700, Dmitry Torokhov
<[email protected]> 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 <[email protected]>
> ---
> 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
>

2022-11-18 16:41:29

by Dmitry Torokhov

[permalink] [raw]

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

On Fri, Nov 18, 2022 at 04:26:38PM +0000, Paul Cercueil wrote:
>
>
> Le ven. 18 nov. 2022 ? 07:58:21 -0800, Dmitry Torokhov
> <[email protected]> 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
> > > <[email protected]> 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 <[email protected]>
> > > > ---
> > > > 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

2022-11-18 17:56:22

by Paul Cercueil

[permalink] [raw]

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

Le ven. 18 nov. 2022 ? 07:58:21 -0800, Dmitry Torokhov
<[email protected]> 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
>> <[email protected]> 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 <[email protected]>
>> > ---
>> > 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

2022-11-18 19:23:01

by Dmitry Torokhov

[permalink] [raw]

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

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

2022-11-18 19:41:24

by Andrew Lunn

[permalink] [raw]

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

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

2022-11-21 13:45:59

by Linus Walleij

[permalink] [raw]

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

On Fri, Nov 18, 2022 at 6:55 PM Dmitry Torokhov
<[email protected]> 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