Re: [PATCH v4 1/1] dmaengine: switchtec-dma: Introduce Switchtec DMA engine PCI driver

[Christoph Hellwig]

On Fri, May 05, 2023 at 12:31:11AM +0000, Kelvin.Cao@xxxxxxxxxxxxx wrote:
> Hi Christoph,
> 
> Thanks for the comments. For the tasklet stuff, I guess your opinion is
> that by default the driver should go with threaded irq instead of
> tasklet as the former is more efficient, unless there's a good reason
> of using tasklet. 
> 
> Tasklet is widely used in DMA drivers, not sure if there's a rational
> reason or people just follow the code structure of the current ones. 

Given that neither nor anyone else from the RT community chimed
in I'm going to throw the towel on the tasklet use.  It looks fairly
suboptimal, but I don't want to block the driver on that.

> > > +     union {
> > > +             __le32 saddr_lo;
> > > +             __le32 widata_lo;
> > > +     };
> > > +     union {
> > > +             __le32 saddr_hi;
> > > +             __le32 widata_hi;
> > > +     };
> > 
> > What is the point for unions of identical data types?
> 
> The same offset could hold either source address or write immediate
> data in different transactions. Unions used here is to give different
> names for the same offset. I guess it improves readability when
> referring to them with proper names.

I find this rather confusing, especially as some code literally
switches on the op to fill in either set.

> The CE is little-endian and is filled by hardware. As an error message,
> I'd like to dump the whole structure. Would the following code look
> better?
> 
> __le32 *p;
> ...
> p = (__le32 *)ce;
> for (i = 0; i < sizeof(*ce)/4; i++) {
>  dev_err(chan_dev, "CE DW%d: 0x%08x\n", i,
>  le32_to_cpu(*p));
>  p++;
> }

Fine with me.

> > > +#define SWITCHTEC_DMA_DEVICE(device_id) \
> > > +     { \
> > > +             .vendor     = PCI_VENDOR_ID_MICROSEMI, \
> > > +             .device     = device_id, \
> > > +             .subvendor  = PCI_ANY_ID, \
> > > +             .subdevice  = PCI_ANY_ID, \
> > > +             .class      = PCI_CLASS_SYSTEM_OTHER << 8, \
> > > +             .class_mask = 0xFFFFFFFF, \
> > > +     }
> > > +
> > > +static const struct pci_device_id switchtec_dma_pci_tbl[] = {
> > > +     SWITCHTEC_DMA_DEVICE(0x4000), /* PFX 100XG4 */
> > 
> > This should use the common PCI_DEVICE() macro instead, i.e.
> > 
> >         PCI_DEVICE(PCI_VENDOR_ID_MICROSEMI, 0x4000), /* PFX 100XG4 */
> >         ...
> 
> We also need to distinguish the .class as we have devices of other
> .class with the same vendor/device ID.

Ok, that's roetty weird and probably worth a little comment.