Re: [PATCH v2 01/24] mpi3mr: add mpi30 Rev-R headers and Kconfig

Bart Van Assche
2021-04-16T15:40:07Z

On 4/16/21 3:53 AM, Kashyap Desai wrote:
[ ... ]
>>> +#ifndef MPI3_POINTER
>>> +#define MPI3_POINTER *
>>> +#endif /* MPI3_POINTER */
>>
>> As far as I know there are no far pointers in the Linux kernel.
>
> Hi Bart - I can remove the comment and just use below line in this file -
> #define MPI3_POINTER *
>
> Common MPI header file which is directly derived from common repository
> within a Broadcom like " mpi30_cnfg.h", " mpi30_transport.h" etc. has
> reference of MPI3_POINTER instead of directly using "*" there.
> It may be useful for some third party development (like SDK) and they can
> replace MPI3_POINTER accordingly. Only mpi30_types.h is Linux only file and
> we add wrapper in this file to make sure common header file compiles on
> Linux instead of completely replacing whole MPI header file.

How about changing all MPI3_POINTER occurrences into MPI3_POINTER_ATTR *
and defining MPI3_POINTER_ATTR as an empty macro in this header file?

>>> +typedef u8 U8;
>>> +typedef __le16 U16;
>>
>> Introducing U16 as an alias for __le16 is confusing since there is already
>> an
>> 'u16' type in the Linux kernel. Please use the __le* types directly.
>
> I explain this issue in above comment.

I'm not sure that explanation is sufficient. Has it been considered to
change all U16 occurrences into __le16 and to add 'typedef uint16_t
__le16;' in the appropriate header file if building for another
operating system than Linux?

>>> +typedef U8 * PU8;
>>> +typedef U16 * PU16;
>>> +typedef U32 * PU32;
>>> +typedef U64 * PU64;
>>
>> Please use __le* directly instead of introducing the above aliases.
>> Please also follow the Linux kernel coding style (no space after '*').
>
> There is no reference in mpi3mr driver to use above defines. I will remove
> them completely.

Thanks!

Bart.