Re: [PATCH v6 4/7] can: Add Nuvoton NCT6694 CANFD support

[Vincent Mailhol]

On 04/02/2025 at 12:24, Ming Yu wrote:
> Dear Vincent,
> 
> Thank you for reviewing,
> I will address the issues you mentioned in the next patch.
> 
> Vincent Mailhol <mailhol.vincent@xxxxxxxxxx> 於 2025年1月26日 週日 下午4:47寫道：
>>
> ...
>>> +static int nct6694_can_get_clock(struct nct6694_can_priv *priv)
>>> +{
>>> +     struct nct6694_can_information *info;
>>> +     struct nct6694_cmd_header cmd_hd = {
>>
>> If the variable only has constant initializer, make it static const:
>>
>>         static const struct nct6694_cmd_header cmd_hd = {
>>
>> Apply this at other locations in your different modules.
>>
>>> +             .mod = NCT6694_CAN_MOD,
>>> +             .cmd = NCT6694_CAN_INFORMATION,
>>> +             .sel = NCT6694_CAN_INFORMATION_SEL,
>>> +             .len = cpu_to_le16(sizeof(*info))
>>> +     };
>>> +     int ret, can_clk;
>>> +
>>> +     info = kzalloc(sizeof(*info), GFP_KERNEL);
>>> +     if (!info)
>>> +             return -ENOMEM;
>>> +
> 
> Excuse me, I would like to confirm, if the variable is constant
> initializer, should the declaration be written as:
> static const struct nct6694_cmd_header cmd_hd = {
>     .mod = NCT6694_CAN_MOD,
>     .cmd = NCT6694_CAN_INFORMATION,
>     .sel = NCT6694_CAN_INFORMATION_SEL,
>     .len = cpu_to_le16(sizeof(struct nct6694_can_information))
> };
> instead of:
> static const struct nct6694_cmd_header cmd_hd = {
>     .mod = NCT6694_CAN_MOD,
>     .cmd = NCT6694_CAN_INFORMATION,
>     .sel = NCT6694_CAN_INFORMATION_SEL,
>     .len = cpu_to_le16(sizeof(*info))
> };
> , correct?

The sizeof() keyword returns a integer constant expression even if
applied on to a variable (unless that variable is a variable length
array, but these are banned in the kernel anyway). This is because only
the type of the variable matters, and that is known at compile time.

So, cpu_to_le16(sizeof(*info)) should work fine. Or are you getting any
error?

> In addition, does this mean that the parameter in nct6694_read_msg()
> and nct6694_write_msg() should be changed to const struct
> nct6694_cmd_header *cmd_hd?

Yes!


Yours sincerely,
Vincent Mailhol