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