Re: [PATCH v9 3/4] [media] cxusb: add analog mode support for Medion MD95700

[Maciej S. Szmigiero]

On 24.04.2019 15:06, Hans Verkuil wrote:
> On 4/16/19 1:40 AM, Maciej S. Szmigiero wrote:
>> On 12.04.2019 11:22, Hans Verkuil wrote:
>>> On 3/30/19 12:51 AM, Maciej S. Szmigiero wrote:
>>>> This patch adds support for analog part of Medion 95700 in the cxusb
>>>> driver.
>>>>
>>>> What works:
>>>> * Video capture at various sizes with sequential fields,
>>>> * Input switching (TV Tuner, Composite, S-Video),
>>>> * TV and radio tuning,
>>>> * Video standard switching and auto detection,
>>>> * Radio mode switching (stereo / mono),
>>>> * Unplugging while capturing,
>>>> * DVB / analog coexistence.
>>>>
>>>> What does not work yet:
>>>> * Audio,
>>>> * VBI,
>>>> * Picture controls.
>>>>
>>>> Signed-off-by: Maciej S. Szmigiero <mail@xxxxxxxxxxxxxxxxxxxxx>
>>>> ---
(..)
>> In this case the code above simply tries first the bigger PAL capture
>> resolution then the smaller NTSC one.
>> Which one will be accepted by the cx25840 depends on the currently set
>> broadcast standard and parameters of the last signal that was received,
>> at least one of these resolutions seem to work even without any
>> signal being received since the chip was powered up.
>>
>> This way the API guarantees should be kept by the driver.
> 
> This is basically a work-around for a cx25840 bug.
> 
> In any case, since the driver initializes to PAL the 720x576 resolution
> should be fine.
> 
> BTW, I noticed that cxdev->norm is initialized to 0. Why isn't that
> PAL?

cxdev->norm is initialized to 0 since this will allow autodetection on
the default Composite input (Composite and S-Video inputs accept any
standard that a cx25840 chip can accept, including NTSC and SECAM).

The tuner and tda9887 are initialized to PAL since it is all they can
accept.

>>
>>>> +int cxusb_medion_analog_init(struct dvb_usb_device *dvbdev)
>>>> +{
>>>> +	struct cxusb_medion_dev *cxdev = dvbdev->priv;
>>>> +	u8 tuner_analog_msg_data[] = { 0x9c, 0x60, 0x85, 0x54 };
>>>> +	struct i2c_msg tuner_analog_msg = { .addr = 0x61, .flags = 0,
>>>> +					    .buf = tuner_analog_msg_data,
>>>> +					    .len =
>>>> +					    sizeof(tuner_analog_msg_data) };
>>>> +	struct v4l2_subdev_format subfmt;
>>>> +	int ret;
>>>> +
>>>> +	/* switch tuner to analog mode so IF demod will become accessible */
>>>> +	ret = i2c_transfer(&dvbdev->i2c_adap, &tuner_analog_msg, 1);
>>>> +	if (ret != 1)
>>>> +		dev_warn(&dvbdev->udev->dev,
>>>> +			 "tuner analog switch failed (%d)\n", ret);
>>>> +
>>>> +	/*
>>>> +	 * cx25840 might have lost power during mode switching so we need
>>>> +	 * to set it again
>>>> +	 */
>>>> +	ret = v4l2_subdev_call(cxdev->cx25840, core, reset, 0);
>>>> +	if (ret != 0)
>>>> +		dev_warn(&dvbdev->udev->dev,
>>>> +			 "cx25840 reset failed (%d)\n", ret);
>>>> +
>>>> +	ret = v4l2_subdev_call(cxdev->cx25840, video, s_routing,
>>>> +			       CX25840_COMPOSITE1, 0, 0);
>>>> +	if (ret != 0)
>>>> +		dev_warn(&dvbdev->udev->dev,
>>>> +			 "cx25840 initial input setting failed (%d)\n", ret);
>>>> +
>>>> +	/* composite */
>>>> +	cxdev->input = 1;
>>>> +	cxdev->norm = 0;
>>>> +
>>>> +	/* TODO: setup audio samples insertion */
>>>> +
>>>> +	ret = v4l2_subdev_call(cxdev->cx25840, core, s_io_pin_config,
>>>> +			       sizeof(cxusub_medion_pin_config) /
>>>> +			       sizeof(cxusub_medion_pin_config[0]),
>>>> +			       cxusub_medion_pin_config);
>>>> +	if (ret != 0)
>>>> +		dev_warn(&dvbdev->udev->dev,
>>>> +			"cx25840 pin config failed (%d)\n", ret);
>>>> +
>>>> +	/* make sure that we aren't in radio mode */
>>>> +	v4l2_subdev_call(cxdev->tda9887, video, s_std, V4L2_STD_PAL);
>>>> +	v4l2_subdev_call(cxdev->tuner, video, s_std, V4L2_STD_PAL);
>>>> +	v4l2_subdev_call(cxdev->cx25840, video, s_std, cxdev->norm);
>>>> +
>>>> +	memset(&subfmt, 0, sizeof(subfmt));
>>>> +	subfmt.which = V4L2_SUBDEV_FORMAT_ACTIVE;
>>>> +	subfmt.format.width = cxdev->width;
>>>> +	subfmt.format.height = cxdev->height;
>>>> +	subfmt.format.code = MEDIA_BUS_FMT_FIXED;
>>>> +	subfmt.format.field = V4L2_FIELD_SEQ_TB;
>>>> +	subfmt.format.colorspace = V4L2_COLORSPACE_SMPTE170M;
>>>> +
>>>> +	ret = v4l2_subdev_call(cxdev->cx25840, pad, set_fmt, NULL, &subfmt);
>>>> +	if (ret != 0) {
>>>> +		subfmt.format.width = 640;
>>>> +		subfmt.format.height = 480;
>>>
>>> Why the fallback to 640x480?
>>
>> This resolution seems to work even without any signal being received,
>> and it is a sensible NTSC-like resolution so it makes a good fallback
>> if restoring the previously used resolution has failed.
> 
> But it is really a work-around for a cx25840 bug. Just fix set_fmt
> and you should not need this anymore.
> 
> Regards.
> 
> 	Hans

Thanks,
Maciej