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
