diff mbox

[net-next,1/8] dsa: mv88e6xxx: Prepare for turning this into a library module

Message ID 1460591998-20598-2-git-send-email-andrew@lunn.ch
State Changes Requested, archived
Delegated to: David Miller
Headers show

Commit Message

Andrew Lunn April 13, 2016, 11:59 p.m. UTC 
Export all the functions so that we can later turn the module into a
library module.

Signed-off-by: Andrew Lunn <andrew@lunn.ch>
---
 drivers/net/dsa/mv88e6xxx.c | 44 ++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 44 insertions(+)

Comments

Vivien Didelot April 14, 2016, 8:22 p.m. UTC | #1 
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
Florian Fainelli April 14, 2016, 9:26 p.m. UTC | #2 
On 14/04/16 13:22, Vivien Didelot wrote:
> 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.

Are you sure this is a good model moving forward? This means the library
needs to know about every new switch added and all its little gory
details, whereas the point is that it represents *most* of what is
needed, defines a good enough, generic model, but does not have to deal
(too much) with HW-specifics, see below.

> 
> 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

The cost of maintaining a smallish piece of driver code that deals with
things that are extremely specific to a given switch HW seems like a
reasonable thing to do. The library should ideally be mostly
HW-independent in the sense that it should only deal with switch HW
properties that are shared and common (number of ports, number of
FIB/VTUs etc.) and the indidivual switch drivers need to deal with all
the ad-hoc stuff that has no place everywhere else.

I believe this is currently the case for most of what is being done by
mv88e6xxx.c, Andrew's patches are not making things worse.
Andrew Lunn April 14, 2016, 9:32 p.m. UTC | #3 
Hi Vivien

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

Please take a look at the full series of 40 patches, before deciding
what you want to clean up. It is too many to post at once, so i'm
breaking them up into chunks. But that does mean some of the 'why' is
missing, which i'm trying to add back via a good description in the
cover note.

https://github.com/lunn/linux.git v5.6-rc2-net-next-dsa-probe

> Here's an example of duplicated code fixed for the 6131 PHY access code:
> 
>     http://ix.io/wJm

I've done this already.

     Andrew
Andrew Lunn April 14, 2016, 9:37 p.m. UTC | #4 
> I believe this is currently the case for most of what is being done by
> mv88e6xxx.c, Andrew's patches are not making things worse.

Hi Florian

That was my aim. I did the minimum restructuring needed to make these
DSA drivers classical Linux drivers. Nothing i've done would prevent
them being merged into one later. I don't want to do such a merge now,
because that is just a distraction from the bigger picture of
reworking how probe works.

	  Andrew
David Miller April 14, 2016, 9:49 p.m. UTC | #5 
From: Florian Fainelli <f.fainelli@gmail.com>
Date: Thu, 14 Apr 2016 14:26:39 -0700

> On 14/04/16 13:22, Vivien Didelot wrote:
>> 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.
> 
> Are you sure this is a good model moving forward? This means the library
> needs to know about every new switch added and all its little gory
> details, whereas the point is that it represents *most* of what is
> needed, defines a good enough, generic model, but does not have to deal
> (too much) with HW-specifics, see below.

I also think all of the mv88e6xxx drivers are insanely similar, and could
be driven by one monolithic driver.  It's not going to be that big at all.

Symbol exporting from a library and having several small similar
drivers reference those symbols, on the other hand, tends to be messy
in my opinion.
diff mbox

Patch

diff --git a/drivers/net/dsa/mv88e6xxx.c b/drivers/net/dsa/mv88e6xxx.c
index 9985a0cf31f1..9b568f39aaf0 100644
--- a/drivers/net/dsa/mv88e6xxx.c
+++ b/drivers/net/dsa/mv88e6xxx.c
@@ -120,6 +120,7 @@  int mv88e6xxx_reg_read(struct dsa_switch *ds, int addr, int reg)
 
 	return ret;
 }
+EXPORT_SYMBOL_GPL(mv88e6xxx_reg_read);
 
 static int __mv88e6xxx_reg_write(struct mii_bus *bus, int sw_addr, int addr,
 				 int reg, u16 val)
@@ -177,6 +178,7 @@  int mv88e6xxx_reg_write(struct dsa_switch *ds, int addr, int reg, u16 val)
 
 	return ret;
 }
+EXPORT_SYMBOL_GPL(mv88e6xxx_reg_write);
 
 int mv88e6xxx_set_addr_direct(struct dsa_switch *ds, u8 *addr)
 {
@@ -186,6 +188,7 @@  int mv88e6xxx_set_addr_direct(struct dsa_switch *ds, u8 *addr)
 
 	return 0;
 }
+EXPORT_SYMBOL_GPL(mv88e6xxx_set_addr_direct);
 
 int mv88e6xxx_set_addr_indirect(struct dsa_switch *ds, u8 *addr)
 {
@@ -211,6 +214,7 @@  int mv88e6xxx_set_addr_indirect(struct dsa_switch *ds, u8 *addr)
 
 	return 0;
 }
+EXPORT_SYMBOL_GPL(mv88e6xxx_set_addr_indirect);
 
 static int _mv88e6xxx_phy_read(struct dsa_switch *ds, int addr, int regnum)
 {
@@ -336,6 +340,7 @@  void mv88e6xxx_ppu_state_init(struct dsa_switch *ds)
 	ps->ppu_timer.data = (unsigned long)ps;
 	ps->ppu_timer.function = mv88e6xxx_ppu_reenable_timer;
 }
+EXPORT_SYMBOL_GPL(mv88e6xxx_ppu_state_init);
 
 int mv88e6xxx_phy_read_ppu(struct dsa_switch *ds, int addr, int regnum)
 {
@@ -349,6 +354,7 @@  int mv88e6xxx_phy_read_ppu(struct dsa_switch *ds, int addr, int regnum)
 
 	return ret;
 }
+EXPORT_SYMBOL_GPL(mv88e6xxx_phy_read_ppu);
 
 int mv88e6xxx_phy_write_ppu(struct dsa_switch *ds, int addr,
 			    int regnum, u16 val)
@@ -363,6 +369,7 @@  int mv88e6xxx_phy_write_ppu(struct dsa_switch *ds, int addr,
 
 	return ret;
 }
+EXPORT_SYMBOL_GPL(mv88e6xxx_phy_write_ppu);
 #endif
 
 static bool mv88e6xxx_6065_family(struct dsa_switch *ds)
@@ -587,6 +594,7 @@  void mv88e6xxx_adjust_link(struct dsa_switch *ds, int port,
 out:
 	mutex_unlock(&ps->smi_mutex);
 }
+EXPORT_SYMBOL_GPL(mv88e6xxx_adjust_link);
 
 static int _mv88e6xxx_stats_wait(struct dsa_switch *ds)
 {
@@ -783,6 +791,7 @@  void mv88e6xxx_get_strings(struct dsa_switch *ds, int port, uint8_t *data)
 		}
 	}
 }
+EXPORT_SYMBOL_GPL(mv88e6xxx_get_strings);
 
 int mv88e6xxx_get_sset_count(struct dsa_switch *ds)
 {
@@ -796,6 +805,7 @@  int mv88e6xxx_get_sset_count(struct dsa_switch *ds)
 	}
 	return j;
 }
+EXPORT_SYMBOL_GPL(mv88e6xxx_get_sset_count);
 
 void
 mv88e6xxx_get_ethtool_stats(struct dsa_switch *ds,
@@ -823,11 +833,13 @@  mv88e6xxx_get_ethtool_stats(struct dsa_switch *ds,
 
 	mutex_unlock(&ps->smi_mutex);
 }
+EXPORT_SYMBOL_GPL(mv88e6xxx_get_ethtool_stats);
 
 int mv88e6xxx_get_regs_len(struct dsa_switch *ds, int port)
 {
 	return 32 * sizeof(u16);
 }
+EXPORT_SYMBOL_GPL(mv88e6xxx_get_regs_len);
 
 void mv88e6xxx_get_regs(struct dsa_switch *ds, int port,
 			struct ethtool_regs *regs, void *_p)
@@ -847,6 +859,7 @@  void mv88e6xxx_get_regs(struct dsa_switch *ds, int port,
 			p[i] = ret;
 	}
 }
+EXPORT_SYMBOL_GPL(mv88e6xxx_get_regs);
 
 static int _mv88e6xxx_wait(struct dsa_switch *ds, int reg, int offset,
 			   u16 mask)
@@ -890,12 +903,14 @@  int mv88e6xxx_eeprom_load_wait(struct dsa_switch *ds)
 	return mv88e6xxx_wait(ds, REG_GLOBAL2, GLOBAL2_EEPROM_OP,
 			      GLOBAL2_EEPROM_OP_LOAD);
 }
+EXPORT_SYMBOL_GPL(mv88e6xxx_eeprom_load_wait);
 
 int mv88e6xxx_eeprom_busy_wait(struct dsa_switch *ds)
 {
 	return mv88e6xxx_wait(ds, REG_GLOBAL2, GLOBAL2_EEPROM_OP,
 			      GLOBAL2_EEPROM_OP_BUSY);
 }
+EXPORT_SYMBOL_GPL(mv88e6xxx_eeprom_busy_wait);
 
 static int _mv88e6xxx_atu_wait(struct dsa_switch *ds)
 {
@@ -962,6 +977,7 @@  out:
 	mutex_unlock(&ps->smi_mutex);
 	return reg;
 }
+EXPORT_SYMBOL_GPL(mv88e6xxx_get_eee);
 
 int mv88e6xxx_set_eee(struct dsa_switch *ds, int port,
 		      struct phy_device *phydev, struct ethtool_eee *e)
@@ -988,6 +1004,7 @@  out:
 
 	return ret;
 }
+EXPORT_SYMBOL_GPL(mv88e6xxx_set_eee);
 
 static int _mv88e6xxx_atu_cmd(struct dsa_switch *ds, u16 fid, u16 cmd)
 {
@@ -1216,6 +1233,7 @@  void mv88e6xxx_port_stp_state_set(struct dsa_switch *ds, int port, u8 state)
 	set_bit(port, ps->port_state_update_mask);
 	schedule_work(&ps->bridge_work);
 }
+EXPORT_SYMBOL_GPL(mv88e6xxx_port_stp_state_set);
 
 static int _mv88e6xxx_port_pvid(struct dsa_switch *ds, int port, u16 *new,
 				u16 *old)
@@ -1456,6 +1474,7 @@  unlock:
 
 	return err;
 }
+EXPORT_SYMBOL_GPL(mv88e6xxx_port_vlan_dump);
 
 static int _mv88e6xxx_vtu_loadpurge(struct dsa_switch *ds,
 				    struct mv88e6xxx_vtu_stu_entry *entry)
@@ -1864,6 +1883,7 @@  unlock:
 
 	return ret;
 }
+EXPORT_SYMBOL_GPL(mv88e6xxx_port_vlan_filtering);
 
 int mv88e6xxx_port_vlan_prepare(struct dsa_switch *ds, int port,
 				const struct switchdev_obj_port_vlan *vlan,
@@ -1884,6 +1904,7 @@  int mv88e6xxx_port_vlan_prepare(struct dsa_switch *ds, int port,
 	 */
 	return 0;
 }
+EXPORT_SYMBOL_GPL(mv88e6xxx_port_vlan_prepare);
 
 static int _mv88e6xxx_port_vlan_add(struct dsa_switch *ds, int port, u16 vid,
 				    bool untagged)
@@ -1924,6 +1945,7 @@  void mv88e6xxx_port_vlan_add(struct dsa_switch *ds, int port,
 
 	mutex_unlock(&ps->smi_mutex);
 }
+EXPORT_SYMBOL_GPL(mv88e6xxx_port_vlan_add);
 
 static int _mv88e6xxx_port_vlan_del(struct dsa_switch *ds, int port, u16 vid)
 {
@@ -1990,6 +2012,7 @@  unlock:
 
 	return err;
 }
+EXPORT_SYMBOL_GPL(mv88e6xxx_port_vlan_del);
 
 static int _mv88e6xxx_atu_mac_write(struct dsa_switch *ds,
 				    const unsigned char *addr)
@@ -2079,6 +2102,7 @@  int mv88e6xxx_port_fdb_prepare(struct dsa_switch *ds, int port,
 	 */
 	return 0;
 }
+EXPORT_SYMBOL_GPL(mv88e6xxx_port_fdb_prepare);
 
 void mv88e6xxx_port_fdb_add(struct dsa_switch *ds, int port,
 			    const struct switchdev_obj_port_fdb *fdb,
@@ -2094,6 +2118,7 @@  void mv88e6xxx_port_fdb_add(struct dsa_switch *ds, int port,
 		netdev_err(ds->ports[port], "failed to load MAC address\n");
 	mutex_unlock(&ps->smi_mutex);
 }
+EXPORT_SYMBOL_GPL(mv88e6xxx_port_fdb_add);
 
 int mv88e6xxx_port_fdb_del(struct dsa_switch *ds, int port,
 			   const struct switchdev_obj_port_fdb *fdb)
@@ -2108,6 +2133,7 @@  int mv88e6xxx_port_fdb_del(struct dsa_switch *ds, int port,
 
 	return ret;
 }
+EXPORT_SYMBOL_GPL(mv88e6xxx_port_fdb_del);
 
 static int _mv88e6xxx_atu_getnext(struct dsa_switch *ds, u16 fid,
 				  struct mv88e6xxx_atu_entry *entry)
@@ -2241,6 +2267,7 @@  unlock:
 
 	return err;
 }
+EXPORT_SYMBOL_GPL(mv88e6xxx_port_fdb_dump);
 
 int mv88e6xxx_port_bridge_join(struct dsa_switch *ds, int port,
 			       struct net_device *bridge)
@@ -2283,6 +2310,7 @@  unlock:
 
 	return err;
 }
+EXPORT_SYMBOL_GPL(mv88e6xxx_port_bridge_join);
 
 void mv88e6xxx_port_bridge_leave(struct dsa_switch *ds, int port)
 {
@@ -2308,6 +2336,7 @@  void mv88e6xxx_port_bridge_leave(struct dsa_switch *ds, int port)
 
 	mutex_unlock(&ps->smi_mutex);
 }
+EXPORT_SYMBOL_GPL(mv88e6xxx_port_bridge_leave);
 
 static void mv88e6xxx_bridge_work(struct work_struct *work)
 {
@@ -2659,6 +2688,7 @@  int mv88e6xxx_setup_ports(struct dsa_switch *ds)
 	}
 	return 0;
 }
+EXPORT_SYMBOL_GPL(mv88e6xxx_setup_ports);
 
 int mv88e6xxx_setup_common(struct dsa_switch *ds)
 {
@@ -2673,6 +2703,7 @@  int mv88e6xxx_setup_common(struct dsa_switch *ds)
 
 	return 0;
 }
+EXPORT_SYMBOL_GPL(mv88e6xxx_setup_common);
 
 int mv88e6xxx_setup_global(struct dsa_switch *ds)
 {
@@ -2793,6 +2824,7 @@  unlock:
 
 	return ret;
 }
+EXPORT_SYMBOL_GPL(mv88e6xxx_setup_global);
 
 int mv88e6xxx_switch_reset(struct dsa_switch *ds, bool ppu_active)
 {
@@ -2842,6 +2874,7 @@  int mv88e6xxx_switch_reset(struct dsa_switch *ds, bool ppu_active)
 
 	return 0;
 }
+EXPORT_SYMBOL_GPL(mv88e6xxx_switch_reset);
 
 int mv88e6xxx_phy_page_read(struct dsa_switch *ds, int port, int page, int reg)
 {
@@ -2854,6 +2887,7 @@  int mv88e6xxx_phy_page_read(struct dsa_switch *ds, int port, int page, int reg)
 
 	return ret;
 }
+EXPORT_SYMBOL_GPL(mv88e6xxx_phy_page_read);
 
 int mv88e6xxx_phy_page_write(struct dsa_switch *ds, int port, int page,
 			     int reg, int val)
@@ -2867,6 +2901,7 @@  int mv88e6xxx_phy_page_write(struct dsa_switch *ds, int port, int page,
 
 	return ret;
 }
+EXPORT_SYMBOL_GPL(mv88e6xxx_phy_page_write);
 
 static int mv88e6xxx_port_to_phy_addr(struct dsa_switch *ds, int port)
 {
@@ -2892,6 +2927,7 @@  mv88e6xxx_phy_read(struct dsa_switch *ds, int port, int regnum)
 	mutex_unlock(&ps->smi_mutex);
 	return ret;
 }
+EXPORT_SYMBOL_GPL(mv88e6xxx_phy_read);
 
 int
 mv88e6xxx_phy_write(struct dsa_switch *ds, int port, int regnum, u16 val)
@@ -2908,6 +2944,7 @@  mv88e6xxx_phy_write(struct dsa_switch *ds, int port, int regnum, u16 val)
 	mutex_unlock(&ps->smi_mutex);
 	return ret;
 }
+EXPORT_SYMBOL_GPL(mv88e6xxx_phy_write);
 
 int
 mv88e6xxx_phy_read_indirect(struct dsa_switch *ds, int port, int regnum)
@@ -2924,6 +2961,7 @@  mv88e6xxx_phy_read_indirect(struct dsa_switch *ds, int port, int regnum)
 	mutex_unlock(&ps->smi_mutex);
 	return ret;
 }
+EXPORT_SYMBOL_GPL(mv88e6xxx_phy_read_indirect);
 
 int
 mv88e6xxx_phy_write_indirect(struct dsa_switch *ds, int port, int regnum,
@@ -2941,6 +2979,7 @@  mv88e6xxx_phy_write_indirect(struct dsa_switch *ds, int port, int regnum,
 	mutex_unlock(&ps->smi_mutex);
 	return ret;
 }
+EXPORT_SYMBOL_GPL(mv88e6xxx_phy_write_indirect);
 
 #ifdef CONFIG_NET_DSA_HWMON
 
@@ -3012,6 +3051,7 @@  int mv88e6xxx_get_temp(struct dsa_switch *ds, int *temp)
 
 	return mv88e61xx_get_temp(ds, temp);
 }
+EXPORT_SYMBOL_GPL(mv88e6xxx_get_temp);
 
 int mv88e6xxx_get_temp_limit(struct dsa_switch *ds, int *temp)
 {
@@ -3031,6 +3071,7 @@  int mv88e6xxx_get_temp_limit(struct dsa_switch *ds, int *temp)
 
 	return 0;
 }
+EXPORT_SYMBOL_GPL(mv88e6xxx_get_temp_limit);
 
 int mv88e6xxx_set_temp_limit(struct dsa_switch *ds, int temp)
 {
@@ -3047,6 +3088,7 @@  int mv88e6xxx_set_temp_limit(struct dsa_switch *ds, int temp)
 	return mv88e6xxx_phy_page_write(ds, phy, 6, 26,
 					(ret & 0xe0ff) | (temp << 8));
 }
+EXPORT_SYMBOL_GPL(mv88e6xxx_set_temp_limit);
 
 int mv88e6xxx_get_temp_alarm(struct dsa_switch *ds, bool *alarm)
 {
@@ -3066,6 +3108,7 @@  int mv88e6xxx_get_temp_alarm(struct dsa_switch *ds, bool *alarm)
 
 	return 0;
 }
+EXPORT_SYMBOL_GPL(mv88e6xxx_get_temp_alarm);
 #endif /* CONFIG_NET_DSA_HWMON */
 
 static char *mv88e6xxx_lookup_name(struct mii_bus *bus, int sw_addr,
@@ -3125,6 +3168,7 @@  char *mv88e6xxx_drv_probe(struct device *dsa_dev, struct device *host_dev,
 	}
 	return name;
 }
+EXPORT_SYMBOL_GPL(mv88e6xxx_drv_probe);
 
 static int __init mv88e6xxx_init(void)
 {