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

[Heiner Kallweit <hkallweit1@gmail.com>]

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
>