* [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
* 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 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 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
* [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
* 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 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
* [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
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 a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).