On 11/3/20 11:00 AM, Pavel Pisa wrote: > 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. I used the lastest one from linus/master :) >> ----------------------------------------- >> 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. 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 |
Attachment:
signature.asc
Description: OpenPGP digital signature
