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

[Kashyap Desai <kashyap.desai@broadcom.com>]

[-- Attachment #1: Type: text/plain, Size: 2586 bytes --]

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


Hi Bart - This is possible to modify, but I have to forward this feedback to
group who owns the MPI header within a Broadcom.
It will be difficult to accommodate requested to change in this series.

I have marked your feedback as TBD for upcoming driver update (not in
current patch set). In V3, this is not accommodated.  Hope this is OK with
you.

>
> >>> +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?

Same as above.

I agree with your concern and same is under discussion within Broadcom and
we will get back to linux community with outcome.
I am not sure how much we can fit considering it is widely used outside the
Linux so need some time. At very high level, I see your proposal should be
doable. Let me check.

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

[-- Attachment #2: S/MIME Cryptographic Signature --]
[-- Type: application/pkcs7-signature, Size: 4212 bytes --]