[PATCH v4 0/5] at24: move write-protect pin handling to nvmem core

[PATCH v4 0/5] at24: move write-protect pin handling to nvmem core

[Khouloud Touil]

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

This patchset:

- adds support for the write-protect pin split into two parts.
The first patch modifies modifies the relevant binding document,
while the second modifies the nvmem code to pull the write-protect
GPIO low (if present) during write operations.

- removes support for the write-protect pin split into two parts.
The first patch modifies the relevant binding document to make the
wp-gpio a reference to the property defined by nvmem , while the
second removes the relevant code in the at24 driver.

- adds reference in the at25 binding document for the wp-gpios property
as it uses nvmem subsystem.

Changes since v1:
-Add an explenation on how the wp-gpios works
-Keep reference to the wp-gpios in the at24 binding

Changes since v2:
-Use the flag GPIO_ACTIVE_HIGH instead of 0

Changes since v3:
-Keep the example of the wp-gpios in the at25 bindings
-Add reference for the wp-gpios property in the at25 binding


Khouloud Touil (5):
  dt-bindings: nvmem: new optional property wp-gpios
  nvmem: add support for the write-protect pin
  dt-bindings: at24: make wp-gpios a reference to the property defined
    by nvmem
  dt-bindings: at25: add reference for the wp-gpios property
  eeprom: at24: remove the write-protect pin support

 .../devicetree/bindings/eeprom/at24.yaml      |  5 +----
 .../devicetree/bindings/eeprom/at25.txt       |  2 ++
 .../devicetree/bindings/nvmem/nvmem.yaml      | 11 +++++++++++
 drivers/misc/eeprom/at24.c                    |  9 ---------
 drivers/nvmem/core.c                          | 19 +++++++++++++++++--
 drivers/nvmem/nvmem.h                         |  2 ++
 include/linux/nvmem-provider.h                |  3 +++
 7 files changed, 36 insertions(+), 15 deletions(-)

-- 
2.17.1

[PATCH v4 1/5] dt-bindings: nvmem: new optional property wp-gpios

[Khouloud Touil]

Several memories have a write-protect pin, that when pulled high, it
blocks the write operation.

On some boards, this pin is connected to a GPIO and pulled high by
default, which forces the user to manually change its state before
writing.

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.

Add a new optional property to the device tree binding document, which
allows to specify the GPIO line to which the write-protect pin is
connected.

Signed-off-by: Khouloud Touil <ktouil@baylibre.com>
---
 Documentation/devicetree/bindings/nvmem/nvmem.yaml | 11 +++++++++++
 1 file changed, 11 insertions(+)

diff --git a/Documentation/devicetree/bindings/nvmem/nvmem.yaml b/Documentation/devicetree/bindings/nvmem/nvmem.yaml
index 1c75a059206c..b43c6c65294e 100644
--- a/Documentation/devicetree/bindings/nvmem/nvmem.yaml
+++ b/Documentation/devicetree/bindings/nvmem/nvmem.yaml
@@ -34,6 +34,14 @@ properties:
     description:
       Mark the provider as read only.
 
+  wp-gpios:
+    description:
+      GPIO to which the write-protect pin of the chip is connected.
+      The write-protect GPIO is asserted, when it's driven high
+      (logical '1') to block the write operation. It's deasserted,
+      when it's driven low (logical '0') to allow writing.
+    maxItems: 1
+
 patternProperties:
   "^.*@[0-9a-f]+$":
     type: object
@@ -63,9 +71,12 @@ patternProperties:
 
 examples:
   - |
+      #include <dt-bindings/gpio/gpio.h>
+
       qfprom: eeprom@700000 {
           #address-cells = <1>;
           #size-cells = <1>;
+          wp-gpios = <&gpio1 3 GPIO_ACTIVE_HIGH>;
 
           /* ... */
 
-- 
2.17.1

[PATCH v4 2/5] nvmem: add support for the write-protect pin

[Khouloud Touil]

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

[PATCH v4 3/5] dt-bindings: at24: make wp-gpios a reference to the property defined by nvmem

[Khouloud Touil]

NVMEM framework is an interface for the at24 EEPROMs as well as for
other drivers, instead of passing the wp-gpios over the different
drivers each time, it would be better to pass it over the NVMEM
subsystem once and for all.

Making wp-gpios a reference to the property defined by nvmem.

Signed-off-by: Khouloud Touil <ktouil@baylibre.com>
Reviewed-by: Linus Walleij <linus.walleij@linaro.org>
---
 Documentation/devicetree/bindings/eeprom/at24.yaml | 5 +----
 1 file changed, 1 insertion(+), 4 deletions(-)

diff --git a/Documentation/devicetree/bindings/eeprom/at24.yaml b/Documentation/devicetree/bindings/eeprom/at24.yaml
index e8778560d966..767959941399 100644
--- a/Documentation/devicetree/bindings/eeprom/at24.yaml
+++ b/Documentation/devicetree/bindings/eeprom/at24.yaml
@@ -145,10 +145,7 @@ properties:
       over reads to the next slave address. Please consult the manual of
       your device.
 
-  wp-gpios:
-    description:
-      GPIO to which the write-protect pin of the chip is connected.
-    maxItems: 1
+  wp-gpios: true
 
   address-width:
     allOf:
-- 
2.17.1

[PATCH v4 4/5] dt-bindings: at25: add reference for the wp-gpios property

[Khouloud Touil]

As the at25 uses the NVMEM subsystem, and the property is now being
handled, adding reference for it in the device tree binding document,
which allows to specify the GPIO line to which the write-protect pin
is connected.

Signed-off-by: Khouloud Touil <ktouil@baylibre.com>
---
 Documentation/devicetree/bindings/eeprom/at25.txt | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/Documentation/devicetree/bindings/eeprom/at25.txt b/Documentation/devicetree/bindings/eeprom/at25.txt
index 42577dd113dd..fcacd97abd0a 100644
--- a/Documentation/devicetree/bindings/eeprom/at25.txt
+++ b/Documentation/devicetree/bindings/eeprom/at25.txt
@@ -20,6 +20,7 @@ Optional properties:
 - spi-cpha : SPI shifted clock phase, as per spi-bus bindings.
 - spi-cpol : SPI inverse clock polarity, as per spi-bus bindings.
 - read-only : this parameter-less property disables writes to the eeprom
+- wp-gpios : GPIO to which the write-protect pin of the chip is connected
 
 Obsolete legacy properties can be used in place of "size", "pagesize",
 "address-width", and "read-only":
@@ -36,6 +37,7 @@ Example:
 		spi-max-frequency = <5000000>;
 		spi-cpha;
 		spi-cpol;
+		wp-gpios = <&gpio1 3 0>;
 
 		pagesize = <64>;
 		size = <32768>;
-- 
2.17.1

[PATCH v4 5/5] eeprom: at24: remove the write-protect pin support

[Khouloud Touil]

NVMEM framework is an interface for the at24 EEPROMs as well as for
other drivers, instead of passing the wp-gpios over the different
drivers each time, it would be better to pass it over the NVMEM
subsystem once and for all.

Removing the support for the write-protect pin after adding it to the
NVMEM subsystem.

Signed-off-by: Khouloud Touil <ktouil@baylibre.com>
Reviewed-by: Linus Walleij <linus.walleij@linaro.org>
---
 drivers/misc/eeprom/at24.c | 9 ---------
 1 file changed, 9 deletions(-)

diff --git a/drivers/misc/eeprom/at24.c b/drivers/misc/eeprom/at24.c
index 0681d5fdd538..8fce49a6d9cd 100644
--- a/drivers/misc/eeprom/at24.c
+++ b/drivers/misc/eeprom/at24.c
@@ -22,7 +22,6 @@
 #include <linux/nvmem-provider.h>
 #include <linux/regmap.h>
 #include <linux/pm_runtime.h>
-#include <linux/gpio/consumer.h>
 
 /* Address pointer is 16 bit. */
 #define AT24_FLAG_ADDR16	BIT(7)
@@ -89,8 +88,6 @@ struct at24_data {
 
 	struct nvmem_device *nvmem;
 
-	struct gpio_desc *wp_gpio;
-
 	/*
 	 * Some chips tie up multiple I2C addresses; dummy devices reserve
 	 * them for us, and we'll use them with SMBus calls.
@@ -457,12 +454,10 @@ static int at24_write(void *priv, unsigned int off, void *val, size_t count)
 	 * from this host, but not from other I2C masters.
 	 */
 	mutex_lock(&at24->lock);
-	gpiod_set_value_cansleep(at24->wp_gpio, 0);
 
 	while (count) {
 		ret = at24_regmap_write(at24, buf, off, count);
 		if (ret < 0) {
-			gpiod_set_value_cansleep(at24->wp_gpio, 1);
 			mutex_unlock(&at24->lock);
 			pm_runtime_put(dev);
 			return ret;
@@ -472,7 +467,6 @@ static int at24_write(void *priv, unsigned int off, void *val, size_t count)
 		count -= ret;
 	}
 
-	gpiod_set_value_cansleep(at24->wp_gpio, 1);
 	mutex_unlock(&at24->lock);
 
 	pm_runtime_put(dev);
@@ -662,9 +656,6 @@ static int at24_probe(struct i2c_client *client)
 	at24->client[0].client = client;
 	at24->client[0].regmap = regmap;
 
-	at24->wp_gpio = devm_gpiod_get_optional(dev, "wp", GPIOD_OUT_HIGH);
-	if (IS_ERR(at24->wp_gpio))
-		return PTR_ERR(at24->wp_gpio);
 
 	writable = !(flags & AT24_FLAG_READONLY);
 	if (writable) {
-- 
2.17.1

Re: [PATCH v4 1/5] dt-bindings: nvmem: new optional property wp-gpios

[Linus Walleij]

On Tue, Jan 7, 2020 at 10:29 AM Khouloud Touil <ktouil@baylibre.com> wrote:

> Several memories have a write-protect pin, that when pulled high, it
> blocks the write operation.
>
> On some boards, this pin is connected to a GPIO and pulled high by
> default, which forces the user to manually change its state before
> writing.
>
> 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.
>
> Add a new optional property to the device tree binding document, which
> allows to specify the GPIO line to which the write-protect pin is
> connected.
>
> Signed-off-by: Khouloud Touil <ktouil@baylibre.com>

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

Yours,
Linus Walleij

Re: [PATCH v4 1/5] dt-bindings: nvmem: new optional property wp-gpios

[Linus Walleij]

On Tue, Jan 7, 2020 at 10:29 AM Khouloud Touil <ktouil@baylibre.com> wrote:

> Several memories have a write-protect pin, that when pulled high, it
> blocks the write operation.
>
> On some boards, this pin is connected to a GPIO and pulled high by
> default, which forces the user to manually change its state before
> writing.
>
> 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.
>
> Add a new optional property to the device tree binding document, which
> allows to specify the GPIO line to which the write-protect pin is
> connected.
>
> Signed-off-by: Khouloud Touil <ktouil@baylibre.com>

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

Yours,
Linus Walleij

Re: [PATCH v4 1/5] dt-bindings: nvmem: new optional property wp-gpios

[Rob Herring]

On Tue,  7 Jan 2020 10:29:18 +0100, Khouloud Touil wrote:
> Several memories have a write-protect pin, that when pulled high, it
> blocks the write operation.
> 
> On some boards, this pin is connected to a GPIO and pulled high by
> default, which forces the user to manually change its state before
> writing.
> 
> 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.
> 
> Add a new optional property to the device tree binding document, which
> allows to specify the GPIO line to which the write-protect pin is
> connected.
> 
> Signed-off-by: Khouloud Touil <ktouil@baylibre.com>
> ---
>  Documentation/devicetree/bindings/nvmem/nvmem.yaml | 11 +++++++++++
>  1 file changed, 11 insertions(+)
> 

Reviewed-by: Rob Herring <robh@kernel.org>

Re: [PATCH v4 1/5] dt-bindings: nvmem: new optional property wp-gpios

[Rob Herring]

On Tue,  7 Jan 2020 10:29:18 +0100, Khouloud Touil wrote:
> Several memories have a write-protect pin, that when pulled high, it
> blocks the write operation.
> 
> On some boards, this pin is connected to a GPIO and pulled high by
> default, which forces the user to manually change its state before
> writing.
> 
> 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.
> 
> Add a new optional property to the device tree binding document, which
> allows to specify the GPIO line to which the write-protect pin is
> connected.
> 
> Signed-off-by: Khouloud Touil <ktouil@baylibre.com>
> ---
>  Documentation/devicetree/bindings/nvmem/nvmem.yaml | 11 +++++++++++
>  1 file changed, 11 insertions(+)
> 

Reviewed-by: Rob Herring <robh@kernel.org>

Re: [PATCH v4 3/5] dt-bindings: at24: make wp-gpios a reference to the property defined by nvmem

[Rob Herring]

On Tue,  7 Jan 2020 10:29:20 +0100, Khouloud Touil wrote:
> NVMEM framework is an interface for the at24 EEPROMs as well as for
> other drivers, instead of passing the wp-gpios over the different
> drivers each time, it would be better to pass it over the NVMEM
> subsystem once and for all.
> 
> Making wp-gpios a reference to the property defined by nvmem.
> 
> Signed-off-by: Khouloud Touil <ktouil@baylibre.com>
> Reviewed-by: Linus Walleij <linus.walleij@linaro.org>
> ---
>  Documentation/devicetree/bindings/eeprom/at24.yaml | 5 +----
>  1 file changed, 1 insertion(+), 4 deletions(-)
> 

Reviewed-by: Rob Herring <robh@kernel.org>

Re: [PATCH v4 3/5] dt-bindings: at24: make wp-gpios a reference to the property defined by nvmem

[Rob Herring]

On Tue,  7 Jan 2020 10:29:20 +0100, Khouloud Touil wrote:
> NVMEM framework is an interface for the at24 EEPROMs as well as for
> other drivers, instead of passing the wp-gpios over the different
> drivers each time, it would be better to pass it over the NVMEM
> subsystem once and for all.
> 
> Making wp-gpios a reference to the property defined by nvmem.
> 
> Signed-off-by: Khouloud Touil <ktouil@baylibre.com>
> Reviewed-by: Linus Walleij <linus.walleij@linaro.org>
> ---
>  Documentation/devicetree/bindings/eeprom/at24.yaml | 5 +----
>  1 file changed, 1 insertion(+), 4 deletions(-)
> 

Reviewed-by: Rob Herring <robh@kernel.org>

Re: [PATCH v4 4/5] dt-bindings: at25: add reference for the wp-gpios property

[Rob Herring]

On Tue,  7 Jan 2020 10:29:21 +0100, Khouloud Touil wrote:
> As the at25 uses the NVMEM subsystem, and the property is now being
> handled, adding reference for it in the device tree binding document,
> which allows to specify the GPIO line to which the write-protect pin
> is connected.
> 
> Signed-off-by: Khouloud Touil <ktouil@baylibre.com>
> ---
>  Documentation/devicetree/bindings/eeprom/at25.txt | 2 ++
>  1 file changed, 2 insertions(+)
> 

Reviewed-by: Rob Herring <robh@kernel.org>

Re: [PATCH v4 4/5] dt-bindings: at25: add reference for the wp-gpios property

[Rob Herring]

On Tue,  7 Jan 2020 10:29:21 +0100, Khouloud Touil wrote:
> As the at25 uses the NVMEM subsystem, and the property is now being
> handled, adding reference for it in the device tree binding document,
> which allows to specify the GPIO line to which the write-protect pin
> is connected.
> 
> Signed-off-by: Khouloud Touil <ktouil@baylibre.com>
> ---
>  Documentation/devicetree/bindings/eeprom/at25.txt | 2 ++
>  1 file changed, 2 insertions(+)
> 

Reviewed-by: Rob Herring <robh@kernel.org>

Re: [PATCH v4 4/5] dt-bindings: at25: add reference for the wp-gpios property

[Bartosz Golaszewski]

śr., 8 sty 2020 o 21:54 Rob Herring <robh@kernel.org> napisał(a):
>
> On Tue,  7 Jan 2020 10:29:21 +0100, Khouloud Touil wrote:
> > As the at25 uses the NVMEM subsystem, and the property is now being
> > handled, adding reference for it in the device tree binding document,
> > which allows to specify the GPIO line to which the write-protect pin
> > is connected.
> >
> > Signed-off-by: Khouloud Touil <ktouil@baylibre.com>
> > ---
> >  Documentation/devicetree/bindings/eeprom/at25.txt | 2 ++
> >  1 file changed, 2 insertions(+)
> >
>
> Reviewed-by: Rob Herring <robh@kernel.org>

Hi Greg,

AT25 patches usually go through the char-misc tree. In this case
however, the change depends on the other patches in this series. Can
you ack this and I'll take it through the AT24 tree exceptionally?

Best regards,
Bartosz Golaszewski

Re: [PATCH v4 4/5] dt-bindings: at25: add reference for the wp-gpios property

[Bartosz Golaszewski]

śr., 8 sty 2020 o 21:54 Rob Herring <robh-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org> napisał(a):
>
> On Tue,  7 Jan 2020 10:29:21 +0100, Khouloud Touil wrote:
> > As the at25 uses the NVMEM subsystem, and the property is now being
> > handled, adding reference for it in the device tree binding document,
> > which allows to specify the GPIO line to which the write-protect pin
> > is connected.
> >
> > Signed-off-by: Khouloud Touil <ktouil-rdvid1DuHRBWk0Htik3J/w@public.gmane.org>
> > ---
> >  Documentation/devicetree/bindings/eeprom/at25.txt | 2 ++
> >  1 file changed, 2 insertions(+)
> >
>
> Reviewed-by: Rob Herring <robh-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org>

Hi Greg,

AT25 patches usually go through the char-misc tree. In this case
however, the change depends on the other patches in this series. Can
you ack this and I'll take it through the AT24 tree exceptionally?

Best regards,
Bartosz Golaszewski

Re: [PATCH v4 0/5] at24: move write-protect pin handling to nvmem core

[Bartosz Golaszewski]

wt., 7 sty 2020 o 10:29 Khouloud Touil <ktouil@baylibre.com> napisał(a):
>
> 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 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.
>
> This patchset:
>
> - adds support for the write-protect pin split into two parts.
> The first patch modifies modifies the relevant binding document,
> while the second modifies the nvmem code to pull the write-protect
> GPIO low (if present) during write operations.
>
> - removes support for the write-protect pin split into two parts.
> The first patch modifies the relevant binding document to make the
> wp-gpio a reference to the property defined by nvmem , while the
> second removes the relevant code in the at24 driver.
>
> - adds reference in the at25 binding document for the wp-gpios property
> as it uses nvmem subsystem.
>
> Changes since v1:
> -Add an explenation on how the wp-gpios works
> -Keep reference to the wp-gpios in the at24 binding
>
> Changes since v2:
> -Use the flag GPIO_ACTIVE_HIGH instead of 0
>
> Changes since v3:
> -Keep the example of the wp-gpios in the at25 bindings
> -Add reference for the wp-gpios property in the at25 binding

I picked up patches 1-3 & 5 into the at24 tree.

Patch 4 will need an Ack from Greg.

Bart

Re: [PATCH v4 4/5] dt-bindings: at25: add reference for the wp-gpios property

[Greg KH]

On Thu, Jan 09, 2020 at 10:47:56AM +0100, Bartosz Golaszewski wrote:
> śr., 8 sty 2020 o 21:54 Rob Herring <robh@kernel.org> napisał(a):
> >
> > On Tue,  7 Jan 2020 10:29:21 +0100, Khouloud Touil wrote:
> > > As the at25 uses the NVMEM subsystem, and the property is now being
> > > handled, adding reference for it in the device tree binding document,
> > > which allows to specify the GPIO line to which the write-protect pin
> > > is connected.
> > >
> > > Signed-off-by: Khouloud Touil <ktouil@baylibre.com>
> > > ---
> > >  Documentation/devicetree/bindings/eeprom/at25.txt | 2 ++
> > >  1 file changed, 2 insertions(+)
> > >
> >
> > Reviewed-by: Rob Herring <robh@kernel.org>
> 
> Hi Greg,
> 
> AT25 patches usually go through the char-misc tree. In this case
> however, the change depends on the other patches in this series. Can
> you ack this and I'll take it through the AT24 tree exceptionally?

Acked-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>

Re: [PATCH v4 4/5] dt-bindings: at25: add reference for the wp-gpios property

[Bartosz Golaszewski]

wt., 14 sty 2020 o 15:42 Greg KH <gregkh@linuxfoundation.org> napisał(a):
>
> On Thu, Jan 09, 2020 at 10:47:56AM +0100, Bartosz Golaszewski wrote:
> > śr., 8 sty 2020 o 21:54 Rob Herring <robh@kernel.org> napisał(a):
> > >
> > > On Tue,  7 Jan 2020 10:29:21 +0100, Khouloud Touil wrote:
> > > > As the at25 uses the NVMEM subsystem, and the property is now being
> > > > handled, adding reference for it in the device tree binding document,
> > > > which allows to specify the GPIO line to which the write-protect pin
> > > > is connected.
> > > >
> > > > Signed-off-by: Khouloud Touil <ktouil@baylibre.com>
> > > > ---
> > > >  Documentation/devicetree/bindings/eeprom/at25.txt | 2 ++
> > > >  1 file changed, 2 insertions(+)
> > > >
> > >
> > > Reviewed-by: Rob Herring <robh@kernel.org>
> >
> > Hi Greg,
> >
> > AT25 patches usually go through the char-misc tree. In this case
> > however, the change depends on the other patches in this series. Can
> > you ack this and I'll take it through the AT24 tree exceptionally?
>
> Acked-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>

Patch applied with Greg's Ack.

Bart

Re: [PATCH v4 4/5] dt-bindings: at25: add reference for the wp-gpios property

[Bartosz Golaszewski]

wt., 14 sty 2020 o 15:42 Greg KH <gregkh-hQyY1W1yCW8ekmWlsbkhG0B+6BGkLq7r@public.gmane.org> napisał(a):
>
> On Thu, Jan 09, 2020 at 10:47:56AM +0100, Bartosz Golaszewski wrote:
> > śr., 8 sty 2020 o 21:54 Rob Herring <robh-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org> napisał(a):
> > >
> > > On Tue,  7 Jan 2020 10:29:21 +0100, Khouloud Touil wrote:
> > > > As the at25 uses the NVMEM subsystem, and the property is now being
> > > > handled, adding reference for it in the device tree binding document,
> > > > which allows to specify the GPIO line to which the write-protect pin
> > > > is connected.
> > > >
> > > > Signed-off-by: Khouloud Touil <ktouil-rdvid1DuHRBWk0Htik3J/w@public.gmane.org>
> > > > ---
> > > >  Documentation/devicetree/bindings/eeprom/at25.txt | 2 ++
> > > >  1 file changed, 2 insertions(+)
> > > >
> > >
> > > Reviewed-by: Rob Herring <robh-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org>
> >
> > Hi Greg,
> >
> > AT25 patches usually go through the char-misc tree. In this case
> > however, the change depends on the other patches in this series. Can
> > you ack this and I'll take it through the AT24 tree exceptionally?
>
> Acked-by: Greg Kroah-Hartman <gregkh-hQyY1W1yCW8ekmWlsbkhG0B+6BGkLq7r@public.gmane.org>

Patch applied with Greg's Ack.

Bart

Re: [PATCH v4 2/5] nvmem: add support for the write-protect pin

[Geert Uytterhoeven]

Hi Khouloud,

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()?

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

Re: [PATCH v4 2/5] nvmem: add support for the write-protect pin

[Geert Uytterhoeven]

Hi Khouloud,

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()?

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

Re: [PATCH v4 2/5] nvmem: add support for the write-protect pin

[Khouloud Touil]

Le jeu. 30 janv. 2020 à 09:06, Geert Uytterhoeven
<geert@linux-m68k.org> a écrit :
>
> Hi Khouloud,
>
> 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()?
>
> 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 -- 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

Re: [PATCH v4 2/5] nvmem: add support for the write-protect pin

[Khouloud Touil]

Le jeu. 30 janv. 2020 à 09:06, Geert Uytterhoeven
<geert@linux-m68k.org> a écrit :
>
> Hi Khouloud,
>
> 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()?
>
> 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 -- 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

Re: [PATCH v4 2/5] nvmem: add support for the write-protect pin

[Bartosz Golaszewski]

czw., 30 sty 2020 o 09:06 Geert Uytterhoeven <geert@linux-m68k.org> napisał(a):
>
> Hi Khouloud,
>
> 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
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

Re: [PATCH v4 2/5] nvmem: add support for the write-protect pin

[Bartosz Golaszewski]

czw., 30 sty 2020 o 09:06 Geert Uytterhoeven <geert-Td1EMuHUCqxL1ZNQvxDV9g@public.gmane.org> napisał(a):
>
> Hi Khouloud,
>
> 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
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

Re: [PATCH v4 2/5] nvmem: add support for the write-protect pin

[Geert Uytterhoeven]

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

Re: [PATCH v4 2/5] nvmem: add support for the write-protect pin

[Geert Uytterhoeven]

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