LKML Archive on lore.kernel.org help / color / mirror / Atom feed
From: Hans Verkuil <email@example.com> To: Tim Harvey <firstname.lastname@example.org> Cc: Rob Herring <email@example.com>, linux-media <firstname.lastname@example.org>, email@example.com, "firstname.lastname@example.org" <email@example.com>, "firstname.lastname@example.org" <email@example.com>, Shawn Guo <firstname.lastname@example.org>, Steve Longerbeam <email@example.com>, Philipp Zabel <firstname.lastname@example.org>, Hans Verkuil <email@example.com>, Mauro Carvalho Chehab <firstname.lastname@example.org> Subject: Re: [PATCH 3/5] media: i2c: Add TDA1997x HDMI receiver driver Date: Thu, 14 Dec 2017 11:11:52 +0100 [thread overview] Message-ID: <email@example.com> (raw) In-Reply-To: <CAJ+vNU1MBqK2ZkDhu7HM-EfNmgceXtah9srygzKuW-ajq9nhtA@mail.gmail.com> On 14/12/17 00:33, Tim Harvey wrote: > On Tue, Dec 12, 2017 at 4:18 AM, Hans Verkuil <firstname.lastname@example.org> wrote: >> Hi Tim, >> >> Sorry for the delay, I needed to find some time to think about this. >> >> On 11/16/17 05:30, Rob Herring wrote: >>> On Wed, Nov 15, 2017 at 10:31:14AM -0800, Tim Harvey wrote: >>>> On Wed, Nov 15, 2017 at 7:52 AM, Rob Herring <email@example.com> wrote: >>>>> On Thu, Nov 09, 2017 at 10:45:34AM -0800, Tim Harvey wrote: >>>>>> Add support for the TDA1997x HDMI receivers. >>>>>> >>>>>> Cc: Hans Verkuil <firstname.lastname@example.org> >>>>>> Signed-off-by: Tim Harvey <email@example.com> >>>>>> --- >>>>>> v3: >>>>>> - use V4L2_DV_BT_FRAME_WIDTH/HEIGHT macros >>>>>> - fixed missing break >>>>>> - use only hdmi_infoframe_log for infoframe logging >>>>>> - simplify tda1997x_s_stream error handling >>>>>> - add delayed work proc to handle hotplug enable/disable >>>>>> - fix set_edid (disable HPD before writing, enable after) >>>>>> - remove enabling edid by default >>>>>> - initialize timings >>>>>> - take quant range into account in colorspace conversion >>>>>> - remove vendor/product tracking (we provide this in log_status via infoframes) >>>>>> - add v4l_controls >>>>>> - add more detail to log_status >>>>>> - calculate vhref generator timings >>>>>> - timing detection fixes (rounding errors, hswidth errors) >>>>>> - rename configure_input/configure_conv functions >>>>>> >>>>>> v2: >>>>>> - implement dv timings enum/cap >>>>>> - remove deprecated g_mbus_config op >>>>>> - fix dv_query_timings >>>>>> - add EDID get/set handling >>>>>> - remove max-pixel-rate support >>>>>> - add audio codec DAI support >>>>>> - change audio bindings >>>>>> --- >>>>>> drivers/media/i2c/Kconfig | 9 + >>>>>> drivers/media/i2c/Makefile | 1 + >>>>>> drivers/media/i2c/tda1997x.c | 3485 ++++++++++++++++++++++++++++++++++ >>>>>> include/dt-bindings/media/tda1997x.h | 78 + >>>>> >>>>> This belongs with the binding documentation patch. >>>>> >>>> >>>> Rob, >>>> >>>> Thanks - missed that. I will move it for v4. >>>> >>>> Regarding your previous comment to the v2 series: >>>>> The rest of the binding looks fine, but I have some reservations about >>>>> this. I think this should be common probably. There's been a few >>>>> bindings for display recently that deal with the interface format. Maybe >>>>> some vendor property is needed here to map a standard interface format >>>>> back to pin configuration. >>>> >>>> I take it this is not an 'Ack' for the bindings? >>>> >>>> Which did you feel should be made common? I admit I was surprised >>>> there wasn't a common binding for audio bus format (i2s|spdif) but if >>>> you were referring to the video data that would probably be much more >>>> complicated. >>> >>> The video data. Either you have to try to come up with some way to map >>> color components to signals/pins (and even cycles) or you just enumerate >>> the formats and keep adding to them when new ones appear. There's h/w >>> that allows the former, but in the end you have to interoperate, so >>> enumerating the formats is probably enough. >>> >>>> I was hoping one of the media/driver maintainers would respond to your >>>> comment with thoughts as I'm not familiar with a very wide variety of >>>> receivers. >>> >>> I am hoping, too. >> >> I don't think it is right to store this in the DT. How you map the output pins >> is a driver thing. So when you are requested to enumerate the mediabus formats >> (include/uapi/linux/media-bus-format.h) you support, you do so based on the >> capabilities of the hardware, and when a format is requested you program the >> hardware accordingly. >> >> The device tree should describe the physical characteristics like the number >> of pins that are hooked up (i.e. are there 24, 30 or 36 pins connected). >> >> These vidout-portcfg mappings do not appear to describe physical properties >> but really register settings. > > Hans, > > They are register settings that define which bits on the internal data > bus are mapped to which pins on the package. Internally these parts > have a 36bit video data bus but externally the tda19971 only has 24 > pins and even then perhaps only 8 are hooked up if using BT656 and the > registers also define 'which 8' as it could have been connected to the > upper or lower part of the bus. So while the bindings from > video-interfaces.txt provide bus-width and details of the sync > signals, additional hardware-specific interconnect details such as how > the video bits are shifted/mixed on the output bus are needed here. > This is why I feel vidout-portcfg should be a dt property vs something > like a module param. The video-interfaces.txt bindings text defines bus-width and data-shift. The last one would define 'which 8'. Only if the hardware can do weird stuff like using pins 4-7 and 16-19 would this be insufficient. But in that case we would need something more detailed in video-interfaces.txt. Frankly, unless you've seen anyone do something weird like that I would stick to bus-width and data-shift. Regards, Hans
next prev parent reply other threads:[~2017-12-14 10:11 UTC|newest] Thread overview: 23+ messages / expand[flat|nested] mbox.gz Atom feed top 2017-11-09 18:45 [PATCH v3 0/5] TDA1997x HDMI video receiver Tim Harvey 2017-11-09 18:45 ` [PATCH 1/5] MAINTAINERS: add entry for NXP TDA1997x driver Tim Harvey 2017-11-09 18:45 ` [PATCH 2/5] media: dt-bindings: Add bindings for TDA1997X Tim Harvey 2017-11-22 7:36 ` Sakari Ailus 2017-11-23 4:37 ` Tim Harvey 2017-11-23 8:25 ` Sakari Ailus 2017-11-29 15:54 ` Tim Harvey 2017-11-09 18:45 ` [PATCH 3/5] media: i2c: Add TDA1997x HDMI receiver driver Tim Harvey 2017-11-15 15:52 ` Rob Herring 2017-11-15 18:31 ` Tim Harvey 2017-11-16 4:30 ` Rob Herring 2017-11-23 4:49 ` Tim Harvey 2017-12-12 12:18 ` Hans Verkuil 2017-12-13 23:33 ` Tim Harvey 2017-12-14 10:11 ` Hans Verkuil [this message] 2017-11-20 15:39 ` Hans Verkuil 2017-11-23 4:27 ` Tim Harvey 2017-11-23 8:08 ` Hans Verkuil 2017-11-29 15:47 ` Tim Harvey 2017-12-16 19:32 ` [alsa-devel] " Fabio Estevam 2017-12-18 22:21 ` Tim Harvey 2017-11-09 18:45 ` [PATCH 4/5] ARM: dts: imx: Add TDA19971 HDMI Receiver to GW54xx Tim Harvey 2017-11-09 18:45 ` [PATCH 5/5] ARM: dts: imx: Add TDA19971 HDMI Receiver to GW551x Tim Harvey
Reply instructions: You may reply publicly to this message via plain-text email using any one of the following methods: * Save the following mbox file, import it into your mail client, and reply-to-all from there: mbox Avoid top-posting and favor interleaved quoting: https://en.wikipedia.org/wiki/Posting_style#Interleaved_style * Reply using the --to, --cc, and --in-reply-to switches of git-send-email(1): git send-email \ --firstname.lastname@example.org \ --email@example.com \ --firstname.lastname@example.org \ --email@example.com \ --firstname.lastname@example.org \ --email@example.com \ --firstname.lastname@example.org \ --email@example.com \ --firstname.lastname@example.org \ --email@example.com \ --firstname.lastname@example.org \ --email@example.com \ --firstname.lastname@example.org \ /path/to/YOUR_REPLY https://kernel.org/pub/software/scm/git/docs/git-send-email.html * If your mail client supports setting the In-Reply-To header via mailto: links, try the mailto: linkBe sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).