[PATCH][I2C] Marvell mv64xxx i2c driver

[Mark A. Greer]

Greg, Philip,

Marvell makes a line of host bridge for PPC and MIPS systems. On those
bridges is an i2c controller. This patch adds the driver for that i2c
controller.

Please let me know if you see any problems with this patch.

Also, if you're not the correct person(s), please point me to who is.

Thanks,

Mark

Signed-off-by: Mark A. Greer <mgr...@mvista.com>
--

[Mark A. Greer]

Greg KH wrote:

>Please put <asm/ after <linux/
>

Done.

>You have a lot of pr_debug and other printk() for stuff in this driver.
>Please use dev_dbg(), dev_err() and friends instead. That way you get a
>consistant message, that points to the exact device that is causing the
>message.
>

Cool. Done.

>You have some big inline functions here. Should they really be inline?
>We aren't really worried about speed here, right? Size is probably a
>bigger issue.
>

No, no, and done.

>
>Is this header file really needed? Does any other file other than this
>single driver ever include it? If not, please just put it into the
>driver itself.
>
>

No, no, and done.

Included is an *incremental* patch that I hope addresses your concerns.

Thanks Greg.

Mark
--

[Greg KH]

On Wed, Jan 26, 2005 at 02:56:55PM -0700, Mark A. Greer wrote:
> +#include <linux/config.h>
> +#include <linux/kernel.h>
> +#include <linux/module.h>
> +#include <linux/sched.h>
> +#include <linux/init.h>
> +#include <linux/pci.h>
> +#include <linux/wait.h>
> +#include <linux/spinlock.h>
> +#include <asm/io.h>
> +#include <asm/ocp.h>
> +#include <linux/i2c.h>
> +#include <linux/interrupt.h>
> +#include <linux/delay.h>
> +#include <linux/mv643xx.h>
> +#include "i2c-mv64xxx.h"

Please put <asm/ after <linux/

> +static inline void
> +mv64xxx_i2c_fsm(struct mv64xxx_i2c_data *drv_data, u32 status)
> +{
> + pr_debug("mv64xxx_i2c_fsm: ENTER--state: %d, status: 0x%x\n",
> + drv_data->state, status);

You have a lot of pr_debug and other printk() for stuff in this driver.
Please use dev_dbg(), dev_err() and friends instead. That way you get a
consistant message, that points to the exact device that is causing the
message.

> +static inline void
> +mv64xxx_i2c_prepare_for_io(struct mv64xxx_i2c_data *drv_data,
> + struct i2c_msg *msg)

You have some big inline functions here. Should they really be inline?
We aren't really worried about speed here, right? Size is probably a
bigger issue.

> diff -Nru a/drivers/i2c/busses/i2c-mv64xxx.h b/drivers/i2c/busses/i2c-mv64xxx.h
> --- /dev/null Wed Dec 31 16:00:00 196900
> +++ b/drivers/i2c/busses/i2c-mv64xxx.h 2005-01-26 14:49:22 -07:00

Is this header file really needed? Does any other file other than this
single driver ever include it? If not, please just put it into the
driver itself.

thanks,

greg k-h
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majo...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/

[Mark A. Greer]

Jean Delvare wrote:

>>+config I2C_MV64XXX
>>+ tristate "Marvell mv64xxx I2C Controller"
>>+ depends on I2C && MV64X60
>>
>>
>
>&& EXPERIMENTAL?
>

Done.

>>+#define I2C_ALGO_MV64XXX 0x170000 /* Marvell mv64xxx i2c ctlr */
>>
>>
>
>0x170000 is reserved within the legacy i2c project for an USB algorithm,
>and 0x180000 for virtual busses. Could you please use 0x190000 instead,
>so as to avoid future collisions?
>

Done.

>
>+#define MV64XXX_I2C_CTLR_NAME "mv64xxx i2c"
>+#define MV64XXX_I2C_OFFSET 0xc000
>+#define MV64XXX_I2C_REG_BLOCK_SIZE 0x0020
>
>
>
>You have a tab instead of space before MV64XXX_I2C_CTLR_NAME, it seems.
>Also, you want to align the numerical values using only tabs, no space.
>
>

Actually, I did use tabs but what was already there used spaces. So the
patch won't mess up the alignment, I changed mine to spaces too.

The attached patch is a complete replacement for the one posted earlier.

[Mark A. Greer]

Jean Delvare wrote:

>Hi Mark,
>

Thanks for the commenting, Jean.

><snip>

>
>
>
>>+config I2C_MV64XXX
>>+ tristate "Marvell mv64xxx I2C Controller"
>>+ depends on I2C && MV64X60
>>
>>
>
>&& EXPERIMENTAL?
>

Yes, I guess that's the correct thing to do. I'll add that.

>>diff -Nru a/include/linux/i2c-id.h b/include/linux/i2c-id.h
>>--- a/include/linux/i2c-id.h 2005-01-25 18:15:24 -07:00
>>+++ b/include/linux/i2c-id.h 2005-01-25 18:15:24 -07:00
>>@@ -200,6 +200,7 @@
>>
>> #define I2C_ALGO_SIBYTE 0x150000 /* Broadcom SiByte SOCs */
>> #define I2C_ALGO_SGI 0x160000 /* SGI algorithm */

>>+#define I2C_ALGO_MV64XXX 0x170000 /* Marvell mv64xxx i2c ctlr */
>>
>>
>
>0x170000 is reserved within the legacy i2c project for an USB algorithm,
>and 0x180000 for virtual busses. Could you please use 0x190000 instead,
>so as to avoid future collisions?
>

Absolutely. I was unaware of the other uses.

>
>
>>-#define MV64340_I2C_SOFT_RESET 0xc01c

>>+#define MV64XXX_I2C_CTLR_NAME "mv64xxx i2c"
>>+#define MV64XXX_I2C_OFFSET 0xc000
>>+#define MV64XXX_I2C_REG_BLOCK_SIZE 0x0020
>>
>>
>
>You have a tab instead of space before MV64XXX_I2C_CTLR_NAME, it seems.
>Also, you want to align the numerical values using only tabs, no space.
>
>

Oops. I'll fix that too and repost later today.

Mark

[Jean Delvare]

Hi Mark,

> Marvell makes a line of host bridge for PPC and MIPS systems. On
> those bridges is an i2c controller. This patch adds the driver for
> that i2c controller.
>
> Please let me know if you see any problems with this patch.

I do not feel qualified for a full review of this code. However, I
noticed the following minor issues:

> +config I2C_MV64XXX
> + tristate "Marvell mv64xxx I2C Controller"
> + depends on I2C && MV64X60

&& EXPERIMENTAL?

> diff -Nru a/include/linux/i2c-id.h b/include/linux/i2c-id.h

> --- a/include/linux/i2c-id.h 2005-01-25 18:15:24 -07:00
> +++ b/include/linux/i2c-id.h 2005-01-25 18:15:24 -07:00
> @@ -200,6 +200,7 @@
>
> #define I2C_ALGO_SIBYTE 0x150000 /* Broadcom SiByte SOCs */
> #define I2C_ALGO_SGI 0x160000 /* SGI algorithm */
> +#define I2C_ALGO_MV64XXX 0x170000 /* Marvell mv64xxx i2c ctlr */

0x170000 is reserved within the legacy i2c project for an USB algorithm,
and 0x180000 for virtual busses. Could you please use 0x190000 instead,
so as to avoid future collisions?

> -#define MV64340_I2C_SLAVE_ADDR 0xc000
> -#define MV64340_I2C_EXTENDED_SLAVE_ADDR 0xc010
> -#define MV64340_I2C_DATA 0xc004
> -#define MV64340_I2C_CONTROL 0xc008
> -#define MV64340_I2C_STATUS_BAUDE_RATE 0xc00C

> -#define MV64340_I2C_SOFT_RESET 0xc01c
> +#define MV64XXX_I2C_CTLR_NAME "mv64xxx i2c"
> +#define MV64XXX_I2C_OFFSET 0xc000
> +#define MV64XXX_I2C_REG_BLOCK_SIZE 0x0020

You have a tab instead of space before MV64XXX_I2C_CTLR_NAME, it seems.
Also, you want to align the numerical values using only tabs, no space.

Thanks,
--
Jean Delvare
http://khali.linux-fr.org/

[Greg KH]

On Tue, Jan 25, 2005 at 06:26:45PM -0700, Mark A. Greer wrote:
> +static inline void
> +mv64xxx_i2c_fsm(struct mv64xxx_i2c_data *drv_data, u32 status)

This is a much too big of a function to be "inline". Please change it.
Same for your other inline functions, that's not really needed, right?

> +{
> + pr_debug("mv64xxx_i2c_fsm: ENTER--state: %d, status: 0x%x\n",
> + drv_data->state, status);

Please use the dev_* calls instead. It gives you an accurate
description of the specific device that emits the messages. Also use it
for all of the printk() calls in the driver too.

thanks,

greg k-h

[Mark A. Greer]

Greg KH wrote:

>On Tue, Jan 25, 2005 at 06:26:45PM -0700, Mark A. Greer wrote:
>
>
>>+static inline void
>>+mv64xxx_i2c_fsm(struct mv64xxx_i2c_data *drv_data, u32 status)
>>
>>
>
>This is a much too big of a function to be "inline". Please change it.
>Same for your other inline functions, that's not really needed, right?
>
>
>
>>+{
>>+ pr_debug("mv64xxx_i2c_fsm: ENTER--state: %d, status: 0x%x\n",
>>+ drv_data->state, status);
>>
>>
>
>Please use the dev_* calls instead. It gives you an accurate
>description of the specific device that emits the messages. Also use it
>for all of the printk() calls in the driver too.
>
>thanks,
>
>greg k-h
>

Certainly. I already posted this [incremental] patch based on your
previous comments,
http://www.ussg.iu.edu/hypermail/linux/kernel/0501.3/0941.html. Is that
better?

Mark

[Greg KH]

On Mon, Jan 31, 2005 at 11:41:28AM -0700, Mark A. Greer wrote:
> Greg KH wrote:
>
> >On Tue, Jan 25, 2005 at 06:26:45PM -0700, Mark A. Greer wrote:
> >
> >
> >>+static inline void
> >>+mv64xxx_i2c_fsm(struct mv64xxx_i2c_data *drv_data, u32 status)
> >>
> >>
> >
> >This is a much too big of a function to be "inline". Please change it.
> >Same for your other inline functions, that's not really needed, right?
> >
> >
> >
> >>+{
> >>+ pr_debug("mv64xxx_i2c_fsm: ENTER--state: %d, status: 0x%x\n",
> >>+ drv_data->state, status);
> >>
> >>
> >
> >Please use the dev_* calls instead. It gives you an accurate
> >description of the specific device that emits the messages. Also use it
> >for all of the printk() calls in the driver too.
> >
> >thanks,
> >
> >greg k-h
> >
>
> Certainly. I already posted this [incremental] patch based on your
> previous comments,
> http://www.ussg.iu.edu/hypermail/linux/kernel/0501.3/0941.html. Is that
> better?

How about a whole new patch that I could apply? That would be better :)

thanks,

greg k-h

[Mark A. Greer]

Greg KH wrote:

>How about a whole new patch that I could apply? That would be better :)
>
>

Oops, sorry. :)

[Greg KH]

On Wed, Feb 02, 2005 at 03:15:14AM +0200, Alexey Dobriyan wrote:

> P. S.: struct mv64xxx_i2c_data revisited...
>
> > + uint state;
> > + ulong reg_base_p;
>
> Silly request, but... Maybe this should be changed to plain old "unsigned int"
> and "unsigned long". Please. I just don't understand why people use "uint",
> "u_int", "uInt", "UINT", "uINT", "uint_t" which are always typedef'ed to
> "unsigned int".

Not a silly request at all. Please use the u32, u64 and so on values
instead. That way we know what you mean, and it's portable.

[Mark A. Greer]

Alexey Dobriyan wrote:

>On Tue, 01 Feb 2005 10:54:32 -0700, Mark A. Greer wrote:
>
>
>
>>+struct mv64xxx_i2c_data {
>>+ void *reg_base;
>>
>>
>
>ioremap() returns "void __iomem *".
>

Okay.

>
>
>>+static void __devexit
>>+mv64xxx_i2c_unmap_regs(struct mv64xxx_i2c_data *drv_data)
>>+{
>>+ if (!drv_data->reg_base) {
>>+ iounmap(drv_data->reg_base);
>>
>>
>
>A typo? You're unmapping known to be invalid address.
>

Yes, a typo. Good catch.

>
>
>>+ drv_data->reg_base = 0;
>>
>>
>
>Use NULL because drv_data->reg_base is a pointer.
>

Done.

>
>
>>+ if ((pd->id == 0) && pdata) {
>>
>>
>
>Rewrite this as:
>
> if ((pd->id != 0) || !pdata)
> return -ENODEV;
>
>and save one level of indentation below.
>

Done.

>
>
> <snip of xxx_probe modifications>

Done.

>
> Alexey

>
>P. S.: struct mv64xxx_i2c_data revisited...
>
>
>
>>+ uint state;
>>+ ulong reg_base_p;
>>
>>
>
>Silly request, but... Maybe this should be changed to plain old "unsigned int"
>and "unsigned long". Please. I just don't understand why people use "uint",
>"u_int", "uInt", "UINT", "uINT", "uint_t" which are always typedef'ed to
>"unsigned int".
>
>
>

Thanks Alexey & Greg. How's this (a complete replacement for previous
patch)?

[Alexey Dobriyan]

On Thursday 03 February 2005 21:12, Mark A. Greer wrote:

> > >+ mv64xxx_i2c_fsm(drv_data, status);
> >
> >It can set drv_data->rc to -ENODEV or -EIO. In both cases ->action goes to
> >MV64XXX_I2C_ACTION_SEND_STOP and mv64xxx_i2c_do_action() will writel()
> >something. Is it correct to _not_ check ->rc here?
>
> I think so. It still needs to go into do_action even when rc != 0 (in
> which case it'll do a STOP condition).

Ok. Thanks for the explanation. Agree, ->rc should be left as is.

> This patch is a replacement patch that should address your concerns
> except maybe the mv64xxx_i2c_data.rc one.

> --- a/include/linux/i2c-id.h
> +++ b/include/linux/i2c-id.h

> + /* 0x170000 - USB */
> + /* 0x180000 - Virtual buses */
> +#define I2C_ALGO_MV64XXX 0x190000 /* Marvell mv64xxx i2c ctlr */

While I searched for typos and you're fixing them, au1550 owned 0x170000.
2.6.11-rc3 says:

#define I2C_ALGO_AU1550 0x170000 /* Au1550 PSC algorithm */

So, I'd remove first two comments.

Oh, and the last note: current sparse and gcc 4 don't produce any warnings.

Alexey

[Mark A. Greer]

Alexey Dobriyan wrote:

>On Thursday 03 February 2005 21:12, Mark A. Greer wrote:
>
>
>
>>>>+ mv64xxx_i2c_fsm(drv_data, status);
>>>>
>>>>
>>>It can set drv_data->rc to -ENODEV or -EIO. In both cases ->action goes to
>>>MV64XXX_I2C_ACTION_SEND_STOP and mv64xxx_i2c_do_action() will writel()
>>>something. Is it correct to _not_ check ->rc here?
>>>
>>>
>>I think so. It still needs to go into do_action even when rc != 0 (in
>>which case it'll do a STOP condition).
>>
>>
>
>Ok. Thanks for the explanation. Agree, ->rc should be left as is.
>

Okay.

>
>
>>--- a/include/linux/i2c-id.h
>>+++ b/include/linux/i2c-id.h
>>
>>
>
>
>
>>+ /* 0x170000 - USB */
>>+ /* 0x180000 - Virtual buses */
>>+#define I2C_ALGO_MV64XXX 0x190000 /* Marvell mv64xxx i2c ctlr */
>>
>>
>
>While I searched for typos and you're fixing them, au1550 owned 0x170000.
>2.6.11-rc3 says:
>
> #define I2C_ALGO_AU1550 0x170000 /* Au1550 PSC algorithm */
>
>So, I'd remove first two comments.
>

I added the comments b/c of this email from Jean Delvare,
http://www.uwsg.iu.edu/hypermail/linux/kernel/0501.3/0977.html. The
relevant part being:

"0x170000 is reserved within the legacy i2c project for an USB algorithm,
and 0x180000 for virtual busses. Could you please use 0x190000 instead,
so as to avoid future collisions?"

It looks like I2C_ALGO_AU1550 was just added so my guess is Jean is
correct and I2C_ALGO_AU1550 should be made 0x1a0000 (or I move mine back
one). Would someone confirm that 0x170000 is used by legacy i2c stuffs?
I don't really know where to look. If so, I can easily make a patch
moving it back.

>Oh, and the last note: current sparse and gcc 4 don't produce any warnings.
>
>

Cool!

Mark

[Jean Delvare]

Hi Mark, Alexey,

> > >+ /* 0x170000 - USB */
> > >+ /* 0x180000 - Virtual buses */
> > >+#define I2C_ALGO_MV64XXX 0x190000 /* Marvell mv64xxx i2c ctlr */
> >
> > While I searched for typos and you're fixing them, au1550 owned 0x170000.
> > 2.6.11-rc3 says:
> >
> > #define I2C_ALGO_AU1550 0x170000 /* Au1550 PSC algorithm */
> >
> > So, I'd remove first two comments.
>
> I added the comments b/c of this email from Jean Delvare,
> http://www.uwsg.iu.edu/hypermail/linux/kernel/0501.3/0977.html. The
> relevant part being:
>
> "0x170000 is reserved within the legacy i2c project for an USB algorithm,
> and 0x180000 for virtual busses. Could you please use 0x190000 instead,
> so as to avoid future collisions?"
>
> It looks like I2C_ALGO_AU1550 was just added so my guess is Jean is
> correct and I2C_ALGO_AU1550 should be made 0x1a0000 (or I move mine back
> one). Would someone confirm that 0x170000 is used by legacy i2c stuffs?
> I don't really know where to look. If so, I can easily make a patch
> moving it back.

I am as surprised as you are to see this here. I2C_ALGO_AU1550 should
really be made a different value. There is also a problem with
I2C_ALGO_PCA and I2C_ALGO_SIBYTE having the same value, which was
already reported to Greg some days ago if memory serves. I think I will
send a patch against 2.6.10-rc3 to Linus this evening, which fixes the
broken algo IDs. That way Mark can keep the algorithm ID he is using
right now, and each algorithm will get its own, unique ID, as should be.

You can see the i2c-id.h list from the legacy i2c project here:
http://www2.lm-sensors.nu/~lm78/cvs/i2c/kernel/i2c-id.h
I am indeed trying to keep it in sync with the one in Linux 2.6 (and with
the one in Linux 2.4 at times), in the hope it'll avoid confusion and
increase portability.

Thanks,
--
Jean Delvare

[Jean Delvare]

Hi all,

Quoting myself:
> I am as surprised as you are to see this here. I2C_ALGO_AU1550 should
> really be made a different value. There is also a problem with
> I2C_ALGO_PCA and I2C_ALGO_SIBYTE having the same value, which was
> already reported to Greg some days ago if memory serves. I think I
> will send a patch against 2.6.10-rc3 to Linus this evening, which
> fixes the broken algo IDs. That way Mark can keep the algorithm ID he
> is using right now, and each algorithm will get its own, unique ID, as
> should be.

I've changed my mind since. There isn't anything critical here and
pushing random patches to Linus right before he releases 2.6.11 wouldn't
have been particularly subtle. So I instead sent a patch to Greg, as can
be seen here:
http://archives.andrew.net.au/lm-sensors/msg29405.html

So Mark, providing Greg accepts this patch, you can assume that
I2C_ALGO_MV64XXX is already properly defined in Greg's tree (where your
own patch will end up) so you don't need to add it yourself. You will
still have to define I2C_HW_MV64XXX though.