[illumos-Developer] [REVIEW] 1299 Support SiI3531 chipset in si3124 driver

[Albert Lee]

On Fri, Jul 29, 2011 at 3:24 PM, Charles Stephens <cfs at cowlabs.com> wrote:
> On Jul 29, 2011, at 12:15 PM, Garrett D'Amore wrote:
>
> Some feedback that came from a Nexenta engineer (Vineeth Pillai):
>
> <snip>
>
> @@ -565,7 +565,11 @@
>
>                                if (si_ctlp->sictl_devid == SI3132_DEV_ID) {
>
>                                                si_ctlp->sictl_num_ports =
> SI3132_MAX_PORTS;
>
>                                } else {
>
> -                                              si_ctlp->sictl_num_ports =
> SI3124_MAX_PORTS;
>
> +                                             if (si_ctlp->sictl_devid ==
> SI3124_DEV_ID) {
>
> +
> si_ctlp->sictl_num_ports = SI3124_MAX_PORTS;
>
> +                                             } else {
>
> +
> si_ctlp->sictl_num_ports = SI3531_MAX_PORTS;
>
> +                                             }
>
>                                }
> <snip>
>
>
>
> Would it make sense to change this to a switch{}, or independent else if{}
> statements rather than putting nested if cases? Might be helpful, if there
> are more chips to be supported in future.
>
> Yes it does.  However, I was trying to be conservative with my changes to
> code I didn't fully understand.
>
>  Also  si3124var.h:
>
> 167    int sictl_devid; /* whether it is 3124 or 3132 */
>
>
>
> Make sense to remove this comment or update this..
>
> Remove the comment.
>
> Also, previously we defaulted to SI3124_MAX_PORTS in case of no match of the
> devid. But with this fix, we default to SI3531_MAX_PORTS. Would this be of
> any potential harm?
>
> --> Garrett talking now:
>
> That last one I think needs addressing.  Charles, can you please fix and
> resend?
>
> Its actually less harmful than before.  But more importantly, this code
> won't be called because the driver would not attach since previously it
> would attempt to match the PCI dev ID's not find the three values it can
> identify.
> It's not a problem though making these changes.
> cfs

While we're here, I don't think James Purdy's fix for 6881910 ever went in:
http://opensolaris.org/jive/thread.jspa?threadID=119683

Is there absolutely no way to tell how many ports present are on a device?

-Albert