From mboxrd@z Thu Jan 1 00:00:00 1970 From: Philipp Zabel Subject: Re: [PATCH 3/3] media: imx-pxp: add i.MX Pixel Pipeline driver Date: Tue, 04 Sep 2018 11:12:25 +0200 Message-ID: <1536052345.3468.1.camel@pengutronix.de> References: <20180810151822.18650-1-p.zabel@pengutronix.de> <20180810151822.18650-4-p.zabel@pengutronix.de> <20180904080408.GJ20333@w540> Mime-Version: 1.0 Content-Type: text/plain; charset="utf-8" Content-Transfer-Encoding: base64 Return-path: In-Reply-To: <20180904080408.GJ20333@w540> List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Sender: "linux-arm-kernel" Errors-To: linux-arm-kernel-bounces+linux-arm-kernel=m.gmane.org@lists.infradead.org To: jacopo mondi Cc: devicetree@vger.kernel.org, Rob Herring , kernel@pengutronix.de, Mauro Carvalho Chehab , Shawn Guo , linux-arm-kernel@lists.infradead.org, linux-media@vger.kernel.org List-Id: devicetree@vger.kernel.org SGkgSmFjb3BvLAoKdGhhbmsgeW91IGZvciB0aGUgcmV2aWV3IQoKT24gVHVlLCAyMDE4LTA5LTA0 IGF0IDEwOjA0ICswMjAwLCBqYWNvcG8gbW9uZGkgd3JvdGU6Cj4gSGkgUGhpbGlwcCwKPiAKPiBP biBGcmksIEF1ZyAxMCwgMjAxOCBhdCAwNToxODoyMlBNICswMjAwLCBQaGlsaXBwIFphYmVsIHdy b3RlOgpbLi4uXQo+ID4gK3N0YXRpYyBzdHJ1Y3QgcHhwX2ZtdCBmb3JtYXRzW10gPSB7Cj4gPiAr CXsKPiA+ICsJCS5mb3VyY2MJPSBWNEwyX1BJWF9GTVRfWEJHUjMyLAo+ID4gKwkJLmRlcHRoCT0g MzIsCj4gPiArCQkvKiBCb3RoIGNhcHR1cmUgYW5kIG91dHB1dCBmb3JtYXQgKi8KPiA+ICsJCS50 eXBlcwk9IE1FTTJNRU1fQ0FQVFVSRSB8IE1FTTJNRU1fT1VUUFVULApbLi4uXQo+ID4gKwl9LCB7 Cj4gPiArCQkuZm91cmNjID0gVjRMMl9QSVhfRk1UX1lVVjMyLAo+ID4gKwkJLmRlcHRoCT0gMTYs Cj4gCj4gQWNjb3JkaW5nIHRvOgo+IGh0dHBzOi8vd3d3LmxpbnV4dHYub3JnL2Rvd25sb2Fkcy92 NGwtZHZiLWFwaXMtb2xkL3BhY2tlZC15dXYuaHRtbAo+IHNob3VsZG4ndCB0aGlzIGJlIDMyPwoK WWVzLCBJJ2xsIGNoYW5nZSBkZXB0aCB0byAzMi4KClsuLi5dCj4gPiArfQo+ID4gKwo+IAo+IE11 bHRpcGxlIGJsYW5rIGxpbmVzIChpbiBhIGZldyBvdGhlciBwbGFjZXMgdG9vKQo+IAo+ID4gKwoK Rm91bmQgdGhlbSwgd2lsbCBmaXggdGhlbS4KClsuLi5dCj4gPiArCS8qIEVuYWJsZSBjbG9ja3Mg YW5kIGR1bXAgcmVnaXN0ZXJzICovCj4gPiArCWNsa19wcmVwYXJlX2VuYWJsZShkZXYtPmNsayk7 Cj4gPiArCXB4cF9zb2Z0X3Jlc2V0KGRldik7Cj4gPiArCj4gPiArCXNwaW5fbG9ja19pbml0KCZk ZXYtPmlycWxvY2spOwo+ID4gKwo+ID4gKwlyZXQgPSB2NGwyX2RldmljZV9yZWdpc3RlcigmcGRl di0+ZGV2LCAmZGV2LT52NGwyX2Rldik7Cj4gPiArCWlmIChyZXQpCj4gPiArCQlyZXR1cm4gcmV0 Owo+ID4gKwo+ID4gKwlhdG9taWNfc2V0KCZkZXYtPm51bV9pbnN0LCAwKTsKPiA+ICsJbXV0ZXhf aW5pdCgmZGV2LT5kZXZfbXV0ZXgpOwo+ID4gKwo+ID4gKwlkZXYtPnZmZCA9IHB4cF92aWRlb2Rl djsKPiAKPiBUaGUgbmFtZSBvZiB0aGUgdmlkZW8gZGV2aWNlIGlzIHRoZSBzYW1lIGZvciBhbGwg aW5zdGFuY2VzIG9mIHRoaXMKPiBkcml2ZXIuIElzIHRoaXMgb2s/CgpJIGV4cGVjdCB0aGF0IHRo ZXJlIGlzIG9ubHkgZXZlciBnb2luZyB0byBiZSBvbmUgaW5zdGFuY2Ugb24gdGhlIFNvQy4KClsu Li5dCj4gPiArCXY0bDJfZGV2aWNlX3VucmVnaXN0ZXIoJmRldi0+djRsMl9kZXYpOwo+IAo+IERp c2FibGUgY2xvY2s/CgpZZXMsIEknbGwgZml4IHRoZSBjbG9jayBoYW5kbGluZy4KClsuLi5dCj4g PiArTU9EVUxFX0RFU0NSSVBUSU9OKCJpLk1YIFBYUCBtZW0ybWVtIHNjYWxlci9DU0Mvcm90YXRv ciIpOwo+ID4gK01PRFVMRV9BVVRIT1IoIlBoaWxpcHAgWmFiZWwgPGtlcm5lbEBwZW5ndXRyb25p eC5kZT4iKTsKPiA+ICtNT0RVTEVfTElDRU5TRSgiR1BMIik7Cj4gCj4gU1BEWCBoZWFkZXIgc2F5 cyBHUEwyLjArCgpTZWUgaW5jbHVkZS9saW51eC9tb2R1bGUuaDoKCi8qCsKgKiBUaGUgZm9sbG93 aW5nIGxpY2Vuc2UgaWRlbnRzIGFyZSBjdXJyZW50bHkgYWNjZXB0ZWQgYXMgaW5kaWNhdGluZyBm cmVlCsKgKiBzb2Z0d2FyZSBtb2R1bGVzCsKgKgrCoCrCoMKgwqDCoMKgwqAiR1BMIsKgwqDCoMKg wqDCoMKgwqDCoMKgwqDCoMKgwqDCoMKgwqDCoMKgwqDCoMKgwqDCoMKgwqDCoFtHTlUgUHVibGlj IExpY2Vuc2UgdjIgb3IgbGF0ZXJdCiAqIFsuLi5dCiAqLwoKVGhpcyBhbHJlYWR5IHNlZW1zIHRv IGJlIHRoZSByaWdodCBjaG9pY2UuCgpyZWdhcmRzClBoaWxpcHAKCl9fX19fX19fX19fX19fX19f X19fX19fX19fX19fX19fX19fX19fX19fX19fX19fCmxpbnV4LWFybS1rZXJuZWwgbWFpbGluZyBs aXN0CmxpbnV4LWFybS1rZXJuZWxAbGlzdHMuaW5mcmFkZWFkLm9yZwpodHRwOi8vbGlzdHMuaW5m cmFkZWFkLm9yZy9tYWlsbWFuL2xpc3RpbmZvL2xpbnV4LWFybS1rZXJuZWwK From mboxrd@z Thu Jan 1 00:00:00 1970 Return-path: Received: from metis.ext.pengutronix.de ([85.220.165.71]:36211 "EHLO metis.ext.pengutronix.de" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1726208AbeIDNgj (ORCPT ); Tue, 4 Sep 2018 09:36:39 -0400 Message-ID: <1536052345.3468.1.camel@pengutronix.de> Subject: Re: [PATCH 3/3] media: imx-pxp: add i.MX Pixel Pipeline driver From: Philipp Zabel To: jacopo mondi Cc: linux-media@vger.kernel.org, Mauro Carvalho Chehab , Rob Herring , Shawn Guo , devicetree@vger.kernel.org, linux-arm-kernel@lists.infradead.org, kernel@pengutronix.de Date: Tue, 04 Sep 2018 11:12:25 +0200 In-Reply-To: <20180904080408.GJ20333@w540> References: <20180810151822.18650-1-p.zabel@pengutronix.de> <20180810151822.18650-4-p.zabel@pengutronix.de> <20180904080408.GJ20333@w540> Content-Type: text/plain; charset="UTF-8" Mime-Version: 1.0 Content-Transfer-Encoding: 8bit Sender: linux-media-owner@vger.kernel.org List-ID: Hi Jacopo, thank you for the review! On Tue, 2018-09-04 at 10:04 +0200, jacopo mondi wrote: > Hi Philipp, > > On Fri, Aug 10, 2018 at 05:18:22PM +0200, Philipp Zabel wrote: [...] > > +static struct pxp_fmt formats[] = { > > + { > > + .fourcc = V4L2_PIX_FMT_XBGR32, > > + .depth = 32, > > + /* Both capture and output format */ > > + .types = MEM2MEM_CAPTURE | MEM2MEM_OUTPUT, [...] > > + }, { > > + .fourcc = V4L2_PIX_FMT_YUV32, > > + .depth = 16, > > According to: > https://www.linuxtv.org/downloads/v4l-dvb-apis-old/packed-yuv.html > shouldn't this be 32? Yes, I'll change depth to 32. [...] > > +} > > + > > Multiple blank lines (in a few other places too) > > > + Found them, will fix them. [...] > > + /* Enable clocks and dump registers */ > > + clk_prepare_enable(dev->clk); > > + pxp_soft_reset(dev); > > + > > + spin_lock_init(&dev->irqlock); > > + > > + ret = v4l2_device_register(&pdev->dev, &dev->v4l2_dev); > > + if (ret) > > + return ret; > > + > > + atomic_set(&dev->num_inst, 0); > > + mutex_init(&dev->dev_mutex); > > + > > + dev->vfd = pxp_videodev; > > The name of the video device is the same for all instances of this > driver. Is this ok? I expect that there is only ever going to be one instance on the SoC. [...] > > + v4l2_device_unregister(&dev->v4l2_dev); > > Disable clock? Yes, I'll fix the clock handling. [...] > > +MODULE_DESCRIPTION("i.MX PXP mem2mem scaler/CSC/rotator"); > > +MODULE_AUTHOR("Philipp Zabel "); > > +MODULE_LICENSE("GPL"); > > SPDX header says GPL2.0+ See include/linux/module.h: /*  * The following license idents are currently accepted as indicating free  * software modules  *  *      "GPL"                           [GNU Public License v2 or later] * [...] */ This already seems to be the right choice. regards Philipp From mboxrd@z Thu Jan 1 00:00:00 1970 From: p.zabel@pengutronix.de (Philipp Zabel) Date: Tue, 04 Sep 2018 11:12:25 +0200 Subject: [PATCH 3/3] media: imx-pxp: add i.MX Pixel Pipeline driver In-Reply-To: <20180904080408.GJ20333@w540> References: <20180810151822.18650-1-p.zabel@pengutronix.de> <20180810151822.18650-4-p.zabel@pengutronix.de> <20180904080408.GJ20333@w540> Message-ID: <1536052345.3468.1.camel@pengutronix.de> To: linux-arm-kernel@lists.infradead.org List-Id: linux-arm-kernel.lists.infradead.org Hi Jacopo, thank you for the review! On Tue, 2018-09-04 at 10:04 +0200, jacopo mondi wrote: > Hi Philipp, > > On Fri, Aug 10, 2018 at 05:18:22PM +0200, Philipp Zabel wrote: [...] > > +static struct pxp_fmt formats[] = { > > + { > > + .fourcc = V4L2_PIX_FMT_XBGR32, > > + .depth = 32, > > + /* Both capture and output format */ > > + .types = MEM2MEM_CAPTURE | MEM2MEM_OUTPUT, [...] > > + }, { > > + .fourcc = V4L2_PIX_FMT_YUV32, > > + .depth = 16, > > According to: > https://www.linuxtv.org/downloads/v4l-dvb-apis-old/packed-yuv.html > shouldn't this be 32? Yes, I'll change depth to 32. [...] > > +} > > + > > Multiple blank lines (in a few other places too) > > > + Found them, will fix them. [...] > > + /* Enable clocks and dump registers */ > > + clk_prepare_enable(dev->clk); > > + pxp_soft_reset(dev); > > + > > + spin_lock_init(&dev->irqlock); > > + > > + ret = v4l2_device_register(&pdev->dev, &dev->v4l2_dev); > > + if (ret) > > + return ret; > > + > > + atomic_set(&dev->num_inst, 0); > > + mutex_init(&dev->dev_mutex); > > + > > + dev->vfd = pxp_videodev; > > The name of the video device is the same for all instances of this > driver. Is this ok? I expect that there is only ever going to be one instance on the SoC. [...] > > + v4l2_device_unregister(&dev->v4l2_dev); > > Disable clock? Yes, I'll fix the clock handling. [...] > > +MODULE_DESCRIPTION("i.MX PXP mem2mem scaler/CSC/rotator"); > > +MODULE_AUTHOR("Philipp Zabel "); > > +MODULE_LICENSE("GPL"); > > SPDX header says GPL2.0+ See include/linux/module.h: /* ?* The following license idents are currently accepted as indicating free ?* software modules ?* ?*??????"GPL"???????????????????????????[GNU Public License v2 or later] * [...] */ This already seems to be the right choice. regards Philipp