| Message ID | 20210827130150.909695-1-festevam@gmail.com |
|---|---|
| State | New |
| Headers | show |
| Series | [RFC,1/3] media: tw9910: Allow to probe from device tree | expand |
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
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 >
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 >
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
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
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
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
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
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
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,
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(-)