Re: [PATCH v7 0/6] CTU CAN FD open-source IP core SocketCAN driver, PCI, platform integration and documentation

[Pavel Pisa]

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