Re: [PATCH 36/47] adv7604: Make output format configurable through pad format operations
Hi Laurent,
On 03/11/14 12:16, Laurent Pinchart wrote:
> Hi Hans,
>
> On Tuesday 11 March 2014 10:05:00 Hans Verkuil wrote:
>> On 03/10/14 23:43, Laurent Pinchart wrote:
>>> On Wednesday 12 February 2014 16:01:17 Hans Verkuil wrote:
On 02/05/14 17:42, Laurent Pinchart wrote:
> Replace the dummy video format operations by pad format operations that
> configure the output format.
>
> Signed-off-by: Laurent Pinchart
> ---
>
> drivers/media/i2c/adv7604.c | 243 -
> include/media/adv7604.h | 47 ++---
> 2 files changed, 225 insertions(+), 65 deletions(-)
> diff --git a/include/media/adv7604.h b/include/media/adv7604.h
> index 22811d3..2cc8e16 100644
> --- a/include/media/adv7604.h
> +++ b/include/media/adv7604.h
> @@ -32,16 +32,6 @@ enum adv7604_ain_sel {
> ADV7604_AIN9_4_5_6_SYNC_2_1 = 4,
> };
>
> -/* Bus rotation and reordering (IO register 0x04, [7:5]) */
> -enum adv7604_op_ch_sel {
> - ADV7604_OP_CH_SEL_GBR = 0,
> - ADV7604_OP_CH_SEL_GRB = 1,
> - ADV7604_OP_CH_SEL_BGR = 2,
> - ADV7604_OP_CH_SEL_RGB = 3,
> - ADV7604_OP_CH_SEL_BRG = 4,
> - ADV7604_OP_CH_SEL_RBG = 5,
> -};
> -
> /* Input Color Space (IO register 0x02, [7:4]) */
> enum adv7604_inp_color_space {
> ADV7604_INP_COLOR_SPACE_LIM_RGB = 0,
> @@ -55,29 +45,11 @@ enum adv7604_inp_color_space {
>
> [snip]
>
> @@ -104,11 +76,8 @@ struct adv7604_platform_data {
>
> /* Analog input muxing mode */
> enum adv7604_ain_sel ain_sel;
>
> - /* Bus rotation and reordering */
> - enum adv7604_op_ch_sel op_ch_sel;
I would keep this as part of the platform_data. This is typically used if
things are wired up in a non-standard way and so is specific to the
hardware. It is not something that will change from format to format.
>>>
>>> Right, some level of configuration is needed to account for non-standard
>>> wiring. However I'm not sure where that should be handled.
>>>
>>> With exotic wiring the format at the receiver will be different than the
>>> format output by the ADV7604. From a pure ADV7604 point of view, the
>>> output format doesn't depend on the wiring. I wonder whether this
>>> shouldn't be a link property instead of being a subdev property. There's
>>> of course the question of where to store that link property if it's not
>>> part of either subdev.
>>>
>>> Even if we decide that the wiring is a property of the source subdev, I
>>> don't think we should duplicate bus reordering code in all subdev
>>> drivers. This should thus be handled by the v4l2 core (either directly or
>>> as helper functions).
>>
>> There are two reasons why you might want to use op_ch_sel: one is to
>> implement weird formats like RBG. Something like that would have to be
>> controlled through mbus and pixel fourcc codes and not by hardcoding this
>> register.
>
> Agreed. This patch only adds a subset of the possible output formats. More
> formats can be added later as needed.
>
>> The other is to compensate for a wiring problem: we have a card where two
>> channels were accidentally swapped. You can either redo the board or just
>> set this register. In this case this register is IMHO a property of this
>> subdev. It needs to know about it, because if it ever needs to output RBG
>> in the future then it needs to compensate for reordering for wiring
>> issues.
>>
>> So you set this field if you have to compensate for wiring errors, making
>> this part of the DT/platform_data. You do not set this field when you
>> want to support special formats, that is done in the driver itself through
>> fourcc codes (or could be done as this isn't implemented at the moment).
>
> I agree with the use case, but I'm not sure whether that's the best way to
> support it. Let's say the system is design for RGB, but the G and B channels
> have been swapped by mistake on the board. To make it work, the ADV7604 would
> need to output RBG, and the bridge would receive RGB. Data reordering would
> thus happen on the link.
>
> There's two ways to expose this to userspace. One of them would be to make
> the
> real formats visible to applications. We would need to extend the link API to
> show that reordering occurs. Userspace would need to explicitly configure the
> adv7604 driver with RBG at its source pad and the bridge driver with RGB at
> its sink pad. This has the advantage of correctly modeling the hardware, and
> not pushing workarounds for board-level issues to individual drivers.
>
> Another solution would be to hide the wiring problem from userspace and
> handle
> it inside the adv7604 driver. Note that, depending on the hardware
> configuration, it could be the bridge driver that need to implement
> reordering. Userspace would configure both ends of the link to RGB and
> wouldn't be aware of the problem. This has the advantage o
Re: [PATCH 36/47] adv7604: Make output format configurable through pad format operations
Hi Hans,
On Tuesday 11 March 2014 10:05:00 Hans Verkuil wrote:
> On 03/10/14 23:43, Laurent Pinchart wrote:
> > On Wednesday 12 February 2014 16:01:17 Hans Verkuil wrote:
> >> On 02/05/14 17:42, Laurent Pinchart wrote:
> >>> Replace the dummy video format operations by pad format operations that
> >>> configure the output format.
> >>>
> >>> Signed-off-by: Laurent Pinchart
> >>> ---
> >>>
> >>> drivers/media/i2c/adv7604.c | 243 -
> >>> include/media/adv7604.h | 47 ++---
> >>> 2 files changed, 225 insertions(+), 65 deletions(-)
> >>
> >>
> >>
> >>> diff --git a/include/media/adv7604.h b/include/media/adv7604.h
> >>> index 22811d3..2cc8e16 100644
> >>> --- a/include/media/adv7604.h
> >>> +++ b/include/media/adv7604.h
> >>> @@ -32,16 +32,6 @@ enum adv7604_ain_sel {
> >>> ADV7604_AIN9_4_5_6_SYNC_2_1 = 4,
> >>> };
> >>>
> >>> -/* Bus rotation and reordering (IO register 0x04, [7:5]) */
> >>> -enum adv7604_op_ch_sel {
> >>> - ADV7604_OP_CH_SEL_GBR = 0,
> >>> - ADV7604_OP_CH_SEL_GRB = 1,
> >>> - ADV7604_OP_CH_SEL_BGR = 2,
> >>> - ADV7604_OP_CH_SEL_RGB = 3,
> >>> - ADV7604_OP_CH_SEL_BRG = 4,
> >>> - ADV7604_OP_CH_SEL_RBG = 5,
> >>> -};
> >>> -
> >>> /* Input Color Space (IO register 0x02, [7:4]) */
> >>> enum adv7604_inp_color_space {
> >>> ADV7604_INP_COLOR_SPACE_LIM_RGB = 0,
> >>> @@ -55,29 +45,11 @@ enum adv7604_inp_color_space {
[snip]
> >>> @@ -104,11 +76,8 @@ struct adv7604_platform_data {
> >>>
> >>> /* Analog input muxing mode */
> >>> enum adv7604_ain_sel ain_sel;
> >>>
> >>> - /* Bus rotation and reordering */
> >>> - enum adv7604_op_ch_sel op_ch_sel;
> >>
> >> I would keep this as part of the platform_data. This is typically used if
> >> things are wired up in a non-standard way and so is specific to the
> >> hardware. It is not something that will change from format to format.
> >
> > Right, some level of configuration is needed to account for non-standard
> > wiring. However I'm not sure where that should be handled.
> >
> > With exotic wiring the format at the receiver will be different than the
> > format output by the ADV7604. From a pure ADV7604 point of view, the
> > output format doesn't depend on the wiring. I wonder whether this
> > shouldn't be a link property instead of being a subdev property. There's
> > of course the question of where to store that link property if it's not
> > part of either subdev.
> >
> > Even if we decide that the wiring is a property of the source subdev, I
> > don't think we should duplicate bus reordering code in all subdev
> > drivers. This should thus be handled by the v4l2 core (either directly or
> > as helper functions).
>
> There are two reasons why you might want to use op_ch_sel: one is to
> implement weird formats like RBG. Something like that would have to be
> controlled through mbus and pixel fourcc codes and not by hardcoding this
> register.
Agreed. This patch only adds a subset of the possible output formats. More
formats can be added later as needed.
> The other is to compensate for a wiring problem: we have a card where two
> channels were accidentally swapped. You can either redo the board or just
> set this register. In this case this register is IMHO a property of this
> subdev. It needs to know about it, because if it ever needs to output RBG
> in the future then it needs to compensate for reordering for wiring
> issues.
>
> So you set this field if you have to compensate for wiring errors, making
> this part of the DT/platform_data. You do not set this field when you
> want to support special formats, that is done in the driver itself through
> fourcc codes (or could be done as this isn't implemented at the moment).
I agree with the use case, but I'm not sure whether that's the best way to
support it. Let's say the system is design for RGB, but the G and B channels
have been swapped by mistake on the board. To make it work, the ADV7604 would
need to output RBG, and the bridge would receive RGB. Data reordering would
thus happen on the link.
There's two ways to expose this to userspace. One of them would be to make the
real formats visible to applications. We would need to extend the link API to
show that reordering occurs. Userspace would need to explicitly configure the
adv7604 driver with RBG at its source pad and the bridge driver with RGB at
its sink pad. This has the advantage of correctly modeling the hardware, and
not pushing workarounds for board-level issues to individual drivers.
Another solution would be to hide the wiring problem from userspace and handle
it inside the adv7604 driver. Note that, depending on the hardware
configuration, it could be the bridge driver that need to implement
reordering. Userspace would configure both ends of the link to RGB and
wouldn't be aware of the problem. This has the advantage of hiding the
problem in the kernel. However, it would require implementing the same
workaround in potentially many drivers
Re: [PATCH 36/47] adv7604: Make output format configurable through pad format operations
On 03/10/14 23:43, Laurent Pinchart wrote:
> Hi Hans,
>
> On Wednesday 12 February 2014 16:01:17 Hans Verkuil wrote:
>> On 02/05/14 17:42, Laurent Pinchart wrote:
>>> Replace the dummy video format operations by pad format operations that
>>> configure the output format.
>>>
>>> Signed-off-by: Laurent Pinchart
>>> ---
>>>
>>> drivers/media/i2c/adv7604.c | 243 ++-
>>> include/media/adv7604.h | 47 ++---
>>> 2 files changed, 225 insertions(+), 65 deletions(-)
>>
>>
>>
>>> diff --git a/include/media/adv7604.h b/include/media/adv7604.h
>>> index 22811d3..2cc8e16 100644
>>> --- a/include/media/adv7604.h
>>> +++ b/include/media/adv7604.h
>>> @@ -32,16 +32,6 @@ enum adv7604_ain_sel {
>>>
>>> ADV7604_AIN9_4_5_6_SYNC_2_1 = 4,
>>>
>>> };
>>>
>>> -/* Bus rotation and reordering (IO register 0x04, [7:5]) */
>>> -enum adv7604_op_ch_sel {
>>> - ADV7604_OP_CH_SEL_GBR = 0,
>>> - ADV7604_OP_CH_SEL_GRB = 1,
>>> - ADV7604_OP_CH_SEL_BGR = 2,
>>> - ADV7604_OP_CH_SEL_RGB = 3,
>>> - ADV7604_OP_CH_SEL_BRG = 4,
>>> - ADV7604_OP_CH_SEL_RBG = 5,
>>> -};
>>> -
>>> /* Input Color Space (IO register 0x02, [7:4]) */
>>> enum adv7604_inp_color_space {
>>> ADV7604_INP_COLOR_SPACE_LIM_RGB = 0,
>>> @@ -55,29 +45,11 @@ enum adv7604_inp_color_space {
>>> ADV7604_INP_COLOR_SPACE_AUTO = 0xf,
>>> };
>>>
>>> -/* Select output format (IO register 0x03, [7:0]) */
>>> -enum adv7604_op_format_sel {
>>> - ADV7604_OP_FORMAT_SEL_SDR_ITU656_8 = 0x00,
>>> - ADV7604_OP_FORMAT_SEL_SDR_ITU656_10 = 0x01,
>>> - ADV7604_OP_FORMAT_SEL_SDR_ITU656_12_MODE0 = 0x02,
>>> - ADV7604_OP_FORMAT_SEL_SDR_ITU656_12_MODE1 = 0x06,
>>> - ADV7604_OP_FORMAT_SEL_SDR_ITU656_12_MODE2 = 0x0a,
>>> - ADV7604_OP_FORMAT_SEL_DDR_422_8 = 0x20,
>>> - ADV7604_OP_FORMAT_SEL_DDR_422_10 = 0x21,
>>> - ADV7604_OP_FORMAT_SEL_DDR_422_12_MODE0 = 0x22,
>>> - ADV7604_OP_FORMAT_SEL_DDR_422_12_MODE1 = 0x23,
>>> - ADV7604_OP_FORMAT_SEL_DDR_422_12_MODE2 = 0x24,
>>> - ADV7604_OP_FORMAT_SEL_SDR_444_24 = 0x40,
>>> - ADV7604_OP_FORMAT_SEL_SDR_444_30 = 0x41,
>>> - ADV7604_OP_FORMAT_SEL_SDR_444_36_MODE0 = 0x42,
>>> - ADV7604_OP_FORMAT_SEL_DDR_444_24 = 0x60,
>>> - ADV7604_OP_FORMAT_SEL_DDR_444_30 = 0x61,
>>> - ADV7604_OP_FORMAT_SEL_DDR_444_36 = 0x62,
>>> - ADV7604_OP_FORMAT_SEL_SDR_ITU656_16 = 0x80,
>>> - ADV7604_OP_FORMAT_SEL_SDR_ITU656_20 = 0x81,
>>> - ADV7604_OP_FORMAT_SEL_SDR_ITU656_24_MODE0 = 0x82,
>>> - ADV7604_OP_FORMAT_SEL_SDR_ITU656_24_MODE1 = 0x86,
>>> - ADV7604_OP_FORMAT_SEL_SDR_ITU656_24_MODE2 = 0x8a,
>>> +/* Select output format (IO register 0x03, [4:2]) */
>>> +enum adv7604_op_format_mode_sel {
>>> + ADV7604_OP_FORMAT_MODE0 = 0x00,
>>> + ADV7604_OP_FORMAT_MODE1 = 0x04,
>>> + ADV7604_OP_FORMAT_MODE2 = 0x08,
>>> };
>>>
>>> enum adv7604_drive_strength {
>>> @@ -104,11 +76,8 @@ struct adv7604_platform_data {
>>> /* Analog input muxing mode */
>>> enum adv7604_ain_sel ain_sel;
>>>
>>> - /* Bus rotation and reordering */
>>> - enum adv7604_op_ch_sel op_ch_sel;
>>
>> I would keep this as part of the platform_data. This is typically used if
>> things are wired up in a non-standard way and so is specific to the
>> hardware. It is not something that will change from format to format.
>
> Right, some level of configuration is needed to account for non-standard
> wiring. However I'm not sure where that should be handled.
>
> With exotic wiring the format at the receiver will be different than the
> format output by the ADV7604. From a pure ADV7604 point of view, the output
> format doesn't depend on the wiring. I wonder whether this shouldn't be a
> link
> property instead of being a subdev property. There's of course the question
> of
> where to store that link property if it's not part of either subdev.
>
> Even if we decide that the wiring is a property of the source subdev, I don't
> think we should duplicate bus reordering code in all subdev drivers. This
> should thus be handled by the v4l2 core (either directly or as helper
> functions).
There are two reasons why you might want to use op_ch_sel: one is to
implement weird formats like RBG. Something like that would have to be
controlled through mbus and pixel fourcc codes and not by hardcoding this
register.
The other is to compensate for a wiring problem: we have a card where two
channels were accidentally swapped. You can either redo the board or just
set this register. In this case this register is IMHO a property of this
subdev. It needs to know about it, because if it ever needs to output RBG
in the future then it needs to compensate for reordering for wiring
issues.
So you set this field if you have to compensate for wiring errors, making
this part of the DT/platform_data. You do not set this field when you
want to support special formats, that is done in the driver itself through
fourcc codes (or could be done as this isn't implemented at the moment).
Regards,
Hans
>
>> Other t
Re: [PATCH 36/47] adv7604: Make output format configurable through pad format operations
Hi Hans,
On Wednesday 12 February 2014 16:01:17 Hans Verkuil wrote:
> On 02/05/14 17:42, Laurent Pinchart wrote:
> > Replace the dummy video format operations by pad format operations that
> > configure the output format.
> >
> > Signed-off-by: Laurent Pinchart
> > ---
> >
> > drivers/media/i2c/adv7604.c | 243 ++-
> > include/media/adv7604.h | 47 ++---
> > 2 files changed, 225 insertions(+), 65 deletions(-)
>
>
>
> > diff --git a/include/media/adv7604.h b/include/media/adv7604.h
> > index 22811d3..2cc8e16 100644
> > --- a/include/media/adv7604.h
> > +++ b/include/media/adv7604.h
> > @@ -32,16 +32,6 @@ enum adv7604_ain_sel {
> >
> > ADV7604_AIN9_4_5_6_SYNC_2_1 = 4,
> >
> > };
> >
> > -/* Bus rotation and reordering (IO register 0x04, [7:5]) */
> > -enum adv7604_op_ch_sel {
> > - ADV7604_OP_CH_SEL_GBR = 0,
> > - ADV7604_OP_CH_SEL_GRB = 1,
> > - ADV7604_OP_CH_SEL_BGR = 2,
> > - ADV7604_OP_CH_SEL_RGB = 3,
> > - ADV7604_OP_CH_SEL_BRG = 4,
> > - ADV7604_OP_CH_SEL_RBG = 5,
> > -};
> > -
> > /* Input Color Space (IO register 0x02, [7:4]) */
> > enum adv7604_inp_color_space {
> > ADV7604_INP_COLOR_SPACE_LIM_RGB = 0,
> > @@ -55,29 +45,11 @@ enum adv7604_inp_color_space {
> > ADV7604_INP_COLOR_SPACE_AUTO = 0xf,
> > };
> >
> > -/* Select output format (IO register 0x03, [7:0]) */
> > -enum adv7604_op_format_sel {
> > - ADV7604_OP_FORMAT_SEL_SDR_ITU656_8 = 0x00,
> > - ADV7604_OP_FORMAT_SEL_SDR_ITU656_10 = 0x01,
> > - ADV7604_OP_FORMAT_SEL_SDR_ITU656_12_MODE0 = 0x02,
> > - ADV7604_OP_FORMAT_SEL_SDR_ITU656_12_MODE1 = 0x06,
> > - ADV7604_OP_FORMAT_SEL_SDR_ITU656_12_MODE2 = 0x0a,
> > - ADV7604_OP_FORMAT_SEL_DDR_422_8 = 0x20,
> > - ADV7604_OP_FORMAT_SEL_DDR_422_10 = 0x21,
> > - ADV7604_OP_FORMAT_SEL_DDR_422_12_MODE0 = 0x22,
> > - ADV7604_OP_FORMAT_SEL_DDR_422_12_MODE1 = 0x23,
> > - ADV7604_OP_FORMAT_SEL_DDR_422_12_MODE2 = 0x24,
> > - ADV7604_OP_FORMAT_SEL_SDR_444_24 = 0x40,
> > - ADV7604_OP_FORMAT_SEL_SDR_444_30 = 0x41,
> > - ADV7604_OP_FORMAT_SEL_SDR_444_36_MODE0 = 0x42,
> > - ADV7604_OP_FORMAT_SEL_DDR_444_24 = 0x60,
> > - ADV7604_OP_FORMAT_SEL_DDR_444_30 = 0x61,
> > - ADV7604_OP_FORMAT_SEL_DDR_444_36 = 0x62,
> > - ADV7604_OP_FORMAT_SEL_SDR_ITU656_16 = 0x80,
> > - ADV7604_OP_FORMAT_SEL_SDR_ITU656_20 = 0x81,
> > - ADV7604_OP_FORMAT_SEL_SDR_ITU656_24_MODE0 = 0x82,
> > - ADV7604_OP_FORMAT_SEL_SDR_ITU656_24_MODE1 = 0x86,
> > - ADV7604_OP_FORMAT_SEL_SDR_ITU656_24_MODE2 = 0x8a,
> > +/* Select output format (IO register 0x03, [4:2]) */
> > +enum adv7604_op_format_mode_sel {
> > + ADV7604_OP_FORMAT_MODE0 = 0x00,
> > + ADV7604_OP_FORMAT_MODE1 = 0x04,
> > + ADV7604_OP_FORMAT_MODE2 = 0x08,
> > };
> >
> > enum adv7604_drive_strength {
> > @@ -104,11 +76,8 @@ struct adv7604_platform_data {
> > /* Analog input muxing mode */
> > enum adv7604_ain_sel ain_sel;
> >
> > - /* Bus rotation and reordering */
> > - enum adv7604_op_ch_sel op_ch_sel;
>
> I would keep this as part of the platform_data. This is typically used if
> things are wired up in a non-standard way and so is specific to the
> hardware. It is not something that will change from format to format.
Right, some level of configuration is needed to account for non-standard
wiring. However I'm not sure where that should be handled.
With exotic wiring the format at the receiver will be different than the
format output by the ADV7604. From a pure ADV7604 point of view, the output
format doesn't depend on the wiring. I wonder whether this shouldn't be a link
property instead of being a subdev property. There's of course the question of
where to store that link property if it's not part of either subdev.
Even if we decide that the wiring is a property of the source subdev, I don't
think we should duplicate bus reordering code in all subdev drivers. This
should thus be handled by the v4l2 core (either directly or as helper
functions).
> Other than this it all looks quite nice! Much more flexible than before.
>
> > -
> > - /* Select output format */
> > - enum adv7604_op_format_sel op_format_sel;
> > + /* Select output format mode */
> > + enum adv7604_op_format_mode_sel op_format_mode_sel;
> >
> > /* Configuration of the INT1 pin */
> > enum adv7604_int1_config int1_config;
> > @@ -116,14 +85,12 @@ struct adv7604_platform_data {
> > /* IO register 0x02 */
> > unsigned alt_gamma:1;
> > unsigned op_656_range:1;
> > - unsigned rgb_out:1;
> > unsigned alt_data_sat:1;
> >
> > /* IO register 0x05 */
> > unsigned blank_data:1;
> > unsigned insert_av_codes:1;
> > unsigned replicate_av_codes:1;
> > - unsigned invert_cbcr:1;
> >
> > /* IO register 0x06 */
> > unsigned inv_vs_pol:1;
--
Regards,
Laurent Pinchart
--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http:
Re: [PATCH 36/47] adv7604: Make output format configurable through pad format operations
On 02/05/14 17:42, Laurent Pinchart wrote:
> Replace the dummy video format operations by pad format operations that
> configure the output format.
>
> Signed-off-by: Laurent Pinchart
> ---
> drivers/media/i2c/adv7604.c | 243
> +++-
> include/media/adv7604.h | 47 ++---
> 2 files changed, 225 insertions(+), 65 deletions(-)
>
> diff --git a/include/media/adv7604.h b/include/media/adv7604.h
> index 22811d3..2cc8e16 100644
> --- a/include/media/adv7604.h
> +++ b/include/media/adv7604.h
> @@ -32,16 +32,6 @@ enum adv7604_ain_sel {
> ADV7604_AIN9_4_5_6_SYNC_2_1 = 4,
> };
>
> -/* Bus rotation and reordering (IO register 0x04, [7:5]) */
> -enum adv7604_op_ch_sel {
> - ADV7604_OP_CH_SEL_GBR = 0,
> - ADV7604_OP_CH_SEL_GRB = 1,
> - ADV7604_OP_CH_SEL_BGR = 2,
> - ADV7604_OP_CH_SEL_RGB = 3,
> - ADV7604_OP_CH_SEL_BRG = 4,
> - ADV7604_OP_CH_SEL_RBG = 5,
> -};
> -
> /* Input Color Space (IO register 0x02, [7:4]) */
> enum adv7604_inp_color_space {
> ADV7604_INP_COLOR_SPACE_LIM_RGB = 0,
> @@ -55,29 +45,11 @@ enum adv7604_inp_color_space {
> ADV7604_INP_COLOR_SPACE_AUTO = 0xf,
> };
>
> -/* Select output format (IO register 0x03, [7:0]) */
> -enum adv7604_op_format_sel {
> - ADV7604_OP_FORMAT_SEL_SDR_ITU656_8 = 0x00,
> - ADV7604_OP_FORMAT_SEL_SDR_ITU656_10 = 0x01,
> - ADV7604_OP_FORMAT_SEL_SDR_ITU656_12_MODE0 = 0x02,
> - ADV7604_OP_FORMAT_SEL_SDR_ITU656_12_MODE1 = 0x06,
> - ADV7604_OP_FORMAT_SEL_SDR_ITU656_12_MODE2 = 0x0a,
> - ADV7604_OP_FORMAT_SEL_DDR_422_8 = 0x20,
> - ADV7604_OP_FORMAT_SEL_DDR_422_10 = 0x21,
> - ADV7604_OP_FORMAT_SEL_DDR_422_12_MODE0 = 0x22,
> - ADV7604_OP_FORMAT_SEL_DDR_422_12_MODE1 = 0x23,
> - ADV7604_OP_FORMAT_SEL_DDR_422_12_MODE2 = 0x24,
> - ADV7604_OP_FORMAT_SEL_SDR_444_24 = 0x40,
> - ADV7604_OP_FORMAT_SEL_SDR_444_30 = 0x41,
> - ADV7604_OP_FORMAT_SEL_SDR_444_36_MODE0 = 0x42,
> - ADV7604_OP_FORMAT_SEL_DDR_444_24 = 0x60,
> - ADV7604_OP_FORMAT_SEL_DDR_444_30 = 0x61,
> - ADV7604_OP_FORMAT_SEL_DDR_444_36 = 0x62,
> - ADV7604_OP_FORMAT_SEL_SDR_ITU656_16 = 0x80,
> - ADV7604_OP_FORMAT_SEL_SDR_ITU656_20 = 0x81,
> - ADV7604_OP_FORMAT_SEL_SDR_ITU656_24_MODE0 = 0x82,
> - ADV7604_OP_FORMAT_SEL_SDR_ITU656_24_MODE1 = 0x86,
> - ADV7604_OP_FORMAT_SEL_SDR_ITU656_24_MODE2 = 0x8a,
> +/* Select output format (IO register 0x03, [4:2]) */
> +enum adv7604_op_format_mode_sel {
> + ADV7604_OP_FORMAT_MODE0 = 0x00,
> + ADV7604_OP_FORMAT_MODE1 = 0x04,
> + ADV7604_OP_FORMAT_MODE2 = 0x08,
> };
>
> enum adv7604_drive_strength {
> @@ -104,11 +76,8 @@ struct adv7604_platform_data {
> /* Analog input muxing mode */
> enum adv7604_ain_sel ain_sel;
>
> - /* Bus rotation and reordering */
> - enum adv7604_op_ch_sel op_ch_sel;
I would keep this as part of the platform_data. This is typically used if things
are wired up in a non-standard way and so is specific to the hardware. It is not
something that will change from format to format.
Other than this it all looks quite nice! Much more flexible than before.
Regards,
Hans
> -
> - /* Select output format */
> - enum adv7604_op_format_sel op_format_sel;
> + /* Select output format mode */
> + enum adv7604_op_format_mode_sel op_format_mode_sel;
>
> /* Configuration of the INT1 pin */
> enum adv7604_int1_config int1_config;
> @@ -116,14 +85,12 @@ struct adv7604_platform_data {
> /* IO register 0x02 */
> unsigned alt_gamma:1;
> unsigned op_656_range:1;
> - unsigned rgb_out:1;
> unsigned alt_data_sat:1;
>
> /* IO register 0x05 */
> unsigned blank_data:1;
> unsigned insert_av_codes:1;
> unsigned replicate_av_codes:1;
> - unsigned invert_cbcr:1;
>
> /* IO register 0x06 */
> unsigned inv_vs_pol:1;
>
--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH 36/47] adv7604: Make output format configurable through pad format operations
Replace the dummy video format operations by pad format operations that
configure the output format.
Signed-off-by: Laurent Pinchart
---
drivers/media/i2c/adv7604.c | 243 +++-
include/media/adv7604.h | 47 ++---
2 files changed, 225 insertions(+), 65 deletions(-)
diff --git a/drivers/media/i2c/adv7604.c b/drivers/media/i2c/adv7604.c
index da32ce9..181372a 100644
--- a/drivers/media/i2c/adv7604.c
+++ b/drivers/media/i2c/adv7604.c
@@ -53,6 +53,28 @@ MODULE_LICENSE("GPL");
/* ADV7604 system clock frequency */
#define ADV7604_fsc (28636360)
+#define ADV7604_RGB_OUT(1 << 1)
+
+#define ADV7604_OP_FORMAT_SEL_8BIT (0 << 0)
+#define ADV7604_OP_FORMAT_SEL_10BIT(1 << 0)
+#define ADV7604_OP_FORMAT_SEL_12BIT(2 << 0)
+
+#define ADV7604_OP_MODE_SEL_SDR_422(0 << 5)
+#define ADV7604_OP_MODE_SEL_DDR_422(1 << 5)
+#define ADV7604_OP_MODE_SEL_SDR_444(2 << 5)
+#define ADV7604_OP_MODE_SEL_DDR_444(3 << 5)
+#define ADV7604_OP_MODE_SEL_SDR_422_2X (4 << 5)
+#define ADV7604_OP_MODE_SEL_ADI_CM (5 << 5)
+
+#define ADV7604_OP_CH_SEL_GBR (0 << 5)
+#define ADV7604_OP_CH_SEL_GRB (1 << 5)
+#define ADV7604_OP_CH_SEL_BGR (2 << 5)
+#define ADV7604_OP_CH_SEL_RGB (3 << 5)
+#define ADV7604_OP_CH_SEL_BRG (4 << 5)
+#define ADV7604_OP_CH_SEL_RBG (5 << 5)
+
+#define ADV7604_OP_SWAP_CB_CR (1 << 0)
+
enum adv7604_type {
ADV7604,
ADV7611,
@@ -63,6 +85,14 @@ struct adv7604_reg_seq {
u8 val;
};
+struct adv7604_format_info {
+ enum v4l2_mbus_pixelcode code;
+ u8 op_ch_sel;
+ bool rgb_out;
+ bool swap_cb_cr;
+ u8 op_format_sel;
+};
+
struct adv7604_chip_info {
enum adv7604_type type;
@@ -78,6 +108,9 @@ struct adv7604_chip_info {
unsigned int tdms_lock_mask;
unsigned int fmt_change_digital_mask;
+ const struct adv7604_format_info *formats;
+ unsigned int nformats;
+
void (*set_termination)(struct v4l2_subdev *sd, bool enable);
void (*setup_irqs)(struct v4l2_subdev *sd);
unsigned int (*read_hdmi_pixelclock)(struct v4l2_subdev *sd);
@@ -111,12 +144,18 @@ enum adv7604_pad {
struct adv7604_state {
const struct adv7604_chip_info *info;
struct adv7604_platform_data pdata;
+
struct v4l2_subdev sd;
struct media_pad pads[ADV7604_PAD_MAX];
unsigned int source_pad;
+
struct v4l2_ctrl_handler hdl;
+
enum adv7604_pad selected_input;
+
struct v4l2_dv_timings timings;
+ const struct adv7604_format_info *format;
+
struct {
u8 edid[256];
u32 present;
@@ -781,6 +820,93 @@ static void adv7604_write_reg_seq(struct v4l2_subdev *sd,
adv7604_write_reg(sd, reg_seq[i].reg, reg_seq[i].val);
}
+/*
-
+ * Format helpers
+ */
+
+static const struct adv7604_format_info adv7604_formats[] = {
+ { V4L2_MBUS_FMT_RGB888_1X24, ADV7604_OP_CH_SEL_RGB, true, false,
+ ADV7604_OP_MODE_SEL_SDR_444 | ADV7604_OP_FORMAT_SEL_8BIT },
+ { V4L2_MBUS_FMT_YUYV8_2X8, ADV7604_OP_CH_SEL_RGB, false, false,
+ ADV7604_OP_MODE_SEL_SDR_422 | ADV7604_OP_FORMAT_SEL_8BIT },
+ { V4L2_MBUS_FMT_YVYU8_2X8, ADV7604_OP_CH_SEL_RGB, false, true,
+ ADV7604_OP_MODE_SEL_SDR_422 | ADV7604_OP_FORMAT_SEL_8BIT },
+ { V4L2_MBUS_FMT_YUYV10_2X10, ADV7604_OP_CH_SEL_RGB, false, false,
+ ADV7604_OP_MODE_SEL_SDR_422 | ADV7604_OP_FORMAT_SEL_10BIT },
+ { V4L2_MBUS_FMT_YVYU10_2X10, ADV7604_OP_CH_SEL_RGB, false, true,
+ ADV7604_OP_MODE_SEL_SDR_422 | ADV7604_OP_FORMAT_SEL_10BIT },
+ { V4L2_MBUS_FMT_YUYV12_2X12, ADV7604_OP_CH_SEL_RGB, false, false,
+ ADV7604_OP_MODE_SEL_SDR_422 | ADV7604_OP_FORMAT_SEL_12BIT },
+ { V4L2_MBUS_FMT_YVYU12_2X12, ADV7604_OP_CH_SEL_RGB, false, true,
+ ADV7604_OP_MODE_SEL_SDR_422 | ADV7604_OP_FORMAT_SEL_12BIT },
+ { V4L2_MBUS_FMT_UYVY8_1X16, ADV7604_OP_CH_SEL_RBG, false, false,
+ ADV7604_OP_MODE_SEL_SDR_422_2X | ADV7604_OP_FORMAT_SEL_8BIT },
+ { V4L2_MBUS_FMT_VYUY8_1X16, ADV7604_OP_CH_SEL_RBG, false, true,
+ ADV7604_OP_MODE_SEL_SDR_422_2X | ADV7604_OP_FORMAT_SEL_8BIT },
+ { V4L2_MBUS_FMT_YUYV8_1X16, ADV7604_OP_CH_SEL_RGB, false, false,
+ ADV7604_OP_MODE_SEL_SDR_422_2X | ADV7604_OP_FORMAT_SEL_8BIT },
+ { V4L2_MBUS_FMT_YVYU8_1X16, ADV7604_OP_CH_SEL_RGB, false, true,
+ ADV7604_OP_MODE_SEL_SDR_422_2X | ADV7604_OP_FORMAT_SEL_8BIT },
+ { V4L2_MBUS_FMT_UYVY10_1X20, ADV7604_OP_CH_SEL_RBG, false, false,
+
