Re: [PATCH net-next v10 04/11] net: ethernet: oa_tc6: Move constant definitions to header file

[netdev-bot+sashiko@kernel.org]

Thank you for your contribution! Sashiko AI review found more severe
issue(s) in other patches of this series. This mail is only an FYI, in case
you want to address the nit picks below when you respin.

Low priority / nit picks for your consideration:

- [Low] Hygiene and commit-message problems in
  drivers/net/ethernet/oa_tc6/oa_tc6_std_def.h, which the commit…

--

net: ethernet: oa_tc6: Move constant definitions to header file

This commit moves the TC6 protocol constants, struct oa_tc6 and the related
enums out of oa_tc6_main.c and into a new private header, oa_tc6_std_def.h,
so other source files in the oa_tc6 directory can share them.
oa_tc6_main.c now includes the new header in place of linux/oa_tc6.h.

> To help other source files within the module share the
> constant definitions, some of the definitions from oa_tc6.c
> are moved to a header file.

[Severity: Low]
This isn't a bug, but the previous patch in the series ("net: ethernet:
oa_tc6: Move oa_tc6.c to its own directory") already renamed this file to
drivers/net/ethernet/oa_tc6/oa_tc6_main.c.

Should the commit message say oa_tc6_main.c instead of oa_tc6.c?

[ ... ]

> diff --git a/drivers/net/ethernet/oa_tc6/oa_tc6_std_def.h b/drivers/net/ethernet/oa_tc6/oa_tc6_std_def.h
> new file mode 100644
> index 0000000000000..c61672aa618c4
> --- /dev/null
> +++ b/drivers/net/ethernet/oa_tc6/oa_tc6_std_def.h
> @@ -0,0 +1,126 @@

[ ... ]

> +#ifndef OA_TC6_STD_DEF_H
> +#define OA_TC6_STD_DEF_H
> +
> +#include <linux/bits.h>
> +#include <linux/ptp_clock_kernel.h>
> +#include <linux/net_tstamp.h>
> +#include <linux/netdevice.h>
> +#include <linux/spi/spi.h>
> +#include <linux/skbuff.h>
> +#include <linux/sched.h>
> +#include <linux/wait.h>
> +#include <linux/oa_tc6.h>

[Severity: Low]
This isn't a bug, but does this header need ptp_clock_kernel.h,
net_tstamp.h, sched.h and wait.h yet? Nothing in oa_tc6_std_def.h uses them
at this commit. Also, struct oa_tc6 has no hwtstamp_config, ptp_clock or
list_head members in this patch.

They only seem to be needed once the later "net: ethernet: oa_tc6: Support
for hardware timestamp" patch adds those members.

Could these includes move to that patch so this one stays a pure move? If
not, could the commit message mention them?

[ ... ]

> +enum oa_tc6_data_end_valid_info {
> +	OA_TC6_DATA_END_INVALID,
> +	OA_TC6_DATA_END_VALID,
> +};
> +#endif /* OA_TC6_STD_DEF_H */
> +

[Severity: Low]
This isn't a bug, but there is an extra blank line after the #endif at the
end of the file. git apply warns "new blank line at EOF" and checkpatch flags
it. The blank line is still there at the end of the series.

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20261106-s2500-mac-phy-support-v10-0-31b5a69b85b7%40onsemi.com