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
