Re: [Intel-wired-lan] [PATCH net-next v2 02/15] idpf: add module register and probe functionality

[Leon Romanovsky <leon@kernel.org>]

On Thu, Apr 20, 2023 at 11:13:09AM -0700, Tantilov, Emil S wrote:
> 
> 
> On 4/12/2023 4:10 PM, Tantilov, Emil S wrote:
> > 
> > 
> > On 4/11/2023 5:36 AM, Leon Romanovsky wrote:
> > > On Mon, Apr 10, 2023 at 06:13:41PM -0700, Pavan Kumar Linga wrote:
> > > > From: Phani Burra <phani.r.burra@intel.com>
> > > > 
> > > > Add the required support to register IDPF PCI driver, as well as
> > > > probe and remove call backs. Enable the PCI device and request
> > > > the kernel to reserve the memory resources that will be used by the
> > > > driver. Finally map the BAR0 address space.
> > > > 
> > > > PCI IDs table is intentionally left blank to prevent the kernel from
> > > > probing the device with the incomplete driver. It will be added
> > > > in the last patch of the series.
> > > > 
> > > > Signed-off-by: Phani Burra <phani.r.burra@intel.com>
> > > > Co-developed-by: Alan Brady <alan.brady@intel.com>
> > > > Signed-off-by: Alan Brady <alan.brady@intel.com>
> > > > Co-developed-by: Madhu Chittim <madhu.chittim@intel.com>
> > > > Signed-off-by: Madhu Chittim <madhu.chittim@intel.com>
> > > > Co-developed-by: Shailendra Bhatnagar <shailendra.bhatnagar@intel.com>
> > > > Signed-off-by: Shailendra Bhatnagar <shailendra.bhatnagar@intel.com>
> > > > Co-developed-by: Pavan Kumar Linga <pavan.kumar.linga@intel.com>
> > > > Signed-off-by: Pavan Kumar Linga <pavan.kumar.linga@intel.com>
> > > > Reviewed-by: Sridhar Samudrala <sridhar.samudrala@intel.com>
> > > > Reviewed-by: Willem de Bruijn <willemb@google.com>
> > > > ---
> > > >   drivers/net/ethernet/intel/Kconfig            | 11 +++
> > > >   drivers/net/ethernet/intel/Makefile           |  1 +
> > > >   drivers/net/ethernet/intel/idpf/Makefile      | 10 ++
> > > >   drivers/net/ethernet/intel/idpf/idpf.h        | 27 ++++++
> > > >   .../net/ethernet/intel/idpf/idpf_controlq.h   | 14 +++
> > > >   drivers/net/ethernet/intel/idpf/idpf_lib.c    | 96 +++++++++++++++++++
> > > >   drivers/net/ethernet/intel/idpf/idpf_main.c   | 70 ++++++++++++++
> > > >   7 files changed, 229 insertions(+)
> > > >   create mode 100644 drivers/net/ethernet/intel/idpf/Makefile
> > > >   create mode 100644 drivers/net/ethernet/intel/idpf/idpf.h
> > > >   create mode 100644 drivers/net/ethernet/intel/idpf/idpf_controlq.h
> > > >   create mode 100644 drivers/net/ethernet/intel/idpf/idpf_lib.c
> > > >   create mode 100644 drivers/net/ethernet/intel/idpf/idpf_main.c
> > > 
> > > <...>
> > > > +}
> > > > +
> > > > +/* idpf_pci_tbl - PCI Dev idpf ID Table
> > > > + */
> > > > +static const struct pci_device_id idpf_pci_tbl[] = {
> > > > +    { /* Sentinel */ }
> > > 
> > > What does it mean empty pci_device_id table?
> > 
> > Device ID's are added later, but it does make sense to be in this patch
> > instead. Will fix in v3.
> 
> On second look, the reason PCI ids are added in the last patch is because
> none of the modules from the previous patches would result in a functional
> driver.

And yet patches should be split to meaningful and logical chunks. If
you add pci_device_id, please add relevant device at the same patch.

Thanks

> 
> Thanks,
> Emil