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

[Wolfram Sang <wsa@kernel.org>]

[-- Attachment #1: Type: text/plain, Size: 1558 bytes --]

Hi Heiner,

>  	/* 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.

>  	 */
> @@ -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?

> -	/* 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?

 	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


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]