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

[Peter Rosin <peda@axentia.se>]

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