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
