linux-mips.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
From: David Miller <davem@davemloft.net>
To: o.rempel@pengutronix.de
Cc: paul.burton@mips.com, ralf@linux-mips.org, jhogan@kernel.org,
	robh+dt@kernel.org, jcliburn@gmail.com, chris.snook@gmail.com,
	mark.rutland@arm.com, kernel@pengutronix.de,
	linux-mips@vger.kernel.org, linux-kernel@vger.kernel.org,
	devicetree@vger.kernel.org, john@phrozen.org, nbd@nbd.name,
	netdev@vger.kernel.org, andrew@lunn.ch, gch981213@gmail.com,
	info@freifunk-bad-gandersheim.net
Subject: Re: [PATCH v5 3/3] net: ethernet: add ag71xx driver
Date: Mon, 20 May 2019 10:51:59 -0700 (PDT)	[thread overview]
Message-ID: <20190520.105159.1094490201484427551.davem@davemloft.net> (raw)
In-Reply-To: <20190520070716.23668-4-o.rempel@pengutronix.de>

From: Oleksij Rempel <o.rempel@pengutronix.de>
Date: Mon, 20 May 2019 09:07:16 +0200

> +struct ag71xx_buf {
> +	union {
> +		struct sk_buff *skb;
> +		void *rx_buf;
> +	};
> +	union {
> +		dma_addr_t dma_addr;
> +		unsigned int len;
> +	};
> +};

I find this double union very confusing.

When using unions you should make it strictly clear which members are used
together, at what times, and in which situations.

Therefore, please use something like anonymous structures to group the
members that are used together at the same time, something like:

struct ag71xx_buf {
	union {
		struct {
			struct sk_buff *skb;
			dma_addr_t dma_addr;
		} tx;
		struct {
			void *rx_buf;
			unsigned int len;
		} rx;
};

Or at the very least add a very big comment that explains the use of
the union members.

> +static int ag71xx_mdio_mii_read(struct mii_bus *bus, int addr, int reg)
> +{
> +	struct ag71xx *ag = bus->priv;
> +	struct net_device *ndev = ag->ndev;
> +	int err, val;

Reverse christmas tree here please.

> +static int ag71xx_mdio_mii_write(struct mii_bus *bus, int addr, int reg,
> +				 u16 val)
> +{
> +	struct ag71xx *ag = bus->priv;
> +	struct net_device *ndev = ag->ndev;
> +

Likewise.

> +static int ag71xx_mdio_probe(struct ag71xx *ag)
> +{
> +	static struct mii_bus *mii_bus;
> +	struct device *dev = &ag->pdev->dev;
> +	struct device_node *np = dev->of_node;
> +	struct net_device *ndev = ag->ndev;
> +	int err;

Likewise.

> +static int ag71xx_tx_packets(struct ag71xx *ag, bool flush)
> +{
> +	struct ag71xx_ring *ring = &ag->tx_ring;
> +	struct net_device *ndev = ag->ndev;
> +	bool dma_stuck = false;
> +	int ring_mask = BIT(ring->order) - 1;
> +	int ring_size = BIT(ring->order);
> +	int sent = 0;
> +	int bytes_compl = 0;
> +	int n = 0;

Likewise.

And so on, and so forth, for the rest of this file.

      parent reply	other threads:[~2019-05-20 17:52 UTC|newest]

Thread overview: 6+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2019-05-20  7:07 [PATCH v5 0/3] MIPS: ath79: add ag71xx support Oleksij Rempel
2019-05-20  7:07 ` [PATCH v5 1/3] dt-bindings: net: add qca,ar71xx.txt documentation Oleksij Rempel
2019-05-20  7:07 ` [PATCH v5 2/3] MIPS: ath79: ar9331: add Ethernet nodes Oleksij Rempel
2019-05-20  7:07 ` [PATCH v5 3/3] net: ethernet: add ag71xx driver Oleksij Rempel
2019-05-20 15:50   ` Andrew Lunn
2019-05-20 17:51   ` David Miller [this message]

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=20190520.105159.1094490201484427551.davem@davemloft.net \
    --to=davem@davemloft.net \
    --cc=andrew@lunn.ch \
    --cc=chris.snook@gmail.com \
    --cc=devicetree@vger.kernel.org \
    --cc=gch981213@gmail.com \
    --cc=info@freifunk-bad-gandersheim.net \
    --cc=jcliburn@gmail.com \
    --cc=jhogan@kernel.org \
    --cc=john@phrozen.org \
    --cc=kernel@pengutronix.de \
    --cc=linux-kernel@vger.kernel.org \
    --cc=linux-mips@vger.kernel.org \
    --cc=mark.rutland@arm.com \
    --cc=nbd@nbd.name \
    --cc=netdev@vger.kernel.org \
    --cc=o.rempel@pengutronix.de \
    --cc=paul.burton@mips.com \
    --cc=ralf@linux-mips.org \
    --cc=robh+dt@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 a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).