Messages in this thread | | | From | "Koul, Vinod" <> | Date | Wed, 28 Jul 2010 13:20:37 +0530 | Subject | RE: Resend [PATCH] intel_mid: Add Mrst & Mfld DMA Drivers |
| |
> The use of the prep_dma_memcpy routine instead of prep_slave_sg is a > bit odd for device-to-memory dma because the dma_addr_t type only > describes the bus address view of host memory from the device > perspective. Other slave dma devices are using a backdoor method > (since a good abstraction does not exist) to associate a known channel > to a known device and then specifying the host address and a direction > via prep_slave_sg. I see that this usage of dma_addr_t was inherited > from the dw_spi implementation, so not much we can do about it now, > but I don't think its a model we want to perpetuate. For the four controllers this drivers supports, two don't have sg support in hardware. For the other two I am going to add the sg support next, expect the patch for that in few weeks or so.
> I do see that the dw_spi_mid driver is only calling > dma_request_channel(mask, NULL, NULL). Are we guaranteed that > dw_spi_mid silicon collateral will only ever exist on an intel_mid_dma > platform? Otherwise can the dw_mid_spi driver survive talking to > another random DMA_SLAVE|DMA_MEMCPY channel? The purpose of the > filter_fn parameter to dma_request_channel() is to guarantee (by > arch-specific hackery) that you are talking to a known compatible > channel for your device. Yes, the filter function in these drivers is supposed to look for this device and accept dma channel for specific controller only. I looked at dw_spi_mid driver and yes it is indeed doing the wrong thing, I will send the patch to its maintainer to fix this. Other driver which use this and are not upstream seem to be using the filter function properly
> > +{ > > + struct intel_mid_dma_chan *midc; > > + struct intel_mid_dma_desc *desc = NULL; > > + struct intel_mid_dma_slave *mids; > > + union intel_mid_dma_ctl_lo ctl_lo; > > + union intel_mid_dma_ctl_hi ctl_hi; > > + union intel_mid_dma_cfg_lo cfg_lo; > > + union intel_mid_dma_cfg_hi cfg_hi; > > + enum intel_mid_dma_width width = 0; > > + > > + pr_debug("MDMA: Prep for memcpy\n"); > > + WARN_ON(!chan); > > + if (!len) > > + return NULL; > > + > > + mids = chan->private; > > + WARN_ON(!mids); > > + > > + midc = to_intel_mid_dma_chan(chan); > > + WARN_ON(!midc); > > These WARN_ONs are a bit odd because if they trigger we'll hit null > pointer dereferences soon afterwards. Is that what you wanted? Agreed, I will send another patch to fix this
> > +struct intel_mid_dma_slave { [snip] > > For 2.6.37 I'd like to see a conversion of this to the result of > Linus' dma_slave_config work, but this isn't a blocker for 2.6.36. Yes, I will update this to use dma_slave_config when I add the DMA sg support
> > *applied with a fix for: > drivers/dma/intel_mid_dma.c:511: warning: format %d expects type int, > but argument 4 has type size_t -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
| |