* [PATCH v2 0/3] media: i2c: ov772x: Enable BT656 mode and test pattern support
@ 2020-08-03 11:39 Lad Prabhakar
2020-08-03 11:39 ` [PATCH v2 1/3] dt-bindings: media: ov772x: Document endpoint properties Lad Prabhakar
` (2 more replies)
0 siblings, 3 replies; 11+ messages in thread
From: Lad Prabhakar @ 2020-08-03 11:39 UTC (permalink / raw
To: Jacopo Mondi, Mauro Carvalho Chehab, Rob Herring, Hans Verkuil,
Kieran Bingham, Sakari Ailus, linux-media
Cc: devicetree, linux-kernel, Biju Das, Prabhakar, linux-renesas-soc,
Lad Prabhakar
Hi All,
This patch series adds support for BT656 mode in the ov772x sensor
and also enables color bar test pattern control.
Cheers,
Prabhakar
Changes for v2:
* Updated DT bindings
* Driver defaults to parallel mode
* Fixed return type when endpoint parsing fails
Lad Prabhakar (3):
dt-bindings: media: ov772x: Document endpoint properties
media: i2c: ov772x: Add support for BT656 mode
media: i2c: ov772x: Add test pattern control
.../devicetree/bindings/media/i2c/ov772x.txt | 16 +++++
drivers/media/i2c/ov772x.c | 65 ++++++++++++++++++-
include/media/i2c/ov772x.h | 1 +
3 files changed, 81 insertions(+), 1 deletion(-)
--
2.17.1
^ permalink raw reply [flat|nested] 11+ messages in thread
* [PATCH v2 1/3] dt-bindings: media: ov772x: Document endpoint properties
2020-08-03 11:39 [PATCH v2 0/3] media: i2c: ov772x: Enable BT656 mode and test pattern support Lad Prabhakar
@ 2020-08-03 11:39 ` Lad Prabhakar
2020-08-06 14:35 ` Jacopo Mondi
2020-08-15 10:34 ` Lad, Prabhakar
2020-08-03 11:39 ` [PATCH v2 2/3] media: i2c: ov772x: Add support for BT656 mode Lad Prabhakar
2020-08-03 11:39 ` [PATCH v2 3/3] media: i2c: ov772x: Add test pattern control Lad Prabhakar
2 siblings, 2 replies; 11+ messages in thread
From: Lad Prabhakar @ 2020-08-03 11:39 UTC (permalink / raw
To: Jacopo Mondi, Mauro Carvalho Chehab, Rob Herring, Hans Verkuil,
Kieran Bingham, Sakari Ailus, linux-media
Cc: devicetree, linux-kernel, Biju Das, Prabhakar, linux-renesas-soc,
Lad Prabhakar
Document endpoint properties required for parallel interface
Signed-off-by: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>
---
.../devicetree/bindings/media/i2c/ov772x.txt | 16 ++++++++++++++++
1 file changed, 16 insertions(+)
diff --git a/Documentation/devicetree/bindings/media/i2c/ov772x.txt b/Documentation/devicetree/bindings/media/i2c/ov772x.txt
index 0b3ede5b8e6a..1f4153484717 100644
--- a/Documentation/devicetree/bindings/media/i2c/ov772x.txt
+++ b/Documentation/devicetree/bindings/media/i2c/ov772x.txt
@@ -21,6 +21,22 @@ subnode for its digital output video port, in accordance with the video
interface bindings defined in Documentation/devicetree/bindings/media/
video-interfaces.txt.
+Endpoint node required properties for parallel connection are:
+- remote-endpoint: a phandle to the bus receiver's endpoint node.
+- bus-width: shall be set to <8> for 8 bits parallel bus
+ or <10> for 10 bits parallel bus
+- data-shift: shall be set to <2> for 8 bits parallel bus
+ (lines 9:2 are used) or <0> for 10 bits parallel bus
+- hsync-active: active state of the HSYNC signal, 0/1 for LOW/HIGH respectively.
+ (Not required for bus-type equal 6)
+- vsync-active: active state of the VSYNC signal, 0/1 for LOW/HIGH respectively.
+ (Not required for bus-type equal 6)
+- pclk-sample: sample data on rising (1) or falling (0) edge of the pixel clock
+ signal. (Not required for bus-type equal 6)
+- bus-type: data bus type. Possible values are:
+ 5 - Parallel
+ 6 - Bt.656
+
Example:
&i2c0 {
--
2.17.1
^ permalink raw reply related [flat|nested] 11+ messages in thread
* [PATCH v2 2/3] media: i2c: ov772x: Add support for BT656 mode
2020-08-03 11:39 [PATCH v2 0/3] media: i2c: ov772x: Enable BT656 mode and test pattern support Lad Prabhakar
2020-08-03 11:39 ` [PATCH v2 1/3] dt-bindings: media: ov772x: Document endpoint properties Lad Prabhakar
@ 2020-08-03 11:39 ` Lad Prabhakar
2020-08-06 14:45 ` Jacopo Mondi
2020-08-03 11:39 ` [PATCH v2 3/3] media: i2c: ov772x: Add test pattern control Lad Prabhakar
2 siblings, 1 reply; 11+ messages in thread
From: Lad Prabhakar @ 2020-08-03 11:39 UTC (permalink / raw
To: Jacopo Mondi, Mauro Carvalho Chehab, Rob Herring, Hans Verkuil,
Kieran Bingham, Sakari Ailus, linux-media
Cc: devicetree, linux-kernel, Biju Das, Prabhakar, linux-renesas-soc,
Lad Prabhakar
Add support to read the bus-type and enable BT656 mode if needed.
The driver defaults to parallel mode if bus-type is not specified in DT.
Signed-off-by: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>
Reviewed-by: Biju Das <biju.das.jz@bp.renesas.com>
---
drivers/media/i2c/ov772x.c | 40 ++++++++++++++++++++++++++++++++++++++
1 file changed, 40 insertions(+)
diff --git a/drivers/media/i2c/ov772x.c b/drivers/media/i2c/ov772x.c
index 2cc6a678069a..2de9248e3689 100644
--- a/drivers/media/i2c/ov772x.c
+++ b/drivers/media/i2c/ov772x.c
@@ -31,6 +31,7 @@
#include <media/v4l2-ctrls.h>
#include <media/v4l2-device.h>
#include <media/v4l2-event.h>
+#include <media/v4l2-fwnode.h>
#include <media/v4l2-image-sizes.h>
#include <media/v4l2-subdev.h>
@@ -434,6 +435,7 @@ struct ov772x_priv {
#ifdef CONFIG_MEDIA_CONTROLLER
struct media_pad pad;
#endif
+ struct v4l2_fwnode_endpoint ep;
};
/*
@@ -574,6 +576,7 @@ static int ov772x_s_stream(struct v4l2_subdev *sd, int enable)
{
struct i2c_client *client = v4l2_get_subdevdata(sd);
struct ov772x_priv *priv = to_ov772x(sd);
+ unsigned int val;
int ret = 0;
mutex_lock(&priv->lock);
@@ -581,6 +584,22 @@ static int ov772x_s_stream(struct v4l2_subdev *sd, int enable)
if (priv->streaming == enable)
goto done;
+ if (priv->ep.bus_type == V4L2_MBUS_BT656 && enable) {
+ ret = regmap_read(priv->regmap, COM7, &val);
+ if (ret)
+ goto done;
+ val |= ITU656_ON_OFF;
+ ret = regmap_write(priv->regmap, COM7, val);
+ } else if (priv->ep.bus_type == V4L2_MBUS_BT656 && !enable) {
+ ret = regmap_read(priv->regmap, COM7, &val);
+ if (ret)
+ goto done;
+ val &= ~ITU656_ON_OFF;
+ ret = regmap_write(priv->regmap, COM7, val);
+ }
+ if (ret)
+ goto done;
+
ret = regmap_update_bits(priv->regmap, COM2, SOFT_SLEEP_MODE,
enable ? 0 : SOFT_SLEEP_MODE);
if (ret)
@@ -1354,6 +1373,7 @@ static const struct v4l2_subdev_ops ov772x_subdev_ops = {
static int ov772x_probe(struct i2c_client *client)
{
+ struct fwnode_handle *endpoint;
struct ov772x_priv *priv;
int ret;
static const struct regmap_config ov772x_regmap_config = {
@@ -1415,6 +1435,26 @@ static int ov772x_probe(struct i2c_client *client)
goto error_clk_put;
}
+ endpoint = fwnode_graph_get_next_endpoint(dev_fwnode(&client->dev),
+ NULL);
+ if (!endpoint) {
+ dev_err(&client->dev, "endpoint node not found\n");
+ ret = -EINVAL;
+ goto error_clk_put;
+ }
+
+ ret = v4l2_fwnode_endpoint_parse(endpoint, &priv->ep);
+ fwnode_handle_put(endpoint);
+ if (ret) {
+ dev_err(&client->dev, "Could not parse endpoint\n");
+ goto error_clk_put;
+ }
+
+ /* fallback to parallel mode */
+ if (priv->ep.bus_type != V4L2_MBUS_PARALLEL &&
+ priv->ep.bus_type != V4L2_MBUS_BT656)
+ priv->ep.bus_type = V4L2_MBUS_PARALLEL;
+
ret = ov772x_video_probe(priv);
if (ret < 0)
goto error_gpio_put;
--
2.17.1
^ permalink raw reply related [flat|nested] 11+ messages in thread
* [PATCH v2 3/3] media: i2c: ov772x: Add test pattern control
2020-08-03 11:39 [PATCH v2 0/3] media: i2c: ov772x: Enable BT656 mode and test pattern support Lad Prabhakar
2020-08-03 11:39 ` [PATCH v2 1/3] dt-bindings: media: ov772x: Document endpoint properties Lad Prabhakar
2020-08-03 11:39 ` [PATCH v2 2/3] media: i2c: ov772x: Add support for BT656 mode Lad Prabhakar
@ 2020-08-03 11:39 ` Lad Prabhakar
2 siblings, 0 replies; 11+ messages in thread
From: Lad Prabhakar @ 2020-08-03 11:39 UTC (permalink / raw
To: Jacopo Mondi, Mauro Carvalho Chehab, Rob Herring, Hans Verkuil,
Kieran Bingham, Sakari Ailus, linux-media
Cc: devicetree, linux-kernel, Biju Das, Prabhakar, linux-renesas-soc,
Lad Prabhakar
Add support for test pattern control supported by the sensor.
Signed-off-by: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>
Reviewed-by: Biju Das <biju.das.jz@bp.renesas.com>
---
drivers/media/i2c/ov772x.c | 25 ++++++++++++++++++++++++-
include/media/i2c/ov772x.h | 1 +
2 files changed, 25 insertions(+), 1 deletion(-)
diff --git a/drivers/media/i2c/ov772x.c b/drivers/media/i2c/ov772x.c
index 2de9248e3689..457887ec548d 100644
--- a/drivers/media/i2c/ov772x.c
+++ b/drivers/media/i2c/ov772x.c
@@ -227,7 +227,7 @@
/* COM3 */
#define SWAP_MASK (SWAP_RGB | SWAP_YUV | SWAP_ML)
-#define IMG_MASK (VFLIP_IMG | HFLIP_IMG)
+#define IMG_MASK (VFLIP_IMG | HFLIP_IMG | SCOLOR_TEST)
#define VFLIP_IMG 0x80 /* Vertical flip image ON/OFF selection */
#define HFLIP_IMG 0x40 /* Horizontal mirror image ON/OFF selection */
@@ -425,6 +425,7 @@ struct ov772x_priv {
const struct ov772x_win_size *win;
struct v4l2_ctrl *vflip_ctrl;
struct v4l2_ctrl *hflip_ctrl;
+ unsigned int test_pattern;
/* band_filter = COM8[5] ? 256 - BDBASE : 0 */
struct v4l2_ctrl *band_filter_ctrl;
unsigned int fps;
@@ -540,6 +541,11 @@ static const struct ov772x_win_size ov772x_win_sizes[] = {
},
};
+static const char * const ov772x_test_pattern_menu[] = {
+ "Disabled",
+ "Vertical Color Bar Type 1",
+};
+
/*
* frame rate settings lists
*/
@@ -772,6 +778,13 @@ static int ov772x_s_frame_interval(struct v4l2_subdev *sd,
return ret;
}
+static int ov772x_enable_test_pattern(struct ov772x_priv *priv, u32 pattern)
+{
+ priv->test_pattern = pattern;
+ return regmap_update_bits(priv->regmap, COM3, SCOLOR_TEST,
+ pattern ? SCOLOR_TEST : 0x00);
+}
+
static int ov772x_s_ctrl(struct v4l2_ctrl *ctrl)
{
struct ov772x_priv *priv = container_of(ctrl->handler,
@@ -819,6 +832,8 @@ static int ov772x_s_ctrl(struct v4l2_ctrl *ctrl)
}
return ret;
+ case V4L2_CID_TEST_PATTERN:
+ return ov772x_enable_test_pattern(priv, ctrl->val);
}
return -EINVAL;
@@ -1113,10 +1128,14 @@ static int ov772x_set_params(struct ov772x_priv *priv,
val |= VFLIP_IMG;
if (priv->info && (priv->info->flags & OV772X_FLAG_HFLIP))
val |= HFLIP_IMG;
+ if (priv->info && (priv->info->flags & OV772X_FLAG_TEST_PATTERN))
+ val |= SCOLOR_TEST;
if (priv->vflip_ctrl->val)
val ^= VFLIP_IMG;
if (priv->hflip_ctrl->val)
val ^= HFLIP_IMG;
+ if (priv->test_pattern)
+ val ^= SCOLOR_TEST;
ret = regmap_update_bits(priv->regmap, COM3, SWAP_MASK | IMG_MASK, val);
if (ret < 0)
@@ -1414,6 +1433,10 @@ static int ov772x_probe(struct i2c_client *client)
priv->band_filter_ctrl = v4l2_ctrl_new_std(&priv->hdl, &ov772x_ctrl_ops,
V4L2_CID_BAND_STOP_FILTER,
0, 256, 1, 0);
+ v4l2_ctrl_new_std_menu_items(&priv->hdl, &ov772x_ctrl_ops,
+ V4L2_CID_TEST_PATTERN,
+ ARRAY_SIZE(ov772x_test_pattern_menu) - 1,
+ 0, 0, ov772x_test_pattern_menu);
priv->subdev.ctrl_handler = &priv->hdl;
if (priv->hdl.error) {
ret = priv->hdl.error;
diff --git a/include/media/i2c/ov772x.h b/include/media/i2c/ov772x.h
index a1702d420087..65e6f8d2f4bb 100644
--- a/include/media/i2c/ov772x.h
+++ b/include/media/i2c/ov772x.h
@@ -12,6 +12,7 @@
/* for flags */
#define OV772X_FLAG_VFLIP (1 << 0) /* Vertical flip image */
#define OV772X_FLAG_HFLIP (1 << 1) /* Horizontal flip image */
+#define OV772X_FLAG_TEST_PATTERN (1 << 2) /* Test pattern */
/*
* for Edge ctrl
--
2.17.1
^ permalink raw reply related [flat|nested] 11+ messages in thread
* Re: [PATCH v2 1/3] dt-bindings: media: ov772x: Document endpoint properties
2020-08-03 11:39 ` [PATCH v2 1/3] dt-bindings: media: ov772x: Document endpoint properties Lad Prabhakar
@ 2020-08-06 14:35 ` Jacopo Mondi
2020-08-10 7:22 ` Lad, Prabhakar
2020-08-15 10:34 ` Lad, Prabhakar
1 sibling, 1 reply; 11+ messages in thread
From: Jacopo Mondi @ 2020-08-06 14:35 UTC (permalink / raw
To: Lad Prabhakar
Cc: Mauro Carvalho Chehab, Rob Herring, Hans Verkuil, Kieran Bingham,
Sakari Ailus, linux-media, devicetree, linux-kernel, Biju Das,
Prabhakar, linux-renesas-soc
Hi Prabhakar,
On Mon, Aug 03, 2020 at 12:39:11PM +0100, Lad Prabhakar wrote:
> Document endpoint properties required for parallel interface
>
> Signed-off-by: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>
> ---
> .../devicetree/bindings/media/i2c/ov772x.txt | 16 ++++++++++++++++
> 1 file changed, 16 insertions(+)
>
> diff --git a/Documentation/devicetree/bindings/media/i2c/ov772x.txt b/Documentation/devicetree/bindings/media/i2c/ov772x.txt
> index 0b3ede5b8e6a..1f4153484717 100644
> --- a/Documentation/devicetree/bindings/media/i2c/ov772x.txt
> +++ b/Documentation/devicetree/bindings/media/i2c/ov772x.txt
> @@ -21,6 +21,22 @@ subnode for its digital output video port, in accordance with the video
> interface bindings defined in Documentation/devicetree/bindings/media/
> video-interfaces.txt.
>
> +Endpoint node required properties for parallel connection are:
> +- remote-endpoint: a phandle to the bus receiver's endpoint node.
we allow endpoints without a remote end connected usually. They can be
filled in later, in example, with an overlay.
> +- bus-width: shall be set to <8> for 8 bits parallel bus
> + or <10> for 10 bits parallel bus
> +- data-shift: shall be set to <2> for 8 bits parallel bus
> + (lines 9:2 are used) or <0> for 10 bits parallel bus
defining what is required or optional might be hard. I don't see the
driver enforcing their presence and I assume they have safe default.
Maybe make them optional and specify what the defaul value is ?
> +- hsync-active: active state of the HSYNC signal, 0/1 for LOW/HIGH respectively.
> + (Not required for bus-type equal 6)
> +- vsync-active: active state of the VSYNC signal, 0/1 for LOW/HIGH respectively.
> + (Not required for bus-type equal 6)
If they're not required, they're optional, aren't they ? :)
> +- pclk-sample: sample data on rising (1) or falling (0) edge of the pixel clock
> + signal. (Not required for bus-type equal 6)
Why the pclk polarity is does not apply to BT.656 ?
> +- bus-type: data bus type. Possible values are:
> + 5 - Parallel
> + 6 - Bt.656
Are we making this required, or do we expect this to be deduced
depending on which other properties have been specified ? Sakari it
seems you would like this to become a properties that has to be
specified most of the times, right ? (I tend to agree with that FWIW),
but does it impact retro-compatibility ?
> +
> Example:
>
> &i2c0 {
> --
> 2.17.1
>
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH v2 2/3] media: i2c: ov772x: Add support for BT656 mode
2020-08-03 11:39 ` [PATCH v2 2/3] media: i2c: ov772x: Add support for BT656 mode Lad Prabhakar
@ 2020-08-06 14:45 ` Jacopo Mondi
2020-08-10 7:25 ` Lad, Prabhakar
0 siblings, 1 reply; 11+ messages in thread
From: Jacopo Mondi @ 2020-08-06 14:45 UTC (permalink / raw
To: Lad Prabhakar
Cc: Mauro Carvalho Chehab, Rob Herring, Hans Verkuil, Kieran Bingham,
Sakari Ailus, linux-media, devicetree, linux-kernel, Biju Das,
Prabhakar, linux-renesas-soc
On Mon, Aug 03, 2020 at 12:39:12PM +0100, Lad Prabhakar wrote:
> Add support to read the bus-type and enable BT656 mode if needed.
>
> The driver defaults to parallel mode if bus-type is not specified in DT.
>
> Signed-off-by: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>
> Reviewed-by: Biju Das <biju.das.jz@bp.renesas.com>
> ---
> drivers/media/i2c/ov772x.c | 40 ++++++++++++++++++++++++++++++++++++++
> 1 file changed, 40 insertions(+)
>
> diff --git a/drivers/media/i2c/ov772x.c b/drivers/media/i2c/ov772x.c
> index 2cc6a678069a..2de9248e3689 100644
> --- a/drivers/media/i2c/ov772x.c
> +++ b/drivers/media/i2c/ov772x.c
> @@ -31,6 +31,7 @@
> #include <media/v4l2-ctrls.h>
> #include <media/v4l2-device.h>
> #include <media/v4l2-event.h>
> +#include <media/v4l2-fwnode.h>
> #include <media/v4l2-image-sizes.h>
> #include <media/v4l2-subdev.h>
>
> @@ -434,6 +435,7 @@ struct ov772x_priv {
> #ifdef CONFIG_MEDIA_CONTROLLER
> struct media_pad pad;
> #endif
> + struct v4l2_fwnode_endpoint ep;
> };
>
> /*
> @@ -574,6 +576,7 @@ static int ov772x_s_stream(struct v4l2_subdev *sd, int enable)
> {
> struct i2c_client *client = v4l2_get_subdevdata(sd);
> struct ov772x_priv *priv = to_ov772x(sd);
> + unsigned int val;
> int ret = 0;
>
> mutex_lock(&priv->lock);
> @@ -581,6 +584,22 @@ static int ov772x_s_stream(struct v4l2_subdev *sd, int enable)
> if (priv->streaming == enable)
> goto done;
>
> + if (priv->ep.bus_type == V4L2_MBUS_BT656 && enable) {
> + ret = regmap_read(priv->regmap, COM7, &val);
> + if (ret)
> + goto done;
> + val |= ITU656_ON_OFF;
> + ret = regmap_write(priv->regmap, COM7, val);
> + } else if (priv->ep.bus_type == V4L2_MBUS_BT656 && !enable) {
is the !enable intentional ? (sorry I don't have access to the sensor
manual). If not, see below:
> + ret = regmap_read(priv->regmap, COM7, &val);
> + if (ret)
> + goto done;
> + val &= ~ITU656_ON_OFF;
> + ret = regmap_write(priv->regmap, COM7, val);
> + }
> + if (ret)
> + goto done;
Could you write this as:
static int ov772x_s_stream(struct v4l2_subdev *sd, int enable)
{
struct i2c_client *client = v4l2_get_subdevdata(sd);
struct ov772x_priv *priv = to_ov772x(sd);
int ret = 0;
mutex_lock(&priv->lock);
if (priv->streaming == enable)
goto done;
if (enable) {
ret = regmap_read(priv->regmap, COM7, &val);
if (ret)
goto done;
if (priv->ep.bus_type == V4L2_MBUS_BT656)
val |= ITU656_ON_OFF;
else /* if you accept my suggestion to consider othe
bus types as errors */
val &= ~ITU656_ON_OFF;
ret = regmap_write(priv->regmap, COM7, val);
if (ret)
goto done;
dev_dbg(&client->dev, "format %d, win %s\n",
priv->cfmt->code, priv->win->name);
}
ret = regmap_update_bits(priv->regmap, COM2, SOFT_SLEEP_MODE,
enable ? 0 : SOFT_SLEEP_MODE);
if (ret)
goto done;
priv->streaming = enable;
done:
mutex_unlock(&priv->lock);
return ret;
}
> ret = regmap_update_bits(priv->regmap, COM2, SOFT_SLEEP_MODE,
> enable ? 0 : SOFT_SLEEP_MODE);
> if (ret)
> @@ -1354,6 +1373,7 @@ static const struct v4l2_subdev_ops ov772x_subdev_ops = {
>
> static int ov772x_probe(struct i2c_client *client)
> {
> + struct fwnode_handle *endpoint;
> struct ov772x_priv *priv;
> int ret;
> static const struct regmap_config ov772x_regmap_config = {
> @@ -1415,6 +1435,26 @@ static int ov772x_probe(struct i2c_client *client)
> goto error_clk_put;
> }
>
> + endpoint = fwnode_graph_get_next_endpoint(dev_fwnode(&client->dev),
> + NULL);
> + if (!endpoint) {
> + dev_err(&client->dev, "endpoint node not found\n");
> + ret = -EINVAL;
> + goto error_clk_put;
> + }
> +
> + ret = v4l2_fwnode_endpoint_parse(endpoint, &priv->ep);
> + fwnode_handle_put(endpoint);
> + if (ret) {
> + dev_err(&client->dev, "Could not parse endpoint\n");
> + goto error_clk_put;
> + }
> +
> + /* fallback to parallel mode */
> + if (priv->ep.bus_type != V4L2_MBUS_PARALLEL &&
> + priv->ep.bus_type != V4L2_MBUS_BT656)
> + priv->ep.bus_type = V4L2_MBUS_PARALLEL;
shouldn't this be an error ? It's either the bus type has not been
specified on DT (which is fine, otherwise old DTB without that
properties will fail) and the bus identification routine implemented
in v4l2_fwnode_endpoint_parse() detected a bus type which is not
supported, hence the DT properties are wrong, and this should be an
error. If you plan to expand the parsing routine to support, say
bus-width and pclk polarity please break this out to a new function.
Thanks
j
> +
> ret = ov772x_video_probe(priv);
> if (ret < 0)
> goto error_gpio_put;
> --
> 2.17.1
>
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH v2 1/3] dt-bindings: media: ov772x: Document endpoint properties
2020-08-06 14:35 ` Jacopo Mondi
@ 2020-08-10 7:22 ` Lad, Prabhakar
0 siblings, 0 replies; 11+ messages in thread
From: Lad, Prabhakar @ 2020-08-10 7:22 UTC (permalink / raw
To: Jacopo Mondi
Cc: Lad Prabhakar, Mauro Carvalho Chehab, Rob Herring, Hans Verkuil,
Kieran Bingham, Sakari Ailus, linux-media,
open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS, LKML,
Biju Das, Linux-Renesas
Hi Jacopo,
Thank you for the review.
On Thu, Aug 6, 2020 at 3:32 PM Jacopo Mondi <jacopo@jmondi.org> wrote:
>
> Hi Prabhakar,
>
> On Mon, Aug 03, 2020 at 12:39:11PM +0100, Lad Prabhakar wrote:
> > Document endpoint properties required for parallel interface
> >
> > Signed-off-by: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>
> > ---
> > .../devicetree/bindings/media/i2c/ov772x.txt | 16 ++++++++++++++++
> > 1 file changed, 16 insertions(+)
> >
> > diff --git a/Documentation/devicetree/bindings/media/i2c/ov772x.txt b/Documentation/devicetree/bindings/media/i2c/ov772x.txt
> > index 0b3ede5b8e6a..1f4153484717 100644
> > --- a/Documentation/devicetree/bindings/media/i2c/ov772x.txt
> > +++ b/Documentation/devicetree/bindings/media/i2c/ov772x.txt
> > @@ -21,6 +21,22 @@ subnode for its digital output video port, in accordance with the video
> > interface bindings defined in Documentation/devicetree/bindings/media/
> > video-interfaces.txt.
> >
> > +Endpoint node required properties for parallel connection are:
> > +- remote-endpoint: a phandle to the bus receiver's endpoint node.
>
> we allow endpoints without a remote end connected usually. They can be
> filled in later, in example, with an overlay.
>
Agreed.
> > +- bus-width: shall be set to <8> for 8 bits parallel bus
> > + or <10> for 10 bits parallel bus
> > +- data-shift: shall be set to <2> for 8 bits parallel bus
> > + (lines 9:2 are used) or <0> for 10 bits parallel bus
>
> defining what is required or optional might be hard. I don't see the
> driver enforcing their presence and I assume they have safe default.
> Maybe make them optional and specify what the defaul value is ?
>
Will do.
>
> > +- hsync-active: active state of the HSYNC signal, 0/1 for LOW/HIGH respectively.
> > + (Not required for bus-type equal 6)
> > +- vsync-active: active state of the VSYNC signal, 0/1 for LOW/HIGH respectively.
> > + (Not required for bus-type equal 6)
>
> If they're not required, they're optional, aren't they ? :)
>
Agreed.
> > +- pclk-sample: sample data on rising (1) or falling (0) edge of the pixel clock
> > + signal. (Not required for bus-type equal 6)
>
> Why the pclk polarity is does not apply to BT.656 ?
>
No it should apply.
> > +- bus-type: data bus type. Possible values are:
> > + 5 - Parallel
> > + 6 - Bt.656
>
> Are we making this required, or do we expect this to be deduced
> depending on which other properties have been specified ? Sakari it
> seems you would like this to become a properties that has to be
> specified most of the times, right ? (I tend to agree with that FWIW),
> but does it impact retro-compatibility ?
>
Agreed can be deduced from other properties. But shall wait for Sakari
to comment.
Cheers,
Prabhakar
> > +
> > Example:
> >
> > &i2c0 {
> > --
> > 2.17.1
> >
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH v2 2/3] media: i2c: ov772x: Add support for BT656 mode
2020-08-06 14:45 ` Jacopo Mondi
@ 2020-08-10 7:25 ` Lad, Prabhakar
0 siblings, 0 replies; 11+ messages in thread
From: Lad, Prabhakar @ 2020-08-10 7:25 UTC (permalink / raw
To: Jacopo Mondi
Cc: Lad Prabhakar, Mauro Carvalho Chehab, Rob Herring, Hans Verkuil,
Kieran Bingham, Sakari Ailus, linux-media,
open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS, LKML,
Biju Das, Linux-Renesas
Hi Jacopo,
Thank you for the review.
On Thu, Aug 6, 2020 at 3:41 PM Jacopo Mondi <jacopo@jmondi.org> wrote:
>
> On Mon, Aug 03, 2020 at 12:39:12PM +0100, Lad Prabhakar wrote:
> > Add support to read the bus-type and enable BT656 mode if needed.
> >
> > The driver defaults to parallel mode if bus-type is not specified in DT.
> >
> > Signed-off-by: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>
> > Reviewed-by: Biju Das <biju.das.jz@bp.renesas.com>
> > ---
> > drivers/media/i2c/ov772x.c | 40 ++++++++++++++++++++++++++++++++++++++
> > 1 file changed, 40 insertions(+)
> >
> > diff --git a/drivers/media/i2c/ov772x.c b/drivers/media/i2c/ov772x.c
> > index 2cc6a678069a..2de9248e3689 100644
> > --- a/drivers/media/i2c/ov772x.c
> > +++ b/drivers/media/i2c/ov772x.c
> > @@ -31,6 +31,7 @@
> > #include <media/v4l2-ctrls.h>
> > #include <media/v4l2-device.h>
> > #include <media/v4l2-event.h>
> > +#include <media/v4l2-fwnode.h>
> > #include <media/v4l2-image-sizes.h>
> > #include <media/v4l2-subdev.h>
> >
> > @@ -434,6 +435,7 @@ struct ov772x_priv {
> > #ifdef CONFIG_MEDIA_CONTROLLER
> > struct media_pad pad;
> > #endif
> > + struct v4l2_fwnode_endpoint ep;
> > };
> >
> > /*
> > @@ -574,6 +576,7 @@ static int ov772x_s_stream(struct v4l2_subdev *sd, int enable)
> > {
> > struct i2c_client *client = v4l2_get_subdevdata(sd);
> > struct ov772x_priv *priv = to_ov772x(sd);
> > + unsigned int val;
> > int ret = 0;
> >
> > mutex_lock(&priv->lock);
> > @@ -581,6 +584,22 @@ static int ov772x_s_stream(struct v4l2_subdev *sd, int enable)
> > if (priv->streaming == enable)
> > goto done;
> >
> > + if (priv->ep.bus_type == V4L2_MBUS_BT656 && enable) {
> > + ret = regmap_read(priv->regmap, COM7, &val);
> > + if (ret)
> > + goto done;
> > + val |= ITU656_ON_OFF;
> > + ret = regmap_write(priv->regmap, COM7, val);
> > + } else if (priv->ep.bus_type == V4L2_MBUS_BT656 && !enable) {
>
> is the !enable intentional ? (sorry I don't have access to the sensor
> manual). If not, see below:
>
> > + ret = regmap_read(priv->regmap, COM7, &val);
> > + if (ret)
> > + goto done;
> > + val &= ~ITU656_ON_OFF;
> > + ret = regmap_write(priv->regmap, COM7, val);
> > + }
> > + if (ret)
> > + goto done;
>
> Could you write this as:
>
Agreed will do.
> static int ov772x_s_stream(struct v4l2_subdev *sd, int enable)
> {
> struct i2c_client *client = v4l2_get_subdevdata(sd);
> struct ov772x_priv *priv = to_ov772x(sd);
> int ret = 0;
>
> mutex_lock(&priv->lock);
>
> if (priv->streaming == enable)
> goto done;
>
> if (enable) {
> ret = regmap_read(priv->regmap, COM7, &val);
> if (ret)
> goto done;
>
> if (priv->ep.bus_type == V4L2_MBUS_BT656)
> val |= ITU656_ON_OFF;
> else /* if you accept my suggestion to consider othe
> bus types as errors */
> val &= ~ITU656_ON_OFF;
>
> ret = regmap_write(priv->regmap, COM7, val);
> if (ret)
> goto done;
>
> dev_dbg(&client->dev, "format %d, win %s\n",
> priv->cfmt->code, priv->win->name);
> }
>
> ret = regmap_update_bits(priv->regmap, COM2, SOFT_SLEEP_MODE,
> enable ? 0 : SOFT_SLEEP_MODE);
> if (ret)
> goto done;
> priv->streaming = enable;
>
> done:
> mutex_unlock(&priv->lock);
>
> return ret;
> }
>
>
> > ret = regmap_update_bits(priv->regmap, COM2, SOFT_SLEEP_MODE,
> > enable ? 0 : SOFT_SLEEP_MODE);
> > if (ret)
> > @@ -1354,6 +1373,7 @@ static const struct v4l2_subdev_ops ov772x_subdev_ops = {
> >
> > static int ov772x_probe(struct i2c_client *client)
> > {
> > + struct fwnode_handle *endpoint;
> > struct ov772x_priv *priv;
> > int ret;
> > static const struct regmap_config ov772x_regmap_config = {
> > @@ -1415,6 +1435,26 @@ static int ov772x_probe(struct i2c_client *client)
> > goto error_clk_put;
> > }
> >
> > + endpoint = fwnode_graph_get_next_endpoint(dev_fwnode(&client->dev),
> > + NULL);
> > + if (!endpoint) {
> > + dev_err(&client->dev, "endpoint node not found\n");
> > + ret = -EINVAL;
> > + goto error_clk_put;
> > + }
> > +
> > + ret = v4l2_fwnode_endpoint_parse(endpoint, &priv->ep);
> > + fwnode_handle_put(endpoint);
> > + if (ret) {
> > + dev_err(&client->dev, "Could not parse endpoint\n");
> > + goto error_clk_put;
> > + }
> > +
> > + /* fallback to parallel mode */
> > + if (priv->ep.bus_type != V4L2_MBUS_PARALLEL &&
> > + priv->ep.bus_type != V4L2_MBUS_BT656)
> > + priv->ep.bus_type = V4L2_MBUS_PARALLEL;
>
> shouldn't this be an error ? It's either the bus type has not been
> specified on DT (which is fine, otherwise old DTB without that
> properties will fail) and the bus identification routine implemented
> in v4l2_fwnode_endpoint_parse() detected a bus type which is not
> supported, hence the DT properties are wrong, and this should be an
> error. If you plan to expand the parsing routine to support, say
> bus-width and pclk polarity please break this out to a new function.
>
Agreed.
Cheers,
Prabhakar
> Thanks
> j
>
> > +
> > ret = ov772x_video_probe(priv);
> > if (ret < 0)
> > goto error_gpio_put;
> > --
> > 2.17.1
> >
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH v2 1/3] dt-bindings: media: ov772x: Document endpoint properties
2020-08-03 11:39 ` [PATCH v2 1/3] dt-bindings: media: ov772x: Document endpoint properties Lad Prabhakar
2020-08-06 14:35 ` Jacopo Mondi
@ 2020-08-15 10:34 ` Lad, Prabhakar
2020-08-17 8:11 ` Jacopo Mondi
1 sibling, 1 reply; 11+ messages in thread
From: Lad, Prabhakar @ 2020-08-15 10:34 UTC (permalink / raw
To: Jacopo Mondi
Cc: Mauro Carvalho Chehab, Rob Herring, Hans Verkuil, Kieran Bingham,
Sakari Ailus, linux-media,
open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS, LKML,
Biju Das, Linux-Renesas, Lad Prabhakar
Hi Jacopo,
On Mon, Aug 3, 2020 at 12:39 PM Lad Prabhakar
<prabhakar.mahadev-lad.rj@bp.renesas.com> wrote:
>
> Document endpoint properties required for parallel interface
>
> Signed-off-by: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>
> ---
> .../devicetree/bindings/media/i2c/ov772x.txt | 16 ++++++++++++++++
> 1 file changed, 16 insertions(+)
>
I see you already have a patch for YAML conversion for OV772x binding
[1], if you plan to post a v2 would you be OK to pick these changes as
part of your conversion changes ?
[1] https://www.spinics.net/lists/linux-media/msg173201.html
Cheers,
Prabhakar
> diff --git a/Documentation/devicetree/bindings/media/i2c/ov772x.txt b/Documentation/devicetree/bindings/media/i2c/ov772x.txt
> index 0b3ede5b8e6a..1f4153484717 100644
> --- a/Documentation/devicetree/bindings/media/i2c/ov772x.txt
> +++ b/Documentation/devicetree/bindings/media/i2c/ov772x.txt
> @@ -21,6 +21,22 @@ subnode for its digital output video port, in accordance with the video
> interface bindings defined in Documentation/devicetree/bindings/media/
> video-interfaces.txt.
>
> +Endpoint node required properties for parallel connection are:
> +- remote-endpoint: a phandle to the bus receiver's endpoint node.
> +- bus-width: shall be set to <8> for 8 bits parallel bus
> + or <10> for 10 bits parallel bus
> +- data-shift: shall be set to <2> for 8 bits parallel bus
> + (lines 9:2 are used) or <0> for 10 bits parallel bus
> +- hsync-active: active state of the HSYNC signal, 0/1 for LOW/HIGH respectively.
> + (Not required for bus-type equal 6)
> +- vsync-active: active state of the VSYNC signal, 0/1 for LOW/HIGH respectively.
> + (Not required for bus-type equal 6)
> +- pclk-sample: sample data on rising (1) or falling (0) edge of the pixel clock
> + signal. (Not required for bus-type equal 6)
> +- bus-type: data bus type. Possible values are:
> + 5 - Parallel
> + 6 - Bt.656
> +
> Example:
>
> &i2c0 {
> --
> 2.17.1
>
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH v2 1/3] dt-bindings: media: ov772x: Document endpoint properties
2020-08-15 10:34 ` Lad, Prabhakar
@ 2020-08-17 8:11 ` Jacopo Mondi
2020-08-17 12:01 ` Lad, Prabhakar
0 siblings, 1 reply; 11+ messages in thread
From: Jacopo Mondi @ 2020-08-17 8:11 UTC (permalink / raw
To: Lad, Prabhakar
Cc: Mauro Carvalho Chehab, Rob Herring, Hans Verkuil, Kieran Bingham,
Sakari Ailus, linux-media,
open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS, LKML,
Biju Das, Linux-Renesas, Lad Prabhakar
Hello Prabhakar,
On Sat, Aug 15, 2020 at 11:34:33AM +0100, Lad, Prabhakar wrote:
> Hi Jacopo,
>
> On Mon, Aug 3, 2020 at 12:39 PM Lad Prabhakar
> <prabhakar.mahadev-lad.rj@bp.renesas.com> wrote:
> >
> > Document endpoint properties required for parallel interface
> >
> > Signed-off-by: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>
> > ---
> > .../devicetree/bindings/media/i2c/ov772x.txt | 16 ++++++++++++++++
> > 1 file changed, 16 insertions(+)
> >
> I see you already have a patch for YAML conversion for OV772x binding
> [1], if you plan to post a v2 would you be OK to pick these changes as
> part of your conversion changes ?
Sure thing, I'll add the following properties to the series!
Thanks
j
>
> [1] https://www.spinics.net/lists/linux-media/msg173201.html
>
> Cheers,
> Prabhakar
>
> > diff --git a/Documentation/devicetree/bindings/media/i2c/ov772x.txt b/Documentation/devicetree/bindings/media/i2c/ov772x.txt
> > index 0b3ede5b8e6a..1f4153484717 100644
> > --- a/Documentation/devicetree/bindings/media/i2c/ov772x.txt
> > +++ b/Documentation/devicetree/bindings/media/i2c/ov772x.txt
> > @@ -21,6 +21,22 @@ subnode for its digital output video port, in accordance with the video
> > interface bindings defined in Documentation/devicetree/bindings/media/
> > video-interfaces.txt.
> >
> > +Endpoint node required properties for parallel connection are:
> > +- remote-endpoint: a phandle to the bus receiver's endpoint node.
> > +- bus-width: shall be set to <8> for 8 bits parallel bus
> > + or <10> for 10 bits parallel bus
> > +- data-shift: shall be set to <2> for 8 bits parallel bus
> > + (lines 9:2 are used) or <0> for 10 bits parallel bus
> > +- hsync-active: active state of the HSYNC signal, 0/1 for LOW/HIGH respectively.
> > + (Not required for bus-type equal 6)
> > +- vsync-active: active state of the VSYNC signal, 0/1 for LOW/HIGH respectively.
> > + (Not required for bus-type equal 6)
> > +- pclk-sample: sample data on rising (1) or falling (0) edge of the pixel clock
> > + signal. (Not required for bus-type equal 6)
> > +- bus-type: data bus type. Possible values are:
> > + 5 - Parallel
> > + 6 - Bt.656
> > +
> > Example:
> >
> > &i2c0 {
> > --
> > 2.17.1
> >
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH v2 1/3] dt-bindings: media: ov772x: Document endpoint properties
2020-08-17 8:11 ` Jacopo Mondi
@ 2020-08-17 12:01 ` Lad, Prabhakar
0 siblings, 0 replies; 11+ messages in thread
From: Lad, Prabhakar @ 2020-08-17 12:01 UTC (permalink / raw
To: Jacopo Mondi
Cc: Mauro Carvalho Chehab, Rob Herring, Hans Verkuil, Kieran Bingham,
Sakari Ailus, linux-media,
open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS, LKML,
Biju Das, Linux-Renesas, Lad Prabhakar
On Mon, Aug 17, 2020 at 9:07 AM Jacopo Mondi <jacopo@jmondi.org> wrote:
>
> Hello Prabhakar,
>
> On Sat, Aug 15, 2020 at 11:34:33AM +0100, Lad, Prabhakar wrote:
> > Hi Jacopo,
> >
> > On Mon, Aug 3, 2020 at 12:39 PM Lad Prabhakar
> > <prabhakar.mahadev-lad.rj@bp.renesas.com> wrote:
> > >
> > > Document endpoint properties required for parallel interface
> > >
> > > Signed-off-by: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>
> > > ---
> > > .../devicetree/bindings/media/i2c/ov772x.txt | 16 ++++++++++++++++
> > > 1 file changed, 16 insertions(+)
> > >
> > I see you already have a patch for YAML conversion for OV772x binding
> > [1], if you plan to post a v2 would you be OK to pick these changes as
> > part of your conversion changes ?
>
> Sure thing, I'll add the following properties to the series!
>
Thank you Jacopo. I'll get on with the v3 version of the series.
Cheers,
Prabhakar
> Thanks
> j
>
> >
> > [1] https://www.spinics.net/lists/linux-media/msg173201.html
> >
> > Cheers,
> > Prabhakar
> >
> > > diff --git a/Documentation/devicetree/bindings/media/i2c/ov772x.txt b/Documentation/devicetree/bindings/media/i2c/ov772x.txt
> > > index 0b3ede5b8e6a..1f4153484717 100644
> > > --- a/Documentation/devicetree/bindings/media/i2c/ov772x.txt
> > > +++ b/Documentation/devicetree/bindings/media/i2c/ov772x.txt
> > > @@ -21,6 +21,22 @@ subnode for its digital output video port, in accordance with the video
> > > interface bindings defined in Documentation/devicetree/bindings/media/
> > > video-interfaces.txt.
> > >
> > > +Endpoint node required properties for parallel connection are:
> > > +- remote-endpoint: a phandle to the bus receiver's endpoint node.
> > > +- bus-width: shall be set to <8> for 8 bits parallel bus
> > > + or <10> for 10 bits parallel bus
> > > +- data-shift: shall be set to <2> for 8 bits parallel bus
> > > + (lines 9:2 are used) or <0> for 10 bits parallel bus
> > > +- hsync-active: active state of the HSYNC signal, 0/1 for LOW/HIGH respectively.
> > > + (Not required for bus-type equal 6)
> > > +- vsync-active: active state of the VSYNC signal, 0/1 for LOW/HIGH respectively.
> > > + (Not required for bus-type equal 6)
> > > +- pclk-sample: sample data on rising (1) or falling (0) edge of the pixel clock
> > > + signal. (Not required for bus-type equal 6)
> > > +- bus-type: data bus type. Possible values are:
> > > + 5 - Parallel
> > > + 6 - Bt.656
> > > +
> > > Example:
> > >
> > > &i2c0 {
> > > --
> > > 2.17.1
> > >
^ permalink raw reply [flat|nested] 11+ messages in thread
end of thread, other threads:[~2020-08-17 12:02 UTC | newest]
Thread overview: 11+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2020-08-03 11:39 [PATCH v2 0/3] media: i2c: ov772x: Enable BT656 mode and test pattern support Lad Prabhakar
2020-08-03 11:39 ` [PATCH v2 1/3] dt-bindings: media: ov772x: Document endpoint properties Lad Prabhakar
2020-08-06 14:35 ` Jacopo Mondi
2020-08-10 7:22 ` Lad, Prabhakar
2020-08-15 10:34 ` Lad, Prabhakar
2020-08-17 8:11 ` Jacopo Mondi
2020-08-17 12:01 ` Lad, Prabhakar
2020-08-03 11:39 ` [PATCH v2 2/3] media: i2c: ov772x: Add support for BT656 mode Lad Prabhakar
2020-08-06 14:45 ` Jacopo Mondi
2020-08-10 7:25 ` Lad, Prabhakar
2020-08-03 11:39 ` [PATCH v2 3/3] media: i2c: ov772x: Add test pattern control Lad Prabhakar
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.