hello,
On Fri, Mar 13, 2015 at 11:59:58AM +0530, Jayachandran C wrote:
> diff --git a/drivers/i2c/busses/i2c-xlp9xx.c b/drivers/i2c/busses/i2c-xlp9xx.c
> new file mode 100644
> index 00000000..2f303ad
> --- /dev/null
> +++ b/drivers/i2c/busses/i2c-xlp9xx.c
> @@ -0,0 +1,446 @@
> +/*
> + * Copyright (c) 2003-2015 Broadcom Corporation
> + *
> + * This file is licensed under the terms of the GNU General Public
> + * License version 2. This program is licensed "as is" without any
> + * warranty of any kind, whether express or implied.
> + */
> +
> +/*
> + * see Documentation/devicetree/bindings/i2c/i2c-xlp9xx.txt for device
> + * tree bindings documentation
> + */
When I asked for documentation here, I didn't meant the device tree
binding, but the hardware reference manual.
> [...]
> +static void xlp9xx_i2c_mask_irq(struct xlp9xx_i2c_dev *priv, u32 mask)
> +{
> + mask = xlp9xx_read_i2c_reg(priv, XLP9XX_I2C_INTEN) & ~mask;
> + xlp9xx_write_i2c_reg(priv, XLP9XX_I2C_INTEN, mask);
You didn't comment on my suggestion to add a variable here for improved
readability:
u32 inten = xlp9xx_read_i2c_reg(...);
inten &= ~mask;
xlp9xx_write_i2c_reg(...);
> +}
> +
> +static void xlp9xx_i2c_unmask_irq(struct xlp9xx_i2c_dev *priv, u32 mask)
> +{
> + mask = xlp9xx_read_i2c_reg(priv, XLP9XX_I2C_INTEN) | mask;
> + xlp9xx_write_i2c_reg(priv, XLP9XX_I2C_INTEN, mask);
> +}
> +
> +static void xlp9xx_i2c_set_rx_fifo_thres(struct xlp9xx_i2c_dev *priv,
> + u32 th)
> +{
> + xlp9xx_write_i2c_reg(priv, XLP9XX_I2C_MFIFOCTRL,
> + (th << XLP9XX_I2C_MFIFOCTRL_HITH_SHIFT));
parenthesis are not needed here.
I suggested to move the min calculation that you have to do before each
call to this function into it. You replied:
The xlp9xx_i2c_... functions were written to do the
hardware/register operations, so it is better to have this logic
here.
...
> +}
> +
> +static void xlp9xx_i2c_fill_tx_fifo(struct xlp9xx_i2c_dev *priv)
> +{
> + u32 len, i;
> + u8 *buf = priv->msg_buf;
> +
> + len = min(priv->msg_buf_remaining, XLP9XX_I2C_FIFO_SIZE);
... this didn't stop you here though :-) So I'm still convinced that
having the min function in xlp9xx_i2c_set_rx_fifo_thres is a good idea.
> + for (i = 0; i < len; i++)
> + xlp9xx_write_i2c_reg(priv, XLP9XX_I2C_MTXFIFO, buf[i]);
> + priv->msg_buf_remaining -= len;
> + priv->msg_buf += len;
> +}
> [...]
> +static int xlp9xx_i2c_init(struct xlp9xx_i2c_dev *priv)
> +{
> + u32 prescale;
> +
> + /*
> + * The controller uses 5 * SCL clock internally.
> + * So prescale value should be divided by 5.
> + */
> + prescale = (((XLP9XX_I2C_IP_CLK_FREQ / priv->clk_hz) - 8) / 5) - 1;
I still wonder if you should round differently here.
> + xlp9xx_write_i2c_reg(priv, XLP9XX_I2C_CTRL, XLP9XX_I2C_CTRL_RST);
> + xlp9xx_write_i2c_reg(priv, XLP9XX_I2C_CTRL, XLP9XX_I2C_CTRL_EN |
> + XLP9XX_I2C_CTRL_MASTER);
> + xlp9xx_write_i2c_reg(priv, XLP9XX_I2C_DIV, prescale);
> + xlp9xx_write_i2c_reg(priv, XLP9XX_I2C_INTEN, 0);
> +
> + return 0;
> +}
Best regards
Uwe
--
Pengutronix e.K. | Uwe Kleine-König |
Industrial Linux Solutions | http://www.pengutronix.de/ |
--
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
