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

[netdev-bot+sashiko]

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] The new private header
  drivers/net/ethernet/oa_tc6/oa_tc6_std_def.h doesn't compile on its…

--

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

This patch moves the control/data header definitions, size constants,
enums and struct oa_tc6 out of oa_tc6.c into a new private header,
drivers/net/ethernet/oa_tc6/oa_tc6_std_def.h. Other source files in the
OA TC6 framework can then share them.

> 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..403a9c22b5f14
> --- /dev/null
> +++ b/drivers/net/ethernet/oa_tc6/oa_tc6_std_def.h
> @@ -0,0 +1,125 @@

[ ... ]

> +#ifndef OA_TC6_STD_DEF_H
> +#define OA_TC6_STD_DEF_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/phy.h>

[ ... ]

> +/* Internal structure for MAC-PHY drivers */
> +struct oa_tc6 {

[ ... ]

> +	bool prot_ctrl;
> +	enum oa_tc6_quirk_flag quirk_flags;
> +	struct gpio_desc *reset_gpio;
> +};

[Severity: Low]
This isn't a bug, but is this header meant to be self-contained?

struct oa_tc6 has quirk_flags as a by-value member of type
enum oa_tc6_quirk_flag. That enum is defined only in
include/linux/oa_tc6.h:

include/linux/oa_tc6.h:
enum oa_tc6_quirk_flag {
	OA_TC6_BROKEN_PHY = BIT(0),
};

oa_tc6_std_def.h doesn't include <linux/oa_tc6.h>. It compiles today
only because oa_tc6.c includes <linux/oa_tc6.h> right before it:

#include <linux/oa_tc6.h>

#include "oa_tc6_std_def.h"

Any new file that includes "oa_tc6_std_def.h" first would fail to build.

At this commit include/linux/oa_tc6.h has no include guard, so adding
the include here would also need a guard there. The later patch "net:
ethernet: oa_tc6: Support for hardware timestamp" adds _LINUX_OA_TC6_H.
Could this patch add the guard and include <linux/oa_tc6.h> directly
from oa_tc6_std_def.h?

A smaller point: nothing in this patch seems to use
<linux/ptp_clock_kernel.h>, <linux/net_tstamp.h>, <linux/sched.h> or
<linux/wait.h>. They look like they are first needed by the later
hardware timestamp patch.

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20261009-s2500-mac-phy-support-v9-0-dcefe1d0bf0d%40onsemi.com