by Hans Verkuil (hansverk)

[permalink] [raw]

Subject: Re: [PATCH 3/5] media: sunxi: Add A10 CSI driver

On 11/13/18 13:48, Fabio Estevam wrote:
> On Tue, Nov 13, 2018 at 6:25 AM Maxime Ripard <[email protected]> wrote:
>
>> --- /dev/null
>> +++ b/drivers/media/platform/sunxi/sun4i-csi/sun4i_csi.c
>> @@ -0,0 +1,275 @@
>> +// SPDX-License-Identifier: GPL-2.0-or-later
>
> According to Documentation/process/license-rules.rst this should be:
>
> +// SPDX-License-Identifier: GPL-2.0+
>
> Same applies to other places in this patch.
>

Actually, LICENSES/preferred/GPL-2.0 has GPL-2.0-or-later
as a valid license:

Valid-License-Identifier: GPL-2.0-or-later

Personally I very much prefer GPL-2.0-or-later since I think it is
much clearer.

Regards,

Hans

2018-11-13 13:54:30

On Tue, Nov 13, 2018 at 03:01:45PM +0100, Hans Verkuil wrote:
> On 11/13/18 14:52, Maxime Ripard wrote:
> > Hi Hans,
> >
> > On Tue, Nov 13, 2018 at 01:30:49PM +0100, Hans Verkuil wrote:
> >> On 11/13/18 09:24, Maxime Ripard wrote:
> >>> Hi,
> >>>
> >>> Here is a series introducing the support for the A10 (and SoCs of the same
> >>> generation) CMOS Sensor Interface (called CSI, not to be confused with
> >>> MIPI-CSI, which isn't support by that IP).
> >>>
> >>> That interface is pretty straightforward, but the driver has a few issues
> >>> that I wanted to bring up:
> >>>
> >>> * The only board I've been testing this with has an ov5640 sensor
> >>> attached, which doesn't work with the upstream driver. Copying the
> >>> Allwinner init sequence works though, and this is how it has been
> >>> tested. Testing with a second sensor would allow to see if it's an
> >>> issue on the CSI side or the sensor side.
> >>> * When starting a capture, the last buffer to capture will fail due to
> >>> double buffering being used, and we don't have a next buffer for the
> >>> last frame. I'm not sure how to deal with that though. It seems like
> >>> some drivers use a scratch buffer in such a case, some don't care, so
> >>> I'm not sure which solution should be preferred.
> >>> * We don't have support for the ISP at the moment, but this can be added
> >>> eventually.
> >>>
> >>> * How to model the CSI module clock isn't really clear to me. It looks
> >>> like it goes through the CSI controller and then is muxed to one of the
> >>> CSI pin so that it can clock the sensor. I'm not quite sure how to
> >>> model it, if it should be a clock, the CSI driver being a clock
> >>> provider, or if the sensor should just use the module clock directly.
> >>>
> >>> Here is the v4l2-compliance output:
> >>
> >> Test v4l2-compliance with the -s option so you test streaming as well.
> >> Even better is -f where it tests streaming with all available formats.
> >
> > I will, thanks for the tip!
> >
> >>> v4l2-compliance SHA : 339d550e92ac15de8668f32d66d16f198137006c
> >>
> >> Hmm, I can't find this SHA. Was this built from the main v4l-utils repo?
> >
> > It was, but using Buildroot. The version packaged in the latest stable
> > version I was using (2018.08) is 1.14.2.
>
> That's seriously out of date. That's why I show the SHA, to see if
> someone is testing with a recent version of the utility, so it served
> its purpose here :-)
>
> Latest release is 1.16.2.
>
> But when submitting new drivers you really need to build it yourself from
> the master branch, that's the only way to be sure you have all the latest
> compliance checks.

Ack, I'll update it and test again then.

> >
> > Looking at the Makefile from v4l2-compliance, it looks like it just
> > invokes git to retrieve the git commit and uses that as the hash. In
> > Buildroot's case, since buildroot will download the tarball, this will
> > end up returning the SHA commit of the buildroot repo building the
> > sources, not the version of the sources themselves.
> >
> > I'm not sure how to address that properly though. Thomas, how do you
> > usually deal with this?
>
> Note that cec-compliance and cec-follower do the same, for the same
> reason.
>
> Where does the tarball come from?

This is the official tarball from linuxtv:
https://git.buildroot.net/buildroot/tree/package/libv4l/libv4l.mk?h=2018.08.2#n8

Maxime

--
Maxime Ripard, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com

Attachments:

(No filename) (3.55 kB)
signature.asc (235.00 B)
Download all attachments

2018-11-13 16:01:45

by Hans Verkuil (hansverk)

[permalink] [raw]

Subject: Re: [PATCH 0/5] media: Allwinner A10 CSI support

On 11/13/18 16:52, Maxime Ripard wrote:
> On Tue, Nov 13, 2018 at 03:01:45PM +0100, Hans Verkuil wrote:
>> On 11/13/18 14:52, Maxime Ripard wrote:
>>> Hi Hans,
>>>
>>> On Tue, Nov 13, 2018 at 01:30:49PM +0100, Hans Verkuil wrote:
>>>> On 11/13/18 09:24, Maxime Ripard wrote:
>>>>> Hi,
>>>>>
>>>>> Here is a series introducing the support for the A10 (and SoCs of the same
>>>>> generation) CMOS Sensor Interface (called CSI, not to be confused with
>>>>> MIPI-CSI, which isn't support by that IP).
>>>>>
>>>>> That interface is pretty straightforward, but the driver has a few issues
>>>>> that I wanted to bring up:
>>>>>
>>>>> * The only board I've been testing this with has an ov5640 sensor
>>>>> attached, which doesn't work with the upstream driver. Copying the
>>>>> Allwinner init sequence works though, and this is how it has been
>>>>> tested. Testing with a second sensor would allow to see if it's an
>>>>> issue on the CSI side or the sensor side.
>>>>> * When starting a capture, the last buffer to capture will fail due to
>>>>> double buffering being used, and we don't have a next buffer for the
>>>>> last frame. I'm not sure how to deal with that though. It seems like
>>>>> some drivers use a scratch buffer in such a case, some don't care, so
>>>>> I'm not sure which solution should be preferred.
>>>>> * We don't have support for the ISP at the moment, but this can be added
>>>>> eventually.
>>>>>
>>>>> * How to model the CSI module clock isn't really clear to me. It looks
>>>>> like it goes through the CSI controller and then is muxed to one of the
>>>>> CSI pin so that it can clock the sensor. I'm not quite sure how to
>>>>> model it, if it should be a clock, the CSI driver being a clock
>>>>> provider, or if the sensor should just use the module clock directly.
>>>>>
>>>>> Here is the v4l2-compliance output:
>>>>
>>>> Test v4l2-compliance with the -s option so you test streaming as well.
>>>> Even better is -f where it tests streaming with all available formats.
>>>
>>> I will, thanks for the tip!
>>>
>>>>> v4l2-compliance SHA : 339d550e92ac15de8668f32d66d16f198137006c
>>>>
>>>> Hmm, I can't find this SHA. Was this built from the main v4l-utils repo?
>>>
>>> It was, but using Buildroot. The version packaged in the latest stable
>>> version I was using (2018.08) is 1.14.2.
>>
>> That's seriously out of date. That's why I show the SHA, to see if
>> someone is testing with a recent version of the utility, so it served
>> its purpose here :-)
>>
>> Latest release is 1.16.2.
>>
>> But when submitting new drivers you really need to build it yourself from
>> the master branch, that's the only way to be sure you have all the latest
>> compliance checks.
>
> Ack, I'll update it and test again then.
>
>>>
>>> Looking at the Makefile from v4l2-compliance, it looks like it just
>>> invokes git to retrieve the git commit and uses that as the hash. In
>>> Buildroot's case, since buildroot will download the tarball, this will
>>> end up returning the SHA commit of the buildroot repo building the
>>> sources, not the version of the sources themselves.
>>>
>>> I'm not sure how to address that properly though. Thomas, how do you
>>> usually deal with this?
>>
>> Note that cec-compliance and cec-follower do the same, for the same
>> reason.
>>
>> Where does the tarball come from?
>
> This is the official tarball from linuxtv:
> https://git.buildroot.net/buildroot/tree/package/libv4l/libv4l.mk?h=2018.08.2#n8

Weird, if I build directly from that tarball, then v4l2-compliance should say:

v4l2-compliance SHA: not available, 64 bits

So that's what I expect to see from buildroot as well.

Regards,

Hans

2018-11-13 16:58:38

by Thomas Petazzoni

[permalink] [raw]

Subject: Re: [PATCH 0/5] media: Allwinner A10 CSI support

Hello,

On Tue, 13 Nov 2018 17:00:25 +0100, Hans Verkuil wrote:

> Weird, if I build directly from that tarball, then v4l2-compliance should say:
>
> v4l2-compliance SHA: not available, 64 bits
>
> So that's what I expect to see from buildroot as well.

Indeed, that's strange, I see that the v4l2-compliance Makefile does:

version.h:
@if git -C $(srcdir) rev-parse HEAD >/dev/null 2>&1; then \
echo -n "#define SHA " >$@ ; \
git -C $(srcdir) rev-parse HEAD >>$@ ; \
else \
touch $@ ; \
fi

which correctly uses $(srcdir), so it shouldn't go "up" the libv4l
build folder and pick up the latest Buildroot commit SHA1. I'll have a
quick look.

Best regards,

Thomas
--
Thomas Petazzoni, CTO, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com

2018-11-13 17:19:09

by Hans Verkuil

[permalink] [raw]

Subject: Re: [PATCH 0/5] media: Allwinner A10 CSI support

On 11/13/2018 05:55 PM, Thomas Petazzoni wrote:
> Hello,
>
> On Tue, 13 Nov 2018 17:00:25 +0100, Hans Verkuil wrote:
>
>> Weird, if I build directly from that tarball, then v4l2-compliance should say:
>>
>> v4l2-compliance SHA: not available, 64 bits
>>
>> So that's what I expect to see from buildroot as well.
>
> Indeed, that's strange, I see that the v4l2-compliance Makefile does:
>
> version.h:
> @if git -C $(srcdir) rev-parse HEAD >/dev/null 2>&1; then \
> echo -n "#define SHA " >$@ ; \
> git -C $(srcdir) rev-parse HEAD >>$@ ; \
> else \
> touch $@ ; \
> fi
>
> which correctly uses $(srcdir), so it shouldn't go "up" the libv4l
> build folder and pick up the latest Buildroot commit SHA1. I'll have a
> quick look.

I think it does, actually. If the tar archive is unpacked inside the
checked-out buildroot git tree, then it will pick up the buildroot SHA.

I fixed v4l-utils to be a bit smarter about this:

https://git.linuxtv.org/v4l-utils.git/patch/?id=98b4c9f276a18535b5691e5f350f59ffbf5a9aa5

Regards,

Hans

2018-11-14 03:25:50

by Chen-Yu Tsai

Hi Sakari,

On Thu, Nov 22, 2018 at 12:01:03AM +0200, Sakari Ailus wrote:
> On Thu, Nov 15, 2018 at 09:51:06PM +0100, Maxime Ripard wrote:
> > Hi Hans,
> >
> > Thanks for your review! I'll address the other comments you made.
> >
> > On Tue, Nov 13, 2018 at 01:24:47PM +0100, Hans Verkuil wrote:
> > > > +static int csi_probe(struct platform_device *pdev)
> > > > +{
> > > > + struct sun4i_csi *csi;
> > > > + struct resource *res;
> > > > + int ret;
> > > > + int irq;
> > > > +
> > > > + csi = devm_kzalloc(&pdev->dev, sizeof(*csi), GFP_KERNEL);
> > >
> > > devm_kzalloc is not recommended: all devm_ memory is freed when the driver
> > > is unbound, but a filehandle might still have a reference open.
> >
> > How would a !devm variant with a kfree in the remove help? We would
> > still fall in the same case, right?
>
> Not quite. For video nodes this is handled: the release callback gets called
> once there are no file handles open to the device. That may well be much
> later than the device has been unbound from the driver.

I might be missing something, but how the release callback will be
able to get the reference to the structure we allocated here so that
it can free it?

Thanks!
Maxime

--
Maxime Ripard, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com

Attachments:

(No filename) (1.30 kB)
signature.asc (235.00 B)
Download all attachments

2018-11-27 06:58:37

by Jagan Teki

[permalink] [raw]

Subject: Re: [PATCH 5/5] DO NOT MERGE: ARM: dts: bananapi: Add Camera support

On Tue, Nov 13, 2018 at 1:54 PM Maxime Ripard <[email protected]> wrote:
>
> Signed-off-by: Maxime Ripard <[email protected]>
> ---
> arch/arm/boot/dts/sun7i-a20-bananapi.dts | 98 +++++++++++++++++++++++++-
> 1 file changed, 98 insertions(+)
>
> diff --git a/arch/arm/boot/dts/sun7i-a20-bananapi.dts b/arch/arm/boot/dts/sun7i-a20-bananapi.dts
> index 70dfc4ac0bb5..18dbff9f1ce9 100644
> --- a/arch/arm/boot/dts/sun7i-a20-bananapi.dts
> +++ b/arch/arm/boot/dts/sun7i-a20-bananapi.dts
> @@ -54,6 +54,9 @@
> compatible = "lemaker,bananapi", "allwinner,sun7i-a20";
>
> aliases {
> + i2c0 = &i2c0;
> + i2c1 = &i2c1;
> + i2c2 = &i2c2;
> serial0 = &uart0;
> serial1 = &uart3;
> serial2 = &uart7;
> @@ -63,6 +66,41 @@
> stdout-path = "serial0:115200n8";
> };
>
> + reg_cam: cam {
> + compatible = "regulator-fixed";
> + regulator-name = "cam";
> + regulator-min-microvolt = <5000000>;
> + regulator-max-microvolt = <5000000>;
> + vin-supply = <&reg_vcc5v0>;
> + gpio = <&pio 7 16 GPIO_ACTIVE_HIGH>;
> + enable-active-high;
> + regulator-always-on;
> + };
> +
> + reg_cam_avdd: cam-avdd {
> + compatible = "regulator-fixed";
> + regulator-name = "cam500b-avdd";
> + regulator-min-microvolt = <2800000>;
> + regulator-max-microvolt = <2800000>;
> + vin-supply = <&reg_cam>;
> + };
> +
> + reg_cam_dovdd: cam-dovdd {
> + compatible = "regulator-fixed";
> + regulator-name = "cam500b-dovdd";
> + regulator-min-microvolt = <1800000>;
> + regulator-max-microvolt = <1800000>;
> + vin-supply = <&reg_cam>;
> + };
> +
> + reg_cam_dvdd: cam-dvdd {
> + compatible = "regulator-fixed";
> + regulator-name = "cam500b-dvdd";
> + regulator-min-microvolt = <1500000>;
> + regulator-max-microvolt = <1500000>;
> + vin-supply = <&reg_cam>;
> + };
> +
> hdmi-connector {
> compatible = "hdmi-connector";
> type = "a";
> @@ -120,6 +158,27 @@
> >;
> };
>
> +&csi0 {
> + pinctrl-names = "default";
> + pinctrl-0 = <&csi0_pins_a>;
> + status = "okay";
> +
> + port {
> + #address-cells = <1>;
> + #size-cells = <0>;
> +
> + csi_from_ov5640: endpoint {
> + remote-endpoint = <&ov5640_to_csi>;
> + bus-width = <8>;
> + data-shift = <2>;
> + hsync-active = <1>; /* Active high */
> + vsync-active = <0>; /* Active low */
> + data-active = <1>; /* Active high */
> + pclk-sample = <1>; /* Rising */
> + };
> + };
> +};
> +
> &de {
> status = "okay";
> };
> @@ -167,6 +226,39 @@
> };
> };
>
> +&i2c1 {
> + pinctrl-names = "default";
> + pinctrl-0 = <&i2c1_pins_a>;
> + status = "okay";
> +
> + camera: camera@21 {
> + compatible = "ovti,ov5640";
> + reg = <0x21>;
> + clocks = <&ccu CLK_CSI0>;
> + clock-names = "xclk";
> + assigned-clocks = <&ccu CLK_CSI0>;
> + assigned-clock-rates = <24000000>;
> +
> + reset-gpios = <&pio 7 14 GPIO_ACTIVE_LOW>;
> + powerdown-gpios = <&pio 7 19 GPIO_ACTIVE_HIGH>;
> + AVDD-supply = <&reg_cam_avdd>;
> + DOVDD-supply = <&reg_cam_dovdd>;
> + DVDD-supply = <&reg_cam_dvdd>;
> +
> + port {
> + ov5640_to_csi: endpoint {
> + remote-endpoint = <&csi_from_ov5640>;
> + bus-width = <8>;
> + data-shift = <2>;
> + hsync-active = <1>; /* Active high */
> + vsync-active = <0>; /* Active low */
> + data-active = <1>; /* Active high */
> + pclk-sample = <1>; /* Rising */
> + };
> + };
> + };

Does ov5640 need any further patches, wrt linux-next? I'm trying to
test this on top of linux-next but the slave id seems not detecting.

[ 2.304711] ov5640 1-0021: Linked as a consumer to regulator.5
[ 2.310639] ov5640 1-0021: Linked as a consumer to regulator.6
[ 2.316592] ov5640 1-0021: Linked as a consumer to regulator.4
[ 2.351540] ov5640 1-0021: ov5640_init_slave_id: failed with -6
[ 2.357543] ov5640 1-0021: Dropping the link to regulator.5
[ 2.363224] ov5640 1-0021: Dropping the link to regulator.6
[ 2.368829] ov5640 1-0021: Dropping the link to regulator.4

Here is the full log [1], please let me know if I miss anything, I
even tried to remove MCLK pin

[1] https://paste.ubuntu.com/p/yfy5cvs32x/

2018-11-27 12:20:37

by Jagan Teki

On Tue, Nov 27, 2018 at 8:49 PM Maxime Ripard <[email protected]> wrote:
>
> On Tue, Nov 27, 2018 at 04:30:55PM +0530, Jagan Teki wrote:
> > > > > +&i2c1 {
> > > > > + pinctrl-names = "default";
> > > > > + pinctrl-0 = <&i2c1_pins_a>;
> > > > > + status = "okay";
> > > > > +
> > > > > + camera: camera@21 {
> > > > > + compatible = "ovti,ov5640";
> > > > > + reg = <0x21>;
> > > > > + clocks = <&ccu CLK_CSI0>;
> > > > > + clock-names = "xclk";
> > > > > + assigned-clocks = <&ccu CLK_CSI0>;
> > > > > + assigned-clock-rates = <24000000>;
> > > > > +
> > > > > + reset-gpios = <&pio 7 14 GPIO_ACTIVE_LOW>;
> > > > > + powerdown-gpios = <&pio 7 19 GPIO_ACTIVE_HIGH>;
> > > > > + AVDD-supply = <&reg_cam_avdd>;
> > > > > + DOVDD-supply = <&reg_cam_dovdd>;
> > > > > + DVDD-supply = <&reg_cam_dvdd>;
> > > > > +
> > > > > + port {
> > > > > + ov5640_to_csi: endpoint {
> > > > > + remote-endpoint = <&csi_from_ov5640>;
> > > > > + bus-width = <8>;
> > > > > + data-shift = <2>;
> > > > > + hsync-active = <1>; /* Active high */
> > > > > + vsync-active = <0>; /* Active low */
> > > > > + data-active = <1>; /* Active high */
> > > > > + pclk-sample = <1>; /* Rising */
> > > > > + };
> > > > > + };
> > > > > + };
> > > >
> > > > Does ov5640 need any further patches, wrt linux-next? I'm trying to
> > > > test this on top of linux-next but the slave id seems not detecting.
> > > >
> > > > [ 2.304711] ov5640 1-0021: Linked as a consumer to regulator.5
> > > > [ 2.310639] ov5640 1-0021: Linked as a consumer to regulator.6
> > > > [ 2.316592] ov5640 1-0021: Linked as a consumer to regulator.4
> > > > [ 2.351540] ov5640 1-0021: ov5640_init_slave_id: failed with -6
> > > > [ 2.357543] ov5640 1-0021: Dropping the link to regulator.5
> > > > [ 2.363224] ov5640 1-0021: Dropping the link to regulator.6
> > > > [ 2.368829] ov5640 1-0021: Dropping the link to regulator.4
> > > >
> > > > Here is the full log [1], please let me know if I miss anything, I
> > > > even tried to remove MCLK pin
> > >
> > > You seem to have made local modifications to your tree, what are they?
> > > This indicates that the communication over i2c doesn't work, what is
> > > your setup?
> >
> > I just used your commits on linux-next [2], with the setup similar in
> > Page 5 on datasheet[3]. The only difference is csi build issue, I have
> > updated similar fix you mentioned on sun6i_csi [4]
> >
> > [2] https://github.com/amarula/linux-amarula/commits/CSI-A20
> > [3] https://www.tme.eu/gb/Document/187887186b98a8f78b47da2774a34f4c/BPI-CAMERA.pdf
> > [4] https://github.com/amarula/linux-amarula/commit/a6762ecd38f000e2bd02dd255f6fd0c1ae755429#diff-0809a7f97ca58771c1cda186e73ec657
>
> That branch doesn't have any commit with the same ID that you have in
> your boot log.

I have created this branch for your reference, here is the clean log on this [5]

[5] https://paste.ubuntu.com/p/4bkFs5WG6c/