Menu
▾
▴
Re: [Emc-developers] [Emc-commit] v2.5_branch: Add support for Mesa 5i21, 5i25 and 4i69 FPGA cards.
|
From: Sebastian K. <se...@hi...> - 2011-08-30 02:28:49
|
On 08/29/2011 06:45 PM, andy pugh wrote:
> Is this what you had in mind?
Looks pretty good, thanks! Comments inline...
> --------
>
> This commit adds a port number and port pin field to the hm2_pin_t struct,
> populates it once, then uses those numbers in all the places where the
> numbering was calculated using an assumed 24-pin port width which is no
> longer true.
>
>
> Signed-off-by: Andy Pugh<an...@bo...>
> ---
> src/hal/drivers/mesa-hostmot2/hostmot2.h | 2 +
> src/hal/drivers/mesa-hostmot2/pins.c | 108 ++++++++++++++++--------------
> 2 files changed, 61 insertions(+), 49 deletions(-)
...
> diff --git a/src/hal/drivers/mesa-hostmot2/pins.c
> b/src/hal/drivers/mesa-hostmot2/pins.c
> index 71f899b..7602bbe 100644
> --- a/src/hal/drivers/mesa-hostmot2/pins.c
> +++ b/src/hal/drivers/mesa-hostmot2/pins.c
> @@ -202,6 +202,8 @@ int hm2_read_pin_descriptors(hostmot2_t *hm2) {
> hm2->pin[i].sec_tag = (d>> 8)& 0x000000FF;
> hm2->pin[i].sec_unit = (d>> 16)& 0x000000FF;
> hm2->pin[i].primary_tag = (d>> 24)& 0x000000FF;
> + hm2->pin[i].port_num = -1;
> + hm2->pin[i].port_pin = -1;
>
> if (hm2->pin[i].primary_tag == 0) {
> // oops, found the Zero sentinel before the promised number of pins
I think I'd prefer to call hm2_set_pin_numbers() in
hm2_read_pin_descriptors(), above, instead of initializing them to
something invalid and hoping we don't need them too soon. At
hm2_read_pin_descriptor()-time we have enough information to set them
correctly.
> @@ -236,25 +238,59 @@ int hm2_read_pin_descriptors(hostmot2_t *hm2) {
> }
>
>
> +// sets the port number and pin number within the port
> +static void hm2_set_pin_numbers(hostmot2_t *hm2, int i){
> + int mio;
> + hm2->pin[i].port_num = i / hm2->llio->pins_per_connector;
> + switch (hm2->llio->pins_per_connector) {
> + case 24: /* standard 50 pin 24 I/O cards, just the odd pins */
> + hm2->pin[i].port_pin = ((i % 24) * 2) + 1;
> + break;
> + case 17: /* 25 pin 17 I/O parallel port type cards funny
> DB25 order */
> + mio = i % 17;
> + if (mio> 7){
> + hm2->pin[i].port_pin = mio - 3;
> + }
> + else {
> + if (mio& 1){
> + hm2->pin[i].port_pin = (mio / 2) + 14;
> + }
> + else {
> + hm2->pin[i].port_pin = (mio / 2) + 1;
> + }
> + }
> + break;
> + case 32: /* 5I21 punt on this for now */
> + hm2->pin[i].port_pin = i + 1;
> + break;
> + default:
> + HM2_ERR("hm2_pins_set_numbers: invalid port width %d\n",
> + hm2->llio->pins_per_connector);
> + }
> +}
It might make sense to swallow the hm2_set_pin_numbers() function into
its caller (whatever that is), since it's called exactly once per pin.
I guess it's a matter of taste, but having it be a separate function
suggests to me that it's a more generally useful piece of code, rather
than the very special-purpose, run-once-only thing it actually is.
>
> void hm2_set_pin_source(hostmot2_t *hm2, int pin_number, int source) {
> - int ioport_number;
> - int bit_number;
> -
> - ioport_number = pin_number / 24;
> - bit_number = pin_number % 24;
>
> - if ((pin_number< 0) || (ioport_number>= hm2->ioport.num_instances)) {
> + if ((pin_number< 0)
> + || (pin_number>= hm2->llio->num_ioport_connectors *
> hm2->llio->pins_per_connector)) {
You can use hm2->num_pins as a clearer(?) shorthand for the product in
the second part of this test.
> HM2_ERR("hm2_set_pin_source: invalid pin number %d\n", pin_number);
> return;
> }
> +
> + if ((hm2->pin[pin_number].port_num< 0)
> + || (hm2->pin[pin_number].port_num>
> hm2->llio->num_ioport_connectors)) {
> + HM2_ERR("hm2_set_pin_source: invalid port number %d\n",
> + hm2->pin[pin_number].port_num);
> + }
This sanity test should be done exactly once, in hm2_set_pin_numbers(),
if you don't trust your own code that sets it in that function.
> if (source == HM2_PIN_SOURCE_IS_PRIMARY) {
> - hm2->ioport.alt_source_reg[ioport_number]&= ~(1<< bit_number);
> + hm2->ioport.alt_source_reg[hm2->pin[pin_number].port_num]
> +&= ~(1<< hm2->pin[pin_number].port_pin);
> hm2->pin[pin_number].gtag = hm2->pin[pin_number].primary_tag;
> } else if (source == HM2_PIN_SOURCE_IS_SECONDARY) {
> - hm2->ioport.alt_source_reg[ioport_number] |= (1<< bit_number);
> + hm2->ioport.alt_source_reg[hm2->pin[pin_number].port_num]
> + |= (1<< hm2->pin[pin_number].port_pin);
> hm2->pin[pin_number].gtag = hm2->pin[pin_number].sec_tag;
> } else {
> HM2_ERR("hm2_set_pin_source: invalid pin source 0x%08X\n", source);
Maybe hm2_pin_t *pin = hm2->pin[pin_number]; would help make the above
code easier to parse?
> @@ -266,16 +302,18 @@ void hm2_set_pin_source(hostmot2_t *hm2, int
> pin_number, int source) {
>
>
> void hm2_set_pin_direction(hostmot2_t *hm2, int pin_number, int direction) {
> - int ioport_number;
> - int bit_number;
> -
> - ioport_number = pin_number / 24;
> - bit_number = pin_number % 24;
>
> - if ((pin_number< 0) || (ioport_number>= hm2->ioport.num_instances)) {
> + if ((pin_number< 0)
> + || (pin_number>= hm2->llio->num_ioport_connectors *
> hm2->llio->pins_per_connector)) {
> HM2_ERR("hm2_set_pin_direction: invalid pin number %d\n", pin_number);
> return;
> }
> +
> + if ((hm2->pin[pin_number].port_num< 0)
> + || (hm2->pin[pin_number].port_num>
> hm2->llio->num_ioport_connectors)) {
> + HM2_ERR("hm2_set_pin_direction: invalid port number %d\n",
> + hm2->pin[pin_number].port_num);
> + }
Same comments as for hm2_set_pin_source(), above.
The rest of the patch looks great. Thanks for the cleanup!
--
Sebastian Kuzminsky
|