From mboxrd@z Thu Jan 1 00:00:00 1970 Return-path: Received: from galahad.ideasonboard.com ([185.26.127.97]:44967 "EHLO galahad.ideasonboard.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1752871AbcLHMZO (ORCPT ); Thu, 8 Dec 2016 07:25:14 -0500 From: Laurent Pinchart To: Mauro Carvalho Chehab Cc: Linux Media Mailing List , Mauro Carvalho Chehab , Mauro Carvalho Chehab , Guennadi Liakhovetski , Javier Martinez Canillas , "Lad, Prabhakar" , Hans Verkuil , Robert Jarzmik , Wei Yongjun , Devin Heitmueller Subject: Re: [PATCH RFC] tvp5150: don't touch register TVP5150_CONF_SHARED_PIN if not needed Date: Thu, 08 Dec 2016 14:25:38 +0200 Message-ID: <4433804.mBBNy2npBT@avalon> In-Reply-To: References: MIME-Version: 1.0 Content-Transfer-Encoding: 7Bit Content-Type: text/plain; charset="us-ascii" Sender: linux-media-owner@vger.kernel.org List-ID: Hi Mauro, Thank you for the patch. 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 > Cc: Javier Martinez Canillas > Cc: Laurent Pinchart , > Signed-off-by: Mauro Carvalho Chehab > --- > > 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. > */ > enum v4l2_mbus_type { > V4L2_MBUS_PARALLEL, > V4L2_MBUS_BT656, > V4L2_MBUS_CSI2, > + V4L2_MBUS_UNKNOWN, > }; > > /** -- Regards, Laurent Pinchart