Re: [PATCH v11 3/4] phy: Add Sparx5 ethernet serdes PHY driver

On Mon, Jan 04, 2021 at 02:16:38PM +0100, Steen Hegelund wrote:

Hi Leon,


On Mon, 2021-01-04 at 14:15 +0200, Leon Romanovsky wrote:

quoted

<...>

quoted

+struct sparx5_sd10g28_args {
+     bool                 skip_cmu_cfg; /* Enable/disable CMU cfg
*/
+     bool                 no_pwrcycle;  /* Omit initial power-
cycle */
+     bool                 txinvert;     /* Enable inversion of
output data */
+     bool                 rxinvert;     /* Enable inversion of
input data */
+     bool                 txmargin;     /* Set output level to 
half/full */
+     u16                  txswing;      /* Set output level */
+     bool                 mute;         /* Mute Output Buffer */
+     bool                 is_6g;
+     bool                 reg_rst;
+};

All those bools in structs can be squeezed into one u8, just use
bitfields, e.g. "u8 a:1;".

Got you.

quoted

Also I strongly advise do not do vertical alignment, it will cause to
too many churn later when this code will be updated.

So just a single space between the type and the name and the comment is
preferable?

Yes, use clang formatter over your code, it will change everything to be
aligned to kernel coding style.
https://linuxplumbersconf.org/event/7/contributions/803/

quoted

quoted

+

<...>

quoted

+static inline void __iomem *sdx5_addr(void __iomem *base[],
+                                   int id, int tinst, int tcnt,
+                                   int gbase, int ginst,
+                                   int gcnt, int gwidth,
+                                   int raddr, int rinst,
+                                   int rcnt, int rwidth)
+{
+#if defined(CONFIG_DEBUG_KERNEL)
+     WARN_ON((tinst) >= tcnt);
+     WARN_ON((ginst) >= gcnt);
+     WARN_ON((rinst) >= rcnt);
+#endif

Please don't put "#if defined(CONFIG_DEBUG_KERNEL)", print WARN_ON().

OK, I will drop the #if and keep the WARN_ON...

Thanks

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel