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
