Message-ID: <b9ab4947-b306-3f9e-1951-a4fd62987f8f@maciej.szmigiero.name>
Date:   Tue, 10 Jul 2018 23:47:53 +0200
From:   "Maciej S. Szmigiero" <mail@...iej.szmigiero.name>
To:     Hans Verkuil <hverkuil@...all.nl>
Cc:     Michael Krufky <mkrufky@...uxtv.org>,
        Mauro Carvalho Chehab <mchehab@...nel.org>,
        Andy Walls <awalls@...metrocast.net>,
        linux-kernel <linux-kernel@...r.kernel.org>,
        linux-media@...r.kernel.org
Subject: Re: [PATCH v7 3/6] cx25840: add pin to pad mapping and output format
 configuration

Hi Hans,

On 04.07.2018 11:05, Hans Verkuil wrote:
> On 02/07/18 23:23, Maciej S. Szmigiero wrote:
(..)
>> @@ -316,6 +319,260 @@ static int cx23885_s_io_pin_config(struct v4l2_subdev *sd, size_t n,
>>  	return 0;
>>  }
>>  
>> +static u8 cx25840_function_to_pad(struct i2c_client *client, u8 function)
>> +{
>> +	switch (function) {
>> +	case CX25840_PAD_ACTIVE:
>> +		return 1;
>> +
>> +	case CX25840_PAD_VACTIVE:
>> +		return 2;
>> +
>> +	case CX25840_PAD_CBFLAG:
>> +		return 3;
>> +
>> +	case CX25840_PAD_VID_DATA_EXT0:
>> +		return 4;
>> +
>> +	case CX25840_PAD_VID_DATA_EXT1:
>> +		return 5;
>> +
>> +	case CX25840_PAD_GPO0:
>> +		return 6;
>> +
>> +	case CX25840_PAD_GPO1:
>> +		return 7;
>> +
>> +	case CX25840_PAD_GPO2:
>> +		return 8;
>> +
>> +	case CX25840_PAD_GPO3:
>> +		return 9;
>> +
>> +	case CX25840_PAD_IRQ_N:
>> +		return 10;
>> +
>> +	case CX25840_PAD_AC_SYNC:
>> +		return 11;
>> +
>> +	case CX25840_PAD_AC_SDOUT:
>> +		return 12;
>> +
>> +	case CX25840_PAD_PLL_CLK:
>> +		return 13;
>> +
>> +	case CX25840_PAD_VRESET:
>> +		return 14;
>> +
>> +	default:
>> +		if (function != CX25840_PAD_DEFAULT)
>> +			v4l_err(client,
>> +				"invalid function %u, assuming default\n",
>> +				(unsigned int)function);
>> +		return 0;
>> +	}
> 
> Unless I am mistaken this function boils down to:
> 
> static u8 cx25840_function_to_pad(struct i2c_client *client, u8 function)
> {
> 	return function > CX25840_PAD_VRESET ? 0 : function;
> }

Yes, you are right these functions are equivalent (sans a warning when
a caller passes an invalid function).

However, these values (CX25840_PAD_*) were meant to be driver-internal.
If we use them also as register value constants (which is what
cx25840_function_to_pad() is supposed to return) then we'll need to add
a comment to their enum cx25840_io_pad so nobody shuffles them or changes
their values by mistake.

>> @@ -1647,6 +1924,119 @@ static void log_audio_status(struct i2c_client *client)
>>  	}
>>  }
>>  
>> +#define CX25840_VCONFIG_OPTION(state, cfg_in, opt_msk)			\
>> +	do {								\
>> +		if ((cfg_in) & (opt_msk)) {				\
>> +			(state)->vid_config &= ~(opt_msk);		\
>> +			(state)->vid_config |= (cfg_in) & (opt_msk);	\
>> +		}							\
>> +	} while (0)
>> +
>> +#define CX25840_VCONFIG_SET_BIT(state, opt_msk, voc, idx, bit, oneval)	\
>> +	do {								\
>> +		if ((state)->vid_config & (opt_msk)) {			\
>> +			if (((state)->vid_config & (opt_msk)) ==	\
>> +			    (oneval))					\
>> +				(voc)[idx] |= BIT(bit);		\
>> +			else						\
>> +				(voc)[idx] &= ~BIT(bit);		\
>> +		}							\
>> +	} while (0)
>> +
>> +int cx25840_vconfig(struct i2c_client *client, u32 cfg_in)
>> +{
>> +	struct cx25840_state *state = to_state(i2c_get_clientdata(client));
>> +	u8 voutctrl[3];
>> +	unsigned int i;
>> +
>> +	/* apply incoming options to the current state */
>> +	CX25840_VCONFIG_OPTION(state, cfg_in, CX25840_VCONFIG_FMT_MASK);
>> +	CX25840_VCONFIG_OPTION(state, cfg_in, CX25840_VCONFIG_RES_MASK);
>> +	CX25840_VCONFIG_OPTION(state, cfg_in, CX25840_VCONFIG_VBIRAW_MASK);
>> +	CX25840_VCONFIG_OPTION(state, cfg_in, CX25840_VCONFIG_ANCDATA_MASK);
>> +	CX25840_VCONFIG_OPTION(state, cfg_in, CX25840_VCONFIG_TASKBIT_MASK);
>> +	CX25840_VCONFIG_OPTION(state, cfg_in, CX25840_VCONFIG_ACTIVE_MASK);
>> +	CX25840_VCONFIG_OPTION(state, cfg_in, CX25840_VCONFIG_VALID_MASK);
>> +	CX25840_VCONFIG_OPTION(state, cfg_in, CX25840_VCONFIG_HRESETW_MASK);
>> +	CX25840_VCONFIG_OPTION(state, cfg_in, CX25840_VCONFIG_CLKGATE_MASK);
>> +	CX25840_VCONFIG_OPTION(state, cfg_in, CX25840_VCONFIG_DCMODE_MASK);
>> +	CX25840_VCONFIG_OPTION(state, cfg_in, CX25840_VCONFIG_IDID0S_MASK);
>> +	CX25840_VCONFIG_OPTION(state, cfg_in, CX25840_VCONFIG_VIPCLAMP_MASK);
> 
> This appears to be a very complex way of saying:
> 
> 	state->vid_config = cfg_in;

This is supposed to change in vid_config only these options that are set
in the incoming cfg_in, leaving the rest as-is.

If the code simply assigns cfg_in to vid_config it will also clear all the
existing options.

> 
>> +
>> +	for (i = 0; i < 3; i++)
>> +		voutctrl[i] = cx25840_read(client, 0x404 + i);
>> +
>> +	/* apply state to hardware regs */
>> +	if (state->vid_config & CX25840_VCONFIG_FMT_MASK)
>> +		voutctrl[0] &= ~3;
>> +	switch (state->vid_config & CX25840_VCONFIG_FMT_MASK) {
>> +	case CX25840_VCONFIG_FMT_BT656:
>> +		voutctrl[0] |= 1;
>> +		break;
>> +
>> +	case CX25840_VCONFIG_FMT_VIP11:
>> +		voutctrl[0] |= 2;
>> +		break;
>> +
>> +	case CX25840_VCONFIG_FMT_VIP2:
>> +		voutctrl[0] |= 3;
>> +		break;
>> +
>> +	case CX25840_VCONFIG_FMT_BT601:
>> +		/* zero */
>> +	default:
>> +		break;
>> +	}
>> +
>> +	CX25840_VCONFIG_SET_BIT(state, CX25840_VCONFIG_RES_MASK, voutctrl,
>> +				0, 2, CX25840_VCONFIG_RES_10BIT);
>> +	CX25840_VCONFIG_SET_BIT(state, CX25840_VCONFIG_VBIRAW_MASK, voutctrl,
>> +				0, 3, CX25840_VCONFIG_VBIRAW_ENABLED);
>> +	CX25840_VCONFIG_SET_BIT(state, CX25840_VCONFIG_ANCDATA_MASK, voutctrl,
>> +				0, 4, CX25840_VCONFIG_ANCDATA_ENABLED);
>> +	CX25840_VCONFIG_SET_BIT(state, CX25840_VCONFIG_TASKBIT_MASK, voutctrl,
>> +				0, 5, CX25840_VCONFIG_TASKBIT_ONE);
>> +	CX25840_VCONFIG_SET_BIT(state, CX25840_VCONFIG_ACTIVE_MASK, voutctrl,
>> +				1, 2, CX25840_VCONFIG_ACTIVE_HORIZONTAL);
>> +	CX25840_VCONFIG_SET_BIT(state, CX25840_VCONFIG_VALID_MASK, voutctrl,
>> +				1, 3, CX25840_VCONFIG_VALID_ANDACTIVE);
>> +	CX25840_VCONFIG_SET_BIT(state, CX25840_VCONFIG_HRESETW_MASK, voutctrl,
>> +				1, 4, CX25840_VCONFIG_HRESETW_PIXCLK);
>> +
>> +	if (state->vid_config & CX25840_VCONFIG_CLKGATE_MASK)
>> +		voutctrl[1] &= ~(3 << 6);
>> +	switch (state->vid_config & CX25840_VCONFIG_CLKGATE_MASK) {
>> +	case CX25840_VCONFIG_CLKGATE_VALID:
>> +		voutctrl[1] |= 2;
>> +		break;
>> +
>> +	case CX25840_VCONFIG_CLKGATE_VALIDACTIVE:
>> +		voutctrl[1] |= 3;
>> +		break;
>> +
>> +	case CX25840_VCONFIG_CLKGATE_NONE:
>> +		/* zero */
>> +	default:
>> +		break;
>> +	}
>> +
>> +
>> +	CX25840_VCONFIG_SET_BIT(state, CX25840_VCONFIG_DCMODE_MASK, voutctrl,
>> +				2, 0, CX25840_VCONFIG_DCMODE_BYTES);
>> +	CX25840_VCONFIG_SET_BIT(state, CX25840_VCONFIG_IDID0S_MASK, voutctrl,
>> +				2, 1, CX25840_VCONFIG_IDID0S_LINECNT);
>> +	CX25840_VCONFIG_SET_BIT(state, CX25840_VCONFIG_VIPCLAMP_MASK, voutctrl,
>> +				2, 4, CX25840_VCONFIG_VIPCLAMP_ENABLED);
>> +
>> +	for (i = 0; i < 3; i++)
>> +		cx25840_write(client, 0x404 + i, voutctrl[i]);
>> +
>> +	return 0;
>> +}
>> +
>> +#undef CX25840_VCONFIG_SET_BIT
>> +#undef CX25840_VCONFIG_OPTION
> 
> Why #undef? You would normally never do that.

The idea here is to catch (unintended) other users of these macros, other
than cx25840_vconfig() and to not pollute the macro namespace.
But these #undefs can be removed if they are against the coding style.

> 
>> +
>>  /* ----------------------------------------------------------------------- */
>>  
>>  /* This load_fw operation must be called to load the driver's firmware.
>> @@ -1836,6 +2226,9 @@ static int cx25840_s_video_routing(struct v4l2_subdev *sd,
>>  	if (is_cx23888(state))
>>  		cx23888_std_setup(client);
>>  
>> +	if (is_cx2584x(state) && state->generic_mode)
>> +		cx25840_vconfig(client, config);
>> +
> 
> You do the vconfig configuration when the video routing changes. But isn't this
> configuration a one-time thing? E.g. something you do only when initializing the
> board?
> 
> At least in the cxusb code the cfg_in value is constant and not dependent on what
> input is chosen.
> 
> If this is true, then you should add the core init op instead. And as a bonus you
> can turn on generic_mode if the init op is called instead of having to add it
> to the platform data.
> 

The problem here is that the Medion MD95700 has two modes:
digital (DVB-T) and analog.
When it is operating in the digital mode the device analog components
(including the cx25840 chip) have their power cut by the hardware.

This means that the cx25840 has to be fully reinitialized (firmware
loaded, format set, etc.) every time the user opens the Medion's v4l2
video or radio device node if the device has previously operated in the
DVB-T mode.
Mode switching is supported transparently by the driver without needing
to reload the module or reconnect the device.

> 
> Regards,
> 
> 	Hans
> 

Thanks and best regards,
Maciej

Powered by blists - more mailing lists