[PATCH] media: staging/imx: do not return error in link_notify for unknown sources

[PATCH] media: staging/imx: do not return error in link_notify for unknown sources

[Steve Longerbeam]

imx_media_link_notify() should not return error if the source subdevice
is not recognized by imx-media, that isn't an error. If the subdev has
controls they will be inherited starting from a known subdev.

Signed-off-by: Steve Longerbeam <steve_longerbeam@mentor.com>
---
 drivers/staging/media/imx/imx-media-dev.c | 11 +++++++++--
 1 file changed, 9 insertions(+), 2 deletions(-)

diff --git a/drivers/staging/media/imx/imx-media-dev.c b/drivers/staging/media/imx/imx-media-dev.c
index b55e5eb..dd47861 100644
--- a/drivers/staging/media/imx/imx-media-dev.c
+++ b/drivers/staging/media/imx/imx-media-dev.c
@@ -508,8 +508,15 @@ static int imx_media_link_notify(struct media_link *link, u32 flags,
 	imxmd = dev_get_drvdata(sd->v4l2_dev->dev);
 
 	imxsd = imx_media_find_subdev_by_sd(imxmd, sd);
-	if (IS_ERR(imxsd))
-		return PTR_ERR(imxsd);
+	if (IS_ERR(imxsd)) {
+		/*
+		 * don't bother if the source subdev isn't known to
+		 * imx-media. If the subdev has controls they will be
+		 * inherited starting from a known subdev.
+		 */
+		return 0;
+	}
+
 	imxpad = &imxsd->pad[pad_idx];
 
 	/*
-- 
2.7.4

Re: [PATCH] media: staging/imx: do not return error in link_notify for unknown sources

[Russell King - ARM Linux]

On Tue, Oct 03, 2017 at 12:09:13PM -0700, Steve Longerbeam wrote:
> imx_media_link_notify() should not return error if the source subdevice
> is not recognized by imx-media, that isn't an error. If the subdev has
> controls they will be inherited starting from a known subdev.

What does "a known subdev" mean?

-- 
RMK's Patch system: http://www.armlinux.org.uk/developer/patches/
FTTC broadband for 0.8mile line in suburbia: sync at 8.8Mbps down 630kbps up
According to speedtest.net: 8.21Mbps down 510kbps up

Re: [PATCH] media: staging/imx: do not return error in link_notify for unknown sources

[Steve Longerbeam]

On 10/11/2017 02:49 PM, Russell King - ARM Linux wrote:
> On Tue, Oct 03, 2017 at 12:09:13PM -0700, Steve Longerbeam wrote:
>> imx_media_link_notify() should not return error if the source subdevice
>> is not recognized by imx-media, that isn't an error. If the subdev has
>> controls they will be inherited starting from a known subdev.
> What does "a known subdev" mean?

It refers to the previous sentence, "not recognized by imx-media". A
subdev that was not registered via async registration and so not in
imx-media's async subdev list. I could elaborate in the commit message
but it seems fairly obvious to me.

Steve

Re: [PATCH] media: staging/imx: do not return error in link_notify for unknown sources

[Russell King - ARM Linux]

On Wed, Oct 11, 2017 at 03:14:26PM -0700, Steve Longerbeam wrote:
> 
> 
> On 10/11/2017 02:49 PM, Russell King - ARM Linux wrote:
> >On Tue, Oct 03, 2017 at 12:09:13PM -0700, Steve Longerbeam wrote:
> >>imx_media_link_notify() should not return error if the source subdevice
> >>is not recognized by imx-media, that isn't an error. If the subdev has
> >>controls they will be inherited starting from a known subdev.
> >What does "a known subdev" mean?
> 
> It refers to the previous sentence, "not recognized by imx-media". A
> subdev that was not registered via async registration and so not in
> imx-media's async subdev list. I could elaborate in the commit message
> but it seems fairly obvious to me.

I don't think it's obvious, and I suspect you won't think it's obvious
in years to come (I talk from experience of some commentry I've added
in the past.)

Now, isn't it true that for a subdev to be part of a media device, it
has to be registered, and if it's part of a media device that is made
up of lots of different drivers, it has to use the async registration
code?  So, is it not also true that any subdev that is part of a
media device, it will be "known"?

Under what circumstances could a subdev be part of a media device but
not be "known" ?

Now, if you mean "known" to be equivalent with "recognised by
imx-media" then, as I've pointed out several times already, that
statement is FALSE.  I'm not sure how many times I'm going to have to
state this fact.  Let me re-iterate again.  On my imx219 driver, I
have two subdevs.  Both subdevs have controls attached.  The pixel
subdev is not "recognised" by imx-media, and without a modification
like my or your patch, it fails.  However, with the modification,
this "unrecognised" subdev _STILL_ has it's controls visible through
imx-media.

Hence, I believe your comment in the code and your commit message
are misleading and wrong.

-- 
RMK's Patch system: http://www.armlinux.org.uk/developer/patches/
FTTC broadband for 0.8mile line in suburbia: sync at 8.8Mbps down 630kbps up
According to speedtest.net: 8.21Mbps down 510kbps up

Re: [PATCH] media: staging/imx: do not return error in link_notify for unknown sources

[Steve Longerbeam]

On 10/11/2017 04:06 PM, Russell King - ARM Linux wrote:
> On Wed, Oct 11, 2017 at 03:14:26PM -0700, Steve Longerbeam wrote:
>>
>> On 10/11/2017 02:49 PM, Russell King - ARM Linux wrote:
>>> On Tue, Oct 03, 2017 at 12:09:13PM -0700, Steve Longerbeam wrote:
>>>> imx_media_link_notify() should not return error if the source subdevice
>>>> is not recognized by imx-media, that isn't an error. If the subdev has
>>>> controls they will be inherited starting from a known subdev.
>>> What does "a known subdev" mean?
>> It refers to the previous sentence, "not recognized by imx-media". A
>> subdev that was not registered via async registration and so not in
>> imx-media's async subdev list. I could elaborate in the commit message
>> but it seems fairly obvious to me.
> I don't think it's obvious, and I suspect you won't think it's obvious
> in years to come (I talk from experience of some commentry I've added
> in the past.)
>
> Now, isn't it true that for a subdev to be part of a media device, it
> has to be registered, and if it's part of a media device that is made
> up of lots of different drivers, it has to use the async registration
> code?  So, is it not also true that any subdev that is part of a
> media device, it will be "known"?
>
> Under what circumstances could a subdev be part of a media device but
> not be "known" ?
>
> Now, if you mean "known" to be equivalent with "recognised by
> imx-media" then, as I've pointed out several times already, that
> statement is FALSE.  I'm not sure how many times I'm going to have to
> state this fact.  Let me re-iterate again.  On my imx219 driver, I
> have two subdevs.  Both subdevs have controls attached.  The pixel
> subdev is not "recognised" by imx-media, and without a modification
> like my or your patch, it fails.  However, with the modification,
> this "unrecognised" subdev _STILL_ has it's controls visible through
> imx-media.

Well that's true, the controls for your pixel subdev (which was
not registered via async) still are visible to imx-media, so in that
sense the subdev is "known" to imx-media.

>
> Hence, I believe your comment in the code and your commit message
> are misleading and wrong.

Ok you convinced me, I'll fix the code comment and commit
message.

Steve

Re: [PATCH] media: staging/imx: do not return error in link_notify for unknown sources

[Russell King - ARM Linux]

On Thu, Oct 12, 2017 at 12:06:33AM +0100, Russell King - ARM Linux wrote:
> Now, if you mean "known" to be equivalent with "recognised by
> imx-media" then, as I've pointed out several times already, that
> statement is FALSE.  I'm not sure how many times I'm going to have to
> state this fact.  Let me re-iterate again.  On my imx219 driver, I
> have two subdevs.  Both subdevs have controls attached.  The pixel
> subdev is not "recognised" by imx-media, and without a modification
> like my or your patch, it fails.  However, with the modification,
> this "unrecognised" subdev _STILL_ has it's controls visible through
> imx-media.

If you refuse to believe what I'm saying, then read through:

http://git.armlinux.org.uk/cgit/linux-arm.git/commit/?h=csi-v6&id=e3f847cd37b007d55b76282414bfcf13abb8fc9a

and look at this:

# for f in 2 3 4 5 6 7 8 9; do v4l2-ctl -L -d /dev/video$f; done
# ./cam/cam-setup.sh
# v4l2-ctl -L -d /dev/video6

User Controls

                           gain (int)    : min=256 max=4095 step=1 default=256 value=256
                     fim_enable (bool)   : default=0 value=0
                fim_num_average (int)    : min=1 max=64 step=1 default=8 value=8              fim_tolerance_min (int)    : min=2 max=200 step=1 default=50 value=50
              fim_tolerance_max (int)    : min=0 max=500 step=1 default=0 value=0
                   fim_num_skip (int)    : min=0 max=256 step=1 default=2 value=2
         fim_input_capture_edge (int)    : min=0 max=3 step=1 default=0 value=0
      fim_input_capture_channel (int)    : min=0 max=1 step=1 default=0 value=0

Camera Controls

         exposure_time_absolute (int)    : min=1 max=399 step=1 default=399 value=166

Image Source Controls

              vertical_blanking (int)    : min=32 max=64814 step=1 default=2509
value=2509 flags=read-only
            horizontal_blanking (int)    : min=2168 max=31472 step=1 default=2168 value=2168 flags=read-only
                  analogue_gain (int)    : min=1000 max=8000 step=1 default=1000 value=1000
                red_pixel_value (int)    : min=0 max=1023 step=1 default=0 value=0 flags=inactive
          green_red_pixel_value (int)    : min=0 max=1023 step=1 default=0 value=0 flags=inactive
               blue_pixel_value (int)    : min=0 max=1023 step=1 default=0 value=0 flags=inactive
         green_blue_pixel_value (int)    : min=0 max=1023 step=1 default=0 value=0 flags=inactive

Image Processing Controls

                     pixel_rate (int64)  : min=160000000 max=278400000 step=1 default=278400000 value=278400000 flags=read-only
                   test_pattern (menu)   : min=0 max=9 default=0 value=0
                                0: Disabled
                                1: Solid Color
                                2: 100% Color Bars
                                3: Fade to Grey Color Bar
                                4: PN9
                                5: 16 Split Color Bar
                                6: 16 Split Inverted Color Bar
                                7: Column Counter
                                8: Inverted Column Counter
                                9: PN31

Now, the pixel array subdev has these controls:
	gain
	vertical_blanking
	horizontal_blanking
	analogue_gain
	pixel_rate
	exposure_time_absolute

The root subdev (which is the one which connects to your code) has
these controls:
	red_pixel_value
	green_red_pixel_value
	blue_pixel_value
	green_blue_pixel_value
	test_pattern

As you can see from the above output, _all_ controls from _both_ subdevs
are listed.

Now, before you spot your old patch adding v4l2_pipeline_inherit_controls()
and try to blame that for this, nothing in my kernel calls that function,
so that patch can be dropped, and so it's not responsible for this.

-- 
RMK's Patch system: http://www.armlinux.org.uk/developer/patches/
FTTC broadband for 0.8mile line in suburbia: sync at 8.8Mbps down 630kbps up
According to speedtest.net: 8.21Mbps down 510kbps up