[U-Boot] [PATCH 1/9] i2c driver support for SPEAr SoCs

[Wolfgang Denk <wd@denx.de>]

Dear Vipin KUMAR,

In message <1260955110-5656-2-git-send-email-vipin.kumar@st.com> you wrote:
> 
> Signed-off-by: Vipin <vipin.kumar@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: wd at denx.de
I have a theory that it's impossible to prove anything, but  I  can't
prove it.