Re: [PATCH v2 1/6] cx25840: add pin to pad mapping and output format configuration

[Maciej S. Szmigiero]

On 11.12.2017 16:27, Mauro Carvalho Chehab wrote:
> Em Tue, 10 Oct 2017 23:34:45 +0200
> "Maciej S. Szmigiero" <mail@xxxxxxxxxxxxxxxxxxxxx> escreveu:
> 
>> This commit adds pin to pad mapping and output format configuration support
>> in CX2584x-series chips to cx25840 driver.
>>
>> This functionality is then used to allow disabling ivtv-specific hacks
>> (called a "generic mode"), so cx25840 driver can be used for other devices
>> not needing them without risking compatibility problems.
>>
>> Signed-off-by: Maciej S. Szmigiero <mail@xxxxxxxxxxxxxxxxxxxxx>
>> ---
>>  drivers/media/i2c/cx25840/cx25840-core.c | 394 ++++++++++++++++++++++++++++++-
>>  drivers/media/i2c/cx25840/cx25840-core.h |  11 +
>>  drivers/media/i2c/cx25840/cx25840-vbi.c  |   3 +
>>  drivers/media/pci/ivtv/ivtv-i2c.c        |   1 +
>>  include/media/drv-intf/cx25840.h         |  74 +++++-
>>  5 files changed, 481 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/media/i2c/cx25840/cx25840-core.c b/drivers/media/i2c/cx25840/cx25840-core.c
>> index f38bf819d805..a1efc975852c 100644
>> --- a/drivers/media/i2c/cx25840/cx25840-core.c
>> +++ b/drivers/media/i2c/cx25840/cx25840-core.c
(..)
>> @@ -1630,6 +1907,117 @@ static void log_audio_status(struct i2c_client *client)
>>  	}
>>  }
>>  
>> +#define CX25840_VCONFIG_OPTION(option_mask)				\
>> +	do {								\
>> +		if (config_in & (option_mask)) {			\
>> +			state->vid_config &= ~(option_mask);		\
>> +			state->vid_config |= config_in & (option_mask); \
> 
> Don't do that: the "config_in" var is not at the macro's parameter.
> It only works if this macro is called at cx25840_vconfig() function.
> The same applies to state. That makes harder for anyone reviewing the
> code to understand it. Also, makes harder to use it on any other place.
> 
> If you want to use a macro, please add all required vars to the macro
> parameters.
> 
>> +		}							\
>> +	} while (0)
>> +
>> +#define CX25840_VCONFIG_SET_BIT(optionmask, reg, bit, oneval)		\
>> +	do {								\
>> +		if (state->vid_config & (optionmask)) {		\
>> +			if ((state->vid_config & (optionmask)) ==	\
>> +			    (oneval))					\
>> +				voutctrl[reg] |= BIT(bit);		\
>> +			else						\
>> +				voutctrl[reg] &= ~BIT(bit);		\
>> +		}							\
>> +	} while (0)
> 
> Same applies here: state and voutctrl aren't at macro's parameters.
> 
>> +
>> +int cx25840_vconfig(struct i2c_client *client, u32 config_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(CX25840_VCONFIG_FMT_MASK);
>> +	CX25840_VCONFIG_OPTION(CX25840_VCONFIG_RES_MASK);
>> +	CX25840_VCONFIG_OPTION(CX25840_VCONFIG_VBIRAW_MASK);
>> +	CX25840_VCONFIG_OPTION(CX25840_VCONFIG_ANCDATA_MASK);
>> +	CX25840_VCONFIG_OPTION(CX25840_VCONFIG_TASKBIT_MASK);
>> +	CX25840_VCONFIG_OPTION(CX25840_VCONFIG_ACTIVE_MASK);
>> +	CX25840_VCONFIG_OPTION(CX25840_VCONFIG_VALID_MASK);
>> +	CX25840_VCONFIG_OPTION(CX25840_VCONFIG_HRESETW_MASK);
>> +	CX25840_VCONFIG_OPTION(CX25840_VCONFIG_CLKGATE_MASK);
>> +	CX25840_VCONFIG_OPTION(CX25840_VCONFIG_DCMODE_MASK);
>> +	CX25840_VCONFIG_OPTION(CX25840_VCONFIG_IDID0S_MASK);
>> +	CX25840_VCONFIG_OPTION(CX25840_VCONFIG_VIPCLAMP_MASK);
> 
> The entire logic here sounds complex, without need. Wouldn't be
> better/clearer if you rewrite it as:
> 
> 	u32 option_mask = CX25840_VCONFIG_FMT_MASK
> 	       | CX25840_VCONFIG_RES_MASK
> ...
> 	       | CX25840_VCONFIG_VIPCLAMP_MASK;
> 
> 	state->vid_config &= ~option_mask;
> 	state->vid_config |= config_in & option_mask;
> 
> 

Unfortunately, this would zero the current configuration in
state->vid_config for every possible parameter, whereas the macros above
only touch these parameters that are provided to a cx25840_vconfig()
invocation, leaving the rest as-is.

(All other your comments were implemented in a respin).

> 
> Thanks,
> Mauro
> 

Thanks,
Maciej