| Message ID | 20190822080556.17109-1-m.felsch@pengutronix.de |
|---|---|
| Headers | show |
| Series | Add TVP5150 features | expand |
On 8/22/19 10:05 AM, Marco Felsch wrote: > Since commit 10d5509c8d50 ("[media] v4l2: remove g/s_crop from video ops") > the 'which' field for set/get_selection must be FORMAT_ACTIVE. There is > no way to try different selections. The patch adds a helper function to > select the correct selection memory space (sub-device file handle or > driver state) which will be set/returned. > > The selection rectangle is updated if the format is FORMAT_ACTIVE and > the rectangle position and/or size differs from the current set > rectangle. > > Signed-off-by: Marco Felsch <m.felsch@pengutronix.de> > --- > Changelog: > > v8: > - adapt commit message > - remove wrong FORMAT_TRY handling for tvp5150_fill_fmt() handling > - return 0 during set_selection if FORMAT_TRY was requested and > CONFIG_VIDEO_V4L2_SUBDEV_API is disabled > - return -EINVAL during get_selection if FORMAT_TRY was requested and > CONFIG_VIDEO_V4L2_SUBDEV_API is disabled > v7: > - __tvp5150_get_pad_crop(): return error on default case > - simplify __tvp5150_get_pad_crop() error handling > - tvp5150_set_selection() squash __tvp5150_set_selection() execution > conditions > v6: > nothing > v5: > - handle stub for v4l2_subdev_get_try_crop() internal since commit > ("media: v4l2-subdev: add stubs for v4l2_subdev_get_try_*") > isn't anymore part of this series. > - add error handling of __tvp5150_get_pad_crop() > v4: > - fix merge conflict due to rebase on top of media-tree/master > - __tvp5150_get_pad_crop(): cosmetic alignment fixes > --- > drivers/media/i2c/tvp5150.c | 111 +++++++++++++++++++++++++----------- > 1 file changed, 79 insertions(+), 32 deletions(-) > > diff --git a/drivers/media/i2c/tvp5150.c b/drivers/media/i2c/tvp5150.c > index 477a929d4f89..62a1c7c3a7c0 100644 > --- a/drivers/media/i2c/tvp5150.c > +++ b/drivers/media/i2c/tvp5150.c > @@ -19,6 +19,7 @@ > #include <media/v4l2-ctrls.h> > #include <media/v4l2-fwnode.h> > #include <media/v4l2-mc.h> > +#include <media/v4l2-rect.h> > > #include "tvp5150_reg.h" > > @@ -995,6 +996,23 @@ static void tvp5150_set_default(v4l2_std_id std, struct v4l2_rect *crop) > crop->height = TVP5150_V_MAX_OTHERS; > } > > +static struct v4l2_rect * > +__tvp5150_get_pad_crop(struct tvp5150 *decoder, > + struct v4l2_subdev_pad_config *cfg, unsigned int pad, > + enum v4l2_subdev_format_whence which) > +{ > + switch (which) { > + case V4L2_SUBDEV_FORMAT_ACTIVE: > + return &decoder->rect; > + case V4L2_SUBDEV_FORMAT_TRY: > +#if defined(CONFIG_VIDEO_V4L2_SUBDEV_API) > + return v4l2_subdev_get_try_crop(&decoder->sd, cfg, pad); > +#endif Hmm, this fall-through is confusing. I'd just do: #else return ERR_PTR(-EINVAL); #endif > + default: > + return ERR_PTR(-EINVAL); > + } > +} > + > static int tvp5150_fill_fmt(struct v4l2_subdev *sd, > struct v4l2_subdev_pad_config *cfg, > struct v4l2_subdev_format *format) > @@ -1019,36 +1037,68 @@ static int tvp5150_fill_fmt(struct v4l2_subdev *sd, > return 0; > } > > +unsigned int tvp5150_get_hmax(struct v4l2_subdev *sd) > +{ > + struct tvp5150 *decoder = to_tvp5150(sd); > + v4l2_std_id std; > + > + /* Calculate height based on current standard */ > + if (decoder->norm == V4L2_STD_ALL) > + std = tvp5150_read_std(sd); > + else > + std = decoder->norm; > + > + return (std & V4L2_STD_525_60) ? > + TVP5150_V_MAX_525_60 : TVP5150_V_MAX_OTHERS; > +} > + > +static inline void > +__tvp5150_set_selection(struct v4l2_subdev *sd, struct v4l2_rect rect) > +{ > + struct tvp5150 *decoder = to_tvp5150(sd); > + unsigned int hmax = tvp5150_get_hmax(sd); > + > + regmap_write(decoder->regmap, TVP5150_VERT_BLANKING_START, rect.top); > + regmap_write(decoder->regmap, TVP5150_VERT_BLANKING_STOP, > + rect.top + rect.height - hmax); > + regmap_write(decoder->regmap, TVP5150_ACT_VD_CROP_ST_MSB, > + rect.left >> TVP5150_CROP_SHIFT); > + regmap_write(decoder->regmap, TVP5150_ACT_VD_CROP_ST_LSB, > + rect.left | (1 << TVP5150_CROP_SHIFT)); > + regmap_write(decoder->regmap, TVP5150_ACT_VD_CROP_STP_MSB, > + (rect.left + rect.width - TVP5150_MAX_CROP_LEFT) >> > + TVP5150_CROP_SHIFT); > + regmap_write(decoder->regmap, TVP5150_ACT_VD_CROP_STP_LSB, > + rect.left + rect.width - TVP5150_MAX_CROP_LEFT); > +} > + > static int tvp5150_set_selection(struct v4l2_subdev *sd, > struct v4l2_subdev_pad_config *cfg, > struct v4l2_subdev_selection *sel) > { > struct tvp5150 *decoder = to_tvp5150(sd); > struct v4l2_rect rect = sel->r; > - v4l2_std_id std; > - int hmax; > + struct v4l2_rect *crop; > + unsigned int hmax; > > - if (sel->which != V4L2_SUBDEV_FORMAT_ACTIVE || > - sel->target != V4L2_SEL_TGT_CROP) > + if (sel->target != V4L2_SEL_TGT_CROP) > return -EINVAL; > > dev_dbg_lvl(sd->dev, 1, debug, "%s left=%d, top=%d, width=%d, height=%d\n", > __func__, rect.left, rect.top, rect.width, rect.height); > > + /* > + * Do not apply the request in case of FORMAT_TRY and disabled > + * CONFIG_VIDEO_V4L2_SUBDEV_API support. > + */ > + crop = __tvp5150_get_pad_crop(decoder, cfg, sel->pad, sel->which); > + if (IS_ERR(crop)) > + return 0; This isn't right. If VIDEO_V4L2_SUBDEV_API isn't set, then set_selection with FORMAT_TRY should succeed: it should just verify (and optionally adjust) the selection against the current active format and return that. I think the easiest would be to do this: #ifdef VIDEO_V4L2_SUBDEV_API crop = __tvp5150_get_pad_crop(decoder, cfg, sel->pad, sel->which); #else crop = __tvp5150_get_pad_crop(decoder, cfg, sel->pad, V4L2_SUBDEV_FORMAT_ACTIVE); #endif > + > /* tvp5150 has some special limits */ > rect.left = clamp(rect.left, 0, TVP5150_MAX_CROP_LEFT); > rect.top = clamp(rect.top, 0, TVP5150_MAX_CROP_TOP); > - > - /* Calculate height based on current standard */ > - if (decoder->norm == V4L2_STD_ALL) > - std = tvp5150_read_std(sd); > - else > - std = decoder->norm; > - > - if (std & V4L2_STD_525_60) > - hmax = TVP5150_V_MAX_525_60; > - else > - hmax = TVP5150_V_MAX_OTHERS; > + hmax = tvp5150_get_hmax(sd); > > /* > * alignments: > @@ -1061,20 +1111,15 @@ static int tvp5150_set_selection(struct v4l2_subdev *sd, > hmax - TVP5150_MAX_CROP_TOP - rect.top, > hmax - rect.top, 0, 0); > > - regmap_write(decoder->regmap, TVP5150_VERT_BLANKING_START, rect.top); > - regmap_write(decoder->regmap, TVP5150_VERT_BLANKING_STOP, > - rect.top + rect.height - hmax); > - regmap_write(decoder->regmap, TVP5150_ACT_VD_CROP_ST_MSB, > - rect.left >> TVP5150_CROP_SHIFT); > - regmap_write(decoder->regmap, TVP5150_ACT_VD_CROP_ST_LSB, > - rect.left | (1 << TVP5150_CROP_SHIFT)); > - regmap_write(decoder->regmap, TVP5150_ACT_VD_CROP_STP_MSB, > - (rect.left + rect.width - TVP5150_MAX_CROP_LEFT) >> > - TVP5150_CROP_SHIFT); > - regmap_write(decoder->regmap, TVP5150_ACT_VD_CROP_STP_LSB, > - rect.left + rect.width - TVP5150_MAX_CROP_LEFT); > + /* > + * Update output image size if the selection (crop) rectangle size or > + * position has been modified. > + */ > + if (sel->which == V4L2_SUBDEV_FORMAT_ACTIVE && > + !v4l2_rect_equal(&rect, crop)) > + __tvp5150_set_selection(sd, rect); > > - decoder->rect = rect; > + *crop = rect; > > return 0; > } > @@ -1084,11 +1129,9 @@ static int tvp5150_get_selection(struct v4l2_subdev *sd, > struct v4l2_subdev_selection *sel) > { > struct tvp5150 *decoder = container_of(sd, struct tvp5150, sd); > + struct v4l2_rect *crop; > v4l2_std_id std; > > - if (sel->which != V4L2_SUBDEV_FORMAT_ACTIVE) > - return -EINVAL; > - > switch (sel->target) { > case V4L2_SEL_TGT_CROP_BOUNDS: > sel->r.left = 0; > @@ -1106,7 +1149,11 @@ static int tvp5150_get_selection(struct v4l2_subdev *sd, > sel->r.height = TVP5150_V_MAX_OTHERS; > return 0; > case V4L2_SEL_TGT_CROP: > - sel->r = decoder->rect; > + crop = __tvp5150_get_pad_crop(decoder, cfg, sel->pad, > + sel->which); > + if (IS_ERR(crop)) > + return PTR_ERR(crop); > + sel->r = *crop; > return 0; > default: > return -EINVAL; > Regards, Hans
On 8/22/19 10:05 AM, Marco Felsch wrote: > Currently every driver needs to parse the connector endpoints by it self. > This is the initial work to make this generic. A generic connector has > common members and connector specific members. The common members are: > - type > - label (optional) > - links > - nr_of_links > > The specific members are stored within a union, since only one of them > can be available at the time. Since this is the initial support the > patch adds only the analog-connector specific ones. > > Signed-off-by: Marco Felsch <m.felsch@pengutronix.de> > --- > [1] https://patchwork.kernel.org/cover/10794703/ > > v8: > - rename CON -> CONN > - supported_tvnorms_stds -> sdtv_stds and adapt description > > v7: > - fix spelling issues > - constify label > - support variable label size > - replace single remote_port/id members by links member of variable > size > - squash v4l2-connector into v4l2-fwnode > > @Jacopo: I dropped your r b tag because I changed the port/id logic. > > v6: > - fix some spelling and style issues > - rm unnecessary comments > - drop vga and dvi connector > - fix misspelt connector > > v2-v4: > - nothing since the patch was squashed from series [1] into this > series. > --- > include/media/v4l2-fwnode.h | 45 +++++++++++++++++++++++++++++++++++++ > 1 file changed, 45 insertions(+) > > diff --git a/include/media/v4l2-fwnode.h b/include/media/v4l2-fwnode.h > index f6a7bcd13197..7ca5669ef6fa 100644 > --- a/include/media/v4l2-fwnode.h > +++ b/include/media/v4l2-fwnode.h > @@ -123,6 +123,51 @@ struct v4l2_fwnode_link { > unsigned int remote_port; > }; > > +/** > + * enum v4l2_connector_type - connector type > + * @V4L2_CONN_UNKNOWN: unknown connector type, no V4L2 connector configuration > + * @V4L2_CONN_COMPOSITE: analog composite connector > + * @V4L2_CONN_SVIDEO: analog svideo connector > + * @V4L2_CONN_HDMI: digital hdmi connector Why exactly is CONN_HDMI added if there is no V4L2 driver that uses it? I would just drop it from this series. Regards, Hans > + */ > +enum v4l2_connector_type { > + V4L2_CONN_UNKNOWN, > + V4L2_CONN_COMPOSITE, > + V4L2_CONN_SVIDEO, > + V4L2_CONN_HDMI, > +}; > + > +/** > + * struct v4l2_fwnode_connector_analog - analog connector data structure > + * @sdtv_stds: sdtv standards this connector supports, set to V4L2_STD_ALL > + * if no restrictions are specified. > + */ > +struct v4l2_fwnode_connector_analog { > + v4l2_std_id sdtv_stds; > +}; > + > +/** > + * struct v4l2_fwnode_connector - the connector data structure > + * @label: optional connector label > + * @type: connector type > + * @links: list of &struct v4l2_fwnode_link links the connector is connected to > + * @nr_of_links: total number of links > + * @connector: connector configuration > + * @connector.analog: analog connector configuration > + * &struct v4l2_fwnode_connector_analog > + */ > +struct v4l2_fwnode_connector { > + const char *label; > + enum v4l2_connector_type type; > + struct v4l2_fwnode_link *links; > + unsigned int nr_of_links; > + > + union { > + struct v4l2_fwnode_connector_analog analog; > + /* future connectors */ > + } connector; > +}; > + > /** > * v4l2_fwnode_endpoint_parse() - parse all fwnode node properties > * @fwnode: pointer to the endpoint's fwnode handle >
On 19-08-29 11:59, Hans Verkuil wrote: > On 8/22/19 10:05 AM, Marco Felsch wrote: > > Currently every driver needs to parse the connector endpoints by it self. > > This is the initial work to make this generic. A generic connector has > > common members and connector specific members. The common members are: > > - type > > - label (optional) > > - links > > - nr_of_links > > > > The specific members are stored within a union, since only one of them > > can be available at the time. Since this is the initial support the > > patch adds only the analog-connector specific ones. > > > > Signed-off-by: Marco Felsch <m.felsch@pengutronix.de> > > --- > > [1] https://patchwork.kernel.org/cover/10794703/ > > > > v8: > > - rename CON -> CONN > > - supported_tvnorms_stds -> sdtv_stds and adapt description > > > > v7: > > - fix spelling issues > > - constify label > > - support variable label size > > - replace single remote_port/id members by links member of variable > > size > > - squash v4l2-connector into v4l2-fwnode > > > > @Jacopo: I dropped your r b tag because I changed the port/id logic. > > > > v6: > > - fix some spelling and style issues > > - rm unnecessary comments > > - drop vga and dvi connector > > - fix misspelt connector > > > > v2-v4: > > - nothing since the patch was squashed from series [1] into this > > series. > > --- > > include/media/v4l2-fwnode.h | 45 +++++++++++++++++++++++++++++++++++++ > > 1 file changed, 45 insertions(+) > > > > diff --git a/include/media/v4l2-fwnode.h b/include/media/v4l2-fwnode.h > > index f6a7bcd13197..7ca5669ef6fa 100644 > > --- a/include/media/v4l2-fwnode.h > > +++ b/include/media/v4l2-fwnode.h > > @@ -123,6 +123,51 @@ struct v4l2_fwnode_link { > > unsigned int remote_port; > > }; > > > > +/** > > + * enum v4l2_connector_type - connector type > > + * @V4L2_CONN_UNKNOWN: unknown connector type, no V4L2 connector configuration > > + * @V4L2_CONN_COMPOSITE: analog composite connector > > + * @V4L2_CONN_SVIDEO: analog svideo connector > > + * @V4L2_CONN_HDMI: digital hdmi connector > > Why exactly is CONN_HDMI added if there is no V4L2 driver that uses it? Just for the sake of completeness.. > I would just drop it from this series. Okay. Regards, Marco > Regards, > > Hans > > > + */ > > +enum v4l2_connector_type { > > + V4L2_CONN_UNKNOWN, > > + V4L2_CONN_COMPOSITE, > > + V4L2_CONN_SVIDEO, > > + V4L2_CONN_HDMI, > > +}; > > + > > +/** > > + * struct v4l2_fwnode_connector_analog - analog connector data structure > > + * @sdtv_stds: sdtv standards this connector supports, set to V4L2_STD_ALL > > + * if no restrictions are specified. > > + */ > > +struct v4l2_fwnode_connector_analog { > > + v4l2_std_id sdtv_stds; > > +}; > > + > > +/** > > + * struct v4l2_fwnode_connector - the connector data structure > > + * @label: optional connector label > > + * @type: connector type > > + * @links: list of &struct v4l2_fwnode_link links the connector is connected to > > + * @nr_of_links: total number of links > > + * @connector: connector configuration > > + * @connector.analog: analog connector configuration > > + * &struct v4l2_fwnode_connector_analog > > + */ > > +struct v4l2_fwnode_connector { > > + const char *label; > > + enum v4l2_connector_type type; > > + struct v4l2_fwnode_link *links; > > + unsigned int nr_of_links; > > + > > + union { > > + struct v4l2_fwnode_connector_analog analog; > > + /* future connectors */ > > + } connector; > > +}; > > + > > /** > > * v4l2_fwnode_endpoint_parse() - parse all fwnode node properties > > * @fwnode: pointer to the endpoint's fwnode handle > > > >
On 19-08-29 11:56, Hans Verkuil wrote: > On 8/22/19 10:05 AM, Marco Felsch wrote: > > Since commit 10d5509c8d50 ("[media] v4l2: remove g/s_crop from video ops") > > the 'which' field for set/get_selection must be FORMAT_ACTIVE. There is > > no way to try different selections. The patch adds a helper function to > > select the correct selection memory space (sub-device file handle or > > driver state) which will be set/returned. > > > > The selection rectangle is updated if the format is FORMAT_ACTIVE and > > the rectangle position and/or size differs from the current set > > rectangle. > > > > Signed-off-by: Marco Felsch <m.felsch@pengutronix.de> > > --- > > Changelog: > > > > v8: > > - adapt commit message > > - remove wrong FORMAT_TRY handling for tvp5150_fill_fmt() handling > > - return 0 during set_selection if FORMAT_TRY was requested and > > CONFIG_VIDEO_V4L2_SUBDEV_API is disabled > > - return -EINVAL during get_selection if FORMAT_TRY was requested and > > CONFIG_VIDEO_V4L2_SUBDEV_API is disabled > > v7: > > - __tvp5150_get_pad_crop(): return error on default case > > - simplify __tvp5150_get_pad_crop() error handling > > - tvp5150_set_selection() squash __tvp5150_set_selection() execution > > conditions > > v6: > > nothing > > v5: > > - handle stub for v4l2_subdev_get_try_crop() internal since commit > > ("media: v4l2-subdev: add stubs for v4l2_subdev_get_try_*") > > isn't anymore part of this series. > > - add error handling of __tvp5150_get_pad_crop() > > v4: > > - fix merge conflict due to rebase on top of media-tree/master > > - __tvp5150_get_pad_crop(): cosmetic alignment fixes > > --- > > drivers/media/i2c/tvp5150.c | 111 +++++++++++++++++++++++++----------- > > 1 file changed, 79 insertions(+), 32 deletions(-) > > > > diff --git a/drivers/media/i2c/tvp5150.c b/drivers/media/i2c/tvp5150.c > > index 477a929d4f89..62a1c7c3a7c0 100644 > > --- a/drivers/media/i2c/tvp5150.c > > +++ b/drivers/media/i2c/tvp5150.c > > @@ -19,6 +19,7 @@ > > #include <media/v4l2-ctrls.h> > > #include <media/v4l2-fwnode.h> > > #include <media/v4l2-mc.h> > > +#include <media/v4l2-rect.h> > > > > #include "tvp5150_reg.h" > > > > @@ -995,6 +996,23 @@ static void tvp5150_set_default(v4l2_std_id std, struct v4l2_rect *crop) > > crop->height = TVP5150_V_MAX_OTHERS; > > } > > > > +static struct v4l2_rect * > > +__tvp5150_get_pad_crop(struct tvp5150 *decoder, > > + struct v4l2_subdev_pad_config *cfg, unsigned int pad, > > + enum v4l2_subdev_format_whence which) > > +{ > > + switch (which) { > > + case V4L2_SUBDEV_FORMAT_ACTIVE: > > + return &decoder->rect; > > + case V4L2_SUBDEV_FORMAT_TRY: > > +#if defined(CONFIG_VIDEO_V4L2_SUBDEV_API) > > + return v4l2_subdev_get_try_crop(&decoder->sd, cfg, pad); > > +#endif > > Hmm, this fall-through is confusing. > > I'd just do: > > #else > return ERR_PTR(-EINVAL); > #endif Okay. > > + default: > > + return ERR_PTR(-EINVAL); > > + } > > +} > > + > > static int tvp5150_fill_fmt(struct v4l2_subdev *sd, > > struct v4l2_subdev_pad_config *cfg, > > struct v4l2_subdev_format *format) > > @@ -1019,36 +1037,68 @@ static int tvp5150_fill_fmt(struct v4l2_subdev *sd, > > return 0; > > } > > > > +unsigned int tvp5150_get_hmax(struct v4l2_subdev *sd) > > +{ > > + struct tvp5150 *decoder = to_tvp5150(sd); > > + v4l2_std_id std; > > + > > + /* Calculate height based on current standard */ > > + if (decoder->norm == V4L2_STD_ALL) > > + std = tvp5150_read_std(sd); > > + else > > + std = decoder->norm; > > + > > + return (std & V4L2_STD_525_60) ? > > + TVP5150_V_MAX_525_60 : TVP5150_V_MAX_OTHERS; > > +} > > + > > +static inline void > > +__tvp5150_set_selection(struct v4l2_subdev *sd, struct v4l2_rect rect) > > +{ > > + struct tvp5150 *decoder = to_tvp5150(sd); > > + unsigned int hmax = tvp5150_get_hmax(sd); > > + > > + regmap_write(decoder->regmap, TVP5150_VERT_BLANKING_START, rect.top); > > + regmap_write(decoder->regmap, TVP5150_VERT_BLANKING_STOP, > > + rect.top + rect.height - hmax); > > + regmap_write(decoder->regmap, TVP5150_ACT_VD_CROP_ST_MSB, > > + rect.left >> TVP5150_CROP_SHIFT); > > + regmap_write(decoder->regmap, TVP5150_ACT_VD_CROP_ST_LSB, > > + rect.left | (1 << TVP5150_CROP_SHIFT)); > > + regmap_write(decoder->regmap, TVP5150_ACT_VD_CROP_STP_MSB, > > + (rect.left + rect.width - TVP5150_MAX_CROP_LEFT) >> > > + TVP5150_CROP_SHIFT); > > + regmap_write(decoder->regmap, TVP5150_ACT_VD_CROP_STP_LSB, > > + rect.left + rect.width - TVP5150_MAX_CROP_LEFT); > > +} > > + > > static int tvp5150_set_selection(struct v4l2_subdev *sd, > > struct v4l2_subdev_pad_config *cfg, > > struct v4l2_subdev_selection *sel) > > { > > struct tvp5150 *decoder = to_tvp5150(sd); > > struct v4l2_rect rect = sel->r; > > - v4l2_std_id std; > > - int hmax; > > + struct v4l2_rect *crop; > > + unsigned int hmax; > > > > - if (sel->which != V4L2_SUBDEV_FORMAT_ACTIVE || > > - sel->target != V4L2_SEL_TGT_CROP) > > + if (sel->target != V4L2_SEL_TGT_CROP) > > return -EINVAL; > > > > dev_dbg_lvl(sd->dev, 1, debug, "%s left=%d, top=%d, width=%d, height=%d\n", > > __func__, rect.left, rect.top, rect.width, rect.height); > > > > + /* > > + * Do not apply the request in case of FORMAT_TRY and disabled > > + * CONFIG_VIDEO_V4L2_SUBDEV_API support. > > + */ > > + crop = __tvp5150_get_pad_crop(decoder, cfg, sel->pad, sel->which); > > + if (IS_ERR(crop)) > > + return 0; > > This isn't right. > > If VIDEO_V4L2_SUBDEV_API isn't set, then set_selection with FORMAT_TRY > should succeed: it should just verify (and optionally adjust) the selection > against the current active format and return that. Okay got that. > I think the easiest would be to do this: > > #ifdef VIDEO_V4L2_SUBDEV_API > crop = __tvp5150_get_pad_crop(decoder, cfg, sel->pad, sel->which); > #else > crop = __tvp5150_get_pad_crop(decoder, cfg, sel->pad, V4L2_SUBDEV_FORMAT_ACTIVE); > #endif Hm.. I think that's a bit to easy since we apply the rect to the crop so we would apply the selection to the driver state. I changed the order of set_selection in such a way to adapt the selection->rect first and return 0 if VIDEO_V4L2_SUBDEV_API isn't available and sel->which was V4L2_SUBDEV_FORMAT_TRY. Futhermore I recognized that the set_selection didn't adapt the sel->r. Instead the local copy is adapted... BTW. this isn't the only driver which has that problem.. Should we add a compliance test here to cover such problems during review? Regards, Marco > > > + > > /* tvp5150 has some special limits */ > > rect.left = clamp(rect.left, 0, TVP5150_MAX_CROP_LEFT); > > rect.top = clamp(rect.top, 0, TVP5150_MAX_CROP_TOP); > > - > > - /* Calculate height based on current standard */ > > - if (decoder->norm == V4L2_STD_ALL) > > - std = tvp5150_read_std(sd); > > - else > > - std = decoder->norm; > > - > > - if (std & V4L2_STD_525_60) > > - hmax = TVP5150_V_MAX_525_60; > > - else > > - hmax = TVP5150_V_MAX_OTHERS; > > + hmax = tvp5150_get_hmax(sd); > > > > /* > > * alignments: > > @@ -1061,20 +1111,15 @@ static int tvp5150_set_selection(struct v4l2_subdev *sd, > > hmax - TVP5150_MAX_CROP_TOP - rect.top, > > hmax - rect.top, 0, 0); > > > > - regmap_write(decoder->regmap, TVP5150_VERT_BLANKING_START, rect.top); > > - regmap_write(decoder->regmap, TVP5150_VERT_BLANKING_STOP, > > - rect.top + rect.height - hmax); > > - regmap_write(decoder->regmap, TVP5150_ACT_VD_CROP_ST_MSB, > > - rect.left >> TVP5150_CROP_SHIFT); > > - regmap_write(decoder->regmap, TVP5150_ACT_VD_CROP_ST_LSB, > > - rect.left | (1 << TVP5150_CROP_SHIFT)); > > - regmap_write(decoder->regmap, TVP5150_ACT_VD_CROP_STP_MSB, > > - (rect.left + rect.width - TVP5150_MAX_CROP_LEFT) >> > > - TVP5150_CROP_SHIFT); > > - regmap_write(decoder->regmap, TVP5150_ACT_VD_CROP_STP_LSB, > > - rect.left + rect.width - TVP5150_MAX_CROP_LEFT); > > + /* > > + * Update output image size if the selection (crop) rectangle size or > > + * position has been modified. > > + */ > > + if (sel->which == V4L2_SUBDEV_FORMAT_ACTIVE && > > + !v4l2_rect_equal(&rect, crop)) > > + __tvp5150_set_selection(sd, rect); > > > > - decoder->rect = rect; > > + *crop = rect; > > > > return 0; > > } > > @@ -1084,11 +1129,9 @@ static int tvp5150_get_selection(struct v4l2_subdev *sd, > > struct v4l2_subdev_selection *sel) > > { > > struct tvp5150 *decoder = container_of(sd, struct tvp5150, sd); > > + struct v4l2_rect *crop; > > v4l2_std_id std; > > > > - if (sel->which != V4L2_SUBDEV_FORMAT_ACTIVE) > > - return -EINVAL; > > - > > switch (sel->target) { > > case V4L2_SEL_TGT_CROP_BOUNDS: > > sel->r.left = 0; > > @@ -1106,7 +1149,11 @@ static int tvp5150_get_selection(struct v4l2_subdev *sd, > > sel->r.height = TVP5150_V_MAX_OTHERS; > > return 0; > > case V4L2_SEL_TGT_CROP: > > - sel->r = decoder->rect; > > + crop = __tvp5150_get_pad_crop(decoder, cfg, sel->pad, > > + sel->which); > > + if (IS_ERR(crop)) > > + return PTR_ERR(crop); > > + sel->r = *crop; > > return 0; > > default: > > return -EINVAL; > > > > Regards, > > Hans >
Hi, this version is exactly the same as my v8 [1]. I fixed only a leading semicolon issue in patch ("media: v4l2-fwnode: add initial connector parsing support") found by the kbuild test robot. Regards, Marco [1] https://www.spinics.net/lists/linux-media/msg156159.html Javier Martinez Canillas (1): partial revert of "[media] tvp5150: add HW input connectors support" Marco Felsch (11): dt-bindings: connector: analog: add sdtv standards property media: v4l2-fwnode: add v4l2_fwnode_connector media: v4l2-fwnode: add initial connector parsing support media: tvp5150: add input source selection of_graph support media: dt-bindings: tvp5150: Add input port connectors DT bindings media: tvp5150: add FORMAT_TRY support for get/set selection handlers media: tvp5150: add s_power callback media: dt-bindings: tvp5150: cleanup bindings stlye media: dt-bindings: tvp5150: add optional sdtv standards documentation media: tvp5150: add support to limit sdtv standards media: tvp5150: make debug output more readable Michael Tretter (1): media: tvp5150: initialize subdev before parsing device tree .../display/connector/analog-tv-connector.txt | 6 + .../devicetree/bindings/media/i2c/tvp5150.txt | 146 +++- drivers/media/i2c/tvp5150.c | 649 +++++++++++++----- drivers/media/v4l2-core/v4l2-fwnode.c | 133 ++++ include/dt-bindings/display/sdtv-standards.h | 76 ++ include/dt-bindings/media/tvp5150.h | 2 - include/media/v4l2-fwnode.h | 83 +++ include/uapi/linux/videodev2.h | 4 + 8 files changed, 909 insertions(+), 190 deletions(-) create mode 100644 include/dt-bindings/display/sdtv-standards.h