On Wed, Feb 09, 2005 at 02:33:59PM -0700, Mark A. Greer wrote:
>
> I can't find any definitive policy on this. I kind of like the explicit
> return, I don't know why. I've had others make the same comment,
> though, so I'll remove them since it obviously bothers people.
>
> Attached is a
On Wed, Feb 09, 2005 at 02:33:59PM -0700, Mark A. Greer wrote:
I can't find any definitive policy on this. I kind of like the explicit
return, I don't know why. I've had others make the same comment,
though, so I'll remove them since it obviously bothers people.
Attached is a
Bartlomiej Zolnierkiewicz wrote:
Thanks, I see that you did global replacement of __devinit
by __init and __devexit by __exit - it seems correct *only* if:
- there can be only one i2c controller in the system
- there can be only one host bridge in the system
- i2c core calls ->probe only once
Bartlomiej Zolnierkiewicz wrote:
Thanks, I see that you did global replacement of __devinit
by __init and __devexit by __exit - it seems correct *only* if:
- there can be only one i2c controller in the system
- there can be only one host bridge in the system
- i2c core calls -probe only once
On Tue, 08 Feb 2005 17:32:11 -0700, Mark A. Greer <[EMAIL PROTECTED]> wrote:
> Bartlomiej Zolnierkiewicz wrote:
>
> >Hi,
> >
> >just a minor thing
> >
> >
> >
> >>+static int __devinit
> >>+mv64xxx_i2c_init(void)
> >>+{
> >>+ return driver_register(_i2c_driver);
> >>+}
> >>
> >>
> >
>
Bartlomiej Zolnierkiewicz wrote:
Hi,
just a minor thing
+static int __devinit
+mv64xxx_i2c_init(void)
+{
+ return driver_register(_i2c_driver);
+}
__init
+static void __devexit
+mv64xxx_i2c_exit(void)
+{
+ driver_unregister(_i2c_driver);
+ return;
+}
__exit
these
Hi,
just a minor thing
> +static int __devinit
> +mv64xxx_i2c_init(void)
> +{
> + return driver_register(_i2c_driver);
> +}
__init
> +static void __devexit
> +mv64xxx_i2c_exit(void)
> +{
> + driver_unregister(_i2c_driver);
> + return;
> +}
__exit
these functions relate to
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 apply.
Depends on patch submitted by Jean Delvare:
http://archives.andrew.net.au/lm-sensors/msg29405.html
Signed-off-by: Mark A.
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 apply.
Depends on patch submitted by Jean Delvare:
http://archives.andrew.net.au/lm-sensors/msg29405.html
Signed-off-by: Mark A.
Hi,
just a minor thing
+static int __devinit
+mv64xxx_i2c_init(void)
+{
+ return driver_register(mv64xxx_i2c_driver);
+}
__init
+static void __devexit
+mv64xxx_i2c_exit(void)
+{
+ driver_unregister(mv64xxx_i2c_driver);
+ return;
+}
__exit
these functions relate
Bartlomiej Zolnierkiewicz wrote:
Hi,
just a minor thing
+static int __devinit
+mv64xxx_i2c_init(void)
+{
+ return driver_register(mv64xxx_i2c_driver);
+}
__init
+static void __devexit
+mv64xxx_i2c_exit(void)
+{
+ driver_unregister(mv64xxx_i2c_driver);
+ return;
+}
On Tue, 08 Feb 2005 17:32:11 -0700, Mark A. Greer [EMAIL PROTECTED] wrote:
Bartlomiej Zolnierkiewicz wrote:
Hi,
just a minor thing
+static int __devinit
+mv64xxx_i2c_init(void)
+{
+ return driver_register(mv64xxx_i2c_driver);
+}
__init
+static void __devexit
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
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
Hi Mark, Alexey,
> > >+ /* 0x17 - USB */
> > >+ /* 0x18 - Virtual buses */
> > >+#define I2C_ALGO_MV64XXX 0x19 /* Marvell mv64xxx i2c ctlr
> > >*/
> >
> > While I searched for typos and
Hi Mark, Alexey,
+ /* 0x17 - USB */
+ /* 0x18 - Virtual buses */
+#define I2C_ALGO_MV64XXX 0x19 /* Marvell mv64xxx i2c ctlr
*/
While I searched for typos and you're fixing
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
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
Hi, Alexey.
--
Alexey Dobriyan wrote:
On Wednesday 02 February 2005 19:26, Mark A. Greer wrote:
How's this (a complete replacement for previous patch)?
--- a/drivers/i2c/busses/Kconfig
+++ b/drivers/i2c/busses/Kconfig
+ If you say yes to this option, support will be included
Hi, Alexey.
--
Alexey Dobriyan wrote:
On Wednesday 02 February 2005 19:26, Mark A. Greer wrote:
How's this (a complete replacement for previous patch)?
--- a/drivers/i2c/busses/Kconfig
+++ b/drivers/i2c/busses/Kconfig
+ If you say yes to this option, support will be included
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
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
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) {
+
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) {
+
On Wed, Feb 02, 2005 at 03:15:14AM +0200, Alexey Dobriyan wrote:
> P. S.: struct mv64xxx_i2c_data revisited...
>
> > + uintstate;
> > + ulong reg_base_p;
>
> Silly request, but... Maybe this should be changed to plain old "unsigned int"
> and "unsigned
Greg KH wrote:
How about a whole new patch that I could apply? That would be better :)
Oops, sorry. :)
diff -Nru a/drivers/i2c/busses/Kconfig b/drivers/i2c/busses/Kconfig
--- a/drivers/i2c/busses/Kconfig2005-02-01 10:45:00 -07:00
+++ b/drivers/i2c/busses/Kconfig2005-02-01
Greg KH wrote:
How about a whole new patch that I could apply? That would be better :)
Oops, sorry. :)
diff -Nru a/drivers/i2c/busses/Kconfig b/drivers/i2c/busses/Kconfig
--- a/drivers/i2c/busses/Kconfig2005-02-01 10:45:00 -07:00
+++ b/drivers/i2c/busses/Kconfig2005-02-01
On Wed, Feb 02, 2005 at 03:15:14AM +0200, Alexey Dobriyan wrote:
P. S.: struct mv64xxx_i2c_data revisited...
+ uintstate;
+ ulong reg_base_p;
Silly request, but... Maybe this should be changed to plain old unsigned int
and unsigned long.
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
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
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?
>
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?
+{
+
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
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.
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.
Jean Delvare wrote:
Hi Mark,
Thanks for the commenting, Jean.
+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
Jean Delvare wrote:
+config I2C_MV64XXX
+ tristate "Marvell mv64xxx I2C Controller"
+ depends on I2C && MV64X60
&& EXPERIMENTAL?
Done.
+#define I2C_ALGO_MV64XXX 0x17 /* Marvell mv64xxx i2c ctlr */
0x17 is reserved within the legacy i2c project for an USB algorithm,
and
On Wed, Jan 26, 2005 at 02:56:55PM -0700, Mark A. Greer wrote:
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include "i2c-mv64xxx.h"
Please put +static
Greg KH wrote:
Please put
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
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
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
Jean Delvare wrote:
+config I2C_MV64XXX
+ tristate Marvell mv64xxx I2C Controller
+ depends on I2C MV64X60
EXPERIMENTAL?
Done.
+#define I2C_ALGO_MV64XXX 0x17 /* Marvell mv64xxx i2c ctlr */
0x17 is reserved within the legacy i2c project for an USB algorithm,
and
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
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,
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
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
46 matches
Mail list logo
