On Sat, Jan 30, 2021 at 4:53 AM Hariprasad Kelam <hke...@marvell.com> wrote:
>
> Hi Willem,
>
> > -----Original Message-----
> > From: Willem de Bruijn <willemdebruijn.ker...@gmail.com>
> > Sent: Thursday, January 28, 2021 1:50 AM
> > To: Hariprasad Kelam <hke...@marvell.com>
> > Cc: Network Development <net...@vger.kernel.org>; LKML <linux-
> > ker...@vger.kernel.org>; David Miller <da...@davemloft.net>; Jakub
> > Kicinski <k...@kernel.org>; Sunil Kovvuri Goutham
> > <sgout...@marvell.com>; Linu Cherian <lcher...@marvell.com>;
> > Geethasowjanya Akula <gak...@marvell.com>; Jerin Jacob Kollanukkaran
> > <jer...@marvell.com>; Subbaraya Sundeep Bhatta <sbha...@marvell.com>;
> > Felix Manlunas <fmanlu...@marvell.com>; Christina Jacob
> > <cja...@marvell.com>; Sunil Kovvuri Goutham
> > <sunil.gout...@cavium.com>
> > Subject: [EXT] Re: [Patch v2 net-next 2/7] octeontx2-af: Add new CGX_CMD
> > to get PHY FEC statistics
> >
> > On Wed, Jan 27, 2021 at 4:04 AM Hariprasad Kelam <hke...@marvell.com>
> > wrote:
> > >
> > > From: Felix Manlunas <fmanlu...@marvell.com>
> > >
> > > This patch adds support to fetch fec stats from PHY. The stats are put
> > > in the shared data struct fwdata. A PHY driver indicates that it has
> > > FEC stats by setting the flag fwdata.phy.misc.has_fec_stats
> > >
> > > Besides CGX_CMD_GET_PHY_FEC_STATS, also add CGX_CMD_PRBS and
> > > CGX_CMD_DISPLAY_EYE to enum cgx_cmd_id so that Linux's enum list is in
> > > sync with firmware's enum list.
> > >
> > > Signed-off-by: Felix Manlunas <fmanlu...@marvell.com>
> > > Signed-off-by: Christina Jacob <cja...@marvell.com>
> > > Signed-off-by: Sunil Kovvuri Goutham <sunil.gout...@cavium.com>
> > > Signed-off-by: Hariprasad Kelam <hke...@marvell.com>
> >
> >
> > > +struct phy_s {
> > > + struct {
> > > + u64 can_change_mod_type : 1;
> > > + u64 mod_type : 1;
> > > + u64 has_fec_stats : 1;
> >
> > this style is not customary
>
> These structures are shared with firmware and stored in a shared memory. Any
> change in size of structures will break compatibility. To avoid frequent
> compatible issues with new vs old firmware we have put spaces where ever we
> see that there could be more fields added in future.
> So changing this to u8 can have an impact in future.
My comment was intended much simpler: don't add whitespace between the
bit-field variable name and its size expression.
u64 mod_type:1;
not
u64 mod_type : 1;
At least, I have not seen that style anywhere else in the kernel.
