From: Geert Uytterhoeven <geert@linux-m68k.org> To: Bartosz Golaszewski <brgl@bgdev.pl> Cc: Khouloud Touil <ktouil@baylibre.com>, Bartosz Golaszewski <bgolaszewski@baylibre.com>, Rob Herring <robh+dt@kernel.org>, Mark Rutland <mark.rutland@arm.com>, Srinivas Kandagatla <srinivas.kandagatla@linaro.org>, baylibre-upstreaming@groups.io, Linux Kernel Mailing List <linux-kernel@vger.kernel.org>, "open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS" <devicetree@vger.kernel.org>, Linux I2C <linux-i2c@vger.kernel.org>, Linus Walleij <linus.walleij@linaro.org> Subject: Re: [PATCH v4 2/5] nvmem: add support for the write-protect pin Date: Mon, 17 Feb 2020 16:11:35 +0100 [thread overview] Message-ID: <CAMuHMdWrJ9LmDqBQYvNVs7yY78Po0sTGc=MUu9+tau2frJ9Ytw@mail.gmail.com> (raw) In-Reply-To: <CAMRc=Mf0-gQJH8Se4sFBCkRNE=b4ww=SWges-7GPD2jsivrv1Q@mail.gmail.com> Hi Bartosz, On Mon, Feb 17, 2020 at 3:34 PM Bartosz Golaszewski <brgl@bgdev.pl> wrote: > czw., 30 sty 2020 o 09:06 Geert Uytterhoeven <geert@linux-m68k.org> napisał(a): > > On Tue, Jan 7, 2020 at 10:30 AM Khouloud Touil <ktouil@baylibre.com> wrote: > > > The write-protect pin handling looks like a standard property that > > > could benefit other users if available in the core nvmem framework. > > > > > > Instead of modifying all the memory drivers to check this pin, make > > > the NVMEM subsystem check if the write-protect GPIO being passed > > > through the nvmem_config or defined in the device tree and pull it > > > low whenever writing to the memory. > > > > > > There was a suggestion for introducing the gpiodesc from pdata, but > > > as pdata is already removed it could be replaced by adding it to > > > nvmem_config. > > > > > > Reference: https://lists.96boards.org/pipermail/dev/2018-August/001056.html > > > > > > Signed-off-by: Khouloud Touil <ktouil@baylibre.com> > > > Reviewed-by: Linus Walleij <linus.walleij@linaro.org> > > > Acked-by: Srinivas Kandagatla <srinivas.kandagatla@linaro.org> > > > > Thanks for your patch! > > > > > --- a/drivers/nvmem/core.c > > > +++ b/drivers/nvmem/core.c > > > @@ -15,6 +15,7 @@ > > > #include <linux/module.h> > > > #include <linux/nvmem-consumer.h> > > > #include <linux/nvmem-provider.h> > > > +#include <linux/gpio/consumer.h> > > > #include <linux/of.h> > > > #include <linux/slab.h> > > > #include "nvmem.h" > > > @@ -54,8 +55,14 @@ static int nvmem_reg_read(struct nvmem_device *nvmem, unsigned int offset, > > > static int nvmem_reg_write(struct nvmem_device *nvmem, unsigned int offset, > > > void *val, size_t bytes) > > > { > > > - if (nvmem->reg_write) > > > - return nvmem->reg_write(nvmem->priv, offset, val, bytes); > > > + int ret; > > > + > > > + if (nvmem->reg_write) { > > > + gpiod_set_value_cansleep(nvmem->wp_gpio, 0); > > > + ret = nvmem->reg_write(nvmem->priv, offset, val, bytes); > > > + gpiod_set_value_cansleep(nvmem->wp_gpio, 1); > > > + return ret; > > > + } > > > > > > return -EINVAL; > > > } > > > @@ -338,6 +345,14 @@ struct nvmem_device *nvmem_register(const struct nvmem_config *config) > > > kfree(nvmem); > > > return ERR_PTR(rval); > > > } > > > + if (config->wp_gpio) > > > + nvmem->wp_gpio = config->wp_gpio; > > > + else > > > + nvmem->wp_gpio = gpiod_get_optional(config->dev, "wp", > > > + GPIOD_OUT_HIGH); > > > > Shouldn't this GPIO be released in nvmem_release(), by calling gpiod_put()? > > > > Hi Geert, > > Khouloud already sent out a patch but I think it still doesn't fix all > the problems. > > While we should call gpiod_put() for the descs we request - we must > not do it for the desc we get over the config structure. Unless... we That's true. > make descs reference counted with kref and add gpiod_ref() helper. > That way we could increase the reference counter in the upper branch > of the if and not do it in the lower. Calling gpiod_put() would > internally call kref_put(). Does it make sense? I think that a > function that's called gpiod_put() but doesn't really use reference > counting is misleading anyway. Yep. > > Once that's implemented, I assume it will be auto-released on registration > > failure by the call to put_device()? > > No, I think this is another leak - why would put_device() lead to > freeing any resources? Am I missing something? Sorry, I don't remember why I wrote that part... Anyway, requested GPIOs should be released on failure, and on unregistration. Gr{oetje,eeting}s, Geert -- Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org In personal conversations with technical people, I call myself a hacker. But when I'm talking to journalists I just say "programmer" or something like that. -- Linus Torvalds
WARNING: multiple messages have this Message-ID (diff)
From: Geert Uytterhoeven <geert-Td1EMuHUCqxL1ZNQvxDV9g@public.gmane.org> To: Bartosz Golaszewski <brgl-ARrdPY/1zhM@public.gmane.org> Cc: Khouloud Touil <ktouil-rdvid1DuHRBWk0Htik3J/w@public.gmane.org>, Bartosz Golaszewski <bgolaszewski-rdvid1DuHRBWk0Htik3J/w@public.gmane.org>, Rob Herring <robh+dt-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org>, Mark Rutland <mark.rutland-5wv7dgnIgG8@public.gmane.org>, Srinivas Kandagatla <srinivas.kandagatla-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org>, baylibre-upstreaming-GWfripvEmMdhl2p70BpVqQ@public.gmane.org, Linux Kernel Mailing List <linux-kernel-u79uwXL29TY76Z2rM5mHXA@public.gmane.org>, "open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS" <devicetree-u79uwXL29TY76Z2rM5mHXA@public.gmane.org>, Linux I2C <linux-i2c-u79uwXL29TY76Z2rM5mHXA@public.gmane.org>, Linus Walleij <linus.walleij-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org> Subject: Re: [PATCH v4 2/5] nvmem: add support for the write-protect pin Date: Mon, 17 Feb 2020 16:11:35 +0100 [thread overview] Message-ID: <CAMuHMdWrJ9LmDqBQYvNVs7yY78Po0sTGc=MUu9+tau2frJ9Ytw@mail.gmail.com> (raw) In-Reply-To: <CAMRc=Mf0-gQJH8Se4sFBCkRNE=b4ww=SWges-7GPD2jsivrv1Q-JsoAwUIsXosN+BqQ9rBEUg@public.gmane.org> Hi Bartosz, On Mon, Feb 17, 2020 at 3:34 PM Bartosz Golaszewski <brgl-ARrdPY/1zhM@public.gmane.org> wrote: > czw., 30 sty 2020 o 09:06 Geert Uytterhoeven <geert-Td1EMuHUCqxL1ZNQvxDV9g@public.gmane.org> napisał(a): > > On Tue, Jan 7, 2020 at 10:30 AM Khouloud Touil <ktouil-rdvid1DuHRBWk0Htik3J/w@public.gmane.org> wrote: > > > The write-protect pin handling looks like a standard property that > > > could benefit other users if available in the core nvmem framework. > > > > > > Instead of modifying all the memory drivers to check this pin, make > > > the NVMEM subsystem check if the write-protect GPIO being passed > > > through the nvmem_config or defined in the device tree and pull it > > > low whenever writing to the memory. > > > > > > There was a suggestion for introducing the gpiodesc from pdata, but > > > as pdata is already removed it could be replaced by adding it to > > > nvmem_config. > > > > > > Reference: https://lists.96boards.org/pipermail/dev/2018-August/001056.html > > > > > > Signed-off-by: Khouloud Touil <ktouil-rdvid1DuHRBWk0Htik3J/w@public.gmane.org> > > > Reviewed-by: Linus Walleij <linus.walleij-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org> > > > Acked-by: Srinivas Kandagatla <srinivas.kandagatla-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org> > > > > Thanks for your patch! > > > > > --- a/drivers/nvmem/core.c > > > +++ b/drivers/nvmem/core.c > > > @@ -15,6 +15,7 @@ > > > #include <linux/module.h> > > > #include <linux/nvmem-consumer.h> > > > #include <linux/nvmem-provider.h> > > > +#include <linux/gpio/consumer.h> > > > #include <linux/of.h> > > > #include <linux/slab.h> > > > #include "nvmem.h" > > > @@ -54,8 +55,14 @@ static int nvmem_reg_read(struct nvmem_device *nvmem, unsigned int offset, > > > static int nvmem_reg_write(struct nvmem_device *nvmem, unsigned int offset, > > > void *val, size_t bytes) > > > { > > > - if (nvmem->reg_write) > > > - return nvmem->reg_write(nvmem->priv, offset, val, bytes); > > > + int ret; > > > + > > > + if (nvmem->reg_write) { > > > + gpiod_set_value_cansleep(nvmem->wp_gpio, 0); > > > + ret = nvmem->reg_write(nvmem->priv, offset, val, bytes); > > > + gpiod_set_value_cansleep(nvmem->wp_gpio, 1); > > > + return ret; > > > + } > > > > > > return -EINVAL; > > > } > > > @@ -338,6 +345,14 @@ struct nvmem_device *nvmem_register(const struct nvmem_config *config) > > > kfree(nvmem); > > > return ERR_PTR(rval); > > > } > > > + if (config->wp_gpio) > > > + nvmem->wp_gpio = config->wp_gpio; > > > + else > > > + nvmem->wp_gpio = gpiod_get_optional(config->dev, "wp", > > > + GPIOD_OUT_HIGH); > > > > Shouldn't this GPIO be released in nvmem_release(), by calling gpiod_put()? > > > > Hi Geert, > > Khouloud already sent out a patch but I think it still doesn't fix all > the problems. > > While we should call gpiod_put() for the descs we request - we must > not do it for the desc we get over the config structure. Unless... we That's true. > make descs reference counted with kref and add gpiod_ref() helper. > That way we could increase the reference counter in the upper branch > of the if and not do it in the lower. Calling gpiod_put() would > internally call kref_put(). Does it make sense? I think that a > function that's called gpiod_put() but doesn't really use reference > counting is misleading anyway. Yep. > > Once that's implemented, I assume it will be auto-released on registration > > failure by the call to put_device()? > > No, I think this is another leak - why would put_device() lead to > freeing any resources? Am I missing something? Sorry, I don't remember why I wrote that part... Anyway, requested GPIOs should be released on failure, and on unregistration. Gr{oetje,eeting}s, Geert -- Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert-Td1EMuHUCqxL1ZNQvxDV9g@public.gmane.org In personal conversations with technical people, I call myself a hacker. But when I'm talking to journalists I just say "programmer" or something like that. -- Linus Torvalds
next prev parent reply other threads:[~2020-02-17 15:11 UTC|newest] Thread overview: 28+ messages / expand[flat|nested] mbox.gz Atom feed top 2020-01-07 9:29 [PATCH v4 0/5] at24: move write-protect pin handling to nvmem core Khouloud Touil 2020-01-07 9:29 ` [PATCH v4 1/5] dt-bindings: nvmem: new optional property wp-gpios Khouloud Touil 2020-01-07 9:50 ` Linus Walleij 2020-01-07 9:50 ` Linus Walleij 2020-01-08 20:54 ` Rob Herring 2020-01-08 20:54 ` Rob Herring 2020-01-07 9:29 ` [PATCH v4 2/5] nvmem: add support for the write-protect pin Khouloud Touil 2020-01-30 8:06 ` Geert Uytterhoeven 2020-01-30 8:06 ` Geert Uytterhoeven 2020-02-17 13:14 ` Khouloud Touil 2020-02-17 13:14 ` Khouloud Touil 2020-02-17 14:34 ` Bartosz Golaszewski 2020-02-17 14:34 ` Bartosz Golaszewski 2020-02-17 15:11 ` Geert Uytterhoeven [this message] 2020-02-17 15:11 ` Geert Uytterhoeven 2020-01-07 9:29 ` [PATCH v4 3/5] dt-bindings: at24: make wp-gpios a reference to the property defined by nvmem Khouloud Touil 2020-01-08 20:54 ` Rob Herring 2020-01-08 20:54 ` Rob Herring 2020-01-07 9:29 ` [PATCH v4 4/5] dt-bindings: at25: add reference for the wp-gpios property Khouloud Touil 2020-01-08 20:54 ` Rob Herring 2020-01-08 20:54 ` Rob Herring 2020-01-09 9:47 ` Bartosz Golaszewski 2020-01-09 9:47 ` Bartosz Golaszewski 2020-01-14 14:42 ` Greg KH 2020-01-14 15:05 ` Bartosz Golaszewski 2020-01-14 15:05 ` Bartosz Golaszewski 2020-01-07 9:29 ` [PATCH v4 5/5] eeprom: at24: remove the write-protect pin support Khouloud Touil 2020-01-09 10:31 ` [PATCH v4 0/5] at24: move write-protect pin handling to nvmem core Bartosz Golaszewski
Reply instructions: You may reply publicly to this message via plain-text email using any one of the following methods: * Save the following mbox file, import it into your mail client, and reply-to-all from there: mbox Avoid top-posting and favor interleaved quoting: https://en.wikipedia.org/wiki/Posting_style#Interleaved_style * Reply using the --to, --cc, and --in-reply-to switches of git-send-email(1): git send-email \ --in-reply-to='CAMuHMdWrJ9LmDqBQYvNVs7yY78Po0sTGc=MUu9+tau2frJ9Ytw@mail.gmail.com' \ --to=geert@linux-m68k.org \ --cc=baylibre-upstreaming@groups.io \ --cc=bgolaszewski@baylibre.com \ --cc=brgl@bgdev.pl \ --cc=devicetree@vger.kernel.org \ --cc=ktouil@baylibre.com \ --cc=linus.walleij@linaro.org \ --cc=linux-i2c@vger.kernel.org \ --cc=linux-kernel@vger.kernel.org \ --cc=mark.rutland@arm.com \ --cc=robh+dt@kernel.org \ --cc=srinivas.kandagatla@linaro.org \ /path/to/YOUR_REPLY https://kernel.org/pub/software/scm/git/docs/git-send-email.html * If your mail client supports setting the In-Reply-To header via mailto: links, try the mailto: linkBe sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.