[PATCH v3 0/3] i2c: gpio: support write-only sda

[PATCH v3 0/3] i2c: gpio: support write-only sda

[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. This patch makes i2c-gpio usable with
such devices, based on new DT property i2c-gpio,sda-output-only.

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

Heiner Kallweit (3):
  dt-bindings: i2c-gpio: Add property i2c-gpio,sda-output-only
  i2c: algo: bit: allow getsda to be NULL
  i2c: gpio: support write-only sda

 .../devicetree/bindings/i2c/i2c-gpio.yaml        |  4 ++++
 drivers/i2c/algos/i2c-algo-bit.c                 | 16 ++++++++++++++--
 drivers/i2c/busses/i2c-gpio.c                    | 14 +++++++++++---
 include/linux/platform_data/i2c-gpio.h           |  3 +++
 4 files changed, 32 insertions(+), 5 deletions(-)

-- 
2.39.0

[PATCH v3 1/3] dt-bindings: i2c-gpio: Add property i2c-gpio,sda-output-only

[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 don't enforce open-drain
if SDA and SCL both are unidirectional. This patch makes
i2c-gpio usable with such devices, based on new DT property
i2c-gpio,sda-output-only.

Reviewed-by: Rob Herring <robh@kernel.org>
Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com>
---
 Documentation/devicetree/bindings/i2c/i2c-gpio.yaml | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/Documentation/devicetree/bindings/i2c/i2c-gpio.yaml b/Documentation/devicetree/bindings/i2c/i2c-gpio.yaml
index e0d76d5eb..fd84a60d9 100644
--- a/Documentation/devicetree/bindings/i2c/i2c-gpio.yaml
+++ b/Documentation/devicetree/bindings/i2c/i2c-gpio.yaml
@@ -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
-- 
2.39.0

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

[Heiner Kallweit]

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

Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com>
---
v3:
- check for adap->getsda in readbytes()
- align warning message level for info on missing getscl/getsda
---
 drivers/i2c/algos/i2c-algo-bit.c | 16 ++++++++++++++--
 1 file changed, 14 insertions(+), 2 deletions(-)

diff --git a/drivers/i2c/algos/i2c-algo-bit.c b/drivers/i2c/algos/i2c-algo-bit.c
index fc90293af..a1b822723 100644
--- a/drivers/i2c/algos/i2c-algo-bit.c
+++ b/drivers/i2c/algos/i2c-algo-bit.c
@@ -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");
 
@@ -232,6 +233,10 @@ static int test_bus(struct i2c_adapter *i2c_adap)
 	const char *name = i2c_adap->name;
 	int scl, sda, ret;
 
+	/* Testing not possible if both pins are write-only. */
+	if (adap->getscl == NULL && adap->getsda == NULL)
+		return 0;
+
 	if (adap->pre_xfer) {
 		ret = adap->pre_xfer(i2c_adap);
 		if (ret < 0)
@@ -420,6 +425,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,8 +679,11 @@ 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->getsda == NULL)
+		dev_warn(&adap->dev, "Not I2C compliant: can't read SDA\n");
+
 	if (bit_adap->getscl == NULL) {
+		/* Complain if SCL can't be read */
 		dev_warn(&adap->dev, "Not I2C compliant: can't read SCL\n");
 		dev_warn(&adap->dev, "Bus may be unreliable\n");
 	}
-- 
2.39.0

[PATCH v3 3/3] i2c: gpio: support write-only sda

[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 don't enforce open-drain
if SDA and SCL both are unidirectional. This patch makes
i2c-gpio usable with such devices, based on new DT property
i2c-gpio,sda-output-only.

Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com>
---
v3:
- improve description of attribute sda_is_output_only
---
 drivers/i2c/busses/i2c-gpio.c          | 14 +++++++++++---
 include/linux/platform_data/i2c-gpio.h |  3 +++
 2 files changed, 14 insertions(+), 3 deletions(-)

diff --git a/drivers/i2c/busses/i2c-gpio.c b/drivers/i2c/busses/i2c-gpio.c
index 0e4385a9b..ea108d7e4 100644
--- a/drivers/i2c/busses/i2c-gpio.c
+++ b/drivers/i2c/busses/i2c-gpio.c
@@ -316,6 +316,8 @@ 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");
 }
 
 static struct gpio_desc *i2c_gpio_get_desc(struct device *dev,
@@ -363,6 +365,7 @@ static int i2c_gpio_probe(struct platform_device *pdev)
 	struct device *dev = &pdev->dev;
 	struct device_node *np = dev->of_node;
 	enum gpiod_flags gflags;
+	bool sda_scl_output_only;
 	int ret;
 
 	priv = devm_kzalloc(dev, sizeof(*priv), GFP_KERNEL);
@@ -391,8 +394,12 @@ static int i2c_gpio_probe(struct platform_device *pdev)
 	 * marking these lines to be handled as open drain, and we should just
 	 * handle them as we handle any other output. Else we enforce open
 	 * drain as this is required for an I2C bus.
+	 * If SCL/SDA both are write-only, then this indicates I2C-like slaves
+	 * with read-only SCL/SDA. Such slaves don't need open-drain, and partially
+	 * don't even work with open-drain.
 	 */
-	if (pdata->sda_is_open_drain)
+	sda_scl_output_only = pdata->sda_is_output_only && pdata->scl_is_output_only;
+	if (pdata->sda_is_open_drain || sda_scl_output_only)
 		gflags = GPIOD_OUT_HIGH;
 	else
 		gflags = GPIOD_OUT_HIGH_OPEN_DRAIN;
@@ -400,7 +407,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 || sda_scl_output_only)
 		gflags = GPIOD_OUT_HIGH;
 	else
 		gflags = GPIOD_OUT_HIGH_OPEN_DRAIN;
@@ -418,7 +425,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;
diff --git a/include/linux/platform_data/i2c-gpio.h b/include/linux/platform_data/i2c-gpio.h
index a907774fd..e9536c078 100644
--- a/include/linux/platform_data/i2c-gpio.h
+++ b/include/linux/platform_data/i2c-gpio.h
@@ -16,6 +16,8 @@
  *	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.
  * @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.
@@ -24,6 +26,7 @@ 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	scl_is_open_drain:1;
 	unsigned int	scl_is_output_only:1;
 };
-- 
2.39.0

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

[Peter Rosin]

Hi!

2023-01-15 at 11:15, Heiner Kallweit wrote:
> This is in preparation of supporting write-only SDA in i2c-gpio.
> 
> Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com>
> ---
> v3:
> - check for adap->getsda in readbytes()
> - align warning message level for info on missing getscl/getsda
> ---
>  drivers/i2c/algos/i2c-algo-bit.c | 16 ++++++++++++++--
>  1 file changed, 14 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/i2c/algos/i2c-algo-bit.c b/drivers/i2c/algos/i2c-algo-bit.c
> index fc90293af..a1b822723 100644
> --- a/drivers/i2c/algos/i2c-algo-bit.c
> +++ b/drivers/i2c/algos/i2c-algo-bit.c
> @@ -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");
>  
> @@ -232,6 +233,10 @@ static int test_bus(struct i2c_adapter *i2c_adap)
>  	const char *name = i2c_adap->name;
>  	int scl, sda, ret;
>  
> +	/* Testing not possible if both pins are write-only. */
> +	if (adap->getscl == NULL && adap->getsda == NULL)
> +		return 0;

Would it not be nice to keep output-only SCL and SDA independent? With
your proposed check before doing the tests, all tests will crash when
adap->getsda is NULL, unless adap->getscl also happens to be NULL.

So, I would like to remove the above check and instead see some changes
along the lines of

-	sda = getsda(adap);
+	sda = (adap->getsda == NULL) ? 1 : getsda(adap);

(BTW, I dislike this way of writing that, and would have written
	sda = adap->getsda ? getsda(adap) : 1;
 had it not been for the preexisting code for the SCL case. Oh well.)

> +
>  	if (adap->pre_xfer) {
>  		ret = adap->pre_xfer(i2c_adap);
>  		if (ret < 0)
> @@ -420,6 +425,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,8 +679,11 @@ 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->getsda == NULL)
> +		dev_warn(&adap->dev, "Not I2C compliant: can't read SDA\n");
> +
>  	if (bit_adap->getscl == NULL) {
> +		/* Complain if SCL can't be read */
>  		dev_warn(&adap->dev, "Not I2C compliant: can't read SCL\n");
>  		dev_warn(&adap->dev, "Bus may be unreliable\n");
>  	}

And here you'd need something like this to make them independently select-able:

	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->getscl == NULL || bit_adap->getsda == NULL)
		dev_warn(&adap->dev, "Bus may be unreliable\n");

Anyway, as is, this patch is broken if getsda is NULL while getscl is not.
There is no documentation describing that limitation. It looks easier to
fix the limitation than to muddy the waters by having ifs and buts.

Cheers,
Peter

Re: [PATCH v3 3/3] i2c: gpio: support write-only sda

[Peter Rosin]

Hi!

2023-01-15 at 11:18, Heiner Kallweit wrote:
> 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 don't enforce open-drain
> if SDA and SCL both are unidirectional. This patch makes
> i2c-gpio usable with such devices, based on new DT property
> i2c-gpio,sda-output-only.
> 
> Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com>
> ---
> v3:
> - improve description of attribute sda_is_output_only
> ---
>  drivers/i2c/busses/i2c-gpio.c          | 14 +++++++++++---
>  include/linux/platform_data/i2c-gpio.h |  3 +++
>  2 files changed, 14 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/i2c/busses/i2c-gpio.c b/drivers/i2c/busses/i2c-gpio.c
> index 0e4385a9b..ea108d7e4 100644
> --- a/drivers/i2c/busses/i2c-gpio.c
> +++ b/drivers/i2c/busses/i2c-gpio.c
> @@ -316,6 +316,8 @@ 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");
>  }
>  
>  static struct gpio_desc *i2c_gpio_get_desc(struct device *dev,
> @@ -363,6 +365,7 @@ static int i2c_gpio_probe(struct platform_device *pdev)
>  	struct device *dev = &pdev->dev;
>  	struct device_node *np = dev->of_node;
>  	enum gpiod_flags gflags;
> +	bool sda_scl_output_only;
>  	int ret;
>  
>  	priv = devm_kzalloc(dev, sizeof(*priv), GFP_KERNEL);
> @@ -391,8 +394,12 @@ static int i2c_gpio_probe(struct platform_device *pdev)
>  	 * marking these lines to be handled as open drain, and we should just
>  	 * handle them as we handle any other output. Else we enforce open
>  	 * drain as this is required for an I2C bus.
> +	 * If SCL/SDA both are write-only, then this indicates I2C-like slaves
> +	 * with read-only SCL/SDA. Such slaves don't need open-drain, and partially
> +	 * don't even work with open-drain.
>  	 */
> -	if (pdata->sda_is_open_drain)
> +	sda_scl_output_only = pdata->sda_is_output_only && pdata->scl_is_output_only;
> +	if (pdata->sda_is_open_drain || sda_scl_output_only)

I have not looked closely, but I see no strong reason to tie the SCL
output-only property to the flags of SDA?

>  		gflags = GPIOD_OUT_HIGH;
>  	else
>  		gflags = GPIOD_OUT_HIGH_OPEN_DRAIN;
> @@ -400,7 +407,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 || sda_scl_output_only)

Same here, why tie the SDA output-only property to the flags of SCL?

Cheers,
Peter

>  		gflags = GPIOD_OUT_HIGH;
>  	else
>  		gflags = GPIOD_OUT_HIGH_OPEN_DRAIN;
> @@ -418,7 +425,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;
> diff --git a/include/linux/platform_data/i2c-gpio.h b/include/linux/platform_data/i2c-gpio.h
> index a907774fd..e9536c078 100644
> --- a/include/linux/platform_data/i2c-gpio.h
> +++ b/include/linux/platform_data/i2c-gpio.h
> @@ -16,6 +16,8 @@
>   *	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.
>   * @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.
> @@ -24,6 +26,7 @@ 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	scl_is_open_drain:1;
>  	unsigned int	scl_is_output_only:1;
>  };

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

[Heiner Kallweit]

On 16.01.2023 08:17, Peter Rosin wrote:
> Hi!
> 
> 2023-01-15 at 11:15, Heiner Kallweit wrote:
>> This is in preparation of supporting write-only SDA in i2c-gpio.
>>
>> Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com>
>> ---
>> v3:
>> - check for adap->getsda in readbytes()
>> - align warning message level for info on missing getscl/getsda
>> ---
>>  drivers/i2c/algos/i2c-algo-bit.c | 16 ++++++++++++++--
>>  1 file changed, 14 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/i2c/algos/i2c-algo-bit.c b/drivers/i2c/algos/i2c-algo-bit.c
>> index fc90293af..a1b822723 100644
>> --- a/drivers/i2c/algos/i2c-algo-bit.c
>> +++ b/drivers/i2c/algos/i2c-algo-bit.c
>> @@ -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");
>>  
>> @@ -232,6 +233,10 @@ static int test_bus(struct i2c_adapter *i2c_adap)
>>  	const char *name = i2c_adap->name;
>>  	int scl, sda, ret;
>>  
>> +	/* Testing not possible if both pins are write-only. */
>> +	if (adap->getscl == NULL && adap->getsda == NULL)
>> +		return 0;
> 
> Would it not be nice to keep output-only SCL and SDA independent? With
> your proposed check before doing the tests, all tests will crash when
> adap->getsda is NULL, unless adap->getscl also happens to be NULL.
> 
> So, I would like to remove the above check and instead see some changes
> along the lines of
> 
> -	sda = getsda(adap);
> +	sda = (adap->getsda == NULL) ? 1 : getsda(adap);
> 
> (BTW, I dislike this way of writing that, and would have written
> 	sda = adap->getsda ? getsda(adap) : 1;
>  had it not been for the preexisting code for the SCL case. Oh well.)
> 
Right, I'll change it accordingly in v2.

>> +
>>  	if (adap->pre_xfer) {
>>  		ret = adap->pre_xfer(i2c_adap);
>>  		if (ret < 0)
>> @@ -420,6 +425,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,8 +679,11 @@ 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->getsda == NULL)
>> +		dev_warn(&adap->dev, "Not I2C compliant: can't read SDA\n");
>> +
>>  	if (bit_adap->getscl == NULL) {
>> +		/* Complain if SCL can't be read */
>>  		dev_warn(&adap->dev, "Not I2C compliant: can't read SCL\n");
>>  		dev_warn(&adap->dev, "Bus may be unreliable\n");
>>  	}
> 
> And here you'd need something like this to make them independently select-able:
> 
> 	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->getscl == NULL || bit_adap->getsda == NULL)
> 		dev_warn(&adap->dev, "Bus may be unreliable\n");
> 
Will be changed accordingly in v2.

> Anyway, as is, this patch is broken if getsda is NULL while getscl is not.
> There is no documentation describing that limitation. It looks easier to
> fix the limitation than to muddy the waters by having ifs and buts.
> 
> Cheers,
> Peter

Re: [PATCH v3 3/3] i2c: gpio: support write-only sda

[Heiner Kallweit]

On 16.01.2023 08:18, Peter Rosin wrote:
> Hi!
> 
> 2023-01-15 at 11:18, Heiner Kallweit wrote:
>> 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 don't enforce open-drain
>> if SDA and SCL both are unidirectional. This patch makes
>> i2c-gpio usable with such devices, based on new DT property
>> i2c-gpio,sda-output-only.
>>
>> Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com>
>> ---
>> v3:
>> - improve description of attribute sda_is_output_only
>> ---
>>  drivers/i2c/busses/i2c-gpio.c          | 14 +++++++++++---
>>  include/linux/platform_data/i2c-gpio.h |  3 +++
>>  2 files changed, 14 insertions(+), 3 deletions(-)
>>
>> diff --git a/drivers/i2c/busses/i2c-gpio.c b/drivers/i2c/busses/i2c-gpio.c
>> index 0e4385a9b..ea108d7e4 100644
>> --- a/drivers/i2c/busses/i2c-gpio.c
>> +++ b/drivers/i2c/busses/i2c-gpio.c
>> @@ -316,6 +316,8 @@ 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");
>>  }
>>  
>>  static struct gpio_desc *i2c_gpio_get_desc(struct device *dev,
>> @@ -363,6 +365,7 @@ static int i2c_gpio_probe(struct platform_device *pdev)
>>  	struct device *dev = &pdev->dev;
>>  	struct device_node *np = dev->of_node;
>>  	enum gpiod_flags gflags;
>> +	bool sda_scl_output_only;
>>  	int ret;
>>  
>>  	priv = devm_kzalloc(dev, sizeof(*priv), GFP_KERNEL);
>> @@ -391,8 +394,12 @@ static int i2c_gpio_probe(struct platform_device *pdev)
>>  	 * marking these lines to be handled as open drain, and we should just
>>  	 * handle them as we handle any other output. Else we enforce open
>>  	 * drain as this is required for an I2C bus.
>> +	 * If SCL/SDA both are write-only, then this indicates I2C-like slaves
>> +	 * with read-only SCL/SDA. Such slaves don't need open-drain, and partially
>> +	 * don't even work with open-drain.
>>  	 */
>> -	if (pdata->sda_is_open_drain)
>> +	sda_scl_output_only = pdata->sda_is_output_only && pdata->scl_is_output_only;
>> +	if (pdata->sda_is_open_drain || sda_scl_output_only)
> 
> I have not looked closely, but I see no strong reason to tie the SCL
> output-only property to the flags of SDA?
> 

Maybe I best start with explaining my use case. It's about a slave device that has
read-only SDA/SCL and is connected to SDA/SCL GPIO's w/o pull-up resistor.
If the GPIO's are configured as open-drain the slave device doesn't work.
Maybe the slave device SDA/SCL pins have an internal pull-low, I don't know.
So I have to ensure that GPIOD_OUT_HIGH is used for both GPIO's.

I have some doubt that it's safe to use the following. The master may pull SCL
high whilst a clock stretching capable slave pulls SCL low.

if (pdata->scl_is_open_drain || pdata->scl_is_output_only)
	gflags = GPIOD_OUT_HIGH;
else
	gflags = GPIOD_OUT_HIGH_OPEN_DRAIN;

I could use the i2c-gpio,scl-open-drain property that does what I need,
but just by chance and it would be misleading to (mis-)use it.

Maybe add a property i2c-gpio,scl-no-pullup that de-facto is an alias for
i2c-gpio,scl-open-drain?

My patch was based on the assumption that if both, SDA and SCL, are write-only,
then only slaves with read-only SDA/SCL are attached.
I can't imagine a meaningful setup where this assumption isn't true.

>>  		gflags = GPIOD_OUT_HIGH;
>>  	else
>>  		gflags = GPIOD_OUT_HIGH_OPEN_DRAIN;
>> @@ -400,7 +407,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 || sda_scl_output_only)
> 
> Same here, why tie the SDA output-only property to the flags of SCL?
> 
> Cheers,
> Peter
> 
>>  		gflags = GPIOD_OUT_HIGH;
>>  	else
>>  		gflags = GPIOD_OUT_HIGH_OPEN_DRAIN;
>> @@ -418,7 +425,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;
>> diff --git a/include/linux/platform_data/i2c-gpio.h b/include/linux/platform_data/i2c-gpio.h
>> index a907774fd..e9536c078 100644
>> --- a/include/linux/platform_data/i2c-gpio.h
>> +++ b/include/linux/platform_data/i2c-gpio.h
>> @@ -16,6 +16,8 @@
>>   *	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.
>>   * @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.
>> @@ -24,6 +26,7 @@ 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	scl_is_open_drain:1;
>>  	unsigned int	scl_is_output_only:1;
>>  };

Re: [PATCH v3 3/3] i2c: gpio: support write-only sda

[Peter Rosin]

Hi!

2023-01-16 at 22:59, Heiner Kallweit wrote:
> On 16.01.2023 08:18, Peter Rosin wrote:
>> Hi!
>>
>> 2023-01-15 at 11:18, Heiner Kallweit wrote:
>>> 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 don't enforce open-drain
>>> if SDA and SCL both are unidirectional. This patch makes
>>> i2c-gpio usable with such devices, based on new DT property
>>> i2c-gpio,sda-output-only.
>>>
>>> Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com>
>>> ---
>>> v3:
>>> - improve description of attribute sda_is_output_only
>>> ---
>>>  drivers/i2c/busses/i2c-gpio.c          | 14 +++++++++++---
>>>  include/linux/platform_data/i2c-gpio.h |  3 +++
>>>  2 files changed, 14 insertions(+), 3 deletions(-)
>>>
>>> diff --git a/drivers/i2c/busses/i2c-gpio.c b/drivers/i2c/busses/i2c-gpio.c
>>> index 0e4385a9b..ea108d7e4 100644
>>> --- a/drivers/i2c/busses/i2c-gpio.c
>>> +++ b/drivers/i2c/busses/i2c-gpio.c
>>> @@ -316,6 +316,8 @@ 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");
>>>  }
>>>  
>>>  static struct gpio_desc *i2c_gpio_get_desc(struct device *dev,
>>> @@ -363,6 +365,7 @@ static int i2c_gpio_probe(struct platform_device *pdev)
>>>  	struct device *dev = &pdev->dev;
>>>  	struct device_node *np = dev->of_node;
>>>  	enum gpiod_flags gflags;
>>> +	bool sda_scl_output_only;
>>>  	int ret;
>>>  
>>>  	priv = devm_kzalloc(dev, sizeof(*priv), GFP_KERNEL);
>>> @@ -391,8 +394,12 @@ static int i2c_gpio_probe(struct platform_device *pdev)
>>>  	 * marking these lines to be handled as open drain, and we should just
>>>  	 * handle them as we handle any other output. Else we enforce open
>>>  	 * drain as this is required for an I2C bus.
>>> +	 * If SCL/SDA both are write-only, then this indicates I2C-like slaves
>>> +	 * with read-only SCL/SDA. Such slaves don't need open-drain, and partially
>>> +	 * don't even work with open-drain.
>>>  	 */
>>> -	if (pdata->sda_is_open_drain)
>>> +	sda_scl_output_only = pdata->sda_is_output_only && pdata->scl_is_output_only;
>>> +	if (pdata->sda_is_open_drain || sda_scl_output_only)
>>
>> I have not looked closely, but I see no strong reason to tie the SCL
>> output-only property to the flags of SDA?
>>
> 
> Maybe I best start with explaining my use case. It's about a slave device that has
> read-only SDA/SCL and is connected to SDA/SCL GPIO's w/o pull-up resistor.
> If the GPIO's are configured as open-drain the slave device doesn't work.
> Maybe the slave device SDA/SCL pins have an internal pull-low, I don't know.
> So I have to ensure that GPIOD_OUT_HIGH is used for both GPIO's.
> 
> I have some doubt that it's safe to use the following. The master may pull SCL
> high whilst a clock stretching capable slave pulls SCL low.
> 
> if (pdata->scl_is_open_drain || pdata->scl_is_output_only)
> 	gflags = GPIOD_OUT_HIGH;
> else
> 	gflags = GPIOD_OUT_HIGH_OPEN_DRAIN;
> 
> I could use the i2c-gpio,scl-open-drain property that does what I need,
> but just by chance and it would be misleading to (mis-)use it.
> 
> Maybe add a property i2c-gpio,scl-no-pullup that de-facto is an alias for
> i2c-gpio,scl-open-drain?
> 
> My patch was based on the assumption that if both, SDA and SCL, are write-only,
> then only slaves with read-only SDA/SCL are attached.
> I can't imagine a meaningful setup where this assumption isn't true.

Oh. I misread the patch, sorry for the confusion. But now that I see that
you avoid open-drain and use push-pull simply because *both* SCL and SDA
are output-only, I have to object to the mixing of these issues. It may
fit your case, but imagine a case where pull-ups are used to provide
a lower voltage level on the I2C bus than what is normal for the master.
That could be useful, even if something is also making SDA/SCL write-
only. Perhaps exactly that low bus level make reading buggy? That case
would perhaps not be possible if the master then pushed SCL and SDA
higher than intended by the pull-ups?

So. Yes, I do agree that i2c-gpio,scl-open-drain and i2c-gpio,sda-open-drain
look very wrong when they are used to get rid of open-drain and am personally
for an alias. i2c-gpio,scl-no-pullup is perhaps not the best name as it
does not describe the hardware, but instead the hardware that isn't there?

IDK, perhaps i2c-gpio,scl-push-pull ? Is that too specific? Are there other
methods to drive a GPIO pin?
Hmm, whatever, I'm ok with i2c-gpio,scl-no-pullup too...

Mixing i2c-gpio,scl-open-drain with whatever new alias should probably be
forbidden even if they trigger the same code in the driver, since it makes
no sense to have neither open-drain AND push-pull, nor open-drain AND
no-pullup.

Cheers,
Peter

>>>  		gflags = GPIOD_OUT_HIGH;
>>>  	else
>>>  		gflags = GPIOD_OUT_HIGH_OPEN_DRAIN;
>>> @@ -400,7 +407,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 || sda_scl_output_only)
>>
>> Same here, why tie the SDA output-only property to the flags of SCL?
>>
>> Cheers,
>> Peter
>>
>>>  		gflags = GPIOD_OUT_HIGH;
>>>  	else
>>>  		gflags = GPIOD_OUT_HIGH_OPEN_DRAIN;
>>> @@ -418,7 +425,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;
>>> diff --git a/include/linux/platform_data/i2c-gpio.h b/include/linux/platform_data/i2c-gpio.h
>>> index a907774fd..e9536c078 100644
>>> --- a/include/linux/platform_data/i2c-gpio.h
>>> +++ b/include/linux/platform_data/i2c-gpio.h
>>> @@ -16,6 +16,8 @@
>>>   *	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.
>>>   * @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.
>>> @@ -24,6 +26,7 @@ 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	scl_is_open_drain:1;
>>>  	unsigned int	scl_is_output_only:1;
>>>  };
>