[U-Boot] [PATCH] i2c: mxs: Add I2C multibus support

[U-Boot] [PATCH] i2c: mxs: Add I2C multibus support

[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(-)

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
-- 
2.1.0

[U-Boot] [PATCH] i2c: mxs: Add I2C multibus support

[Heiko Schocher]

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

-- 
DENX Software Engineering GmbH,     MD: Wolfgang Denk & Detlev Zundel
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany

[U-Boot] [PATCH] i2c: mxs: Add I2C multibus support

[Otavio Salvador]

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.

-- 
Otavio Salvador                             O.S. Systems
http://www.ossystems.com.br        http://code.ossystems.com.br
Mobile: +55 (53) 9981-7854            Mobile: +1 (347) 903-9750

[U-Boot] [PATCH] i2c: mxs: Add I2C multibus support

[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
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

[U-Boot] [PATCH] i2c: mxs: Add I2C multibus support

[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.

[U-Boot] [PATCH] i2c: mxs: Add I2C multibus support

[Heiko Schocher]

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
-- 
DENX Software Engineering GmbH,     MD: Wolfgang Denk & Detlev Zundel
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany

[U-Boot] [PATCH] i2c: mxs: Add I2C multibus support

[Heiko Schocher]

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
-- 
DENX Software Engineering GmbH,     MD: Wolfgang Denk & Detlev Zundel
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany