Re: [PATCH v4 1/3] dmaengine: ptdma: Initial driver for the AMD PTDMA controller

[Sanjay R Mehta]

Apologies for my delayed response.

>> +#include <linux/module.h>
>> +#include <linux/kernel.h>
>> +#include <linux/pci.h>
>> +#include <linux/dma-mapping.h>
>> +#include <linux/interrupt.h>
>> +
>> +#include "ptdma.h"
>> +
>> +static int cmd_queue_length = 32;
>> +module_param(cmd_queue_length, uint, 0644);
>> +MODULE_PARM_DESC(cmd_queue_length,
>> +              " length of the command queue, a power of 2 (2 <= val <= 128)");
> 
> Any reason for this as module param? who will configure this and how?
> 
The command queue length can be from 2 to 64K command.
Therefore added as module parameter to allow the length of the queue to be specified at load time.

>> + * List of PTDMAs, PTDMA count, read-write access lock, and access functions
>> + *
>> + * Lock structure: get pt_unit_lock for reading whenever we need to
>> + * examine the PTDMA list. While holding it for reading we can acquire
>> + * the RR lock to update the round-robin next-PTDMA pointer. The unit lock
>> + * must be acquired before the RR lock.
>> + *
>> + * If the unit-lock is acquired for writing, we have total control over
>> + * the list, so there's no value in getting the RR lock.
>> + */
>> +static DEFINE_RWLOCK(pt_unit_lock);
>> +static LIST_HEAD(pt_units);
>> +
>> +static struct pt_device *pt_rr;
> 
> why do we need these globals and not in driver context?
> 
The AMD SOC has multiple PT controller's with the same PCI device ID and hence the same driver is probed for each instance.
The driver stores the pt_device context of each PT controller in this global list.

>> +static void pt_add_device(struct pt_device *pt)
>> +{
>> +     unsigned long flags;
>> +
>> +     write_lock_irqsave(&pt_unit_lock, flags);
>> +     list_add_tail(&pt->entry, &pt_units);
>> +     if (!pt_rr)
>> +             /*
>> +              * We already have the list lock (we're first) so this
>> +              * pointer can't change on us. Set its initial value.
>> +              */
>> +             pt_rr = pt;
>> +     write_unlock_irqrestore(&pt_unit_lock, flags);
>> +}
> 
> Can you please explain what do you mean by having a list of devices and
> why are we adding/removing dynamically?
> 
Since AMD SOC has many PT controller's with the same PCI device ID and
hence the same driver probed for initialization of each PT controller device instance.
Also, the number of PT controller varies for different AMD SOC's.
Therefore the dynamic adding/removing of each PT controller context to global device list implemented.
> --
> ~Vinod
>