Hi Sean,
On Fri, 15 Feb 2008 23:11:12 -0500, Sean MacLennan wrote:
Here is the of platform patch. I removed the retries and removed the spaces used for spacing.
Cheers,
Sean
Signed-off-by: Sean MacLennan <redacted>
First of all: please run scripts/checkpatch.pl on your patch and fix
the reported errors. It tells me:
total: 10 errors, 5 warnings, 222 lines checked
which is definitely too much.
Review:
quoted hunk
---
--- old-i2c-ibm_iic.c 2008-02-15 23:01:58.000000000 -0500
+++ i2c-ibm_iic.c 2008-02-15 23:00:44.000000000 -0500
Please send a proper -p1 patch next time.
quoted hunk
@@ -6,6 +6,9 @@
* Copyright (c) 2003, 2004 Zultys Technologies.
* Eugene Surovegin <eugene.surovegin@zultys.com> or <ebs@ebshome.net>
*
+ * Copyright (c) 2008 PIKA Technologies
+ * Sean MacLennan <smaclennan@pikatech.com>
+ *
* Based on original work by
* Ian DaSilva <idasilva@mvista.com>
* Armin Kuster <akuster@mvista.com>
@@ -39,12 +42,17 @@
#include <asm/io.h>
#include <linux/i2c.h>
#include <linux/i2c-id.h>
+
+#ifdef CONFIG_IBM_OCP
Your patch seems to be incomplete. There is still
config I2C_IBM_IIC
tristate "IBM PPC 4xx on-chip I2C interface"
depends on IBM_OCP
in drivers/i2c/busses/Kconfig, so the new code can never be active.
quoted hunk
#include <asm/ocp.h>
#include <asm/ibm4xx.h>
+#else
+#include <linux/of_platform.h>
+#endif
#include "i2c-ibm_iic.h"
-#define DRIVER_VERSION "2.1"
+#define DRIVER_VERSION "2.2"
MODULE_DESCRIPTION("IBM IIC driver v" DRIVER_VERSION);
MODULE_LICENSE("GPL");@@ -657,6 +665,7 @@
return (u8)((opb + 9) / 10 - 1);
}
+#ifdef CONFIG_IBM_OCP
/*
* Register single IIC interface
*/
@@ -830,3 +839,188 @@
module_init(iic_init);
module_exit(iic_exit);
+#else
Please add a comment saying what this #else corresponds to.
+/*
+ * Register single IIC interface
+ */
+static int __devinit iic_probe(struct of_device *ofdev,
+ const struct of_device_id *match)
+{
+ static int index = 0;
+ struct device_node *np = ofdev->node;
+ struct ibm_iic_private* dev;
Confusing variable name.
+ struct i2c_adapter* adap;
+ const u32 *indexp, *freq;
+ int ret;
+
+ dev = kzalloc(sizeof(*dev), GFP_KERNEL);
+ if (!dev) {
+ printk(KERN_ERR "ibm-iic: failed to allocate device data\n");
Please use dev_err. Same for all other messages below.
+ return -ENOMEM;
+ }
+
+ /* This assumes we don't mix index and non-index entries. */
+ indexp = of_get_property(np, "index", NULL);
+ dev->idx = indexp ? *indexp : index++;
I don't like this static index thing much. Can't you just make the
"index" OF property mandatory? Mixing ways to number things can become
very confusing. In particular as you are using dev->idx later to call
i2c_add_numbered_adapter(), the caller is really supposed to know what
they are doing with the bus numbers.
+
+ dev_set_drvdata(&ofdev->dev, dev);
+
+ dev->vaddr = of_iomap(np, 0);
+ if (dev->vaddr == NULL) {
+ printk(KERN_ERR "ibm-iic%d: failed to ioremap device registers\n",
+ dev->idx);
+ ret = -ENXIO;
+ goto fail1;
+ }
+
+ init_waitqueue_head(&dev->wq);
+
+ if (iic_force_poll)
+ dev->irq = NO_IRQ;
+ else {
+ dev->irq = irq_of_parse_and_map(np, 0);
+ if (dev->irq == NO_IRQ)
+ printk(KERN_ERR __FILE__ ": irq_of_parse_and_map failed\n");
+ else {
+ /* Disable interrupts until we finish initialization,
+ assumes level-sensitive IRQ setup...
+ */
+ iic_interrupt_mode(dev, 0);
+ if (request_irq(dev->irq, iic_handler, 0, "IBM IIC", dev)){
+ printk(KERN_ERR "ibm-iic%d: request_irq %d failed\n",
+ dev->idx, dev->irq);
+ /* Fallback to the polling mode */
+ dev->irq = NO_IRQ;
+ }
+ }
+ }
+
+ if (dev->irq == NO_IRQ)
+ printk(KERN_WARNING "ibm-iic%d: using polling mode\n",
+ dev->idx);
+
+ /* Board specific settings */
+ if (iic_force_fast || of_get_property(np, "fast-mode", NULL))
+ dev->fast_mode = 1;
+ else
+ dev->fast_mode = 0;
The second part is not needed, 0 is the default thanks to kzalloc.
+
+ /* clckdiv is the same for *all* IIC interfaces, but I'd rather
+ * make a copy than introduce another global. --ebs
+ */
+ freq = of_get_property(np, "clock-frequency", NULL);
+ if (freq == NULL) {
+ freq = of_get_property(np->parent, "clock-frequency", NULL);
+ if (freq == NULL) {
+ printk(KERN_ERR "ibm-iic%d: Unable to get bus frequency\n",
+ dev->idx);
+ ret = -EBUSY;
+ goto fail;
+ }
+ }
+
+ dev->clckdiv = iic_clckdiv(*freq);
+ DBG("%d: clckdiv = %d\n", dev->idx, dev->clckdiv);
+
+ /* Initialize IIC interface */
+ iic_dev_init(dev);
+
+ /* Register it with i2c layer */
+ adap = &dev->adap;
+ adap->dev.parent = &ofdev->dev;
+ strcpy(adap->name, "IBM IIC");
strlcpy please.
+ i2c_set_adapdata(adap, dev);
+ adap->id = I2C_HW_OCP;
+ adap->class = I2C_CLASS_HWMON;
+ adap->algo = &iic_algo;
+ adap->client_register = NULL;
+ adap->client_unregister = NULL;
The last two statements are not needed, again kzalloc did it for you.
+ adap->timeout = 1;
+ adap->nr = dev->idx;
Looks to me like the block above has much in common with the original
probe function, maybe it would be worth sharing the code to make future
maintenance easier?
+
+ ret = i2c_add_numbered_adapter(adap);
+ if (ret < 0) {
+ printk(KERN_ERR "ibm-iic%d: failed to register i2c adapter\n",
+ dev->idx);
+ goto fail;
+ }
+
+ printk(KERN_INFO "ibm-iic%d: using %s mode\n", dev->idx,
+ dev->fast_mode ? "fast (400 kHz)" : "standard (100 kHz)");
+
+ return 0;
+
+fail:
+ if (dev->irq != NO_IRQ){
+ iic_interrupt_mode(dev, 0);
+ free_irq(dev->irq, dev);
+ }
+
+ iounmap(dev->vaddr);
+fail1:
I suggest giving explicit names to your labels based on the next
action, e.g. err_free_irq and err_kfree.
+ dev_set_drvdata(&ofdev->dev, NULL);
+ kfree(dev);
+ return ret;
+}
+
+/*
+ * Cleanup initialized IIC interface
+ */
+static int __devexit iic_remove(struct of_device *ofdev)
+{
+ struct ibm_iic_private* dev = dev_get_drvdata(&ofdev->dev);
+
+ BUG_ON(dev == NULL);
How could this happen at all?
+ if (i2c_del_adapter(&dev->adap)){
i2c_del_adapter can't really fail and almost all other drivers don't
even bother checking the error code. But if you do, you should really
return the error code.
+ printk(KERN_ERR "ibm-iic%d: failed to delete i2c adapter :(\n",
+ dev->idx);
+ /* That's *very* bad, just shutdown IRQ ... */
+ if (dev->irq != NO_IRQ){
+ iic_interrupt_mode(dev, 0);
+ free_irq(dev->irq, dev);
+ dev->irq = NO_IRQ;
Bad indentation.
+ }
+ } else {
+ if (dev->irq != NO_IRQ){
+ iic_interrupt_mode(dev, 0);
+ free_irq(dev->irq, dev);
Bad indentation.
+ }
+ iounmap(dev->vaddr);
May I suggest adding:
dev_set_drvdata(&ofdev->dev, NULL);
+ kfree(dev);
+ }
+
+ return 0;
+}
+
+
+static const struct of_device_id ibm_iic_match[] =
+{
+ { .compatible = "ibm,iic-405ex", },
+ { .compatible = "ibm,iic-405gp", },
+ { .compatible = "ibm,iic-440gp", },
+ { .compatible = "ibm,iic-440gpx", },
+ { .compatible = "ibm,iic-440grx", },
+ {}
+};
+
+static struct of_platform_driver ibm_iic_driver =
+{
+ .name = "ibm-iic",
+ .match_table = ibm_iic_match,
+ .probe = iic_probe,
+ .remove = iic_remove,
Missing __devexit_p.
+};
+
+static int __init ibm_iic_init(void)
+{
+ printk(KERN_INFO "IBM IIC driver v" DRIVER_VERSION "\n");
+ return of_register_platform_driver(&ibm_iic_driver);
+}
+module_init(ibm_iic_init);
+
+static void __exit ibm_iic_exit(void)
+{
+ of_unregister_platform_driver(&ibm_iic_driver);
+}
+module_exit(ibm_iic_exit);
If you would name these functions iic_init and iic_exit as the original
code does, you wouldn't have to duplicate the module_init and
module_exit statements.
+#endif
Please add a comment saying what this #endif corresponds to.
Note that I cannot test the code so I am relying on the linuxppc-dev
folks to test it.
--
Jean Delvare
Jean Delvare wrote:
Hi Sean,
On Fri, 15 Feb 2008 23:11:12 -0500, Sean MacLennan wrote:
quoted
Here is the of platform patch. I removed the retries and removed the spaces used for spacing.
Cheers,
Sean
Signed-off-by: Sean MacLennan <redacted>
First of all: please run scripts/checkpatch.pl on your patch and fix
the reported errors. It tells me:
total: 10 errors, 5 warnings, 222 lines checked
which is definitely too much.
Many of the errors/warnings are from the cut and paste of the original
code. I realize this doesn't justify my new code, but does beg the
question; should I also cleanup the original code? And how much? I am
tempted to cleanup all but the #ifdef CONFIG_IBM_OCP part.
A few comments, everything else I agree with.
quoted
+ return -ENOMEM;
+ }
+
+ /* This assumes we don't mix index and non-index entries. */
+ indexp = of_get_property(np, "index", NULL);
+ dev->idx = indexp ? *indexp : index++;
I don't like this static index thing much. Can't you just make the
"index" OF property mandatory? Mixing ways to number things can become
very confusing. In particular as you are using dev->idx later to call
i2c_add_numbered_adapter(), the caller is really supposed to know what
they are doing with the bus numbers.
I also don't really like mixing the two methods, hence the comment. I
did this so existing dts files, which don't currently have an index,
would work as is. Maybe it would be better to add the index?
Without my patch, or one like it, you cannot currently use the IBM IIC
in the arch/powerpc branch, so maybe now *is* the time to enforce an index?
So if nobody responds with a good reason not to, I will change the code
to require the index to be set. Less testing for me ;)
quoted
+ adap->timeout = 1;
+ adap->nr = dev->idx;
Looks to me like the block above has much in common with the original
probe function, maybe it would be worth sharing the code to make future
maintenance easier?
It is my understanding the the arch/ppc branch is going away in the
middle of the year. So I kept the of platform code separate and clean
expecting the CONFIG_IBM_OCP block to be removed in the near future. So
future maintenance should not be an issue.
Note that I cannot test the code so I am relying on the linuxppc-dev
folks to test it
I can test the of platform code and the common code. I am not setup to
test the OCP code, which is why I am loath to change it. I use the IBM
IIC for access to an ad7414 thermal chip (currently not in the kernel,
I am working on that too) and for reading an eeprom.
Cheers,
Sean
Hi Sean,
On Sat, 16 Feb 2008 15:54:14 -0500, Sean MacLennan wrote:
Jean Delvare wrote:
quoted
Hi Sean,
On Fri, 15 Feb 2008 23:11:12 -0500, Sean MacLennan wrote:
quoted
Here is the of platform patch. I removed the retries and removed the spaces used for spacing.
Cheers,
Sean
Signed-off-by: Sean MacLennan <redacted>
First of all: please run scripts/checkpatch.pl on your patch and fix
the reported errors. It tells me:
total: 10 errors, 5 warnings, 222 lines checked
which is definitely too much.
Many of the errors/warnings are from the cut and paste of the original
code. I realize this doesn't justify my new code, but does beg the
question; should I also cleanup the original code? And how much? I am
tempted to cleanup all but the #ifdef CONFIG_IBM_OCP part.
Just take care of the code you are adding. The old code will go away
soon anyway as I understand it, so don't bother cleaning it up.
A few comments, everything else I agree with.
quoted
quoted
+ return -ENOMEM;
+ }
+
+ /* This assumes we don't mix index and non-index entries. */
+ indexp = of_get_property(np, "index", NULL);
+ dev->idx = indexp ? *indexp : index++;
I don't like this static index thing much. Can't you just make the
"index" OF property mandatory? Mixing ways to number things can become
very confusing. In particular as you are using dev->idx later to call
i2c_add_numbered_adapter(), the caller is really supposed to know what
they are doing with the bus numbers.
I also don't really like mixing the two methods, hence the comment. I
did this so existing dts files, which don't currently have an index,
would work as is. Maybe it would be better to add the index?
Without my patch, or one like it, you cannot currently use the IBM IIC
in the arch/powerpc branch, so maybe now *is* the time to enforce an index?
So if nobody responds with a good reason not to, I will change the code
to require the index to be set. Less testing for me ;)
Yes, I think this is the right time to enforce the index.
quoted
quoted
+ adap->timeout = 1;
+ adap->nr = dev->idx;
Looks to me like the block above has much in common with the original
probe function, maybe it would be worth sharing the code to make future
maintenance easier?
It is my understanding the the arch/ppc branch is going away in the
middle of the year. So I kept the of platform code separate and clean
expecting the CONFIG_IBM_OCP block to be removed in the near future. So
future maintenance should not be an issue.
OK, fine with me then.
quoted
Note that I cannot test the code so I am relying on the linuxppc-dev
folks to test it
I can test the of platform code and the common code. I am not setup to
test the OCP code, which is why I am loath to change it. I use the IBM
IIC for access to an ad7414 thermal chip (currently not in the kernel,
I am working on that too) and for reading an eeprom.
Note that Frank Edelhaeuser has submitted a driver for this chip
recently:
http://lists.lm-sensors.org/pipermail/lm-sensors/2008-February/022478.html
I didn't have time to review this patch yet, maybe you will want to do
it yourself.
--
Jean Delvare
Hi Sean,
On Mon, 18 Feb 2008 20:42:46 -0500, Sean MacLennan wrote:
quoted hunk
An updated version of the patch. This one should answer all of Jean's
concerns.
Cheers,
Sean
Signed-off-by: Sean MacLennan <redacted>
---
--- for-2.6.25/drivers/i2c/busses/orig-i2c-ibm_iic.c 2008-02-18 16:36:30.000000000 -0500
+++ for-2.6.25/drivers/i2c/busses/i2c-ibm_iic.c 2008-02-18 17:39:53.000000000 -0500
@@ -6,6 +6,9 @@
* Copyright (c) 2003, 2004 Zultys Technologies.
* Eugene Surovegin <eugene.surovegin@zultys.com> or <ebs@ebshome.net>
*
+ * Copyright (c) 2008 PIKA Technologies
+ * Sean MacLennan <smaclennan@pikatech.com>
+ *
* Based on original work by
* Ian DaSilva <idasilva@mvista.com>
* Armin Kuster <akuster@mvista.com>
@@ -39,12 +42,17 @@
#include <asm/io.h>
#include <linux/i2c.h>
#include <linux/i2c-id.h>
+
+#ifdef CONFIG_IBM_OCP
#include <asm/ocp.h>
#include <asm/ibm4xx.h>
+#else
+#include <linux/of_platform.h>
+#endif
#include "i2c-ibm_iic.h"
-#define DRIVER_VERSION "2.1"
+#define DRIVER_VERSION "2.2"
MODULE_DESCRIPTION("IBM IIC driver v" DRIVER_VERSION);
MODULE_LICENSE("GPL");@@ -657,6 +665,7 @@
return (u8)((opb + 9) / 10 - 1);
}
+#ifdef CONFIG_IBM_OCP
/*
* Register single IIC interface
*/
@@ -828,5 +837,182 @@
ocp_unregister_driver(&ibm_iic_driver);
}
+#else /* !CONFIG_IBM_OCP */
+
+static int __devinit iic_request_irq(struct of_device *ofdev,
+ struct ibm_iic_private *dev)
+{
+ struct device_node *np = ofdev->node;
+ int irq;
+
+ if (iic_force_poll)
+ return NO_IRQ;
+
+ irq = irq_of_parse_and_map(np, 0);
+ if (irq == NO_IRQ) {
+ dev_err(&ofdev->dev, "irq_of_parse_and_map failed\n");
+ return NO_IRQ;
+ }
+
+ /* Disable interrupts until we finish initialization, assumes
+ * level-sensitive IRQ setup...
+ */
+ iic_interrupt_mode(dev, 0);
+ if (request_irq(irq, iic_handler, 0, "IBM IIC", dev)) {
+ dev_err(&ofdev->dev, "request_irq %d failed\n", irq);
+ /* Fallback to the polling mode */
+ return NO_IRQ;
+ }
+
+ return irq;
+}
+
+/*
+ * Register single IIC interface
+ */
+static int __devinit iic_probe(struct of_device *ofdev,
+ const struct of_device_id *match)
+{
+ struct device_node *np = ofdev->node;
+ struct ibm_iic_private *dev;
+ struct i2c_adapter *adap;
+ const u32 *indexp, *freq;
+ int ret;
+
+
Double blank line is prohibited inside a function.
+ dev = kzalloc(sizeof(*dev), GFP_KERNEL);
+ if (!dev) {
+ dev_err(&ofdev->dev, "failed to allocate device data\n");
+ return -ENOMEM;
+ }
+
+ dev_set_drvdata(&ofdev->dev, dev);
+
+ indexp = of_get_property(np, "index", NULL);
+ if (!indexp) {
+ dev_err(&ofdev->dev, "no index specified.\n");
This is the only error message that has a trailing dot. Remove it?
+ ret = -EINVAL;
Isn't it a bit inconsistent to return -EINVAL here...
+ goto error_cleanup;
+ }
+ dev->idx = *indexp;
+
+ dev->vaddr = of_iomap(np, 0);
+ if (dev->vaddr == NULL) {
+ dev_err(&ofdev->dev, "failed to iomap device\n");
+ ret = -ENXIO;
+ goto error_cleanup;
+ }
+
+ init_waitqueue_head(&dev->wq);
+
+ dev->irq = iic_request_irq(ofdev, dev);
+ if (dev->irq == NO_IRQ)
+ dev_warn(&ofdev->dev, "using polling mode\n");
+
+ /* Board specific settings */
+ if (iic_force_fast || of_get_property(np, "fast-mode", NULL))
+ dev->fast_mode = 1;
+
+ freq = of_get_property(np, "clock-frequency", NULL);
+ if (freq == NULL) {
+ freq = of_get_property(np->parent, "clock-frequency", NULL);
+ if (freq == NULL) {
+ dev_err(&ofdev->dev, "Unable to get bus frequency\n");
+ ret = -EBUSY;
... but -EBUSY there?
+ goto error_cleanup;
+ }
+ }
+
+ dev->clckdiv = iic_clckdiv(*freq);
+ dev_dbg(&ofdev->dev, "clckdiv = %d\n", dev->clckdiv);
+
+ /* Initialize IIC interface */
+ iic_dev_init(dev);
+
+ /* Register it with i2c layer */
+ adap = &dev->adap;
+ adap->dev.parent = &ofdev->dev;
+ strlcpy(adap->name, "IBM IIC", sizeof(adap->name));
+ i2c_set_adapdata(adap, dev);
+ adap->id = I2C_HW_OCP;
+ adap->class = I2C_CLASS_HWMON;
+ adap->algo = &iic_algo;
+ adap->timeout = 1;
+ adap->nr = dev->idx;
+
+ ret = i2c_add_numbered_adapter(adap);
+ if (ret < 0) {
+ dev_err(&ofdev->dev, "failed to register i2c adapter\n");
+ goto error_cleanup;
+ }
+
+ dev_dbg(&ofdev->dev, "using %s mode\n",
dev_info?
quoted hunk
+ dev->fast_mode ? "fast (400 kHz)" : "standard (100 kHz)");
+
+ return 0;
+
+error_cleanup:
+ if (dev->irq != NO_IRQ) {
+ iic_interrupt_mode(dev, 0);
+ free_irq(dev->irq, dev);
+ }
+
+ if (dev->vaddr)
+ iounmap(dev->vaddr);
+
+ dev_set_drvdata(&ofdev->dev, NULL);
+ kfree(dev);
+ return ret;
+}
+
+/*
+ * Cleanup initialized IIC interface
+ */
+static int __devexit iic_remove(struct of_device *ofdev)
+{
+ struct ibm_iic_private *dev = dev_get_drvdata(&ofdev->dev);
+
+ dev_set_drvdata(&ofdev->dev, NULL);
+
+ i2c_del_adapter(&dev->adap);
+
+ if (dev->irq != NO_IRQ) {
+ iic_interrupt_mode(dev, 0);
+ free_irq(dev->irq, dev);
+ }
+
+ iounmap(dev->vaddr);
+ kfree(dev);
+
+ return 0;
+}
+
+static const struct of_device_id ibm_iic_match[] = {
+ { .compatible = "ibm,iic-405ex", },
+ { .compatible = "ibm,iic-405gp", },
+ { .compatible = "ibm,iic-440gp", },
+ { .compatible = "ibm,iic-440gpx", },
+ { .compatible = "ibm,iic-440grx", },
+ {}
+};
+
+static struct of_platform_driver ibm_iic_driver = {
+ .name = "ibm-iic",
+ .match_table = ibm_iic_match,
+ .probe = iic_probe,
+ .remove = __devexit_p(iic_remove),
+};
+
+static int __init iic_init(void)
+{
+ return of_register_platform_driver(&ibm_iic_driver);
+}
+
+static void __exit iic_exit(void)
+{
+ of_unregister_platform_driver(&ibm_iic_driver);
+}
+#endif /* CONFIG_IBM_OCP */
+
module_init(iic_init);
module_exit(iic_exit);diff --git a/drivers/i2c/busses/Kconfig b/drivers/i2c/busses/Kconfig
index b61f56b..44c0984 100644
--- a/drivers/i2c/busses/Kconfig
+++ b/drivers/i2c/busses/Kconfig
@@ -244,7 +244,6 @@ config I2C_PIIX4
config I2C_IBM_IIC
tristate "IBM PPC 4xx on-chip I2C interface"
- depends on IBM_OCP
help
Say Y here if you want to use IIC peripheral found on
embedded IBM PPC 4xx based systems.
With this Kconfig change, "make menuconfig" lets me select the
i2c-ibm_iic driver on x86_64, but it fails to build horribly. I think
that you want to restrict the build to PPC machines somehow, or at
least make sure that either IBM_OCP or OF support is present.
The rest looks fine to me. If you can address my last few comments, I
can push your 2 i2c-ibm_iic patches to -mm.
Thanks,
--
Jean Delvare