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

[PATCH v2 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

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           |  2 ++
 4 files changed, 30 insertions(+), 6 deletions(-)

-- 
2.35.3

[PATCH v2 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.

Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com>
---
v2:
- improve commit message
---
 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 fd0402845..25cd1b260 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.35.3

[PATCH v2 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>
---
 drivers/i2c/algos/i2c-algo-bit.c | 16 +++++++++++++---
 1 file changed, 13 insertions(+), 3 deletions(-)

diff --git a/drivers/i2c/algos/i2c-algo-bit.c b/drivers/i2c/algos/i2c-algo-bit.c
index fc90293af..ab5a73e90 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).
+	 * 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");
 
@@ -203,6 +204,9 @@ static int i2c_inb(struct i2c_adapter *i2c_adap)
 	unsigned char indata = 0;
 	struct i2c_algo_bit_data *adap = i2c_adap->algo_data;
 
+	if (!adap->getsda)
+		return -EOPNOTSUPP;
+
 	/* assert: scl is low */
 	sdahi(adap);
 	for (i = 0; i < 8; i++) {
@@ -232,6 +236,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)
@@ -670,8 +678,10 @@ 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->getscl == NULL && bit_adap->getsda == NULL)
+		dev_info(&adap->dev, "I2C-like interface, SDA and SCL are write-only\n");
+	else 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.35.3

[PATCH v2 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>
---
 drivers/i2c/busses/i2c-gpio.c          | 14 +++++++++++---
 include/linux/platform_data/i2c-gpio.h |  2 ++
 2 files changed, 13 insertions(+), 3 deletions(-)

diff --git a/drivers/i2c/busses/i2c-gpio.c b/drivers/i2c/busses/i2c-gpio.c
index 7a048abbf..0a87af4a7 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..928c87b00 100644
--- a/include/linux/platform_data/i2c-gpio.h
+++ b/include/linux/platform_data/i2c-gpio.h
@@ -16,6 +16,7 @@
  *	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: I2C-like interface, SDA is write-only.
  * @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 +25,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.35.3

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

[Rob Herring]

On Wed, 27 Apr 2022 21:23:28 +0200, 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>
> ---
> v2:
> - improve commit message
> ---
>  Documentation/devicetree/bindings/i2c/i2c-gpio.yaml | 4 ++++
>  1 file changed, 4 insertions(+)
> 

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

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

[Wolfram Sang]

[-- Attachment #1: Type: text/plain, Size: 1558 bytes --]

Hi Heiner,

>  	/* read ack: SDA should be pulled down by slave, or it may
>  	 * NAK (usually to report problems with the data we wrote).
> +	 * Report ACK if SDA is write-only.

Minor nit: On first read, I didn't understand. "Always report ACK..." is
maybe a tad clearer.

>  	 */
> @@ -203,6 +204,9 @@ static int i2c_inb(struct i2c_adapter *i2c_adap)
>  	unsigned char indata = 0;
>  	struct i2c_algo_bit_data *adap = i2c_adap->algo_data;
>  
> +	if (!adap->getsda)
> +		return -EOPNOTSUPP;

Wouldn't it be better in 'readbytes' returning an errno there?

> -	/* Complain if SCL can't be read */
> -	if (bit_adap->getscl == NULL) {
> +	if (bit_adap->getscl == NULL && bit_adap->getsda == NULL)
> +		dev_info(&adap->dev, "I2C-like interface, SDA and SCL are write-only\n");
> +	else 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");

Hmm, this is a bit inconsistent with dev_warn and dev_info. How about
this?

 	if (bit_adap->getscl == NULL)
  		dev_warn(&adap->dev, "Not I2C compliant: can't read SCL\n");

 	if (bit_adap->getsda == NULL)
  		dev_warn(&adap->dev, "Not I2C compliant: can't read SDA\n");

 	if (bit_adap->getscl == NULL || bit_adap->getsda == NULL)
  		dev_warn(&adap->dev, "Bus may be unreliable\n");

The above code can surely be simplified. I just wanted to show this
simple approach so we can discuss my suggestion.

All the best,

   Wolfram


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

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

[Wolfram Sang]

[-- Attachment #1: Type: text/plain, Size: 661 bytes --]


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

This comment is great!

> + * @sda_is_output_only: I2C-like interface, SDA is write-only.

This one not so much ;)

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

I think we whould use a similar description for SDA like we have for
SCL. Maybe we should also repeat here that this setup is for clients
which can only read SDA/SCL?


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

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

[Heiner Kallweit]

On 16.05.2022 21:31, Wolfram Sang wrote:
> Hi Heiner,
> 
Hi Wolfram,

sorry for answering quite late ..

>>  	/* read ack: SDA should be pulled down by slave, or it may
>>  	 * NAK (usually to report problems with the data we wrote).
>> +	 * Report ACK if SDA is write-only.
> 
> Minor nit: On first read, I didn't understand. "Always report ACK..." is
> maybe a tad clearer.
> 

OK

>>  	 */
>> @@ -203,6 +204,9 @@ static int i2c_inb(struct i2c_adapter *i2c_adap)
>>  	unsigned char indata = 0;
>>  	struct i2c_algo_bit_data *adap = i2c_adap->algo_data;
>>  
>> +	if (!adap->getsda)
>> +		return -EOPNOTSUPP;
> 
> Wouldn't it be better in 'readbytes' returning an errno there?
> 

I think that's something we can do in addition. We have other users of i2c_inb()
than readbytes() (in i2c_algo_pcf), therefore I'd prefer to let i2c_inb()
return an error instead of relying on upper layers only.

>> -	/* Complain if SCL can't be read */
>> -	if (bit_adap->getscl == NULL) {
>> +	if (bit_adap->getscl == NULL && bit_adap->getsda == NULL)
>> +		dev_info(&adap->dev, "I2C-like interface, SDA and SCL are write-only\n");
>> +	else 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");
> 
> Hmm, this is a bit inconsistent with dev_warn and dev_info. How about
> this?
> 
Right, it would be a bit inconsistent. My thought was:
If both getscl and getsda are NULL, then the driver is intentionally used this way
and it reflects the design of the respective system.
It's expected that both are NULL and there's nothing wrong with it.
At least to me a warning means: Something isn't ok and requires an action.

However I could also understand the point of view that everything not being really
I2C-compliant should trigger a warning.

I'm fine with both options, please advise.

>  	if (bit_adap->getscl == NULL)
>   		dev_warn(&adap->dev, "Not I2C compliant: can't read SCL\n");
> 
>  	if (bit_adap->getsda == NULL)
>   		dev_warn(&adap->dev, "Not I2C compliant: can't read SDA\n");
> 
>  	if (bit_adap->getscl == NULL || bit_adap->getsda == NULL)
>   		dev_warn(&adap->dev, "Bus may be unreliable\n");
> 
> The above code can surely be simplified. I just wanted to show this
> simple approach so we can discuss my suggestion.
> 
> All the best,
> 
>    Wolfram
> 

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

[Heiner Kallweit]

On 14.08.2022 23:06, Heiner Kallweit wrote:
> On 16.05.2022 21:31, Wolfram Sang wrote:
>> Hi Heiner,
>>
> Hi Wolfram,
> 
> sorry for answering quite late ..
> 
>>>  	/* read ack: SDA should be pulled down by slave, or it may
>>>  	 * NAK (usually to report problems with the data we wrote).
>>> +	 * Report ACK if SDA is write-only.
>>
>> Minor nit: On first read, I didn't understand. "Always report ACK..." is
>> maybe a tad clearer.
>>
> 
> OK
> 
>>>  	 */
>>> @@ -203,6 +204,9 @@ static int i2c_inb(struct i2c_adapter *i2c_adap)
>>>  	unsigned char indata = 0;
>>>  	struct i2c_algo_bit_data *adap = i2c_adap->algo_data;
>>>  
>>> +	if (!adap->getsda)
>>> +		return -EOPNOTSUPP;
>>
>> Wouldn't it be better in 'readbytes' returning an errno there?
>>
> 
> I think that's something we can do in addition. We have other users of i2c_inb()
> than readbytes() (in i2c_algo_pcf), therefore I'd prefer to let i2c_inb()
> return an error instead of relying on upper layers only.
> 
>>> -	/* Complain if SCL can't be read */
>>> -	if (bit_adap->getscl == NULL) {
>>> +	if (bit_adap->getscl == NULL && bit_adap->getsda == NULL)
>>> +		dev_info(&adap->dev, "I2C-like interface, SDA and SCL are write-only\n");
>>> +	else 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");
>>
>> Hmm, this is a bit inconsistent with dev_warn and dev_info. How about
>> this?
>>
> Right, it would be a bit inconsistent. My thought was:
> If both getscl and getsda are NULL, then the driver is intentionally used this way
> and it reflects the design of the respective system.
> It's expected that both are NULL and there's nothing wrong with it.
> At least to me a warning means: Something isn't ok and requires an action.
> 
> However I could also understand the point of view that everything not being really
> I2C-compliant should trigger a warning.
> 
> I'm fine with both options, please advise.
> 
>>  	if (bit_adap->getscl == NULL)
>>   		dev_warn(&adap->dev, "Not I2C compliant: can't read SCL\n");
>>
>>  	if (bit_adap->getsda == NULL)
>>   		dev_warn(&adap->dev, "Not I2C compliant: can't read SDA\n");
>>
>>  	if (bit_adap->getscl == NULL || bit_adap->getsda == NULL)
>>   		dev_warn(&adap->dev, "Bus may be unreliable\n");
>>
>> The above code can surely be simplified. I just wanted to show this
>> simple approach so we can discuss my suggestion.
>>
>> All the best,
>>
>>    Wolfram
>>
> 
Just stumbled across this open discussion. Could you please have a look
at my feedback to your review comments?
Thanks