Dear dirk.eibach@gdsys.cc,
In message 1370959364-28943-2-git-send-email-dirk.eibach@gdsys.cc you wrote:
From: Dirk Eibach eibach@gdsys.de
A set of accessor functions was added to be able to access not only memory mapped FPGA in a generic way.
Thanks to Wolfgang Denk for getting this sorted properly.
Sorry, I still have comments...
+#ifdef CONFIG_SYS_FPGA_NO_RFL_HI
FPGA_GET_REG(k, reflection_low, &val);+#else
FPGA_GET_REG(k, reflection_high, &val);+#endif
Can this #ifdef here (and similar below) not be avoided by defining a proper value to some macro and using this as function argument instead?
diff --git a/board/gdsys/405ep/dlvision-10g.c b/board/gdsys/405ep/dlvision-10g.c index 644493b..332d82f 100644 --- a/board/gdsys/405ep/dlvision-10g.c +++ b/board/gdsys/405ep/dlvision-10g.c
...
+struct ihs_fpga *fpga_ptr[] = CONFIG_SYS_FPGA_PTR;
+int fpga_set_reg(u32 fpga, u16 *reg, off_t regoff, u16 data) +{
- out_le16(reg, data);
- return 0;
+}
+int fpga_get_reg(u32 fpga, u16 *reg, off_t regoff, u16 *data) +{
- *data = in_le16(reg);
- return 0;
+}
...
diff --git a/board/gdsys/405ep/io.c b/board/gdsys/405ep/io.c index 070dcbb..05707c4 100644 --- a/board/gdsys/405ep/io.c +++ b/board/gdsys/405ep/io.c @@ -53,6 +53,22 @@ enum { HWVER_122 = 3, };
+struct ihs_fpga *fpga_ptr[] = CONFIG_SYS_FPGA_PTR;
+int fpga_set_reg(u32 fpga, u16 *reg, off_t regoff, u16 data) +{
- out_le16(reg, data);
- return 0;
+}
+int fpga_get_reg(u32 fpga, u16 *reg, off_t regoff, u16 *data) +{
- *data = in_le16(reg);
- return 0;
+}
...
diff --git a/board/gdsys/405ep/iocon.c b/board/gdsys/405ep/iocon.c index 0fc7db2..a531e5d 100644 --- a/board/gdsys/405ep/iocon.c +++ b/board/gdsys/405ep/iocon.c @@ -69,6 +69,22 @@ enum { RAM_DDR2_32 = 0, };
+struct ihs_fpga *fpga_ptr[] = CONFIG_SYS_FPGA_PTR;
+int fpga_set_reg(u32 fpga, u16 *reg, off_t regoff, u16 data) +{
- out_le16(reg, data);
- return 0;
+}
+int fpga_get_reg(u32 fpga, u16 *reg, off_t regoff, u16 *data) +{
- *data = in_le16(reg);
- return 0;
+}
We have the very same code repeated 3 times aready here, and two more times follow further down. Please factor out this common code so we have a single copy only.
Also, this does not handle the non-memory-mapped case (for fpga == 0) ?
@@ -230,17 +249,21 @@ int last_stage_init(void) */ void fpga_gpio_set(int pin) {
- out_le16((void *)(CONFIG_SYS_FPGA0_BASE + 0x18), pin);
- FPGA_SET_REG(0, gpio.set, pin);
}
void fpga_gpio_clear(int pin) {
- out_le16((void *)(CONFIG_SYS_FPGA0_BASE + 0x16), pin);
- FPGA_SET_REG(0, gpio.clear, pin);
}
int fpga_gpio_get(int pin) {
- return in_le16((void *)(CONFIG_SYS_FPGA0_BASE + 0x14)) & pin;
- u16 val;
- FPGA_GET_REG(0, gpio.read, &val);
- return val & pin;
}
Do you really have to keep these fpga_gpio_set() / fpga_gpio_clear() / fpga_gpio_get() functions? Can you not fix the related code to use FPGA_GET_REG() directly?
@@ -282,7 +288,7 @@ static int osd_print(cmd_tbl_t *cmdtp, int flag, int argc, char * const argv[]) { unsigned screen;
- for (screen = 0; screen < CONFIG_SYS_OSD_SCREENS; ++screen) {
- for (screen = 0; screen <= max_osd_screen; ++screen) {
This looks like an unrelated change. Is it intentionally included here?
...
- out_le16(&osd->xy_size, ((32 - 1) << 8) | (16 - 1));
- out_le16(&osd->x_pos, 0x007f);
- out_le16(&osd->y_pos, 0x005f);
- if (screen > max_osd_screen)
max_osd_screen = screen;
Ditto here?
@@ -386,7 +396,7 @@ int osd_write(cmd_tbl_t *cmdtp, int flag, int argc, char * const argv[]) { unsigned screen;
- for (screen = 0; screen < CONFIG_SYS_OSD_SCREENS; ++screen) {
- for (screen = 0; screen <= max_osd_screen; ++screen) {
And here?
Best regards,
Wolfgang Denk