Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
From:   Oleg Verych <olecom@gmail.com>
To:     wens@kernel.org
Cc:     devicetree@vger.kernel.org, linux-arm-kernel@lists.infradead.org,
        linux-kernel@vger.kernel.org, linux-media@vger.kernel.org,
        mark.rutland@arm.com, mchehab@kernel.org, mripard@kernel.org,
        robh+dt@kernel.org, sakari.ailus@linux.intel.com, wens@csie.org,
        Oleg Verych <olecom@gmail.com>
Subject: Re: [PATCH 04/14] media: sun4i-csi: Fix [HV]sync polarity handling
Date:   Mon, 16 Jan 2023 13:03:59 +0300
Message-Id: <20230116100359.4479-1-olecom@gmail.com>
In-Reply-To: <20191215165924.28314-5-wens@kernel.org>
References: <20191215165924.28314-5-wens@kernel.org>
MIME-Version: 1.0
Content-Transfer-Encoding: 8bit
Precedence: bulk

Hi, Chen-Yu Tsai

> -	hsync_pol = !!(bus->flags & V4L2_MBUS_HSYNC_ACTIVE_HIGH);
> -	vsync_pol = !!(bus->flags & V4L2_MBUS_VSYNC_ACTIVE_HIGH);
> +	/*
> +	 * This hardware uses [HV]REF instead of [HV]SYNC. Based on the
> +	 * provided timing diagrams in the manual, positive polarity
> +	 * equals active high [HV]REF.
> +	 *
> +	 * When the back porch is 0, [HV]REF is more or less equivalent
> +	 * to [HV]SYNC inverted.
> +	 */
> +	href_pol = !!(bus->flags & V4L2_MBUS_HSYNC_ACTIVE_LOW);
> +	vref_pol = !!(bus->flags & V4L2_MBUS_VSYNC_ACTIVE_LOW);

After this change has been made there is a need of explicit explanation
of what "Active high" / "Active low" in dts really mean.

Currently physical high/low voltage levels are like that:
(I'm not sure about vsync-active)

* hsync-active = <0>; /* HSYNC active 'low' => wire active is 'high' */
  CSI register setting: href_pol: 1,

That is confusing:

[PATCH v6 5/5] DO NOT MERGE: ARM: dts: bananapi: Add Camera support
https://lore.kernel.org/linux-arm-kernel/cf0e40b0bca9219d2bb023a5b7f23bad8baba1e5.1562847292.git-series.maxime.ripard@bootlin.com/#r

> +	port {
> +		csi_from_ov5640: endpoint {
> +                        remote-endpoint = <&ov5640_to_csi>;
> +                        bus-width = <8>;
> +                        hsync-active = <1>; /* Active high */

original CSI driver

> +                        vsync-active = <0>; /* Active low */
> +                        data-active = <1>;  /* Active high */
> +                        pclk-sample = <1>;  /* Rising */
> +                };
> +	};

[PATCH 13/14] [DO NOT MERGE] ARM: dts: sun7i: cubieboard2: Enable OV7670 camera on CSI1
https://lore.kernel.org/linux-arm-kernel/20191215165924.28314-14-wens@kernel.org/

> +	port {
> +		/* Parallel bus endpoint */
> +		csi_from_ov7670: endpoint {
> +			remote-endpoint = <&ov7670_to_csi>;
> +			bus-width = <8>;
> +			/* driver is broken */
> +			hsync-active = <0>; /* Active high */

this change patchset

> +			vsync-active = <1>; /* Active high */
> +			data-active = <1>;  /* Active high */
> +			pclk-sample = <1>;  /* Rising */
> +		};

> +			ov7670_to_csi: endpoint {
> +				remote-endpoint = <&csi_from_ov7670>;
> +				bus-width = <8>;
> +				hsync-active = <1>; /* Active high */

this patcheset

> +				vsync-active = <1>; /* Active high */
> +				data-active = <1>;  /* Active high */
> +				pclk-sample = <1>;  /* Rising */
> +			};
> +		};