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 <[email protected]>
Reviewed-by: Linus Walleij <[email protected]>
Acked-by: Srinivas Kandagatla <[email protected]>
---
drivers/nvmem/core.c | 19 +++++++++++++++++--
drivers/nvmem/nvmem.h | 2 ++
include/linux/nvmem-provider.h | 3 +++
3 files changed, 22 insertions(+), 2 deletions(-)
diff --git a/drivers/nvmem/core.c b/drivers/nvmem/core.c
index 9f1ee9c766ec..3e1c94c4eee8 100644
--- 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);
+ if (IS_ERR(nvmem->wp_gpio))
+ return PTR_ERR(nvmem->wp_gpio);
+
kref_init(&nvmem->refcnt);
INIT_LIST_HEAD(&nvmem->cells);
diff --git a/drivers/nvmem/nvmem.h b/drivers/nvmem/nvmem.h
index eb8ed7121fa3..be0d66d75c8a 100644
--- a/drivers/nvmem/nvmem.h
+++ b/drivers/nvmem/nvmem.h
@@ -9,6 +9,7 @@
#include <linux/list.h>
#include <linux/nvmem-consumer.h>
#include <linux/nvmem-provider.h>
+#include <linux/gpio/consumer.h>
struct nvmem_device {
struct module *owner;
@@ -26,6 +27,7 @@ struct nvmem_device {
struct list_head cells;
nvmem_reg_read_t reg_read;
nvmem_reg_write_t reg_write;
+ struct gpio_desc *wp_gpio;
void *priv;
};
diff --git a/include/linux/nvmem-provider.h b/include/linux/nvmem-provider.h
index fe051323be0a..6d6f8e5d24c9 100644
--- a/include/linux/nvmem-provider.h
+++ b/include/linux/nvmem-provider.h
@@ -11,6 +11,7 @@
#include <linux/err.h>
#include <linux/errno.h>
+#include <linux/gpio/consumer.h>
struct nvmem_device;
struct nvmem_cell_info;
@@ -45,6 +46,7 @@ enum nvmem_type {
* @word_size: Minimum read/write access granularity.
* @stride: Minimum read/write access stride.
* @priv: User context passed to read/write callbacks.
+ * @wp-gpio: Write protect pin
*
* Note: A default "nvmem<id>" name will be assigned to the device if
* no name is specified in its configuration. In such case "<id>" is
@@ -58,6 +60,7 @@ struct nvmem_config {
const char *name;
int id;
struct module *owner;
+ struct gpio_desc *wp_gpio;
const struct nvmem_cell_info *cells;
int ncells;
enum nvmem_type type;
--
2.17.1
Hi Khouloud,
On Tue, Jan 7, 2020 at 10:30 AM Khouloud Touil <[email protected]> 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 <[email protected]>
> Reviewed-by: Linus Walleij <[email protected]>
> Acked-by: Srinivas Kandagatla <[email protected]>
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()?
Once that's implemented, I assume it will be auto-released on registration
failure by the call to put_device()?
> + if (IS_ERR(nvmem->wp_gpio))
> + return PTR_ERR(nvmem->wp_gpio);
> +
>
> kref_init(&nvmem->refcnt);
> INIT_LIST_HEAD(&nvmem->cells);
Gr{oetje,eeting}s,
Geert
--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- [email protected]
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
Le jeu. 30 janv. 2020 à 09:06, Geert Uytterhoeven
<[email protected]> a écrit :
>
> Hi Khouloud,
>
> On Tue, Jan 7, 2020 at 10:30 AM Khouloud Touil <[email protected]> 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 <[email protected]>
> > Reviewed-by: Linus Walleij <[email protected]>
> > Acked-by: Srinivas Kandagatla <[email protected]>
>
> 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()?
>
> Once that's implemented, I assume it will be auto-released on registration
> failure by the call to put_device()?
Hello Geert,
Thanks for your review.
Yes you are right, I will add that
Khouloud,
>
> > + if (IS_ERR(nvmem->wp_gpio))
> > + return PTR_ERR(nvmem->wp_gpio);
> > +
> >
> > kref_init(&nvmem->refcnt);
> > INIT_LIST_HEAD(&nvmem->cells);
>
> Gr{oetje,eeting}s,
>
> Geert
>
> --
> Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- [email protected]
>
> 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
czw., 30 sty 2020 o 09:06 Geert Uytterhoeven <[email protected]> napisał(a):
>
> Hi Khouloud,
>
> On Tue, Jan 7, 2020 at 10:30 AM Khouloud Touil <[email protected]> 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 <[email protected]>
> > Reviewed-by: Linus Walleij <[email protected]>
> > Acked-by: Srinivas Kandagatla <[email protected]>
>
> 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
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.
> 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?
Bart
Hi Bartosz,
On Mon, Feb 17, 2020 at 3:34 PM Bartosz Golaszewski <[email protected]> wrote:
> czw., 30 sty 2020 o 09:06 Geert Uytterhoeven <[email protected]> napisał(a):
> > On Tue, Jan 7, 2020 at 10:30 AM Khouloud Touil <[email protected]> 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 <[email protected]>
> > > Reviewed-by: Linus Walleij <[email protected]>
> > > Acked-by: Srinivas Kandagatla <[email protected]>
> >
> > 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 -- [email protected]
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