RE: [PATCH v11 2/2] dmaengine: dw-edma: Add non-LL mode

[Verma, Devendra]

[AMD Official Use Only - AMD Internal Distribution Only]

> -----Original Message-----
> From: Frank Li <Frank.li@xxxxxxx>
> Sent: Saturday, March 7, 2026 2:47 AM
> To: Verma, Devendra <Devendra.Verma@xxxxxxx>
> Cc: bhelgaas@xxxxxxxxxx; mani@xxxxxxxxxx; vkoul@xxxxxxxxxx;
> dmaengine@xxxxxxxxxxxxxxx; linux-pci@xxxxxxxxxxxxxxx; linux-
> kernel@xxxxxxxxxxxxxxx; Simek, Michal <michal.simek@xxxxxxx>
> Subject: Re: [PATCH v11 2/2] dmaengine: dw-edma: Add non-LL mode
>
> Caution: This message originated from an External Source. Use proper
> caution when opening attachments, clicking links, or responding.
>
>
> On Fri, Mar 06, 2026 at 05:22:28PM +0530, Devendra K Verma wrote:
> ...
> > +             /*
> > +              * When there is no valid LLP base address available then the
> > +              * default DMA ops will use the non-LL mode.
> > +              *
> > +              * Cases where LL mode is enabled and client wants to use the
> > +              * non-LL mode then also client can do so via providing the
> > +              * peripheral_config param.
> > +              */
> > +             cfg_non_ll = chan->dw->chip->cfg_non_ll;
> > +             if (config->peripheral_config) {
> > +                     non_ll = *(int *)config->peripheral_config;
> > +
> > +                     if (cfg_non_ll && !non_ll) {
> > +                             dev_err(dchan->device->dev, "invalid configuration\n");
> > +                             return -EINVAL;
> > +                     }
> > +             }
> > +
> > +             if (cfg_non_ll || (!cfg_non_ll && non_ll))
> > +                     chan->non_ll = true;
>
> this logic have a little bit complex
>
> if (cfg_non_ll)
>         chan->non_ll = true;
> else
>         chan->non_ll = non_ll;
>

Thank you for your suggestion.
I think it is individual preference. I am not sure what seem to be complex in the
logic floated for review as all the boolean operations are easy to comprehend
in a single statement.
I am sure there are multiple ways to write the same logic.
To me, the logic you suggested looks bigger with the same outcome delivered.
If after distinction in variable names and simple boolean ops still cause confusion
then I am not sure till what point it can be simplified.
If fewer lines of code can deliver the same result, then it should be ok to keep it.
I would request to keep the change of the floated review.
Thanks!

>
> > +     } else if (config->peripheral_config) {
> > +             dev_err(dchan->device->dev,
> > +                     "peripheral config param applicable only for HDMA\n");
> > +             return -EINVAL;
> > +     }
> >
> ...
> >
> >  struct dw_edma_irq {
> > diff --git a/drivers/dma/dw-edma/dw-edma-pcie.c
> > b/drivers/dma/dw-edma/dw-edma-pcie.c
> > index b8208186a250..f538d728609f 100644
> > --- a/drivers/dma/dw-edma/dw-edma-pcie.c
> > +++ b/drivers/dma/dw-edma/dw-edma-pcie.c
> > @@ -295,6 +295,15 @@ static void
> dw_edma_pcie_get_xilinx_dma_data(struct pci_dev *pdev,
> >       pdata->devmem_phys_off = off;
> >  }
> >
> > +static u64 dw_edma_get_phys_addr(struct pci_dev *pdev,
> > +                              struct dw_edma_pcie_data *pdata,
> > +                              enum pci_barno bar) {
> > +     if (pdev->vendor == PCI_VENDOR_ID_XILINX)
> > +             return pdata->devmem_phys_off;
> > +     return pci_bus_address(pdev, bar); }
> > +
>
> Reduce each patches's changes, make each patch is straightforward
>
> Create Prepare patch firstly, change pci_bus_address() to
> dw_edma_get_phys_addr()
>
> just
>
> dw_edma_get_phys_addr() {
> {
>         return return pci_bus_address(pdev, bar); }
>
> So this patch just add
> two lines here
>
> if (pdev->vendor == PCI_VENDOR_ID_XILINX)
>         return pdata->devmem_phys_off;
>
>
> others look good.
>
> Frank
>

Regarding this we already had discussion and it was concluded to let this piece of code
to be as is. Please check the discussion at the following link:
https://lore.kernel.org/all/aXe5ts7E6lUF7YRq@lizhi-Precision-Tower-5810/

> >