Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::3:7 as permitted sender) client-ip=2620:137:e000::3:7;
Date:   Mon, 23 Oct 2023 23:28:58 +0200
From:   Andrew Lunn <andrew@lunn.ch>
To:     Parthiban Veerasooran <Parthiban.Veerasooran@microchip.com>
Cc:     davem@davemloft.net, edumazet@google.com, kuba@kernel.org,
        pabeni@redhat.com, robh+dt@kernel.org,
        krzysztof.kozlowski+dt@linaro.org, conor+dt@kernel.org,
        corbet@lwn.net, steen.hegelund@microchip.com,
        rdunlap@infradead.org, horms@kernel.org, casper.casan@gmail.com,
        netdev@vger.kernel.org, devicetree@vger.kernel.org,
        linux-kernel@vger.kernel.org, linux-doc@vger.kernel.org,
        horatiu.vultur@microchip.com, Woojung.Huh@microchip.com,
        Nicolas.Ferre@microchip.com, UNGLinuxDriver@microchip.com,
        Thorsten.Kummermehr@microchip.com
Subject: Re: [PATCH net-next v2 1/9] net: ethernet: implement OPEN Alliance
 control transaction interface
Message-ID: <c51d9660-d6c3-4202-9fc6-b9add06b64ce@lunn.ch>
References: <20231023154649.45931-1-Parthiban.Veerasooran@microchip.com>
 <20231023154649.45931-2-Parthiban.Veerasooran@microchip.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20231023154649.45931-2-Parthiban.Veerasooran@microchip.com>
Precedence: bulk

> +static void oa_tc6_prepare_ctrl_buf(struct oa_tc6 *tc6, u32 addr, u32 val[],
> +				    u8 len, bool wnr, u8 *buf, bool prote)

One of the comments i made last time was that wnr is not obvious. I
assume it means write-not-read. So why not just write? Since it a
boolean, i assume war is never needed, so !wnr cal always be
considered rnw.

And prote could well be protect, the two extra characters make it a
lot more obvious. Or better still.

> +{
> +	u32 hdr;
> +
> +	/* Prepare the control header with the required details */
> +	hdr = FIELD_PREP(CTRL_HDR_DNC, 0) |
> +	      FIELD_PREP(CTRL_HDR_WNR, wnr) |
> +	      FIELD_PREP(CTRL_HDR_AID, 0) |
> +	      FIELD_PREP(CTRL_HDR_MMS, addr >> 16) |
> +	      FIELD_PREP(CTRL_HDR_ADDR, addr) |
> +	      FIELD_PREP(CTRL_HDR_LEN, len - 1);
> +	hdr |= FIELD_PREP(CTRL_HDR_P, oa_tc6_get_parity(hdr));
> +	*(__be32 *)buf = cpu_to_be32(hdr);
> +
> +	if (wnr) {
> +		for (u8 i = 0; i < len; i++) {
> +			u16 pos;
> +
> +			if (!prote) {

nitpick, but please use positive logic. Do the protected case first.

	 Andrew