Skip to contentSkip to conversation

i2c: gpio: support write-only sda

PATCH Applied Heiner Kallweit 3 participants 9 messages Jan 18, 2023 – Jan 23, 2023

Subsystems (3)
Device tree bindings I2C subsystem host drivers Generic GPIO I2C driver
lore
Share & tools
lore

Heiner Kallweit

author
#0 lore raw

v5 cover i2c: gpio: support write-only sda

There are slave devices that understand I2C but have read-only SDA and SCL. Examples are FD650 7-segment LED controller and its derivatives. Typical board designs don't even have a pull-up for both pins. Therefore add properties for not using open-drain. For write-only SCL we have a property already, add one for write-only SDA.

v2:
- improve commit message for patch 1

v3:
- patch 2: check for adap->getsda in readbytes()
- patch 2: align warning message level for info on missing getscl/getsda
- patch 3: improve description of attribute sda_is_output_only

v4:
- patch 1: add no-pullup properties
- patch 2: handle SDA and SCL independently
- patch 2: properly handle case that SDA is NULL but SCL not
- patch 3: handle new no-pullup attributes
v5:
- patch 1: add checking mutually-exclusive attributes to schema

Heiner Kallweit (3):
dt-bindings: i2c-gpio: Add properties for dealing with write-only SDA/SCL w/o pullup
i2c: algo: bit: allow getsda to be NULL
i2c: gpio: support write-only sda/scl w/o pull-up

 .../devicetree/bindings/i2c/i2c-gpio.yaml     | 26 +++++++
 drivers/i2c/algos/i2c-algo-bit.c              | 77 +++++++++----------
 drivers/i2c/busses/i2c-gpio.c                 | 13 +++-
 include/linux/platform_data/i2c-gpio.h        |  9 +++
 4 files changed, 80 insertions(+), 45 deletions(-)
Signature · 1 line
2.39.0
author
#1 lore raw

PATCH v5 1/3 dt-bindings: i2c-gpio: Add properties for dealing with write-only SDA/SCL w/o pullup

There are slave devices that understand I2C but have read-only SDA and SCL. Examples are FD650 7-segment LED controller and its derivatives. Typical board designs don't even have a pull-up for both pins. Therefore add properties for not using open-drain. For write-only SCL we have a property already, add one for write-only SDA.

Signed-off-by Heiner Kallweit <[email protected]>

---
v4:
- add no-pullup properties
v5:
- add checking mutually-exclusive properties to schema

---
 .../devicetree/bindings/i2c/i2c-gpio.yaml     | 26 +++++++++++++++++++
 1 file changed, 26 insertions(+)
Documentation/devicetree/bindings/i2c/i2c-gpio.yaml +26 -0
index e0d76d5eb..afd4925c2 100644
@@ -33,6 +33,10 @@ properties:
       open drain.
     maxItems: 1
 
+  i2c-gpio,sda-output-only:
+    description: sda as output only
+    type: boolean
+
   i2c-gpio,scl-output-only:
     description: scl as output only
     type: boolean

@@ -63,6 +67,28 @@ properties:
       GPIO line used for SCL into open drain mode, and that something is not
       the GPIO chip. It is essentially an inconsistency flag.
 
+  i2c-gpio,sda-has-no-pullup:
+    type: boolean
+    description: sda is used in a non-compliant way and has no pull-up.
+      Therefore disable open-drain. This property is mutually-exclusive
+      with i2c-gpio,sda-open-drain.
+
+  i2c-gpio,scl-has-no-pullup:
+    type: boolean
+    description: scl is used in a non-compliant way and has no pull-up.
+      Therefore disable open-drain. This property is mutually-exclusive
+      with i2c-gpio,scl-open-drain.
+
+dependencies:
+  i2c-gpio,sda-has-no-pullup:
+    not:
+      required:
+        - i2c-gpio,sda-open-drain
+  i2c-gpio,scl-has-no-pullup:
+    not:
+      required:
+        - i2c-gpio,scl-open-drain
+
 required:
   - compatible
   - sda-gpios
Signature · 1 line
2.39.0
maintainer Reviewed-by on v5 1/3
#2 lore raw

Re: [PATCH v5 1/3] dt-bindings: i2c-gpio: Add properties for dealing with write-only SDA/SCL w/o pullup

Quoted 7 lines · Heiner Kallweit

There are slave devices that understand I2C but have read-only SDA and SCL. Examples are FD650 7-segment LED controller and its derivatives. Typical board designs don't even have a pull-up for both pins. Therefore add properties for not using open-drain. For write-only SCL we have a property already, add one for write-only SDA.

Signed-off-by Heiner Kallweit <[email protected]>
Quoted 9 lines

---
v4:
- add no-pullup properties
v5:
- add checking mutually-exclusive properties to schema

---
 .../devicetree/bindings/i2c/i2c-gpio.yaml     | 26 +++++++++++++++++++
 1 file changed, 26 insertions(+)
Reviewed-by Rob Herring <[email protected]>
Applied
#3 lore raw

Re: [PATCH v5 1/3] dt-bindings: i2c-gpio: Add properties for dealing with write-only SDA/SCL w/o pullup

In reply to Heiner Kallweit
Quoted 7 lines · Heiner Kallweit

There are slave devices that understand I2C but have read-only SDA and SCL. Examples are FD650 7-segment LED controller and its derivatives. Typical board designs don't even have a pull-up for both pins. Therefore add properties for not using open-drain. For write-only SCL we have a property already, add one for write-only SDA.

Signed-off-by Heiner Kallweit <[email protected]>

Applied to for-next, thanks!

author
#4 lore raw

PATCH v5 2/3 i2c: algo: bit: allow getsda to be NULL

In reply to Heiner Kallweit

This is in preparation of supporting write-only SDA in i2c-gpio.

Signed-off-by Heiner Kallweit <[email protected]>

---
v3:
- check for adap->getsda in readbytes()
- align warning message level for info on missing getscl/getsda v4:
- handle SDA and SCL independently
- properly handle case that SDA is NULL but SCL not

---
 drivers/i2c/algos/i2c-algo-bit.c | 77 +++++++++++++++-----------------
 1 file changed, 35 insertions(+), 42 deletions(-)
drivers/i2c/algos/i2c-algo-bit.c +35 -42
index fc90293af..eddf25b90 100644
@@ -184,8 +184,9 @@ static int i2c_outb(struct i2c_adapter *i2c_adap, unsigned char c)
 
 	/* read ack: SDA should be pulled down by slave, or it may
 	 * NAK (usually to report problems with the data we wrote).
+	 * Always report ACK if SDA is write-only.
 	 */
-	ack = !getsda(adap);    /* ack: sda is pulled low -> success */
+	ack = !adap->getsda || !getsda(adap);    /* ack: sda is pulled low -> success */
 	bit_dbg(2, &i2c_adap->dev, "i2c_outb: 0x%02x %s\n", (int)c,
 		ack ? "A" : "NA");
 

@@ -238,71 +239,55 @@ static int test_bus(struct i2c_adapter *i2c_adap)
 			return -ENODEV;
 	}
 
+	if (adap->getsda == NULL)
+		pr_info("%s: SDA is write-only, testing not possible\n", name);
 	if (adap->getscl == NULL)
-		pr_info("%s: Testing SDA only, SCL is not readable\n", name);
+		pr_info("%s: SCL is write-only, testing not possible\n", name);
 
-	sda = getsda(adap);
-	scl = (adap->getscl == NULL) ? 1 : getscl(adap);
+	sda = adap->getsda ? getsda(adap) : 1;
+	scl = adap->getscl ? getscl(adap) : 1;
 	if (!scl || !sda) {
-		printk(KERN_WARNING
-		       "%s: bus seems to be busy (scl=%d, sda=%d)\n",
-		       name, scl, sda);
+		pr_warn("%s: bus seems to be busy (scl=%d, sda=%d)\n", name, scl, sda);
 		goto bailout;
 	}
 
 	sdalo(adap);
-	sda = getsda(adap);
-	scl = (adap->getscl == NULL) ? 1 : getscl(adap);
-	if (sda) {
-		printk(KERN_WARNING "%s: SDA stuck high!\n", name);
+	if (adap->getsda && getsda(adap)) {
+		pr_warn("%s: SDA stuck high!\n", name);
 		goto bailout;
 	}
-	if (!scl) {
-		printk(KERN_WARNING
-		       "%s: SCL unexpected low while pulling SDA low!\n",
-		       name);
+	if (adap->getscl && !getscl(adap)) {
+		pr_warn("%s: SCL unexpected low while pulling SDA low!\n", name);
 		goto bailout;
 	}
 
 	sdahi(adap);
-	sda = getsda(adap);
-	scl = (adap->getscl == NULL) ? 1 : getscl(adap);
-	if (!sda) {
-		printk(KERN_WARNING "%s: SDA stuck low!\n", name);
+	if (adap->getsda && !getsda(adap)) {
+		pr_warn("%s: SDA stuck low!\n", name);
 		goto bailout;
 	}
-	if (!scl) {
-		printk(KERN_WARNING
-		       "%s: SCL unexpected low while pulling SDA high!\n",
-		       name);
+	if (adap->getscl && !getscl(adap)) {
+		pr_warn("%s: SCL unexpected low while pulling SDA high!\n", name);
 		goto bailout;
 	}
 
 	scllo(adap);
-	sda = getsda(adap);
-	scl = (adap->getscl == NULL) ? 0 : getscl(adap);
-	if (scl) {
-		printk(KERN_WARNING "%s: SCL stuck high!\n", name);
+	if (adap->getscl && getscl(adap)) {
+		pr_warn("%s: SCL stuck high!\n", name);
 		goto bailout;
 	}
-	if (!sda) {
-		printk(KERN_WARNING
-		       "%s: SDA unexpected low while pulling SCL low!\n",
-		       name);
+	if (adap->getsda && !getsda(adap)) {
+		pr_warn("%s: SDA unexpected low while pulling SCL low!\n", name);
 		goto bailout;
 	}
 
 	sclhi(adap);
-	sda = getsda(adap);
-	scl = (adap->getscl == NULL) ? 1 : getscl(adap);
-	if (!scl) {
-		printk(KERN_WARNING "%s: SCL stuck low!\n", name);
+	if (adap->getscl && !getscl(adap)) {
+		pr_warn("%s: SCL stuck low!\n", name);
 		goto bailout;
 	}
-	if (!sda) {
-		printk(KERN_WARNING
-		       "%s: SDA unexpected low while pulling SCL high!\n",
-		       name);
+	if (adap->getsda && !getsda(adap)) {
+		pr_warn("%s: SDA unexpected low while pulling SCL high!\n", name);
 		goto bailout;
 	}
 

@@ -420,6 +405,10 @@ static int readbytes(struct i2c_adapter *i2c_adap, struct i2c_msg *msg)
 	unsigned char *temp = msg->buf;
 	int count = msg->len;
 	const unsigned flags = msg->flags;
+	struct i2c_algo_bit_data *adap = i2c_adap->algo_data;
+
+	if (!adap->getsda)
+		return -EOPNOTSUPP;
 
 	while (count > 0) {
 		inval = i2c_inb(i2c_adap);

@@ -670,11 +659,15 @@ static int __i2c_bit_add_bus(struct i2c_adapter *adap,
 	if (ret < 0)
 		return ret;
 
-	/* Complain if SCL can't be read */
-	if (bit_adap->getscl == NULL) {
+	if (bit_adap->getsda == NULL)
+		dev_warn(&adap->dev, "Not I2C compliant: can't read SDA\n");
+
+	if (bit_adap->getscl == NULL)
 		dev_warn(&adap->dev, "Not I2C compliant: can't read SCL\n");
+
+	if (bit_adap->getsda == NULL || bit_adap->getscl == NULL)
 		dev_warn(&adap->dev, "Bus may be unreliable\n");
-	}
+
 	return 0;
 }
Signature · 1 line
2.39.0
Applied
#5 lore raw

Re: [PATCH v5 2/3] i2c: algo: bit: allow getsda to be NULL

Quoted 3 lines · Heiner Kallweit

This is in preparation of supporting write-only SDA in i2c-gpio.

Signed-off-by Heiner Kallweit <[email protected]>

Applied to for-next, thanks!

author
#6 lore raw

PATCH v5 3/3 i2c: gpio: support write-only sda/scl w/o pull-up

In reply to Heiner Kallweit

There are slave devices that understand I2C but have read-only SDA and SCL. Examples are FD650 7-segment LED controller and its derivatives. Typical board designs don't even have a pull-up for both pins. Handle the new attributes for write-only SDA and missing pull-up on SDA/SCL.

For either pin the open-drain and has-no-pullup properties are mutually-exclusive, what is documented in the DT property documentation. We don't add an extra warning here because the open-drain properties are marked deprecated anyway.

Signed-off-by Heiner Kallweit <[email protected]>

---
v3:
- improve description of attribute sda_is_output_only v4:
- handle new no-pullup attributes

---
 drivers/i2c/busses/i2c-gpio.c          | 13 ++++++++++---
 include/linux/platform_data/i2c-gpio.h |  9 +++++++++
 2 files changed, 19 insertions(+), 3 deletions(-)
drivers/i2c/busses/i2c-gpio.c +10 -3
index 0e4385a9b..85b3beb20 100644
@@ -316,6 +316,12 @@ static void of_i2c_gpio_get_props(struct device_node *np,
 		of_property_read_bool(np, "i2c-gpio,scl-open-drain");
 	pdata->scl_is_output_only =
 		of_property_read_bool(np, "i2c-gpio,scl-output-only");
+	pdata->sda_is_output_only =
+		of_property_read_bool(np, "i2c-gpio,sda-output-only");
+	pdata->sda_has_no_pullup =
+		of_property_read_bool(np, "i2c-gpio,sda-has-no-pullup");
+	pdata->scl_has_no_pullup =
+		of_property_read_bool(np, "i2c-gpio,scl-has-no-pullup");
 }
 
 static struct gpio_desc *i2c_gpio_get_desc(struct device *dev,

@@ -392,7 +398,7 @@ static int i2c_gpio_probe(struct platform_device *pdev)
 	 * handle them as we handle any other output. Else we enforce open
 	 * drain as this is required for an I2C bus.
 	 */
-	if (pdata->sda_is_open_drain)
+	if (pdata->sda_is_open_drain || pdata->sda_has_no_pullup)
 		gflags = GPIOD_OUT_HIGH;
 	else
 		gflags = GPIOD_OUT_HIGH_OPEN_DRAIN;

@@ -400,7 +406,7 @@ static int i2c_gpio_probe(struct platform_device *pdev)
 	if (IS_ERR(priv->sda))
 		return PTR_ERR(priv->sda);
 
-	if (pdata->scl_is_open_drain)
+	if (pdata->scl_is_open_drain || pdata->scl_has_no_pullup)
 		gflags = GPIOD_OUT_HIGH;
 	else
 		gflags = GPIOD_OUT_HIGH_OPEN_DRAIN;

@@ -418,7 +424,8 @@ static int i2c_gpio_probe(struct platform_device *pdev)
 
 	if (!pdata->scl_is_output_only)
 		bit_data->getscl = i2c_gpio_getscl;
-	bit_data->getsda = i2c_gpio_getsda;
+	if (!pdata->sda_is_output_only)
+		bit_data->getsda = i2c_gpio_getsda;
 
 	if (pdata->udelay)
 		bit_data->udelay = pdata->udelay;
include/linux/platform_data/i2c-gpio.h +9 -0
index a907774fd..545639bcc 100644
@@ -16,16 +16,25 @@
  *	isn't actively driven high when setting the output value high.
  *	gpio_get_value() must return the actual pin state even if the
  *	pin is configured as an output.
+ * @sda_is_output_only: SDA output drivers can't be turned off.
+ *	This is for clients that can only read SDA/SCL.
+ * @sda_has_no_pullup: SDA is used in a non-compliant way and has no pull-up.
+ *	Therefore disable open-drain.
  * @scl_is_open_drain: SCL is set up as open drain. Same requirements
  *	as for sda_is_open_drain apply.
  * @scl_is_output_only: SCL output drivers cannot be turned off.
+ * @scl_has_no_pullup: SCL is used in a non-compliant way and has no pull-up.
+ *	Therefore disable open-drain.
  */
 struct i2c_gpio_platform_data {
 	int		udelay;
 	int		timeout;
 	unsigned int	sda_is_open_drain:1;
+	unsigned int	sda_is_output_only:1;
+	unsigned int	sda_has_no_pullup:1;
 	unsigned int	scl_is_open_drain:1;
 	unsigned int	scl_is_output_only:1;
+	unsigned int	scl_has_no_pullup:1;
 };
 
 #endif /* _LINUX_I2C_GPIO_H */
Signature · 1 line
2.39.0
Applied
#7 lore raw

Re: [PATCH v5 3/3] i2c: gpio: support write-only sda/scl w/o pull-up

Quoted 6 lines · Heiner Kallweit

There are slave devices that understand I2C but have read-only SDA and SCL. Examples are FD650 7-segment LED controller and its derivatives. Typical board designs don't even have a pull-up for both pins. Handle the new attributes for write-only SDA and missing pull-up on SDA/SCL.

Quoted 6 lines

For either pin the open-drain and has-no-pullup properties are mutually-exclusive, what is documented in the DT property documentation. We don't add an extra warning here because the open-drain properties are marked deprecated anyway.

Signed-off-by Heiner Kallweit <[email protected]>

Applied to for-next, thanks!

drivers/i2c/busses/i2c-gpio.c static void of_i2c_gpio_get_props(struct device_node *np, · line 324

Quoted 6 lines
drivers/i2c/busses/i2c-gpio.c +6 -0
+	pdata->sda_is_output_only =
+		of_property_read_bool(np, "i2c-gpio,sda-output-only");
+	pdata->sda_has_no_pullup =
+		of_property_read_bool(np, "i2c-gpio,sda-has-no-pullup");
+	pdata->scl_has_no_pullup =
+		of_property_read_bool(np, "i2c-gpio,scl-has-no-pullup");

I converted these to device_property_read_bool() because of 7b6e9dc7e42d ("i2c: gpio: Add support on ACPI-based system") which is in my for-next.

#8 lore raw
In reply to Heiner Kallweit

Hi Heiner,

Quoted 5 lines · Heiner Kallweit

There are slave devices that understand I2C but have read-only SDA and SCL. Examples are FD650 7-segment LED controller and its derivatives. Typical board designs don't even have a pull-up for both pins. Therefore add properties for not using open-drain. For write-only SCL we have a property already, add one for write-only SDA.

The code looks good to me. I am just waiting for an ack on the bindings. Thanks a lot for keeping at it!

All the best,

Wolfram