> +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
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
>
> >> +/* 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?
Look around kernel header files. How often do you see ~BIT(5)? My
guess it is approximately 0. So i'm suggesting you remove the ~ and
have the user of the #define assemble the mask and then do the ~ .
Andrew
Hi Andrew,
On 19/09/23 6:24 pm, Andrew Lunn wrote:
> EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe
>
>>>> +/* 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?
>
> Look around kernel header files. How often do you see ~BIT(5)? My
> guess it is approximately 0. So i'm suggesting you remove the ~ and
> have the user of the #define assemble the mask and then do the ~ .
Ah ok ok, thanks for the clarification.
Best Regards,
Parthiban V
>
> Andrew