Re: [RFC PATCH net-next 3/6] net: ethernet: implement OA TC6 configuration function

[Andrew Lunn]

> +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