eeprom: at24: write-protect pin support

PATCH In review Bartosz Golaszewski 3 participants 8 messages Dec 20, 2017

Subsystems (3)

Device tree bindings Char and misc drivers I2C subsystem host drivers

lore

Share & tools

lore

Bartosz Golaszewski

author

2017-12-20 08:26 UTC #0 lore raw

v2 cover eeprom: at24: write-protect pin support

AT24 EEPROMs have a write-protect pin, which - when pulled high - inhibits writes to the upper quadrant of memory (although it has been observed that on some chips it disables writing to the entire memory range).

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. On linux this means that we either need to hog the line all the time, or set the GPIO value before writing from outside of the at24 driver.

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

v1 -> v2:
- renamed the DT property to wp-gpios

Bartosz Golaszewski (2):
dt-bindings: at24: new optional property - wp-gpios
eeprom: at24: add support for the write-protect pin

 Documentation/devicetree/bindings/eeprom/at24.txt |  3 +++
 drivers/misc/eeprom/at24.c                        | 11 +++++++++++
 2 files changed, 14 insertions(+)

Signature · 1 line

2.15.1

Bartosz Golaszewski

author

2017-12-20 08:26 UTC #1 lore raw

PATCH v2 1/2 dt-bindings: at24: new optional property - wp-gpios

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

---
 Documentation/devicetree/bindings/eeprom/at24.txt | 3 +++
 1 file changed, 3 insertions(+)

Documentation/devicetree/bindings/eeprom/at24.txt +3 -0

index a0415b8471bb..cbc80e194ac6 100644
@@ -43,10 +43,13 @@ Optional properties:
 			eeprom does not automatically roll over reads to the next
 			slave address. Please consult the manual of your device.
 
+  - wp-gpios: GPIO to which the write-protect pin of the chip is connected.
+
 Example:
 
 eeprom@52 {
 	compatible = "atmel,24c32";
 	reg = <0x52>;
 	pagesize = <32>;
+	wp-gpios = <&gpio1 3 0>;
 };

Signature · 1 line

2.15.1

Rob Herring

maintainer Reviewed-by on v2 1/2

2017-12-20 21:48 UTC #2 lore raw

Re: [PATCH v2 1/2] dt-bindings: at24: new optional property - wp-gpios

Quoted 11 lines · Bartosz Golaszewski

AT24 EEPROMs have a write-protect pin, which - when pulled high - inhibits writes to the upper quadrant of memory (although it has been observed that on some chips it disables writing to the entire memory range).
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. On linux this means that we either need to hog the line all the time, or set the GPIO value before writing from outside of the at24 driver.

Quoted 8 lines

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 Bartosz Golaszewski <brgl-ARrdPY/[email protected]>
---
 Documentation/devicetree/bindings/eeprom/at24.txt | 3 +++
 1 file changed, 3 insertions(+)

Reviewed-by Rob Herring <[email protected]>

--
To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to [email protected] More majordomo info at http://vger.kernel.org/majordomo-info.html

Bartosz Golaszewski

author

2017-12-20 08:26 UTC #3 lore raw

PATCH v2 2/2 eeprom: at24: add support for the write-protect pin

In reply to Bartosz Golaszewski

Make the driver check if the write-protect GPIO was defined in the device tree and pull it low whenever writing to the EEPROM.

Signed-off-by Bartosz Golaszewski <[email protected]>

---
 drivers/misc/eeprom/at24.c | 11 +++++++++++
 1 file changed, 11 insertions(+)

drivers/misc/eeprom/at24.c +11 -0

index b44a3d2b2b20..4456e192681c 100644
@@ -27,6 +27,7 @@
 #include <linux/regmap.h>
 #include <linux/platform_data/at24.h>
 #include <linux/pm_runtime.h>
+#include <linux/gpio/consumer.h>
 
 /*
  * I2C EEPROMs from most vendors are inexpensive and mostly interchangeable.

@@ -77,6 +78,8 @@ struct at24_data {
 	struct nvmem_config nvmem_config;
 	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.

@@ -442,12 +445,14 @@ 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) {
 		int status;
 
 		status = at24_regmap_write(at24, buf, off, count);
 		if (status < 0) {
+			gpiod_set_value_cansleep(at24->wp_gpio, 1);
 			mutex_unlock(&at24->lock);
 			pm_runtime_put(dev);
 			return status;

@@ -457,6 +462,7 @@ static int at24_write(void *priv, unsigned int off, void *val, size_t count)
 		count -= status;
 	}
 
+	gpiod_set_value_cansleep(at24->wp_gpio, 1);
 	mutex_unlock(&at24->lock);
 
 	pm_runtime_put(dev);

@@ -604,6 +610,11 @@ static int at24_probe(struct i2c_client *client, const struct i2c_device_id *id)
 	at24->num_addresses = num_addresses;
 	at24->offset_adj = at24_get_offset_adj(chip.flags, chip.byte_len);
 
+	at24->wp_gpio = devm_gpiod_get_optional(&client->dev,
+						"wp", GPIOD_OUT_HIGH);
+	if (IS_ERR(at24->wp_gpio))
+		return PTR_ERR(at24->wp_gpio);
+
 	at24->client[0].client = client;
 	at24->client[0].regmap = devm_regmap_init_i2c(client, &regmap_config);
 	if (IS_ERR(at24->client[0].regmap))

Signature · 1 line

2.15.1

Andy Shevchenko

Reviewed-by on the v2 series

2017-12-20 10:21 UTC #4 lore raw

In reply to Bartosz Golaszewski

Quoted 11 lines · Bartosz Golaszewski

AT24 EEPROMs have a write-protect pin, which - when pulled high - inhibits writes to the upper quadrant of memory (although it has been observed that on some chips it disables writing to the entire memory range).
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. On linux this means that we either need to hog the line all the time, or set the GPIO value before writing from outside of the at24 driver.

Quoted 5 lines

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

Reviewed-by Andy Shevchenko <[email protected]>

A one totally minor nit: if it possible now to have one line where devm_gpiod_get_optional() is called?
You may ignore this nit anyway.

Quoted 14 lines

v1 -> v2:
- renamed the DT property to wp-gpios
Bartosz Golaszewski (2):
dt-bindings: at24: new optional property - wp-gpios
eeprom: at24: add support for the write-protect pin
 Documentation/devicetree/bindings/eeprom/at24.txt |  3 +++
 drivers/misc/eeprom/at24.c                        | 11 +++++++++++
 2 files changed, 14 insertions(+)
--
2.15.1

Signature · 6 lines

With Best Regards,
Andy Shevchenko
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to [email protected]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Bartosz Golaszewski

author

2017-12-20 12:41 UTC #5 lore raw

2017-12-20 11:21 GMT+01:00 Andy Shevchenko <[email protected]>:

Quoted 18 lines · Bartosz Golaszewski

Quoted 11 lines
AT24 EEPROMs have a write-protect pin, which - when pulled high - inhibits writes to the upper quadrant of memory (although it has been observed that on some chips it disables writing to the entire memory range).
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. On linux this means that we either need to hog the line all the time, or set the GPIO value before writing from outside of the at24 driver.
Quoted 5 lines
This series adds support for the write-protect pin split into two parts. The first patch extends the relevant binding document, while the second modifies the at24 code to pull the write-protect GPIO low (if present) during write operations.

Quoted 6 lines

Reviewed-by Andy Shevchenko <[email protected]>
A one totally minor nit: if it possible now to have one line where devm_gpiod_get_optional() is called?
You may ignore this nit anyway.

Hi Andy,

I'm afraid I don't understand what you mean here. I do use devm_gpiod_get_optional() in patch 2/2.

Thanks,
Bartosz
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to [email protected] More majordomo info at http://vger.kernel.org/majordomo-info.html

Andy Shevchenko

2017-12-20 15:00 UTC #6 lore raw

Quoted 26 lines · Bartosz Golaszewski

2017-12-20 11:21 GMT+01:00 Andy Shevchenko <[email protected]>:
Quoted 18 lines · Bartosz Golaszewski
Quoted 11 lines
AT24 EEPROMs have a write-protect pin, which - when pulled high - inhibits writes to the upper quadrant of memory (although it has been observed that on some chips it disables writing to the entire memory range).
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. On linux this means that we either need to hog the line all the time, or set the GPIO value before writing from outside of the at24 driver.
Quoted 5 lines
This series adds support for the write-protect pin split into two parts. The first patch extends the relevant binding document, while the second modifies the at24 code to pull the write-protect GPIO low (if present) during write operations.
Quoted 6 lines
Reviewed-by Andy Shevchenko <[email protected]>
A one totally minor nit: if it possible now to have one line where devm_gpiod_get_optional() is called?
You may ignore this nit anyway.

Quoted 4 lines

Hi Andy,
I'm afraid I don't understand what you mean here. I do use devm_gpiod_get_optional() in patch 2/2.

I meant to do something like
...->wp_gpio = devm_gpiod_get_optional(...);
if (IS_ERR(...))
return ...;

So, note that the first is occupied only one line.

Signature · 2 lines

With Best Regards,
Andy Shevchenko

Bartosz Golaszewski

author

2017-12-20 15:05 UTC #7 lore raw

2017-12-20 16:00 GMT+01:00 Andy Shevchenko <[email protected]>:

Quoted 37 lines · Bartosz Golaszewski

Quoted 26 lines
2017-12-20 11:21 GMT+01:00 Andy Shevchenko <[email protected]>:
Quoted 18 lines · Bartosz Golaszewski
Quoted 11 lines
AT24 EEPROMs have a write-protect pin, which - when pulled high - inhibits writes to the upper quadrant of memory (although it has been observed that on some chips it disables writing to the entire memory range).
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. On linux this means that we either need to hog the line all the time, or set the GPIO value before writing from outside of the at24 driver.
Quoted 5 lines
This series adds support for the write-protect pin split into two parts. The first patch extends the relevant binding document, while the second modifies the at24 code to pull the write-protect GPIO low (if present) during write operations.
Quoted 6 lines
Reviewed-by Andy Shevchenko <[email protected]>
A one totally minor nit: if it possible now to have one line where devm_gpiod_get_optional() is called?
You may ignore this nit anyway.
Quoted 4 lines
Hi Andy,
I'm afraid I don't understand what you mean here. I do use devm_gpiod_get_optional() in patch 2/2.
I meant to do something like
...->wp_gpio = devm_gpiod_get_optional(...);
if (IS_ERR(...))
return ...;

Quoted 5 lines

So, note that the first is occupied only one line.
--
With Best Regards,
Andy Shevchenko

Oh, now I get it.

It doesn't fit into 80 characters if we use &client->dev as the first argument, but I see that client->dev is used extensively in probe() so we could probably use a separate struct device *dev helper variable for that. I'll note it for future refactoring that will happen soon. I prefer that this patch stays as it is though.

Thanks,
Bartosz