Re: [PATCH v6 4/6] can: ctucanfd: CTU CAN FD open-source IP core - PCI bus support.

[Pavel Pisa]

Hello Pavel,

thanks for review.

On Thursday 22 of October 2020 13:39:52 Pavel Machek wrote:
> Hi!
>
> > @@ -12,4 +12,13 @@ config CAN_CTUCANFD
> >
> >  if CAN_CTUCANFD
> >
> > +config CAN_CTUCANFD_PCI
> > +	tristate "CTU CAN-FD IP core PCI/PCIe driver"
> > +	depends on PCI
> > +	help
> > +	  This driver adds PCI/PCIe support for CTU CAN-FD IP core.
> > +	  The project providing FPGA design for Intel EP4CGX15 based DB4CGX15
> > +	  PCIe board with PiKRON.com designed transceiver riser shield is
> > available +	  at https://gitlab.fel.cvut.cz/canbus/pcie-ctu_can_fd .
> > +
> >  endif
>
> Ok, now the if in the first patch makes sense. It can stay.
>
> And it is separate module, so EXPORT_SYMBOLs make sense. Ok.

Great.

> > +#ifndef PCI_VENDOR_ID_TEDIA
> > +#define PCI_VENDOR_ID_TEDIA 0x1760
> > +#endif
> >
> > +#define PCI_DEVICE_ID_ALTERA_CTUCAN_TEST  0xCAFD
> > +#define PCI_DEVICE_ID_TEDIA_CTUCAN_VER21 0xff00
>
> These should go elsewhere.

They should propagate somehow from

https://pci-ids.ucw.cz/read/PC/1760/ff00

We have registered them long time ago.
I am not sure what is right mechanism.

> > +#ifndef PCI_VENDOR_ID_TEDIA
> > +#define PCI_VENDOR_ID_TEDIA 0x1760
> > +#endif

So this one should be known to kernel globally, but I would
be happy if driver build even if global process to introduce
define did not proceed end even backports would be required
for long time until kernel including CTU CAN FD propagates
into distributions, and industrial systems distributions
lag often a lot

> > +#define PCI_DEVICE_ID_ALTERA_CTUCAN_TEST  0xCAFD

We drop this, I hope we have no system running old test
version of the core integration before Tedia offered us
to reserve some IDs (promissed that they would never use them
in future) for us.

> > +#define PCI_DEVICE_ID_TEDIA_CTUCAN_VER21 0xff00

This should propagate into kernel from registry or at least
match registry.

> > +static bool use_msi = 1;
> > +static bool pci_use_second = 1;
>
> true?

Done

Best wishes,

                Pavel