Messages in this thread | | | Date | Tue, 19 Jul 2022 02:17:20 +0200 | From | Christian Marangi <> | Subject | Re: [net-next RFC PATCH 1/4] net: dsa: qca8k: drop qca8k_read/write/rmw for regmap variant |
| |
On Tue, Jul 19, 2022 at 03:18:11AM +0300, Vladimir Oltean wrote: > On Tue, Jul 19, 2022 at 01:32:26AM +0200, Christian Marangi wrote: > > If the ops is not supported should I return -ENOSUPP? > > Example some ops won't be supported like the get_phy_flags or > > connect_tag_protocol for example. > > That's a slight disadvantage of this approach, that DSA sometimes checks > for the presence of a certain function pointer as an indication of > whether a feature is supported or not. However that doesn't work in all > cases, and then, it is actually necessary to call and see if it returns > -EOPNOTSUPP or not. For example, commit 1054457006d4 ("net: phy: > phylink: fix DSA mac_select_pcs() introduction") had to do just that > in phylink because of DSA. > > However, you need to check how each specific DSA operation is handled. > For example, the no-op implementation of get_phy_flags is to return 0 > (meaning "no special flags, thank you"). The no-op implementation for > connect_tag_protocol is to return success (0) for the tagging protocol > you support, and -EOPNOTSUPP for everything else. Here -EOPNOTSUPP isn't > a special code, it is an actual hard error that denies a certain tag > protocol from attaching. > > The advantage is that your driver-private ops don't have to map 1:1 with > the dsa_switch_ops, so there is more potential for code reuse than if > you had to reimplement an entire (*setup) function for example. You can > have ops for small things like regmap creation, things like that. > > > Anyway the series is ready, I was just pushing it... At the end it's 23 > > patch big... (I know you will hate me but at least it's reviewable) > > Please optimize the patches for a reviewer with average intelligence and > the attention span of a fish. 23 patches sounds like the series would > fail on the attention span count. > > > My solution currently was this... > > > > ops = devm_kzalloc(&mdiodev->dev, sizeof(*ops), GFP_KERNEL); > > if (!ops) > > return -ENOMEM; > > > > /* Copy common ops */ > > memcpy(ops, &qca8k_switch_ops, sizeof(*ops)); > > > > /* Setup specific ops */ > > ops->get_tag_protocol = qca8k_get_tag_protocol; > > Answered above. > > > ops->setup = qca8k_setup; > > Separate sub-operation, although this is a sub-optimal short-term > solution that kind of undermines the approach with a single > dsa_switch_ops in the long run. > > > ops->phylink_get_caps = qca8k_phylink_get_caps; > > Not sure what's going to be common and what's going to be different, but > you can take other drivers as an example, some parts will be common and > some hidden behind priv->info->mac_port_get_caps(). > > static void mt753x_phylink_get_caps(struct dsa_switch *ds, int port, > struct phylink_config *config) > { > struct mt7530_priv *priv = ds->priv; > > /* This switch only supports full-duplex at 1Gbps */ > config->mac_capabilities = MAC_ASYM_PAUSE | MAC_SYM_PAUSE | > MAC_10 | MAC_100 | MAC_1000FD; > > /* This driver does not make use of the speed, duplex, pause or the > * advertisement in its mac_config, so it is safe to mark this driver > * as non-legacy. > */ > config->legacy_pre_march2020 = false; > > priv->info->mac_port_get_caps(ds, port, config); > } > > > ops->phylink_mac_select_pcs = qca8k_phylink_mac_select_pcs; > > ops->phylink_mac_config = qca8k_phylink_mac_config; > > ops->phylink_mac_link_down = qca8k_phylink_mac_link_down; > > ops->phylink_mac_link_up = qca8k_phylink_mac_link_up; > > Hard to comment for these phylink ops how to organize the switch > differences in the best way, since I don't actually know what those > differences are. Again, other drivers may be useful. > > > ops->get_phy_flags = qca8k_get_phy_flags; > > ops->master_state_change = qca8k_master_change; > > ops->connect_tag_protocol = qca8k_connect_tag_protocol; > > > > /* Assign the final ops */ > > priv->ds->ops = ops; > > > > Will wait your response on how to hanle ops that are not supported. > > (I assume dsa checks if an ops is declared and not if it does return > > ENOSUPP, so this is my concern your example) > > Maybe it's best to think this conversion through and not rush a patch set. > I don't want you to blindly follow my advice to have a single dsa_switch_ops, > then half-ass it. This kind of thing needs to be done with care and > forethought.
Wonder if a good idea would be leave things as is for now and work of a single dsa_switch_ops on another series.
With "leave things as is" I mean that function will get migrated to qca8k-common.c and exposed with the header file.
And the dsa_switch_ops is defined in qca8k specific code.
The warn about the 23 patch was scary so considering this series is already a bit big and I can squash only a few patch, putting extra logic to correctly handle each would make this even bigger.
Think the right thing to do is handling the changes for single dsa_switch_ops to a separate series and at the same time also get some info on ipq4019 and what can be generalized.
What do you think?
-- Ansuel
| |