Dear dirk.eib...@gdsys.cc,
In message <1370959364-28943-2-git-send-email-dirk.eib...@gdsys.cc> you wrote:
> From: Dirk Eibach <eib...@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
--
DENX Software Engineering GmbH, MD: Wolfgang Denk & Detlev Zundel
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: w...@denx.de
Do not simplify the design of a program if a way can be found to make
it complex and wonderful.
_______________________________________________
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot
