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

[Kelvin.Cao]

On Mon, 2023-04-10 at 18:42 +0200, Christoph Hellwig wrote:
> 
> > +     writew((__force u16)cpu_to_le16(SWITCHTEC_DMA_SQ_SIZE),
> > +            &swdma_chan->mmio_chan_fw->sq_size);
> > +     writew((__force u16)cpu_to_le16(SWITCHTEC_DMA_CQ_SIZE),
> > +            &swdma_chan->mmio_chan_fw->cq_size);
> 
> This looks broken to me, writew always expects cpu endian arguments
> and byte swaps on big endian systems.

Do you mean writew assumes the peripherals be little-endian, and will
do the swap when host is bit-endian?
> 
> And if it isn't bogus: __force casts need a good detailed comment
> explaining them.
> 
> > +void switchtec_chan_kobject_del(struct switchtec_dma_chan
> > *swdma_chan);
> 
> This function doesn't actually seem to exists, and thus doesn't need
> a forward declaration.
> 
> > +     if (!swdma_dev)
> > +             return -ENOMEM;
> > +
> > +     bar = pcim_iomap_table(pdev)[0];
> > +     swdma_dev->bar = bar;
> > +
> > +     swdma_dev->mmio_dmac_ver = bar +
> > SWITCHTEC_DMAC_VERSION_OFFSET;
> > +     swdma_dev->mmio_dmac_cap = bar +
> > SWITCHTEC_DMAC_CAPABILITY_OFFSET;
> > +     swdma_dev->mmio_dmac_status = bar +
> > SWITCHTEC_DMAC_STATUS_OFFSET;
> > +     swdma_dev->mmio_dmac_ctrl = bar +
> > SWITCHTEC_DMAC_CONTROL_OFFSET;
> > +     swdma_dev->mmio_chan_hw_all = bar +
> > SWITCHTEC_DMAC_CHAN_CTRL_OFFSET;
> > +     swdma_dev->mmio_chan_fw_all = bar +
> > SWITCHTEC_DMAC_CHAN_CFG_STS_OFFSET;
> 
> These are all either unused or only used once or twice.  I'd drop
> all the extra pointers and just do the little bit of arithmetics
> in the callers.

Will clean it up.
> 
> But I find the whole pcim_iomap_table concept very confusing to the
> reader of the driver, and given that it doesn't really use many
> devm or pcim routines I'd suggest removing them all and sticking to
> one well understood way of manging resource lifetimes.

I didn't get it. Do you have specific suggestion?
> 
> > +     rc = dma_set_mask_and_coherent(&pdev->dev, DMA_BIT_MASK(64));
> > +     if (rc)
> > +             rc = dma_set_mask_and_coherent(&pdev->dev,
> > DMA_BIT_MASK(32));
> > +     if (rc)
> > +             return rc;
> 
> dma_set_mask_and_coherent for a smaller mask will never succeed when
> trying to set it to a larger one failed.  So you can remove the
> second
> call here.

By default the kernel assumes the device can address 32-bit address
space, I wonder why it wouldn't allow 32-bit mask when it failes 64-
bit?
> 
> > +#define MICROSEMI_VENDOR_ID 0x11f8
> 
> This is already in include/linux/pci_ids.h as
> PCI_VENDOR_ID_MICROSEMI, please use that.
> 
> > +
> > +#define SWITCHTEC_PCI_DEVICE(device_id) \
> > +     { \
> > +             .vendor     = MICROSEMI_VENDOR_ID, \
> > +             .device     = device_id, \
> > +             .subvendor  = PCI_ANY_ID, \
> > +             .subdevice  = PCI_ANY_ID, \
> > +             .class      = PCI_CLASS_SYSTEM_OTHER << 8, \
> > +             .class_mask = 0xFFFFFFFF, \
> > +     }
> 
> Please just use
> 
>         PCI_DEVICE(PCI_VENDOR_ID_MICROSEMI, 0x1234),
> 
> instead.
> 
> > +     SWITCHTEC_PCI_DEVICE(0x4000),  //PFX 100XG4
> 
> and please use normal /* */ comments.
Ok.

Thanks,
Kelvin