Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.37 as permitted sender) client-ip=23.128.96.37;
Date:   Thu, 14 Sep 2023 02:46:03 +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: [RFC PATCH net-next 3/6] net: ethernet: implement OA TC6
 configuration function
Message-ID: <dd0a6cd5-91e5-4e13-8025-d6c88bdab5a2@lunn.ch>
References: <20230908142919.14849-1-Parthiban.Veerasooran@microchip.com>
 <20230908142919.14849-4-Parthiban.Veerasooran@microchip.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20230908142919.14849-4-Parthiban.Veerasooran@microchip.com>
Precedence: bulk

> +int oa_tc6_configure(struct oa_tc6 *tc6, u8 cps, bool ctrl_prot, bool tx_cut_thr,
> +		     bool rx_cut_thr)
> +{
> +	u32 regval;
> +	int ret;
> +
> +	/* Read and configure the IMASK0 register for unmasking the interrupts */
> +	ret = oa_tc6_read_register(tc6, OA_TC6_IMASK0, &regval, 1);
> +	if (ret)
> +		return ret;
> +
> +	regval &= TXPEM & TXBOEM & TXBUEM & RXBOEM & LOFEM & HDREM;
> +	ret = oa_tc6_write_register(tc6, OA_TC6_IMASK0, &regval, 1);

It is not so obvious what this 1 means. Maybe change to regval[1], and
user ARRAY_SIZE(). What also does not help is the function name,
oa_tc6_write_register(). Singular. So it appears to write one register,
not multiple registers. It might even make sense to make
oa_tc6_write_register() truly access a single register, and add
oa_tc6_write_registers() for multiple registers.

> +/* Unmasking interrupt fields in IMASK0 */
> +#define HDREM		~BIT(5)		/* Header Error Mask */
> +#define LOFEM		~BIT(4)		/* Loss of Framing Error Mask */
> +#define RXBOEM		~BIT(3)		/* Rx Buffer Overflow Error Mask */
> +#define TXBUEM		~BIT(2)		/* Tx Buffer Underflow Error Mask */
> +#define TXBOEM		~BIT(1)		/* Tx Buffer Overflow Error Mask */
> +#define TXPEM		~BIT(0)		/* Tx Protocol Error Mask */

Using ~BIT(X) is very usual. I would not do this, Principle of Least
Surprise.

>  struct oa_tc6 {
> -	struct spi_device *spi;
> -	bool ctrl_prot;
> +	struct completion rst_complete;
>  	struct task_struct *tc6_task;
>  	wait_queue_head_t tc6_wq;
> +	struct spi_device *spi;
> +	bool tx_cut_thr;
> +	bool rx_cut_thr;
> +	bool ctrl_prot;
>  	bool int_flag;
> -	struct completion rst_complete;
> +	u8 cps;
>  };

Please try not to move stuff around. It makes the diff bigger than it
should be.

       Andrew