On 16-06-20, 20:11, Sanjay R Mehta wrote:
> +static int pt_core_execute_cmd(struct ptdma_desc *desc,
> + struct pt_cmd_queue *cmd_q)
> +{
> + __le32 *mp;
> + u32 *dp;
> + u32 tail;
> + int i;
no tabs, spaces pls
> + int ret = 0;
ret is initialized to 0
> +
> + if (desc->dw0.soc) {
> + desc->dw0.ioc = 1;
> + desc->dw0.soc = 0;
> + }
> + mutex_lock(&cmd_q->q_mutex);
> +
> + mp = (__le32 *)&cmd_q->qbase[cmd_q->qidx];
> + dp = (u32 *)desc;
> + for (i = 0; i < 8; i++)
> + mp[i] = cpu_to_le32(dp[i]); /* handle endianness */
> +
> + cmd_q->qidx = (cmd_q->qidx + 1) % CMD_Q_LEN;
> +
> + /* The data used by this command must be flushed to memory */
> + wmb();
> +
> + /* Write the new tail address back to the queue register */
> + tail = lower_32_bits(cmd_q->qdma_tail + cmd_q->qidx * Q_DESC_SIZE);
> + iowrite32(tail, cmd_q->reg_tail_lo);
> +
> + /* Turn the queue back on using our cached control register */
> + pt_start_queue(cmd_q);
> + mutex_unlock(&cmd_q->q_mutex);
> +
> + return ret;
and returned here!, why not return 0, or even do void return here
> +int pt_core_perform_passthru(struct pt_cmd_queue *cmd_q,
> + struct pt_passthru_engine *pt_engine)
> +{
> + struct ptdma_desc desc;
> +
> + cmd_q->cmd_error = 0;
> +
> + memset(&desc, 0, Q_DESC_SIZE);
why not sizeof(desc) insteadof Q_DESC_SIZE, this makes code harder to
look to check what this is defined to
> +int pt_core_init(struct pt_device *pt)
> +{
> + struct device *dev = pt->dev;
> + struct pt_cmd_queue *cmd_q = &pt->cmd_q;
> + struct dma_pool *dma_pool;
> + char dma_pool_name[MAX_DMAPOOL_NAME_LEN];
> + int ret;
> + u32 dma_addr_lo, dma_addr_hi;
reverse christmas tree please
> +
> + /* Allocate a dma pool for the queue */
> + snprintf(dma_pool_name, sizeof(dma_pool_name), "%s_q", pt->name);
> +
> + dma_pool = dma_pool_create(dma_pool_name, dev,
> + PT_DMAPOOL_MAX_SIZE,
> + PT_DMAPOOL_ALIGN, 0);
> + if (!dma_pool) {
> + dev_err(dev, "unable to allocate dma pool\n");
> + ret = -ENOMEM;
> + return ret;
> + }
> +
> + /* ptdma core initialisation */
> + iowrite32(CMD_CONFIG_VHB_EN, pt->io_regs + CMD_CONFIG_OFFSET);
> + iowrite32(CMD_QUEUE_PRIO, pt->io_regs + CMD_QUEUE_PRIO_OFFSET);
> + iowrite32(CMD_TIMEOUT_DISABLE, pt->io_regs + CMD_TIMEOUT_OFFSET);
> + iowrite32(CMD_CLK_GATE_CONFIG, pt->io_regs + CMD_CLK_GATE_CTL_OFFSET);
> + iowrite32(CMD_CONFIG_REQID, pt->io_regs + CMD_REQID_CONFIG_OFFSET);
> +
> + cmd_q->pt = pt;
> + cmd_q->dma_pool = dma_pool;
> + mutex_init(&cmd_q->q_mutex);
> +
> + /* Page alignment satisfies our needs for N <= 128 */
> + cmd_q->qsize = Q_SIZE(Q_DESC_SIZE);
> + cmd_q->qbase = dma_alloc_coherent(dev, cmd_q->qsize,
> + &cmd_q->qbase_dma,
> + GFP_KERNEL);
last line seems misaligned, please run checkpatch with --strict options
to find these.
> + if (!cmd_q->qbase) {
> + dev_err(dev, "unable to allocate command queue\n");
> + ret = -ENOMEM;
> + goto e_dma_alloc;
> + }
> +
> + cmd_q->qidx = 0;
> +
> + /* Preset some register values */
> + cmd_q->reg_control = pt->io_regs + CMD_Q_STATUS_INCR;
> + pt_init_cmdq_regs(cmd_q);
> +
> + dev_dbg(dev, "queue available\n");
debug artifacts, pls remove this and others
> +static int pt_pci_probe(struct pci_dev *pdev, const struct pci_device_id *id)
> +{
> + struct pt_device *pt;
> + struct pt_msix *pt_msix;
> + struct device *dev = &pdev->dev;
> + void __iomem * const *iomap_table;
> + int bar_mask;
> + int ret = -ENOMEM;
> +
> + pt = pt_alloc_struct(dev);
> + if (!pt)
> + goto e_err;
> +
> + pt_msix = devm_kzalloc(dev, sizeof(*pt_msix), GFP_KERNEL);
> + if (!pt_msix)
> + goto e_err;
> +
> + pt->pt_msix = pt_msix;
> + pt->dev_vdata = (struct pt_dev_vdata *)id->driver_data;
> + if (!pt->dev_vdata) {
> + ret = -ENODEV;
> + dev_err(dev, "missing driver data\n");
> + goto e_err;
> + }
> +
> + ret = pcim_enable_device(pdev);
> + if (ret) {
> + dev_err(dev, "pcim_enable_device failed (%d)\n", ret);
> + goto e_err;
> + }
> +
> + bar_mask = pci_select_bars(pdev, IORESOURCE_MEM);
> + ret = pcim_iomap_regions(pdev, bar_mask, "ptdma");
> + if (ret) {
> + dev_err(dev, "pcim_iomap_regions failed (%d)\n", ret);
> + goto e_err;
> + }
> +
> + iomap_table = pcim_iomap_table(pdev);
> + if (!iomap_table) {
> + dev_err(dev, "pcim_iomap_table failed\n");
> + ret = -ENOMEM;
> + goto e_err;
> + }
> +
> + pt->io_regs = iomap_table[pt->dev_vdata->bar];
> + if (!pt->io_regs) {
> + dev_err(dev, "ioremap failed\n");
> + ret = -ENOMEM;
> + goto e_err;
> + }
> +
> + ret = pt_get_irqs(pt);
> + if (ret)
> + goto e_err;
> +
> + pci_set_master(pdev);
> +
> + ret = dma_set_mask_and_coherent(dev, DMA_BIT_MASK(48));
> + if (ret) {
> + ret = dma_set_mask_and_coherent(dev, DMA_BIT_MASK(32));
> + if (ret) {
> + dev_err(dev, "dma_set_mask_and_coherent failed (%d)\n",
> + ret);
> + goto e_err;
> + }
> + }
> +
> + dev_set_drvdata(dev, pt);
> +
> + if (pt->dev_vdata)
> + ret = pt_core_init(pt);
> +
> + if (ret) {
> + dev_notice(dev, "PTDMA initialization failed\n");
> + goto e_err;
> + }
> +
> + dev_notice(dev, "PTDMA enabled\n");
dev_dbg?
> +
> + return 0;
> +
> +e_err:
> + dev_notice(dev, "initialization failed\n");
dev_err? Also no rollback?
> + return ret;
> +}
> +
> +static void pt_pci_remove(struct pci_dev *pdev)
> +{
> + struct device *dev = &pdev->dev;
> + struct pt_device *pt = dev_get_drvdata(dev);
> +
> + if (!pt)
> + return;
> +
> + if (pt->dev_vdata)
> + pt_core_destroy(pt);
> +
> + pt_free_irqs(pt);
> +}
> +
> +static const struct pt_dev_vdata dev_vdata[] = {
> + {
> + .bar = 2,
Is this PCI bars?
> + .version = PT_VERSION(5, 0),
Hw doesn't tell that?
--
~Vinod
