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

[Marc Kleine-Budde]

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