On Tue, Jun 13, 2017 at 6:47 PM, Sebastian Reichel <sre@xxxxxxxxxx> 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. > +static void sc18is600_setup_clock_frequency(struct sc18is600dev *dev) > +{ > + int reg = DIV_ROUND_UP(dev->clock_base, dev->i2c_clock_frequency); > + > + if (reg < 5) > + reg = 5; > + if (reg > 255) > + reg = 255; clamp_t() > + > + dev_dbg(&dev->spi->dev, "i2c clock frequency: %08x", reg); > + regmap_write(dev->regmap, SC18IS600_REG_I2C_CLOCK, reg); > +} > +static void sc18is600_setup_timeout(struct sc18is600dev *dev, > + bool enable, int timeout_ms) > +{ > + int timeout = DIV_ROUND_UP(timeout_ms * dev->chip->timeout_base, 10000); > + u8 reg; > + > + if (timeout <= 0) > + timeout = 1; > + if (timeout > 255) > + timeout = 255; Ditto. > + > + reg = (timeout & 0x7F) << 1; > + reg |= (!!enable); Redundant parens. Might be one line. > + > + dev_dbg(&dev->spi->dev, "i2c timeout: %08x", reg); > + regmap_write(dev->regmap, SC18IS600_REG_I2C_TIMEOUT, reg); > +} > +static int sc18is600_xfer(struct i2c_adapter *adapter, > + struct i2c_msg *msgs, int num) > +{ > + if (num == 1 && read_operations == 1) > + err = sc18is600_read(dev, &msgs[0]); > + else if (num == 1) > + err = sc18is600_write(dev, &msgs[0]); > + else if (num == 2 && read_operations == 1) > + err = sc18is600_read_after_write(dev, &msgs[0], &msgs[1]); > + else if (num == 2) > + err = sc18is600_write_after_write(dev, &msgs[0], &msgs[1]); > + else > + return -EOPNOTSUPP; It will look better if (x == 1) { if (y) else } else if (x == 2) { if (y) else } > + switch (dev->state) { > + case SC18IS600_STAT_OK: > + break; > + case SC18IS600_STAT_NAK_ADDR: > + return -EIO; > + case SC18IS600_STAT_NAK_DATA: > + return -EREMOTEIO; > + case SC18IS600_STAT_SIZE: > + return -EINVAL; > + case SC18IS600_STAT_TIMEOUT: > + return -ETIMEDOUT; > + case SC18IS600_STAT_TIMEOUT2: > + return -ETIMEDOUT; > + case SC18IS600_STAT_BLOCKED: > + return -ETIMEDOUT; You may use case X: case Y: return Z; > + default: > + case SC18IS600_STAT_BUSY: > + dev_err(&dev->spi->dev, "device hangup detected, reset!"); > + sc18is600_reset(dev); > + return -EAGAIN; > + } > +static int sc18is600_probe(struct spi_device *spi) > +{ > + const struct of_device_id *of_id; > + struct sc18is600dev *dev; > + int err; > + > + of_id = of_match_device(sc18is600_of_match, &spi->dev); > + if (!of_id) > + return -ENODEV; of_device_get_match_data() ? > + dev = devm_kzalloc(&spi->dev, sizeof(*dev), GFP_KERNEL); > + if (dev == NULL) if (!dev) ...and better to use s600dev or alike to avoid confusion. > + return -ENOMEM; > + snprintf(dev->adapter.name, sizeof(dev->adapter.name), > + "SC18IS600 at SPI %02d device %02d", Isn't too much for adapter name? I don't remember if it's part of ABI, in that case it's even worse. > + spi->master->bus_num, spi->chip_select); > + if (!dev->chip->clock_base) { > + dev->clk = devm_clk_get(&spi->dev, "clkin"); > + if (IS_ERR(dev->clk)) { > + err = PTR_ERR(dev->clk); > + dev_err(&spi->dev, "could not acquire vdd: %d\n", err); vdd-> Vdd ? > + return err; > + } > + > + clk_prepare_enable(dev->clk); This might fail. > + dev->clock_base = clk_get_rate(dev->clk) / 4; Magic. > + } else { > + dev->clock_base = dev->chip->clock_base; > + } -- With Best Regards, Andy Shevchenko -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html