Re: [PATCH RESEND] i2c: add sc18is600 driver

[Andy Shevchenko]

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