* [PATCH v4 1/7] partial revert of "[media] tvp5150: add HW input connectors support"
2019-01-29 16:07 [PATCH v4 0/7] TVP5150 new features Marco Felsch
@ 2019-01-29 16:07 ` Marco Felsch
2019-01-29 16:07 ` [PATCH v4 2/7] media: tvp5150: add input source selection of_graph support Marco Felsch
` (6 subsequent siblings)
7 siblings, 0 replies; 14+ messages in thread
From: Marco Felsch @ 2019-01-29 16:07 UTC (permalink / raw)
To: mchehab, robh+dt, mark.rutland, sakari.ailus
Cc: devicetree, p.zabel, javierm, afshin.nasser, laurent.pinchart,
linux-media, kernel, Mauro Carvalho Chehab, Rob Herring
From: Javier Martinez Canillas <javierm@redhat.com>
Commit f7b4b54e6364 ("[media] tvp5150: add HW input connectors support")
added input signals support for the tvp5150, but the approach was found
to be incorrect so the corresponding DT binding commit 82c2ffeb217a
("[media] tvp5150: document input connectors DT bindings") was reverted.
This left the driver with an undocumented (and wrong) DT parsing logic,
so lets get rid of this code as well until the input connectors support
is implemented properly.
It's a partial revert due other patches added on top of mentioned commit
not allowing the commit to be reverted cleanly anymore. But all the code
related to the DT parsing logic and input entities creation are removed.
Suggested-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
Signed-off-by: Javier Martinez Canillas <javierm@redhat.com>
Acked-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
[m.felsch@pengutronix.de: rm TVP5150_INPUT_NUM define]
Signed-off-by: Marco Felsch <m.felsch@pengutronix.de>
Signed-off-by: Mauro Carvalho Chehab <mchehab+samsung@kernel.org>
Acked-by: Rob Herring <robh@kernel.org>
---
drivers/media/i2c/tvp5150.c | 141 ----------------------------
include/dt-bindings/media/tvp5150.h | 2 -
2 files changed, 143 deletions(-)
diff --git a/drivers/media/i2c/tvp5150.c b/drivers/media/i2c/tvp5150.c
index eaddd977ba40..89da921c8886 100644
--- a/drivers/media/i2c/tvp5150.c
+++ b/drivers/media/i2c/tvp5150.c
@@ -53,8 +53,6 @@ struct tvp5150 {
struct v4l2_subdev sd;
#ifdef CONFIG_MEDIA_CONTROLLER
struct media_pad pads[TVP5150_NUM_PADS];
- struct media_entity input_ent[TVP5150_INPUT_NUM];
- struct media_pad input_pad[TVP5150_INPUT_NUM];
#endif
struct v4l2_ctrl_handler hdl;
struct v4l2_rect rect;
@@ -1169,40 +1167,6 @@ static int tvp5150_enum_frame_size(struct v4l2_subdev *sd,
return 0;
}
-/****************************************************************************
- Media entity ops
- ****************************************************************************/
-
-#ifdef CONFIG_MEDIA_CONTROLLER
-static int tvp5150_link_setup(struct media_entity *entity,
- const struct media_pad *local,
- const struct media_pad *remote, u32 flags)
-{
- struct v4l2_subdev *sd = media_entity_to_v4l2_subdev(entity);
- struct tvp5150 *decoder = to_tvp5150(sd);
- int i;
-
- for (i = 0; i < TVP5150_INPUT_NUM; i++) {
- if (remote->entity == &decoder->input_ent[i])
- break;
- }
-
- /* Do nothing for entities that are not input connectors */
- if (i == TVP5150_INPUT_NUM)
- return 0;
-
- decoder->input = i;
-
- tvp5150_selmux(sd);
-
- return 0;
-}
-
-static const struct media_entity_operations tvp5150_sd_media_ops = {
- .link_setup = tvp5150_link_setup,
-};
-#endif
-
/****************************************************************************
I2C Command
****************************************************************************/
@@ -1350,42 +1314,6 @@ static int tvp5150_g_tuner(struct v4l2_subdev *sd, struct v4l2_tuner *vt)
return 0;
}
-static int tvp5150_registered(struct v4l2_subdev *sd)
-{
-#ifdef CONFIG_MEDIA_CONTROLLER
- struct tvp5150 *decoder = to_tvp5150(sd);
- int ret = 0;
- int i;
-
- for (i = 0; i < TVP5150_INPUT_NUM; i++) {
- struct media_entity *input = &decoder->input_ent[i];
- struct media_pad *pad = &decoder->input_pad[i];
-
- if (!input->name)
- continue;
-
- decoder->input_pad[i].flags = MEDIA_PAD_FL_SOURCE;
-
- ret = media_entity_pads_init(input, 1, pad);
- if (ret < 0)
- return ret;
-
- ret = media_device_register_entity(sd->v4l2_dev->mdev, input);
- if (ret < 0)
- return ret;
-
- ret = media_create_pad_link(input, 0, &sd->entity,
- TVP5150_PAD_IF_INPUT, 0);
- if (ret < 0) {
- media_device_unregister_entity(input);
- return ret;
- }
- }
-#endif
-
- return 0;
-}
-
/* ----------------------------------------------------------------------- */
static const struct v4l2_ctrl_ops tvp5150_ctrl_ops = {
@@ -1439,10 +1367,6 @@ static const struct v4l2_subdev_ops tvp5150_ops = {
.pad = &tvp5150_pad_ops,
};
-static const struct v4l2_subdev_internal_ops tvp5150_internal_ops = {
- .registered = tvp5150_registered,
-};
-
/****************************************************************************
I2C Client & Driver
****************************************************************************/
@@ -1595,12 +1519,6 @@ static int tvp5150_parse_dt(struct tvp5150 *decoder, struct device_node *np)
{
struct v4l2_fwnode_endpoint bus_cfg = { .bus_type = 0 };
struct device_node *ep;
-#ifdef CONFIG_MEDIA_CONTROLLER
- struct device_node *connectors, *child;
- struct media_entity *input;
- const char *name;
- u32 input_type;
-#endif
unsigned int flags;
int ret = 0;
@@ -1624,63 +1542,6 @@ static int tvp5150_parse_dt(struct tvp5150 *decoder, struct device_node *np)
decoder->mbus_type = bus_cfg.bus_type;
-#ifdef CONFIG_MEDIA_CONTROLLER
- connectors = of_get_child_by_name(np, "connectors");
-
- if (!connectors)
- goto err;
-
- for_each_available_child_of_node(connectors, child) {
- ret = of_property_read_u32(child, "input", &input_type);
- if (ret) {
- dev_err(decoder->sd.dev,
- "missing type property in node %pOFn\n",
- child);
- goto err_connector;
- }
-
- if (input_type >= TVP5150_INPUT_NUM) {
- ret = -EINVAL;
- goto err_connector;
- }
-
- input = &decoder->input_ent[input_type];
-
- /* Each input connector can only be defined once */
- if (input->name) {
- dev_err(decoder->sd.dev,
- "input %s with same type already exists\n",
- input->name);
- ret = -EINVAL;
- goto err_connector;
- }
-
- switch (input_type) {
- case TVP5150_COMPOSITE0:
- case TVP5150_COMPOSITE1:
- input->function = MEDIA_ENT_F_CONN_COMPOSITE;
- break;
- case TVP5150_SVIDEO:
- input->function = MEDIA_ENT_F_CONN_SVIDEO;
- break;
- }
-
- input->flags = MEDIA_ENT_FL_CONNECTOR;
-
- ret = of_property_read_string(child, "label", &name);
- if (ret < 0) {
- dev_err(decoder->sd.dev,
- "missing label property in node %pOFn\n",
- child);
- goto err_connector;
- }
-
- input->name = name;
- }
-
-err_connector:
- of_node_put(connectors);
-#endif
err:
of_node_put(ep);
return ret;
@@ -1732,7 +1593,6 @@ static int tvp5150_probe(struct i2c_client *c,
}
v4l2_i2c_subdev_init(sd, c, &tvp5150_ops);
- sd->internal_ops = &tvp5150_internal_ops;
sd->flags |= V4L2_SUBDEV_FL_HAS_DEVNODE;
#if defined(CONFIG_MEDIA_CONTROLLER)
@@ -1747,7 +1607,6 @@ static int tvp5150_probe(struct i2c_client *c,
if (res < 0)
return res;
- sd->entity.ops = &tvp5150_sd_media_ops;
#endif
res = tvp5150_detect_version(core);
diff --git a/include/dt-bindings/media/tvp5150.h b/include/dt-bindings/media/tvp5150.h
index c852a35e916e..8637910aae76 100644
--- a/include/dt-bindings/media/tvp5150.h
+++ b/include/dt-bindings/media/tvp5150.h
@@ -26,8 +26,6 @@
#define TVP5150_COMPOSITE1 1
#define TVP5150_SVIDEO 2
-#define TVP5150_INPUT_NUM 3
-
/* TVP5150 HW outputs */
#define TVP5150_NORMAL 0
#define TVP5150_BLACK_SCREEN 1
--
2.20.1
^ permalink raw reply related [flat|nested] 14+ messages in thread* [PATCH v4 2/7] media: tvp5150: add input source selection of_graph support
2019-01-29 16:07 [PATCH v4 0/7] TVP5150 new features Marco Felsch
2019-01-29 16:07 ` [PATCH v4 1/7] partial revert of "[media] tvp5150: add HW input connectors support" Marco Felsch
@ 2019-01-29 16:07 ` Marco Felsch
2019-03-21 13:03 ` Jacopo Mondi
2019-01-29 16:07 ` [PATCH v4 3/7] media: dt-bindings: tvp5150: Add input port connectors DT bindings Marco Felsch
` (5 subsequent siblings)
7 siblings, 1 reply; 14+ messages in thread
From: Marco Felsch @ 2019-01-29 16:07 UTC (permalink / raw)
To: mchehab, robh+dt, mark.rutland, sakari.ailus
Cc: devicetree, p.zabel, javierm, afshin.nasser, laurent.pinchart,
linux-media, kernel
This patch adds the of_graph support to describe the tvp connections.
Physical the TVP5150 has three ports: AIP1A, AIP1B and YOUT. As result
of discussion [1],[2] the device-tree maps these ports 1:1. The svideo
connector must be conneted to port@0/endpoint@1, look at the Documentation
for more information. Since the TVP5150 is a converter the device-tree
must contain at least 1-input and 1-output port. The mc-connectors and
mc-links are only created if the device-tree contains the corresponding
connector nodes. If more than one connector is available the
media_entity_operations.link_setup() callback ensures that only one
connector is active.
[1] https://www.spinics.net/lists/linux-media/msg138545.html
[2] https://www.spinics.net/lists/linux-media/msg138546.html
Signed-off-by: Marco Felsch <m.felsch@pengutronix.de>
---
Changelog:
v4:
- rebase on top of media_tree/master, fix merge conflict due to commit
60359a28d592 ("media: v4l: fwnode: Initialise the V4L2 fwnode endpoints
to zero")
v3:
- probe(): s/err/err_free_v4l2_ctrls
- drop MC dependency for tvp5150_pads
v2:
- adapt commit message
- unify ifdef switches
- rename tvp5150_valid_input -> tvp5150_of_valid_input, to be more precise
- mc: use 2-input and 1-output pad
- mc: link svideo connector to both input pads
- mc: enable/disable svideo links in one go
- mc: change link_setup() behaviour, switch the input src don't require a
explicite disable before.
- mc: rename 'local' media_pad param to tvp5150_pad to avoid confusion
- mc: enable link to the first available connector and set the
corresponding tvp5150 input src per default during registered()
- mc/of: factor out oftree connector allocation
- of: drop svideo dt port
- of: move svideo connector to port@0/endpoint@1
- of: require at least 1-in and 1-out endpoint
drivers/media/i2c/tvp5150.c | 481 +++++++++++++++++++++++++++++++++---
1 file changed, 442 insertions(+), 39 deletions(-)
diff --git a/drivers/media/i2c/tvp5150.c b/drivers/media/i2c/tvp5150.c
index 89da921c8886..a89b83f69266 100644
--- a/drivers/media/i2c/tvp5150.c
+++ b/drivers/media/i2c/tvp5150.c
@@ -44,15 +44,38 @@ MODULE_PARM_DESC(debug, "Debug level (0-2)");
#define dprintk0(__dev, __arg...) dev_dbg_lvl(__dev, 0, 0, __arg)
enum tvp5150_pads {
- TVP5150_PAD_IF_INPUT,
+ TVP5150_PAD_AIP1A = TVP5150_COMPOSITE0,
+ TVP5150_PAD_AIP1B,
TVP5150_PAD_VID_OUT,
TVP5150_NUM_PADS
};
+#if defined(CONFIG_MEDIA_CONTROLLER)
+enum tvp5150_pads_state {
+ TVP5150_PAD_INACTIVE,
+ TVP5150_PAD_ACTIVE_COMPOSITE,
+ TVP5150_PAD_ACTIVE_SVIDEO,
+};
+
+struct tvp5150_connector {
+ struct media_entity ent;
+ struct media_pad pad;
+ unsigned int port_num;
+ bool is_svideo;
+};
+#endif
+
struct tvp5150 {
struct v4l2_subdev sd;
-#ifdef CONFIG_MEDIA_CONTROLLER
+ /* additional additional endpoint for the svideo connector */
+ struct device_node *endpoints[TVP5150_NUM_PADS + 1];
+ unsigned int endpoints_num;
+#if defined(CONFIG_MEDIA_CONTROLLER)
struct media_pad pads[TVP5150_NUM_PADS];
+ int pads_state[TVP5150_NUM_PADS];
+ struct tvp5150_connector *connectors;
+ int connectors_num;
+ bool modify_second_link;
#endif
struct v4l2_ctrl_handler hdl;
struct v4l2_rect rect;
@@ -1167,6 +1190,160 @@ static int tvp5150_enum_frame_size(struct v4l2_subdev *sd,
return 0;
}
+/****************************************************************************
+ * Media entity ops
+ ****************************************************************************/
+#if defined(CONFIG_MEDIA_CONTROLLER)
+static int tvp5150_active_pad_idx(struct tvp5150 *decoder)
+{
+ int *pad_state = &decoder->pads_state[0];
+ int i, idx = -1;
+
+ for (i = 0; i < TVP5150_NUM_PADS - 1; i++) {
+ if ((pad_state[i] == TVP5150_PAD_ACTIVE_COMPOSITE) ||
+ (pad_state[i] == TVP5150_PAD_ACTIVE_SVIDEO)) {
+ idx = i;
+ break;
+ }
+ }
+
+ return idx;
+}
+
+static int tvp5150_active_svideo_links(struct tvp5150 *decoder)
+{
+ int *pad_state = &decoder->pads_state[0];
+ int i, links = 0;
+
+ for (i = 0; i < TVP5150_NUM_PADS - 1; i++)
+ if (pad_state[i] == TVP5150_PAD_ACTIVE_SVIDEO)
+ links++;
+
+ return links;
+}
+
+static int tvp5150_modify_link(struct tvp5150 *decoder, unsigned int pad_idx,
+ struct media_pad *pad, int flags)
+{
+ struct media_pad *src_pad;
+ struct media_link *link;
+
+ if (pad)
+ src_pad = pad;
+ else
+ src_pad = media_entity_remote_pad(&decoder->pads[pad_idx]);
+
+ if (!src_pad)
+ return -1;
+
+ link = media_entity_find_link(src_pad,
+ &decoder->pads[pad_idx]);
+
+ /*
+ * Don't use locked version, since we are running already within the
+ * media_entity_setup_link() context.
+ */
+ return __media_entity_setup_link(link, flags);
+
+}
+
+static int tvp5150_s_routing(struct v4l2_subdev *sd, u32 input, u32 output,
+ u32 config);
+
+static int tvp5150_link_setup(struct media_entity *entity,
+ const struct media_pad *tvp5150_pad,
+ const struct media_pad *remote, u32 flags)
+{
+ struct v4l2_subdev *sd = media_entity_to_v4l2_subdev(entity);
+ struct tvp5150 *decoder = to_tvp5150(sd);
+ int *pad_state = &decoder->pads_state[0];
+ int i, active_pad, ret = 0;
+ bool is_svideo = false;
+
+ /*
+ * The tvp state is determined by the enabled sink pad link.
+ * Enabling or disabling the source pad link has no effect.
+ */
+ if (tvp5150_pad->flags & MEDIA_PAD_FL_SOURCE)
+ return 0;
+
+ /* check if the svideo connector should be enabled */
+ for (i = 0; i < decoder->connectors_num; i++) {
+ if (remote->entity == &decoder->connectors[i].ent) {
+ is_svideo = decoder->connectors[i].is_svideo;
+ break;
+ }
+ }
+
+ /* check if there is already a enabled svideo link and determine pad */
+ active_pad = tvp5150_active_pad_idx(decoder);
+
+ dev_dbg(sd->dev, "link setup '%s':%d->'%s':%d[%d]",
+ remote->entity->name, remote->index, tvp5150_pad->entity->name,
+ tvp5150_pad->index, flags & MEDIA_LNK_FL_ENABLED);
+
+ if (flags & MEDIA_LNK_FL_ENABLED) {
+ if (active_pad >= 0 && !decoder->modify_second_link)
+ tvp5150_modify_link(decoder, active_pad, NULL, 0);
+
+ dev_dbg(sd->dev, "Setting %d active [%s]\n", tvp5150_pad->index,
+ is_svideo ? "svideo" : "composite");
+ pad_state[tvp5150_pad->index] =
+ is_svideo ? TVP5150_PAD_ACTIVE_SVIDEO :
+ TVP5150_PAD_ACTIVE_COMPOSITE;
+
+ if (is_svideo) {
+ if (tvp5150_active_svideo_links(decoder) < 2) {
+ unsigned int idx = tvp5150_pad->index ^ 1;
+
+ decoder->modify_second_link = true;
+ tvp5150_modify_link(decoder, idx,
+ (struct media_pad *)remote,
+ MEDIA_LNK_FL_ENABLED);
+ } else {
+ decoder->modify_second_link = false;
+ tvp5150_s_routing(sd, TVP5150_SVIDEO,
+ TVP5150_NORMAL, 0);
+ }
+ } else {
+ tvp5150_s_routing(sd, tvp5150_pad->index,
+ TVP5150_NORMAL, 0);
+ }
+ } else {
+ /*
+ * Svideo streams on two pads and user can request to AIP1A or
+ * AIP1B pad. In either case both pads gets disabled in in go.
+ * So check only if user wants to disable a not enabled
+ * composite pad.
+ */
+ if (!is_svideo && tvp5150_pad->index != active_pad)
+ goto out;
+
+ dev_dbg(sd->dev, "going inactive\n");
+ pad_state[tvp5150_pad->index] = TVP5150_PAD_INACTIVE;
+
+ /* in case of svideo we need to disable the second pad too */
+ if (tvp5150_active_svideo_links(decoder) > 0) {
+ unsigned int idx = tvp5150_pad->index ^ 1;
+
+ decoder->modify_second_link = true;
+ tvp5150_modify_link(decoder, idx,
+ (struct media_pad *)remote, 0);
+ }
+
+ if (!decoder->modify_second_link)
+ tvp5150_s_routing(sd, is_svideo ? TVP5150_SVIDEO :
+ active_pad, TVP5150_BLACK_SCREEN, 0);
+ decoder->modify_second_link = false;
+ }
+out:
+ return ret;
+}
+
+static const struct media_entity_operations tvp5150_sd_media_ops = {
+ .link_setup = tvp5150_link_setup,
+};
+#endif
/****************************************************************************
I2C Command
****************************************************************************/
@@ -1314,6 +1491,76 @@ static int tvp5150_g_tuner(struct v4l2_subdev *sd, struct v4l2_tuner *vt)
return 0;
}
+static int tvp5150_registered(struct v4l2_subdev *sd)
+{
+#if defined(CONFIG_MEDIA_CONTROLLER)
+ struct tvp5150 *decoder = to_tvp5150(sd);
+ unsigned int i;
+ int ret;
+
+ /*
+ * Setup connector pads and links. Enable the link to the first
+ * available connector per default.
+ */
+ for (i = 0; i < decoder->connectors_num; i++) {
+ struct media_entity *con = &decoder->connectors[i].ent;
+ struct media_pad *pad = &decoder->connectors[i].pad;
+ unsigned int port = decoder->connectors[i].port_num;
+ bool is_svideo = decoder->connectors[i].is_svideo;
+ int flags = i ? 0 : MEDIA_LNK_FL_ENABLED;
+
+ pad->flags = MEDIA_PAD_FL_SOURCE;
+ ret = media_entity_pads_init(con, 1, pad);
+ if (ret < 0)
+ return ret;
+
+ ret = media_device_register_entity(sd->v4l2_dev->mdev, con);
+ if (ret < 0)
+ return ret;
+
+ ret = media_create_pad_link(con, 0, &sd->entity, port, flags);
+ if (ret < 0) {
+ media_device_unregister_entity(con);
+ return ret;
+ }
+
+ if (is_svideo) {
+ /* svideo links to both aip1a and aip1b */
+ ret = media_create_pad_link(con, 0, &sd->entity,
+ port + 1, flags);
+ if (ret < 0) {
+ media_device_unregister_entity(con);
+ return ret;
+ }
+ }
+
+ /* enable default input */
+ if (flags == MEDIA_LNK_FL_ENABLED) {
+ if (is_svideo) {
+ decoder->pads_state[TVP5150_PAD_AIP1A] =
+ decoder->pads_state[TVP5150_PAD_AIP1B] =
+ TVP5150_PAD_ACTIVE_SVIDEO;
+ decoder->input = TVP5150_SVIDEO;
+ } else {
+ if (port == 0) {
+ decoder->pads_state[TVP5150_PAD_AIP1A] =
+ TVP5150_PAD_ACTIVE_COMPOSITE;
+ decoder->input = TVP5150_COMPOSITE0;
+ } else {
+ decoder->pads_state[TVP5150_PAD_AIP1B] =
+ TVP5150_PAD_ACTIVE_COMPOSITE;
+ decoder->input = TVP5150_COMPOSITE1;
+ }
+ }
+ tvp5150_selmux(sd);
+ decoder->modify_second_link = false;
+ }
+ }
+#endif
+ return 0;
+}
+
+
/* ----------------------------------------------------------------------- */
static const struct v4l2_ctrl_ops tvp5150_ctrl_ops = {
@@ -1367,6 +1614,10 @@ static const struct v4l2_subdev_ops tvp5150_ops = {
.pad = &tvp5150_pad_ops,
};
+static const struct v4l2_subdev_internal_ops tvp5150_internal_ops = {
+ .registered = tvp5150_registered,
+};
+
/****************************************************************************
I2C Client & Driver
****************************************************************************/
@@ -1515,38 +1766,197 @@ static int tvp5150_init(struct i2c_client *c)
return 0;
}
-static int tvp5150_parse_dt(struct tvp5150 *decoder, struct device_node *np)
+#if defined(CONFIG_MEDIA_CONTROLLER)
+static int tvp5150_add_of_connectors(struct tvp5150 *decoder)
{
- struct v4l2_fwnode_endpoint bus_cfg = { .bus_type = 0 };
- struct device_node *ep;
- unsigned int flags;
- int ret = 0;
+ struct device *dev = decoder->sd.dev;
+ struct device_node *rp;
+ struct of_endpoint ep;
+ struct tvp5150_connector *connectors;
+ unsigned int connectors_num = decoder->connectors_num;
+ int i, ret;
+
+ /* Allocate and initialize all available input connectors */
+ connectors = devm_kcalloc(dev, connectors_num, sizeof(*connectors),
+ GFP_KERNEL);
+ if (!connectors)
+ return -ENOMEM;
- ep = of_graph_get_next_endpoint(np, NULL);
- if (!ep)
- return -EINVAL;
+ for (i = 0; i < connectors_num; i++) {
+ rp = of_graph_get_remote_port_parent(decoder->endpoints[i]);
+ of_graph_parse_endpoint(decoder->endpoints[i], &ep);
+ connectors[i].port_num = ep.port;
+ connectors[i].is_svideo = !!of_device_is_compatible(rp,
+ "svideo-connector");
- ret = v4l2_fwnode_endpoint_parse(of_fwnode_handle(ep), &bus_cfg);
- if (ret)
- goto err;
+ if (connectors[i].is_svideo)
+ connectors[i].ent.function = MEDIA_ENT_F_CONN_SVIDEO;
+ else
+ connectors[i].ent.function = MEDIA_ENT_F_CONN_COMPOSITE;
+
+ connectors[i].ent.flags = MEDIA_ENT_FL_CONNECTOR;
+ ret = of_property_read_string(rp, "label",
+ &connectors[i].ent.name);
+ if (ret < 0)
+ return ret;
+ }
+
+ decoder->connectors = connectors;
+
+ return 0;
+}
+#endif
+
+static int tvp5150_mc_init(struct v4l2_subdev *sd)
+{
+#if defined(CONFIG_MEDIA_CONTROLLER)
+ struct tvp5150 *decoder = to_tvp5150(sd);
+ unsigned int i;
+ int ret;
+
+ sd->entity.ops = &tvp5150_sd_media_ops;
+ sd->entity.function = MEDIA_ENT_F_ATV_DECODER;
- flags = bus_cfg.bus.parallel.flags;
+ /* Initialize all TVP5150 pads */
+ for (i = 0; i < TVP5150_NUM_PADS; i++) {
+ if (i < TVP5150_NUM_PADS - 1) {
+ decoder->pads[i].flags = MEDIA_PAD_FL_SINK;
+ decoder->pads[i].sig_type = PAD_SIGNAL_ANALOG;
+ } else {
+ decoder->pads[i].flags = MEDIA_PAD_FL_SOURCE;
+ decoder->pads[i].sig_type = PAD_SIGNAL_DV;
+ }
+ }
+ ret = media_entity_pads_init(&sd->entity, TVP5150_NUM_PADS,
+ decoder->pads);
+ if (ret < 0)
+ goto out;
- if (bus_cfg.bus_type == V4L2_MBUS_PARALLEL &&
- !(flags & V4L2_MBUS_HSYNC_ACTIVE_HIGH &&
- flags & V4L2_MBUS_VSYNC_ACTIVE_HIGH &&
- flags & V4L2_MBUS_FIELD_EVEN_LOW)) {
+ if (IS_ENABLED(CONFIG_OF))
+ ret = tvp5150_add_of_connectors(decoder);
+#endif
+out:
+ return ret;
+}
+
+static bool tvp5150_of_valid_input(struct device_node *endpoint,
+ unsigned int port, unsigned int id)
+{
+ struct device_node *rp = of_graph_get_remote_port_parent(endpoint);
+ const char *input;
+ int ret;
+
+ /* perform some basic checks needed for later mc_init */
+ switch (port) {
+ case TVP5150_PAD_AIP1A:
+ /* svideo must be connected to endpoint@1 */
+ ret = id ? of_device_is_compatible(rp, "svideo-connector") :
+ of_device_is_compatible(rp,
+ "composite-video-connector");
+ if (!ret)
+ return false;
+ break;
+ case TVP5150_PAD_AIP1B:
+ ret = of_device_is_compatible(rp, "composite-video-connector");
+ if (!ret)
+ return false;
+ break;
+ }
+
+ ret = of_property_read_string(rp, "label", &input);
+ if (ret < 0)
+ return false;
+
+ return true;
+}
+
+static int tvp5150_parse_dt(struct tvp5150 *decoder, struct device_node *np)
+{
+ struct device *dev = decoder->sd.dev;
+ struct v4l2_fwnode_endpoint bus_cfg = { .bus_type = 0 };
+ struct device_node *ep_np;
+ unsigned int flags;
+ int ret, i = 0, in = 0;
+ bool found = false;
+
+ /* at least 1 output and 1 input */
+ decoder->endpoints_num = of_graph_get_endpoint_count(np);
+ if (decoder->endpoints_num < 2 || decoder->endpoints_num > 4) {
ret = -EINVAL;
goto err;
}
- decoder->mbus_type = bus_cfg.bus_type;
+ for_each_endpoint_of_node(np, ep_np) {
+ struct of_endpoint ep;
+
+ of_graph_parse_endpoint(ep_np, &ep);
+ if (decoder->endpoints[i]) {
+ /* this should never happen */
+ dev_err(dev, "Invalid endpoint %pOF on port %d\n",
+ ep.local_node, ep.port);
+ ret = -EINVAL;
+ goto err;
+ }
+ switch (ep.port) {
+ /* fall through */
+ case TVP5150_PAD_AIP1A:
+ case TVP5150_PAD_AIP1B:
+ if (!tvp5150_of_valid_input(ep_np, ep.port, ep.id)) {
+ dev_err(dev,
+ "Invalid endpoint %pOF on port %d\n",
+ ep.local_node, ep.port);
+ ret = -EINVAL;
+ goto err;
+ }
+ in++;
+ break;
+ case TVP5150_PAD_VID_OUT:
+ ret = v4l2_fwnode_endpoint_parse(of_fwnode_handle(ep_np),
+ &bus_cfg);
+ if (ret)
+ goto err;
+
+ flags = bus_cfg.bus.parallel.flags;
+
+ if (bus_cfg.bus_type == V4L2_MBUS_PARALLEL &&
+ !(flags & V4L2_MBUS_HSYNC_ACTIVE_HIGH &&
+ flags & V4L2_MBUS_VSYNC_ACTIVE_HIGH &&
+ flags & V4L2_MBUS_FIELD_EVEN_LOW)) {
+ ret = -EINVAL;
+ goto err;
+ }
+
+ decoder->mbus_type = bus_cfg.bus_type;
+ break;
+ default:
+ dev_err(dev, "Invalid port %d for endpoint %pOF\n",
+ ep.port, ep.local_node);
+ ret = -EINVAL;
+ goto err;
+ }
+
+ of_node_get(ep_np);
+ decoder->endpoints[i] = ep_np;
+ i++;
+
+ found = true;
+ }
+
+ decoder->connectors_num = in;
+ return found ? 0 : -ENODEV;
err:
- of_node_put(ep);
return ret;
}
+static void tvp5150_dt_cleanup(struct tvp5150 *decoder)
+{
+ unsigned int i;
+
+ for (i = 0; i < TVP5150_NUM_PADS; i++)
+ of_node_put(decoder->endpoints[i]);
+}
+
static const char * const tvp5150_test_patterns[2] = {
"Disabled",
"Black screen"
@@ -1585,7 +1995,7 @@ static int tvp5150_probe(struct i2c_client *c,
res = tvp5150_parse_dt(core, np);
if (res) {
dev_err(sd->dev, "DT parsing error: %d\n", res);
- return res;
+ goto err_cleanup_dt;
}
} else {
/* Default to BT.656 embedded sync */
@@ -1593,25 +2003,16 @@ static int tvp5150_probe(struct i2c_client *c,
}
v4l2_i2c_subdev_init(sd, c, &tvp5150_ops);
+ sd->internal_ops = &tvp5150_internal_ops;
sd->flags |= V4L2_SUBDEV_FL_HAS_DEVNODE;
-#if defined(CONFIG_MEDIA_CONTROLLER)
- core->pads[TVP5150_PAD_IF_INPUT].flags = MEDIA_PAD_FL_SINK;
- core->pads[TVP5150_PAD_IF_INPUT].sig_type = PAD_SIGNAL_ANALOG;
- core->pads[TVP5150_PAD_VID_OUT].flags = MEDIA_PAD_FL_SOURCE;
- core->pads[TVP5150_PAD_VID_OUT].sig_type = PAD_SIGNAL_DV;
-
- sd->entity.function = MEDIA_ENT_F_ATV_DECODER;
-
- res = media_entity_pads_init(&sd->entity, TVP5150_NUM_PADS, core->pads);
- if (res < 0)
- return res;
-
-#endif
+ res = tvp5150_mc_init(sd);
+ if (res)
+ goto err_cleanup_dt;
res = tvp5150_detect_version(core);
if (res < 0)
- return res;
+ goto err_cleanup_dt;
core->norm = V4L2_STD_ALL; /* Default is autodetect */
core->detected_norm = V4L2_STD_UNKNOWN;
@@ -1637,7 +2038,7 @@ static int tvp5150_probe(struct i2c_client *c,
sd->ctrl_handler = &core->hdl;
if (core->hdl.error) {
res = core->hdl.error;
- goto err;
+ goto err_free_v4l2_ctrls;
}
tvp5150_set_default(tvp5150_read_std(sd), &core->rect);
@@ -1649,19 +2050,21 @@ static int tvp5150_probe(struct i2c_client *c,
tvp5150_isr, IRQF_TRIGGER_HIGH |
IRQF_ONESHOT, "tvp5150", core);
if (res)
- goto err;
+ goto err_free_v4l2_ctrls;
}
res = v4l2_async_register_subdev(sd);
if (res < 0)
- goto err;
+ goto err_free_v4l2_ctrls;
if (debug > 1)
tvp5150_log_status(sd);
return 0;
-err:
+err_free_v4l2_ctrls:
v4l2_ctrl_handler_free(&core->hdl);
+err_cleanup_dt:
+ tvp5150_dt_cleanup(core);
return res;
}
--
2.20.1
^ permalink raw reply related [flat|nested] 14+ messages in thread* Re: [PATCH v4 2/7] media: tvp5150: add input source selection of_graph support
2019-01-29 16:07 ` [PATCH v4 2/7] media: tvp5150: add input source selection of_graph support Marco Felsch
@ 2019-03-21 13:03 ` Jacopo Mondi
2019-03-27 16:22 ` Marco Felsch
0 siblings, 1 reply; 14+ messages in thread
From: Jacopo Mondi @ 2019-03-21 13:03 UTC (permalink / raw)
To: Marco Felsch
Cc: mchehab, robh+dt, mark.rutland, sakari.ailus, devicetree,
p.zabel, javierm, afshin.nasser, laurent.pinchart, linux-media,
kernel
[-- Attachment #1: Type: text/plain, Size: 21439 bytes --]
On Tue, Jan 29, 2019 at 05:07:52PM +0100, Marco Felsch wrote:
> This patch adds the of_graph support to describe the tvp connections.
> Physical the TVP5150 has three ports: AIP1A, AIP1B and YOUT. As result
> of discussion [1],[2] the device-tree maps these ports 1:1. The svideo
> connector must be conneted to port@0/endpoint@1, look at the Documentation
> for more information. Since the TVP5150 is a converter the device-tree
> must contain at least 1-input and 1-output port. The mc-connectors and
> mc-links are only created if the device-tree contains the corresponding
> connector nodes. If more than one connector is available the
> media_entity_operations.link_setup() callback ensures that only one
> connector is active.
>
> [1] https://www.spinics.net/lists/linux-media/msg138545.html
> [2] https://www.spinics.net/lists/linux-media/msg138546.html
>
> Signed-off-by: Marco Felsch <m.felsch@pengutronix.de>
> ---
> Changelog:
>
> v4:
> - rebase on top of media_tree/master, fix merge conflict due to commit
> 60359a28d592 ("media: v4l: fwnode: Initialise the V4L2 fwnode endpoints
> to zero")
>
> v3:
> - probe(): s/err/err_free_v4l2_ctrls
> - drop MC dependency for tvp5150_pads
>
> v2:
> - adapt commit message
> - unify ifdef switches
> - rename tvp5150_valid_input -> tvp5150_of_valid_input, to be more precise
> - mc: use 2-input and 1-output pad
> - mc: link svideo connector to both input pads
> - mc: enable/disable svideo links in one go
> - mc: change link_setup() behaviour, switch the input src don't require a
> explicite disable before.
> - mc: rename 'local' media_pad param to tvp5150_pad to avoid confusion
> - mc: enable link to the first available connector and set the
> corresponding tvp5150 input src per default during registered()
> - mc/of: factor out oftree connector allocation
> - of: drop svideo dt port
> - of: move svideo connector to port@0/endpoint@1
> - of: require at least 1-in and 1-out endpoint
>
> drivers/media/i2c/tvp5150.c | 481 +++++++++++++++++++++++++++++++++---
> 1 file changed, 442 insertions(+), 39 deletions(-)
>
> diff --git a/drivers/media/i2c/tvp5150.c b/drivers/media/i2c/tvp5150.c
> index 89da921c8886..a89b83f69266 100644
> --- a/drivers/media/i2c/tvp5150.c
> +++ b/drivers/media/i2c/tvp5150.c
> @@ -44,15 +44,38 @@ MODULE_PARM_DESC(debug, "Debug level (0-2)");
> #define dprintk0(__dev, __arg...) dev_dbg_lvl(__dev, 0, 0, __arg)
>
> enum tvp5150_pads {
> - TVP5150_PAD_IF_INPUT,
> + TVP5150_PAD_AIP1A = TVP5150_COMPOSITE0,
> + TVP5150_PAD_AIP1B,
> TVP5150_PAD_VID_OUT,
> TVP5150_NUM_PADS
> };
>
> +#if defined(CONFIG_MEDIA_CONTROLLER)
> +enum tvp5150_pads_state {
> + TVP5150_PAD_INACTIVE,
> + TVP5150_PAD_ACTIVE_COMPOSITE,
> + TVP5150_PAD_ACTIVE_SVIDEO,
> +};
> +
> +struct tvp5150_connector {
> + struct media_entity ent;
> + struct media_pad pad;
> + unsigned int port_num;
> + bool is_svideo;
> +};
> +#endif
> +
> struct tvp5150 {
> struct v4l2_subdev sd;
> -#ifdef CONFIG_MEDIA_CONTROLLER
> + /* additional additional endpoint for the svideo connector */
> + struct device_node *endpoints[TVP5150_NUM_PADS + 1];
> + unsigned int endpoints_num;
> +#if defined(CONFIG_MEDIA_CONTROLLER)
> struct media_pad pads[TVP5150_NUM_PADS];
> + int pads_state[TVP5150_NUM_PADS];
> + struct tvp5150_connector *connectors;
> + int connectors_num;
> + bool modify_second_link;
> #endif
> struct v4l2_ctrl_handler hdl;
> struct v4l2_rect rect;
> @@ -1167,6 +1190,160 @@ static int tvp5150_enum_frame_size(struct v4l2_subdev *sd,
> return 0;
> }
>
> +/****************************************************************************
> + * Media entity ops
> + ****************************************************************************/
> +#if defined(CONFIG_MEDIA_CONTROLLER)
> +static int tvp5150_active_pad_idx(struct tvp5150 *decoder)
> +{
> + int *pad_state = &decoder->pads_state[0];
> + int i, idx = -1;
> +
> + for (i = 0; i < TVP5150_NUM_PADS - 1; i++) {
> + if ((pad_state[i] == TVP5150_PAD_ACTIVE_COMPOSITE) ||
> + (pad_state[i] == TVP5150_PAD_ACTIVE_SVIDEO)) {
> + idx = i;
> + break;
> + }
> + }
> +
> + return idx;
> +}
> +
> +static int tvp5150_active_svideo_links(struct tvp5150 *decoder)
> +{
> + int *pad_state = &decoder->pads_state[0];
> + int i, links = 0;
> +
> + for (i = 0; i < TVP5150_NUM_PADS - 1; i++)
> + if (pad_state[i] == TVP5150_PAD_ACTIVE_SVIDEO)
> + links++;
> +
> + return links;
> +}
> +
> +static int tvp5150_modify_link(struct tvp5150 *decoder, unsigned int pad_idx,
> + struct media_pad *pad, int flags)
> +{
> + struct media_pad *src_pad;
> + struct media_link *link;
> +
> + if (pad)
> + src_pad = pad;
> + else
> + src_pad = media_entity_remote_pad(&decoder->pads[pad_idx]);
> +
> + if (!src_pad)
> + return -1;
> +
> + link = media_entity_find_link(src_pad,
> + &decoder->pads[pad_idx]);
> +
> + /*
> + * Don't use locked version, since we are running already within the
> + * media_entity_setup_link() context.
> + */
> + return __media_entity_setup_link(link, flags);
> +
> +}
> +
> +static int tvp5150_s_routing(struct v4l2_subdev *sd, u32 input, u32 output,
> + u32 config);
> +
> +static int tvp5150_link_setup(struct media_entity *entity,
> + const struct media_pad *tvp5150_pad,
> + const struct media_pad *remote, u32 flags)
> +{
> + struct v4l2_subdev *sd = media_entity_to_v4l2_subdev(entity);
> + struct tvp5150 *decoder = to_tvp5150(sd);
> + int *pad_state = &decoder->pads_state[0];
> + int i, active_pad, ret = 0;
> + bool is_svideo = false;
> +
> + /*
> + * The tvp state is determined by the enabled sink pad link.
> + * Enabling or disabling the source pad link has no effect.
> + */
> + if (tvp5150_pad->flags & MEDIA_PAD_FL_SOURCE)
> + return 0;
> +
> + /* check if the svideo connector should be enabled */
> + for (i = 0; i < decoder->connectors_num; i++) {
> + if (remote->entity == &decoder->connectors[i].ent) {
> + is_svideo = decoder->connectors[i].is_svideo;
> + break;
> + }
> + }
> +
> + /* check if there is already a enabled svideo link and determine pad */
> + active_pad = tvp5150_active_pad_idx(decoder);
could this return -1 ? Is this an error?
> +
> + dev_dbg(sd->dev, "link setup '%s':%d->'%s':%d[%d]",
> + remote->entity->name, remote->index, tvp5150_pad->entity->name,
> + tvp5150_pad->index, flags & MEDIA_LNK_FL_ENABLED);
> +
> + if (flags & MEDIA_LNK_FL_ENABLED) {
> + if (active_pad >= 0 && !decoder->modify_second_link)
> + tvp5150_modify_link(decoder, active_pad, NULL, 0);
> +
> + dev_dbg(sd->dev, "Setting %d active [%s]\n", tvp5150_pad->index,
> + is_svideo ? "svideo" : "composite");
> + pad_state[tvp5150_pad->index] =
> + is_svideo ? TVP5150_PAD_ACTIVE_SVIDEO :
> + TVP5150_PAD_ACTIVE_COMPOSITE;
> +
> + if (is_svideo) {
> + if (tvp5150_active_svideo_links(decoder) < 2) {
> + unsigned int idx = tvp5150_pad->index ^ 1;
> +
> + decoder->modify_second_link = true;
> + tvp5150_modify_link(decoder, idx,
> + (struct media_pad *)remote,
> + MEDIA_LNK_FL_ENABLED);
> + } else {
> + decoder->modify_second_link = false;
> + tvp5150_s_routing(sd, TVP5150_SVIDEO,
> + TVP5150_NORMAL, 0);
> + }
> + } else {
> + tvp5150_s_routing(sd, tvp5150_pad->index,
> + TVP5150_NORMAL, 0);
> + }
I admit I didn't get how you use modify_second_link and s_routing(),
so I'll just assume it's correct :)
> + } else {
> + /*
> + * Svideo streams on two pads and user can request to AIP1A or
> + * AIP1B pad. In either case both pads gets disabled in in go.
> + * So check only if user wants to disable a not enabled
> + * composite pad.
> + */
> + if (!is_svideo && tvp5150_pad->index != active_pad)
> + goto out;
> +
> + dev_dbg(sd->dev, "going inactive\n");
> + pad_state[tvp5150_pad->index] = TVP5150_PAD_INACTIVE;
> +
> + /* in case of svideo we need to disable the second pad too */
> + if (tvp5150_active_svideo_links(decoder) > 0) {
No need for "> 0"
> + unsigned int idx = tvp5150_pad->index ^ 1;
> +
> + decoder->modify_second_link = true;
> + tvp5150_modify_link(decoder, idx,
> + (struct media_pad *)remote, 0);
> + }
> +
> + if (!decoder->modify_second_link)
> + tvp5150_s_routing(sd, is_svideo ? TVP5150_SVIDEO :
> + active_pad, TVP5150_BLACK_SCREEN, 0);
> + decoder->modify_second_link = false;
> + }
> +out:
> + return ret;
> +}
> +
> +static const struct media_entity_operations tvp5150_sd_media_ops = {
> + .link_setup = tvp5150_link_setup,
> +};
> +#endif
> /****************************************************************************
> I2C Command
> ****************************************************************************/
> @@ -1314,6 +1491,76 @@ static int tvp5150_g_tuner(struct v4l2_subdev *sd, struct v4l2_tuner *vt)
> return 0;
> }
>
> +static int tvp5150_registered(struct v4l2_subdev *sd)
> +{
> +#if defined(CONFIG_MEDIA_CONTROLLER)
> + struct tvp5150 *decoder = to_tvp5150(sd);
> + unsigned int i;
> + int ret;
> +
> + /*
> + * Setup connector pads and links. Enable the link to the first
> + * available connector per default.
> + */
> + for (i = 0; i < decoder->connectors_num; i++) {
> + struct media_entity *con = &decoder->connectors[i].ent;
> + struct media_pad *pad = &decoder->connectors[i].pad;
> + unsigned int port = decoder->connectors[i].port_num;
> + bool is_svideo = decoder->connectors[i].is_svideo;
> + int flags = i ? 0 : MEDIA_LNK_FL_ENABLED;
> +
> + pad->flags = MEDIA_PAD_FL_SOURCE;
> + ret = media_entity_pads_init(con, 1, pad);
> + if (ret < 0)
> + return ret;
> +
> + ret = media_device_register_entity(sd->v4l2_dev->mdev, con);
> + if (ret < 0)
> + return ret;
> +
> + ret = media_create_pad_link(con, 0, &sd->entity, port, flags);
> + if (ret < 0) {
> + media_device_unregister_entity(con);
> + return ret;
> + }
> +
> + if (is_svideo) {
> + /* svideo links to both aip1a and aip1b */
> + ret = media_create_pad_link(con, 0, &sd->entity,
> + port + 1, flags);
This would link to aip1b endpoint@0, right? It seems from your DT
bindings example that it is accepted to have aip1b with a composite
endpoint and an s_video endpoint@1 in aip1a, isn't it?
> + if (ret < 0) {
> + media_device_unregister_entity(con);
> + return ret;
> + }
> + }
> +
> + /* enable default input */
> + if (flags == MEDIA_LNK_FL_ENABLED) {
> + if (is_svideo) {
> + decoder->pads_state[TVP5150_PAD_AIP1A] =
> + decoder->pads_state[TVP5150_PAD_AIP1B] =
> + TVP5150_PAD_ACTIVE_SVIDEO;
> + decoder->input = TVP5150_SVIDEO;
> + } else {
> + if (port == 0) {
> + decoder->pads_state[TVP5150_PAD_AIP1A] =
> + TVP5150_PAD_ACTIVE_COMPOSITE;
> + decoder->input = TVP5150_COMPOSITE0;
> + } else {
> + decoder->pads_state[TVP5150_PAD_AIP1B] =
> + TVP5150_PAD_ACTIVE_COMPOSITE;
> + decoder->input = TVP5150_COMPOSITE1;
> + }
> + }
> + tvp5150_selmux(sd);
> + decoder->modify_second_link = false;
> + }
> + }
> +#endif
> + return 0;
> +}
> +
> +
> /* ----------------------------------------------------------------------- */
>
> static const struct v4l2_ctrl_ops tvp5150_ctrl_ops = {
> @@ -1367,6 +1614,10 @@ static const struct v4l2_subdev_ops tvp5150_ops = {
> .pad = &tvp5150_pad_ops,
> };
>
> +static const struct v4l2_subdev_internal_ops tvp5150_internal_ops = {
> + .registered = tvp5150_registered,
> +};
> +
> /****************************************************************************
> I2C Client & Driver
> ****************************************************************************/
> @@ -1515,38 +1766,197 @@ static int tvp5150_init(struct i2c_client *c)
> return 0;
> }
>
> -static int tvp5150_parse_dt(struct tvp5150 *decoder, struct device_node *np)
> +#if defined(CONFIG_MEDIA_CONTROLLER)
> +static int tvp5150_add_of_connectors(struct tvp5150 *decoder)
> {
> - struct v4l2_fwnode_endpoint bus_cfg = { .bus_type = 0 };
> - struct device_node *ep;
> - unsigned int flags;
> - int ret = 0;
> + struct device *dev = decoder->sd.dev;
> + struct device_node *rp;
> + struct of_endpoint ep;
> + struct tvp5150_connector *connectors;
> + unsigned int connectors_num = decoder->connectors_num;
> + int i, ret;
> +
> + /* Allocate and initialize all available input connectors */
> + connectors = devm_kcalloc(dev, connectors_num, sizeof(*connectors),
> + GFP_KERNEL);
> + if (!connectors)
> + return -ENOMEM;
>
> - ep = of_graph_get_next_endpoint(np, NULL);
> - if (!ep)
> - return -EINVAL;
> + for (i = 0; i < connectors_num; i++) {
> + rp = of_graph_get_remote_port_parent(decoder->endpoints[i]);
> + of_graph_parse_endpoint(decoder->endpoints[i], &ep);
> + connectors[i].port_num = ep.port;
> + connectors[i].is_svideo = !!of_device_is_compatible(rp,
> + "svideo-connector");
>
> - ret = v4l2_fwnode_endpoint_parse(of_fwnode_handle(ep), &bus_cfg);
> - if (ret)
> - goto err;
> + if (connectors[i].is_svideo)
> + connectors[i].ent.function = MEDIA_ENT_F_CONN_SVIDEO;
> + else
> + connectors[i].ent.function = MEDIA_ENT_F_CONN_COMPOSITE;
> +
> + connectors[i].ent.flags = MEDIA_ENT_FL_CONNECTOR;
> + ret = of_property_read_string(rp, "label",
> + &connectors[i].ent.name);
> + if (ret < 0)
> + return ret;
> + }
> +
> + decoder->connectors = connectors;
> +
> + return 0;
> +}
> +#endif
> +
> +static int tvp5150_mc_init(struct v4l2_subdev *sd)
> +{
> +#if defined(CONFIG_MEDIA_CONTROLLER)
> + struct tvp5150 *decoder = to_tvp5150(sd);
> + unsigned int i;
> + int ret;
> +
> + sd->entity.ops = &tvp5150_sd_media_ops;
> + sd->entity.function = MEDIA_ENT_F_ATV_DECODER;
>
> - flags = bus_cfg.bus.parallel.flags;
> + /* Initialize all TVP5150 pads */
> + for (i = 0; i < TVP5150_NUM_PADS; i++) {
> + if (i < TVP5150_NUM_PADS - 1) {
> + decoder->pads[i].flags = MEDIA_PAD_FL_SINK;
> + decoder->pads[i].sig_type = PAD_SIGNAL_ANALOG;
> + } else {
> + decoder->pads[i].flags = MEDIA_PAD_FL_SOURCE;
> + decoder->pads[i].sig_type = PAD_SIGNAL_DV;
> + }
> + }
> + ret = media_entity_pads_init(&sd->entity, TVP5150_NUM_PADS,
> + decoder->pads);
> + if (ret < 0)
> + goto out;
>
> - if (bus_cfg.bus_type == V4L2_MBUS_PARALLEL &&
> - !(flags & V4L2_MBUS_HSYNC_ACTIVE_HIGH &&
> - flags & V4L2_MBUS_VSYNC_ACTIVE_HIGH &&
> - flags & V4L2_MBUS_FIELD_EVEN_LOW)) {
> + if (IS_ENABLED(CONFIG_OF))
> + ret = tvp5150_add_of_connectors(decoder);
> +#endif
> +out:
> + return ret;
if CONFIG_MEDIA_CONTROLLER is not enabled, ret is not defined.
Also please consider making a stub in the #else branch where you just
return 0;
As a general note, try to build without media controller support, I
see a few errors.
> +}
> +
> +static bool tvp5150_of_valid_input(struct device_node *endpoint,
> + unsigned int port, unsigned int id)
> +{
> + struct device_node *rp = of_graph_get_remote_port_parent(endpoint);
> + const char *input;
> + int ret;
> +
> + /* perform some basic checks needed for later mc_init */
> + switch (port) {
> + case TVP5150_PAD_AIP1A:
> + /* svideo must be connected to endpoint@1 */
> + ret = id ? of_device_is_compatible(rp, "svideo-connector") :
> + of_device_is_compatible(rp,
> + "composite-video-connector");
> + if (!ret)
> + return false;
> + break;
> + case TVP5150_PAD_AIP1B:
> + ret = of_device_is_compatible(rp, "composite-video-connector");
> + if (!ret)
> + return false;
> + break;
> + }
> +
> + ret = of_property_read_string(rp, "label", &input);
> + if (ret < 0)
> + return false;
What are you using 'input' for, to make sure the property is there as
you use it later? Be aware that "label" is not mandatory for
connectors, and you need it only when media controller is enabled. If
you fail here, it will fail also for non-media controller devices that
do not need it. I would drop the check.
> +
> + return true;
> +}
> +
> +static int tvp5150_parse_dt(struct tvp5150 *decoder, struct device_node *np)
> +{
> + struct device *dev = decoder->sd.dev;
> + struct v4l2_fwnode_endpoint bus_cfg = { .bus_type = 0 };
please use V4L2_MBUS_UNKNOWN instead of 0
> + struct device_node *ep_np;
> + unsigned int flags;
> + int ret, i = 0, in = 0;
> + bool found = false;
> +
> + /* at least 1 output and 1 input */
Nitpicking: some comments start with lowercase other with upper case.
Also, sometimes there's a full stop at the end of the statement,
sometimes not.
> + decoder->endpoints_num = of_graph_get_endpoint_count(np);
> + if (decoder->endpoints_num < 2 || decoder->endpoints_num > 4) {
I would print an error, as bindings are wrong and this should appear
in boot logs.
> ret = -EINVAL;
> goto err;
> }
>
> - decoder->mbus_type = bus_cfg.bus_type;
> + for_each_endpoint_of_node(np, ep_np) {
> + struct of_endpoint ep;
> +
> + of_graph_parse_endpoint(ep_np, &ep);
> + if (decoder->endpoints[i]) {
Can you trigger this error condition with wrong bindings? I don't
think so. This is a development error, and if your code is correct,
this won't happen. I would drop the check, if I'm not mistaken.
> + /* this should never happen */
> + dev_err(dev, "Invalid endpoint %pOF on port %d\n",
> + ep.local_node, ep.port);
> + ret = -EINVAL;
> + goto err;
> + }
>
> + switch (ep.port) {
> + /* fall through */
> + case TVP5150_PAD_AIP1A:
> + case TVP5150_PAD_AIP1B:
> + if (!tvp5150_of_valid_input(ep_np, ep.port, ep.id)) {
> + dev_err(dev,
> + "Invalid endpoint %pOF on port %d\n",
> + ep.local_node, ep.port);
> + ret = -EINVAL;
> + goto err;
> + }
> + in++;
> + break;
> + case TVP5150_PAD_VID_OUT:
> + ret = v4l2_fwnode_endpoint_parse(of_fwnode_handle(ep_np),
> + &bus_cfg);
> + goto err;
> +
> + flags = bus_cfg.bus.parallel.flags;
> +
> + if (bus_cfg.bus_type == V4L2_MBUS_PARALLEL &&
> + !(flags & V4L2_MBUS_HSYNC_ACTIVE_HIGH &&
> + flags & V4L2_MBUS_VSYNC_ACTIVE_HIGH &&
> + flags & V4L2_MBUS_FIELD_EVEN_LOW)) {
There's a bit of confusion in bindings related to this.
The properties are listed as required, then it says if none are
specified it is assumed embedded BT.656 sync, and indeed here you want
all of them to be present to accept the bus as parallel.
So you either make them optional, or use 'bus-type'
property and use the newly introduced bus hints to
v4l2_fwnode_endpoint_parse and try to parse
V4L2_MBUS_PARALLEL first, V4L2_MBUS_BT656 if parallel fails, and
return an error otherwise.
I think making the properties optional is the easiest way, you're
touching bindings anyway in the next patch.
Thanks
j
> + ret = -EINVAL;
> + goto err;
> + }
> +
> + decoder->mbus_type = bus_cfg.bus_type;
> + break;
> + default:
> + dev_err(dev, "Invalid port %d for endpoint %pOF\n",
> + ep.port, ep.local_node);
> + ret = -EINVAL;
> + goto err;
> + }
> +
> + of_node_get(ep_np);
> + decoder->endpoints[i] = ep_np;
> + i++;
> +
> + found = true;
> + }
> +
> + decoder->connectors_num = in;
> + return found ? 0 : -ENODEV;
> err:
> - of_node_put(ep);
> return ret;
> }
>
> +static void tvp5150_dt_cleanup(struct tvp5150 *decoder)
> +{
> + unsigned int i;
> +
> + for (i = 0; i < TVP5150_NUM_PADS; i++)
> + of_node_put(decoder->endpoints[i]);
> +}
> +
> static const char * const tvp5150_test_patterns[2] = {
> "Disabled",
> "Black screen"
> @@ -1585,7 +1995,7 @@ static int tvp5150_probe(struct i2c_client *c,
> res = tvp5150_parse_dt(core, np);
> if (res) {
> dev_err(sd->dev, "DT parsing error: %d\n", res);
> - return res;
> + goto err_cleanup_dt;
> }
> } else {
> /* Default to BT.656 embedded sync */
> @@ -1593,25 +2003,16 @@ static int tvp5150_probe(struct i2c_client *c,
> }
>
> v4l2_i2c_subdev_init(sd, c, &tvp5150_ops);
> + sd->internal_ops = &tvp5150_internal_ops;
> sd->flags |= V4L2_SUBDEV_FL_HAS_DEVNODE;
>
> -#if defined(CONFIG_MEDIA_CONTROLLER)
> - core->pads[TVP5150_PAD_IF_INPUT].flags = MEDIA_PAD_FL_SINK;
> - core->pads[TVP5150_PAD_IF_INPUT].sig_type = PAD_SIGNAL_ANALOG;
> - core->pads[TVP5150_PAD_VID_OUT].flags = MEDIA_PAD_FL_SOURCE;
> - core->pads[TVP5150_PAD_VID_OUT].sig_type = PAD_SIGNAL_DV;
> -
> - sd->entity.function = MEDIA_ENT_F_ATV_DECODER;
> -
> - res = media_entity_pads_init(&sd->entity, TVP5150_NUM_PADS, core->pads);
> - if (res < 0)
> - return res;
> -
> -#endif
> + res = tvp5150_mc_init(sd);
> + if (res)
> + goto err_cleanup_dt;
>
> res = tvp5150_detect_version(core);
> if (res < 0)
> - return res;
> + goto err_cleanup_dt;
>
> core->norm = V4L2_STD_ALL; /* Default is autodetect */
> core->detected_norm = V4L2_STD_UNKNOWN;
> @@ -1637,7 +2038,7 @@ static int tvp5150_probe(struct i2c_client *c,
> sd->ctrl_handler = &core->hdl;
> if (core->hdl.error) {
> res = core->hdl.error;
> - goto err;
> + goto err_free_v4l2_ctrls;
> }
>
> tvp5150_set_default(tvp5150_read_std(sd), &core->rect);
> @@ -1649,19 +2050,21 @@ static int tvp5150_probe(struct i2c_client *c,
> tvp5150_isr, IRQF_TRIGGER_HIGH |
> IRQF_ONESHOT, "tvp5150", core);
> if (res)
> - goto err;
> + goto err_free_v4l2_ctrls;
> }
>
> res = v4l2_async_register_subdev(sd);
> if (res < 0)
> - goto err;
> + goto err_free_v4l2_ctrls;
>
> if (debug > 1)
> tvp5150_log_status(sd);
> return 0;
>
> -err:
> +err_free_v4l2_ctrls:
> v4l2_ctrl_handler_free(&core->hdl);
> +err_cleanup_dt:
> + tvp5150_dt_cleanup(core);
> return res;
> }
>
> --
> 2.20.1
>
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply [flat|nested] 14+ messages in thread* Re: [PATCH v4 2/7] media: tvp5150: add input source selection of_graph support
2019-03-21 13:03 ` Jacopo Mondi
@ 2019-03-27 16:22 ` Marco Felsch
0 siblings, 0 replies; 14+ messages in thread
From: Marco Felsch @ 2019-03-27 16:22 UTC (permalink / raw)
To: Jacopo Mondi
Cc: mchehab, robh+dt, mark.rutland, sakari.ailus, devicetree,
p.zabel, javierm, afshin.nasser, laurent.pinchart, linux-media,
kernel
Hi Jacopo,
first thanks for your review and sorry for the delayed reply.
On 19-03-21 14:03, Jacopo Mondi wrote:
> On Tue, Jan 29, 2019 at 05:07:52PM +0100, Marco Felsch wrote:
> > This patch adds the of_graph support to describe the tvp connections.
> > Physical the TVP5150 has three ports: AIP1A, AIP1B and YOUT. As result
> > of discussion [1],[2] the device-tree maps these ports 1:1. The svideo
> > connector must be conneted to port@0/endpoint@1, look at the Documentation
> > for more information. Since the TVP5150 is a converter the device-tree
> > must contain at least 1-input and 1-output port. The mc-connectors and
> > mc-links are only created if the device-tree contains the corresponding
> > connector nodes. If more than one connector is available the
> > media_entity_operations.link_setup() callback ensures that only one
> > connector is active.
> >
> > [1] https://www.spinics.net/lists/linux-media/msg138545.html
> > [2] https://www.spinics.net/lists/linux-media/msg138546.html
> >
> > Signed-off-by: Marco Felsch <m.felsch@pengutronix.de>
> > ---
> > Changelog:
> >
> > v4:
> > - rebase on top of media_tree/master, fix merge conflict due to commit
> > 60359a28d592 ("media: v4l: fwnode: Initialise the V4L2 fwnode endpoints
> > to zero")
> >
> > v3:
> > - probe(): s/err/err_free_v4l2_ctrls
> > - drop MC dependency for tvp5150_pads
> >
> > v2:
> > - adapt commit message
> > - unify ifdef switches
> > - rename tvp5150_valid_input -> tvp5150_of_valid_input, to be more precise
> > - mc: use 2-input and 1-output pad
> > - mc: link svideo connector to both input pads
> > - mc: enable/disable svideo links in one go
> > - mc: change link_setup() behaviour, switch the input src don't require a
> > explicite disable before.
> > - mc: rename 'local' media_pad param to tvp5150_pad to avoid confusion
> > - mc: enable link to the first available connector and set the
> > corresponding tvp5150 input src per default during registered()
> > - mc/of: factor out oftree connector allocation
> > - of: drop svideo dt port
> > - of: move svideo connector to port@0/endpoint@1
> > - of: require at least 1-in and 1-out endpoint
> >
> > drivers/media/i2c/tvp5150.c | 481 +++++++++++++++++++++++++++++++++---
> > 1 file changed, 442 insertions(+), 39 deletions(-)
> >
> > diff --git a/drivers/media/i2c/tvp5150.c b/drivers/media/i2c/tvp5150.c
> > index 89da921c8886..a89b83f69266 100644
> > --- a/drivers/media/i2c/tvp5150.c
> > +++ b/drivers/media/i2c/tvp5150.c
> > @@ -44,15 +44,38 @@ MODULE_PARM_DESC(debug, "Debug level (0-2)");
> > #define dprintk0(__dev, __arg...) dev_dbg_lvl(__dev, 0, 0, __arg)
> >
> > enum tvp5150_pads {
> > - TVP5150_PAD_IF_INPUT,
> > + TVP5150_PAD_AIP1A = TVP5150_COMPOSITE0,
> > + TVP5150_PAD_AIP1B,
> > TVP5150_PAD_VID_OUT,
> > TVP5150_NUM_PADS
> > };
> >
> > +#if defined(CONFIG_MEDIA_CONTROLLER)
> > +enum tvp5150_pads_state {
> > + TVP5150_PAD_INACTIVE,
> > + TVP5150_PAD_ACTIVE_COMPOSITE,
> > + TVP5150_PAD_ACTIVE_SVIDEO,
> > +};
> > +
> > +struct tvp5150_connector {
> > + struct media_entity ent;
> > + struct media_pad pad;
> > + unsigned int port_num;
> > + bool is_svideo;
> > +};
> > +#endif
> > +
> > struct tvp5150 {
> > struct v4l2_subdev sd;
> > -#ifdef CONFIG_MEDIA_CONTROLLER
> > + /* additional additional endpoint for the svideo connector */
> > + struct device_node *endpoints[TVP5150_NUM_PADS + 1];
> > + unsigned int endpoints_num;
> > +#if defined(CONFIG_MEDIA_CONTROLLER)
> > struct media_pad pads[TVP5150_NUM_PADS];
> > + int pads_state[TVP5150_NUM_PADS];
> > + struct tvp5150_connector *connectors;
> > + int connectors_num;
> > + bool modify_second_link;
> > #endif
> > struct v4l2_ctrl_handler hdl;
> > struct v4l2_rect rect;
> > @@ -1167,6 +1190,160 @@ static int tvp5150_enum_frame_size(struct v4l2_subdev *sd,
> > return 0;
> > }
> >
> > +/****************************************************************************
> > + * Media entity ops
> > + ****************************************************************************/
> > +#if defined(CONFIG_MEDIA_CONTROLLER)
> > +static int tvp5150_active_pad_idx(struct tvp5150 *decoder)
> > +{
> > + int *pad_state = &decoder->pads_state[0];
> > + int i, idx = -1;
> > +
> > + for (i = 0; i < TVP5150_NUM_PADS - 1; i++) {
> > + if ((pad_state[i] == TVP5150_PAD_ACTIVE_COMPOSITE) ||
> > + (pad_state[i] == TVP5150_PAD_ACTIVE_SVIDEO)) {
> > + idx = i;
> > + break;
> > + }
> > + }
> > +
> > + return idx;
> > +}
> > +
> > +static int tvp5150_active_svideo_links(struct tvp5150 *decoder)
> > +{
> > + int *pad_state = &decoder->pads_state[0];
> > + int i, links = 0;
> > +
> > + for (i = 0; i < TVP5150_NUM_PADS - 1; i++)
> > + if (pad_state[i] == TVP5150_PAD_ACTIVE_SVIDEO)
> > + links++;
> > +
> > + return links;
> > +}
> > +
> > +static int tvp5150_modify_link(struct tvp5150 *decoder, unsigned int pad_idx,
> > + struct media_pad *pad, int flags)
> > +{
> > + struct media_pad *src_pad;
> > + struct media_link *link;
> > +
> > + if (pad)
> > + src_pad = pad;
> > + else
> > + src_pad = media_entity_remote_pad(&decoder->pads[pad_idx]);
> > +
> > + if (!src_pad)
> > + return -1;
> > +
> > + link = media_entity_find_link(src_pad,
> > + &decoder->pads[pad_idx]);
> > +
> > + /*
> > + * Don't use locked version, since we are running already within the
> > + * media_entity_setup_link() context.
> > + */
> > + return __media_entity_setup_link(link, flags);
> > +
> > +}
> > +
> > +static int tvp5150_s_routing(struct v4l2_subdev *sd, u32 input, u32 output,
> > + u32 config);
> > +
> > +static int tvp5150_link_setup(struct media_entity *entity,
> > + const struct media_pad *tvp5150_pad,
> > + const struct media_pad *remote, u32 flags)
> > +{
> > + struct v4l2_subdev *sd = media_entity_to_v4l2_subdev(entity);
> > + struct tvp5150 *decoder = to_tvp5150(sd);
> > + int *pad_state = &decoder->pads_state[0];
> > + int i, active_pad, ret = 0;
> > + bool is_svideo = false;
> > +
> > + /*
> > + * The tvp state is determined by the enabled sink pad link.
> > + * Enabling or disabling the source pad link has no effect.
> > + */
> > + if (tvp5150_pad->flags & MEDIA_PAD_FL_SOURCE)
> > + return 0;
> > +
> > + /* check if the svideo connector should be enabled */
> > + for (i = 0; i < decoder->connectors_num; i++) {
> > + if (remote->entity == &decoder->connectors[i].ent) {
> > + is_svideo = decoder->connectors[i].is_svideo;
> > + break;
> > + }
> > + }
> > +
> > + /* check if there is already a enabled svideo link and determine pad */
> > + active_pad = tvp5150_active_pad_idx(decoder);
>
> could this return -1 ? Is this an error?
Yes it could return -1. This indicates that no pads are active so it
isn't a error.
> > +
> > + dev_dbg(sd->dev, "link setup '%s':%d->'%s':%d[%d]",
> > + remote->entity->name, remote->index, tvp5150_pad->entity->name,
> > + tvp5150_pad->index, flags & MEDIA_LNK_FL_ENABLED);
> > +
> > + if (flags & MEDIA_LNK_FL_ENABLED) {
> > + if (active_pad >= 0 && !decoder->modify_second_link)
> > + tvp5150_modify_link(decoder, active_pad, NULL, 0);
> > +
> > + dev_dbg(sd->dev, "Setting %d active [%s]\n", tvp5150_pad->index,
> > + is_svideo ? "svideo" : "composite");
> > + pad_state[tvp5150_pad->index] =
> > + is_svideo ? TVP5150_PAD_ACTIVE_SVIDEO :
> > + TVP5150_PAD_ACTIVE_COMPOSITE;
> > +
> > + if (is_svideo) {
> > + if (tvp5150_active_svideo_links(decoder) < 2) {
> > + unsigned int idx = tvp5150_pad->index ^ 1;
> > +
> > + decoder->modify_second_link = true;
> > + tvp5150_modify_link(decoder, idx,
> > + (struct media_pad *)remote,
> > + MEDIA_LNK_FL_ENABLED);
> > + } else {
> > + decoder->modify_second_link = false;
> > + tvp5150_s_routing(sd, TVP5150_SVIDEO,
> > + TVP5150_NORMAL, 0);
> > + }
> > + } else {
> > + tvp5150_s_routing(sd, tvp5150_pad->index,
> > + TVP5150_NORMAL, 0);
> > + }
>
> I admit I didn't get how you use modify_second_link and s_routing(),
> so I'll just assume it's correct :)
Sorry for that.. I should add a comment here since it isn't intuitive.
My goal was to enable/disable both connector links in one shot in case
of s-video. I solved that using a recursion calling tvp5150_link_setup()
a second time. Maybe I have to reconsidering about that implementation
since it is a bit counter intuitve or at least adding a comment above.
> > + } else {
> > + /*
> > + * Svideo streams on two pads and user can request to AIP1A or
> > + * AIP1B pad. In either case both pads gets disabled in in go.
> > + * So check only if user wants to disable a not enabled
> > + * composite pad.
> > + */
> > + if (!is_svideo && tvp5150_pad->index != active_pad)
> > + goto out;
> > +
> > + dev_dbg(sd->dev, "going inactive\n");
> > + pad_state[tvp5150_pad->index] = TVP5150_PAD_INACTIVE;
> > +
> > + /* in case of svideo we need to disable the second pad too */
> > + if (tvp5150_active_svideo_links(decoder) > 0) {
>
> No need for "> 0"
Yes, I will change that.
> > + unsigned int idx = tvp5150_pad->index ^ 1;
> > +
> > + decoder->modify_second_link = true;
> > + tvp5150_modify_link(decoder, idx,
> > + (struct media_pad *)remote, 0);
> > + }
> > +
> > + if (!decoder->modify_second_link)
> > + tvp5150_s_routing(sd, is_svideo ? TVP5150_SVIDEO :
> > + active_pad, TVP5150_BLACK_SCREEN, 0);
> > + decoder->modify_second_link = false;
> > + }
> > +out:
> > + return ret;
> > +}
> > +
> > +static const struct media_entity_operations tvp5150_sd_media_ops = {
> > + .link_setup = tvp5150_link_setup,
> > +};
> > +#endif
> > /****************************************************************************
> > I2C Command
> > ****************************************************************************/
> > @@ -1314,6 +1491,76 @@ static int tvp5150_g_tuner(struct v4l2_subdev *sd, struct v4l2_tuner *vt)
> > return 0;
> > }
> >
> > +static int tvp5150_registered(struct v4l2_subdev *sd)
> > +{
> > +#if defined(CONFIG_MEDIA_CONTROLLER)
> > + struct tvp5150 *decoder = to_tvp5150(sd);
> > + unsigned int i;
> > + int ret;
> > +
> > + /*
> > + * Setup connector pads and links. Enable the link to the first
> > + * available connector per default.
> > + */
> > + for (i = 0; i < decoder->connectors_num; i++) {
> > + struct media_entity *con = &decoder->connectors[i].ent;
> > + struct media_pad *pad = &decoder->connectors[i].pad;
> > + unsigned int port = decoder->connectors[i].port_num;
> > + bool is_svideo = decoder->connectors[i].is_svideo;
> > + int flags = i ? 0 : MEDIA_LNK_FL_ENABLED;
> > +
> > + pad->flags = MEDIA_PAD_FL_SOURCE;
> > + ret = media_entity_pads_init(con, 1, pad);
> > + if (ret < 0)
> > + return ret;
> > +
> > + ret = media_device_register_entity(sd->v4l2_dev->mdev, con);
> > + if (ret < 0)
> > + return ret;
> > +
> > + ret = media_create_pad_link(con, 0, &sd->entity, port, flags);
> > + if (ret < 0) {
> > + media_device_unregister_entity(con);
> > + return ret;
> > + }
> > +
> > + if (is_svideo) {
> > + /* svideo links to both aip1a and aip1b */
> > + ret = media_create_pad_link(con, 0, &sd->entity,
> > + port + 1, flags);
>
> This would link to aip1b endpoint@0, right? It seems from your DT
> bindings example that it is accepted to have aip1b with a composite
> endpoint and an s_video endpoint@1 in aip1a, isn't it?
Yes and yes. The documentation layout is the result of a longer
port/endpoint discussion. In real the HW has only two input port (no
dedicated s-video port). The s-video "port" is archived enabling both
AIP1A and AIP1B port.
>
> > + if (ret < 0) {
> > + media_device_unregister_entity(con);
> > + return ret;
> > + }
> > + }
> > +
> > + /* enable default input */
> > + if (flags == MEDIA_LNK_FL_ENABLED) {
> > + if (is_svideo) {
> > + decoder->pads_state[TVP5150_PAD_AIP1A] =
> > + decoder->pads_state[TVP5150_PAD_AIP1B] =
> > + TVP5150_PAD_ACTIVE_SVIDEO;
> > + decoder->input = TVP5150_SVIDEO;
> > + } else {
> > + if (port == 0) {
> > + decoder->pads_state[TVP5150_PAD_AIP1A] =
> > + TVP5150_PAD_ACTIVE_COMPOSITE;
> > + decoder->input = TVP5150_COMPOSITE0;
> > + } else {
> > + decoder->pads_state[TVP5150_PAD_AIP1B] =
> > + TVP5150_PAD_ACTIVE_COMPOSITE;
> > + decoder->input = TVP5150_COMPOSITE1;
> > + }
> > + }
> > + tvp5150_selmux(sd);
> > + decoder->modify_second_link = false;
> > + }
> > + }
> > +#endif
> > + return 0;
> > +}
> > +
> > +
> > /* ----------------------------------------------------------------------- */
> >
> > static const struct v4l2_ctrl_ops tvp5150_ctrl_ops = {
> > @@ -1367,6 +1614,10 @@ static const struct v4l2_subdev_ops tvp5150_ops = {
> > .pad = &tvp5150_pad_ops,
> > };
> >
> > +static const struct v4l2_subdev_internal_ops tvp5150_internal_ops = {
> > + .registered = tvp5150_registered,
> > +};
> > +
> > /****************************************************************************
> > I2C Client & Driver
> > ****************************************************************************/
> > @@ -1515,38 +1766,197 @@ static int tvp5150_init(struct i2c_client *c)
> > return 0;
> > }
> >
> > -static int tvp5150_parse_dt(struct tvp5150 *decoder, struct device_node *np)
> > +#if defined(CONFIG_MEDIA_CONTROLLER)
> > +static int tvp5150_add_of_connectors(struct tvp5150 *decoder)
> > {
> > - struct v4l2_fwnode_endpoint bus_cfg = { .bus_type = 0 };
> > - struct device_node *ep;
> > - unsigned int flags;
> > - int ret = 0;
> > + struct device *dev = decoder->sd.dev;
> > + struct device_node *rp;
> > + struct of_endpoint ep;
> > + struct tvp5150_connector *connectors;
> > + unsigned int connectors_num = decoder->connectors_num;
> > + int i, ret;
> > +
> > + /* Allocate and initialize all available input connectors */
> > + connectors = devm_kcalloc(dev, connectors_num, sizeof(*connectors),
> > + GFP_KERNEL);
> > + if (!connectors)
> > + return -ENOMEM;
> >
> > - ep = of_graph_get_next_endpoint(np, NULL);
> > - if (!ep)
> > - return -EINVAL;
> > + for (i = 0; i < connectors_num; i++) {
> > + rp = of_graph_get_remote_port_parent(decoder->endpoints[i]);
> > + of_graph_parse_endpoint(decoder->endpoints[i], &ep);
> > + connectors[i].port_num = ep.port;
> > + connectors[i].is_svideo = !!of_device_is_compatible(rp,
> > + "svideo-connector");
> >
> > - ret = v4l2_fwnode_endpoint_parse(of_fwnode_handle(ep), &bus_cfg);
> > - if (ret)
> > - goto err;
> > + if (connectors[i].is_svideo)
> > + connectors[i].ent.function = MEDIA_ENT_F_CONN_SVIDEO;
> > + else
> > + connectors[i].ent.function = MEDIA_ENT_F_CONN_COMPOSITE;
> > +
> > + connectors[i].ent.flags = MEDIA_ENT_FL_CONNECTOR;
> > + ret = of_property_read_string(rp, "label",
> > + &connectors[i].ent.name);
> > + if (ret < 0)
> > + return ret;
> > + }
> > +
> > + decoder->connectors = connectors;
> > +
> > + return 0;
> > +}
> > +#endif
> > +
> > +static int tvp5150_mc_init(struct v4l2_subdev *sd)
> > +{
> > +#if defined(CONFIG_MEDIA_CONTROLLER)
> > + struct tvp5150 *decoder = to_tvp5150(sd);
> > + unsigned int i;
> > + int ret;
> > +
> > + sd->entity.ops = &tvp5150_sd_media_ops;
> > + sd->entity.function = MEDIA_ENT_F_ATV_DECODER;
> >
> > - flags = bus_cfg.bus.parallel.flags;
> > + /* Initialize all TVP5150 pads */
> > + for (i = 0; i < TVP5150_NUM_PADS; i++) {
> > + if (i < TVP5150_NUM_PADS - 1) {
> > + decoder->pads[i].flags = MEDIA_PAD_FL_SINK;
> > + decoder->pads[i].sig_type = PAD_SIGNAL_ANALOG;
> > + } else {
> > + decoder->pads[i].flags = MEDIA_PAD_FL_SOURCE;
> > + decoder->pads[i].sig_type = PAD_SIGNAL_DV;
> > + }
> > + }
> > + ret = media_entity_pads_init(&sd->entity, TVP5150_NUM_PADS,
> > + decoder->pads);
> > + if (ret < 0)
> > + goto out;
> >
> > - if (bus_cfg.bus_type == V4L2_MBUS_PARALLEL &&
> > - !(flags & V4L2_MBUS_HSYNC_ACTIVE_HIGH &&
> > - flags & V4L2_MBUS_VSYNC_ACTIVE_HIGH &&
> > - flags & V4L2_MBUS_FIELD_EVEN_LOW)) {
> > + if (IS_ENABLED(CONFIG_OF))
> > + ret = tvp5150_add_of_connectors(decoder);
> > +#endif
> > +out:
> > + return ret;
>
> if CONFIG_MEDIA_CONTROLLER is not enabled, ret is not defined.
> Also please consider making a stub in the #else branch where you just
> return 0;
You're absolutly right. I will change that, thanks.
>
> As a general note, try to build without media controller support, I
> see a few errors.
Okay, I'm trying that and fix those errors.
> > +}
> > +
> > +static bool tvp5150_of_valid_input(struct device_node *endpoint,
> > + unsigned int port, unsigned int id)
> > +{
> > + struct device_node *rp = of_graph_get_remote_port_parent(endpoint);
> > + const char *input;
> > + int ret;
> > +
> > + /* perform some basic checks needed for later mc_init */
> > + switch (port) {
> > + case TVP5150_PAD_AIP1A:
> > + /* svideo must be connected to endpoint@1 */
> > + ret = id ? of_device_is_compatible(rp, "svideo-connector") :
> > + of_device_is_compatible(rp,
> > + "composite-video-connector");
> > + if (!ret)
> > + return false;
> > + break;
> > + case TVP5150_PAD_AIP1B:
> > + ret = of_device_is_compatible(rp, "composite-video-connector");
> > + if (!ret)
> > + return false;
> > + break;
> > + }
> > +
> > + ret = of_property_read_string(rp, "label", &input);
> > + if (ret < 0)
> > + return false;
>
> What are you using 'input' for, to make sure the property is there as
> you use it later? Be aware that "label" is not mandatory for
> connectors, and you need it only when media controller is enabled. If
> you fail here, it will fail also for non-media controller devices that
> do not need it. I would drop the check.
You're right. The label is used later on to set the connector entity
name. There is a second patchset fyling around which introduces a
generic parsing routine which fixing this too. I should merge those
two sets into one for the next round.
> > +
> > + return true;
> > +}
> > +
> > +static int tvp5150_parse_dt(struct tvp5150 *decoder, struct device_node *np)
> > +{
> > + struct device *dev = decoder->sd.dev;
> > + struct v4l2_fwnode_endpoint bus_cfg = { .bus_type = 0 };
>
> please use V4L2_MBUS_UNKNOWN instead of 0
Okay.
> > + struct device_node *ep_np;
> > + unsigned int flags;
> > + int ret, i = 0, in = 0;
> > + bool found = false;
> > +
> > + /* at least 1 output and 1 input */
>
> Nitpicking: some comments start with lowercase other with upper case.
> Also, sometimes there's a full stop at the end of the statement,
> sometimes not.
Okay I will fix that too.
> > + decoder->endpoints_num = of_graph_get_endpoint_count(np);
> > + if (decoder->endpoints_num < 2 || decoder->endpoints_num > 4) {
>
> I would print an error, as bindings are wrong and this should appear
> in boot logs.
Okay.
> > ret = -EINVAL;
> > goto err;
> > }
> >
> > - decoder->mbus_type = bus_cfg.bus_type;
> > + for_each_endpoint_of_node(np, ep_np) {
> > + struct of_endpoint ep;
> > +
> > + of_graph_parse_endpoint(ep_np, &ep);
> > + if (decoder->endpoints[i]) {
>
> Can you trigger this error condition with wrong bindings? I don't
> think so. This is a development error, and if your code is correct,
> this won't happen. I would drop the check, if I'm not mistaken.
Yes, it's a dev error of course I can drop it to make it cleaner.
> > + /* this should never happen */
> > + dev_err(dev, "Invalid endpoint %pOF on port %d\n",
> > + ep.local_node, ep.port);
> > + ret = -EINVAL;
> > + goto err;
> > + }
> >
> > + switch (ep.port) {
> > + /* fall through */
> > + case TVP5150_PAD_AIP1A:
> > + case TVP5150_PAD_AIP1B:
> > + if (!tvp5150_of_valid_input(ep_np, ep.port, ep.id)) {
> > + dev_err(dev,
> > + "Invalid endpoint %pOF on port %d\n",
> > + ep.local_node, ep.port);
> > + ret = -EINVAL;
> > + goto err;
> > + }
> > + in++;
> > + break;
> > + case TVP5150_PAD_VID_OUT:
> > + ret = v4l2_fwnode_endpoint_parse(of_fwnode_handle(ep_np),
> > + &bus_cfg);
>
> > + goto err;
> > +
> > + flags = bus_cfg.bus.parallel.flags;
> > +
> > + if (bus_cfg.bus_type == V4L2_MBUS_PARALLEL &&
> > + !(flags & V4L2_MBUS_HSYNC_ACTIVE_HIGH &&
> > + flags & V4L2_MBUS_VSYNC_ACTIVE_HIGH &&
> > + flags & V4L2_MBUS_FIELD_EVEN_LOW)) {
>
> There's a bit of confusion in bindings related to this.
> The properties are listed as required, then it says if none are
> specified it is assumed embedded BT.656 sync, and indeed here you want
> all of them to be present to accept the bus as parallel.
Yes I was confused too during checking your feedback but then I covered
it hopefully right. This piece of code will only fail if we are in
V4L2_MBUS_PARALLEL mode (with discrete sync) because the sync signals are
pinned to that levels by the hardware. If non of these three properties
are specified the v4l2_fwnode_endpoint_parse() will set the bus_type to
V4L2_MBUS_BT656.
So I think the test it correct but maybe we need to adapt the bindings
a bit.
Regards,
Marco
> So you either make them optional, or use 'bus-type'
> property and use the newly introduced bus hints to
> v4l2_fwnode_endpoint_parse and try to parse
> V4L2_MBUS_PARALLEL first, V4L2_MBUS_BT656 if parallel fails, and
> return an error otherwise.
>
> I think making the properties optional is the easiest way, you're
> touching bindings anyway in the next patch.
>
> Thanks
> j
>
> > + ret = -EINVAL;
> > + goto err;
> > + }
> > +
> > + decoder->mbus_type = bus_cfg.bus_type;
> > + break;
> > + default:
> > + dev_err(dev, "Invalid port %d for endpoint %pOF\n",
> > + ep.port, ep.local_node);
> > + ret = -EINVAL;
> > + goto err;
> > + }
> > +
> > + of_node_get(ep_np);
> > + decoder->endpoints[i] = ep_np;
> > + i++;
> > +
> > + found = true;
> > + }
> > +
> > + decoder->connectors_num = in;
> > + return found ? 0 : -ENODEV;
> > err:
> > - of_node_put(ep);
> > return ret;
> > }
> >
> > +static void tvp5150_dt_cleanup(struct tvp5150 *decoder)
> > +{
> > + unsigned int i;
> > +
> > + for (i = 0; i < TVP5150_NUM_PADS; i++)
> > + of_node_put(decoder->endpoints[i]);
> > +}
> > +
> > static const char * const tvp5150_test_patterns[2] = {
> > "Disabled",
> > "Black screen"
> > @@ -1585,7 +1995,7 @@ static int tvp5150_probe(struct i2c_client *c,
> > res = tvp5150_parse_dt(core, np);
> > if (res) {
> > dev_err(sd->dev, "DT parsing error: %d\n", res);
> > - return res;
> > + goto err_cleanup_dt;
> > }
> > } else {
> > /* Default to BT.656 embedded sync */
> > @@ -1593,25 +2003,16 @@ static int tvp5150_probe(struct i2c_client *c,
> > }
> >
> > v4l2_i2c_subdev_init(sd, c, &tvp5150_ops);
> > + sd->internal_ops = &tvp5150_internal_ops;
> > sd->flags |= V4L2_SUBDEV_FL_HAS_DEVNODE;
> >
> > -#if defined(CONFIG_MEDIA_CONTROLLER)
> > - core->pads[TVP5150_PAD_IF_INPUT].flags = MEDIA_PAD_FL_SINK;
> > - core->pads[TVP5150_PAD_IF_INPUT].sig_type = PAD_SIGNAL_ANALOG;
> > - core->pads[TVP5150_PAD_VID_OUT].flags = MEDIA_PAD_FL_SOURCE;
> > - core->pads[TVP5150_PAD_VID_OUT].sig_type = PAD_SIGNAL_DV;
> > -
> > - sd->entity.function = MEDIA_ENT_F_ATV_DECODER;
> > -
> > - res = media_entity_pads_init(&sd->entity, TVP5150_NUM_PADS, core->pads);
> > - if (res < 0)
> > - return res;
> > -
> > -#endif
> > + res = tvp5150_mc_init(sd);
> > + if (res)
> > + goto err_cleanup_dt;
> >
> > res = tvp5150_detect_version(core);
> > if (res < 0)
> > - return res;
> > + goto err_cleanup_dt;
> >
> > core->norm = V4L2_STD_ALL; /* Default is autodetect */
> > core->detected_norm = V4L2_STD_UNKNOWN;
> > @@ -1637,7 +2038,7 @@ static int tvp5150_probe(struct i2c_client *c,
> > sd->ctrl_handler = &core->hdl;
> > if (core->hdl.error) {
> > res = core->hdl.error;
> > - goto err;
> > + goto err_free_v4l2_ctrls;
> > }
> >
> > tvp5150_set_default(tvp5150_read_std(sd), &core->rect);
> > @@ -1649,19 +2050,21 @@ static int tvp5150_probe(struct i2c_client *c,
> > tvp5150_isr, IRQF_TRIGGER_HIGH |
> > IRQF_ONESHOT, "tvp5150", core);
> > if (res)
> > - goto err;
> > + goto err_free_v4l2_ctrls;
> > }
> >
> > res = v4l2_async_register_subdev(sd);
> > if (res < 0)
> > - goto err;
> > + goto err_free_v4l2_ctrls;
> >
> > if (debug > 1)
> > tvp5150_log_status(sd);
> > return 0;
> >
> > -err:
> > +err_free_v4l2_ctrls:
> > v4l2_ctrl_handler_free(&core->hdl);
> > +err_cleanup_dt:
> > + tvp5150_dt_cleanup(core);
> > return res;
> > }
> >
> > --
> > 2.20.1
> >
--
Pengutronix e.K. | |
Industrial Linux Solutions | http://www.pengutronix.de/ |
Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0 |
Amtsgericht Hildesheim, HRA 2686 | Fax: +49-5121-206917-5555 |
^ permalink raw reply [flat|nested] 14+ messages in thread
* [PATCH v4 3/7] media: dt-bindings: tvp5150: Add input port connectors DT bindings
2019-01-29 16:07 [PATCH v4 0/7] TVP5150 new features Marco Felsch
2019-01-29 16:07 ` [PATCH v4 1/7] partial revert of "[media] tvp5150: add HW input connectors support" Marco Felsch
2019-01-29 16:07 ` [PATCH v4 2/7] media: tvp5150: add input source selection of_graph support Marco Felsch
@ 2019-01-29 16:07 ` Marco Felsch
2019-01-29 16:07 ` [PATCH v4 4/7] media: v4l2-subdev: add stubs for v4l2_subdev_get_try_* Marco Felsch
` (4 subsequent siblings)
7 siblings, 0 replies; 14+ messages in thread
From: Marco Felsch @ 2019-01-29 16:07 UTC (permalink / raw)
To: mchehab, robh+dt, mark.rutland, sakari.ailus
Cc: devicetree, p.zabel, javierm, afshin.nasser, laurent.pinchart,
linux-media, kernel, Rob Herring
The TVP5150/1 decoders support different video input sources to their
AIP1A/B pins.
Possible configurations are as follows:
- Analog Composite signal connected to AIP1A.
- Analog Composite signal connected to AIP1B.
- Analog S-Video Y (luminance) and C (chrominance)
signals connected to AIP1A and AIP1B respectively.
This patch extends the device tree bindings documentation to describe
how the input connectors for these devices should be defined in a DT.
Signed-off-by: Marco Felsch <m.felsch@pengutronix.de>
Reviewed-by: Rob Herring <robh@kernel.org>
---
Changelog:
v3:
- remove examples for one and two inputs
- replace space by tabs
v2:
- adapt port layout in accordance with
https://www.spinics.net/lists/linux-media/msg138546.html with the
svideo-connector deviation (use only one endpoint)
.../devicetree/bindings/media/i2c/tvp5150.txt | 92 +++++++++++++++++--
1 file changed, 85 insertions(+), 7 deletions(-)
diff --git a/Documentation/devicetree/bindings/media/i2c/tvp5150.txt b/Documentation/devicetree/bindings/media/i2c/tvp5150.txt
index 8c0fc1a26bf0..bdd273d8b44d 100644
--- a/Documentation/devicetree/bindings/media/i2c/tvp5150.txt
+++ b/Documentation/devicetree/bindings/media/i2c/tvp5150.txt
@@ -12,11 +12,31 @@ Optional Properties:
- pdn-gpios: phandle for the GPIO connected to the PDN pin, if any.
- reset-gpios: phandle for the GPIO connected to the RESETB pin, if any.
-The device node must contain one 'port' child node for its digital output
-video port, in accordance with the video interface bindings defined in
-Documentation/devicetree/bindings/media/video-interfaces.txt.
+The device node must contain one 'port' child node per device physical input
+and output port, in accordance with the video interface bindings defined in
+Documentation/devicetree/bindings/media/video-interfaces.txt. The port nodes
+are numbered as follows
-Required Endpoint Properties for parallel synchronization:
+ Name Type Port
+ --------------------------------------
+ AIP1A sink 0
+ AIP1B sink 1
+ Y-OUT src 2
+
+The device node must contain at least one sink port and the src port. Each input
+port must be linked to an endpoint defined in
+Documentation/devicetree/bindings/display/connector/analog-tv-connector.txt. The
+port/connector layout is as follows
+
+tvp-5150 port@0 (AIP1A)
+ endpoint@0 -----------> Comp0-Con port
+ endpoint@1 -----------> Svideo-Con port
+tvp-5150 port@1 (AIP1B)
+ endpoint -----------> Comp1-Con port
+tvp-5150 port@2
+ endpoint (video bitstream output at YOUT[0-7] parallel bus)
+
+Required Endpoint Properties for parallel synchronization on output port:
- hsync-active: active state of the HSYNC signal. Must be <1> (HIGH).
- vsync-active: active state of the VSYNC signal. Must be <1> (HIGH).
@@ -26,17 +46,75 @@ Required Endpoint Properties for parallel synchronization:
If none of hsync-active, vsync-active and field-even-active is specified,
the endpoint is assumed to use embedded BT.656 synchronization.
-Example:
+Example - three input sources:
+
+comp_connector_0 {
+ compatible = "composite-video-connector";
+ label = "Composite0";
+
+ port {
+ composite0_to_tvp5150: endpoint {
+ remote-endpoint = <&tvp5150_to_composite0>;
+ };
+ };
+};
+
+comp_connector_1 {
+ compatible = "composite-video-connector";
+ label = "Composite1";
+
+ port {
+ composite1_to_tvp5150: endpoint {
+ remote-endpoint = <&tvp5150_to_composite1>;
+ };
+ };
+};
+
+svid_connector {
+ compatible = "svideo-connector";
+ label = "S-Video";
+
+ port {
+ svideo_to_tvp5150: endpoint {
+ remote-endpoint = <&tvp5150_to_svideo>;
+ };
+ };
+};
&i2c2 {
- ...
tvp5150@5c {
compatible = "ti,tvp5150";
reg = <0x5c>;
pdn-gpios = <&gpio4 30 GPIO_ACTIVE_LOW>;
reset-gpios = <&gpio6 7 GPIO_ACTIVE_LOW>;
- port {
+ port@0 {
+ #address-cells = <1>;
+ #size-cells = <0>;
+ reg = <0>;
+
+ tvp5150_to_composite0: endpoint@0 {
+ reg = <0>;
+ remote-endpoint = <&composite0_to_tvp5150>;
+ };
+
+ tvp5150_to_svideo: endpoint@1 {
+ reg = <1>;
+ remote-endpoint = <&svideo_to_tvp5150>;
+ };
+ };
+
+ port@1 {
+ reg = <1>;
+
+ tvp5150_to_composite1: endpoint {
+ remote-endpoint = <&composite1_to_tvp5150>;
+ };
+ };
+
+ port@2 {
+ reg = <2>;
+
tvp5150_1: endpoint {
remote-endpoint = <&ccdc_ep>;
};
--
2.20.1
^ permalink raw reply related [flat|nested] 14+ messages in thread* [PATCH v4 4/7] media: v4l2-subdev: add stubs for v4l2_subdev_get_try_*
2019-01-29 16:07 [PATCH v4 0/7] TVP5150 new features Marco Felsch
` (2 preceding siblings ...)
2019-01-29 16:07 ` [PATCH v4 3/7] media: dt-bindings: tvp5150: Add input port connectors DT bindings Marco Felsch
@ 2019-01-29 16:07 ` Marco Felsch
2019-03-21 10:01 ` Jacopo Mondi
2019-01-29 16:07 ` [PATCH v4 5/7] media: tvp5150: add FORMAT_TRY support for get/set selection handlers Marco Felsch
` (3 subsequent siblings)
7 siblings, 1 reply; 14+ messages in thread
From: Marco Felsch @ 2019-01-29 16:07 UTC (permalink / raw)
To: mchehab, robh+dt, mark.rutland, sakari.ailus
Cc: devicetree, p.zabel, javierm, afshin.nasser, laurent.pinchart,
linux-media, kernel
In case of missing CONFIG_VIDEO_V4L2_SUBDEV_API those helpers aren't
available. So each driver have to add ifdefs around those helpers or
add the CONFIG_VIDEO_V4L2_SUBDEV_API as dependcy.
Make these helpers available in case of CONFIG_VIDEO_V4L2_SUBDEV_API
isn't set to avoid ifdefs. This approach is less error prone too.
Signed-off-by: Marco Felsch <m.felsch@pengutronix.de>
---
include/media/v4l2-subdev.h | 15 ++++++++++++---
1 file changed, 12 insertions(+), 3 deletions(-)
diff --git a/include/media/v4l2-subdev.h b/include/media/v4l2-subdev.h
index 47af609dc8f1..90c9a301d72a 100644
--- a/include/media/v4l2-subdev.h
+++ b/include/media/v4l2-subdev.h
@@ -916,8 +916,6 @@ struct v4l2_subdev_fh {
#define to_v4l2_subdev_fh(fh) \
container_of(fh, struct v4l2_subdev_fh, vfh)
-#if defined(CONFIG_VIDEO_V4L2_SUBDEV_API)
-
/**
* v4l2_subdev_get_try_format - ancillary routine to call
* &struct v4l2_subdev_pad_config->try_fmt
@@ -931,9 +929,13 @@ static inline struct v4l2_mbus_framefmt
struct v4l2_subdev_pad_config *cfg,
unsigned int pad)
{
+#if defined(CONFIG_VIDEO_V4L2_SUBDEV_API)
if (WARN_ON(pad >= sd->entity.num_pads))
pad = 0;
return &cfg[pad].try_fmt;
+#else
+ return NULL;
+#endif
}
/**
@@ -949,9 +951,13 @@ static inline struct v4l2_rect
struct v4l2_subdev_pad_config *cfg,
unsigned int pad)
{
+#if defined(CONFIG_VIDEO_V4L2_SUBDEV_API)
if (WARN_ON(pad >= sd->entity.num_pads))
pad = 0;
return &cfg[pad].try_crop;
+#else
+ return NULL;
+#endif
}
/**
@@ -967,11 +973,14 @@ static inline struct v4l2_rect
struct v4l2_subdev_pad_config *cfg,
unsigned int pad)
{
+#if defined(CONFIG_VIDEO_V4L2_SUBDEV_API)
if (WARN_ON(pad >= sd->entity.num_pads))
pad = 0;
return &cfg[pad].try_compose;
-}
+#else
+ return NULL;
#endif
+}
extern const struct v4l2_file_operations v4l2_subdev_fops;
--
2.20.1
^ permalink raw reply related [flat|nested] 14+ messages in thread* Re: [PATCH v4 4/7] media: v4l2-subdev: add stubs for v4l2_subdev_get_try_*
2019-01-29 16:07 ` [PATCH v4 4/7] media: v4l2-subdev: add stubs for v4l2_subdev_get_try_* Marco Felsch
@ 2019-03-21 10:01 ` Jacopo Mondi
2019-03-21 10:59 ` Marco Felsch
0 siblings, 1 reply; 14+ messages in thread
From: Jacopo Mondi @ 2019-03-21 10:01 UTC (permalink / raw)
To: Marco Felsch, Hans Verkuil, Sakari Ailus
Cc: mchehab, robh+dt, mark.rutland, sakari.ailus, devicetree,
p.zabel, javierm, afshin.nasser, laurent.pinchart, linux-media,
kernel
[-- Attachment #1: Type: text/plain, Size: 2875 bytes --]
Hi Marco,
FYI we've been there already:
https://patchwork.kernel.org/patch/10703029/
and that ended with Hans' patch:
https://patchwork.linuxtv.org/patch/53370/
which didn't get far unfortunately.
On Tue, Jan 29, 2019 at 05:07:54PM +0100, Marco Felsch wrote:
> In case of missing CONFIG_VIDEO_V4L2_SUBDEV_API those helpers aren't
> available. So each driver have to add ifdefs around those helpers or
> add the CONFIG_VIDEO_V4L2_SUBDEV_API as dependcy.
>
> Make these helpers available in case of CONFIG_VIDEO_V4L2_SUBDEV_API
> isn't set to avoid ifdefs. This approach is less error prone too.
>
> Signed-off-by: Marco Felsch <m.felsch@pengutronix.de>
> ---
> include/media/v4l2-subdev.h | 15 ++++++++++++---
> 1 file changed, 12 insertions(+), 3 deletions(-)
>
> diff --git a/include/media/v4l2-subdev.h b/include/media/v4l2-subdev.h
> index 47af609dc8f1..90c9a301d72a 100644
> --- a/include/media/v4l2-subdev.h
> +++ b/include/media/v4l2-subdev.h
> @@ -916,8 +916,6 @@ struct v4l2_subdev_fh {
> #define to_v4l2_subdev_fh(fh) \
> container_of(fh, struct v4l2_subdev_fh, vfh)
>
> -#if defined(CONFIG_VIDEO_V4L2_SUBDEV_API)
> -
> /**
> * v4l2_subdev_get_try_format - ancillary routine to call
> * &struct v4l2_subdev_pad_config->try_fmt
> @@ -931,9 +929,13 @@ static inline struct v4l2_mbus_framefmt
> struct v4l2_subdev_pad_config *cfg,
> unsigned int pad)
> {
> +#if defined(CONFIG_VIDEO_V4L2_SUBDEV_API)
> if (WARN_ON(pad >= sd->entity.num_pads))
> pad = 0;
> return &cfg[pad].try_fmt;
> +#else
> + return NULL;
Since Hans' attempt didn't succeed, maybe we want to reconsider this
approach? I liked Lubomir's version better, but in any case, small
details...
Shouldn't you return ERR_PTR(-ENOTTY) here instead of NULL ?
+ Sakari, Hans:
Alternatively, what if we add CONFIG_VIDEO_V4L2_SUBDEV_API as a
dependency of all sensor drivers that still use the
#ifdef CONFIG_VIDEO_V4L2_SUBDEV_API
v4l2_subdev_get_try_format(sd, cfg, format->pad);
#else
-ENOTTY
#endif
pattern so we could remove that block #ifdef blocks and do not touch the
v4l2-subdev.h header? Should I send a patch?
Thanks
j
> +#endif
> }
>
> /**
> @@ -949,9 +951,13 @@ static inline struct v4l2_rect
> struct v4l2_subdev_pad_config *cfg,
> unsigned int pad)
> {
> +#if defined(CONFIG_VIDEO_V4L2_SUBDEV_API)
> if (WARN_ON(pad >= sd->entity.num_pads))
> pad = 0;
> return &cfg[pad].try_crop;
> +#else
> + return NULL;
> +#endif
> }
>
> /**
> @@ -967,11 +973,14 @@ static inline struct v4l2_rect
> struct v4l2_subdev_pad_config *cfg,
> unsigned int pad)
> {
> +#if defined(CONFIG_VIDEO_V4L2_SUBDEV_API)
> if (WARN_ON(pad >= sd->entity.num_pads))
> pad = 0;
> return &cfg[pad].try_compose;
> -}
> +#else
> + return NULL;
> #endif
> +}
>
> extern const struct v4l2_file_operations v4l2_subdev_fops;
>
> --
> 2.20.1
>
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply [flat|nested] 14+ messages in thread* Re: [PATCH v4 4/7] media: v4l2-subdev: add stubs for v4l2_subdev_get_try_*
2019-03-21 10:01 ` Jacopo Mondi
@ 2019-03-21 10:59 ` Marco Felsch
0 siblings, 0 replies; 14+ messages in thread
From: Marco Felsch @ 2019-03-21 10:59 UTC (permalink / raw)
To: Jacopo Mondi
Cc: Hans Verkuil, Sakari Ailus, mchehab, robh+dt, mark.rutland,
devicetree, p.zabel, javierm, afshin.nasser, laurent.pinchart,
linux-media, kernel
Hi Jacopo,
On 19-03-21 11:01, Jacopo Mondi wrote:
> Hi Marco,
>
> FYI we've been there already:
> https://patchwork.kernel.org/patch/10703029/
>
> and that ended with Hans' patch:
> https://patchwork.linuxtv.org/patch/53370/
> which didn't get far unfortunately.
Thanks for this links :) It seems that there isn't a simple solution.
> On Tue, Jan 29, 2019 at 05:07:54PM +0100, Marco Felsch wrote:
> > In case of missing CONFIG_VIDEO_V4L2_SUBDEV_API those helpers aren't
> > available. So each driver have to add ifdefs around those helpers or
> > add the CONFIG_VIDEO_V4L2_SUBDEV_API as dependcy.
> >
> > Make these helpers available in case of CONFIG_VIDEO_V4L2_SUBDEV_API
> > isn't set to avoid ifdefs. This approach is less error prone too.
> >
> > Signed-off-by: Marco Felsch <m.felsch@pengutronix.de>
> > ---
> > include/media/v4l2-subdev.h | 15 ++++++++++++---
> > 1 file changed, 12 insertions(+), 3 deletions(-)
> >
> > diff --git a/include/media/v4l2-subdev.h b/include/media/v4l2-subdev.h
> > index 47af609dc8f1..90c9a301d72a 100644
> > --- a/include/media/v4l2-subdev.h
> > +++ b/include/media/v4l2-subdev.h
> > @@ -916,8 +916,6 @@ struct v4l2_subdev_fh {
> > #define to_v4l2_subdev_fh(fh) \
> > container_of(fh, struct v4l2_subdev_fh, vfh)
> >
> > -#if defined(CONFIG_VIDEO_V4L2_SUBDEV_API)
> > -
> > /**
> > * v4l2_subdev_get_try_format - ancillary routine to call
> > * &struct v4l2_subdev_pad_config->try_fmt
> > @@ -931,9 +929,13 @@ static inline struct v4l2_mbus_framefmt
> > struct v4l2_subdev_pad_config *cfg,
> > unsigned int pad)
> > {
> > +#if defined(CONFIG_VIDEO_V4L2_SUBDEV_API)
> > if (WARN_ON(pad >= sd->entity.num_pads))
> > pad = 0;
> > return &cfg[pad].try_fmt;
> > +#else
> > + return NULL;
>
> Since Hans' attempt didn't succeed, maybe we want to reconsider this
> approach? I liked Lubomir's version better, but in any case, small
> details...
>
> Shouldn't you return ERR_PTR(-ENOTTY) here instead of NULL ?
Yes that would be better.
> + Sakari, Hans:
> Alternatively, what if we add CONFIG_VIDEO_V4L2_SUBDEV_API as a
> dependency of all sensor drivers that still use the
>
> #ifdef CONFIG_VIDEO_V4L2_SUBDEV_API
> v4l2_subdev_get_try_format(sd, cfg, format->pad);
> #else
> -ENOTTY
> #endif
>
> pattern so we could remove that block #ifdef blocks and do not touch the
> v4l2-subdev.h header? Should I send a patch?
If I remember rightly the mt9v111 is using the dependency approach to
avoid such ifdef's too. But this seems like Hans approach if I got it
right which wasn't ok for Mauro.
Regards,
Marco
> Thanks
> j
>
>
> > +#endif
> > }
> >
> > /**
> > @@ -949,9 +951,13 @@ static inline struct v4l2_rect
> > struct v4l2_subdev_pad_config *cfg,
> > unsigned int pad)
> > {
> > +#if defined(CONFIG_VIDEO_V4L2_SUBDEV_API)
> > if (WARN_ON(pad >= sd->entity.num_pads))
> > pad = 0;
> > return &cfg[pad].try_crop;
> > +#else
> > + return NULL;
> > +#endif
> > }
> >
> > /**
> > @@ -967,11 +973,14 @@ static inline struct v4l2_rect
> > struct v4l2_subdev_pad_config *cfg,
> > unsigned int pad)
> > {
> > +#if defined(CONFIG_VIDEO_V4L2_SUBDEV_API)
> > if (WARN_ON(pad >= sd->entity.num_pads))
> > pad = 0;
> > return &cfg[pad].try_compose;
> > -}
> > +#else
> > + return NULL;
> > #endif
> > +}
> >
> > extern const struct v4l2_file_operations v4l2_subdev_fops;
> >
> > --
> > 2.20.1
> >
--
Pengutronix e.K. | |
Industrial Linux Solutions | http://www.pengutronix.de/ |
Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0 |
Amtsgericht Hildesheim, HRA 2686 | Fax: +49-5121-206917-5555 |
^ permalink raw reply [flat|nested] 14+ messages in thread
* [PATCH v4 5/7] media: tvp5150: add FORMAT_TRY support for get/set selection handlers
2019-01-29 16:07 [PATCH v4 0/7] TVP5150 new features Marco Felsch
` (3 preceding siblings ...)
2019-01-29 16:07 ` [PATCH v4 4/7] media: v4l2-subdev: add stubs for v4l2_subdev_get_try_* Marco Felsch
@ 2019-01-29 16:07 ` Marco Felsch
2019-01-29 16:07 ` [PATCH v4 6/7] media: tvp5150: initialize subdev before parsing device tree Marco Felsch
` (2 subsequent siblings)
7 siblings, 0 replies; 14+ messages in thread
From: Marco Felsch @ 2019-01-29 16:07 UTC (permalink / raw)
To: mchehab, robh+dt, mark.rutland, sakari.ailus
Cc: devicetree, p.zabel, javierm, afshin.nasser, laurent.pinchart,
linux-media, kernel
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 TVP5150 AVID will be updated if the 'which' field is FORMAT_ACTIVE
and the requested selection rectangle differs from the already set one.
Signed-off-by: Marco Felsch <m.felsch@pengutronix.de>
---
Changelog:
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 | 108 ++++++++++++++++++++++++------------
1 file changed, 74 insertions(+), 34 deletions(-)
diff --git a/drivers/media/i2c/tvp5150.c b/drivers/media/i2c/tvp5150.c
index a89b83f69266..83b976aa4492 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"
@@ -1007,20 +1008,40 @@ 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_TRY:
+ return v4l2_subdev_get_try_crop(&decoder->sd, cfg, pad);
+ case V4L2_SUBDEV_FORMAT_ACTIVE:
+ return &decoder->rect;
+ default:
+ return NULL;
+ }
+}
+
static int tvp5150_fill_fmt(struct v4l2_subdev *sd,
struct v4l2_subdev_pad_config *cfg,
struct v4l2_subdev_format *format)
{
struct v4l2_mbus_framefmt *f;
+ struct v4l2_rect *__crop;
struct tvp5150 *decoder = to_tvp5150(sd);
if (!format || (format->pad != TVP5150_PAD_VID_OUT))
return -EINVAL;
f = &format->format;
+ __crop = __tvp5150_get_pad_crop(decoder, cfg, format->pad,
+ format->which);
+ if (!__crop)
+ return -EINVAL;
- f->width = decoder->rect.width;
- f->height = decoder->rect.height / 2;
+ f->width = __crop->width;
+ f->height = __crop->height / 2;
f->code = TVP5150_MBUS_FMT;
f->field = TVP5150_FIELD;
@@ -1031,17 +1052,51 @@ 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",
@@ -1050,17 +1105,7 @@ static int tvp5150_set_selection(struct v4l2_subdev *sd,
/* 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:
@@ -1073,20 +1118,17 @@ 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);
+ __crop = __tvp5150_get_pad_crop(decoder, cfg, sel->pad, sel->which);
- decoder->rect = rect;
+ /*
+ * Update output image size if the selection (crop) rectangle size or
+ * position has been modified.
+ */
+ if (!v4l2_rect_equal(&rect, __crop))
+ if (sel->which == V4L2_SUBDEV_FORMAT_ACTIVE)
+ __tvp5150_set_selection(sd, rect);
+
+ *__crop = rect;
return 0;
}
@@ -1098,9 +1140,6 @@ static int tvp5150_get_selection(struct v4l2_subdev *sd,
struct tvp5150 *decoder = container_of(sd, struct tvp5150, sd);
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;
@@ -1118,7 +1157,8 @@ 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;
+ sel->r = *__tvp5150_get_pad_crop(decoder, cfg, sel->pad,
+ sel->which);
return 0;
default:
return -EINVAL;
--
2.20.1
^ permalink raw reply related [flat|nested] 14+ messages in thread* [PATCH v4 6/7] media: tvp5150: initialize subdev before parsing device tree
2019-01-29 16:07 [PATCH v4 0/7] TVP5150 new features Marco Felsch
` (4 preceding siblings ...)
2019-01-29 16:07 ` [PATCH v4 5/7] media: tvp5150: add FORMAT_TRY support for get/set selection handlers Marco Felsch
@ 2019-01-29 16:07 ` Marco Felsch
2019-01-29 16:07 ` [PATCH v4 7/7] media: tvp5150: add s_power callback Marco Felsch
2019-02-12 16:09 ` [PATCH v4 0/7] TVP5150 new features Marco Felsch
7 siblings, 0 replies; 14+ messages in thread
From: Marco Felsch @ 2019-01-29 16:07 UTC (permalink / raw)
To: mchehab, robh+dt, mark.rutland, sakari.ailus
Cc: devicetree, p.zabel, javierm, afshin.nasser, laurent.pinchart,
linux-media, kernel, Michael Tretter
From: Michael Tretter <m.tretter@pengutronix.de>
There are several debug prints in the tvp5150_parse_dt() function, which
do not print the prefix, because the v4l2_subdev is not initialized, yet.
Initialize the v4l2_subdev before parsing the device tree to fix the
debug messages.
Signed-off-by: Michael Tretter <m.tretter@pengutronix.de>
Signed-off-by: Marco Felsch <m.felsch@pengutronix.de>
---
drivers/media/i2c/tvp5150.c | 7 +++----
1 file changed, 3 insertions(+), 4 deletions(-)
diff --git a/drivers/media/i2c/tvp5150.c b/drivers/media/i2c/tvp5150.c
index 83b976aa4492..e4f2efadd1aa 100644
--- a/drivers/media/i2c/tvp5150.c
+++ b/drivers/media/i2c/tvp5150.c
@@ -2030,6 +2030,9 @@ static int tvp5150_probe(struct i2c_client *c,
core->regmap = map;
sd = &core->sd;
+ v4l2_i2c_subdev_init(sd, c, &tvp5150_ops);
+ sd->internal_ops = &tvp5150_internal_ops;
+ sd->flags |= V4L2_SUBDEV_FL_HAS_DEVNODE;
if (IS_ENABLED(CONFIG_OF) && np) {
res = tvp5150_parse_dt(core, np);
@@ -2042,10 +2045,6 @@ static int tvp5150_probe(struct i2c_client *c,
core->mbus_type = V4L2_MBUS_BT656;
}
- v4l2_i2c_subdev_init(sd, c, &tvp5150_ops);
- sd->internal_ops = &tvp5150_internal_ops;
- sd->flags |= V4L2_SUBDEV_FL_HAS_DEVNODE;
-
res = tvp5150_mc_init(sd);
if (res)
goto err_cleanup_dt;
--
2.20.1
^ permalink raw reply related [flat|nested] 14+ messages in thread* [PATCH v4 7/7] media: tvp5150: add s_power callback
2019-01-29 16:07 [PATCH v4 0/7] TVP5150 new features Marco Felsch
` (5 preceding siblings ...)
2019-01-29 16:07 ` [PATCH v4 6/7] media: tvp5150: initialize subdev before parsing device tree Marco Felsch
@ 2019-01-29 16:07 ` Marco Felsch
2019-02-12 16:09 ` [PATCH v4 0/7] TVP5150 new features Marco Felsch
7 siblings, 0 replies; 14+ messages in thread
From: Marco Felsch @ 2019-01-29 16:07 UTC (permalink / raw)
To: mchehab, robh+dt, mark.rutland, sakari.ailus
Cc: devicetree, p.zabel, javierm, afshin.nasser, laurent.pinchart,
linux-media, kernel
Don't en-/disable the interrupts during s_stream because someone can
disable the stream but wants to get informed if the stream is locked
again. So keep the interrupts enabled the whole time the pipeline is
opened.
Signed-off-by: Marco Felsch <m.felsch@pengutronix.de>
---
drivers/media/i2c/tvp5150.c | 23 +++++++++++++++++------
1 file changed, 17 insertions(+), 6 deletions(-)
diff --git a/drivers/media/i2c/tvp5150.c b/drivers/media/i2c/tvp5150.c
index e4f2efadd1aa..1f0dd9d3655c 100644
--- a/drivers/media/i2c/tvp5150.c
+++ b/drivers/media/i2c/tvp5150.c
@@ -1387,11 +1387,26 @@ static const struct media_entity_operations tvp5150_sd_media_ops = {
/****************************************************************************
I2C Command
****************************************************************************/
+static int tvp5150_s_power(struct v4l2_subdev *sd, int on)
+{
+ struct tvp5150 *decoder = to_tvp5150(sd);
+ unsigned int val = 0;
+
+ if (on)
+ val = TVP5150_INT_A_LOCK;
+
+ if (decoder->irq)
+ /* Enable / Disable lock interrupt */
+ regmap_update_bits(decoder->regmap, TVP5150_INT_ENABLE_REG_A,
+ TVP5150_INT_A_LOCK, val);
+
+ return 0;
+}
static int tvp5150_s_stream(struct v4l2_subdev *sd, int enable)
{
struct tvp5150 *decoder = to_tvp5150(sd);
- unsigned int mask, val = 0, int_val = 0;
+ unsigned int mask, val = 0;
mask = TVP5150_MISC_CTL_YCBCR_OE | TVP5150_MISC_CTL_SYNC_OE |
TVP5150_MISC_CTL_CLOCK_OE;
@@ -1404,15 +1419,10 @@ static int tvp5150_s_stream(struct v4l2_subdev *sd, int enable)
val = decoder->lock ? decoder->oe : 0;
else
val = decoder->oe;
- int_val = TVP5150_INT_A_LOCK;
v4l2_subdev_notify_event(&decoder->sd, &tvp5150_ev_fmt);
}
regmap_update_bits(decoder->regmap, TVP5150_MISC_CTL, mask, val);
- if (decoder->irq)
- /* Enable / Disable lock interrupt */
- regmap_update_bits(decoder->regmap, TVP5150_INT_ENABLE_REG_A,
- TVP5150_INT_A_LOCK, int_val);
return 0;
}
@@ -1614,6 +1624,7 @@ static const struct v4l2_subdev_core_ops tvp5150_core_ops = {
.g_register = tvp5150_g_register,
.s_register = tvp5150_s_register,
#endif
+ .s_power = tvp5150_s_power,
};
static const struct v4l2_subdev_tuner_ops tvp5150_tuner_ops = {
--
2.20.1
^ permalink raw reply related [flat|nested] 14+ messages in thread* Re: [PATCH v4 0/7] TVP5150 new features
2019-01-29 16:07 [PATCH v4 0/7] TVP5150 new features Marco Felsch
` (6 preceding siblings ...)
2019-01-29 16:07 ` [PATCH v4 7/7] media: tvp5150: add s_power callback Marco Felsch
@ 2019-02-12 16:09 ` Marco Felsch
2019-03-05 9:46 ` Marco Felsch
7 siblings, 1 reply; 14+ messages in thread
From: Marco Felsch @ 2019-02-12 16:09 UTC (permalink / raw)
To: mchehab, robh+dt, mark.rutland, sakari.ailus
Cc: devicetree, kernel, javierm, laurent.pinchart, p.zabel,
afshin.nasser, linux-media
Hi Mauro,
gentle ping..
On 19-01-29 17:07, Marco Felsch wrote:
> Hi,
>
> this is the v4 of my TVP5150 series which adds the of_graph support.
> Basically this series was just rebased on top of the media-tree/master
> as mentioned by Mauro [1].
>
> I dropped commit ("media: tvp5150: fix irq_request error path during
> probe") since it was already applied and commit ("media: v4l2-subdev:
> fix v4l2_subdev_get_try_* dependency") as mentioned by Sakari [2].
>
> To have a quick overview I added the range-diff to the v3 below.
>
> [1] https://www.spinics.net/lists/devicetree/msg262787.html
> [2] https://www.spinics.net/lists/devicetree/msg249354.html
>
> Javier Martinez Canillas (1):
> partial revert of "[media] tvp5150: add HW input connectors support"
>
> Marco Felsch (5):
> media: tvp5150: add input source selection of_graph support
> media: dt-bindings: tvp5150: Add input port connectors DT bindings
> media: v4l2-subdev: add stubs for v4l2_subdev_get_try_*
> media: tvp5150: add FORMAT_TRY support for get/set selection handlers
> media: tvp5150: add s_power callback
>
> Michael Tretter (1):
> media: tvp5150: initialize subdev before parsing device tree
>
> .../devicetree/bindings/media/i2c/tvp5150.txt | 92 ++-
> drivers/media/i2c/tvp5150.c | 652 +++++++++++++-----
> include/dt-bindings/media/tvp5150.h | 2 -
> include/media/v4l2-subdev.h | 15 +-
> 4 files changed, 579 insertions(+), 182 deletions(-)
>
> Range-diff against v3:
> 1: c3d26f4af009 ! 1: 4a35a7fb3a24 partial revert of "[media] tvp5150: add HW input connectors support"
> @@ -21,6 +21,7 @@
> [m.felsch@pengutronix.de: rm TVP5150_INPUT_NUM define]
> Signed-off-by: Marco Felsch <m.felsch@pengutronix.de>
> Signed-off-by: Mauro Carvalho Chehab <mchehab+samsung@kernel.org>
> + Acked-by: Rob Herring <robh@kernel.org>
>
> diff --git a/drivers/media/i2c/tvp5150.c b/drivers/media/i2c/tvp5150.c
> --- a/drivers/media/i2c/tvp5150.c
> @@ -131,7 +132,7 @@
> ****************************************************************************/
> @@
> {
> - struct v4l2_fwnode_endpoint bus_cfg;
> + struct v4l2_fwnode_endpoint bus_cfg = { .bus_type = 0 };
> struct device_node *ep;
> -#ifdef CONFIG_MEDIA_CONTROLLER
> - struct device_node *connectors, *child;
> 2: 88f4c4a30a08 < -: ------------ media: tvp5150: fix irq_request error path during probe
> 3: 48b34c9c9e5f ! 2: f5ca703e5523 media: tvp5150: add input source selection of_graph support
> @@ -21,6 +21,11 @@
> ---
> Changelog:
>
> + v4:
> + - rebase on top of media_tree/master, fix merge conflict due to commit
> + 60359a28d592 ("media: v4l: fwnode: Initialise the V4L2 fwnode endpoints
> + to zero")
> +
> v3:
> - probe(): s/err/err_free_v4l2_ctrls
> - drop MC dependency for tvp5150_pads
> @@ -49,13 +54,11 @@
> #define dprintk0(__dev, __arg...) dev_dbg_lvl(__dev, 0, 0, __arg)
>
> enum tvp5150_pads {
> -- TVP5150_PAD_IF_INPUT,
> -- TVP5150_PAD_VID_OUT,
> -- TVP5150_NUM_PADS
> +- TVP5150_PAD_IF_INPUT,
> + TVP5150_PAD_AIP1A = TVP5150_COMPOSITE0,
> + TVP5150_PAD_AIP1B,
> -+ TVP5150_PAD_VID_OUT,
> -+ TVP5150_NUM_PADS
> + TVP5150_PAD_VID_OUT,
> + TVP5150_NUM_PADS
> };
>
> +#if defined(CONFIG_MEDIA_CONTROLLER)
> @@ -345,7 +348,7 @@
> +#if defined(CONFIG_MEDIA_CONTROLLER)
> +static int tvp5150_add_of_connectors(struct tvp5150 *decoder)
> {
> -- struct v4l2_fwnode_endpoint bus_cfg;
> +- struct v4l2_fwnode_endpoint bus_cfg = { .bus_type = 0 };
> - struct device_node *ep;
> - unsigned int flags;
> - int ret = 0;
> @@ -464,7 +467,7 @@
> +static int tvp5150_parse_dt(struct tvp5150 *decoder, struct device_node *np)
> +{
> + struct device *dev = decoder->sd.dev;
> -+ struct v4l2_fwnode_endpoint bus_cfg;
> ++ struct v4l2_fwnode_endpoint bus_cfg = { .bus_type = 0 };
> + struct device_node *ep_np;
> + unsigned int flags;
> + int ret, i = 0, in = 0;
> 4: 0b168180f4a4 ! 3: a7d06df79366 media: dt-bindings: tvp5150: Add input port connectors DT bindings
> @@ -15,6 +15,7 @@
> how the input connectors for these devices should be defined in a DT.
>
> Signed-off-by: Marco Felsch <m.felsch@pengutronix.de>
> + Reviewed-by: Rob Herring <robh@kernel.org>
>
> ---
> Changelog:
> 5: 871eb653fcf3 = 4: 860087e6c286 media: v4l2-subdev: add stubs for v4l2_subdev_get_try_*
> 6: fb141d6c8098 < -: ------------ media: v4l2-subdev: fix v4l2_subdev_get_try_* dependency
> 7: 795b4a45cb68 ! 5: fcd223cd1563 media: tvp5150: add FORMAT_TRY support for get/set selection handlers
> @@ -13,6 +13,13 @@
>
> Signed-off-by: Marco Felsch <m.felsch@pengutronix.de>
>
> + ---
> + Changelog:
> +
> + v4:
> + - fix merge conflict due to rebase on top of media-tree/master
> + - __tvp5150_get_pad_crop(): cosmetic alignment fixes
> +
> diff --git a/drivers/media/i2c/tvp5150.c b/drivers/media/i2c/tvp5150.c
> --- a/drivers/media/i2c/tvp5150.c
> +++ b/drivers/media/i2c/tvp5150.c
> @@ -148,19 +155,19 @@
>
> - regmap_write(decoder->regmap, TVP5150_VERT_BLANKING_START, rect.top);
> - regmap_write(decoder->regmap, TVP5150_VERT_BLANKING_STOP,
> -- rect.top + rect.height - hmax);
> +- rect.top + rect.height - hmax);
> - regmap_write(decoder->regmap, TVP5150_ACT_VD_CROP_ST_MSB,
> -- rect.left >> TVP5150_CROP_SHIFT);
> +- rect.left >> TVP5150_CROP_SHIFT);
> - regmap_write(decoder->regmap, TVP5150_ACT_VD_CROP_ST_LSB,
> -- rect.left | (1 << TVP5150_CROP_SHIFT));
> +- 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);
> +- (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);
> -+ __crop = __tvp5150_get_pad_crop(decoder, cfg, sel->pad,
> -+ sel->which);
> -+
> +- rect.left + rect.width - TVP5150_MAX_CROP_LEFT);
> ++ __crop = __tvp5150_get_pad_crop(decoder, cfg, sel->pad, sel->which);
> +
> +- decoder->rect = rect;
> + /*
> + * Update output image size if the selection (crop) rectangle size or
> + * position has been modified.
> @@ -168,8 +175,7 @@
> + if (!v4l2_rect_equal(&rect, __crop))
> + if (sel->which == V4L2_SUBDEV_FORMAT_ACTIVE)
> + __tvp5150_set_selection(sd, rect);
> -
> -- decoder->rect = rect;
> ++
> + *__crop = rect;
>
> return 0;
> @@ -183,14 +189,14 @@
> -
> switch (sel->target) {
> case V4L2_SEL_TGT_CROP_BOUNDS:
> - case V4L2_SEL_TGT_CROP_DEFAULT:
> + sel->r.left = 0;
> @@
> sel->r.height = TVP5150_V_MAX_OTHERS;
> return 0;
> case V4L2_SEL_TGT_CROP:
> - sel->r = decoder->rect;
> + sel->r = *__tvp5150_get_pad_crop(decoder, cfg, sel->pad,
> -+ sel->which);
> ++ sel->which);
> return 0;
> default:
> return -EINVAL;
> 8: 8d88797ba94c = 6: 08cc83bcb513 media: tvp5150: initialize subdev before parsing device tree
> 9: b152e29bc83e = 7: 1249b386cce0 media: tvp5150: add s_power callback
> --
> 2.20.1
>
>
>
--
Pengutronix e.K. | |
Industrial Linux Solutions | http://www.pengutronix.de/ |
Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0 |
Amtsgericht Hildesheim, HRA 2686 | Fax: +49-5121-206917-5555 |
^ permalink raw reply [flat|nested] 14+ messages in thread* Re: [PATCH v4 0/7] TVP5150 new features
2019-02-12 16:09 ` [PATCH v4 0/7] TVP5150 new features Marco Felsch
@ 2019-03-05 9:46 ` Marco Felsch
0 siblings, 0 replies; 14+ messages in thread
From: Marco Felsch @ 2019-03-05 9:46 UTC (permalink / raw)
To: mchehab, robh+dt, mark.rutland, sakari.ailus
Cc: devicetree, kernel, javierm, laurent.pinchart, p.zabel,
afshin.nasser, linux-media
Hi Mauro,
it would be cool to receive some feedback. I hoped to get this in the
media_tree/master a long time ago..
Regards,
Marco
On 19-02-12 17:09, Marco Felsch wrote:
> Hi Mauro,
>
> gentle ping..
>
> On 19-01-29 17:07, Marco Felsch wrote:
> > Hi,
> >
> > this is the v4 of my TVP5150 series which adds the of_graph support.
> > Basically this series was just rebased on top of the media-tree/master
> > as mentioned by Mauro [1].
> >
> > I dropped commit ("media: tvp5150: fix irq_request error path during
> > probe") since it was already applied and commit ("media: v4l2-subdev:
> > fix v4l2_subdev_get_try_* dependency") as mentioned by Sakari [2].
> >
> > To have a quick overview I added the range-diff to the v3 below.
> >
> > [1] https://www.spinics.net/lists/devicetree/msg262787.html
> > [2] https://www.spinics.net/lists/devicetree/msg249354.html
> >
> > Javier Martinez Canillas (1):
> > partial revert of "[media] tvp5150: add HW input connectors support"
> >
> > Marco Felsch (5):
> > media: tvp5150: add input source selection of_graph support
> > media: dt-bindings: tvp5150: Add input port connectors DT bindings
> > media: v4l2-subdev: add stubs for v4l2_subdev_get_try_*
> > media: tvp5150: add FORMAT_TRY support for get/set selection handlers
> > media: tvp5150: add s_power callback
> >
> > Michael Tretter (1):
> > media: tvp5150: initialize subdev before parsing device tree
> >
> > .../devicetree/bindings/media/i2c/tvp5150.txt | 92 ++-
> > drivers/media/i2c/tvp5150.c | 652 +++++++++++++-----
> > include/dt-bindings/media/tvp5150.h | 2 -
> > include/media/v4l2-subdev.h | 15 +-
> > 4 files changed, 579 insertions(+), 182 deletions(-)
> >
> > Range-diff against v3:
> > 1: c3d26f4af009 ! 1: 4a35a7fb3a24 partial revert of "[media] tvp5150: add HW input connectors support"
> > @@ -21,6 +21,7 @@
> > [m.felsch@pengutronix.de: rm TVP5150_INPUT_NUM define]
> > Signed-off-by: Marco Felsch <m.felsch@pengutronix.de>
> > Signed-off-by: Mauro Carvalho Chehab <mchehab+samsung@kernel.org>
> > + Acked-by: Rob Herring <robh@kernel.org>
> >
> > diff --git a/drivers/media/i2c/tvp5150.c b/drivers/media/i2c/tvp5150.c
> > --- a/drivers/media/i2c/tvp5150.c
> > @@ -131,7 +132,7 @@
> > ****************************************************************************/
> > @@
> > {
> > - struct v4l2_fwnode_endpoint bus_cfg;
> > + struct v4l2_fwnode_endpoint bus_cfg = { .bus_type = 0 };
> > struct device_node *ep;
> > -#ifdef CONFIG_MEDIA_CONTROLLER
> > - struct device_node *connectors, *child;
> > 2: 88f4c4a30a08 < -: ------------ media: tvp5150: fix irq_request error path during probe
> > 3: 48b34c9c9e5f ! 2: f5ca703e5523 media: tvp5150: add input source selection of_graph support
> > @@ -21,6 +21,11 @@
> > ---
> > Changelog:
> >
> > + v4:
> > + - rebase on top of media_tree/master, fix merge conflict due to commit
> > + 60359a28d592 ("media: v4l: fwnode: Initialise the V4L2 fwnode endpoints
> > + to zero")
> > +
> > v3:
> > - probe(): s/err/err_free_v4l2_ctrls
> > - drop MC dependency for tvp5150_pads
> > @@ -49,13 +54,11 @@
> > #define dprintk0(__dev, __arg...) dev_dbg_lvl(__dev, 0, 0, __arg)
> >
> > enum tvp5150_pads {
> > -- TVP5150_PAD_IF_INPUT,
> > -- TVP5150_PAD_VID_OUT,
> > -- TVP5150_NUM_PADS
> > +- TVP5150_PAD_IF_INPUT,
> > + TVP5150_PAD_AIP1A = TVP5150_COMPOSITE0,
> > + TVP5150_PAD_AIP1B,
> > -+ TVP5150_PAD_VID_OUT,
> > -+ TVP5150_NUM_PADS
> > + TVP5150_PAD_VID_OUT,
> > + TVP5150_NUM_PADS
> > };
> >
> > +#if defined(CONFIG_MEDIA_CONTROLLER)
> > @@ -345,7 +348,7 @@
> > +#if defined(CONFIG_MEDIA_CONTROLLER)
> > +static int tvp5150_add_of_connectors(struct tvp5150 *decoder)
> > {
> > -- struct v4l2_fwnode_endpoint bus_cfg;
> > +- struct v4l2_fwnode_endpoint bus_cfg = { .bus_type = 0 };
> > - struct device_node *ep;
> > - unsigned int flags;
> > - int ret = 0;
> > @@ -464,7 +467,7 @@
> > +static int tvp5150_parse_dt(struct tvp5150 *decoder, struct device_node *np)
> > +{
> > + struct device *dev = decoder->sd.dev;
> > -+ struct v4l2_fwnode_endpoint bus_cfg;
> > ++ struct v4l2_fwnode_endpoint bus_cfg = { .bus_type = 0 };
> > + struct device_node *ep_np;
> > + unsigned int flags;
> > + int ret, i = 0, in = 0;
> > 4: 0b168180f4a4 ! 3: a7d06df79366 media: dt-bindings: tvp5150: Add input port connectors DT bindings
> > @@ -15,6 +15,7 @@
> > how the input connectors for these devices should be defined in a DT.
> >
> > Signed-off-by: Marco Felsch <m.felsch@pengutronix.de>
> > + Reviewed-by: Rob Herring <robh@kernel.org>
> >
> > ---
> > Changelog:
> > 5: 871eb653fcf3 = 4: 860087e6c286 media: v4l2-subdev: add stubs for v4l2_subdev_get_try_*
> > 6: fb141d6c8098 < -: ------------ media: v4l2-subdev: fix v4l2_subdev_get_try_* dependency
> > 7: 795b4a45cb68 ! 5: fcd223cd1563 media: tvp5150: add FORMAT_TRY support for get/set selection handlers
> > @@ -13,6 +13,13 @@
> >
> > Signed-off-by: Marco Felsch <m.felsch@pengutronix.de>
> >
> > + ---
> > + Changelog:
> > +
> > + v4:
> > + - fix merge conflict due to rebase on top of media-tree/master
> > + - __tvp5150_get_pad_crop(): cosmetic alignment fixes
> > +
> > diff --git a/drivers/media/i2c/tvp5150.c b/drivers/media/i2c/tvp5150.c
> > --- a/drivers/media/i2c/tvp5150.c
> > +++ b/drivers/media/i2c/tvp5150.c
> > @@ -148,19 +155,19 @@
> >
> > - regmap_write(decoder->regmap, TVP5150_VERT_BLANKING_START, rect.top);
> > - regmap_write(decoder->regmap, TVP5150_VERT_BLANKING_STOP,
> > -- rect.top + rect.height - hmax);
> > +- rect.top + rect.height - hmax);
> > - regmap_write(decoder->regmap, TVP5150_ACT_VD_CROP_ST_MSB,
> > -- rect.left >> TVP5150_CROP_SHIFT);
> > +- rect.left >> TVP5150_CROP_SHIFT);
> > - regmap_write(decoder->regmap, TVP5150_ACT_VD_CROP_ST_LSB,
> > -- rect.left | (1 << TVP5150_CROP_SHIFT));
> > +- 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);
> > +- (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);
> > -+ __crop = __tvp5150_get_pad_crop(decoder, cfg, sel->pad,
> > -+ sel->which);
> > -+
> > +- rect.left + rect.width - TVP5150_MAX_CROP_LEFT);
> > ++ __crop = __tvp5150_get_pad_crop(decoder, cfg, sel->pad, sel->which);
> > +
> > +- decoder->rect = rect;
> > + /*
> > + * Update output image size if the selection (crop) rectangle size or
> > + * position has been modified.
> > @@ -168,8 +175,7 @@
> > + if (!v4l2_rect_equal(&rect, __crop))
> > + if (sel->which == V4L2_SUBDEV_FORMAT_ACTIVE)
> > + __tvp5150_set_selection(sd, rect);
> > -
> > -- decoder->rect = rect;
> > ++
> > + *__crop = rect;
> >
> > return 0;
> > @@ -183,14 +189,14 @@
> > -
> > switch (sel->target) {
> > case V4L2_SEL_TGT_CROP_BOUNDS:
> > - case V4L2_SEL_TGT_CROP_DEFAULT:
> > + sel->r.left = 0;
> > @@
> > sel->r.height = TVP5150_V_MAX_OTHERS;
> > return 0;
> > case V4L2_SEL_TGT_CROP:
> > - sel->r = decoder->rect;
> > + sel->r = *__tvp5150_get_pad_crop(decoder, cfg, sel->pad,
> > -+ sel->which);
> > ++ sel->which);
> > return 0;
> > default:
> > return -EINVAL;
> > 8: 8d88797ba94c = 6: 08cc83bcb513 media: tvp5150: initialize subdev before parsing device tree
> > 9: b152e29bc83e = 7: 1249b386cce0 media: tvp5150: add s_power callback
> > --
> > 2.20.1
> >
> >
> >
>
> --
> Pengutronix e.K. | |
> Industrial Linux Solutions | http://www.pengutronix.de/ |
> Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0 |
> Amtsgericht Hildesheim, HRA 2686 | Fax: +49-5121-206917-5555 |
>
>
--
Pengutronix e.K. | |
Industrial Linux Solutions | http://www.pengutronix.de/ |
Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0 |
Amtsgericht Hildesheim, HRA 2686 | Fax: +49-5121-206917-5555 |
^ permalink raw reply [flat|nested] 14+ messages in thread