> +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, ®val, 1); > + if (ret) > + return ret; > + > + regval &= TXPEM & TXBOEM & TXBUEM & RXBOEM & LOFEM & HDREM; > + ret = oa_tc6_write_register(tc6, OA_TC6_IMASK0, ®val, 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