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

[Heiner Kallweit <hkallweit1@gmail.com>]

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