Re: [PATCH RFC] tvp5150: don't touch register TVP5150_CONF_SHARED_PIN if not needed

[Laurent Pinchart <laurent.pinchart@ideasonboard.com>]

Hi Mauro,

On Thursday 08 Dec 2016 10:53:33 Mauro Carvalho Chehab wrote:
> Em Thu, 08 Dec 2016 14:25:38 +0200 Laurent Pinchart escreveu:
> > On Thursday 08 Dec 2016 06:55:58 Mauro Carvalho Chehab wrote:
> >> changeset 460b6c0831cb ("[media] tvp5150: Add s_stream subdev operation
> >> support") added a logic that overrides TVP5150_CONF_SHARED_PIN setting,
> >> depending on the type of bus set via the .set_fmt() subdev callback.
> >> 
> >> This is known to cause trobules on devices that don't use a V4L2
> >> subdev devnode, and a fix for it was made by changeset 47de9bf8931e
> >> ("[media] tvp5150: Fix breakage for serial usage"). Unfortunately,
> >> such fix doesn't consider the case of progressive video inputs,
> >> causing chroma decoding issues on such videos, as it overrides not
> >> only the type of video output, but also other unrelated bits.
> >> 
> >> So, instead of trying to guess, let's detect if the device is set
> >> via a V4L2 subdev node or not. If not, just ignore the bogus logic.
> >> 
> >> Fixes: 460b6c0831cb ("[media] tvp5150: Add s_stream subdev operation
> >> support") Cc: Devin Heitmueller <dheitmueller@kernellabs.com>
> >> Cc: Javier Martinez Canillas <javier@osg.samsung.com>
> >> Cc: Laurent Pinchart <laurent.pinchart@ideasonboard.com>,
> >> Signed-off-by: Mauro Carvalho Chehab <mchehab@s-opensource.com>
> >> ---
> >> 
> >> Devin,
> >> 
> >> I didn't test this patch. As I explained on my previous e-mail, my
> >> current test scenario for analog TV inputs is not  ideal, as I lack
> >> progressive video and RF output testcases.
> >> 
> >> Could you please test if this will fix for you?
> >> 
> >> Laurent/Javier,
> >> 
> >> With regards to OMAP3, it would be good to try to reproduce the issues
> >> Devin noticed on your hardware, testing with both progressive and
> >> interlaced sources and checking if the chroma is being decoded properly
> >> or not with a NTSC signal.
> >> 
> >>  drivers/media/i2c/tvp5150.c                      | 7 ++++++-
> >>  drivers/media/platform/pxa_camera.c              | 1 +
> >>  drivers/media/platform/soc_camera/soc_mediabus.c | 1 +
> >>  include/media/v4l2-mediabus.h                    | 3 +++
> >>  4 files changed, 11 insertions(+), 1 deletion(-)
> > 
> > [snip]
> > 
> >> diff --git a/include/media/v4l2-mediabus.h
> >> b/include/media/v4l2-mediabus.h
> >> index 34cc99e093ef..8af6b96d628b 100644
> >> --- a/include/media/v4l2-mediabus.h
> >> +++ b/include/media/v4l2-mediabus.h
> >> @@ -70,11 +70,14 @@
> >>   * @V4L2_MBUS_BT656:	parallel interface with embedded
> >>   synchronisation, can
> >>   *			also be used for BT.1120
> >>   * @V4L2_MBUS_CSI2:	MIPI CSI-2 serial interface
> >> + * @V4L2_MBUS_UNKNOWN:	used to indicate that the device is not
> >> controlled
> >> + *			via a V4L2 subdev devnode interface
> > 
> > Please, don't. v4l2_mbus_type has nothing to do with subdev device nodes.
> > It identifies the type of physical bus used to carry video data.
> 
> As explained, for the devices that the regression was introduced,
> there's no way to control the physical bus, as it is hardwired.
> 
> For those devices, mbus_type is just an useless information.

But the fact that a field is not used in some cases is no excuse to abuse it 
for something completely unrelated to its purpose.

> The problem that your patch introduced is that it now assumes that
> the mbus would be controlled via a V4L2 subdevice. This is a wrong
> assumption, and caused a regression.
> 
> So, we need a simple way to get rid of the broken code on those
> devices, and that's the simplest way.

We need a correct way to fix the problem, and this isn't the correct way.

> If you didn't like the name, we could, instead, call it as:
> V4L2_MBUS_CONTROLLED_BY_VIDEODEV, with actually tells more about it.

No, V4L2_MBUS_* has *nothing* to do with how the device is controlled. 

> ---
> 
> That's said, as OMAP3 support was added considering this code,
> if the progressive video bug is also present there, we could
> try some other solution, although, in such case, it is not a
> regression. Unfortunately, I don't have any OMAP3 hardware with
> tvp5151 to test.

I do, so I could test that.

> So, if nobody comes with another solution that would work for
> both cases, I'll likely send this regression fix to 4.10, as
> it is better than reverting the broken patch, as reverting it
> would cause regressions for OMAP3.

You're modifying the in-kernel V4L2 API as a hack to work around a bug. Let's 
not do that.

-- 
Regards,

Laurent Pinchart