Re: [PATCH net-next 1/7] tg3: Add 5720 ASIC rev

["Matt Carlson" <mcarlson@broadcom.com>]

On Mon, Apr 04, 2011 at 11:37:42AM -0700, Joe Perches wrote:
> On Mon, 2011-04-04 at 11:24 -0700, Matt Carlson wrote:
> > This patch adds support for the 5720 ASIC rev.
> > 
> > Signed-off-by: Matt Carlson <mcarlson@broadcom.com>
> > Reviewed-by: Michael Chan <mchan@broadcom.com>
> > ---
> >  drivers/net/tg3.c |   67 +++++++++++++++++++++++++++++++++++++---------------
> >  drivers/net/tg3.h |    5 ++++
> >  2 files changed, 52 insertions(+), 20 deletions(-)
> > 
> > diff --git a/drivers/net/tg3.c b/drivers/net/tg3.c
> > index b7e03a6..b105cdd 100644
> > --- a/drivers/net/tg3.c
> > +++ b/drivers/net/tg3.c
> > @@ -98,12 +98,14 @@
> >   */
> >  #define TG3_RX_STD_RING_SIZE(tp) \
> >  	((GET_ASIC_REV(tp->pci_chip_rev_id) == ASIC_REV_5717 || \
> > -	  GET_ASIC_REV(tp->pci_chip_rev_id) == ASIC_REV_5719) ? \
> > +	  GET_ASIC_REV(tp->pci_chip_rev_id) == ASIC_REV_5719 || \
> > +	  GET_ASIC_REV(tp->pci_chip_rev_id) == ASIC_REV_5720) ? \
> 
> Maybe it'd be better to convert all of these:
> 
> 	if (GET_ASIC_REV(tp->pci_chip_rev_id) == ASIC_REV_5717 ||
> 	    GET_ASIC_REV(tp->pci_chip_rev_id) == ASIC_REV_5719 ||
> 	    GET_ASIC_REV(tp->pci_chip_rev_id) == ASIC_REV_5720)
> 
> tests to a common functions something like:
> 
> static bool can_asic_<foo>(struct tg3 *tp)
> {
> 	int rev = GET_ASIC_REV(tp->pci_chip_rev_id);
> 	return (rev == ASIC_REV_5717) ||
> 	       (rev == ASIC_REV_5719) ||
> 	       (rev == ASIC_REV_5720);
> }

In general, we do try to do this.  We already have a TG3_FLG3_5717_PLUS
flag, but it includes the 57765 ASIC rev.  I thought :

	if ((tp->tg3_flags3 & TG3_FLG3_5717_PLUS) &&
	    GET_ASIC_REV(tp->pci_chip_rev_id) != ASIC_REV_57765)

didn't look much cleaner.  Demoting the 57765 from that flag also
uglifies the code too much.

Let me think about this a little more.