| Message ID | 1411470944-6147-1-git-send-email-marex@denx.de |
|---|---|
| State | Superseded |
| Delegated to: | Heiko Schocher |
| Headers | show |
Hello Marek, Am 23.09.2014 13:15, schrieb Marek Vasut: > The i.MX28 has two I2C IP blocks, but the MXS I2C driver is hard-coded > to use the I2C block 0 . Add multibus support so we can use both I2C > busses as seen fit. > > Signed-off-by: Marek Vasut<marex@denx.de> > Cc: Stefano Babic<sbabic@denx.de> > --- > drivers/i2c/mxs_i2c.c | 47 +++++++++++++++++++++++++++++++++++++---------- > include/configs/mxs.h | 1 + > 2 files changed, 38 insertions(+), 10 deletions(-) Sorry, I have to NACK this patch. Please convert the driver for using the CONFIG_SYS_I2C framework, see for example the drivers/i2c/mxc_i2c.c driver, thanks! bye, Heiko > > diff --git a/drivers/i2c/mxs_i2c.c b/drivers/i2c/mxs_i2c.c > index de3b194..989cf07 100644 > --- a/drivers/i2c/mxs_i2c.c > +++ b/drivers/i2c/mxs_i2c.c > @@ -24,9 +24,12 @@ > > #define MXS_I2C_MAX_TIMEOUT 1000000 > > +static struct mxs_i2c_regs *i2c_regs = (struct mxs_i2c_regs *)MXS_I2C0_BASE; > +static int mxs_i2c_bus; > +static int mxs_i2c_speed = CONFIG_SYS_I2C_SPEED; > + > static void mxs_i2c_reset(void) > { > - struct mxs_i2c_regs *i2c_regs = (struct mxs_i2c_regs *)MXS_I2C0_BASE; > int ret; > int speed = i2c_get_bus_speed(); > > @@ -48,8 +51,6 @@ static void mxs_i2c_reset(void) > > static void mxs_i2c_setup_read(uint8_t chip, int len) > { > - struct mxs_i2c_regs *i2c_regs = (struct mxs_i2c_regs *)MXS_I2C0_BASE; > - > writel(I2C_QUEUECMD_RETAIN_CLOCK | I2C_QUEUECMD_PRE_SEND_START | > I2C_QUEUECMD_MASTER_MODE | I2C_QUEUECMD_DIRECTION | > (1<< I2C_QUEUECMD_XFER_COUNT_OFFSET), > @@ -67,7 +68,6 @@ static void mxs_i2c_setup_read(uint8_t chip, int len) > static int mxs_i2c_write(uchar chip, uint addr, int alen, > uchar *buf, int blen, int stop) > { > - struct mxs_i2c_regs *i2c_regs = (struct mxs_i2c_regs *)MXS_I2C0_BASE; > uint32_t data, tmp; > int i, remain, off; > int timeout = MXS_I2C_MAX_TIMEOUT; > @@ -124,7 +124,6 @@ static int mxs_i2c_write(uchar chip, uint addr, int alen, > > static int mxs_i2c_wait_for_ack(void) > { > - struct mxs_i2c_regs *i2c_regs = (struct mxs_i2c_regs *)MXS_I2C0_BASE; > uint32_t tmp; > int timeout = MXS_I2C_MAX_TIMEOUT; > > @@ -162,7 +161,6 @@ err: > > int i2c_read(uchar chip, uint addr, int alen, uchar *buffer, int len) > { > - struct mxs_i2c_regs *i2c_regs = (struct mxs_i2c_regs *)MXS_I2C0_BASE; > uint32_t tmp = 0; > int timeout = MXS_I2C_MAX_TIMEOUT; > int ret; > @@ -237,7 +235,6 @@ int i2c_probe(uchar chip) > > int i2c_set_bus_speed(unsigned int speed) > { > - struct mxs_i2c_regs *i2c_regs = (struct mxs_i2c_regs *)MXS_I2C0_BASE; > /* > * The timing derivation algorithm. There is no documentation for this > * algorithm available, it was derived by using the scope and fiddling > @@ -278,7 +275,6 @@ int i2c_set_bus_speed(unsigned int speed) > > unsigned int i2c_get_bus_speed(void) > { > - struct mxs_i2c_regs *i2c_regs = (struct mxs_i2c_regs *)MXS_I2C0_BASE; > uint32_t clk = mxc_get_clock(MXC_XTAL_CLK); > uint32_t timing0; > > @@ -290,10 +286,41 @@ unsigned int i2c_get_bus_speed(void) > return clk / ((((timing0>> 16) - 3) * 2) + 38); > } > > -void i2c_init(int speed, int slaveadd) > +int i2c_set_bus_num(unsigned int bus) > { > + uint32_t mxs_i2c_regs; > + > + switch (bus) { > + case 0: > + mxs_i2c_regs = MXS_I2C0_BASE; > + break; > +#ifdef CONFIG_MX28 > + case 1: > + mxs_i2c_regs = MXS_I2C1_BASE; > + break; > +#endif > + default: > + printf("Bad bus: %d\n", bus); > + return -EINVAL; > + } > + > + mxs_i2c_bus = bus; > + i2c_regs = (struct mxs_i2c_regs *)mxs_i2c_regs; > + > mxs_i2c_reset(); > - i2c_set_bus_speed(speed); > + i2c_set_bus_speed(mxs_i2c_speed); > > + return 0; > +} > + > +unsigned int i2c_get_bus_num(void) > +{ > + return mxs_i2c_bus; > +} > + > +void i2c_init(int speed, int slaveadd) > +{ > + mxs_i2c_speed = speed; > + i2c_set_bus_num(0); > return; > } > diff --git a/include/configs/mxs.h b/include/configs/mxs.h > index eb96fc1..3936029 100644 > --- a/include/configs/mxs.h > +++ b/include/configs/mxs.h > @@ -149,6 +149,7 @@ > /* I2C */ > #ifdef CONFIG_CMD_I2C > #define CONFIG_I2C_MXS > +#define CONFIG_I2C_MULTI_BUS > #define CONFIG_HARD_I2C > #ifndef CONFIG_SYS_I2C_SPEED > #define CONFIG_SYS_I2C_SPEED 400000
On Tue, Sep 23, 2014 at 8:48 AM, Heiko Schocher <hs@denx.de> wrote: > Hello Marek, > > Am 23.09.2014 13:15, schrieb Marek Vasut: >> >> The i.MX28 has two I2C IP blocks, but the MXS I2C driver is hard-coded >> to use the I2C block 0 . Add multibus support so we can use both I2C >> busses as seen fit. >> >> Signed-off-by: Marek Vasut<marex@denx.de> >> Cc: Stefano Babic<sbabic@denx.de> >> --- >> drivers/i2c/mxs_i2c.c | 47 >> +++++++++++++++++++++++++++++++++++++---------- >> include/configs/mxs.h | 1 + >> 2 files changed, 38 insertions(+), 10 deletions(-) > > > Sorry, I have to NACK this patch. Please convert the driver for using > the CONFIG_SYS_I2C framework, see for example the drivers/i2c/mxc_i2c.c > driver, thanks! I think the conversion could be a follow up patch, I see no good reason to block this patch as is.
On Tuesday, September 23, 2014 at 03:49:27 PM, Otavio Salvador wrote: > On Tue, Sep 23, 2014 at 8:48 AM, Heiko Schocher <hs@denx.de> wrote: > > Hello Marek, > > > > Am 23.09.2014 13:15, schrieb Marek Vasut: > >> The i.MX28 has two I2C IP blocks, but the MXS I2C driver is hard-coded > >> to use the I2C block 0 . Add multibus support so we can use both I2C > >> busses as seen fit. > >> > >> Signed-off-by: Marek Vasut<marex@denx.de> > >> Cc: Stefano Babic<sbabic@denx.de> > >> --- > >> > >> drivers/i2c/mxs_i2c.c | 47 > >> > >> +++++++++++++++++++++++++++++++++++++---------- > >> > >> include/configs/mxs.h | 1 + > >> 2 files changed, 38 insertions(+), 10 deletions(-) > > > > Sorry, I have to NACK this patch. Please convert the driver for using > > the CONFIG_SYS_I2C framework, see for example the drivers/i2c/mxc_i2c.c > > driver, thanks! > > I think the conversion could be a follow up patch, I see no good > reason to block this patch as is. I disagree with you. Accepting this patch as is would only hinder progress toward the conversion and would set a bad example further down the line, leading only to more crap patches. Moreover, this is 2015.01 matter, so there is plenty of time for the rework to take place. Thus, I agree with Heiko. Best regards, Marek Vasut
Hi Marek, On Tue, Sep 23, 2014 at 8:15 AM, Marek Vasut <marex@denx.de> wrote: > -void i2c_init(int speed, int slaveadd) > +int i2c_set_bus_num(unsigned int bus) > { > + uint32_t mxs_i2c_regs; > + > + switch (bus) { > + case 0: > + mxs_i2c_regs = MXS_I2C0_BASE; > + break; > +#ifdef CONFIG_MX28 > + case 1: > + mxs_i2c_regs = MXS_I2C1_BASE; > + break; > +#endif > + default: > + printf("Bad bus: %d\n", bus); Something like "Invalid I2C bus" would be a clearer error message here.
Hello Marek, Am 23.09.2014 16:30, schrieb Marek Vasut: > On Tuesday, September 23, 2014 at 03:49:27 PM, Otavio Salvador wrote: >> On Tue, Sep 23, 2014 at 8:48 AM, Heiko Schocher<hs@denx.de> wrote: >>> Hello Marek, >>> >>> Am 23.09.2014 13:15, schrieb Marek Vasut: >>>> The i.MX28 has two I2C IP blocks, but the MXS I2C driver is hard-coded >>>> to use the I2C block 0 . Add multibus support so we can use both I2C >>>> busses as seen fit. >>>> >>>> Signed-off-by: Marek Vasut<marex@denx.de> >>>> Cc: Stefano Babic<sbabic@denx.de> >>>> --- >>>> >>>> drivers/i2c/mxs_i2c.c | 47 >>>> >>>> +++++++++++++++++++++++++++++++++++++---------- >>>> >>>> include/configs/mxs.h | 1 + >>>> 2 files changed, 38 insertions(+), 10 deletions(-) >>> >>> Sorry, I have to NACK this patch. Please convert the driver for using >>> the CONFIG_SYS_I2C framework, see for example the drivers/i2c/mxc_i2c.c >>> driver, thanks! >> >> I think the conversion could be a follow up patch, I see no good >> reason to block this patch as is. > > I disagree with you. Accepting this patch as is would only hinder progress > toward the conversion and would set a bad example further down the line, leading Full ack ... > only to more crap patches. Moreover, this is 2015.01 matter, so there is plenty > of time for the rework to take place. Thus, I agree with Heiko. Ok, perfect, thanks! And reworking this driver should be easy... bye, Heiko
Hello Fabio, Am 23.09.2014 17:16, schrieb Fabio Estevam: > Hi Marek, > > On Tue, Sep 23, 2014 at 8:15 AM, Marek Vasut<marex@denx.de> wrote: > >> -void i2c_init(int speed, int slaveadd) >> +int i2c_set_bus_num(unsigned int bus) >> { >> + uint32_t mxs_i2c_regs; >> + >> + switch (bus) { >> + case 0: >> + mxs_i2c_regs = MXS_I2C0_BASE; >> + break; >> +#ifdef CONFIG_MX28 >> + case 1: >> + mxs_i2c_regs = MXS_I2C1_BASE; >> + break; >> +#endif >> + default: >> + printf("Bad bus: %d\n", bus); > > Something like "Invalid I2C bus" would be a clearer error message here. If Marek convert this driver to the CONFIG_SYS_I2C framework, he can drop this function complete ... bye, Heiko
diff --git a/drivers/i2c/mxs_i2c.c b/drivers/i2c/mxs_i2c.c index de3b194..989cf07 100644 --- a/drivers/i2c/mxs_i2c.c +++ b/drivers/i2c/mxs_i2c.c @@ -24,9 +24,12 @@ #define MXS_I2C_MAX_TIMEOUT 1000000 +static struct mxs_i2c_regs *i2c_regs = (struct mxs_i2c_regs *)MXS_I2C0_BASE; +static int mxs_i2c_bus; +static int mxs_i2c_speed = CONFIG_SYS_I2C_SPEED; + static void mxs_i2c_reset(void) { - struct mxs_i2c_regs *i2c_regs = (struct mxs_i2c_regs *)MXS_I2C0_BASE; int ret; int speed = i2c_get_bus_speed(); @@ -48,8 +51,6 @@ static void mxs_i2c_reset(void) static void mxs_i2c_setup_read(uint8_t chip, int len) { - struct mxs_i2c_regs *i2c_regs = (struct mxs_i2c_regs *)MXS_I2C0_BASE; - writel(I2C_QUEUECMD_RETAIN_CLOCK | I2C_QUEUECMD_PRE_SEND_START | I2C_QUEUECMD_MASTER_MODE | I2C_QUEUECMD_DIRECTION | (1 << I2C_QUEUECMD_XFER_COUNT_OFFSET), @@ -67,7 +68,6 @@ static void mxs_i2c_setup_read(uint8_t chip, int len) static int mxs_i2c_write(uchar chip, uint addr, int alen, uchar *buf, int blen, int stop) { - struct mxs_i2c_regs *i2c_regs = (struct mxs_i2c_regs *)MXS_I2C0_BASE; uint32_t data, tmp; int i, remain, off; int timeout = MXS_I2C_MAX_TIMEOUT; @@ -124,7 +124,6 @@ static int mxs_i2c_write(uchar chip, uint addr, int alen, static int mxs_i2c_wait_for_ack(void) { - struct mxs_i2c_regs *i2c_regs = (struct mxs_i2c_regs *)MXS_I2C0_BASE; uint32_t tmp; int timeout = MXS_I2C_MAX_TIMEOUT; @@ -162,7 +161,6 @@ err: int i2c_read(uchar chip, uint addr, int alen, uchar *buffer, int len) { - struct mxs_i2c_regs *i2c_regs = (struct mxs_i2c_regs *)MXS_I2C0_BASE; uint32_t tmp = 0; int timeout = MXS_I2C_MAX_TIMEOUT; int ret; @@ -237,7 +235,6 @@ int i2c_probe(uchar chip) int i2c_set_bus_speed(unsigned int speed) { - struct mxs_i2c_regs *i2c_regs = (struct mxs_i2c_regs *)MXS_I2C0_BASE; /* * The timing derivation algorithm. There is no documentation for this * algorithm available, it was derived by using the scope and fiddling @@ -278,7 +275,6 @@ int i2c_set_bus_speed(unsigned int speed) unsigned int i2c_get_bus_speed(void) { - struct mxs_i2c_regs *i2c_regs = (struct mxs_i2c_regs *)MXS_I2C0_BASE; uint32_t clk = mxc_get_clock(MXC_XTAL_CLK); uint32_t timing0; @@ -290,10 +286,41 @@ unsigned int i2c_get_bus_speed(void) return clk / ((((timing0 >> 16) - 3) * 2) + 38); } -void i2c_init(int speed, int slaveadd) +int i2c_set_bus_num(unsigned int bus) { + uint32_t mxs_i2c_regs; + + switch (bus) { + case 0: + mxs_i2c_regs = MXS_I2C0_BASE; + break; +#ifdef CONFIG_MX28 + case 1: + mxs_i2c_regs = MXS_I2C1_BASE; + break; +#endif + default: + printf("Bad bus: %d\n", bus); + return -EINVAL; + } + + mxs_i2c_bus = bus; + i2c_regs = (struct mxs_i2c_regs *)mxs_i2c_regs; + mxs_i2c_reset(); - i2c_set_bus_speed(speed); + i2c_set_bus_speed(mxs_i2c_speed); + return 0; +} + +unsigned int i2c_get_bus_num(void) +{ + return mxs_i2c_bus; +} + +void i2c_init(int speed, int slaveadd) +{ + mxs_i2c_speed = speed; + i2c_set_bus_num(0); return; } diff --git a/include/configs/mxs.h b/include/configs/mxs.h index eb96fc1..3936029 100644 --- a/include/configs/mxs.h +++ b/include/configs/mxs.h @@ -149,6 +149,7 @@ /* I2C */ #ifdef CONFIG_CMD_I2C #define CONFIG_I2C_MXS +#define CONFIG_I2C_MULTI_BUS #define CONFIG_HARD_I2C #ifndef CONFIG_SYS_I2C_SPEED #define CONFIG_SYS_I2C_SPEED 400000
The i.MX28 has two I2C IP blocks, but the MXS I2C driver is hard-coded to use the I2C block 0 . Add multibus support so we can use both I2C busses as seen fit. Signed-off-by: Marek Vasut <marex@denx.de> Cc: Stefano Babic <sbabic@denx.de> --- drivers/i2c/mxs_i2c.c | 47 +++++++++++++++++++++++++++++++++++++---------- include/configs/mxs.h | 1 + 2 files changed, 38 insertions(+), 10 deletions(-)