Re: [PATCH v3 17/17] PCI: dwc: Add Baikal-T1 PCIe controller support

[Bjorn Helgaas]

On Tue, Jun 28, 2022 at 03:23:56PM +0300, Serge Semin wrote:
> Bjorn,
> Do you have anything to say based on the notes below?

I'm focused on the first series for now.

> On Wed, Jun 22, 2022 at 08:04:37PM +0300, Serge Semin wrote:
> > On Tue, Jun 21, 2022 at 01:29:41PM -0500, Bjorn Helgaas wrote:
> > > On Mon, Jun 20, 2022 at 08:13:47PM +0300, Serge Semin wrote:
> > > > On Wed, Jun 15, 2022 at 11:48:48AM -0500, Bjorn Helgaas wrote:
> > > > > On Fri, Jun 10, 2022 at 11:57:05AM +0300, Serge Semin wrote:

> > > > > > +struct dw_pcie_ops bt1_pcie_dw_ops = {
> > > > > > +	.read_dbi = bt1_pcie_read_dbi,
> > > > > > +	.write_dbi = bt1_pcie_write_dbi,
> > > > > > +	.write_dbi2 = bt1_pcie_write_dbi2,
> > > > > > +	.start_link = bt1_pcie_start_ltssm,
> > > > > > +	.stop_link = bt1_pcie_stop_ltssm,
> > > > > > +};
> > > 
> > > > > Please rename to "dw_pcie_ops" as most drivers use. 
> > > > 
> > > > IMO matching the structure and its instance names is not a good idea.
> > > > Other than confusing objects nature, at the very least it forces you to
> > > > violate the local namespace convention. Thus in the line of the
> > > > dw_pcie->ops initialization it looks like you use some generic
> > > > operations while in fact you just refer to the locally defined
> > > > DW PCIe ops instance with the generic variable name. Moreover AFAICS
> > > > the latest platform drivers mainly use the vendor-specific prefix in
> > > > the dw_pcie_ops structure instance including the ones acked by you,
> > > > Lorenzo and Gustavo. What makes my code any different from them?
> > 
> > > That's fair.  I suggest "bt1_pcie_ops" or "bt1_dw_pcie_ops" to match
> > > the other drivers that include the driver name:
> > > 
> > >   intel_pcie_ops
> > >   keembay_pcie_ops
> > >   kirin_dw_pcie_ops
> > >   tegra_dw_pcie_ops
> > 
> > +   ks_pcie_dw_pcie_ops
> > 
> > which is even further from the suggested names.)

As I said, they're not 100% consistent, but they almost all end in
"pcie_ops".  Yours ends in "pcie_dw_ops", which is why I suggested
renaming to be similar to the others.

I don't want to make a huge deal about this, but I do want as much
similarity across drivers as possible.  I get that it doesn't benefit
*you*, but it's a huge benefit to everybody else.

> > > They're not 100% consistent, but hopefully we can at least not make
> > > things *less* consistent.
> > 
> > I don't think we can make something less consistent if there is no real
> > consistency.) There are at most five ops descriptors can be defined in
> > the DW PCIe platform drivers: