Re: [PATCH v2 3/4] media: i2c: Add TDA1997x HDMI receiver driver

From: Tim Harvey
Date: Thu Oct 19 2017 - 03:21:02 EST

Next message: Hans Verkuil: "Re: [PATCH 0/2] [media] rc/keymaps: add support for two RCs of hisilicon boards."
Previous message: Hans de Goede: "[PATCH v3 2/2] platform/x86: silead_dmi: Add silead,home-button property to some tablets"
In reply to: Hans Verkuil: "Re: [PATCH v2 3/4] media: i2c: Add TDA1997x HDMI receiver driver"
Next in thread: Hans Verkuil: "Re: [PATCH v2 3/4] media: i2c: Add TDA1997x HDMI receiver driver"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]

On Wed, Oct 18, 2017 at 5:04 AM, Hans Verkuil <hverkuil@xxxxxxxxx> wrote:
> Hi Tim,
>
> Here is my review of this v2:
>
> On 10/12/17 06:45, Tim Harvey wrote:
>> Add support for the TDA1997x HDMI receivers.
>>
<snip>
>> +
>> +/*
>> + * Video Input formats
>> + */
>> +struct vhref_values {
>> + u16 href_start;
>> + u16 href_end;
>> + u16 vref_f1_start;
>> + u8 vref_f1_width;
>> + u16 vref_f2_start;
>> + u8 vref_f2_width;
>> + u16 fieldref_f1_start;
>> + u8 fieldPolarity;
>> + u16 fieldref_f2_start;
>
> Since we don't support interlaced (yet) I'd just drop the 'f2' fields.
> Ditto for fieldPolarity.
>
> Can't these href/vref values be calculated from the timings?
>

The values in this struct are used to configure the tda1997x VHREF
timing generator in tda1997x_configure_input_resolution() for
generating the video output bus timings so I can't really drop them
unless I can calculate them. Let me look into this - should be
possible.

>> +};
>> +
<snip>
>> +/* parse an infoframe and do some sanity checks on it */
>> +static unsigned int
>> +tda1997x_parse_infoframe(struct tda1997x_state *state, u16 addr)
>> +{
>> + struct v4l2_subdev *sd = &state->sd;
>> + union hdmi_infoframe frame;
>> + u8 buffer[40];
>> + u8 reg;
>> + int len, err;
>> +
>> + /* read data */
>> + len = io_readn(sd, addr, sizeof(buffer), buffer);
>> + err = hdmi_infoframe_unpack(&frame, buffer);
>> + if (err) {
>> + v4l_err(state->client,
>> + "failed parsing %d byte infoframe: 0x%04x/0x%02x\n",
>> + len, addr, buffer[0]);
>> + return err;
>> + }
>> + if (debug > 1)
>> + hdmi_infoframe_log(KERN_INFO, &state->client->dev, &frame);
>> + switch (frame.any.type) {
>> + /* Audio InfoFrame: see HDMI spec 8.2.2 */
>> + case HDMI_INFOFRAME_TYPE_AUDIO:
>> + /* sample rate */
>> + switch (frame.audio.sample_frequency) {
>> + case HDMI_AUDIO_SAMPLE_FREQUENCY_32000:
>> + state->audio_samplerate = 32000;
>> + break;
>> + case HDMI_AUDIO_SAMPLE_FREQUENCY_44100:
>> + state->audio_samplerate = 44100;
>> + break;
>> + case HDMI_AUDIO_SAMPLE_FREQUENCY_48000:
>> + state->audio_samplerate = 48000;
>> + break;
>> + case HDMI_AUDIO_SAMPLE_FREQUENCY_88200:
>> + state->audio_samplerate = 88200;
>> + break;
>> + case HDMI_AUDIO_SAMPLE_FREQUENCY_96000:
>> + state->audio_samplerate = 96000;
>> + break;
>> + case HDMI_AUDIO_SAMPLE_FREQUENCY_176400:
>> + state->audio_samplerate = 176400;
>> + break;
>> + case HDMI_AUDIO_SAMPLE_FREQUENCY_192000:
>> + state->audio_samplerate = 192000;
>> + break;
>> + default:
>> + case HDMI_AUDIO_SAMPLE_FREQUENCY_STREAM:
>> + break;
>> + }
>> +
>> + /* sample size */
>> + switch (frame.audio.sample_size) {
>> + case HDMI_AUDIO_SAMPLE_SIZE_16:
>> + state->audio_samplesize = 16;
>> + break;
>> + case HDMI_AUDIO_SAMPLE_SIZE_20:
>> + state->audio_samplesize = 20;
>> + break;
>> + case HDMI_AUDIO_SAMPLE_SIZE_24:
>> + state->audio_samplesize = 24;
>> + break;
>> + case HDMI_AUDIO_SAMPLE_SIZE_STREAM:
>> + default:
>> + break;
>> + }
>> +
>> + /* Channel Count */
>> + state->audio_channels = frame.audio.channels;
>> + if (frame.audio.channel_allocation &&
>> + frame.audio.channel_allocation != state->audio_ch_alloc) {
>> + /* use the channel assignment from the infoframe */
>> + state->audio_ch_alloc = frame.audio.channel_allocation;
>> + tda1997x_configure_audout(sd, state->audio_ch_alloc);
>> + /* reset the audio FIFO */
>> + tda1997x_hdmi_info_reset(sd, RESET_AUDIO, false);
>> + }
>> + break;
>> +
>> + /* Source Product Descriptor information (SPD) */
>> + case HDMI_INFOFRAME_TYPE_SPD:
>> + strncpy(frame.spd.vendor, state->vendor,
>> + sizeof(frame.spd.vendor));
>> + strncpy(frame.spd.product, state->product,
>> + sizeof(frame.spd.product));
>> + v4l_info(state->client, "Source Product Descriptor: %s %s\n",
>> + state->vendor, state->product);
>
> Use hdmi_infoframe_log() for logging infoframes.

ok - I will always call hdmi_infoframe_log() above and refrain from
outputs that just repeat those details.

>
>> + break;
>> +
>> + /* Auxiliary Video information (AVI) InfoFrame: see HDMI spec 8.2.1 */
>> + case HDMI_INFOFRAME_TYPE_AVI:
>> + state->colorspace = frame.avi.colorspace;
>> + state->colorimetry = frame.avi.colorimetry;
>> + /*
>> + * If colorimetry not specified, conversion depends on res type:
>> + * - SDTV: ITU601 for SD (480/576/240/288 line resolution)
>> + * - HDTV: ITU709 for HD (720/1080 line resolution)
>> + * - PC: sRGB
>> + * see HDMI specification section 6.7
>> + */
>> + if ((state->colorspace == HDMI_COLORSPACE_YUV422 ||
>> + state->colorspace == HDMI_COLORSPACE_YUV444) &&
>> + (state->colorimetry == HDMI_COLORIMETRY_EXTENDED ||
>> + state->colorimetry == HDMI_COLORIMETRY_NONE)) {
>> + switch (state->timings.bt.height) {
>> + case 480:
>> + case 576:
>> + case 240:
>> + case 288:
>> + state->colorimetry = HDMI_COLORIMETRY_ITU_601;
>> + break;
>> + case 720:
>> + case 1080:
>> + state->colorimetry = HDMI_COLORIMETRY_ITU_709;
>> + break;
>> + default:
>> + state->colorimetry = HDMI_COLORIMETRY_NONE;
>
> Missing break.
>

oops - thanks

>> + }
>> + }
>> + v4l_dbg(1, debug, state->client,
>> + "Colorspace=%d Colorimetry=%d\n",
>> + state->colorspace, state->colorimetry);
>> +
>> + /* configure upsampler: 0=bypass 1=repeatchroma 2=interpolate */
>> + reg = io_read(sd, REG_PIX_REPEAT);
>> + reg &= ~PIX_REPEAT_MASK_UP_SEL;
>> + if (state->colorspace == HDMI_COLORSPACE_YUV422)
>> + reg |= (PIX_REPEAT_CHROMA << PIX_REPEAT_SHIFT);
>> + io_write(sd, REG_PIX_REPEAT, reg);
>> +
>> + /* ConfigurePixelRepeater: repeat n-times each pixel */
>> + reg = io_read(sd, REG_PIX_REPEAT);
>> + reg &= ~PIX_REPEAT_MASK_REP;
>> + reg |= frame.avi.pixel_repeat;
>> + io_write(sd, REG_PIX_REPEAT, reg);
>> +
>> + /* configure the receiver with the new colorspace */
>> + tda1997x_configure_conv(sd, state->colorspace,
>> + state->colorimetry);
>
> What I am missing here is handling of the RGB quantization range.
> An HDMI receiver will typically send full range RGB or limited range YUV
> to the SoC. The HDMI source can however send full or limited range RGB
> or limited range YUV (full range YUV is theoretically possible, but nobody
> does that).
>

isn't this quantization range a function of the colorspace and
colorimetry dictated by the AVI infoframe? I'm taking these into
consideration when setting up the conversion matrix in
tda1997x_configure_conv().

> For a Full HD receiver the rules when receiving RGB video are as follows:
>
> If the EDID supports selectable RGB Quantization Range, then check if the
> source explicitly sets the RGB quantization range in the AVI InfoFrame and
> use that value.
>
> Otherwise fall back to the default rules:
>
> if VIC == 0, then expect full range RGB, otherwise expect limited range RGB.

Are you referring to the video_code field of the AVI infoframe or vic
from a vendor infoframe?

>
> It gets even more complicated with 4k video, but this is full HD only.
>
> In addition, you may also want to implement the V4L2_CID_DV_RX_RGB_RANGE control
> to let userspace override the autodetection.

I'll add that as an additional patch. Are there other V4L2_CID's that
I should be adding here?

>
> RGB Quantization Range handling is *the* biggest headache for HDMI receivers.
>
> If you happen to attend the Embedded Linux Conference Europe in Prague next
> week, then attend my presentation on HDMI 4k Video on the Wednesday for all
> the reasons why this is so tricky.
>
>> + break;
>> + default:
>> + break;
>> + }
>> + return 0;
>> +}
>> +
<snip>
>> +static int tda1997x_query_dv_timings(struct v4l2_subdev *sd,
>> + struct v4l2_dv_timings *timings)
>> +{
>> + struct tda1997x_state *state = to_state(sd);
>> + int ret;
>> +
>> + v4l_dbg(1, debug, state->client, "%s\n", __func__);
>> + if (!timings)
>> + return -EINVAL;
>> +
>> + memset(timings, 0, sizeof(struct v4l2_dv_timings));
>> + mutex_lock(&state->lock);
>> + ret = tda1997x_detect_std(state);
>> + if (ret)
>> + goto error;
>> + *timings = state->std->timings;
>> + mutex_unlock(&state->lock);
>> + return 0;
>> +
>> +error:
>> + mutex_unlock(&state->lock);
>> + return ret;
>
> This can be simplified:
>
> ret = tda1997x_detect_std(state);
> if (!ret)
> *timings = state->std->timings;
> mutex_unlock(&state->lock);
> return ret;
>

yes, will do

>> +}
>> +
<snip>
>> +
>> +static int tda1997x_set_edid(struct v4l2_subdev *sd, struct v4l2_edid *edid)
>> +{
>> + struct tda1997x_state *state = to_state(sd);
>> + int i;
>> +
>> + v4l_dbg(1, debug, state->client, "%s pad=%d\n", __func__, edid->pad);
>> + memset(edid->reserved, 0, sizeof(edid->reserved));
>> +
>> + if (edid->start_block != 0)
>> + return -EINVAL;
>> +
>> + if (edid->blocks == 0) {
>> + state->edid.blocks = 0;
>> + state->edid.present = 0;
>> + tda1997x_manual_hpd(&state->sd, HPD_LOW_BP);
>> + return 0;
>> + }
>> +
>> + if (edid->blocks > 2) {
>> + edid->blocks = 2;
>> + return -E2BIG;
>> + }
>> +
>> + /* write base EDID */
>> + for (i = 0; i < 128; i++)
>> + io_write(sd, REG_EDID_IN_BYTE0 + i, edid->edid[i]);
>> +
>> + /* write CEA Extension */
>> + for (i = 0; i < 128; i++)
>> + io_write(sd, REG_EDID_IN_BYTE128 + i, edid->edid[i+128]);
>> +
>
> Before updating the EDID pull the HPD low. Afterwards pull it up again.
> The minimum time the HPD should remain low is 100 ms.
>

ok - I will add a delayed work procedure to handle this.

By the way, how do I get/set EDID on a v4l2-subdev?

root@ventana:~# v4l2-ctl -d4 --set-edid=pad=0,type=hdmi

CEA-861 Header
IT Formats Underscanned: yes
Audio: yes
YCbCr 4:4:4: yes
YCbCr 4:2:2: yes

Speaker Allocation Data Block
FL/FR: yes
LFE: no
FC: no
RL/RR: no
RC: no
FLC/FRC: no
RLC/RRC: no
FLW/FRW: no
FLH/FRH: no
TC: no
FCH: no

HDMI Vendor-Specific Data Block
Max TMDS Clock: 170 MHz
Physical Address: 1.0.0.0
YCbCr 4:4:4 Deep Color: no
30-bit: no
36-bit: no
48-bit: no
Graphics: yes
Photo: no
Cinema: no
Game: no

CEA-861 Video Capability Descriptor
RGB Quantization Range: yes
YCC Quantization Range: yes
PT: Always Underscanned
IT: Always Underscanned
CE: Always Underscanned

CEA-861 Colorimetry Data Block
xvYCC 601: no
xvYCC 709: no
sYCC: no
AdobeRGB: no
AdobeYCC: no
BT.2020 RGB: no
BT.2020 YCC: no
BT.2020 cYCC: no

CEA-861 HDR Static Metadata Data Block
SDR (Traditional Gamma): yes
HDR (Traditional Gamma): no
SMPTE 2084: no
VIDIOC_S_EDID: failed: Inappropriate ioctl for device
root@ventana:~#

I'm also not clear how to run v4l2-compliance on a v4l2-subdev, so I
just ran it on one of the video devs from the capture driver I'm
linked to via media-ctl.

>> + return 0;
>> +}
>> +
>> +static int tda1997x_get_dv_timings_cap(struct v4l2_subdev *sd,
>> + struct v4l2_dv_timings_cap *cap)
>> +{
>> + *cap = tda1997x_dv_timings_cap;
>> + return 0;
>> +}
>> +
>> +static int tda1997x_enum_dv_timings(struct v4l2_subdev *sd,
>> + struct v4l2_enum_dv_timings *timings)
>> +{
>> + struct tda1997x_state *state = to_state(sd);
>> +
>> + return v4l2_enum_dv_timings_cap(timings, &tda1997x_dv_timings_cap,
>> + tda1997x_check_dv_timings, state);
>> +}
>> +
>> +static const struct v4l2_subdev_pad_ops tda1997x_pad_ops = {
>> + .enum_mbus_code = tda1997x_enum_mbus_code,
>> + .get_fmt = tda1997x_get_pad_format,
>> + .set_fmt = tda1997x_set_pad_format,
>> + .get_edid = tda1997x_get_edid,
>> + .set_edid = tda1997x_set_edid,
>> + .dv_timings_cap = tda1997x_get_dv_timings_cap,
>> + .enum_dv_timings = tda1997x_enum_dv_timings,
>> +};
>> +
>> +/* -----------------------------------------------------------------------------
>> + * v4l2_subdev_core_ops
>> + */
>> +
>> +static int tda1997x_log_status(struct v4l2_subdev *sd)
>> +{
>> + struct tda1997x_state *state = to_state(sd);
>> + const struct v4l2_dv_timings *timings = &state->timings;
>> +
>> + v4l2_info(sd, "-----Signal status-----\n");
>> + if (!timings) {
>
> timings can never be NULL.

oops - yes, this should be if (!state->std) to detect signal status

>
>> + v4l2_info(sd, "no signal\n");
>> + return 0;
>> + }
>> + v4l2_info(sd, "resolution: %dx%d%c@%dHz\n",
>> + timings->bt.width, timings->bt.height,
>> + timings->bt.interlaced ? 'i' : 'p',
>> + state->fps);
>> + v4l2_print_dv_timings(sd->name, "Detected format: ",
>> + timings, true);
>> + v4l2_info(sd, "colorspace: %d\n", state->colorspace);
>> + v4l2_info(sd, "colorimetry: %d\n", state->colorimetry);
>> + if (state->audio_channels)
>> + v4l2_info(sd, "audio: %dch %dHz\n", state->audio_channels,
>> + state->audio_samplerate);
>> + else
>> + v4l2_info(sd, "audio: none\n");
>> + v4l2_info(sd, "vendor: %s\n", state->vendor);
>> + v4l2_info(sd, "product: %s\n", state->product);
>
> If at all possible you should log the received InfoFrames here (hdmi_infoframe_log).
> Also whether an EDID is loaded or not and the HPD state.
>
> If the hardware supports 5V detection, then you should log that too. In that case
> also implement support for the V4L2_CID_DV_RX_POWER_PRESENT control.
>
> Also any information on the signal detection (clock lock, sync lock(s), whatever).
>
> This all helps enormously when debugging problems. It's important to spend some
> time on this function. The adv7604.c source might be a good place to look for
> inspiration.

ok - I will add what I can.

I can't figure out how to use log-status on a subdev either:

root@ventana:~# cat /sys/class/video4linux/v4l-subdev1/name
tda19971 2-0048
root@ventana:~# v4l2-ctl -d /dev/v4l-subdev1 --log-status
VIDIOC_QUERYCAP: failed: Inappropriate ioctl for device
/dev/v4l-subdev1: not a v4l2 node

>
>> +
>> + return 0;
>> +}
>> +
<snip>
>> +
>> +static int tda1997x_core_init(struct v4l2_subdev *sd)
>> +{
>> + struct tda1997x_state *state = to_state(sd);
>> + struct tda1997x_platform_data *pdata = &state->pdata;
>> + u8 reg;
>> + int i;
>> +
>> + /* disable HPD */
>> + io_write(sd, REG_HPD_AUTO_CTRL, HPD_AUTO_HPD_UNSEL);
>> + if (state->chip_revision == 0) {
>> + io_write(sd, REG_MAN_SUS_HDMI_SEL, MAN_DIS_HDCP | MAN_RST_HDCP);
>> + io_write(sd, REG_CGU_DBG_SEL, 1 << CGU_DBG_CLK_SEL_SHIFT);
>> + }
>> +
>> + /* reset infoframe at end of start-up-sequencer */
>> + io_write(sd, REG_SUS_SET_RGB2, 0x06);
>> + io_write(sd, REG_SUS_SET_RGB3, 0x06);
>> +
>> + /* Enable TMDS pull-ups */
>> + io_write(sd, REG_RT_MAN_CTRL, RT_MAN_CTRL_RT |
>> + RT_MAN_CTRL_RT_B | RT_MAN_CTRL_RT_A);
>> +
>> + /* enable sync measurement timing */
>> + tda1997x_cec_write(sd, REG_PWR_CONTROL & 0xff, 0x04);
>> + /* adjust CEC clock divider */
>> + tda1997x_cec_write(sd, REG_OSC_DIVIDER & 0xff, 0x03);
>> + tda1997x_cec_write(sd, REG_EN_OSC_PERIOD_LSB & 0xff, 0xa0);
>> + io_write(sd, REG_TIMER_D, 0x54);
>> + /* enable power switch */
>> + reg = tda1997x_cec_read(sd, REG_CONTROL & 0xff);
>> + reg |= 0x20;
>> + tda1997x_cec_write(sd, REG_CONTROL & 0xff, reg);
>> + mdelay(50);
>> +
>> + /* read the chip version */
>> + reg = io_read(sd, REG_VERSION);
>> + /* get the chip configuration */
>> + reg = io_read(sd, REG_CMTP_REG10);
>> +
>> + /* enable interrupts we care about */
>> + io_write(sd, REG_INT_MASK_TOP,
>> + INTERRUPT_HDCP | INTERRUPT_AUDIO | INTERRUPT_INFO |
>> + INTERRUPT_RATE | INTERRUPT_SUS);
>> + /* config_mtp,fmt,sus_end,sus_st */
>> + io_write(sd, REG_INT_MASK_SUS, MASK_MPT | MASK_FMT | MASK_SUS_END);
>> + /* rate stability change for inputs A/B */
>> + io_write(sd, REG_INT_MASK_RATE, MASK_RATE_B_ST | MASK_RATE_A_ST);
>> + /* aud,spd,avi*/
>> + io_write(sd, REG_INT_MASK_INFO,
>> + MASK_AUD_IF | MASK_SPD_IF | MASK_AVI_IF);
>> + /* audio_freq,audio_flg,mute_flg,fifo_err */
>> + io_write(sd, REG_INT_MASK_AUDIO,
>> + MASK_AUDIO_FREQ_FLG | MASK_AUDIO_FLG | MASK_MUTE_FLG |
>> + MASK_ERROR_FIFO_PT);
>> + /* HDCP C5 state reached */
>> + io_write(sd, REG_INT_MASK_HDCP, MASK_STATE_C5);
>> + /* don't care about AFE/DDC/MODE */
>> + io_write(sd, REG_INT_MASK_AFE, 0);
>> + io_write(sd, REG_INT_MASK_DDC, 0);
>> + io_write(sd, REG_INT_MASK_MODE, 0);
>> +
>> + /* clear all interrupts */
>> + io_write(sd, REG_INT_FLG_CLR_TOP, 0xff);
>> + io_write(sd, REG_INT_FLG_CLR_SUS, 0xff);
>> + io_write(sd, REG_INT_FLG_CLR_DDC, 0xff);
>> + io_write(sd, REG_INT_FLG_CLR_RATE, 0xff);
>> + io_write(sd, REG_INT_FLG_CLR_MODE, 0xff);
>> + io_write(sd, REG_INT_FLG_CLR_INFO, 0xff);
>> + io_write(sd, REG_INT_FLG_CLR_AUDIO, 0xff);
>> + io_write(sd, REG_INT_FLG_CLR_HDCP, 0xff);
>> + io_write(sd, REG_INT_FLG_CLR_AFE, 0xff);
>> +
>> + /* init TMDS equalizer */
>> + if (state->chip_revision == 0)
>> + io_write(sd, REG_CGU_DBG_SEL, 1 << CGU_DBG_CLK_SEL_SHIFT);
>> + io_write24(sd, REG_CLK_MIN_RATE, CLK_MIN_RATE);
>> + io_write24(sd, REG_CLK_MAX_RATE, CLK_MAX_RATE);
>> + if (state->chip_revision == 0)
>> + io_write(sd, REG_WDL_CFG, WDL_CFG_VAL);
>> + /* DC filter */
>> + io_write(sd, REG_DEEP_COLOR_CTRL, DC_FILTER_VAL);
>> + /* disable test pattern */
>> + io_write(sd, REG_SVC_MODE, 0x00);
>> + /* update HDMI INFO CTRL */
>> + io_write(sd, REG_INFO_CTRL, 0xff);
>> + /* write HDMI INFO EXCEED value */
>> + io_write(sd, REG_INFO_EXCEED, 3);
>> +
>> + if (state->chip_revision == 0)
>> + tda1997x_reset_n1(state);
>> +
>> + /*
>> + * No HDCP acknowledge when HDCP is disabled
>> + * and reset SUS to force format detection
>> + */
>> + tda1997x_hdmi_info_reset(sd, NACK_HDCP, true);
>> +
>> + /* Set HPD low */
>> + tda1997x_manual_hpd(sd, HPD_LOW_BP);
>> +
>> + /* Configure receiver capabilities */
>> + io_write(sd, REG_HDCP_BCAPS, HDCP_HDMI | HDCP_FAST_REAUTH);
>> +
>> + /* Configure HDMI: Auto HDCP mode, packet controlled mute */
>> + reg = HDMI_CTRL_MUTE_AUTO << HDMI_CTRL_MUTE_SHIFT;
>> + reg |= HDMI_CTRL_HDCP_AUTO << HDMI_CTRL_HDCP_SHIFT;
>> + io_write(sd, REG_HDMI_CTRL, reg);
>> +
>> + /* reset start-up-sequencer to force format detection */
>> + tda1997x_hdmi_info_reset(sd, 0, true);
>> +
>> + /* Set HPD high */
>> + tda1997x_manual_hpd(sd, HPD_HIGH_OTHER);
>> + tda1997x_manual_hpd(sd, HPD_HIGH_BP);
>
> How can you set the HPD high if there is no EDID? No EDID, no HPD.
>

right - I'll remove this

>> +
>> + /* disable matrix conversion */
>> + reg = io_read(sd, REG_VDP_CTRL);
>> + reg |= VDP_CTRL_MATRIX_BP;
>> + io_write(sd, REG_VDP_CTRL, reg);
>> +
<snip>
>> +
>> + ret = 0x34 + ((io_read(sd, REG_SLAVE_ADDR)>>4) & 0x03);
>> + state->client_cec = i2c_new_dummy(client->adapter, ret);
>> + v4l_info(client, "CEC slave address 0x%02x\n", ret);
>> +
>> + ret = tda1997x_core_init(sd);
>
> Unless I missed it, I don't think state->timings has been initialized
> to something valid. During probe the hdmi receiver has to be initialized
> to something. The API expects that. Usually VGA or 720p60 or 1080p60 is
> chosen for this.

you didn't miss it - I didn't know exactly what to do there.

I'll initialize it to VGA

>
>> + if (ret)
>> + goto err_free_mutex;
>> +
<snip>
>>
>
> Regards,
>
> Hans

Regarding video standard detection where this chip provides me with
vertical-period, horizontal-period, and horizontal-pulse-width I
should be able to detect the standard simply based off of
vertical-period (framerate) and horizontal-period (line width
including blanking) right? I wasn't sure if my method of matching
these within 14% tolerance made sense. I will be removing the hsmatch
logic from that as it seems the horizontal-pulse-width should be
irrelevant.

Thanks for the review!

Tim

Next message: Hans Verkuil: "Re: [PATCH 0/2] [media] rc/keymaps: add support for two RCs of hisilicon boards."
Previous message: Hans de Goede: "[PATCH v3 2/2] platform/x86: silead_dmi: Add silead,home-button property to some tablets"
In reply to: Hans Verkuil: "Re: [PATCH v2 3/4] media: i2c: Add TDA1997x HDMI receiver driver"
Next in thread: Hans Verkuil: "Re: [PATCH v2 3/4] media: i2c: Add TDA1997x HDMI receiver driver"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]