> -----Original Message-----
> From: Bjorn Helgaas <helgaas@xxxxxxxxxx>
> Sent: Wednesday, September 6, 2023 10:38 PM
> To: D M, Sharath Kumar <sharath.kumar.d.m@xxxxxxxxx>
> Cc: lpieralisi@xxxxxxxxxx; kw@xxxxxxxxx; robh@xxxxxxxxxx;
> bhelgaas@xxxxxxxxxx; linux-pci@xxxxxxxxxxxxxxx; dinguyen@xxxxxxxxxx;
> linux-kernel@xxxxxxxxxxxxxxx
> Subject: Re: [PATCH v2 1/2] PCI: altera: refactor driver for supporting new
> platform
>
> Capitalize subject line to match history:
>
> PCI: altera: Refactor driver for supporting new platform
>
> On Wed, Sep 06, 2023 at 04:39:17PM +0530, sharath.kumar.d.m@xxxxxxxxx
> wrote:
> > From: D M Sharath Kumar <sharath.kumar.d.m@xxxxxxxxx>
>
> Needs a commit log. It's OK to repeat the subject line. Also helpful if you can
> hint about why it needs to be refactored. In this case, it looks like you'll need
> to have a different ISR for Agilex, and also something different about root
> bus vs downstream config accesses.
>
ok
> > @@ -100,10 +101,15 @@ struct altera_pcie_ops {
> > void (*tlp_write_pkt)(struct altera_pcie *pcie, u32 *headers,
> > u32 data, bool align);
> > bool (*get_link_status)(struct altera_pcie *pcie);
> > - int (*rp_read_cfg)(struct altera_pcie *pcie, int where,
> > - int size, u32 *value);
> > + int (*rp_read_cfg)(struct altera_pcie *pcie, u8 busno,
> > + unsigned int devfn, int where, int size, u32 *value);
> > int (*rp_write_cfg)(struct altera_pcie *pcie, u8 busno,
> > - int where, int size, u32 value);
> > + unsigned int devfn, int where, int size, u32 value);
> > + int (*ep_read_cfg)(struct altera_pcie *pcie, u8 busno,
> > + unsigned int devfn, int where, int size, u32 *value);
> > + int (*ep_write_cfg)(struct altera_pcie *pcie, u8 busno,
> > + unsigned int devfn, int where, int size, u32 value);
>
> "ep_read_cfg" isn't the ideal name because it suggests "endpoint", but it may
> be either an endpoint or a switch upstream port. The rockchip driver uses
> "other", which isn't super descriptive either but might be better.
>
Ok will change to "nonrp_read_cfg" ?
> > + void (*rp_isr)(struct irq_desc *desc);
> > };
>
> > +static int _altera_pcie_cfg_read(struct altera_pcie *pcie, u8 busno,
> > + unsigned int devfn, int where, int size,
> > + u32 *value)
> > +{
> > + if (busno == pcie->root_bus_nr && pcie->pcie_data->ops-
> >rp_read_cfg)
> > + return pcie->pcie_data->ops->rp_read_cfg(pcie, busno,
> devfn,
> > + where, size, value);
>
> Several other drivers use pci_is_root_bus() instead of keeping track of
> root_bus_nr and watching for changes to it. Maybe simpler and more
> reliable? That would be best as a separate patch all by itself if you go that
> direction.
>
Will not take up as part of this commit
> > +
> > + if (pcie->pcie_data->ops->ep_read_cfg)
> > + return pcie->pcie_data->ops->ep_read_cfg(pcie, busno,
> devfn,
> > + where, size, value);
> > + return PCIBIOS_FUNC_NOT_SUPPORTED;
> > +}
