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