From mboxrd@z Thu Jan 1 00:00:00 1970 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: base64 Subject: [2/2] dma: xilinx: dpdma: Add the Xilinx DisplayPort DMA engine driver From: Vinod Koul Message-Id: <20180124144059.GI18649@localhost> Date: Wed, 24 Jan 2018 20:10:59 +0530 To: Hyun Kwon Cc: "dmaengine@vger.kernel.org" , "devicetree@vger.kernel.org" , Michal Simek , Tejas Upadhyay List-ID: T24gVHVlLCBKYW4gMjMsIDIwMTggYXQgMDU6MDM6MTlQTSArMDAwMCwgSHl1biBLd29uIHdyb3Rl OgoKPiA+IHdoeSBzaG91bGQgdGhpcyBiZSBhIGNvbXBpbGUgb3B0aW9uCj4gPiAKPiAKPiBUaGlz IGRlYnVnZnMgY29kZSBpcyBmb3IgdGVzdGluZyAvIHJlZ3Jlc3Npb24sIGFuZCB3ZSBkb24ndCB3 YW50IHRvIGVuYWJsZSBpdAo+IGZvciByZWd1bGFyIHVzZXJzLgoKUmlnaHQgYW5kIHRoYXQgaXMg d2h5IHlvdSBoYXZlIENPTkZJR19ERUJVR0ZTLCB3aHkgbm90IHVzZSB0aGF0PwoKPiA+IGRvIHlv dSBuZWVkIGFsbCB0aGVzZSBoZWFkZXJzPwo+ID4gCj4gCj4gSSBkaXJlY3RseSBpbmNsdWRlZCBh bGwgaGVhZGVycyB0aGF0IGFyZSB1c2VkIGluIHRoaXMgZHJpdmVyLiBTb21lIG9mIHRoZW0gY2Fu Cj4gYmUgcmVtb3ZlZCBmcm9tIGluZGlyZWN0IGluY2x1c2lvbnMsIGFuZCBJJ20gZmluZSB3aXRo IHRoYXQuIFBsZWFzZSBsZXQgbWUga25vdwo+IGlmIHRoYXQncyB5b3VyIHByZWZlcmVuY2UuCgpZ ZXMgcGxzIHJlbW92ZSB0aGUgb25lcyB0aGF0IGFyZSBuZWVkZWQKCj4gPiA+ICsjZGVmaW5lIFhJ TElOWF9EUERNQV9JTlRSX0RFU0NfRE9ORV9TSElGVAkJMAo+ID4gCj4gPiB5b3UgY2FuIGRlZmlu ZSBhIGNvbW1vbiBzaGlmdCB1c2luZyBmZnMKPiA+IAo+IAo+IEkgZ3Vlc3MgeW91IG1lYW4gdG8g cmVwbGFjZSwgKHZhbHVlICYgTUFTSykgPDwgU0hJRlQsIHdpdGgKPiAodmFsdWUgJiBNQVNLKSA8 PCBmZnMoTUFTSykuIEknbGwgY2hhbmdlIHRvIHRoYXQgd2F5LiBMZXQgbWUga25vdyBvdGhlcndp c2UuCgp5ZXMgYW5kIHlvdSBtYXkgdXNlIHRoZSBvbmVzIGluIGJpdGZpZWxkLmgKCj4gPiB3aGF0 IGRvZXMgaXQgbWVhbiAobG93ZXIgMzIgYml0IG9mIE50aCA0S0IgcGFnZSkKPiA+IAo+IAo+IEVh Y2ggZGVzY3JpcHRvciBjYW4gcG9pbnQgdXAgdG8gNSAtIDQ4Yml0IGFkZHJlc3MgcGF5bG9hZHMu IHNyY19hZGRyKiBmaWVsZHMKPiBjb250YWluIGxvd2VyIDMyYml0IG9mIDQ4Yml0IGFkZHJlc3Mu IFJlbWFpbmluZyB1cHBlciAxNmJpdCBpcyBwcm9ncmFtbWVkIGluCj4gYWRkcl9leHQqIGZpZWxk cy4KCnBscyBkb2N1bWVudCB0aGlzCgo+ID4gPiArc3RhdGljIGludCB4aWxpbnhfZHBkbWFfY29u ZmlnKHN0cnVjdCBkbWFfY2hhbiAqZGNoYW4sCj4gPiA+ICsJCQkgICAgICAgc3RydWN0IGRtYV9z bGF2ZV9jb25maWcgKmNvbmZpZykKPiA+ID4gK3sKPiA+ID4gKwlpZiAoY29uZmlnLT5kaXJlY3Rp b24gIT0gRE1BX01FTV9UT19ERVYpCj4gPiA+ICsJCXJldHVybiAtRUlOVkFMOwo+ID4gCj4gPiA/ PyBXaHkgYXJlIHRoZSBjb25maWcgdmFsdWVzIG5vdCB1c2VkLCBob3cgZWxzZSBkbyB5b3UgY29u ZmlndXJlIHRoZQo+ID4gY2hhbm5lbD8KPiA+IAo+IAo+IFRoZXJlJ3Mgbm90IG11Y2ggY29uZmln dXJhdGlvbiB0byBiZSBkb25lLiBDaGFubmVscyBhcmUgcHJldHR5IG11Y2ggaWRlbnRpY2FsCj4g d2l0aCBmaXhlZCBjb25maWd1cmF0aW9ucy4KCmhtbW0sIHlvdSBkbyBzdXBwb3J0IERNQV9TTEFW RSByaWdodD8KCj4gPiB3aHkgaGF2ZSB0aGVzZSB3cmFwcGVycyB3aGljaCBkbyBub3QgZG8gYW55 dGhpbmc/Cj4gPiAKPiAKPiBJdCdzIGp1c3QgbXkgcGVyc29uYWwgcHJlZmVyZW5jZSB0byBzcGxp dCBpbnRvIGRpZmZlcmVudCBjb2RlIHBhcnRpdGlvbnMsIGFuZAo+IGVhY2ggc2VjdGlvbiBpcyBw YXJ0aXRpb25lZCAvIGdyb3VwZWQgd2l0aCBzb21lIGNvbW1lbnQgbGluZS4gOi0pCj4gRXgsIGEg cGFydGl0aW9uIGZvciBzdHJ1Y3QgZG1hX2NoYW4sIGFuZCBhbm90aGVyIG9uZSBmb3IKPiBzdHJ1 Y3QgeGlsaW54X2RwZG1hX2NoYW4uIEl0IGdpdmVzIG1lIHNvcnQgb2YgYWJzdHJhY3RlZCB2aWV3 LiBCdXQgaXQgbWF5IGJlCj4ganVzdCBtZSwgYW5kIGl0IGNvbWVzIHdpdGggZXh0cmEgaW5kaXJl Y3Rpb25zLiBJJ2xsIHJlbW92ZSB1bm5lY2Vzc2FyeSB3cmFwcGVycy4KCndyYXBwZXIgd2l0aG91 dCBhbnkgbG9naWMgZG9udCBoZWxwCgo+ID4gPiArCWZvciAoaSA9IDA7IGkgPCBYSUxJTlhfRFBE TUFfTlVNX0NIQU47IGkrKykKPiA+ID4gKwkJaWYgKHhkZXYtPmNoYW5baV0pCj4gPiA+ICsJCQl4 aWxpbnhfZHBkbWFfY2hhbl9yZW1vdmUoeGRldi0+Y2hhbltpXSk7Cj4gPiAKPiA+IEF0IHRoaXMg cG9pbnQgeW91ciBpcnEgaXMgc3RpbGwgZW5hYmxlZCBhbmQgY2FuIGZpcmUsIGFuZCBjYW4gc2No ZWR1bGUKPiA+IHRhc2tsZXQuLiBBcmUgeW91IHN1cmUgeW91IGFyZSBva2F5IHdpdGggdGhhdD8K PiA+IAo+IAo+IE9rLiBZb3UgbWVhbiB0aGF0IGFuIGludGVycnVwdCBjYW4gb2NjdXIgcmlnaHQg YmVmb3JlCj4geGlsaW54X2RwZG1hX2Rpc2FibGVfaW50cigpLCBhbmQgdGhlIGludGVycnVwdCBt YXkgYmUgaGFuZGxlZCAgaW4gdGhlIG1pZGRsZSBvcgo+IGFmdGVyIHJlbW92aW5nIHRoaXMgZHJp dmVyLiBJJ2xsIHN3aXRjaCB0byByZXF1ZXN0X2lycSgpIGZyb20KPiBkZXZtX3JlcXVlc3RfaXJx KCksIGFuZCByZW1vdmUgdGhlIGlycSB3aGVuIHJlbW92aW5nIHRoZSBkcml2ZXIuCgp5ZXMgYW5k IGVuc3VyZSB0YXNrbGV0cyBhcmUgcXVpZXNjZWQKCj4gPiA+ICtNT0RVTEVfQVVUSE9SKCJYaWxp bngsIEluYy4iKTsKPiA+ID4gK01PRFVMRV9ERVNDUklQVElPTigiWGlsaW54IERQRE1BIGRyaXZl ciIpOwo+ID4gPiArTU9EVUxFX0xJQ0VOU0UoIkdQTCB2MiIpOwo+ID4gCj4gPiBObyBNT0RVTEVf QUxJQVMoKT8KPiAKPiBJcyBpdCByZXF1aXJlZD8gQ291bGQgeW91IHBsZWFzZSBlbGFib3JhdGUs IHRvIGhlbHAgbXkgdW5kZXJzdGFuZGluZz8gCgpkaWQgeW91IHRyeSBidWlsaW5nIGFzIG1vZHVs ZXMgYW5kIHRlc3RpbmcgdGhpcz8K From mboxrd@z Thu Jan 1 00:00:00 1970 From: Vinod Koul Subject: Re: [PATCH 2/2] dma: xilinx: dpdma: Add the Xilinx DisplayPort DMA engine driver Date: Wed, 24 Jan 2018 20:10:59 +0530 Message-ID: <20180124144059.GI18649@localhost> References: <1515204848-3493-1-git-send-email-hyun.kwon@xilinx.com> <1515204848-3493-2-git-send-email-hyun.kwon@xilinx.com> <20180112141355.GO18649@localhost> Mime-Version: 1.0 Content-Type: text/plain; charset=us-ascii Return-path: Content-Disposition: inline In-Reply-To: Sender: devicetree-owner-u79uwXL29TY76Z2rM5mHXA@public.gmane.org To: Hyun Kwon Cc: "dmaengine-u79uwXL29TY76Z2rM5mHXA@public.gmane.org" , "devicetree-u79uwXL29TY76Z2rM5mHXA@public.gmane.org" , Michal Simek , Tejas Upadhyay List-Id: devicetree@vger.kernel.org On Tue, Jan 23, 2018 at 05:03:19PM +0000, Hyun Kwon wrote: > > why should this be a compile option > > > > This debugfs code is for testing / regression, and we don't want to enable it > for regular users. Right and that is why you have CONFIG_DEBUGFS, why not use that? > > do you need all these headers? > > > > I directly included all headers that are used in this driver. Some of them can > be removed from indirect inclusions, and I'm fine with that. Please let me know > if that's your preference. Yes pls remove the ones that are needed > > > +#define XILINX_DPDMA_INTR_DESC_DONE_SHIFT 0 > > > > you can define a common shift using ffs > > > > I guess you mean to replace, (value & MASK) << SHIFT, with > (value & MASK) << ffs(MASK). I'll change to that way. Let me know otherwise. yes and you may use the ones in bitfield.h > > what does it mean (lower 32 bit of Nth 4KB page) > > > > Each descriptor can point up to 5 - 48bit address payloads. src_addr* fields > contain lower 32bit of 48bit address. Remaining upper 16bit is programmed in > addr_ext* fields. pls document this > > > +static int xilinx_dpdma_config(struct dma_chan *dchan, > > > + struct dma_slave_config *config) > > > +{ > > > + if (config->direction != DMA_MEM_TO_DEV) > > > + return -EINVAL; > > > > ?? Why are the config values not used, how else do you configure the > > channel? > > > > There's not much configuration to be done. Channels are pretty much identical > with fixed configurations. hmmm, you do support DMA_SLAVE right? > > why have these wrappers which do not do anything? > > > > It's just my personal preference to split into different code partitions, and > each section is partitioned / grouped with some comment line. :-) > Ex, a partition for struct dma_chan, and another one for > struct xilinx_dpdma_chan. It gives me sort of abstracted view. But it may be > just me, and it comes with extra indirections. I'll remove unnecessary wrappers. wrapper without any logic dont help > > > + for (i = 0; i < XILINX_DPDMA_NUM_CHAN; i++) > > > + if (xdev->chan[i]) > > > + xilinx_dpdma_chan_remove(xdev->chan[i]); > > > > At this point your irq is still enabled and can fire, and can schedule > > tasklet.. Are you sure you are okay with that? > > > > Ok. You mean that an interrupt can occur right before > xilinx_dpdma_disable_intr(), and the interrupt may be handled in the middle or > after removing this driver. I'll switch to request_irq() from > devm_request_irq(), and remove the irq when removing the driver. yes and ensure tasklets are quiesced > > > +MODULE_AUTHOR("Xilinx, Inc."); > > > +MODULE_DESCRIPTION("Xilinx DPDMA driver"); > > > +MODULE_LICENSE("GPL v2"); > > > > No MODULE_ALIAS()? > > Is it required? Could you please elaborate, to help my understanding? did you try builing as modules and testing this? -- ~Vinod -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org More majordomo info at http://vger.kernel.org/majordomo-info.html