Re: [net-next RFC PATCH 1/4] net: dsa: qca8k: drop qca8k_read/write/rmw for regmap variant

[Vladimir Oltean]

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.