Hi Andrew, On 14/09/23 6:16 am, Andrew Lunn wrote: > EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe > >> +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. Ok, I will implement two functions to serve their purposes. > >> +/* 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. Sorry, I don't get your point. Could you please explain bit more? > >> 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. Ah ok, will take care in the next version. Best Regards, Parthiban V > > Andrew >