[U-Boot] [PATCH v2 06/10] powerpc/ppc4xx: Use generic FPGA accessors on all gdsys 405ep/ex boards

[Wolfgang Denk]

Dear Dirk Eibach,

In message <449c45a6a9978c55e84d3fe7efe6f0ac at gdsys.cc> you wrote:
> 
> Read the source, Luke :)

It was a bit difficult to spot in this unordered mix of patches ;-)

> +void fpga_set_reg(unsigned int fpga, u16 reg, u16 data)
> +{
> +	int res;
> +	struct ihs_fpga *fpga_0 = (struct ihs_fpga *)CONFIG_SYS_FPGA_BASE(0);
> +
> +	switch (fpga) {
> +	case 0:
> +		out_le16((u16 *)fpga_0 + reg / sizeof(u16), data);
> +		break;
> +	default:
> +		res = mclink_send(fpga - 1, reg, data);
> +		if (res < 0)
> +			printf("mclink_send reg %02x data %04x returned %d\n",
> +			       reg, data, res);
> +		break;
> +	}
> +}
> 
> So no memory mapping here. That's the reason for all this fuzz.

OK - but I don't see why mclink_send() could not be used in the same
way, i. e. taking a struct member as argument?

Apparently all your FPGA accesses are for  u16  data only?  Then you
could rewrite fpga_set_reg() similar to this:

	int fpga_set_reg(u32 fpga, u16 *addr, u16 data)

And voila, no need to move away from the C structs.

Note 1: You print an error message if mclink_send() returns an error;
	I think this error should propagate, i. e. this function
	should not return  void.

Note 2: I changed the type of the first arg into "u32" so it is
        consistent with the other args. Mixing native types (unsigned
        int) here and defined types (as u16 instead of unsigned
        short) looks weird to me.

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: wd at denx.de
Eeeek!
'eval' on strings should have been named 'evil'.    -- Tom Phoenix in
        <Pine.GSO.3.96.980526121813.27437N-100000 at user2.teleport.com>