On 11/3/20 11:00 AM, Pavel Pisa wrote:

On Saturday 31 of October 2020 12:35:11 Marc Kleine-Budde wrote:

quoted

On 10/30/20 11:19 PM, Pavel Pisa wrote:

quoted

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.

I used the lastest one from linus/master :)

quoted

-----------------------------------------
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.

quoted

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.

confirmed :)

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.

It's probably historically related :)

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.

This is another point I wanted to address. Obviously checkpatch doesn't complain
about bitfields, but it's frowned upon.

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

The current Linux way is to define bitmask with GENMASK() and single bit mask
with BIT().

For example the mcp251xfd driver:

First the register offset:

#define MCP251XFD_REG_CON 0x00

Then a bitmask:

#define MCP251XFD_REG_CON_TXBWS_MASK GENMASK(31, 28)

And a single bit:

#define MCP251XFD_REG_CON_ABAT BIT(27)

see:
https://elixir.bootlin.com/linux/v5.10-rc2/source/drivers/net/can/spi/mcp251xfd/mcp251xfd.h#L24

The masks are used with FIELD_GET, FIELD_PREP.

For example:
https://elixir.bootlin.com/linux/v5.10-rc2/source/drivers/net/can/spi/mcp251xfd/mcp251xfd-core.c#L1386

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.

Hope that helps,
Marc

-- 
Pengutronix e.K.                 | Marc Kleine-Budde           |
Embedded Linux                   | https://www.pengutronix.de  |
Vertretung West/Dortmund         | Phone: +49-231-2826-924     |
Amtsgericht Hildesheim, HRA 2686 | Fax:   +49-5121-206917-5555 |