Hi Tim,
We're almost there. Two more comments:
On 02/09/2018 07:32 AM, Tim Harvey wrote:
> +static int
> +tda1997x_detect_std(struct tda1997x_state *state,
> + struct v4l2_dv_timings *timings)
> +{
> + struct v4l2_subdev *sd = &state->sd;
> + u32 vper;
> + u16 hper;
> + u16 hsper;
> + int i;
> +
> + /*
> + * Read the FMT registers
> + * REG_V_PER: Period of a frame (or two fields) in MCLK(27MHz) cycles
> + * REG_H_PER: Period of a line in MCLK(27MHz) cycles
> + * REG_HS_WIDTH: Period of horiz sync pulse in MCLK(27MHz) cycles
> + */
> + vper = io_read24(sd, REG_V_PER) & MASK_VPER;
> + hper = io_read16(sd, REG_H_PER) & MASK_HPER;
> + hsper = io_read16(sd, REG_HS_WIDTH) & MASK_HSWIDTH;
> + if (!vper || !hper || !hsper)
> + return -ENOLINK;
See my comment for g_input_status below. This condition looks more like a
ENOLCK.
Or perhaps it should be:
if (!vper && !hper && !hsper)
return -ENOLINK;
if (!vper || !hper || !hsper)
return -ENOLCK;
I would recommend that you test a bit with no signal and a bad signal (perhaps
one that uses a pixelclock that is too high for this device?).
> + v4l2_dbg(1, debug, sd, "Signal Timings: %u/%u/%u\n", vper, hper, hsper);
> +
> + for (i = 0; v4l2_dv_timings_presets[i].bt.width; i++) {
> + const struct v4l2_bt_timings *bt;
> + u32 lines, width, _hper, _hsper;
> + u32 vmin, vmax, hmin, hmax, hsmin, hsmax;
> + bool vmatch, hmatch, hsmatch;
> +
> + bt = &v4l2_dv_timings_presets[i].bt;
> + width = V4L2_DV_BT_FRAME_WIDTH(bt);
> + lines = V4L2_DV_BT_FRAME_HEIGHT(bt);
> + _hper = (u32)bt->pixelclock / width;
> + if (bt->interlaced)
> + lines /= 2;
> + /* vper +/- 0.7% */
> + vmin = ((27000000 / 1000) * 993) / _hper * lines;
> + vmax = ((27000000 / 1000) * 1007) / _hper * lines;
> + /* hper +/- 1.0% */
> + hmin = ((27000000 / 100) * 99) / _hper;
> + hmax = ((27000000 / 100) * 101) / _hper;
> + /* hsper +/- 2 (take care to avoid 32bit overflow) */
> + _hsper = 27000 * bt->hsync / ((u32)bt->pixelclock/1000);
> + hsmin = _hsper - 2;
> + hsmax = _hsper + 2;
> +
> + /* vmatch matches the framerate */
> + vmatch = ((vper <= vmax) && (vper >= vmin)) ? 1 : 0;
> + /* hmatch matches the width */
> + hmatch = ((hper <= hmax) && (hper >= hmin)) ? 1 : 0;
> + /* hsmatch matches the hswidth */
> + hsmatch = ((hsper <= hsmax) && (hsper >= hsmin)) ? 1 : 0;
> + if (hmatch && vmatch && hsmatch) {
> + *timings = v4l2_dv_timings_presets[i];
> + v4l2_print_dv_timings(sd->name, "Detected format: ",
> + timings, false);
> + return 0;
> + }
> + }
> +
> + v4l_err(state->client, "no resolution match for timings: %d/%d/%d\n",
> + vper, hper, hsper);
> + return -EINVAL;
> +}
-EINVAL isn't the correct error code here. I would go for -ERANGE. It's not
perfect, but close enough.
-EINVAL indicates that the user filled in wrong values, but that's not the
case here.
> +static int
> +tda1997x_g_input_status(struct v4l2_subdev *sd, u32 *status)
> +{
> + struct tda1997x_state *state = to_state(sd);
> + u32 vper;
> + u16 hper;
> + u16 hsper;
> +
> + mutex_lock(&state->lock);
> + v4l2_dbg(1, debug, sd, "inputs:%d/%d\n",
> + state->input_detect[0], state->input_detect[1]);
> + if (state->input_detect[0] || state->input_detect[1])
I'm confused. This device has two HDMI inputs?
Does 'detecting input' equate to 'I see a signal and I am locked'?
I gather from the irq function that sets these values that it is closer
to 'I see a signal' and that 'I am locked' is something you would test
by looking at the vper/hper/hsper.
> + *status = 0;
> + else {
> + vper = io_read24(sd, REG_V_PER) & MASK_VPER;
> + hper = io_read16(sd, REG_H_PER) & MASK_HPER;
> + hsper = io_read16(sd, REG_HS_WIDTH) & MASK_HSWIDTH;
> + v4l2_dbg(1, debug, sd, "timings:%d/%d/%d\n", vper, hper, hsper);
> + if (!vper || !hper || !hsper)
> + *status |= V4L2_IN_ST_NO_SYNC;
> + else
> + *status |= V4L2_IN_ST_NO_SIGNAL;
So if we have valid vper, hper and hsper, then there is no signal? That doesn't
make sense.
I'd expect to see something like this:
if (!input_detect[0] && !input_detect[1])
// no signal
else if (!vper || !hper || !vsper)
// no sync
else
// have signal and sync
I'm not sure about the precise meaning of input_detect, so I might be wrong about
that bit.
Regards,
Hans
