Dear Vipin KUMAR,
In message <1260955110-5656-2-git-send-email-vipin.ku...@st.com> you wrote:
>
> Signed-off-by: Vipin <vipin.ku...@st.com>
> ---
> drivers/i2c/Makefile | 1 +
> drivers/i2c/spr_i2c.c | 321
> ++++++++++++++++++++++++++++++++++
> include/asm-arm/arch-spear/spr_i2c.h | 143 +++++++++++++++
> 3 files changed, 465 insertions(+), 0 deletions(-)
> mode change 100644 => 100755 drivers/i2c/Makefile
> create mode 100755 drivers/i2c/spr_i2c.c
> create mode 100755 include/asm-arm/arch-spear/spr_i2c.h
Your patch order is, um, sub-optimal.
You start adding an I2C driver for a non-existing CPU here.
This makes no sense, please reorder.
> --- a/drivers/i2c/Makefile
> +++ b/drivers/i2c/Makefile
> @@ -38,6 +38,7 @@ COBJS-$(CONFIG_DRIVER_S3C24X0_I2C) += s3c24x0_i2c.o
> COBJS-$(CONFIG_S3C44B0_I2C) += s3c44b0_i2c.o
> COBJS-$(CONFIG_SOFT_I2C) += soft_i2c.o
> COBJS-$(CONFIG_TSI108_I2C) += tsi108_i2c.o
> +COBJS-$(CONFIG_SPEARI2C) += spr_i2c.o
Please keep lists sorted (fix globally).
> +/**
> + * i2c_setfreq - Set i2c working mode frequency
> + *
> + * Set i2c working mode frequency
> + */
Incorrect multiline comment style. Please fix globally.
> +static void set_speed(int i2c_spd)
> +{
> + unsigned int cntl;
> +
> + if (i2c_spd == IC_SPEED_MODE_MAX) {
> + cntl = readl(&i2c_regs_p->ic_con);
> + cntl |= IC_CON_SPH | IC_CON_SPL;
> + writel(cntl, &i2c_regs_p->ic_con);
> + i2c_setfreq(MIN_HS_SCL_HIGHTIME, MIN_HS_SCL_LOWTIME);
> + } else if (i2c_spd == IC_SPEED_MODE_FAST) {
> + cntl = readl(&i2c_regs_p->ic_con);
> + cntl |= IC_CON_SPH;
> + cntl &= ~IC_CON_SPL;
> + writel(cntl, &i2c_regs_p->ic_con);
> + i2c_setfreq(MIN_FS_SCL_HIGHTIME, MIN_FS_SCL_LOWTIME);
> + } else if (i2c_spd == IC_SPEED_MODE_STANDARD) {
> + cntl = readl(&i2c_regs_p->ic_con);
> + cntl |= IC_CON_SPF;
> + cntl &= ~IC_CON_SPL;
> + writel(cntl, &i2c_regs_p->ic_con);
> + i2c_setfreq(MIN_SS_SCL_HIGHTIME, MIN_SS_SCL_LOWTIME);
> + }
It seems you can move the lines
writel(cntl, &i2c_regs_p->ic_con);
i2c_setfreq(MIN_FS_SCL_HIGHTIME, MIN_FS_SCL_LOWTIME);
out of the if/else blocks and make them common code.
> +void i2c_set_bus_speed(int speed)
> +{
> + if (speed >= I2C_MAX_SPEED)
> + set_speed(IC_SPEED_MODE_MAX);
> + else
> + if (speed >= I2C_FAST_SPEED)
Missing braces (mandatory for multiline statements).
> + set_speed(IC_SPEED_MODE_FAST);
> + else
> + set_speed(IC_SPEED_MODE_STANDARD);
> +}
> +/**
> + * i2c_get_bus_speed - Gets the i2c speed
> + *
> + * Gets the i2c speed.
> + */
> +int i2c_get_bus_speed(void)
> +{
> + if (((readl(&i2c_regs_p->ic_con) & IC_CON_SPH) == IC_CON_SPH) &&
> + ((readl(&i2c_regs_p->ic_con) & IC_CON_SPL) == IC_CON_SPL)) {
> + return I2C_MAX_SPEED;
> +
> + } else if (((readl(&i2c_regs_p->ic_con) & IC_CON_SPH) == IC_CON_SPH) &&
> + ((readl(&i2c_regs_p->ic_con) & IC_CON_SPL) == 0)) {
> + return I2C_FAST_SPEED;
> +
> + } else if (((readl(&i2c_regs_p->ic_con) & IC_CON_SPF) == IC_CON_SPF) &&
> + ((readl(&i2c_regs_p->ic_con) & IC_CON_SPL) == 0)) {
> + return I2C_STANDARD_SPEED;
> + }
It makes no sense to run "readl(&i2c_regs_p->ic_con)" six times - run
it once and latch the value.
Also I tend to think the logic can be written clearer.
> +void i2c_init(int speed, int slaveadd)
> +{
> + unsigned int enbl;
> +
> + /* Disable i2c */
> + enbl = readl(&i2c_regs_p->ic_enable);
> + enbl &= ~IC_ENABLE_0B;
> + writel(enbl, &i2c_regs_p->ic_enable);
> +
> + writel((IC_CON_SD | IC_CON_SPF | IC_CON_MM), &i2c_regs_p->ic_con);
> + writel(IC_TL0, &i2c_regs_p->ic_rx_tl);
> + writel(IC_TL0, &i2c_regs_p->ic_tx_tl);
Is this duplication intentional? If so, a comment is needed to explain
why.
> +/**
> + * i2c_probe - Probe the i2c chip
> + *
> + * TBD
> + */
> +int i2c_probe(uchar chip)
> +{
> + return 0;
> +}
Please do not add dead code.
> +int i2c_read(uchar chip, uint addr, int alen, uchar *buffer, int len)
> +{
> + unsigned long start_time_rx;
> +
> + if (buffer == NULL) {
> + printf("I2C read: buffer is invalid\n");
> + return 1;
> + }
> +
> + if (alen > 1) {
> + printf("I2C read: addr len %d not supported\n", alen);
> + return 1;
> + }
> +
> + if (addr + len > 256) {
> + printf("I2C read: address out of range\n");
> + return 1;
> + }
> +
> + if (i2c_wait_for_bb())
> + return 1;
Why no error message here?
> + i2c_setaddress(chip);
> + writel(addr, &i2c_regs_p->ic_cmd_data);
> +
> + start_time_rx = get_timer_masked();
> + while (len) {
> + writel(IC_CMD, &i2c_regs_p->ic_cmd_data);
> + if ((readl(&i2c_regs_p->ic_status) & IC_STATUS_RFNE) ==
> + IC_STATUS_RFNE) {
> + *buffer++ = (uchar)readl(&i2c_regs_p->ic_cmd_data);
> + len--;
> + start_time_rx = get_timer_masked();
> + } else {
> + if (get_timer(start_time_rx) > I2C_BYTE_TO)
> + return 1;
Why no error message here?
> + udelay(4000);
Why is this needed?
> + if ((readl(&i2c_regs_p->ic_raw_intr_stat) & IC_STOP_DET))
> + readl(&i2c_regs_p->ic_clr_stop_det);
> +
> + if (i2c_wait_for_bb())
> + return 1;
Why no error message here?
> + i2c_flush_rxfifo();
> +
> + return 0;
> +}
> +
> +/**
> + * i2c_write - Write to i2c memory
> + * @chip: target i2c address
> + * @addr: address to read from
> + * @alen:
> + * @buffer: buffer for read data
> + * @len: no of bytes to be read
> + *
> + * Write to i2c memory.
> + */
> +int i2c_write(uchar chip, uint addr, int alen, uchar *buffer, int len)
> +{
> + int nb = len;
> + unsigned long start_time_tx;
> +
> + if (buffer == NULL) {
> + printf("I2C write: buffer is invalid\n");
> + return 1;
> + }
> +
> + if (alen > 1) {
> + printf("I2C write: addr len %d not supported\n", alen);
> + return 1;
> + }
> +
> + if (addr + len > 256) {
> + printf("I2C write: address out of range\n");
> + return 1;
> + }
> +
> + if (i2c_wait_for_bb())
> + return 1;
> +
> + i2c_setaddress(chip);
> +
> + writel(addr, &i2c_regs_p->ic_cmd_data);
> +
> + start_time_tx = get_timer_masked();
> + while (len) {
> + if ((readl(&i2c_regs_p->ic_status) & IC_STATUS_TFNF)
> + == IC_STATUS_TFNF) {
> + writel(*buffer, &i2c_regs_p->ic_cmd_data);
> + buffer++;
> + len--;
> + start_time_tx = get_timer_masked();
> + } else {
> + if (get_timer(start_time_tx) > (nb * I2C_BYTE_TO))
> + return 1;
> + }
> + }
> +
> + udelay(4000);
> + if ((readl(&i2c_regs_p->ic_raw_intr_stat) & IC_STOP_DET))
> + readl(&i2c_regs_p->ic_clr_stop_det);
> +
> + if (i2c_wait_for_bb())
> + return 1;
> +
> + i2c_flush_rxfifo();
> +
> + return 0;
> +}
This shares a _lot_ of common code with i2c_read() - factor out?
Best regards,
Wolfgang Denk
--
DENX Software Engineering GmbH, MD: Wolfgang Denk & Detlev Zundel
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: w...@denx.de
I have a theory that it's impossible to prove anything, but I can't
prove it.
_______________________________________________
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot
