Re: [PATCHv2] i2c: add sc18is600 driver

[Wolfram Sang]

Hi,

thanks for V2.

On Sat, Jul 01, 2017 at 05:08:31PM +0200, Sebastian Reichel wrote:
> This adds an IÂC master driver for SPI -> IÂC bus bridge chips.
> It currently supports NXP's SC18IS600 and SC18IS601, as well as
> Silicon Labs' CP2120. The driver was only tested on SC18IS600.

I prefer 'I2C' to "IÂC' to keep things simple, but I'll leave it to you.

> +++ b/Documentation/devicetree/bindings/i2c/i2c-cp2120.txt
> @@ -0,0 +1 @@
> +Please see binding for i2c-sc18is600

I still prefer this to be dropped. There is no i2c-cp2120 driver.

> + * Datasheets:
> + *  - http://www.nxp.com/documents/data_sheet/SC18IS600.pdf
> + *  - https://www.silabs.com/documents/public/data-sheets/CP2120.pdf

Nice pointers.

> +static int sc18is600_read_write(struct sc18is600dev *s600dev,

This function used to be named '*_read_after_write' which is correct.
'*_read_write' sounds like it does the opposite!

> +static int sc18is600_xfer(struct i2c_adapter *adapter,
> +			  struct i2c_msg *msgs, int num)
> +{
> +	struct sc18is600dev *s600dev = adapter->algo_data;
> +	int read_operations = 0;
> +	bool ignore_nak = false;
> +	int i, err;
> +
> +	for (i = 0; i < num; i++) {
> +		if (msgs[i].len > s600dev->chip->buffer_size)
> +			return -EOPNOTSUPP;
> +
> +		/* chip only support standard read & write */
> +		if (msgs[i].flags & ~I2C_M_RD)
> +			return -EOPNOTSUPP;

This will bail out on I2C_M_IGNORE_NAK which you added support for.
Didn't you test that?

> +
> +		if (msgs[i].flags & I2C_M_RD)
> +			read_operations++;
> +	}

I'd think the for loop can completely go by populating an i2c_quirks
structure. From what I see, you'd need I2C_AQ_COMB |
I2C_AQ_COMB_WRITE_FIRST with max_comb_1st_msg_len and
max_comb_2nd_msg_len properly set. But please double check.


> +
> +	reinit_completion(&s600dev->completion);
> +
> +	if (num == 1) {
> +		if (msgs[0].flags & I2C_M_IGNORE_NAK)
> +			ignore_nak = true;

Why do you ignore NAK only for 1 message?

> +
> +		if (read_operations == 1)
> +			err = sc18is600_read(s600dev, &msgs[0]);
> +		else
> +			err = sc18is600_write(s600dev, &msgs[0]);
> +	} else if (num == 2) {
> +		if (read_operations == 1)
> +			err = sc18is600_read_write(s600dev, &msgs[0], &msgs[1]);
> +		else
> +			err = sc18is600_write_write(s600dev, &msgs[0], &msgs[1]);
> +	} else {
> +		return -EOPNOTSUPP;
> +	}

With i2c_quirks, the last else-block could also go.


> +	if (!err) {
> +		dev_warn(&s600dev->spi->dev,
> +			 "timeout waiting for irq, poll status register");

Don't log on timeouts, it is not unusual to happen on the bus.

> +		s600dev->state = SC18IS600_STAT_BUSY;
> +		regmap_read(s600dev->regmap, SC18IS600_REG_I2C_STAT,
> +			    &s600dev->state);
> +	}

...

> +	s600dev->spi = spi;
> +	s600dev->adapter.owner = THIS_MODULE;
> +	s600dev->adapter.class = I2C_CLASS_DEPRECATED;

Huh? This is a new driver, why do you need it? Hmm, maybe I could
rephrase the docs a little.

...

> +		err = clk_prepare_enable(s600dev->clk);

You never disable the clock anywhere.

Regards,

   Wolfram

Attachment: signature.asc
Description: PGP signature