On Mon, Mar 06, 2023 at 09:31:54PM +0800, Cai Huoqing wrote: > On 03 3月 23 19:51:25, Serge Semin wrote: > > On Fri, Mar 03, 2023 at 08:46:31PM +0800, Cai Huoqing wrote: > > > From: Cai huoqing <cai.huoqing@xxxxxxxxx> > > > > > > > > Rename dw_edma_core_ops structure to dw_edma_plat_ops, the ops is platform > > > specific operations: the DMA device environment configs like IRQs, > > > address translation, etc. > > > > > > The dw_edma_plat_ops name was supposed to refer to the platform which > > > the DW eDMA engine is embedded to, like PCIe end-point (accessible via > > > the PCIe bus) or a PCIe root port (directly accessible by CPU). > > > Needless to say that for them the IRQ-vector and PCI-addresses are > > > differently determined. The suggested name has a connection with the > > > kernel platform device only as a private case of the eDMA/hDMA embedded > > > into the DW PCI Root ports, though basically it was supposed to refer to > > > any platform in which the DMA hardware lives. > > > > > > Anyway the renaming was necessary to distinguish two types of > > > the implementation callbacks: > > > 1. DW eDMA/hDMA IP-core specific operations: device-specific CSR > > > setups in one or another aspect of the DMA-engine initialization. > > > 2. DW eDMA/hDMA platform specific operations: the DMA device > > > environment configs like IRQs, address translation, etc. > > > > > > dw_edma_core_ops is supposed to be used for the case 1, and > > > dw_edma_plat_ops - for the case 2. > > > > This text was my explanation to Bjorn of why the renaming was > > necessary. The patch log has a bit different context so I would > > change it to something like this: > > > > "The dw_edma_core_ops structure contains a set of the operations: > > device IRQ numbers getter, CPU/PCI address translation. Based on the > > functions semantics the structure name "dw_edma_plat_ops" looks more > > descriptive since indeed the operations are platform-specific. The > > "dw_edma_core_ops" name shall be used for a structure with the IP-core > > specific set of callbacks in order to abstract out DW eDMA and DW HDMA > > setups. Such structure will be added in one of the next commit in the > > framework of the set of changes adding the DW HDMA device support." > OK > > > > > > > > > Signed-off-by: Cai huoqing <cai.huoqing@xxxxxxxxx> > > > --- > > > v4->v5: > > > 1.Revert the instance dw_edma_pcie_core_ops > > > 2.Move the change EDMA_MF_HDMA_NATIVE to patch[3/4] > > > > > > v4 link: > > > https://lore.kernel.org/lkml/20230221034656.14476-2-cai.huoqing@xxxxxxxxx/ > > > > > > drivers/dma/dw-edma/dw-edma-pcie.c | 2 +- > > > drivers/pci/controller/dwc/pcie-designware.c | 2 +- > > > include/linux/dma/edma.h | 4 ++-- > > > 3 files changed, 4 insertions(+), 4 deletions(-) > > > > > > diff --git a/drivers/dma/dw-edma/dw-edma-pcie.c b/drivers/dma/dw-edma/dw-edma-pcie.c > > > index 2b40f2b44f5e..190b32d8016d 100644 > > > --- a/drivers/dma/dw-edma/dw-edma-pcie.c > > > +++ b/drivers/dma/dw-edma/dw-edma-pcie.c > > > @@ -109,7 +109,7 @@ static u64 dw_edma_pcie_address(struct device *dev, phys_addr_t cpu_addr) > > > return region.start; > > > } > > > > > > > > -static const struct dw_edma_core_ops dw_edma_pcie_core_ops = { > > > +static const struct dw_edma_plat_ops dw_edma_pcie_core_ops = { > > > > Please carefully note my comment to v4. I asked to add the prefix > > specific to the local naming convention: > > > > -static const struct dw_edma_core_ops dw_edma_pcie_core_ops = { > > +static const struct dw_edma_plat_ops dw_edma_pcie_plat_ops = { > > > > But besides of adding the correct prefix you changed the suffix to the > > improper one. Please get it back so the instance name would be > > "dw_edma_pcie_plat_ops". > Do you mean ditto > - chip->ops = &dw_edma_pcie_core_ops; > + chip->ops = &dw_edma_pcie_plat_ops; Yes, please. -Serge(y) > > > > > -Serge(y) > > > > > .irq_vector = dw_edma_pcie_irq_vector, > > > .pci_address = dw_edma_pcie_address, > > > }; > > > diff --git a/drivers/pci/controller/dwc/pcie-designware.c b/drivers/pci/controller/dwc/pcie-designware.c > > > index 53a16b8b6ac2..44e90b71d429 100644 > > > --- a/drivers/pci/controller/dwc/pcie-designware.c > > > +++ b/drivers/pci/controller/dwc/pcie-designware.c > > > @@ -828,7 +828,7 @@ static int dw_pcie_edma_irq_vector(struct device *dev, unsigned int nr) > > > return platform_get_irq_byname_optional(pdev, name); > > > } > > > > > > -static struct dw_edma_core_ops dw_pcie_edma_ops = { > > > +static struct dw_edma_plat_ops dw_pcie_edma_ops = { > > > .irq_vector = dw_pcie_edma_irq_vector, > > > }; > > > > > > diff --git a/include/linux/dma/edma.h b/include/linux/dma/edma.h > > > index d2638d9259dc..ed401c965a87 100644 > > > --- a/include/linux/dma/edma.h > > > +++ b/include/linux/dma/edma.h > > > @@ -40,7 +40,7 @@ struct dw_edma_region { > > > * iATU windows. That will be done by the controller > > > * automatically. > > > */ > > > -struct dw_edma_core_ops { > > > +struct dw_edma_plat_ops { > > > int (*irq_vector)(struct device *dev, unsigned int nr); > > > u64 (*pci_address)(struct device *dev, phys_addr_t cpu_addr); > > > }; > > > @@ -80,7 +80,7 @@ enum dw_edma_chip_flags { > > > struct dw_edma_chip { > > > struct device *dev; > > > int nr_irqs; > > > - const struct dw_edma_core_ops *ops; > > > + const struct dw_edma_plat_ops *ops; > > > u32 flags; > > > > > > void __iomem *reg_base; > > > -- > > > 2.34.1 > > >
