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; > > -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 > >
