Re: [PATCH v6 4/6] dmaengine: ae4dma: Register AE4DMA using pt_dmaengine_register

[Vinod Koul <vkoul@kernel.org>]

On 22-10-24, 10:53, Basavaraj Natikar wrote:
> 
> On 10/21/2024 11:02 PM, Vinod Koul wrote:
> > On 09-09-24, 18:09, Basavaraj Natikar wrote:
> > > Use the pt_dmaengine_register function to register a AE4DMA DMA engine.
> > > 
> > > Reviewed-by: Raju Rangoju <Raju.Rangoju@amd.com>
> > > Reviewed-by: Philipp Stanner <pstanner@redhat.com>
> > > Signed-off-by: Basavaraj Natikar <Basavaraj.Natikar@amd.com>
> > > ---
> > >   drivers/dma/amd/ae4dma/ae4dma-dev.c     |  51 +----------
> > >   drivers/dma/amd/ae4dma/ae4dma-pci.c     |   1 +
> > >   drivers/dma/amd/ae4dma/ae4dma.h         |   3 +
> > >   drivers/dma/amd/ptdma/ptdma-dmaengine.c | 114 +++++++++++++++++++++++-
> > >   drivers/dma/amd/ptdma/ptdma.h           |   3 +
> > >   5 files changed, 123 insertions(+), 49 deletions(-)
> > > 
> > > diff --git a/drivers/dma/amd/ae4dma/ae4dma-dev.c b/drivers/dma/amd/ae4dma/ae4dma-dev.c
> > > index f0b3a3763adc..cd84b502265e 100644
> > > --- a/drivers/dma/amd/ae4dma/ae4dma-dev.c
> > > +++ b/drivers/dma/amd/ae4dma/ae4dma-dev.c
> > > @@ -14,53 +14,6 @@ static unsigned int max_hw_q = 1;
> > >   module_param(max_hw_q, uint, 0444);
> > >   MODULE_PARM_DESC(max_hw_q, "max hw queues supported by engine (any non-zero value, default: 1)");
> > > -static char *ae4_error_codes[] = {
> > > -	"",
> > > -	"ERR 01: INVALID HEADER DW0",
> > > -	"ERR 02: INVALID STATUS",
> > > -	"ERR 03: INVALID LENGTH - 4 BYTE ALIGNMENT",
> > > -	"ERR 04: INVALID SRC ADDR - 4 BYTE ALIGNMENT",
> > > -	"ERR 05: INVALID DST ADDR - 4 BYTE ALIGNMENT",
> > > -	"ERR 06: INVALID ALIGNMENT",
> > > -	"ERR 07: INVALID DESCRIPTOR",
> > > -};
> > > -
> > > -static void ae4_log_error(struct pt_device *d, int e)
> > > -{
> > > -	/* ERR 01 - 07 represents Invalid AE4 errors */
> > > -	if (e <= 7)
> > > -		dev_info(d->dev, "AE4DMA error: %s (0x%x)\n", ae4_error_codes[e], e);
> > > -	/* ERR 08 - 15 represents Invalid Descriptor errors */
> > > -	else if (e > 7 && e <= 15)
> > > -		dev_info(d->dev, "AE4DMA error: %s (0x%x)\n", "INVALID DESCRIPTOR", e);
> > > -	/* ERR 16 - 31 represents Firmware errors */
> > > -	else if (e > 15 && e <= 31)
> > > -		dev_info(d->dev, "AE4DMA error: %s (0x%x)\n", "FIRMWARE ERROR", e);
> > > -	/* ERR 32 - 63 represents Fatal errors */
> > > -	else if (e > 31 && e <= 63)
> > > -		dev_info(d->dev, "AE4DMA error: %s (0x%x)\n", "FATAL ERROR", e);
> > > -	/* ERR 64 - 255 represents PTE errors */
> > > -	else if (e > 63 && e <= 255)
> > > -		dev_info(d->dev, "AE4DMA error: %s (0x%x)\n", "PTE ERROR", e);
> > > -	else
> > > -		dev_info(d->dev, "Unknown AE4DMA error");
> > > -}
> > > -
> > > -static void ae4_check_status_error(struct ae4_cmd_queue *ae4cmd_q, int idx)
> > > -{
> > > -	struct pt_cmd_queue *cmd_q = &ae4cmd_q->cmd_q;
> > > -	struct ae4dma_desc desc;
> > > -	u8 status;
> > > -
> > > -	memcpy(&desc, &cmd_q->qbase[idx], sizeof(struct ae4dma_desc));
> > > -	status = desc.dw1.status;
> > > -	if (status && status != AE4_DESC_COMPLETED) {
> > > -		cmd_q->cmd_error = desc.dw1.err_code;
> > > -		if (cmd_q->cmd_error)
> > > -			ae4_log_error(cmd_q->pt, cmd_q->cmd_error);
> > > -	}
> > > -}
> > why is this getting moved?
> 
> To avoid circular dependency between PTDMA and AE4DMA, we are reusing PTDMA code to prevent
> duplication, as AE4DMA depends on PTDMA.

Okay please move the code first and then do changes, that makes it easier to
review and follow

-- 
~Vinod