lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
Open Source and information security mailing list archives
| ||
|
Date: Fri, 18 Nov 2022 08:30:24 -0800 From: Dmitry Torokhov <dmitry.torokhov@...il.com> To: Paul Cercueil <paul@...pouillou.net> Cc: "David S. Miller" <davem@...emloft.net>, Andrew Lunn <andrew@...n.ch>, Heiner Kallweit <hkallweit1@...il.com>, Linus Walleij <linus.walleij@...aro.org>, Bartosz Golaszewski <brgl@...ev.pl>, netdev@...r.kernel.org, linux-kernel@...r.kernel.org 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 > <dmitry.torokhov@...il.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@...il.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@...il.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
Powered by blists - more mailing lists