Hello Marc, thanks for response On Saturday 31 of October 2020 12:35:11 Marc Kleine-Budde wrote: > On 10/30/20 11:19 PM, Pavel Pisa wrote: > > This driver adds support for the CTU CAN FD open-source IP core. > > Please fix the following checkpatch warnings/errors: Yes I recheck with actual checkpatch, I have used 5.4 one and may it be overlooked something during last upadates. > ----------------------------------------- > drivers/net/can/ctucanfd/ctucanfd_frame.h > ----------------------------------------- > CHECK: Please don't use multiple blank lines > #46: FILE: drivers/net/can/ctucanfd/ctucanfd_frame.h:46: OK, we find a reason for this blank line in header generator. > CHECK: Prefer kernel type 'u32' over 'uint32_t' > #49: FILE: drivers/net/can/ctucanfd/ctucanfd_frame.h:49: > + uint32_t u32; In this case, please confirm that even your personal opinion is against uint32_t in headers, you request the change. uint32_t is used in many kernel headers and in this case allows our tooling to use headers for mutual test of HDL design match with HW access in the C. If the reasons to remove uint32_t prevails, we need to separate Linux generator from the one used for other purposes. When we add Linux mode then we can revamp headers even more and in such case we can even invest time to switch from structure bitfields to plain bitmask defines. It is quite lot of work and takes some time, but if there is consensus I do it during next weeks, I would like to see what is preferred way to define registers bitfields. I personally like RTEMS approach for which we have prepared generator from parsed PDFs when we added BSP for TMS570 https://git.rtems.org/rtems/tree/bsps/arm/tms570/include/bsp/ti_herc/reg_dcan.h#n152 Other solution I like (biased, because I have even designed it) is #define __val2mfld(mask,val) (((mask)&~((mask)<<1))*(val)&(mask)) #define __mfld2val(mask,val) (((val)&(mask))/((mask)&~((mask)<<1))) https://gitlab.com/pikron/sw-base/sysless/-/blob/master/arch/arm/generic/defines/cpu_def.h#L314 Which allows to use simple masks, i.e. #define SSP_CR0_DSS_m 0x000f /* Data Size Select (num bits - 1) */ #define SSP_CR0_FRF_m 0x0030 /* Frame Format: 0 SPI, 1 TI, 2 Microwire */ #define SSP_CR0_CPOL_m 0x0040 /* SPI Clock Polarity. 0 low between frames, 1 high */ # https://gitlab.com/pikron/sw-base/sysless/-/blob/master/libs4c/spi/spi_lpcssp.c#L46 in the sources lpcssp_drv->ssp_regs->CR0 = __val2mfld(SSP_CR0_DSS_m, lpcssp_drv->data16_fl? 16 - 1 : 8 - 1) | __val2mfld(SSP_CR0_FRF_m, 0) | (msg->size_mode & SPI_MODE_CPOL? SSP_CR0_CPOL_m: 0) | (msg->size_mode & SPI_MODE_CPHA? SSP_CR0_CPHA_m: 0) | __val2mfld(SSP_CR0_SCR_m, rate); https://gitlab.com/pikron/sw-base/sysless/-/blob/master/libs4c/spi/spi_lpcssp.c#L217 If you have some preferred Linux style then please send us pointers. In the fact, Ondrej Ille has based his structure bitfileds style on the other driver included in the Linux kernel and it seems to be a problem now. So when I invest my time, I want to use style which pleases me and others. Thanks for the support and best wishes, Pavel Pisa
