Re: [PATCH] media: adv7343: accept configuration through platform data

[Prabhakar Lad]

Hi Hans,

Thanks for the review!

On Mon, Jan 21, 2013 at 3:31 PM, Hans Verkuil <hverkuil@xxxxxxxxx> wrote:
> On Tue January 15 2013 09:00:53 Lad, Prabhakar wrote:
>> The current code was implemented with some default configurations,
>> this default configuration works on board and doesn't work on other.
>>
>> This patch accepts the configuration through platform data and configures
>> the encoder depending on the data set.
>
> Just one small comment...
>
>>
>> Signed-off-by: Lad, Prabhakar <prabhakar.lad@xxxxxx>
>> ---
>>  drivers/media/i2c/adv7343.c |   36 +++++++++++++++++++++++++++++++-----
>>  include/media/adv7343.h     |   32 ++++++++++++++++++++++++++++++++
>>  2 files changed, 63 insertions(+), 5 deletions(-)
>>
>> diff --git a/drivers/media/i2c/adv7343.c b/drivers/media/i2c/adv7343.c
>> index 2b5aa67..a058058 100644
>> --- a/drivers/media/i2c/adv7343.c
>> +++ b/drivers/media/i2c/adv7343.c
>> @@ -43,6 +43,7 @@ MODULE_PARM_DESC(debug, "Debug level 0-1");
>>  struct adv7343_state {
>>       struct v4l2_subdev sd;
>>       struct v4l2_ctrl_handler hdl;
>> +     const struct adv7343_platform_data *pdata;
>>       u8 reg00;
>>       u8 reg01;
>>       u8 reg02;
>> @@ -215,12 +216,23 @@ static int adv7343_setoutput(struct v4l2_subdev *sd, u32 output_type)
>>       /* Enable Appropriate DAC */
>>       val = state->reg00 & 0x03;
>>
>> -     if (output_type == ADV7343_COMPOSITE_ID)
>> -             val |= ADV7343_COMPOSITE_POWER_VALUE;
>> -     else if (output_type == ADV7343_COMPONENT_ID)
>> -             val |= ADV7343_COMPONENT_POWER_VALUE;
>> +     /* configure default configuration */
>> +     if (!state->pdata)
>> +             if (output_type == ADV7343_COMPOSITE_ID)
>> +                     val |= ADV7343_COMPOSITE_POWER_VALUE;
>> +             else if (output_type == ADV7343_COMPONENT_ID)
>> +                     val |= ADV7343_COMPONENT_POWER_VALUE;
>> +             else
>> +                     val |= ADV7343_SVIDEO_POWER_VALUE;
>>       else
>> -             val |= ADV7343_SVIDEO_POWER_VALUE;
>> +             val = state->pdata->mode_config.sleep_mode << 0 |
>> +                   state->pdata->mode_config.pll_control << 1 |
>> +                   state->pdata->mode_config.dac_3 << 2 |
>> +                   state->pdata->mode_config.dac_2 << 3 |
>> +                   state->pdata->mode_config.dac_1 << 4 |
>> +                   state->pdata->mode_config.dac_6 << 5 |
>> +                   state->pdata->mode_config.dac_5 << 6 |
>> +                   state->pdata->mode_config.dac_4 << 7;
>>
>>       err = adv7343_write(sd, ADV7343_POWER_MODE_REG, val);
>>       if (err < 0)
>> @@ -238,6 +250,17 @@ static int adv7343_setoutput(struct v4l2_subdev *sd, u32 output_type)
>>
>>       /* configure SD DAC Output 2 and SD DAC Output 1 bit to zero */
>>       val = state->reg82 & (SD_DAC_1_DI & SD_DAC_2_DI);
>> +
>> +     if (state->pdata && state->pdata->sd_config.sd_dac_out1)
>> +             val = val | (state->pdata->sd_config.sd_dac_out1 << 1);
>> +     else if (state->pdata && !state->pdata->sd_config.sd_dac_out1)
>> +             val = val & ~(state->pdata->sd_config.sd_dac_out1 << 1);
>> +
>> +     if (state->pdata && state->pdata->sd_config.sd_dac_out2)
>> +             val = val | (state->pdata->sd_config.sd_dac_out2 << 2);
>> +     else if (state->pdata && !state->pdata->sd_config.sd_dac_out2)
>> +             val = val & ~(state->pdata->sd_config.sd_dac_out2 << 2);
>> +
>>       err = adv7343_write(sd, ADV7343_SD_MODE_REG2, val);
>>       if (err < 0)
>>               goto setoutput_exit;
>> @@ -401,6 +424,9 @@ static int adv7343_probe(struct i2c_client *client,
>>       if (state == NULL)
>>               return -ENOMEM;
>>
>> +     /* Copy board specific information here */
>> +     state->pdata = client->dev.platform_data;
>> +
>>       state->reg00    = 0x80;
>>       state->reg01    = 0x00;
>>       state->reg02    = 0x20;
>> diff --git a/include/media/adv7343.h b/include/media/adv7343.h
>> index d6f8a4e..8086e46 100644
>> --- a/include/media/adv7343.h
>> +++ b/include/media/adv7343.h
>> @@ -20,4 +20,36 @@
>>  #define ADV7343_COMPONENT_ID (1)
>>  #define ADV7343_SVIDEO_ID    (2)
>>
>> +struct adv7343_power_mode {
>> +     bool sleep_mode;
>> +     bool pll_control;
>> +     bool dac_1;
>> +     bool dac_2;
>> +     bool dac_3;
>> +     bool dac_4;
>> +     bool dac_5;
>> +     bool dac_6;
>> +};
>
> Can you add a short description for struct adv7343_power_mode? It's
> sufficient to point to the relevant section in the datasheet (add a url
> or something like that).
>
Ok I'll do the needy. and respin v2.

Regards,
--Prabhakar

> Regards,
>
>         Hans
>
>> +
>> +/**
>> + * struct adv7343_sd_config - SD Only Output Configuration.
>> + * @sd_dac_out1: Configure SD DAC Output 1.
>> + * @sd_dac_out2: Configure SD DAC Output 2.
>> + */
>> +struct adv7343_sd_config {
>> +     /* SD only Output Configuration */
>> +     bool sd_dac_out1;
>> +     bool sd_dac_out2;
>> +};
>> +
>> +/**
>> + * struct adv7343_platform_data - Platform data values and access functions.
>> + * @mode_config: Configuration for power mode.
>> + * @sd_config: SD Only Configuration.
>> + */
>> +struct adv7343_platform_data {
>> +     struct adv7343_power_mode mode_config;
>> +     struct adv7343_sd_config sd_config;
>> +};
>> +
>>  #endif                               /* End of #ifndef ADV7343_H */
>>
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/