> > Auxiliary driver registration.
> > Net device_ops registration but open/stop are empty stubs.
> > tx/rx logic.
>
> Take care for spelling: Tx/Rx; HW (just below).
Please elaborate. Spelling of what?
>
> >
> > All major data structures of the driver are fully introduced with the
> > code that uses them but without their initialization code that requires
> > management interface with the hw.
> >
> > Submitted-by: Gur Stavi <gur.stavi@xxxxxxxxxx>
>
> this tag is not needed
>
> > Signed-off-by: Gur Stavi <gur.stavi@xxxxxxxxxx>
>
> your tag should be last
>
> > Signed-off-by: Xin Guo <guoxin09@xxxxxxxxxx>
>
> no idea what Xin did, if you want to give credit to someone
> you could use Co-developed-by: tag (put it just above the SoB)
>
> > Signed-off-by: gongfan <gongfan1@xxxxxxxxxx>
>
> It would be much appricieated if you will spell the full name
> for gongfan, at least two parts (like you did for the rest),
> especially for someone you put into the MAINTAINERS file.
> OTOH, please keep in mind that maintainer should be active
> on the mailing list (regarding this driver).
>
Ack
> > +
> > +Global function ID
> > +------------------
> > +
> > +Every function, PF or VF, has a unique ordinal identification within the device.
> > +Many commands to management (mbox or cmdq) contain this ID so HW can apply the
>
> s/commands to management/management commands/
>
Ack
> > +L: netdev@xxxxxxxxxxxxxxx
> > +S: Supported
>
> there is a (relatively new) requirement that the device needs to report
> to the netdev CI to have an "S" here, so for now you should use "M"
>
Ack
> > + # Fields of HW and management structures are little endian and are
> > + # currently not converted
> > + depends on !CPU_BIG_ENDIAN
> > + depends on X86 || ARM64 || COMPILE_TEST
> > + depends on PCI_MSI && 64BIT
> > + select AUXILIARY_BUS
> > + help
> > + This driver supports HiNIC PCIE Ethernet cards.
> > + To compile this driver as part of the kernel, choose Y here.
> > + If unsure, choose N.
> > + The default is N.
>
> the last 3 lines are very much obvious/redundant
>
Ack
> > diff --git a/drivers/net/ethernet/huawei/hinic3/Makefile b/drivers/net/ethernet/huawei/hinic3/Makefile
> > new file mode 100644
> > index 000000000000..02656853f629
> > --- /dev/null
> > +++ b/drivers/net/ethernet/huawei/hinic3/Makefile
> > @@ -0,0 +1,21 @@
> > +# SPDX-License-Identifier: GPL-2.0
> > +# Copyright (c) Huawei Technologies Co., Ltd. 2024. All rights reserved.
>
> too late
>
Ack
> > +
> > +obj-$(CONFIG_HINIC3) += hinic3.o
> > +
> > +hinic3-objs := hinic3_hwdev.o \
> > + hinic3_lld.o \
> > + hinic3_common.o \
> > + hinic3_hwif.o \
> > + hinic3_hw_cfg.o \
> > + hinic3_queue_common.o \
> > + hinic3_mbox.o \
> > + hinic3_hw_comm.o \
> > + hinic3_wq.o \
> > + hinic3_nic_io.o \
> > + hinic3_nic_cfg.o \
> > + hinic3_tx.o \
> > + hinic3_rx.o \
> > + hinic3_netdev_ops.o \
> > + hinic3_rss.o \
> > + hinic3_main.o
>
> in general, any list of random things could be sorted (to ease out git
> merges in the future)
>
Ack
> > + (((u32)(hwdev)->cfg_mgmt->svc_cap.chip_svc_type) & BIT(HINIC3_SERVICE_T_NIC))
> > +
> > +bool hinic3_support_nic(struct hinic3_hwdev *hwdev)
> > +{
> > + if (!IS_NIC_TYPE(hwdev))
> > + return false;
> > +
> > + return true;
>
> this is just:
> return IS_NIC_TYPE(hwdev);
>
Ack
> > +static int comm_msg_to_mgmt_sync(struct hinic3_hwdev *hwdev, u16 cmd, const void *buf_in,
> > + u32 in_size, void *buf_out, u32 *out_size)
> > +{
> > + return hinic3_send_mbox_to_mgmt(hwdev, HINIC3_MOD_COMM, cmd, buf_in,
> > + in_size, buf_out, out_size, 0);
> > +}
> > +
> > +int hinic3_func_reset(struct hinic3_hwdev *hwdev, u16 func_id, u64 reset_flag)
> > +{
> > + struct comm_cmd_func_reset func_reset;
>
> = {}
>
> > + u32 out_size = sizeof(func_reset);
> > + int err;
> > +
> > + memset(&func_reset, 0, sizeof(func_reset));
>
> with "= {}" above, memset() could be eliminated
>
> > + func_reset.func_id = func_id;
> > + func_reset.reset_flag = reset_flag;
>
> alternative would be
> struct comm_cmd_func_reset func_reset = {
> .func_id = func_id,
> .reset_flag = reset_flag,
> };
>
Ack
> > +++ b/drivers/net/ethernet/huawei/hinic3/hinic3_hw_intf.h
> > @@ -0,0 +1,88 @@
> > +/* SPDX-License-Identifier: GPL-2.0 */
> > +/* Copyright (c) Huawei Technologies Co., Ltd. 2024. All rights reserved. */
> > +
> > +#ifndef HINIC3_HW_INTF_H
>
> please add two underscores to the header guards
>
Ack
> > +#define HINIC3_HW_INTF_H
