[RFC 1/3] media: tw9910: Allow to probe from device tree

[RFC 1/3] media: tw9910: Allow to probe from device tree

[Fabio Estevam]

Currently the driver only probes via platform data passed from
board file.

Allow to probe from device tree too.

Signed-off-by: Fabio Estevam <festevam@gmail.com>
---
Hi,

I am currently sending this series as RFC because I was
not able to get the TW9990 to work on a imx6sx board yet.

# media-ctl -p
Media controller API version 5.14.0

Media device information
------------------------
driver          imx7-csi
model           imx-media
serial          
bus info        
hw revision     0x0
driver version  5.14.0

Device topology
- entity 1: csi (2 pads, 2 links)
            type V4L2 subdev subtype Unknown flags 0
            device node name /dev/v4l-subdev0
	pad0: Sink
		[fmt:UYVY8_2X8/800x600 field:none colorspace:srgb xfer:srgb ycbcr:601 quantization:lim-range]
		<- "tw9910 2-0044":0 [ENABLED,IMMUTABLE]
	pad1: Source
		[fmt:UYVY8_2X8/800x600 field:none colorspace:srgb xfer:srgb ycbcr:601 quantization:lim-range]
		-> "csi capture":0 [ENABLED,IMMUTABLE]

- entity 4: csi capture (1 pad, 1 link)
            type Node subtype V4L flags 0
            device node name /dev/video0
	pad0: Sink
		<- "csi":1 [ENABLED,IMMUTABLE]

- entity 10: tw9910 2-0044 (1 pad, 1 link)
             type V4L2 subdev subtype Decoder flags 0
             device node name /dev/v4l-subdev1
	pad0: Source
		[fmt:UYVY8_2X8/640x480 field:interlaced-bt colorspace:smpte170m
		 crop.bounds:(0,0)/640x480
		 crop:(0,0)/640x480]
		-> "csi":0 [ENABLED,IMMUTABLE]

I get the following error when setting up the pipeline:

media-ctl -l "'tw9910 2-0044':0 -> 'csi':0[1]"
media-ctl -l "'csi':1 -> 'csi capture':0[1]"
media-ctl -v -V "'tw9910 2-0044':0 [fmt:UYVY8_2X8/800x480 field:any]"

Opening media device /dev/media0
Enumerating entities
Found 3 entities
Enumerating pads and links
Setting up format UYVY8_2X8 800x480 on pad tw9910 2-0044/0
Unable to set format: No such device or address (-6)
Unable to setup formats: No such device or address (6)

This -6 (ENXIO) error comes from:

tw9910_set_frame() ---> tw9910_mask_set() ---> i2c_smbus_read_byte_data():


static int tw9910_mask_set(struct i2c_client *client, u8 command,
			   u8 mask, u8 set)
{
	s32 val = i2c_smbus_read_byte_data(client, command);

I am able to dump TW9990 registers via i2cdetect and also via the probe
function, so I2C access is OK.

Not sure why I am getting these i2c_smbus_read_byte_data() errors.


Any ideas?

Thanks!


 drivers/media/i2c/tw9910.c | 47 ++++++++++++++++++++++++++++++--------
 1 file changed, 38 insertions(+), 9 deletions(-)

diff --git a/drivers/media/i2c/tw9910.c b/drivers/media/i2c/tw9910.c
index 09f5b3986928..04f3c2dbc1cc 100644
--- a/drivers/media/i2c/tw9910.c
+++ b/drivers/media/i2c/tw9910.c
@@ -22,6 +22,7 @@
 #include <linux/init.h>
 #include <linux/kernel.h>
 #include <linux/module.h>
+#include <linux/of.h>
 #include <linux/slab.h>
 #include <linux/v4l2-mediabus.h>
 #include <linux/videodev2.h>
@@ -928,22 +929,27 @@ static const struct v4l2_subdev_ops tw9910_subdev_ops = {
  * i2c_driver function
  */
 
+static int tw9910_parse_dt(struct i2c_client *client, struct tw9910_priv *priv)
+{
+	priv->info = devm_kzalloc(&client->dev, sizeof(*priv->info), GFP_KERNEL);
+	if (!priv->info)
+		return -ENOMEM;
+
+	/* Use default for now. Will retrieve from dt later */
+	priv->info->mpout = 0;
+	priv->info->buswidth = 8;
+
+	return 0;
+}
+
 static int tw9910_probe(struct i2c_client *client,
 			const struct i2c_device_id *did)
 
 {
 	struct tw9910_priv		*priv;
-	struct tw9910_video_info	*info;
 	struct i2c_adapter		*adapter = client->adapter;
 	int ret;
 
-	if (!client->dev.platform_data) {
-		dev_err(&client->dev, "TW9910: missing platform data!\n");
-		return -EINVAL;
-	}
-
-	info = client->dev.platform_data;
-
 	if (!i2c_check_functionality(adapter, I2C_FUNC_SMBUS_BYTE_DATA)) {
 		dev_err(&client->dev,
 			"I2C-Adapter doesn't support I2C_FUNC_SMBUS_BYTE_DATA\n");
@@ -954,7 +960,18 @@ static int tw9910_probe(struct i2c_client *client,
 	if (!priv)
 		return -ENOMEM;
 
-	priv->info = info;
+	if (IS_ENABLED(CONFIG_OF) && client->dev.of_node) {
+		ret = tw9910_parse_dt(client, priv);
+		if (ret < 0) {
+			v4l_err(client, "DT parsing error\n");
+			return ret;
+		}
+	} else if (client->dev.platform_data) {
+		priv->info = client->dev.platform_data;
+	} else {
+		v4l_err(client, "No platform data!\n");
+		return -ENODEV;
+	}
 
 	v4l2_i2c_subdev_init(&priv->subdev, client, &tw9910_subdev_ops);
 
@@ -1007,13 +1024,25 @@ static int tw9910_remove(struct i2c_client *client)
 
 static const struct i2c_device_id tw9910_id[] = {
 	{ "tw9910", 0 },
+	{ "tw9990", 0 },
 	{ }
 };
 MODULE_DEVICE_TABLE(i2c, tw9910_id);
 
+#ifdef CONFIG_OF
+static const struct of_device_id tw9910_of_id[] = {
+	{ .compatible = "renesas,tw9910", },
+	{ .compatible = "renesas,tw9990", },
+	{ },
+};
+
+MODULE_DEVICE_TABLE(of, tw9910_of_id);
+#endif
+
 static struct i2c_driver tw9910_i2c_driver = {
 	.driver = {
 		.name = "tw9910",
+		.of_match_table = of_match_ptr(tw9910_of_id),
 	},
 	.probe    = tw9910_probe,
 	.remove   = tw9910_remove,
-- 
2.25.1

[RFC 2/3] media: tw9910: Allow the TW9990 to probe

[Fabio Estevam]

Currently the driver rejects to probe when the ID is
different from the TW9910 one.

Allow TW9990 to probe by allowing its ID too.

Signed-off-by: Fabio Estevam <festevam@gmail.com>
---
 drivers/media/i2c/tw9910.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/media/i2c/tw9910.c b/drivers/media/i2c/tw9910.c
index 04f3c2dbc1cc..0411b8ea9bda 100644
--- a/drivers/media/i2c/tw9910.c
+++ b/drivers/media/i2c/tw9910.c
@@ -859,7 +859,7 @@ static int tw9910_video_probe(struct i2c_client *client)
 	priv->revision = GET_REV(id);
 	id = GET_ID(id);
 
-	if (id != 0x0b || priv->revision > 0x01) {
+	if ((id != 0x0b && id != 0x00) || priv->revision > 0x01) {
 		dev_err(&client->dev, "Product ID error %x:%x\n",
 			id, priv->revision);
 		ret = -ENODEV;
-- 
2.25.1

Re: [RFC 2/3] media: tw9910: Allow the TW9990 to probe

[Jacopo Mondi]

Hi Fabio,

On Fri, Aug 27, 2021 at 10:01:49AM -0300, Fabio Estevam wrote:
> Currently the driver rejects to probe when the ID is
> different from the TW9910 one.
>
> Allow TW9990 to probe by allowing its ID too.
>
> Signed-off-by: Fabio Estevam <festevam@gmail.com>
> ---
>  drivers/media/i2c/tw9910.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/media/i2c/tw9910.c b/drivers/media/i2c/tw9910.c
> index 04f3c2dbc1cc..0411b8ea9bda 100644
> --- a/drivers/media/i2c/tw9910.c
> +++ b/drivers/media/i2c/tw9910.c
> @@ -859,7 +859,7 @@ static int tw9910_video_probe(struct i2c_client *client)
>  	priv->revision = GET_REV(id);
>  	id = GET_ID(id);
>
> -	if (id != 0x0b || priv->revision > 0x01) {
> +	if ((id != 0x0b && id != 0x00) || priv->revision > 0x01) {

I would define these. Up to you!

>  		dev_err(&client->dev, "Product ID error %x:%x\n",
>  			id, priv->revision);
>  		ret = -ENODEV;
> --
> 2.25.1
>

[RFC 3/3] media: tw9910: Initialize the entity type

[Fabio Estevam]

Currently the following warning is seen:

imx7-csi 2214000.csi: Entity type for entity tw9910 2-0044 was not initialized!

Initialize the entity type accordingly.

Signed-off-by: Fabio Estevam <festevam@gmail.com>
---
 drivers/media/i2c/tw9910.c | 18 +++++++++++++++---
 1 file changed, 15 insertions(+), 3 deletions(-)

diff --git a/drivers/media/i2c/tw9910.c b/drivers/media/i2c/tw9910.c
index 0411b8ea9bda..ad4149073ba4 100644
--- a/drivers/media/i2c/tw9910.c
+++ b/drivers/media/i2c/tw9910.c
@@ -228,6 +228,7 @@ struct tw9910_scale_ctrl {
 
 struct tw9910_priv {
 	struct v4l2_subdev		subdev;
+	struct media_pad		pad;
 	struct clk			*clk;
 	struct tw9910_video_info	*info;
 	struct gpio_desc		*pdn_gpio;
@@ -948,6 +949,7 @@ static int tw9910_probe(struct i2c_client *client,
 {
 	struct tw9910_priv		*priv;
 	struct i2c_adapter		*adapter = client->adapter;
+	struct v4l2_subdev *sd;
 	int ret;
 
 	if (!i2c_check_functionality(adapter, I2C_FUNC_SMBUS_BYTE_DATA)) {
@@ -973,14 +975,22 @@ static int tw9910_probe(struct i2c_client *client,
 		return -ENODEV;
 	}
 
-	v4l2_i2c_subdev_init(&priv->subdev, client, &tw9910_subdev_ops);
+	sd = &priv->subdev;
+	v4l2_i2c_subdev_init(sd, client, &tw9910_subdev_ops);
+	priv->pad.flags = MEDIA_PAD_FL_SOURCE;
+	sd->flags |= V4L2_SUBDEV_FL_HAS_DEVNODE | V4L2_SUBDEV_FL_HAS_EVENTS;
+	sd->entity.function = MEDIA_ENT_F_ATV_DECODER;
+	ret = media_entity_pads_init(&sd->entity, 1, &priv->pad);
+	if (ret)
+		return ret;
 
 	priv->clk = clk_get(&client->dev, "xti");
 	if (PTR_ERR(priv->clk) == -ENOENT) {
 		priv->clk = NULL;
 	} else if (IS_ERR(priv->clk)) {
 		dev_err(&client->dev, "Unable to get xti clock\n");
-		return PTR_ERR(priv->clk);
+		ret = PTR_ERR(priv->clk);
+		goto media_entity_cleanup;
 	}
 
 	priv->pdn_gpio = gpiod_get_optional(&client->dev, "pdn",
@@ -1006,7 +1016,9 @@ static int tw9910_probe(struct i2c_client *client,
 		gpiod_put(priv->pdn_gpio);
 error_clk_put:
 	clk_put(priv->clk);
-
+	
+media_entity_cleanup:
+	media_entity_cleanup(&sd->entity);
 	return ret;
 }
 
-- 
2.25.1

Re: [RFC 3/3] media: tw9910: Initialize the entity type

[Jacopo Mondi]

Hi Fabio,

On Fri, Aug 27, 2021 at 10:01:50AM -0300, Fabio Estevam wrote:
> Currently the following warning is seen:
>
> imx7-csi 2214000.csi: Entity type for entity tw9910 2-0044 was not initialized!
>
> Initialize the entity type accordingly.
>
> Signed-off-by: Fabio Estevam <festevam@gmail.com>
> ---
>  drivers/media/i2c/tw9910.c | 18 +++++++++++++++---
>  1 file changed, 15 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/media/i2c/tw9910.c b/drivers/media/i2c/tw9910.c
> index 0411b8ea9bda..ad4149073ba4 100644
> --- a/drivers/media/i2c/tw9910.c
> +++ b/drivers/media/i2c/tw9910.c
> @@ -228,6 +228,7 @@ struct tw9910_scale_ctrl {
>
>  struct tw9910_priv {
>  	struct v4l2_subdev		subdev;
> +	struct media_pad		pad;
>  	struct clk			*clk;
>  	struct tw9910_video_info	*info;
>  	struct gpio_desc		*pdn_gpio;
> @@ -948,6 +949,7 @@ static int tw9910_probe(struct i2c_client *client,
>  {
>  	struct tw9910_priv		*priv;
>  	struct i2c_adapter		*adapter = client->adapter;
> +	struct v4l2_subdev *sd;

As questionable as it could be, the existing variables are aligned
differently.

>  	int ret;
>
>  	if (!i2c_check_functionality(adapter, I2C_FUNC_SMBUS_BYTE_DATA)) {
> @@ -973,14 +975,22 @@ static int tw9910_probe(struct i2c_client *client,
>  		return -ENODEV;
>  	}
>
> -	v4l2_i2c_subdev_init(&priv->subdev, client, &tw9910_subdev_ops);
> +	sd = &priv->subdev;
> +	v4l2_i2c_subdev_init(sd, client, &tw9910_subdev_ops);
> +	priv->pad.flags = MEDIA_PAD_FL_SOURCE;
> +	sd->flags |= V4L2_SUBDEV_FL_HAS_DEVNODE | V4L2_SUBDEV_FL_HAS_EVENTS;
> +	sd->entity.function = MEDIA_ENT_F_ATV_DECODER;

Seems appropriate!

> +	ret = media_entity_pads_init(&sd->entity, 1, &priv->pad);
> +	if (ret)
> +		return ret;
>
>  	priv->clk = clk_get(&client->dev, "xti");
>  	if (PTR_ERR(priv->clk) == -ENOENT) {
>  		priv->clk = NULL;
>  	} else if (IS_ERR(priv->clk)) {
>  		dev_err(&client->dev, "Unable to get xti clock\n");
> -		return PTR_ERR(priv->clk);
> +		ret = PTR_ERR(priv->clk);
> +		goto media_entity_cleanup;
>  	}
>
>  	priv->pdn_gpio = gpiod_get_optional(&client->dev, "pdn",
> @@ -1006,7 +1016,9 @@ static int tw9910_probe(struct i2c_client *client,
>  		gpiod_put(priv->pdn_gpio);
>  error_clk_put:
>  	clk_put(priv->clk);
> -
> +

Rougue tab, or is it my mail client ?

> +media_entity_cleanup:
> +	media_entity_cleanup(&sd->entity);
>  	return ret;
>  }
>
> --
> 2.25.1
>

Re: [RFC 1/3] media: tw9910: Allow to probe from device tree

[Fabio Estevam]

On Fri, Aug 27, 2021 at 10:02 AM Fabio Estevam <festevam@gmail.com> wrote:
>
> Currently the driver only probes via platform data passed from
> board file.
>
> Allow to probe from device tree too.
>
> Signed-off-by: Fabio Estevam <festevam@gmail.com>
> ---
> Hi,
>
> I am currently sending this series as RFC because I was
> not able to get the TW9990 to work on a imx6sx board yet.
>
> # media-ctl -p
> Media controller API version 5.14.0
>
> Media device information
> ------------------------
> driver          imx7-csi
> model           imx-media
> serial
> bus info
> hw revision     0x0
> driver version  5.14.0
>
> Device topology
> - entity 1: csi (2 pads, 2 links)
>             type V4L2 subdev subtype Unknown flags 0
>             device node name /dev/v4l-subdev0
>         pad0: Sink
>                 [fmt:UYVY8_2X8/800x600 field:none colorspace:srgb xfer:srgb ycbcr:601 quantization:lim-range]
>                 <- "tw9910 2-0044":0 [ENABLED,IMMUTABLE]
>         pad1: Source
>                 [fmt:UYVY8_2X8/800x600 field:none colorspace:srgb xfer:srgb ycbcr:601 quantization:lim-range]
>                 -> "csi capture":0 [ENABLED,IMMUTABLE]
>
> - entity 4: csi capture (1 pad, 1 link)
>             type Node subtype V4L flags 0
>             device node name /dev/video0
>         pad0: Sink
>                 <- "csi":1 [ENABLED,IMMUTABLE]
>
> - entity 10: tw9910 2-0044 (1 pad, 1 link)
>              type V4L2 subdev subtype Decoder flags 0
>              device node name /dev/v4l-subdev1
>         pad0: Source
>                 [fmt:UYVY8_2X8/640x480 field:interlaced-bt colorspace:smpte170m
>                  crop.bounds:(0,0)/640x480
>                  crop:(0,0)/640x480]
>                 -> "csi":0 [ENABLED,IMMUTABLE]
>
> I get the following error when setting up the pipeline:
>
> media-ctl -l "'tw9910 2-0044':0 -> 'csi':0[1]"
> media-ctl -l "'csi':1 -> 'csi capture':0[1]"
> media-ctl -v -V "'tw9910 2-0044':0 [fmt:UYVY8_2X8/800x480 field:any]"
>
> Opening media device /dev/media0
> Enumerating entities
> Found 3 entities
> Enumerating pads and links
> Setting up format UYVY8_2X8 800x480 on pad tw9910 2-0044/0
> Unable to set format: No such device or address (-6)
> Unable to setup formats: No such device or address (6)
>
> This -6 (ENXIO) error comes from:
>
> tw9910_set_frame() ---> tw9910_mask_set() ---> i2c_smbus_read_byte_data():
>
>
> static int tw9910_mask_set(struct i2c_client *client, u8 command,
>                            u8 mask, u8 set)
> {
>         s32 val = i2c_smbus_read_byte_data(client, command);
>
> I am able to dump TW9990 registers via i2cdetect and also via the probe
> function, so I2C access is OK.
>
> Not sure why I am getting these i2c_smbus_read_byte_data() errors.

I found the reason: the tw9910 driver was getting unpowered in the
incorrect place.
This problem was present prior to the DT conversion.

I will send a fix for it soon.

Thanks

Re: [RFC 1/3] media: tw9910: Allow to probe from device tree

[Jacopo Mondi]

Hi Fabio,

On Fri, Aug 27, 2021 at 10:01:48AM -0300, Fabio Estevam wrote:
> Currently the driver only probes via platform data passed from
> board file.
>
> Allow to probe from device tree too.
>
> Signed-off-by: Fabio Estevam <festevam@gmail.com>
> ---
> Hi,
>
> I am currently sending this series as RFC because I was
> not able to get the TW9990 to work on a imx6sx board yet.

I'm sorry but I've no access to any TW99xx device at the moment to
test with

>
> # media-ctl -p
> Media controller API version 5.14.0
>
> Media device information
> ------------------------
> driver          imx7-csi
> model           imx-media
> serial
> bus info
> hw revision     0x0
> driver version  5.14.0
>
> Device topology
> - entity 1: csi (2 pads, 2 links)
>             type V4L2 subdev subtype Unknown flags 0
>             device node name /dev/v4l-subdev0
> 	pad0: Sink
> 		[fmt:UYVY8_2X8/800x600 field:none colorspace:srgb xfer:srgb ycbcr:601 quantization:lim-range]
> 		<- "tw9910 2-0044":0 [ENABLED,IMMUTABLE]
> 	pad1: Source
> 		[fmt:UYVY8_2X8/800x600 field:none colorspace:srgb xfer:srgb ycbcr:601 quantization:lim-range]
> 		-> "csi capture":0 [ENABLED,IMMUTABLE]
>
> - entity 4: csi capture (1 pad, 1 link)
>             type Node subtype V4L flags 0
>             device node name /dev/video0
> 	pad0: Sink
> 		<- "csi":1 [ENABLED,IMMUTABLE]
>
> - entity 10: tw9910 2-0044 (1 pad, 1 link)
>              type V4L2 subdev subtype Decoder flags 0
>              device node name /dev/v4l-subdev1
> 	pad0: Source
> 		[fmt:UYVY8_2X8/640x480 field:interlaced-bt colorspace:smpte170m
> 		 crop.bounds:(0,0)/640x480
> 		 crop:(0,0)/640x480]
> 		-> "csi":0 [ENABLED,IMMUTABLE]
>
> I get the following error when setting up the pipeline:
>
> media-ctl -l "'tw9910 2-0044':0 -> 'csi':0[1]"
> media-ctl -l "'csi':1 -> 'csi capture':0[1]"
> media-ctl -v -V "'tw9910 2-0044':0 [fmt:UYVY8_2X8/800x480 field:any]"
>
> Opening media device /dev/media0
> Enumerating entities
> Found 3 entities
> Enumerating pads and links
> Setting up format UYVY8_2X8 800x480 on pad tw9910 2-0044/0
> Unable to set format: No such device or address (-6)
> Unable to setup formats: No such device or address (6)
>
> This -6 (ENXIO) error comes from:
>
> tw9910_set_frame() ---> tw9910_mask_set() ---> i2c_smbus_read_byte_data():
>
>
> static int tw9910_mask_set(struct i2c_client *client, u8 command,
> 			   u8 mask, u8 set)
> {
> 	s32 val = i2c_smbus_read_byte_data(client, command);
>
> I am able to dump TW9990 registers via i2cdetect and also via the probe
> function, so I2C access is OK.
>
> Not sure why I am getting these i2c_smbus_read_byte_data() errors.

There's an

	tw9910_s_power(&priv->subdev, 0);

at the end of the video_probe() function.

The driver handles power management with the legacy s_power() call
chain, and the receiver driver needs to v4l2_pipeline_pm_get() which
the imx driver does when the capture node is open.

Just an hint, you might have noticed already

>
>
> Any ideas?
>
> Thanks!
>
>
>  drivers/media/i2c/tw9910.c | 47 ++++++++++++++++++++++++++++++--------
>  1 file changed, 38 insertions(+), 9 deletions(-)
>
> diff --git a/drivers/media/i2c/tw9910.c b/drivers/media/i2c/tw9910.c
> index 09f5b3986928..04f3c2dbc1cc 100644
> --- a/drivers/media/i2c/tw9910.c
> +++ b/drivers/media/i2c/tw9910.c
> @@ -22,6 +22,7 @@
>  #include <linux/init.h>
>  #include <linux/kernel.h>
>  #include <linux/module.h>
> +#include <linux/of.h>

If you need this for struct of_device_id then you should include
<linux/mod_devicetable.h>

>  #include <linux/slab.h>
>  #include <linux/v4l2-mediabus.h>
>  #include <linux/videodev2.h>
> @@ -928,22 +929,27 @@ static const struct v4l2_subdev_ops tw9910_subdev_ops = {
>   * i2c_driver function
>   */
>
> +static int tw9910_parse_dt(struct i2c_client *client, struct tw9910_priv *priv)
> +{
> +	priv->info = devm_kzalloc(&client->dev, sizeof(*priv->info), GFP_KERNEL);
> +	if (!priv->info)

IS_ERR_OR_NULL() ?

> +		return -ENOMEM;

PTR_ERR() ?

> +
> +	/* Use default for now. Will retrieve from dt later */
> +	priv->info->mpout = 0;
> +	priv->info->buswidth = 8;
> +
> +	return 0;
> +}
> +
>  static int tw9910_probe(struct i2c_client *client,
>  			const struct i2c_device_id *did)
>
>  {
>  	struct tw9910_priv		*priv;
> -	struct tw9910_video_info	*info;
>  	struct i2c_adapter		*adapter = client->adapter;
>  	int ret;
>
> -	if (!client->dev.platform_data) {
> -		dev_err(&client->dev, "TW9910: missing platform data!\n");
> -		return -EINVAL;
> -	}
> -
> -	info = client->dev.platform_data;
> -
>  	if (!i2c_check_functionality(adapter, I2C_FUNC_SMBUS_BYTE_DATA)) {
>  		dev_err(&client->dev,
>  			"I2C-Adapter doesn't support I2C_FUNC_SMBUS_BYTE_DATA\n");
> @@ -954,7 +960,18 @@ static int tw9910_probe(struct i2c_client *client,
>  	if (!priv)
>  		return -ENOMEM;
>
> -	priv->info = info;
> +	if (IS_ENABLED(CONFIG_OF) && client->dev.of_node) {
> +		ret = tw9910_parse_dt(client, priv);
> +		if (ret < 0) {
> +			v4l_err(client, "DT parsing error\n");
> +			return ret;
> +		}
> +	} else if (client->dev.platform_data) {
> +		priv->info = client->dev.platform_data;
> +	} else {
> +		v4l_err(client, "No platform data!\n");
> +		return -ENODEV;
> +	}
>
>  	v4l2_i2c_subdev_init(&priv->subdev, client, &tw9910_subdev_ops);
>
> @@ -1007,13 +1024,25 @@ static int tw9910_remove(struct i2c_client *client)
>
>  static const struct i2c_device_id tw9910_id[] = {
>  	{ "tw9910", 0 },
> +	{ "tw9990", 0 },

Unrelated to this patch maybe ?

>  	{ }
>  };
>  MODULE_DEVICE_TABLE(i2c, tw9910_id);
>
> +#ifdef CONFIG_OF

I think this can be dropped ?

> +static const struct of_device_id tw9910_of_id[] = {
> +	{ .compatible = "renesas,tw9910", },
> +	{ .compatible = "renesas,tw9990", },


TW9910 was originally manufactured by Techwell, then bought by
Intersil, then bought by Renesas. Not sure what I would use as the
chip is still marketed as "Techwell" and the public documentation
released by "Intersil", according to my web searches..


> +	{ },
> +};
> +
> +MODULE_DEVICE_TABLE(of, tw9910_of_id);
> +#endif
> +
>  static struct i2c_driver tw9910_i2c_driver = {
>  	.driver = {
>  		.name = "tw9910",
> +		.of_match_table = of_match_ptr(tw9910_of_id),
>  	},
>  	.probe    = tw9910_probe,
>  	.remove   = tw9910_remove,
> --
> 2.25.1
>

Re: [RFC 1/3] media: tw9910: Allow to probe from device tree

[Fabio Estevam]

Hi Jacopo,

On Mon, Sep 13, 2021 at 5:58 AM Jacopo Mondi <jacopo@jmondi.org> wrote:

> There's an
>
>         tw9910_s_power(&priv->subdev, 0);
>
> at the end of the video_probe() function.
>
> The driver handles power management with the legacy s_power() call
> chain, and the receiver driver needs to v4l2_pipeline_pm_get() which
> the imx driver does when the capture node is open.
>
> Just an hint, you might have noticed already

Thanks for your comments and review. Yes, I have fixed the I2C errors.

I plan to re-submit the entire series after I get the TW9990 to work
on my imx6sx-based board.

The capture driver is drivers/staging/media/imx/imx7-media-csi.c.

Currently, I am not able to get the stream to start.

This is the configuration I am using:

media-ctl -l "'tw9910 2-0044':0 -> 'csi':0[1]"
media-ctl -l "'csi':1 -> 'csi capture':0[1]"
media-ctl -v -V "'tw9910 2-0044':0 [fmt:UYVY8_2X8/720x480 field:any]"
Opening media device /dev/media0
Enumerating entities
Found 3 entities
Enumerating pads and links
Setting up format UYVY8_2X8 720x480 on pad tw9910 2-0044/0
Format set: UYVY8_2X8 720x480
Setting up format UYVY8_2X8 720x480 on pad csi/0
Format set: UYVY8_2X8 720x480

Then I launch the capture stream command:

v4l2-ctl --stream-mmap -d /dev/video1

but nothing happens here, no >>>>> frame indication progress is shown.

If I hit CTRL + C then I get:
[  715.467623] csi: wait last EOF timeout

Any suggestions?

Thanks

Re: [RFC 1/3] media: tw9910: Allow to probe from device tree

[Fabio Estevam]

On Mon, Sep 13, 2021 at 9:53 AM Fabio Estevam <festevam@gmail.com> wrote:
>
> Hi Jacopo,
>
> On Mon, Sep 13, 2021 at 5:58 AM Jacopo Mondi <jacopo@jmondi.org> wrote:
>
> > There's an
> >
> >         tw9910_s_power(&priv->subdev, 0);
> >
> > at the end of the video_probe() function.
> >
> > The driver handles power management with the legacy s_power() call
> > chain, and the receiver driver needs to v4l2_pipeline_pm_get() which
> > the imx driver does when the capture node is open.
> >
> > Just an hint, you might have noticed already
>
> Thanks for your comments and review. Yes, I have fixed the I2C errors.
>
> I plan to re-submit the entire series after I get the TW9990 to work
> on my imx6sx-based board.
>
> The capture driver is drivers/staging/media/imx/imx7-media-csi.c.
>
> Currently, I am not able to get the stream to start.
>
> This is the configuration I am using:
>
> media-ctl -l "'tw9910 2-0044':0 -> 'csi':0[1]"
> media-ctl -l "'csi':1 -> 'csi capture':0[1]"
> media-ctl -v -V "'tw9910 2-0044':0 [fmt:UYVY8_2X8/720x480 field:any]"
> Opening media device /dev/media0
> Enumerating entities
> Found 3 entities
> Enumerating pads and links
> Setting up format UYVY8_2X8 720x480 on pad tw9910 2-0044/0
> Format set: UYVY8_2X8 720x480
> Setting up format UYVY8_2X8 720x480 on pad csi/0
> Format set: UYVY8_2X8 720x480
>
> Then I launch the capture stream command:
>
> v4l2-ctl --stream-mmap -d /dev/video1
>
> but nothing happens here, no >>>>> frame indication progress is shown.
>
> If I hit CTRL + C then I get:
> [  715.467623] csi: wait last EOF timeout

I also need to set the video standard to PAL:

# v4l2-ctl --device /dev/v4l-subdev1 --set-standard PAL
Standard set to 000000ff

# media-ctl -l "'tw9910 2-0044':0 -> 'csi':0[1]"
# media-ctl -l "'csi':1 -> 'csi capture':0[1]"
# media-ctl -v -V "'tw9910 2-0044':0 [fmt:UYVY8_2X8/720x576
field:interlaced-bt]"
Opening media device /dev/media0
Enumerating entities
Found 3 entities
Enumerating pads and links
Setting up format UYVY8_2X8 720x576 on pad tw9910 2-0044/0
Format set: UYVY8_2X8 720x576
Setting up format UYVY8_2X8 720x576 on pad csi/0
Format set: UYVY8_2X8 720x576

# v4l2-ctl --stream-mmap -d /dev/video1
[   38.554032] priv->vdev.fmt.fmt.pix.width is 800
[   38.561444] f.fmt.pix.width is 720
[   38.567607] priv->vdev.fmt.fmt.pix.height is 600
[   38.574973] f.fmt.pix.height is 576
[   38.580992] priv->vdev.cc->cs is 1
[   38.586958] cc->cs is 1
[   38.591761] priv->vdev.compose.width is 800
[   38.598420] compose.width is 720
[   38.604048] priv->vdev.compose.height is 600
[   38.610681] compose.height is 576
[   38.616347] csi: capture format not valid
VIDIOC_STREAMON returned -1 (Broken pipe)

I added some printk debug lines to show the mismatch that prevents
the pipeline to start in capture_validate_fmt():

diff --git a/drivers/staging/media/imx/imx-media-capture.c
b/drivers/staging/media/imx/imx-media-capture.c
index b2f2cb3d6a60..511625981e93 100644
--- a/drivers/staging/media/imx/imx-media-capture.c
+++ b/drivers/staging/media/imx/imx-media-capture.c
@@ -552,6 +552,21 @@ static int capture_validate_fmt(struct capture_priv *priv)
        ret = __capture_try_fmt_vid_cap(priv, &fmt_src, &f, &cc, &compose);
        if (ret)
                return ret;
+
+       pr_err("priv->vdev.fmt.fmt.pix.width is %d\n",
priv->vdev.fmt.fmt.pix.width);
+       pr_err("f.fmt.pix.width is %d\n", f.fmt.pix.width);
+
+       pr_err("priv->vdev.fmt.fmt.pix.height is %d\n",
priv->vdev.fmt.fmt.pix.height);
+       pr_err("f.fmt.pix.height is %d\n", f.fmt.pix.height );
+
+       pr_err("priv->vdev.cc->cs is %d\n", priv->vdev.cc->cs);
+       pr_err("cc->cs is %d\n", cc->cs);
+
+       pr_err("priv->vdev.compose.width is %d\n", priv->vdev.compose.width);
+       pr_err("compose.width is %d\n", compose.width);
+
+       pr_err("priv->vdev.compose.height is %d\n", priv->vdev.compose.height);
+       pr_err("compose.height is %d\n", compose.height);

        return (priv->vdev.fmt.fmt.pix.width != f.fmt.pix.width ||
                priv->vdev.fmt.fmt.pix.height != f.fmt.pix.height ||

Any ideas as to whu such mismatch happens?

Thanks

Re: [RFC 1/3] media: tw9910: Allow to probe from device tree

[Rui Miguel Silva]

Hi Fabio,
On Mon Sep 13, 2021 at 9:03 PM WEST, Fabio Estevam wrote:

> On Mon, Sep 13, 2021 at 9:53 AM Fabio Estevam <festevam@gmail.com> wrote:
> >
> > Hi Jacopo,
> >
> > On Mon, Sep 13, 2021 at 5:58 AM Jacopo Mondi <jacopo@jmondi.org> wrote:
> >
> > > There's an
> > >
> > >         tw9910_s_power(&priv->subdev, 0);
> > >
> > > at the end of the video_probe() function.
> > >
> > > The driver handles power management with the legacy s_power() call
> > > chain, and the receiver driver needs to v4l2_pipeline_pm_get() which
> > > the imx driver does when the capture node is open.
> > >
> > > Just an hint, you might have noticed already
> >
> > Thanks for your comments and review. Yes, I have fixed the I2C errors.
> >
> > I plan to re-submit the entire series after I get the TW9990 to work
> > on my imx6sx-based board.
> >
> > The capture driver is drivers/staging/media/imx/imx7-media-csi.c.
> >
> > Currently, I am not able to get the stream to start.
> >
> > This is the configuration I am using:
> >
> > media-ctl -l "'tw9910 2-0044':0 -> 'csi':0[1]"
> > media-ctl -l "'csi':1 -> 'csi capture':0[1]"
> > media-ctl -v -V "'tw9910 2-0044':0 [fmt:UYVY8_2X8/720x480 field:any]"
> > Opening media device /dev/media0
> > Enumerating entities
> > Found 3 entities
> > Enumerating pads and links
> > Setting up format UYVY8_2X8 720x480 on pad tw9910 2-0044/0
> > Format set: UYVY8_2X8 720x480
> > Setting up format UYVY8_2X8 720x480 on pad csi/0
> > Format set: UYVY8_2X8 720x480
> >
> > Then I launch the capture stream command:
> >
> > v4l2-ctl --stream-mmap -d /dev/video1
> >
> > but nothing happens here, no >>>>> frame indication progress is shown.
> >
> > If I hit CTRL + C then I get:
> > [  715.467623] csi: wait last EOF timeout
>
> I also need to set the video standard to PAL:
>
> # v4l2-ctl --device /dev/v4l-subdev1 --set-standard PAL
> Standard set to 000000ff
>
> # media-ctl -l "'tw9910 2-0044':0 -> 'csi':0[1]"
> # media-ctl -l "'csi':1 -> 'csi capture':0[1]"
> # media-ctl -v -V "'tw9910 2-0044':0 [fmt:UYVY8_2X8/720x576
> field:interlaced-bt]"
> Opening media device /dev/media0
> Enumerating entities
> Found 3 entities
> Enumerating pads and links
> Setting up format UYVY8_2X8 720x576 on pad tw9910 2-0044/0
> Format set: UYVY8_2X8 720x576
> Setting up format UYVY8_2X8 720x576 on pad csi/0
> Format set: UYVY8_2X8 720x576
>
> # v4l2-ctl --stream-mmap -d /dev/video1
> [   38.554032] priv->vdev.fmt.fmt.pix.width is 800
> [   38.561444] f.fmt.pix.width is 720
> [   38.567607] priv->vdev.fmt.fmt.pix.height is 600
> [   38.574973] f.fmt.pix.height is 576
> [   38.580992] priv->vdev.cc->cs is 1
> [   38.586958] cc->cs is 1
> [   38.591761] priv->vdev.compose.width is 800
> [   38.598420] compose.width is 720
> [   38.604048] priv->vdev.compose.height is 600
> [   38.610681] compose.height is 576
> [   38.616347] csi: capture format not valid
> VIDIOC_STREAMON returned -1 (Broken pipe)
>
> I added some printk debug lines to show the mismatch that prevents
> the pipeline to start in capture_validate_fmt():
>
> diff --git a/drivers/staging/media/imx/imx-media-capture.c
> b/drivers/staging/media/imx/imx-media-capture.c
> index b2f2cb3d6a60..511625981e93 100644
> --- a/drivers/staging/media/imx/imx-media-capture.c
> +++ b/drivers/staging/media/imx/imx-media-capture.c
> @@ -552,6 +552,21 @@ static int capture_validate_fmt(struct capture_priv *priv)
>         ret = __capture_try_fmt_vid_cap(priv, &fmt_src, &f, &cc, &compose);
>         if (ret)
>                 return ret;
> +
> +       pr_err("priv->vdev.fmt.fmt.pix.width is %d\n",
> priv->vdev.fmt.fmt.pix.width);
> +       pr_err("f.fmt.pix.width is %d\n", f.fmt.pix.width);
> +
> +       pr_err("priv->vdev.fmt.fmt.pix.height is %d\n",
> priv->vdev.fmt.fmt.pix.height);
> +       pr_err("f.fmt.pix.height is %d\n", f.fmt.pix.height );
> +
> +       pr_err("priv->vdev.cc->cs is %d\n", priv->vdev.cc->cs);
> +       pr_err("cc->cs is %d\n", cc->cs);
> +
> +       pr_err("priv->vdev.compose.width is %d\n", priv->vdev.compose.width);
> +       pr_err("compose.width is %d\n", compose.width);
> +
> +       pr_err("priv->vdev.compose.height is %d\n", priv->vdev.compose.height);
> +       pr_err("compose.height is %d\n", compose.height);
>
>         return (priv->vdev.fmt.fmt.pix.width != f.fmt.pix.width ||
>                 priv->vdev.fmt.fmt.pix.height != f.fmt.pix.height ||
>
> Any ideas as to whu such mismatch happens?

Looking  at this code it looks like you are using an old tree, Laurent
meanwhile have sent a patch that change this function:

a9512b261afd ("media: imx: capture: Simplify capture_validate_fmt() implementation")

can you test with the latest code as see if you have the same
problems.

------
Cheers,
     Rui

Re: [RFC 1/3] media: tw9910: Allow to probe from device tree

[Fabio Estevam]

Hi Rui,

On Mon, Sep 13, 2021 at 7:12 PM Rui Miguel Silva <rui.silva@linaro.org> wrote:

> Looking  at this code it looks like you are using an old tree, Laurent
> meanwhile have sent a patch that change this function:
>
> a9512b261afd ("media: imx: capture: Simplify capture_validate_fmt() implementation")
>
> can you test with the latest code as see if you have the same
> problems.

I have just tested on 5.15-rc1 and this is what I get:

# v4l2-ctl --device /dev/v4l-subdev1 --set-standard PAL
Standard set to 000000ff
# media-ctl -l "'tw9910 2-0044':0 -> 'csi':0[1]"
# media-ctl -l "'csi':1 -> 'csi capture':0[1]"
# media-ctl -v -V "'tw9910 2-0044':0 [fmt:UYVY8_2X8/720x576
field:interlaced-bt]"
Opening media device /dev/media0
Enumerating entities
Found 3 entities
Enumerating pads and links
Setting up format UYVY8_2X8 720x576 on pad tw9910 2-0044/0
Format set: UYVY8_2X8 720x576
Setting up format UYVY8_2X8 720x576 on pad csi/0
Format set: UYVY8_2X8 720x576

# v4l2-ctl --stream-mmap -d /dev/video1
[   26.860654] priv->vdev.compose.width is 640
[   26.865419] fmt_src.format.width is 720
[   26.869390] priv->vdev.compose.height is 480
[   26.874268] compose.height is 576
[   26.877716] imx7-csi 2214000.csi: capture format not valid
VIDIOC_STREAMON returned -1 (Broken pipe)

Do you know why I am getting such a width/height mismatch?

Thanks,

Fabio Estevam

Re: [RFC 1/3] media: tw9910: Allow to probe from device tree

[Rui Miguel Silva]

Oi Fabio,
On Thu Sep 16, 2021 at 9:52 PM WEST, Fabio Estevam wrote:

> Hi Rui,
>
> On Mon, Sep 13, 2021 at 7:12 PM Rui Miguel Silva <rui.silva@linaro.org> wrote:
>
> > Looking  at this code it looks like you are using an old tree, Laurent
> > meanwhile have sent a patch that change this function:
> >
> > a9512b261afd ("media: imx: capture: Simplify capture_validate_fmt() implementation")
> >
> > can you test with the latest code as see if you have the same
> > problems.
>
> I have just tested on 5.15-rc1 and this is what I get:
>
> # v4l2-ctl --device /dev/v4l-subdev1 --set-standard PAL
> Standard set to 000000ff
> # media-ctl -l "'tw9910 2-0044':0 -> 'csi':0[1]"
> # media-ctl -l "'csi':1 -> 'csi capture':0[1]"
> # media-ctl -v -V "'tw9910 2-0044':0 [fmt:UYVY8_2X8/720x576
> field:interlaced-bt]"
> Opening media device /dev/media0
> Enumerating entities
> Found 3 entities
> Enumerating pads and links
> Setting up format UYVY8_2X8 720x576 on pad tw9910 2-0044/0
> Format set: UYVY8_2X8 720x576
> Setting up format UYVY8_2X8 720x576 on pad csi/0
> Format set: UYVY8_2X8 720x576
>
> # v4l2-ctl --stream-mmap -d /dev/video1
> [   26.860654] priv->vdev.compose.width is 640

This looks like the default pix width (IMX_MEDIA_DEF_PIX_WIDTH)

> [   26.865419] fmt_src.format.width is 720
> [   26.869390] priv->vdev.compose.height is 480

and this the default pix height (IMX_MEDIA_DEF_PIX_HEIGHT)

> [   26.874268] compose.height is 576
> [   26.877716] imx7-csi 2214000.csi: capture format not valid
> VIDIOC_STREAMON returned -1 (Broken pipe)
> Do you know why I am getting such a width/height mismatch?

So, it seems like it is not configuring the pads, don't you need to do
a media-ctl -V in the csi:0?

or maybe dump the output of media-ctl -p and check the links and pads
configurations.

Hope this helps.

Cheers,
   Rui

Re: [RFC 1/3] media: tw9910: Allow to probe from device tree

[Fabio Estevam]

Hi Rui,

On Thu, Sep 16, 2021 at 7:05 PM Rui Miguel Silva <rui.silva@linaro.org> wrote:

> So, it seems like it is not configuring the pads, don't you need to do
> a media-ctl -V in the csi:0?

Thanks for the suggestion. I tried passing media-ctl -V to csi:0:

# v4l2-ctl --device /dev/v4l-subdev1 --set-standard PAL
Standard set to 000000ff
# media-ctl -l "'tw9910 2-0044':0 -> 'csi':0[1]"
# media-ctl -l "'csi':1 -> 'csi capture':0[1]"
# media-ctl -v -V "'tw9910 2-0044':0 [fmt:UYVY8_2X8/720x576
field:interlaced-bt]"
# media-ctl -v -V "'csi':0 [fmt:UYVY8_2X8/720x576 field:interlaced-bt]"

Does this look correct?

Opening media device /dev/media0
Enumerating entities
Found 3 entities
Enumerating pads and links
Setting up format UYVY8_2X8 720x576 on pad tw9910 2-0044/0
Format set: UYVY8_2X8 720x576
Setting up format UYVY8_2X8 720x576 on pad csi/0
Format set: UYVY8_2X8 720x576
Opening media device /dev/media0
Enumerating entities
Found 3 entities
Enumerating pads and links
Setting up format UYVY8_2X8 720x576 on pad csi/0
Format set: UYVY8_2X8 720x576

# v4l2-ctl --stream-mmap -d /dev/video1
[   50.969910] priv->vdev.compose.width is 640
[   50.974685] fmt_src.format.width is 720
[   50.978659] priv->vdev.compose.height is 480
[   50.983208] compose.height is 576
[   50.986646] imx7-csi 2214000.csi: capture format not valid
VIDIOC_STREAMON returned -1 (Broken pipe)

but still getting the width/height mismatch.

# media-ctl -p
Media controller API version 5.15.0

Media device information
------------------------
driver          imx7-csi
model           imx-media
serial
bus info
hw revision     0x0
driver version  5.15.0

Device topology
- entity 1: csi (2 pads, 2 links)
            type V4L2 subdev subtype Unknown flags 0
            device node name /dev/v4l-subdev0
pad0: Sink
[fmt:UYVY8_2X8/720x576 field:none colorspace:srgb xfer:srgb ycbcr:601
quantization:lim-range]
<- "tw9910 2-0044":0 [ENABLED,IMMUTABLE]
pad1: Source
[fmt:UYVY8_2X8/720x576 field:none colorspace:srgb xfer:srgb ycbcr:601
quantization:lim-range]
-> "csi capture":0 [ENABLED,IMMUTABLE]

- entity 4: csi capture (1 pad, 1 link)
            type Node subtype V4L flags 0
            device node name /dev/video1
pad0: Sink
<- "csi":1 [ENABLED,IMMUTABLE]

- entity 10: tw9910 2-0044 (1 pad, 1 link)
             type V4L2 subdev subtype Decoder flags 0
             device node name /dev/v4l-subdev1
pad0: Source
[fmt:UYVY8_2X8/720x576 field:interlaced-bt colorspace:smpte170m
crop.bounds:(0,0)/768x576
crop:(0,0)/768x576]
-> "csi":0 [ENABLED,IMMUTABLE]

Thanks



> or maybe dump the output of media-ctl -p and check the links and pads
> configurations.
>
> Hope this helps.
>
> Cheers,
>    Rui