All of lore.kernel.org
 help / color / mirror / Atom feed
From: Vivien Didelot <vivien.didelot@savoirfairelinux.com>
To: Andrew Lunn <andrew@lunn.ch>, David Miller <davem@davemloft.net>
Cc: Florian Fainelli <f.fainelli@gmail.com>,
	netdev <netdev@vger.kernel.org>, Andrew Lunn <andrew@lunn.ch>
Subject: Re: [PATCH net-next 1/8] dsa: mv88e6xxx: Prepare for turning this into a library module
Date: Thu, 14 Apr 2016 16:22:08 -0400	[thread overview]
Message-ID: <874mb454b3.fsf@ketchup.mtl.sfl> (raw)
In-Reply-To: <1460591998-20598-2-git-send-email-andrew@lunn.ch>

Hi Andrew,

Andrew Lunn <andrew@lunn.ch> writes:

> Export all the functions so that we can later turn the module into a
> library module.
>
> Signed-off-by: Andrew Lunn <andrew@lunn.ch>

Sorry but I don't like this. We don't want one module per 88E6xxx switch
model. We need one driver supporting them all, like any other driver.

Multiple modules will continue to confuse us with duplicated code. For
instance, every specific mv88e6*_setup_global functions program the
switch's DSA device number with something like:

    REG_WRITE(REG_GLOBAL, GLOBAL_CONTROL_2, ds->index & 0x1f);

Looking at every drivers/net/dsa/mv88e6*.c file, there are only few
differences in their dsa_switch_driver structures:

The .setup function is always specific, but easily factorizable in a
mv88e6xxx_setup function. The .probe function can be merged once we have
a single driver. mv88e6131 has different phy_{read,write} functions
which can be abstracted in mv88e6xxx_phy_{read,write}. Only mv88e6352
has support for the EEPROM, which is simple to abstract too.

I'm working on a few patches right away to factorize this and lighten up
that part from your current refactoring of DSA.

Here's an example of duplicated code fixed for the 6131 PHY access code:

    http://ix.io/wJm

Thanks,
Vivien

  reply	other threads:[~2016-04-14 20:22 UTC|newest]

Thread overview: 16+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2016-04-13 23:59 [PATCH net-next 0/8] DSA refactoring: set 2 Andrew Lunn
2016-04-13 23:59 ` [PATCH net-next 1/8] dsa: mv88e6xxx: Prepare for turning this into a library module Andrew Lunn
2016-04-14 20:22   ` Vivien Didelot [this message]
2016-04-14 21:26     ` Florian Fainelli
2016-04-14 21:37       ` Andrew Lunn
2016-04-14 21:49       ` David Miller
2016-04-14 21:32     ` Andrew Lunn
2016-04-13 23:59 ` [PATCH net-next 2/8] dsa: mv88e6xxx: Add macro for registering the drivers Andrew Lunn
2016-04-13 23:59 ` [PATCH net-next 3/8] dsa: Add mdio device support to Marvell switches Andrew Lunn
2016-04-13 23:59 ` [PATCH net-next 4/8] dsa: Move gpio reset into switch driver Andrew Lunn
2016-04-14  2:03   ` David Miller
2016-04-13 23:59 ` [PATCH net-next 5/8] dsa: mv88e6xxx: Kill the REG_READ and REG_WRITE macros Andrew Lunn
2016-04-14 14:54   ` Vivien Didelot
2016-04-13 23:59 ` [PATCH net-next 6/8] dsa: mv88e6xxx: Replace ds with ps where possible Andrew Lunn
2016-04-13 23:59 ` [PATCH net-next 7/8] dsa: mv88e6xxx: Use the name table to determine number of ports Andrew Lunn
2016-04-13 23:59 ` [PATCH net-next 8/8] dsa: mv88e6131: Don't special case a single device Andrew Lunn

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=874mb454b3.fsf@ketchup.mtl.sfl \
    --to=vivien.didelot@savoirfairelinux.com \
    --cc=andrew@lunn.ch \
    --cc=davem@davemloft.net \
    --cc=f.fainelli@gmail.com \
    --cc=netdev@vger.kernel.org \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.