Re: [PATCH v4 5/5] net: dsa: add support for Atheros AR9331 build-in switch

From: Andrew Lunn <andrew@lunn.ch>
To: Oleksij Rempel <o.rempel@pengutronix.de>
Cc: Mark Rutland <mark.rutland@arm.com>,
	devicetree@vger.kernel.org, Jay Cliburn <jcliburn@gmail.com>,
	Florian Fainelli <f.fainelli@gmail.com>,
	Chris Snook <chris.snook@gmail.com>,
	linux-kernel@vger.kernel.org, Ralf Baechle <ralf@linux-mips.org>,
	"David S. Miller" <davem@davemloft.net>,
	Paul Burton <paul.burton@mips.com>,
	Rob Herring <robh+dt@kernel.org>,
	Pengutronix Kernel Team <kernel@pengutronix.de>,
	James Hogan <jhogan@kernel.org>,
	Russell King <linux@armlinux.org.uk>,
	netdev@vger.kernel.org, linux-mips@vger.kernel.org,
	Vivien Didelot <vivien.didelot@gmail.com>
Subject: Re: [PATCH v4 5/5] net: dsa: add support for Atheros AR9331 build-in switch
Date: Tue, 29 Oct 2019 13:38:54 +0100	[thread overview]
Message-ID: <20191029123854.GN15259@lunn.ch> (raw)
In-Reply-To: <20191029071404.pl34q4rmadusc2u5@pengutronix.de>

Hi Oleksij

> > > +static void ar9331_sw_port_disable(struct dsa_switch *ds, int port)
> > > +{
> > > +	struct ar9331_sw_priv *priv = (struct ar9331_sw_priv *)ds->priv;
> > > +	struct regmap *regmap = priv->regmap;
> > > +	int ret;
> > > +
> > > +	ret = regmap_write(regmap, AR9331_SW_REG_PORT_STATUS(port), 0);
> > > +	if (ret)
> > > +		dev_err_ratelimited(priv->dev, "%s: %i\n", __func__, ret);
> > > +}
> > 
> > I've asked this before, but i don't remember the answer. Why are
> > port_enable and port_disable the same?
> 
> I have only MAC TX/RX enable bit. This bit is set by phylink_mac_link_up and
> removed by phylink_mac_link_down.
> The port enable I use only to set predictable state of the port
> register: all bits cleared. May be i should just drop port enable
> function? What do you think? 

At minimum, it needs a comment about why enable and disable are the
same. If i keep asking, others will as well.

If there is nothing useful to do, then drop it.

   Andrew