| Message ID | 1391618558-5580-37-git-send-email-laurent.pinchart@ideasonboard.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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 <laurent.pinchart@ideasonboard.com> > --- > drivers/media/i2c/adv7604.c | 243 +++++++++++++++++++++++++++++++++++++++----- > include/media/adv7604.h | 47 ++------- > 2 files changed, 225 insertions(+), 65 deletions(-) > <snip> > 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 majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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 <laurent.pinchart@ideasonboard.com> > > --- > > > > drivers/media/i2c/adv7604.c | 243 ++++++++++++++++++++++++++++++++++----- > > include/media/adv7604.h | 47 ++------- > > 2 files changed, 225 insertions(+), 65 deletions(-) > > <snip> > > > 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;
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 <laurent.pinchart@ideasonboard.com> >>> --- >>> >>> drivers/media/i2c/adv7604.c | 243 ++++++++++++++++++++++++++++++++++----- >>> include/media/adv7604.h | 47 ++------- >>> 2 files changed, 225 insertions(+), 65 deletions(-) >> >> <snip> >> >>> 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 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; > -- To unsubscribe from this list: send the line "unsubscribe linux-media" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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 <laurent.pinchart@ideasonboard.com> > >>> --- > >>> > >>> drivers/media/i2c/adv7604.c | 243 ++++++++++++++++++++++++++++++++----- > >>> include/media/adv7604.h | 47 ++------- > >>> 2 files changed, 225 insertions(+), 65 deletions(-) > >> > >> <snip> > >> > >>> 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. If we decide to go for the second solution I would like to make it a bit more generic than just keeping the op_ch_sel field in platform data. I think we should instead have a generic representation of the reordering in platform data, and implement a V4L2 core function that would translate the format on the link to the format required at the device. For instance, still assuming G and B are swapped, userspace would configure the ADV7604 output to V4L2_MBUS_FMT_RGB888_1X24, the adv7604 driver would pass V4L2_MBUS_FMT_RGB888_1X24 along with the reordering information to a V4L2 core function which would return V4L2_MBUS_FMT_RBG888_1X24, and the driver would then configure the device to output RBG instead of RGB. This leaves three open questions : - Should the workaround be visible to userspace ? - If not, how should reordering be expressed in platform data (and DT) ? - Does this need to be part of this patch set or can it be implemented later ?
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 <laurent.pinchart@ideasonboard.com> >>>>> --- >>>>> >>>>> drivers/media/i2c/adv7604.c | 243 ++++++++++++++++++++++++++++++++----- >>>>> include/media/adv7604.h | 47 ++------- >>>>> 2 files changed, 225 insertions(+), 65 deletions(-) >>>> >>>> <snip> >>>> >>>>> 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. > > If we decide to go for the second solution I would like to make it a bit more > generic than just keeping the op_ch_sel field in platform data. I think we > should instead have a generic representation of the reordering in platform > data, and implement a V4L2 core function that would translate the format on > the link to the format required at the device. For instance, still assuming G > and B are swapped, userspace would configure the ADV7604 output to > V4L2_MBUS_FMT_RGB888_1X24, the adv7604 driver would pass > V4L2_MBUS_FMT_RGB888_1X24 along with the reordering information to a V4L2 core > function which would return V4L2_MBUS_FMT_RBG888_1X24, and the driver would > then configure the device to output RBG instead of RGB. > > This leaves three open questions : > > - Should the workaround be visible to userspace ? Absolutely not. Userspace really, really does not want to care about this. It would also lead to a crazy proliferation of different fourccs, one for each 'reordering' combination of channels. > - If not, how should reordering be expressed in platform data (and DT) ? It could be a more generic reordering define (i.e. less ADV specific). Proposals are welcome. > - Does this need to be part of this patch set or can it be implemented later ? Yes. I need it :-) But it doesn't have to be in the DT, keeping it as platform_data for now is perfectly fine with me, nor do I mind changing it in the future. Regards, Hans -- To unsubscribe from this list: send the line "unsubscribe linux-media" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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, + ADV7604_OP_MODE_SEL_SDR_422_2X | ADV7604_OP_FORMAT_SEL_10BIT }, + { V4L2_MBUS_FMT_VYUY10_1X20, ADV7604_OP_CH_SEL_RBG, false, true, + ADV7604_OP_MODE_SEL_SDR_422_2X | ADV7604_OP_FORMAT_SEL_10BIT }, + { V4L2_MBUS_FMT_YUYV10_1X20, ADV7604_OP_CH_SEL_RGB, false, false, + ADV7604_OP_MODE_SEL_SDR_422_2X | ADV7604_OP_FORMAT_SEL_10BIT }, + { V4L2_MBUS_FMT_YVYU10_1X20, ADV7604_OP_CH_SEL_RGB, false, true, + ADV7604_OP_MODE_SEL_SDR_422_2X | ADV7604_OP_FORMAT_SEL_10BIT }, + { V4L2_MBUS_FMT_UYVY12_1X24, ADV7604_OP_CH_SEL_RBG, false, false, + ADV7604_OP_MODE_SEL_SDR_422_2X | ADV7604_OP_FORMAT_SEL_12BIT }, + { V4L2_MBUS_FMT_VYUY12_1X24, ADV7604_OP_CH_SEL_RBG, false, true, + ADV7604_OP_MODE_SEL_SDR_422_2X | ADV7604_OP_FORMAT_SEL_12BIT }, + { V4L2_MBUS_FMT_YUYV12_1X24, ADV7604_OP_CH_SEL_RGB, false, false, + ADV7604_OP_MODE_SEL_SDR_422_2X | ADV7604_OP_FORMAT_SEL_12BIT }, + { V4L2_MBUS_FMT_YVYU12_1X24, ADV7604_OP_CH_SEL_RGB, false, true, + ADV7604_OP_MODE_SEL_SDR_422_2X | ADV7604_OP_FORMAT_SEL_12BIT }, +}; + +static const struct adv7604_format_info adv7611_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_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_UYVY12_1X24, ADV7604_OP_CH_SEL_RBG, false, false, + ADV7604_OP_MODE_SEL_SDR_422_2X | ADV7604_OP_FORMAT_SEL_12BIT }, + { V4L2_MBUS_FMT_VYUY12_1X24, ADV7604_OP_CH_SEL_RBG, false, true, + ADV7604_OP_MODE_SEL_SDR_422_2X | ADV7604_OP_FORMAT_SEL_12BIT }, + { V4L2_MBUS_FMT_YUYV12_1X24, ADV7604_OP_CH_SEL_RGB, false, false, + ADV7604_OP_MODE_SEL_SDR_422_2X | ADV7604_OP_FORMAT_SEL_12BIT }, + { V4L2_MBUS_FMT_YVYU12_1X24, ADV7604_OP_CH_SEL_RGB, false, true, + ADV7604_OP_MODE_SEL_SDR_422_2X | ADV7604_OP_FORMAT_SEL_12BIT }, +}; + +static const struct adv7604_format_info * +adv7604_format_info(struct adv7604_state *state, enum v4l2_mbus_pixelcode code) +{ + unsigned int i; + + for (i = 0; i < state->info->nformats; ++i) { + if (state->info->formats[i].code == code) + return &state->info->formats[i]; + } + + return NULL; +} + /* ----------------------------------------------------------------------- */ static inline bool is_analog_input(struct v4l2_subdev *sd) @@ -1730,29 +1856,95 @@ static int adv7604_s_routing(struct v4l2_subdev *sd, return 0; } -static int adv7604_enum_mbus_fmt(struct v4l2_subdev *sd, unsigned int index, - enum v4l2_mbus_pixelcode *code) +static int adv7604_enum_mbus_code(struct v4l2_subdev *sd, + struct v4l2_subdev_fh *fh, + struct v4l2_subdev_mbus_code_enum *code) { - if (index) + struct adv7604_state *state = to_state(sd); + + if (code->index >= state->info->nformats) return -EINVAL; - /* Good enough for now */ - *code = V4L2_MBUS_FMT_FIXED; + + code->code = state->info->formats[code->index].code; + return 0; } -static int adv7604_g_mbus_fmt(struct v4l2_subdev *sd, - struct v4l2_mbus_framefmt *fmt) +static void adv7604_fill_format(struct adv7604_state *state, + struct v4l2_mbus_framefmt *format) { - struct adv7604_state *state = to_state(sd); + memset(format, 0, sizeof(*format)); - fmt->width = state->timings.bt.width; - fmt->height = state->timings.bt.height; - fmt->code = V4L2_MBUS_FMT_FIXED; - fmt->field = V4L2_FIELD_NONE; - if (state->timings.bt.standards & V4L2_DV_BT_STD_CEA861) { - fmt->colorspace = (state->timings.bt.height <= 576) ? + format->width = state->timings.bt.width; + format->height = state->timings.bt.height; + format->field = V4L2_FIELD_NONE; + + if (state->timings.bt.standards & V4L2_DV_BT_STD_CEA861) + format->colorspace = (state->timings.bt.height <= 576) ? V4L2_COLORSPACE_SMPTE170M : V4L2_COLORSPACE_REC709; +} + +static void adv7604_setup_format(struct adv7604_state *state) +{ + struct v4l2_subdev *sd = &state->sd; + + io_write_and_or(sd, 0x02, 0xfd, + state->format->rgb_out ? ADV7604_RGB_OUT : 0); + io_write(sd, 0x03, state->format->op_format_sel | + state->pdata.op_format_mode_sel); + io_write_and_or(sd, 0x04, 0x1f, state->format->op_ch_sel); + io_write_and_or(sd, 0x05, 0xfe, + state->format->swap_cb_cr ? ADV7604_OP_SWAP_CB_CR : 0); +} + +static int adv7604_get_format(struct v4l2_subdev *sd, struct v4l2_subdev_fh *fh, + struct v4l2_subdev_format *format) +{ + struct adv7604_state *state = to_state(sd); + + if (format->pad != state->source_pad) + return -EINVAL; + + adv7604_fill_format(state, &format->format); + + if (format->which == V4L2_SUBDEV_FORMAT_TRY) { + struct v4l2_mbus_framefmt *fmt; + + fmt = v4l2_subdev_get_try_format(fh, format->pad); + format->format.code = fmt->code; + } else { + format->format.code = state->format->code; } + + return 0; +} + +static int adv7604_set_format(struct v4l2_subdev *sd, struct v4l2_subdev_fh *fh, + struct v4l2_subdev_format *format) +{ + struct adv7604_state *state = to_state(sd); + const struct adv7604_format_info *info; + + if (format->pad != state->source_pad) + return -EINVAL; + + info = adv7604_format_info(state, format->format.code); + if (info == NULL) + info = adv7604_format_info(state, V4L2_MBUS_FMT_YUYV8_2X8); + + adv7604_fill_format(state, &format->format); + format->format.code = info->code; + + if (format->which == V4L2_SUBDEV_FORMAT_TRY) { + struct v4l2_mbus_framefmt *fmt; + + fmt = v4l2_subdev_get_try_format(fh, format->pad); + fmt->code = format->format.code; + } else { + state->format = info; + adv7604_setup_format(state); + } + return 0; } @@ -2203,13 +2395,12 @@ static const struct v4l2_subdev_video_ops adv7604_video_ops = { .query_dv_timings = adv7604_query_dv_timings, .enum_dv_timings = adv7604_enum_dv_timings, .dv_timings_cap = adv7604_dv_timings_cap, - .enum_mbus_fmt = adv7604_enum_mbus_fmt, - .g_mbus_fmt = adv7604_g_mbus_fmt, - .try_mbus_fmt = adv7604_g_mbus_fmt, - .s_mbus_fmt = adv7604_g_mbus_fmt, }; static const struct v4l2_subdev_pad_ops adv7604_pad_ops = { + .enum_mbus_code = adv7604_enum_mbus_code, + .get_fmt = adv7604_get_format, + .set_fmt = adv7604_set_format, .get_edid = adv7604_get_edid, .set_edid = adv7604_set_edid, }; @@ -2278,14 +2469,11 @@ static int adv7604_core_init(struct v4l2_subdev *sd) io_write_and_or(sd, 0x02, 0xf0, pdata->alt_gamma << 3 | pdata->op_656_range << 2 | - pdata->rgb_out << 1 | pdata->alt_data_sat << 0); - io_write(sd, 0x03, pdata->op_format_sel); - io_write_and_or(sd, 0x04, 0x1f, pdata->op_ch_sel << 5); - io_write_and_or(sd, 0x05, 0xf0, pdata->blank_data << 3 | - pdata->insert_av_codes << 2 | - pdata->replicate_av_codes << 1 | - pdata->invert_cbcr << 0); + io_write_and_or(sd, 0x05, 0xf1, pdata->blank_data << 3 | + pdata->insert_av_codes << 2 | + pdata->replicate_av_codes << 1); + adv7604_setup_format(state); cp_write(sd, 0x69, 0x30); /* Enable CP CSC */ @@ -2453,6 +2641,8 @@ static const struct adv7604_chip_info adv7604_chip_info[] = { .tdms_lock_mask = 0xe0, .cable_det_mask = 0x1e, .fmt_change_digital_mask = 0xc1, + .formats = adv7604_formats, + .nformats = ARRAY_SIZE(adv7604_formats), .set_termination = adv7604_set_termination, .setup_irqs = adv7604_setup_irqs, .read_hdmi_pixelclock = adv7604_read_hdmi_pixelclock, @@ -2484,6 +2674,8 @@ static const struct adv7604_chip_info adv7604_chip_info[] = { .tdms_lock_mask = 0x43, .cable_det_mask = 0x01, .fmt_change_digital_mask = 0x03, + .formats = adv7611_formats, + .nformats = ARRAY_SIZE(adv7611_formats), .set_termination = adv7611_set_termination, .setup_irqs = adv7611_setup_irqs, .read_hdmi_pixelclock = adv7611_read_hdmi_pixelclock, @@ -2539,6 +2731,7 @@ static int adv7604_probe(struct i2c_client *client, } state->pdata = *pdata; state->timings = cea640x480; + state->format = adv7604_format_info(state, V4L2_MBUS_FMT_YUYV8_2X8); sd = &state->sd; v4l2_i2c_subdev_init(sd, client, &adv7604_ops); 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; - - /* 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;
Replace the dummy video format operations by pad format operations that configure the output format. Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> --- drivers/media/i2c/adv7604.c | 243 +++++++++++++++++++++++++++++++++++++++----- include/media/adv7604.h | 47 ++------- 2 files changed, 225 insertions(+), 65 deletions(-)