[U-Boot] [PATCH v4] sh: i2c: Add support I2C controller of SH7734

[Nobuhiro Iwamatsu]

Hi, Mike.

2012/3/1 Mike Frysinger <vapier at gentoo.org>:
> On Wednesday 29 February 2012 21:58:42 Nobuhiro Iwamatsu wrote:
>> --- /dev/null
>> +++ b/drivers/i2c/sh_sh7734_i2c.c
>>
>> +#include <common.h>
>> +#include <asm/io.h>
>
> should include i2c.h too so you know your funcs match the prototypes everyone
> else uses

OK.

>
>> +static int check_stop(struct sh_i2c *base)
>> +{
>> +     int i, ret = 1;
>> +
>> +     for (i = 0; i < IRQ_WAIT; i++) {
>> +             if (SH_I2C_ICSR_STOP & readb(&base->icsr)) {
>> +                     ret = 0;
>> +                     break;
>> +             }
>> +             udelay(10);
>> +     }
>> +
>> +     clrbits_le8(&base->icsr, SH_I2C_ICSR_STOP);
>> +
>> +     return ret;
>> +}
>> +
>> +static int check_tend(struct sh_i2c *base, int stop)
>> +{
>> +     int i, ret = 1;
>> +
>> +     for (i = 0; i < IRQ_WAIT; i++) {
>> +             if (SH_I2C_ICSR_TEND & readb(&base->icsr)) {
>> +                     ret = 0;
>> +                     break;
>> +             }
>> +             udelay(10);
>> +     }
>> +
>> +     if (stop) {
>> +             u8 data;
>> +
>> +             clrbits_le8(&base->icsr, SH_I2C_ICSR_STOP);
>> +
>> +             sh_i2c_send_stop(base);
>> +     }
>> +
>> +     clrbits_le8(&base->icsr, SH_I2C_ICSR_TEND);
>> +
>> +     return ret;
>> +}
>> +
>> +static int check_tdre(struct sh_i2c *base)
>> +{
>> +     int i;
>> +
>> +     for (i = 0; i < IRQ_WAIT; i++) {
>> +             if (SH_I2C_ICSR_TDRE & readb(&base->icsr))
>> +                     return 0;
>> +             udelay(10);
>> +     }
>> +
>> +     return 1;
>> +}
>> +
>> +static int check_rdrf(struct sh_i2c *base)
>> +{
>> +     int i;
>> +
>> +     for (i = 0; i < IRQ_WAIT; i++) {
>> +             if (SH_I2C_ICSR_RDRF & readb(&base->icsr))
>> +                     return 0;
>> +             udelay(10);
>> +     }
>> +
>> +     return 1;
>> +}
>
> you've got the same icsr bit polling logic here.  probably better to put it
> into a dedicated func which takes the bit flag to check and the rest will call
> that.
>        static int check_icsr_bit(struct sh_i2c *base, uint bit)
>        {
>                int i;
>
>                for (i = 0; i < IRQ_WAIT; i++) {
>                        if (bit & readb(&base->icsr))
>                                return 0;
>                        udelay(10);
>                }
>
>                return 1;
>        }
>
>        static int check_tdre(struct sh_i2c *base)
>        {
>                return check_icsr_bit(base, SH_I2C_ICSR_TDRE);
>        }

OK, I fixed.
>
>> +static u8 i2c_raw_read(struct sh_i2c *base, u8 id, u8 reg)
>> +{
>> ...
>> +     writeb(id << 1 | 1, &base->icdrt);
>
> shifting and bit ops can lead to unexpected behavior ... might be better:
>        writeb((id << 1) | 1, &base->icdrt);
>

OK, I fixed.

>> +/*
>> + * i2c_probe: - Test if a chip answers for a given i2c address
>> + *
>> + * @chip:   address of the chip which is searched for
>> + * @return: 0 if a chip was found, -1 otherwhise
>> + */
>> +int i2c_probe(u8 chip)
>> +{
>> +     return 0;
>> +}
>
> should implement this.  look at the Blackfin twi driver for an easy one.

Thanks for your infomation.
I implemented this.

Thanks,
  Nobuhiro
-- 
Nobuhiro Iwamatsu