On Mon, Apr 20, 2015 at 11:49:27AM +0800, Jun Nie wrote:
> 2015-04-17 16:34 GMT+08:00 Maxime Ripard <maxime.ripard@xxxxxxxxxxxxxxxxxx>:
>
> > Hi,
> >
> > On Thu, Apr 16, 2015 at 11:18:34AM +0800, Jun Nie wrote:
> > > Add ZTE ZX296702 dma controller support
> > >
> > > Signed-off-by: Jun Nie <jun.nie@xxxxxxxxxx>
> > > ---
> > > drivers/dma/Kconfig | 8 +
> > > drivers/dma/Makefile | 1 +
> > > drivers/dma/zx_dma.c | 888
> > +++++++++++++++++++++++++++++++++++++++++++++++++++
> > > 3 files changed, 897 insertions(+)
> > > create mode 100644 drivers/dma/zx_dma.c
> >
> > There's quite a few checkpatch warnings and to-checks to fix, make
> > sure you run it.
>
> Thanks for checking! Will fix printk warning with proper log API.
> MAINTAINER and dts warning can be ignored for it will be fixed by later
> dts/MAINTAINER patch.
Also make sure that you pass the --strict option. There was a lot of
arguments alignments and similar errors, that are not reported by
default.
> > > +static void zx_dma_tasklet(unsigned long arg)
> > > +{
> > > + struct zx_dma_dev *d = (struct zx_dma_dev *)arg;
> > > + struct zx_dma_phy *p;
> > > + struct zx_dma_chan *c, *cn;
> > > + unsigned pch, pch_alloc = 0;
> > > +
> > > + /* check new dma request of running channel in vc->desc_issued */
> > > + list_for_each_entry_safe(c, cn, &d->slave.channels,
> > > + vc.chan.device_node) {
> > > + spin_lock_irq(&c->vc.lock);
> > > + p = c->phy;
> > > + if (p && p->ds_done && zx_dma_start_txd(c)) {
> > > + /* No current txd associated with this channel */
> > > + dev_dbg(d->slave.dev, "pchan %u: free\n", p->idx);
> > > + /* Mark this channel free */
> > > + c->phy = NULL;
> > > + p->vchan = NULL;
> > > + }
> > > + spin_unlock_irq(&c->vc.lock);
> > > + }
> > > +
> > > + /* check new channel request in d->chan_pending */
> > > + spin_lock_irq(&d->lock);
> > > + while (!list_empty(&d->chan_pending)) {
> > > + c = list_first_entry(&d->chan_pending,
> > > + struct zx_dma_chan, node);
> > > + p = &d->phy[c->id];
> > > + if (p->vchan == NULL) {
> > > + /* remove from d->chan_pending */
> > > + list_del_init(&c->node);
> > > + pch_alloc |= 1 << c->id;
> > > + /* Mark this channel allocated */
> > > + p->vchan = c;
> > > + c->phy = p;
> > > + } else {
> > > + dev_dbg(d->slave.dev, "pchan %u: busy!\n", c->id);
> > > + }
> > > + }
> > > + spin_unlock_irq(&d->lock);
> > > +
> > > + for (pch = 0; pch < d->dma_channels; pch++) {
> > > + if (pch_alloc & (1 << pch)) {
> > > + p = &d->phy[pch];
> > > + c = p->vchan;
> > > + if (c) {
> > > + spin_lock_irq(&c->vc.lock);
> > > + zx_dma_start_txd(c);
> > > + spin_unlock_irq(&c->vc.lock);
> > > + }
> > > + }
> > > + }
> > > +}
> >
> > It's better to have the election of a new transfer be done directly in
> > the interrupt handler.
> >
> > It will simplify your driver (since you won't have the tasklet
> > anymore), and it should increase the performances (since you won't
> > have to wait for the tasklet to be scheduled to start a new transfer).
>
> Performance is not a concern currently as all DMA channel are for slow
> peripherals. The logic in tasklet is a bit heavy for irq handler.
Still, having more performance is always good :)
And we really try to have a global policy on this.
> > > +static int zx_dma_probe(struct platform_device *op)
> > > +{
> > > + struct zx_dma_dev *d;
> > > + const struct of_device_id *of_id;
> > > + struct resource *iores;
> > > + int i, ret, irq = 0;
> > > +
> > > + iores = platform_get_resource(op, IORESOURCE_MEM, 0);
> > > + if (!iores)
> > > + return -EINVAL;
> > > +
> > > + d = devm_kzalloc(&op->dev, sizeof(*d), GFP_KERNEL);
> > > + if (!d)
> > > + return -ENOMEM;
> > > +
> > > + d->base = devm_ioremap_resource(&op->dev, iores);
> > > + if (IS_ERR(d->base))
> > > + return PTR_ERR(d->base);
> > > +
> > > + of_id = of_match_device(zx6702_dma_dt_ids, &op->dev);
> > > + if (of_id) {
> >
> > You should always be in such a case, do you support any other kind
> > of probing than through the DT?
>
> Only DT is supported per current requirement. Other probing can be
> added when needed.
Then you can just remove such a test. You should always match the DT
ids.
Thanks!
Maxime
--
Maxime Ripard, Free Electrons
Embedded Linux, Kernel and Android engineering
http://free-electrons.com
Attachment:
signature.asc
Description: Digital signature
