RE: [PATCH v7 3/5] drivers/i2c/busses/i2c-at91.c: add new driver
Hi,
Arnd Bergmann wrote on 2011-11-23:
On Tuesday 08 November 2011, Nikolaus Voss wrote:
+
+static unsigned at91_twi_read(struct at91_twi_dev *dev, unsigned reg)
+{
+ return __raw_readl(dev-base + reg);
+}
+
+static void at91_twi_write(struct at91_twi_dev *dev, unsigned reg, unsigned
val)
+{
+ __raw_writel(val, dev-base + reg);
+}
Better use readl/writel or readl_relaxed/writel_relaxed.
ok, changed.
+/*
+ * Calculate and set symmetric clock as stated in datasheet:
+ * twi_clk = F_MAIN / (2 * cdiv * (1 ckdiv) + 4)
+ */
+static void __devinit at91_set_twi_clock(struct at91_twi_dev *dev, int
twi_clk)
+{
+ const int div = DIV_ROUND_UP(clk_get_rate(dev-clk), 2 * twi_clk) - 2;
+ int ckdiv = fls(div 8);
+ const int cdiv = div ckdiv;
+
+ if (cpu_is_at91rm9200() (ckdiv 5)) {
+ dev_warn(dev-dev, AT91RM9200 erratum 22: using ckdiv = 5.\n);
+ ckdiv = 5;
+ }
Don't check a global property like this locally in the driver. Instead,
make it a property of the device that you check here and set it based on
the the device name set by the platform, or by a device tree property.
Yes, this is a known problem and was discussed in
https://lkml.org/lkml/2011/11/8/217 . The main problem is that there a no
revisions for specific at91 IP devices but the revision is implicitly
defined by the cpu type and version. Changing this would need to add some
arch infrastructure which I think is beyond the scope of this driver.
diff --git a/drivers/i2c/busses/i2c-at91.h b/drivers/i2c/busses/i2c-at91.h
new file mode 100644
index 000..a898159
--- /dev/null
+++ b/drivers/i2c/busses/i2c-at91.h
@@ -0,0 +1,80 @@
+
+#ifndef AT91_TWI_H
+#define AT91_TWI_H
+
No need for a header file if all the values in it are used in only one
source file. Just move everything to i2c_at91.c
+#defineAT91_TWI_MMR0x04/* Master Mode Register
*/
+#defineAT91_TWI_IADRSZ (3 8)/* Internal Device
Address
+* Size */
+#defineAT91_TWI_IADRSZ_NO (0 8)
+#defineAT91_TWI_IADRSZ_1 (1 8)
+#defineAT91_TWI_IADRSZ_2 (2 8)
+#defineAT91_TWI_IADRSZ_3 (3 8)
+#defineAT91_TWI_MREAD (1 12)/* Master Read Direction
*/
+#defineAT91_TWI_DADR (0x7f 16)/* Device Address */
These are harder to read than just using hexadecimal bitmasks:
#define AT91_TWI_MMR0x0004
#define AT91_TWI_IADRSZ 0x0300
#define AT91_TWI_IADRSZ_NO 0x
#define AT91_TWI_IADRSZ_1 0x0100
...
I agree, but this header file was already used by the old driver and
converting would add possible errors to register definitions which are
not (yet) used. That's why I've left it as is and just made it a local
include.
Thanks,
Niko
--
To unsubscribe from this list: send the line unsubscribe linux-i2c in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v7 3/5] drivers/i2c/busses/i2c-at91.c: add new driver
On Thursday 24 November 2011, Voss, Nikolaus wrote:
+/*
+ * Calculate and set symmetric clock as stated in datasheet:
+ * twi_clk = F_MAIN / (2 * cdiv * (1 ckdiv) + 4)
+ */
+static void __devinit at91_set_twi_clock(struct at91_twi_dev *dev, int
twi_clk)
+{
+ const int div = DIV_ROUND_UP(clk_get_rate(dev-clk), 2 * twi_clk) - 2;
+ int ckdiv = fls(div 8);
+ const int cdiv = div ckdiv;
+
+ if (cpu_is_at91rm9200() (ckdiv 5)) {
+ dev_warn(dev-dev, AT91RM9200 erratum 22: using ckdiv =
5.\n);
+ ckdiv = 5;
+ }
Don't check a global property like this locally in the driver. Instead,
make it a property of the device that you check here and set it based on
the the device name set by the platform, or by a device tree property.
Yes, this is a known problem and was discussed in
https://lkml.org/lkml/2011/11/8/217 . The main problem is that there a no
revisions for specific at91 IP devices but the revision is implicitly
defined by the cpu type and version. Changing this would need to add some
arch infrastructure which I think is beyond the scope of this driver.
Aside from the flamewar in that thread, my impression is that in general
people (me certainly) prefer to have driver-local workarounds be expressed
in a driver specific way, not in a platform or architecture specific way
because that makes the driver less portable.
Anyway, I don't see this point as a show-stopper since the driver is already
platform specific, but it's generally a good idea to write code like this
defensively, if only to avoid getting comments about it.
diff --git a/drivers/i2c/busses/i2c-at91.h b/drivers/i2c/busses/i2c-at91.h
new file mode 100644
index 000..a898159
--- /dev/null
+++ b/drivers/i2c/busses/i2c-at91.h
@@ -0,0 +1,80 @@
+
+#ifndef AT91_TWI_H
+#define AT91_TWI_H
+
No need for a header file if all the values in it are used in only one
source file. Just move everything to i2c_at91.c
+#defineAT91_TWI_MMR0x04/* Master Mode
Register */
+#defineAT91_TWI_IADRSZ (3 8)/* Internal Device
Address
+* Size */
+#defineAT91_TWI_IADRSZ_NO (0 8)
+#defineAT91_TWI_IADRSZ_1 (1 8)
+#defineAT91_TWI_IADRSZ_2 (2 8)
+#defineAT91_TWI_IADRSZ_3 (3 8)
+#defineAT91_TWI_MREAD (1 12)/* Master Read
Direction
*/
+#defineAT91_TWI_DADR (0x7f 16)/* Device Address */
These are harder to read than just using hexadecimal bitmasks:
#define AT91_TWI_MMR0x0004
#define AT91_TWI_IADRSZ 0x0300
#define AT91_TWI_IADRSZ_NO 0x
#define AT91_TWI_IADRSZ_1 0x0100
...
I agree, but this header file was already used by the old driver and
converting would add possible errors to register definitions which are
not (yet) used. That's why I've left it as is and just made it a local
include.
But you are presenting the driver as a new one, so you should be
prepared to get review comments like any other new code.
Please at least move the data into the main driver file to get rid of
the header file.
Arnd
--
To unsubscribe from this list: send the line unsubscribe linux-i2c in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
RE: [PATCH v7 3/5] drivers/i2c/busses/i2c-at91.c: add new driver
Hi,
Arnd Bergmann wrote on 2011-11-24:
On Thursday 24 November 2011, Voss, Nikolaus wrote:
+/*
+ * Calculate and set symmetric clock as stated in datasheet:
+ * twi_clk = F_MAIN / (2 * cdiv * (1 ckdiv) + 4)
+ */
+static void __devinit at91_set_twi_clock(struct at91_twi_dev *dev, int
twi_clk)
+{
+ const int div = DIV_ROUND_UP(clk_get_rate(dev-clk), 2 * twi_clk) -
2;
+ int ckdiv = fls(div 8);
+ const int cdiv = div ckdiv;
+
+ if (cpu_is_at91rm9200() (ckdiv 5)) {
+ dev_warn(dev-dev, AT91RM9200 erratum 22: using ckdiv =
5.\n);
+ ckdiv = 5;
+ }
Don't check a global property like this locally in the driver. Instead,
make it a property of the device that you check here and set it based on
the the device name set by the platform, or by a device tree property.
Yes, this is a known problem and was discussed in
https://lkml.org/lkml/2011/11/8/217 . The main problem is that there a no
revisions for specific at91 IP devices but the revision is implicitly
defined by the cpu type and version. Changing this would need to add some
arch infrastructure which I think is beyond the scope of this driver.
Aside from the flamewar in that thread, my impression is that in general
people (me certainly) prefer to have driver-local workarounds be expressed
in a driver specific way, not in a platform or architecture specific way
because that makes the driver less portable.
I guess I see your point now. So you want something like pdev.has_bugX to be
set by mach setup and later check this flag in the driver?
diff --git a/drivers/i2c/busses/i2c-at91.h b/drivers/i2c/busses/i2c-
at91.h
new file mode 100644
index 000..a898159
--- /dev/null
+++ b/drivers/i2c/busses/i2c-at91.h
@@ -0,0 +1,80 @@
+
+#ifndef AT91_TWI_H
+#define AT91_TWI_H
+
No need for a header file if all the values in it are used in only one
source file. Just move everything to i2c_at91.c
+#defineAT91_TWI_MMR0x04/* Master Mode
Register */
+#defineAT91_TWI_IADRSZ (3 8)/* Internal Device
Address
+* Size */
+#defineAT91_TWI_IADRSZ_NO (0 8)
+#defineAT91_TWI_IADRSZ_1 (1 8)
+#defineAT91_TWI_IADRSZ_2 (2 8)
+#defineAT91_TWI_IADRSZ_3 (3 8)
+#defineAT91_TWI_MREAD (1 12)/* Master Read
Direction
*/
+#defineAT91_TWI_DADR (0x7f 16)/* Device Address */
These are harder to read than just using hexadecimal bitmasks:
#define AT91_TWI_MMR0x0004
#define AT91_TWI_IADRSZ 0x0300
#define AT91_TWI_IADRSZ_NO 0x
#define AT91_TWI_IADRSZ_1 0x0100
...
I agree, but this header file was already used by the old driver and
converting would add possible errors to register definitions which are
not (yet) used. That's why I've left it as is and just made it a local
include.
But you are presenting the driver as a new one, so you should be
prepared to get review comments like any other new code.
Please at least move the data into the main driver file to get rid of
the header file.
I didn't want to appear ignorant about this, I actually appreciate your
comment. I just wanted to point out that there might be a reason to keep
the old file which you weren't aware of (because I presented this as a new
driver). So, I will move the register definitions to the main driver.
Thanks,
Niko
--
To unsubscribe from this list: send the line unsubscribe linux-i2c in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v7 3/5] drivers/i2c/busses/i2c-at91.c: add new driver
On Thursday 24 November 2011, Voss, Nikolaus wrote: Aside from the flamewar in that thread, my impression is that in general people (me certainly) prefer to have driver-local workarounds be expressed in a driver specific way, not in a platform or architecture specific way because that makes the driver less portable. I guess I see your point now. So you want something like pdev.has_bugX to be set by mach setup and later check this flag in the driver? Yes, that would be the idea. I would not introduce platform_data for one driver just for this though, because we are generally moving away from platform_data towards device tree probing. You can do it with a match table that has two entries with different names for the two kinds of device that you need to distinguish and set the platform_device_id-driver_data field to '1' for the type that needs the fixup. #define AT91_TWI_MMR0x0004 #define AT91_TWI_IADRSZ 0x0300 #define AT91_TWI_IADRSZ_NO 0x #define AT91_TWI_IADRSZ_1 0x0100 ... I agree, but this header file was already used by the old driver and converting would add possible errors to register definitions which are not (yet) used. That's why I've left it as is and just made it a local include. But you are presenting the driver as a new one, so you should be prepared to get review comments like any other new code. Please at least move the data into the main driver file to get rid of the header file. I didn't want to appear ignorant about this, I actually appreciate your comment. I just wanted to point out that there might be a reason to keep the old file which you weren't aware of (because I presented this as a new driver). So, I will move the register definitions to the main driver. Ok, good. Moving it into the driver is really the important part anyway, and I understand your reasoning for not wanting to modify the definitions, it just didn't apply to the one of the two comments I made about the header. Arnd -- To unsubscribe from this list: send the line unsubscribe linux-i2c in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v7 3/5] drivers/i2c/busses/i2c-at91.c: add new driver
On Tuesday 08 November 2011, Nikolaus Voss wrote:
+
+static unsigned at91_twi_read(struct at91_twi_dev *dev, unsigned reg)
+{
+ return __raw_readl(dev-base + reg);
+}
+
+static void at91_twi_write(struct at91_twi_dev *dev, unsigned reg, unsigned
val)
+{
+ __raw_writel(val, dev-base + reg);
+}
Better use readl/writel or readl_relaxed/writel_relaxed.
+/*
+ * Calculate and set symmetric clock as stated in datasheet:
+ * twi_clk = F_MAIN / (2 * cdiv * (1 ckdiv) + 4)
+ */
+static void __devinit at91_set_twi_clock(struct at91_twi_dev *dev, int
twi_clk)
+{
+ const int div = DIV_ROUND_UP(clk_get_rate(dev-clk), 2 * twi_clk) - 2;
+ int ckdiv = fls(div 8);
+ const int cdiv = div ckdiv;
+
+ if (cpu_is_at91rm9200() (ckdiv 5)) {
+ dev_warn(dev-dev, AT91RM9200 erratum 22: using ckdiv = 5.\n);
+ ckdiv = 5;
+ }
Don't check a global property like this locally in the driver. Instead,
make it a property of the device that you check here and set it based on
the the device name set by the platform, or by a device tree property.
diff --git a/drivers/i2c/busses/i2c-at91.h b/drivers/i2c/busses/i2c-at91.h
new file mode 100644
index 000..a898159
--- /dev/null
+++ b/drivers/i2c/busses/i2c-at91.h
@@ -0,0 +1,80 @@
+
+#ifndef AT91_TWI_H
+#define AT91_TWI_H
+
No need for a header file if all the values in it are used in only one
source file. Just move everything to i2c_at91.c
+#define AT91_TWI_MMR0x04/* Master Mode Register
*/
+#define AT91_TWI_IADRSZ (3 8)/* Internal Device
Address
+ * Size */
+#define AT91_TWI_IADRSZ_NO (0 8)
+#define AT91_TWI_IADRSZ_1 (1 8)
+#define AT91_TWI_IADRSZ_2 (2 8)
+#define AT91_TWI_IADRSZ_3 (3 8)
+#define AT91_TWI_MREAD (1 12)/* Master Read
Direction */
+#define AT91_TWI_DADR (0x7f 16)/* Device Address */
These are harder to read than just using hexadecimal bitmasks:
#define AT91_TWI_MMR0x0004
#define AT91_TWI_IADRSZ 0x0300
#define AT91_TWI_IADRSZ_NO 0x
#define AT91_TWI_IADRSZ_1 0x0100
...
Arnd
--
To unsubscribe from this list: send the line unsubscribe linux-i2c in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
