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

[Heiner Kallweit <hkallweit1@gmail.com>]

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