* [PATCH v2 1/5] media: i2c: adv748x: Support probing a single output
2018-09-05 15:27 [PATCH v2 0/5] media: adv748x: Allow probe with a single output endpoint Jacopo Mondi
@ 2018-09-05 15:27 ` Jacopo Mondi
2018-09-12 13:14 ` Kieran Bingham
2018-09-05 15:27 ` [PATCH v2 2/5] media: i2c: adv748x: Handle TX[A|B] power management Jacopo Mondi
` (3 subsequent siblings)
4 siblings, 1 reply; 12+ messages in thread
From: Jacopo Mondi @ 2018-09-05 15:27 UTC (permalink / raw)
To: laurent.pinchart, kieran.bingham+renesas,
niklas.soderlund+renesas
Cc: Jacopo Mondi, linux-renesas-soc, linux-media
Currently the adv748x driver refuses to probe if both its output endpoints
(TXA and TXB) are not connected.
Make the driver support probing with (at least) one output endpoint connected
and protect the cleanup function from accessing un-initialized fields.
Following patches will fix other user of un-initialized TXs in the driver,
such as power management functions.
Signed-off-by: Jacopo Mondi <jacopo+renesas@jmondi.org>
---
drivers/media/i2c/adv748x/adv748x-core.c | 25 ++++++++++++++++++++++---
drivers/media/i2c/adv748x/adv748x-csi2.c | 18 ++++++------------
drivers/media/i2c/adv748x/adv748x.h | 2 ++
3 files changed, 30 insertions(+), 15 deletions(-)
diff --git a/drivers/media/i2c/adv748x/adv748x-core.c b/drivers/media/i2c/adv748x/adv748x-core.c
index 6ca88daa..65c3024 100644
--- a/drivers/media/i2c/adv748x/adv748x-core.c
+++ b/drivers/media/i2c/adv748x/adv748x-core.c
@@ -569,7 +569,8 @@ static int adv748x_parse_dt(struct adv748x_state *state)
{
struct device_node *ep_np = NULL;
struct of_endpoint ep;
- bool found = false;
+ bool out_found = false;
+ bool in_found = false;
for_each_endpoint_of_node(state->dev->of_node, ep_np) {
of_graph_parse_endpoint(ep_np, &ep);
@@ -592,10 +593,17 @@ static int adv748x_parse_dt(struct adv748x_state *state)
of_node_get(ep_np);
state->endpoints[ep.port] = ep_np;
- found = true;
+ /*
+ * At least one input endpoint and one output endpoint shall
+ * be defined.
+ */
+ if (ep.port < ADV748X_PORT_TXA)
+ in_found = true;
+ else
+ out_found = true;
}
- return found ? 0 : -ENODEV;
+ return in_found && out_found ? 0 : -ENODEV;
}
static void adv748x_dt_cleanup(struct adv748x_state *state)
@@ -627,6 +635,17 @@ static int adv748x_probe(struct i2c_client *client,
state->i2c_clients[ADV748X_PAGE_IO] = client;
i2c_set_clientdata(client, state);
+ /*
+ * We can not use container_of to get back to the state with two TXs;
+ * Initialize the TXs's fields unconditionally on the endpoint
+ * presence to access them later.
+ */
+ state->txa.state = state->txb.state = state;
+ state->txa.page = ADV748X_PAGE_TXA;
+ state->txb.page = ADV748X_PAGE_TXB;
+ state->txa.port = ADV748X_PORT_TXA;
+ state->txb.port = ADV748X_PORT_TXB;
+
/* Discover and process ports declared by the Device tree endpoints */
ret = adv748x_parse_dt(state);
if (ret) {
diff --git a/drivers/media/i2c/adv748x/adv748x-csi2.c b/drivers/media/i2c/adv748x/adv748x-csi2.c
index 469be87..556e13c 100644
--- a/drivers/media/i2c/adv748x/adv748x-csi2.c
+++ b/drivers/media/i2c/adv748x/adv748x-csi2.c
@@ -266,19 +266,10 @@ static int adv748x_csi2_init_controls(struct adv748x_csi2 *tx)
int adv748x_csi2_init(struct adv748x_state *state, struct adv748x_csi2 *tx)
{
- struct device_node *ep;
int ret;
- /* We can not use container_of to get back to the state with two TXs */
- tx->state = state;
- tx->page = is_txa(tx) ? ADV748X_PAGE_TXA : ADV748X_PAGE_TXB;
-
- ep = state->endpoints[is_txa(tx) ? ADV748X_PORT_TXA : ADV748X_PORT_TXB];
- if (!ep) {
- adv_err(state, "No endpoint found for %s\n",
- is_txa(tx) ? "txa" : "txb");
- return -ENODEV;
- }
+ if (!is_tx_enabled(tx))
+ return 0;
/* Initialise the virtual channel */
adv748x_csi2_set_virtual_channel(tx, 0);
@@ -288,7 +279,7 @@ int adv748x_csi2_init(struct adv748x_state *state, struct adv748x_csi2 *tx)
is_txa(tx) ? "txa" : "txb");
/* Ensure that matching is based upon the endpoint fwnodes */
- tx->sd.fwnode = of_fwnode_handle(ep);
+ tx->sd.fwnode = of_fwnode_handle(state->endpoints[tx->port]);
/* Register internal ops for incremental subdev registration */
tx->sd.internal_ops = &adv748x_csi2_internal_ops;
@@ -321,6 +312,9 @@ int adv748x_csi2_init(struct adv748x_state *state, struct adv748x_csi2 *tx)
void adv748x_csi2_cleanup(struct adv748x_csi2 *tx)
{
+ if (!is_tx_enabled(tx))
+ return;
+
v4l2_async_unregister_subdev(&tx->sd);
media_entity_cleanup(&tx->sd.entity);
v4l2_ctrl_handler_free(&tx->ctrl_hdl);
diff --git a/drivers/media/i2c/adv748x/adv748x.h b/drivers/media/i2c/adv748x/adv748x.h
index 65f8374..1cf46c40 100644
--- a/drivers/media/i2c/adv748x/adv748x.h
+++ b/drivers/media/i2c/adv748x/adv748x.h
@@ -82,6 +82,7 @@ struct adv748x_csi2 {
struct adv748x_state *state;
struct v4l2_mbus_framefmt format;
unsigned int page;
+ unsigned int port;
struct media_pad pads[ADV748X_CSI2_NR_PADS];
struct v4l2_ctrl_handler ctrl_hdl;
@@ -91,6 +92,7 @@ struct adv748x_csi2 {
#define notifier_to_csi2(n) container_of(n, struct adv748x_csi2, notifier)
#define adv748x_sd_to_csi2(sd) container_of(sd, struct adv748x_csi2, sd)
+#define is_tx_enabled(_tx) ((_tx)->state->endpoints[(_tx)->port] != NULL)
enum adv748x_hdmi_pads {
ADV748X_HDMI_SINK,
--
2.7.4
^ permalink raw reply related [flat|nested] 12+ messages in thread* Re: [PATCH v2 1/5] media: i2c: adv748x: Support probing a single output
2018-09-05 15:27 ` [PATCH v2 1/5] media: i2c: adv748x: Support probing a single output Jacopo Mondi
@ 2018-09-12 13:14 ` Kieran Bingham
0 siblings, 0 replies; 12+ messages in thread
From: Kieran Bingham @ 2018-09-12 13:14 UTC (permalink / raw)
To: Jacopo Mondi, laurent.pinchart, niklas.soderlund+renesas
Cc: linux-renesas-soc, linux-media
Hi Jacopo,
Thank you for this patch and series,
On 05/09/18 16:27, Jacopo Mondi wrote:
> Currently the adv748x driver refuses to probe if both its output endpoints
> (TXA and TXB) are not connected.
>
> Make the driver support probing with (at least) one output endpoint connected
> and protect the cleanup function from accessing un-initialized fields.
I would expect it to fail if both were not connected, however this patch
tackles the case where there is not at least one output provided.
This patch also ensures that there is at least one input provided too,
so it would be worth specifying that as well.
Perhaps this could be better worded as:
=====
Currently the adv748x driver will fail to probe unless both of it's
output endpoints (TXA and TXB) are connected.
Make the driver support probing provided that there is at least one
input, and one output connected and protect the clean-up function from
accessing un-initialized fields.
=====
>
> Following patches will fix other user of un-initialized TXs in the driver,
/user/uses/
> such as power management functions.
>
> Signed-off-by: Jacopo Mondi <jacopo+renesas@jmondi.org>
Implementation looks good to me.
Reviewed-by: Kieran Bingham <kieran.bingham+renesas@ideasonboard.com>
> ---
> drivers/media/i2c/adv748x/adv748x-core.c | 25 ++++++++++++++++++++++---
> drivers/media/i2c/adv748x/adv748x-csi2.c | 18 ++++++------------
> drivers/media/i2c/adv748x/adv748x.h | 2 ++
> 3 files changed, 30 insertions(+), 15 deletions(-)
>
> diff --git a/drivers/media/i2c/adv748x/adv748x-core.c b/drivers/media/i2c/adv748x/adv748x-core.c
> index 6ca88daa..65c3024 100644
> --- a/drivers/media/i2c/adv748x/adv748x-core.c
> +++ b/drivers/media/i2c/adv748x/adv748x-core.c
> @@ -569,7 +569,8 @@ static int adv748x_parse_dt(struct adv748x_state *state)
> {
> struct device_node *ep_np = NULL;
> struct of_endpoint ep;
> - bool found = false;
> + bool out_found = false;
> + bool in_found = false;
>
> for_each_endpoint_of_node(state->dev->of_node, ep_np) {
> of_graph_parse_endpoint(ep_np, &ep);
> @@ -592,10 +593,17 @@ static int adv748x_parse_dt(struct adv748x_state *state)
> of_node_get(ep_np);
> state->endpoints[ep.port] = ep_np;
>
> - found = true;
> + /*
> + * At least one input endpoint and one output endpoint shall
> + * be defined.
> + */
> + if (ep.port < ADV748X_PORT_TXA)
> + in_found = true;
> + else
> + out_found = true;
> }
>
> - return found ? 0 : -ENODEV;
> + return in_found && out_found ? 0 : -ENODEV;
> }
>
> static void adv748x_dt_cleanup(struct adv748x_state *state)
> @@ -627,6 +635,17 @@ static int adv748x_probe(struct i2c_client *client,
> state->i2c_clients[ADV748X_PAGE_IO] = client;
> i2c_set_clientdata(client, state);
>
> + /*
> + * We can not use container_of to get back to the state with two TXs;
> + * Initialize the TXs's fields unconditionally on the endpoint
> + * presence to access them later.
> + */
> + state->txa.state = state->txb.state = state;
> + state->txa.page = ADV748X_PAGE_TXA;
> + state->txb.page = ADV748X_PAGE_TXB;
> + state->txa.port = ADV748X_PORT_TXA;
> + state->txb.port = ADV748X_PORT_TXB;
> +
> /* Discover and process ports declared by the Device tree endpoints */
> ret = adv748x_parse_dt(state);
> if (ret) {
> diff --git a/drivers/media/i2c/adv748x/adv748x-csi2.c b/drivers/media/i2c/adv748x/adv748x-csi2.c
> index 469be87..556e13c 100644
> --- a/drivers/media/i2c/adv748x/adv748x-csi2.c
> +++ b/drivers/media/i2c/adv748x/adv748x-csi2.c
> @@ -266,19 +266,10 @@ static int adv748x_csi2_init_controls(struct adv748x_csi2 *tx)
>
> int adv748x_csi2_init(struct adv748x_state *state, struct adv748x_csi2 *tx)
> {
> - struct device_node *ep;
> int ret;
>
> - /* We can not use container_of to get back to the state with two TXs */
> - tx->state = state;
> - tx->page = is_txa(tx) ? ADV748X_PAGE_TXA : ADV748X_PAGE_TXB;
> -
> - ep = state->endpoints[is_txa(tx) ? ADV748X_PORT_TXA : ADV748X_PORT_TXB];
> - if (!ep) {
> - adv_err(state, "No endpoint found for %s\n",
> - is_txa(tx) ? "txa" : "txb");
> - return -ENODEV;
> - }
> + if (!is_tx_enabled(tx))
> + return 0;
>
> /* Initialise the virtual channel */
> adv748x_csi2_set_virtual_channel(tx, 0);
> @@ -288,7 +279,7 @@ int adv748x_csi2_init(struct adv748x_state *state, struct adv748x_csi2 *tx)
> is_txa(tx) ? "txa" : "txb");
>
> /* Ensure that matching is based upon the endpoint fwnodes */
> - tx->sd.fwnode = of_fwnode_handle(ep);
> + tx->sd.fwnode = of_fwnode_handle(state->endpoints[tx->port]);
>
> /* Register internal ops for incremental subdev registration */
> tx->sd.internal_ops = &adv748x_csi2_internal_ops;
> @@ -321,6 +312,9 @@ int adv748x_csi2_init(struct adv748x_state *state, struct adv748x_csi2 *tx)
>
> void adv748x_csi2_cleanup(struct adv748x_csi2 *tx)
> {
> + if (!is_tx_enabled(tx))
> + return;
> +
> v4l2_async_unregister_subdev(&tx->sd);
> media_entity_cleanup(&tx->sd.entity);
> v4l2_ctrl_handler_free(&tx->ctrl_hdl);
> diff --git a/drivers/media/i2c/adv748x/adv748x.h b/drivers/media/i2c/adv748x/adv748x.h
> index 65f8374..1cf46c40 100644
> --- a/drivers/media/i2c/adv748x/adv748x.h
> +++ b/drivers/media/i2c/adv748x/adv748x.h
> @@ -82,6 +82,7 @@ struct adv748x_csi2 {
> struct adv748x_state *state;
> struct v4l2_mbus_framefmt format;
> unsigned int page;
> + unsigned int port;
>
> struct media_pad pads[ADV748X_CSI2_NR_PADS];
> struct v4l2_ctrl_handler ctrl_hdl;
> @@ -91,6 +92,7 @@ struct adv748x_csi2 {
>
> #define notifier_to_csi2(n) container_of(n, struct adv748x_csi2, notifier)
> #define adv748x_sd_to_csi2(sd) container_of(sd, struct adv748x_csi2, sd)
> +#define is_tx_enabled(_tx) ((_tx)->state->endpoints[(_tx)->port] != NULL)
>
> enum adv748x_hdmi_pads {
> ADV748X_HDMI_SINK,
>
^ permalink raw reply [flat|nested] 12+ messages in thread
* [PATCH v2 2/5] media: i2c: adv748x: Handle TX[A|B] power management
2018-09-05 15:27 [PATCH v2 0/5] media: adv748x: Allow probe with a single output endpoint Jacopo Mondi
2018-09-05 15:27 ` [PATCH v2 1/5] media: i2c: adv748x: Support probing a single output Jacopo Mondi
@ 2018-09-05 15:27 ` Jacopo Mondi
2018-09-16 14:09 ` Kieran Bingham
2018-09-05 15:27 ` [PATCH v2 3/5] media: i2c: adv748x: Conditionally enable only CSI-2 outputs Jacopo Mondi
` (2 subsequent siblings)
4 siblings, 1 reply; 12+ messages in thread
From: Jacopo Mondi @ 2018-09-05 15:27 UTC (permalink / raw)
To: laurent.pinchart, kieran.bingham+renesas,
niklas.soderlund+renesas
Cc: Jacopo Mondi, linux-renesas-soc, linux-media
As the driver is now allowed to probe with a single output endpoint,
power management routines shall now take into account the case a CSI-2 TX
is not enabled.
Unify the adv748x_tx_power() routine to handle transparently TXA and TXB,
and enable the CSI-2 outputs conditionally.
Signed-off-by: Jacopo Mondi <jacopo+renesas@jmondi.org>
---
drivers/media/i2c/adv748x/adv748x-afe.c | 2 +-
drivers/media/i2c/adv748x/adv748x-core.c | 52 +++++++++++++-------------------
drivers/media/i2c/adv748x/adv748x-csi2.c | 5 ---
drivers/media/i2c/adv748x/adv748x-hdmi.c | 2 +-
drivers/media/i2c/adv748x/adv748x.h | 7 ++---
5 files changed, 25 insertions(+), 43 deletions(-)
diff --git a/drivers/media/i2c/adv748x/adv748x-afe.c b/drivers/media/i2c/adv748x/adv748x-afe.c
index edd25e8..6d78105 100644
--- a/drivers/media/i2c/adv748x/adv748x-afe.c
+++ b/drivers/media/i2c/adv748x/adv748x-afe.c
@@ -286,7 +286,7 @@ static int adv748x_afe_s_stream(struct v4l2_subdev *sd, int enable)
goto unlock;
}
- ret = adv748x_txb_power(state, enable);
+ ret = adv748x_tx_power(&state->txb, enable);
if (ret)
goto unlock;
diff --git a/drivers/media/i2c/adv748x/adv748x-core.c b/drivers/media/i2c/adv748x/adv748x-core.c
index 65c3024..72a6692 100644
--- a/drivers/media/i2c/adv748x/adv748x-core.c
+++ b/drivers/media/i2c/adv748x/adv748x-core.c
@@ -292,33 +292,16 @@ static const struct adv748x_reg_value adv748x_power_down_txb_1lane[] = {
{ADV748X_PAGE_EOR, 0xff, 0xff} /* End of register table */
};
-int adv748x_txa_power(struct adv748x_state *state, bool on)
+int adv748x_tx_power(struct adv748x_csi2 *tx, bool on)
{
+ struct adv748x_state *state = tx->state;
+ const struct adv748x_reg_value *reglist;
int val;
- val = txa_read(state, ADV748X_CSI_FS_AS_LS);
- if (val < 0)
- return val;
-
- /*
- * This test against BIT(6) is not documented by the datasheet, but was
- * specified in the downstream driver.
- * Track with a WARN_ONCE to determine if it is ever set by HW.
- */
- WARN_ONCE((on && val & ADV748X_CSI_FS_AS_LS_UNKNOWN),
- "Enabling with unknown bit set");
-
- if (on)
- return adv748x_write_regs(state, adv748x_power_up_txa_4lane);
-
- return adv748x_write_regs(state, adv748x_power_down_txa_4lane);
-}
-
-int adv748x_txb_power(struct adv748x_state *state, bool on)
-{
- int val;
+ if (!is_tx_enabled(tx))
+ return 0;
- val = txb_read(state, ADV748X_CSI_FS_AS_LS);
+ val = tx_read(tx, ADV748X_CSI_FS_AS_LS);
if (val < 0)
return val;
@@ -331,9 +314,13 @@ int adv748x_txb_power(struct adv748x_state *state, bool on)
"Enabling with unknown bit set");
if (on)
- return adv748x_write_regs(state, adv748x_power_up_txb_1lane);
+ reglist = is_txa(tx) ? adv748x_power_up_txa_4lane :
+ adv748x_power_up_txb_1lane;
+ else
+ reglist = is_txa(tx) ? adv748x_power_down_txa_4lane :
+ adv748x_power_down_txb_1lane;
- return adv748x_write_regs(state, adv748x_power_down_txb_1lane);
+ return adv748x_write_regs(state, reglist);
}
/* -----------------------------------------------------------------------------
@@ -482,6 +469,7 @@ static const struct adv748x_reg_value adv748x_init_txb_1lane[] = {
static int adv748x_reset(struct adv748x_state *state)
{
int ret;
+ u8 regval = ADV748X_IO_10_PIX_OUT_EN;
ret = adv748x_write_regs(state, adv748x_sw_reset);
if (ret < 0)
@@ -496,22 +484,24 @@ static int adv748x_reset(struct adv748x_state *state)
if (ret)
return ret;
- adv748x_txa_power(state, 0);
+ adv748x_tx_power(&state->txa, 0);
/* Init and power down TXB */
ret = adv748x_write_regs(state, adv748x_init_txb_1lane);
if (ret)
return ret;
- adv748x_txb_power(state, 0);
+ adv748x_tx_power(&state->txb, 0);
/* Disable chip powerdown & Enable HDMI Rx block */
io_write(state, ADV748X_IO_PD, ADV748X_IO_PD_RX_EN);
- /* Enable 4-lane CSI Tx & Pixel Port */
- io_write(state, ADV748X_IO_10, ADV748X_IO_10_CSI4_EN |
- ADV748X_IO_10_CSI1_EN |
- ADV748X_IO_10_PIX_OUT_EN);
+ /* Conditionally enable TXa and TXb. */
+ if (is_tx_enabled(&state->txa))
+ regval |= ADV748X_IO_10_CSI4_EN;
+ if (is_tx_enabled(&state->txb))
+ regval |= ADV748X_IO_10_CSI1_EN;
+ io_write(state, ADV748X_IO_10, regval);
/* Use vid_std and v_freq as freerun resolution for CP */
cp_clrset(state, ADV748X_CP_CLMP_POS, ADV748X_CP_CLMP_POS_DIS_AUTO,
diff --git a/drivers/media/i2c/adv748x/adv748x-csi2.c b/drivers/media/i2c/adv748x/adv748x-csi2.c
index 556e13c..034fd93 100644
--- a/drivers/media/i2c/adv748x/adv748x-csi2.c
+++ b/drivers/media/i2c/adv748x/adv748x-csi2.c
@@ -18,11 +18,6 @@
#include "adv748x.h"
-static bool is_txa(struct adv748x_csi2 *tx)
-{
- return tx == &tx->state->txa;
-}
-
static int adv748x_csi2_set_virtual_channel(struct adv748x_csi2 *tx,
unsigned int vc)
{
diff --git a/drivers/media/i2c/adv748x/adv748x-hdmi.c b/drivers/media/i2c/adv748x/adv748x-hdmi.c
index aecc2a8..abb6568 100644
--- a/drivers/media/i2c/adv748x/adv748x-hdmi.c
+++ b/drivers/media/i2c/adv748x/adv748x-hdmi.c
@@ -362,7 +362,7 @@ static int adv748x_hdmi_s_stream(struct v4l2_subdev *sd, int enable)
mutex_lock(&state->mutex);
- ret = adv748x_txa_power(state, enable);
+ ret = adv748x_tx_power(&state->txa, enable);
if (ret)
goto done;
diff --git a/drivers/media/i2c/adv748x/adv748x.h b/drivers/media/i2c/adv748x/adv748x.h
index 1cf46c40..eeadf05 100644
--- a/drivers/media/i2c/adv748x/adv748x.h
+++ b/drivers/media/i2c/adv748x/adv748x.h
@@ -93,6 +93,7 @@ struct adv748x_csi2 {
#define notifier_to_csi2(n) container_of(n, struct adv748x_csi2, notifier)
#define adv748x_sd_to_csi2(sd) container_of(sd, struct adv748x_csi2, sd)
#define is_tx_enabled(_tx) ((_tx)->state->endpoints[(_tx)->port] != NULL)
+#define is_txa(_tx) ((_tx) == &(_tx)->state->txa)
enum adv748x_hdmi_pads {
ADV748X_HDMI_SINK,
@@ -378,9 +379,6 @@ int adv748x_write_block(struct adv748x_state *state, int client_page,
#define cp_write(s, r, v) adv748x_write(s, ADV748X_PAGE_CP, r, v)
#define cp_clrset(s, r, m, v) cp_write(s, r, (cp_read(s, r) & ~m) | v)
-#define txa_read(s, r) adv748x_read(s, ADV748X_PAGE_TXA, r)
-#define txb_read(s, r) adv748x_read(s, ADV748X_PAGE_TXB, r)
-
#define tx_read(t, r) adv748x_read(t->state, t->page, r)
#define tx_write(t, r, v) adv748x_write(t->state, t->page, r, v)
@@ -400,8 +398,7 @@ void adv748x_subdev_init(struct v4l2_subdev *sd, struct adv748x_state *state,
int adv748x_register_subdevs(struct adv748x_state *state,
struct v4l2_device *v4l2_dev);
-int adv748x_txa_power(struct adv748x_state *state, bool on);
-int adv748x_txb_power(struct adv748x_state *state, bool on);
+int adv748x_tx_power(struct adv748x_csi2 *tx, bool on);
int adv748x_afe_init(struct adv748x_afe *afe);
void adv748x_afe_cleanup(struct adv748x_afe *afe);
--
2.7.4
^ permalink raw reply related [flat|nested] 12+ messages in thread* Re: [PATCH v2 2/5] media: i2c: adv748x: Handle TX[A|B] power management
2018-09-05 15:27 ` [PATCH v2 2/5] media: i2c: adv748x: Handle TX[A|B] power management Jacopo Mondi
@ 2018-09-16 14:09 ` Kieran Bingham
0 siblings, 0 replies; 12+ messages in thread
From: Kieran Bingham @ 2018-09-16 14:09 UTC (permalink / raw)
To: Jacopo Mondi, laurent.pinchart, niklas.soderlund+renesas
Cc: linux-renesas-soc, linux-media
Hi Jacopo,
Thank you for addressing my comments on V1.
This looks good to me here.
On 05/09/18 16:27, Jacopo Mondi wrote:
> As the driver is now allowed to probe with a single output endpoint,
> power management routines shall now take into account the case a CSI-2 TX
> is not enabled.
>
> Unify the adv748x_tx_power() routine to handle transparently TXA and TXB,
> and enable the CSI-2 outputs conditionally.
>
> Signed-off-by: Jacopo Mondi <jacopo+renesas@jmondi.org>
Reviewed-by: Kieran Bingham <kieran.bingham+renesas@ideasonboard.com>
> ---
> drivers/media/i2c/adv748x/adv748x-afe.c | 2 +-
> drivers/media/i2c/adv748x/adv748x-core.c | 52 +++++++++++++-------------------
> drivers/media/i2c/adv748x/adv748x-csi2.c | 5 ---
> drivers/media/i2c/adv748x/adv748x-hdmi.c | 2 +-
> drivers/media/i2c/adv748x/adv748x.h | 7 ++---
> 5 files changed, 25 insertions(+), 43 deletions(-)
>
> diff --git a/drivers/media/i2c/adv748x/adv748x-afe.c b/drivers/media/i2c/adv748x/adv748x-afe.c
> index edd25e8..6d78105 100644
> --- a/drivers/media/i2c/adv748x/adv748x-afe.c
> +++ b/drivers/media/i2c/adv748x/adv748x-afe.c
> @@ -286,7 +286,7 @@ static int adv748x_afe_s_stream(struct v4l2_subdev *sd, int enable)
> goto unlock;
> }
>
> - ret = adv748x_txb_power(state, enable);
> + ret = adv748x_tx_power(&state->txb, enable);
> if (ret)
> goto unlock;
>
> diff --git a/drivers/media/i2c/adv748x/adv748x-core.c b/drivers/media/i2c/adv748x/adv748x-core.c
> index 65c3024..72a6692 100644
> --- a/drivers/media/i2c/adv748x/adv748x-core.c
> +++ b/drivers/media/i2c/adv748x/adv748x-core.c
> @@ -292,33 +292,16 @@ static const struct adv748x_reg_value adv748x_power_down_txb_1lane[] = {
> {ADV748X_PAGE_EOR, 0xff, 0xff} /* End of register table */
> };
>
> -int adv748x_txa_power(struct adv748x_state *state, bool on)
> +int adv748x_tx_power(struct adv748x_csi2 *tx, bool on)
> {
> + struct adv748x_state *state = tx->state;
> + const struct adv748x_reg_value *reglist;
> int val;
>
> - val = txa_read(state, ADV748X_CSI_FS_AS_LS);
> - if (val < 0)
> - return val;
> -
> - /*
> - * This test against BIT(6) is not documented by the datasheet, but was
> - * specified in the downstream driver.
> - * Track with a WARN_ONCE to determine if it is ever set by HW.
> - */
> - WARN_ONCE((on && val & ADV748X_CSI_FS_AS_LS_UNKNOWN),
> - "Enabling with unknown bit set");
> -
> - if (on)
> - return adv748x_write_regs(state, adv748x_power_up_txa_4lane);
> -
> - return adv748x_write_regs(state, adv748x_power_down_txa_4lane);
> -}
> -
> -int adv748x_txb_power(struct adv748x_state *state, bool on)
> -{
> - int val;
> + if (!is_tx_enabled(tx))
> + return 0;
>
> - val = txb_read(state, ADV748X_CSI_FS_AS_LS);
> + val = tx_read(tx, ADV748X_CSI_FS_AS_LS);
> if (val < 0)
> return val;
>
> @@ -331,9 +314,13 @@ int adv748x_txb_power(struct adv748x_state *state, bool on)
> "Enabling with unknown bit set");
>
> if (on)
> - return adv748x_write_regs(state, adv748x_power_up_txb_1lane);
> + reglist = is_txa(tx) ? adv748x_power_up_txa_4lane :
> + adv748x_power_up_txb_1lane;
> + else
> + reglist = is_txa(tx) ? adv748x_power_down_txa_4lane :
> + adv748x_power_down_txb_1lane;
>
> - return adv748x_write_regs(state, adv748x_power_down_txb_1lane);
> + return adv748x_write_regs(state, reglist);
> }
>
> /* -----------------------------------------------------------------------------
> @@ -482,6 +469,7 @@ static const struct adv748x_reg_value adv748x_init_txb_1lane[] = {
> static int adv748x_reset(struct adv748x_state *state)
> {
> int ret;
> + u8 regval = ADV748X_IO_10_PIX_OUT_EN;
>
> ret = adv748x_write_regs(state, adv748x_sw_reset);
> if (ret < 0)
> @@ -496,22 +484,24 @@ static int adv748x_reset(struct adv748x_state *state)
> if (ret)
> return ret;
>
> - adv748x_txa_power(state, 0);
> + adv748x_tx_power(&state->txa, 0);
>
> /* Init and power down TXB */
> ret = adv748x_write_regs(state, adv748x_init_txb_1lane);
> if (ret)
> return ret;
>
> - adv748x_txb_power(state, 0);
> + adv748x_tx_power(&state->txb, 0);
>
> /* Disable chip powerdown & Enable HDMI Rx block */
> io_write(state, ADV748X_IO_PD, ADV748X_IO_PD_RX_EN);
>
> - /* Enable 4-lane CSI Tx & Pixel Port */
> - io_write(state, ADV748X_IO_10, ADV748X_IO_10_CSI4_EN |
> - ADV748X_IO_10_CSI1_EN |
> - ADV748X_IO_10_PIX_OUT_EN);
> + /* Conditionally enable TXa and TXb. */
> + if (is_tx_enabled(&state->txa))
> + regval |= ADV748X_IO_10_CSI4_EN;
> + if (is_tx_enabled(&state->txb))
> + regval |= ADV748X_IO_10_CSI1_EN;
> + io_write(state, ADV748X_IO_10, regval);
>
> /* Use vid_std and v_freq as freerun resolution for CP */
> cp_clrset(state, ADV748X_CP_CLMP_POS, ADV748X_CP_CLMP_POS_DIS_AUTO,
> diff --git a/drivers/media/i2c/adv748x/adv748x-csi2.c b/drivers/media/i2c/adv748x/adv748x-csi2.c
> index 556e13c..034fd93 100644
> --- a/drivers/media/i2c/adv748x/adv748x-csi2.c
> +++ b/drivers/media/i2c/adv748x/adv748x-csi2.c
> @@ -18,11 +18,6 @@
>
> #include "adv748x.h"
>
> -static bool is_txa(struct adv748x_csi2 *tx)
> -{
> - return tx == &tx->state->txa;
> -}
> -
> static int adv748x_csi2_set_virtual_channel(struct adv748x_csi2 *tx,
> unsigned int vc)
> {
> diff --git a/drivers/media/i2c/adv748x/adv748x-hdmi.c b/drivers/media/i2c/adv748x/adv748x-hdmi.c
> index aecc2a8..abb6568 100644
> --- a/drivers/media/i2c/adv748x/adv748x-hdmi.c
> +++ b/drivers/media/i2c/adv748x/adv748x-hdmi.c
> @@ -362,7 +362,7 @@ static int adv748x_hdmi_s_stream(struct v4l2_subdev *sd, int enable)
>
> mutex_lock(&state->mutex);
>
> - ret = adv748x_txa_power(state, enable);
> + ret = adv748x_tx_power(&state->txa, enable);
> if (ret)
> goto done;
>
> diff --git a/drivers/media/i2c/adv748x/adv748x.h b/drivers/media/i2c/adv748x/adv748x.h
> index 1cf46c40..eeadf05 100644
> --- a/drivers/media/i2c/adv748x/adv748x.h
> +++ b/drivers/media/i2c/adv748x/adv748x.h
> @@ -93,6 +93,7 @@ struct adv748x_csi2 {
> #define notifier_to_csi2(n) container_of(n, struct adv748x_csi2, notifier)
> #define adv748x_sd_to_csi2(sd) container_of(sd, struct adv748x_csi2, sd)
> #define is_tx_enabled(_tx) ((_tx)->state->endpoints[(_tx)->port] != NULL)
> +#define is_txa(_tx) ((_tx) == &(_tx)->state->txa)
>
> enum adv748x_hdmi_pads {
> ADV748X_HDMI_SINK,
> @@ -378,9 +379,6 @@ int adv748x_write_block(struct adv748x_state *state, int client_page,
> #define cp_write(s, r, v) adv748x_write(s, ADV748X_PAGE_CP, r, v)
> #define cp_clrset(s, r, m, v) cp_write(s, r, (cp_read(s, r) & ~m) | v)
>
> -#define txa_read(s, r) adv748x_read(s, ADV748X_PAGE_TXA, r)
> -#define txb_read(s, r) adv748x_read(s, ADV748X_PAGE_TXB, r)
> -
> #define tx_read(t, r) adv748x_read(t->state, t->page, r)
> #define tx_write(t, r, v) adv748x_write(t->state, t->page, r, v)
>
> @@ -400,8 +398,7 @@ void adv748x_subdev_init(struct v4l2_subdev *sd, struct adv748x_state *state,
> int adv748x_register_subdevs(struct adv748x_state *state,
> struct v4l2_device *v4l2_dev);
>
> -int adv748x_txa_power(struct adv748x_state *state, bool on);
> -int adv748x_txb_power(struct adv748x_state *state, bool on);
> +int adv748x_tx_power(struct adv748x_csi2 *tx, bool on);
>
> int adv748x_afe_init(struct adv748x_afe *afe);
> void adv748x_afe_cleanup(struct adv748x_afe *afe);
>
^ permalink raw reply [flat|nested] 12+ messages in thread
* [PATCH v2 3/5] media: i2c: adv748x: Conditionally enable only CSI-2 outputs
2018-09-05 15:27 [PATCH v2 0/5] media: adv748x: Allow probe with a single output endpoint Jacopo Mondi
2018-09-05 15:27 ` [PATCH v2 1/5] media: i2c: adv748x: Support probing a single output Jacopo Mondi
2018-09-05 15:27 ` [PATCH v2 2/5] media: i2c: adv748x: Handle TX[A|B] power management Jacopo Mondi
@ 2018-09-05 15:27 ` Jacopo Mondi
2018-09-12 15:56 ` Kieran Bingham
2018-09-05 15:27 ` [PATCH v2 4/5] media: i2c: adv748x: Register only enabled inputs Jacopo Mondi
2018-09-05 15:27 ` [PATCH v2 5/5] media: i2c: adv748x: Register all input subdevices Jacopo Mondi
4 siblings, 1 reply; 12+ messages in thread
From: Jacopo Mondi @ 2018-09-05 15:27 UTC (permalink / raw)
To: laurent.pinchart, kieran.bingham+renesas,
niklas.soderlund+renesas
Cc: Jacopo Mondi, linux-renesas-soc, linux-media
The ADV748x has two CSI-2 output port and one TLL input/output port for
digital video reception/transmission. The TTL digital pad is un-conditionally
enabled during the device reset even if not used. Same goes for the TXa
and TXb CSI-2 outputs, which are enabled by the initial settings blob
programmed into the chip.
In order to improve power saving and do not enable un-used output interfaces:
keep TLL output disabled, as it is not used, and drop CSI-2 output enabling
from the intial settings list, as they get conditionally enabled later.
Signed-off-by: Jacopo Mondi <jacopo+renesas@jmondi.org>
---
drivers/media/i2c/adv748x/adv748x-core.c | 8 +-------
1 file changed, 1 insertion(+), 7 deletions(-)
diff --git a/drivers/media/i2c/adv748x/adv748x-core.c b/drivers/media/i2c/adv748x/adv748x-core.c
index 72a6692..7b79b0c 100644
--- a/drivers/media/i2c/adv748x/adv748x-core.c
+++ b/drivers/media/i2c/adv748x/adv748x-core.c
@@ -386,8 +386,6 @@ static const struct adv748x_reg_value adv748x_init_txa_4lane[] = {
{ADV748X_PAGE_IO, 0x0c, 0xe0}, /* Enable LLC_DLL & Double LLC Timing */
{ADV748X_PAGE_IO, 0x0e, 0xdd}, /* LLC/PIX/SPI PINS TRISTATED AUD */
- /* Outputs Enabled */
- {ADV748X_PAGE_IO, 0x10, 0xa0}, /* Enable 4-lane CSI Tx & Pixel Port */
{ADV748X_PAGE_TXA, 0x00, 0x84}, /* Enable 4-lane MIPI */
{ADV748X_PAGE_TXA, 0x00, 0xa4}, /* Set Auto DPHY Timing */
@@ -441,10 +439,6 @@ static const struct adv748x_reg_value adv748x_init_txb_1lane[] = {
{ADV748X_PAGE_SDP, 0x31, 0x12}, /* ADI Required Write */
{ADV748X_PAGE_SDP, 0xe6, 0x4f}, /* V bit end pos manually in NTSC */
- /* Enable 1-Lane MIPI Tx, */
- /* enable pixel output and route SD through Pixel port */
- {ADV748X_PAGE_IO, 0x10, 0x70},
-
{ADV748X_PAGE_TXB, 0x00, 0x81}, /* Enable 1-lane MIPI */
{ADV748X_PAGE_TXB, 0x00, 0xa1}, /* Set Auto DPHY Timing */
{ADV748X_PAGE_TXB, 0xd2, 0x40}, /* ADI Required Write */
@@ -469,7 +463,7 @@ static const struct adv748x_reg_value adv748x_init_txb_1lane[] = {
static int adv748x_reset(struct adv748x_state *state)
{
int ret;
- u8 regval = ADV748X_IO_10_PIX_OUT_EN;
+ u8 regval = 0;
ret = adv748x_write_regs(state, adv748x_sw_reset);
if (ret < 0)
--
2.7.4
^ permalink raw reply related [flat|nested] 12+ messages in thread* Re: [PATCH v2 3/5] media: i2c: adv748x: Conditionally enable only CSI-2 outputs
2018-09-05 15:27 ` [PATCH v2 3/5] media: i2c: adv748x: Conditionally enable only CSI-2 outputs Jacopo Mondi
@ 2018-09-12 15:56 ` Kieran Bingham
0 siblings, 0 replies; 12+ messages in thread
From: Kieran Bingham @ 2018-09-12 15:56 UTC (permalink / raw)
To: Jacopo Mondi, laurent.pinchart, niklas.soderlund+renesas
Cc: linux-renesas-soc, linux-media
Hi Jacopo,
This looks good - just some spelling and grammar to fix in the commit
message.
On 05/09/18 16:27, Jacopo Mondi wrote:
> The ADV748x has two CSI-2 output port and one TLL input/output port for
s/TLL/TTL/
> digital video reception/transmission. The TTL digital pad is un-conditionally
s/un-conditionally/unconditionally/
> enabled during the device reset even if not used. Same goes for the TXa
> and TXb CSI-2 outputs, which are enabled by the initial settings blob
Could we keep TXa, TXb capitalized as TXA and TXB to match the other
uses please?
> programmed into the chip.
>
> In order to improve power saving and do not enable un-used output interfaces:
s/power saving and do not/power saving, do not/
s/un-used/unused/
> keep TLL output disabled, as it is not used, and drop CSI-2 output enabling
s/TLL/TTL/
> from the intial settings list, as they get conditionally enabled later.
s/intial/initial/
>
> Signed-off-by: Jacopo Mondi <jacopo+renesas@jmondi.org>
I wonder if the power savings are measurable ... I might have to see if
I can get a comparison to work with the ACME power monitor I have.
Reviewed-by: Kieran Bingham <kieran.bingham+renesas@ideasonboard.com>
> ---
> drivers/media/i2c/adv748x/adv748x-core.c | 8 +-------
> 1 file changed, 1 insertion(+), 7 deletions(-)
>
> diff --git a/drivers/media/i2c/adv748x/adv748x-core.c b/drivers/media/i2c/adv748x/adv748x-core.c
> index 72a6692..7b79b0c 100644
> --- a/drivers/media/i2c/adv748x/adv748x-core.c
> +++ b/drivers/media/i2c/adv748x/adv748x-core.c
> @@ -386,8 +386,6 @@ static const struct adv748x_reg_value adv748x_init_txa_4lane[] = {
>
> {ADV748X_PAGE_IO, 0x0c, 0xe0}, /* Enable LLC_DLL & Double LLC Timing */
> {ADV748X_PAGE_IO, 0x0e, 0xdd}, /* LLC/PIX/SPI PINS TRISTATED AUD */
> - /* Outputs Enabled */
> - {ADV748X_PAGE_IO, 0x10, 0xa0}, /* Enable 4-lane CSI Tx & Pixel Port */
>
> {ADV748X_PAGE_TXA, 0x00, 0x84}, /* Enable 4-lane MIPI */
> {ADV748X_PAGE_TXA, 0x00, 0xa4}, /* Set Auto DPHY Timing */
> @@ -441,10 +439,6 @@ static const struct adv748x_reg_value adv748x_init_txb_1lane[] = {
> {ADV748X_PAGE_SDP, 0x31, 0x12}, /* ADI Required Write */
> {ADV748X_PAGE_SDP, 0xe6, 0x4f}, /* V bit end pos manually in NTSC */
>
> - /* Enable 1-Lane MIPI Tx, */
> - /* enable pixel output and route SD through Pixel port */
> - {ADV748X_PAGE_IO, 0x10, 0x70},
> -
> {ADV748X_PAGE_TXB, 0x00, 0x81}, /* Enable 1-lane MIPI */
> {ADV748X_PAGE_TXB, 0x00, 0xa1}, /* Set Auto DPHY Timing */
> {ADV748X_PAGE_TXB, 0xd2, 0x40}, /* ADI Required Write */
> @@ -469,7 +463,7 @@ static const struct adv748x_reg_value adv748x_init_txb_1lane[] = {
> static int adv748x_reset(struct adv748x_state *state)
> {
> int ret;
> - u8 regval = ADV748X_IO_10_PIX_OUT_EN;
> + u8 regval = 0;
>
> ret = adv748x_write_regs(state, adv748x_sw_reset);
> if (ret < 0)
>
^ permalink raw reply [flat|nested] 12+ messages in thread
* [PATCH v2 4/5] media: i2c: adv748x: Register only enabled inputs
2018-09-05 15:27 [PATCH v2 0/5] media: adv748x: Allow probe with a single output endpoint Jacopo Mondi
` (2 preceding siblings ...)
2018-09-05 15:27 ` [PATCH v2 3/5] media: i2c: adv748x: Conditionally enable only CSI-2 outputs Jacopo Mondi
@ 2018-09-05 15:27 ` Jacopo Mondi
2018-09-16 14:19 ` Kieran Bingham
2018-09-05 15:27 ` [PATCH v2 5/5] media: i2c: adv748x: Register all input subdevices Jacopo Mondi
4 siblings, 1 reply; 12+ messages in thread
From: Jacopo Mondi @ 2018-09-05 15:27 UTC (permalink / raw)
To: laurent.pinchart, kieran.bingham+renesas,
niklas.soderlund+renesas
Cc: Jacopo Mondi, linux-renesas-soc, linux-media
The adv748x assumes input endpoints are always enabled, and registers
a subdevice for each of them everytime the corresponding output subdevice
is registered.
Fix this by conditionally register the input subdevice only if it is
actually described in device tree.
Signed-off-by: Jacopo Mondi <jacopo+renesas@jmondi.org>
---
drivers/media/i2c/adv748x/adv748x-csi2.c | 6 +++---
drivers/media/i2c/adv748x/adv748x.h | 10 ++++++++++
2 files changed, 13 insertions(+), 3 deletions(-)
diff --git a/drivers/media/i2c/adv748x/adv748x-csi2.c b/drivers/media/i2c/adv748x/adv748x-csi2.c
index 034fd93..9e9df51 100644
--- a/drivers/media/i2c/adv748x/adv748x-csi2.c
+++ b/drivers/media/i2c/adv748x/adv748x-csi2.c
@@ -82,15 +82,15 @@ static int adv748x_csi2_registered(struct v4l2_subdev *sd)
*
* Link HDMI->TXA, and AFE->TXB directly.
*/
- if (is_txa(tx)) {
+ if (is_txa(tx) && is_hdmi_enabled(state))
return adv748x_csi2_register_link(tx, sd->v4l2_dev,
&state->hdmi.sd,
ADV748X_HDMI_SOURCE);
- } else {
+ if (!is_txa(tx) && is_afe_enabled(state))
return adv748x_csi2_register_link(tx, sd->v4l2_dev,
&state->afe.sd,
ADV748X_AFE_SOURCE);
- }
+ return 0;
}
static const struct v4l2_subdev_internal_ops adv748x_csi2_internal_ops = {
diff --git a/drivers/media/i2c/adv748x/adv748x.h b/drivers/media/i2c/adv748x/adv748x.h
index eeadf05..a34004e 100644
--- a/drivers/media/i2c/adv748x/adv748x.h
+++ b/drivers/media/i2c/adv748x/adv748x.h
@@ -94,6 +94,16 @@ struct adv748x_csi2 {
#define adv748x_sd_to_csi2(sd) container_of(sd, struct adv748x_csi2, sd)
#define is_tx_enabled(_tx) ((_tx)->state->endpoints[(_tx)->port] != NULL)
#define is_txa(_tx) ((_tx) == &(_tx)->state->txa)
+#define is_afe_enabled(_state) \
+ ((_state)->endpoints[ADV748X_PORT_AIN0] != NULL || \
+ (_state)->endpoints[ADV748X_PORT_AIN1] != NULL || \
+ (_state)->endpoints[ADV748X_PORT_AIN2] != NULL || \
+ (_state)->endpoints[ADV748X_PORT_AIN3] != NULL || \
+ (_state)->endpoints[ADV748X_PORT_AIN4] != NULL || \
+ (_state)->endpoints[ADV748X_PORT_AIN5] != NULL || \
+ (_state)->endpoints[ADV748X_PORT_AIN6] != NULL || \
+ (_state)->endpoints[ADV748X_PORT_AIN7] != NULL)
+#define is_hdmi_enabled(_state) ((_state)->endpoints[ADV748X_PORT_HDMI] != NULL)
enum adv748x_hdmi_pads {
ADV748X_HDMI_SINK,
--
2.7.4
^ permalink raw reply related [flat|nested] 12+ messages in thread* Re: [PATCH v2 4/5] media: i2c: adv748x: Register only enabled inputs
2018-09-05 15:27 ` [PATCH v2 4/5] media: i2c: adv748x: Register only enabled inputs Jacopo Mondi
@ 2018-09-16 14:19 ` Kieran Bingham
0 siblings, 0 replies; 12+ messages in thread
From: Kieran Bingham @ 2018-09-16 14:19 UTC (permalink / raw)
To: Jacopo Mondi, laurent.pinchart, niklas.soderlund+renesas
Cc: linux-renesas-soc, linux-media
Hi Jacopo,
Thankyou for the patch,
On 05/09/18 16:27, Jacopo Mondi wrote:
> The adv748x assumes input endpoints are always enabled, and registers
> a subdevice for each of them everytime the corresponding output subdevice
> is registered.
>
s/everytime/every time/
Although this sounds like something that happens repeatedly in the
lifetime of the driver when it only actually happens once I believe?
In which case perhaps:
s/everytime/when/
> Fix this by conditionally register the input subdevice only if it is
s/register/registering/
> actually described in device tree.
>
> Signed-off-by: Jacopo Mondi <jacopo+renesas@jmondi.org>
Otherwise,
Reviewed-by: Kieran Bingham <kieran.bingham+renesas@ideasonboard.com>
> ---
> drivers/media/i2c/adv748x/adv748x-csi2.c | 6 +++---
> drivers/media/i2c/adv748x/adv748x.h | 10 ++++++++++
> 2 files changed, 13 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/media/i2c/adv748x/adv748x-csi2.c b/drivers/media/i2c/adv748x/adv748x-csi2.c
> index 034fd93..9e9df51 100644
> --- a/drivers/media/i2c/adv748x/adv748x-csi2.c
> +++ b/drivers/media/i2c/adv748x/adv748x-csi2.c
> @@ -82,15 +82,15 @@ static int adv748x_csi2_registered(struct v4l2_subdev *sd)
> *
> * Link HDMI->TXA, and AFE->TXB directly.
> */
> - if (is_txa(tx)) {
> + if (is_txa(tx) && is_hdmi_enabled(state))
> return adv748x_csi2_register_link(tx, sd->v4l2_dev,
> &state->hdmi.sd,
> ADV748X_HDMI_SOURCE);
> - } else {
> + if (!is_txa(tx) && is_afe_enabled(state))
> return adv748x_csi2_register_link(tx, sd->v4l2_dev,
> &state->afe.sd,
> ADV748X_AFE_SOURCE);
> - }
> + return 0;
> }
>
> static const struct v4l2_subdev_internal_ops adv748x_csi2_internal_ops = {
> diff --git a/drivers/media/i2c/adv748x/adv748x.h b/drivers/media/i2c/adv748x/adv748x.h
> index eeadf05..a34004e 100644
> --- a/drivers/media/i2c/adv748x/adv748x.h
> +++ b/drivers/media/i2c/adv748x/adv748x.h
> @@ -94,6 +94,16 @@ struct adv748x_csi2 {
> #define adv748x_sd_to_csi2(sd) container_of(sd, struct adv748x_csi2, sd)
> #define is_tx_enabled(_tx) ((_tx)->state->endpoints[(_tx)->port] != NULL)
> #define is_txa(_tx) ((_tx) == &(_tx)->state->txa)
> +#define is_afe_enabled(_state) \
> + ((_state)->endpoints[ADV748X_PORT_AIN0] != NULL || \
> + (_state)->endpoints[ADV748X_PORT_AIN1] != NULL || \
> + (_state)->endpoints[ADV748X_PORT_AIN2] != NULL || \
> + (_state)->endpoints[ADV748X_PORT_AIN3] != NULL || \
> + (_state)->endpoints[ADV748X_PORT_AIN4] != NULL || \
> + (_state)->endpoints[ADV748X_PORT_AIN5] != NULL || \
> + (_state)->endpoints[ADV748X_PORT_AIN6] != NULL || \
> + (_state)->endpoints[ADV748X_PORT_AIN7] != NULL)
> +#define is_hdmi_enabled(_state) ((_state)->endpoints[ADV748X_PORT_HDMI] != NULL)
Wow that's quite some conditionals on the AFE :)
We could introduce a flag instead... but it's just implementation
detail. I'll leave that up to you, if you prefer this then that's fine.
We're not on a hot-path here so it's not a big deal.
>
> enum adv748x_hdmi_pads {
> ADV748X_HDMI_SINK,
>
^ permalink raw reply [flat|nested] 12+ messages in thread
* [PATCH v2 5/5] media: i2c: adv748x: Register all input subdevices
2018-09-05 15:27 [PATCH v2 0/5] media: adv748x: Allow probe with a single output endpoint Jacopo Mondi
` (3 preceding siblings ...)
2018-09-05 15:27 ` [PATCH v2 4/5] media: i2c: adv748x: Register only enabled inputs Jacopo Mondi
@ 2018-09-05 15:27 ` Jacopo Mondi
2018-09-16 14:39 ` Kieran Bingham
4 siblings, 1 reply; 12+ messages in thread
From: Jacopo Mondi @ 2018-09-05 15:27 UTC (permalink / raw)
To: laurent.pinchart, kieran.bingham+renesas,
niklas.soderlund+renesas
Cc: Jacopo Mondi, linux-renesas-soc, linux-media
The input subdevice registration, being the link between adv728x's inputs
and outputs fixed, happens at output subdevice registration time. In the
current design the TXA output subdevice 'registered()' callback registers
the HDMI input subdevice and the TXB output subdevice 'registered()' callback
registers the AFE input subdevice instead. Media links are created
accordingly to the fixed routing.
As the adv748x driver can now probe with at least a single output port
enabled an input subdevice linked to a disabled output is never registered
to the media graph. Fix this by having the first registered output subdevice
register all the available input subdevices.
This change is necessary to have dynamic routing between the adv748x inputs
and outputs implemented.
Signed-off-by: Jacopo Mondi <jacopo+renesas@jmondi.org>
---
drivers/media/i2c/adv748x/adv748x-csi2.c | 85 +++++++++++++++-----------------
1 file changed, 40 insertions(+), 45 deletions(-)
diff --git a/drivers/media/i2c/adv748x/adv748x-csi2.c b/drivers/media/i2c/adv748x/adv748x-csi2.c
index 9e9df51..fd4aa9d 100644
--- a/drivers/media/i2c/adv748x/adv748x-csi2.c
+++ b/drivers/media/i2c/adv748x/adv748x-csi2.c
@@ -24,42 +24,6 @@ static int adv748x_csi2_set_virtual_channel(struct adv748x_csi2 *tx,
return tx_write(tx, ADV748X_CSI_VC_REF, vc << ADV748X_CSI_VC_REF_SHIFT);
}
-/**
- * adv748x_csi2_register_link : Register and link internal entities
- *
- * @tx: CSI2 private entity
- * @v4l2_dev: Video registration device
- * @src: Source subdevice to establish link
- * @src_pad: Pad number of source to link to this @tx
- *
- * Ensure that the subdevice is registered against the v4l2_device, and link the
- * source pad to the sink pad of the CSI2 bus entity.
- */
-static int adv748x_csi2_register_link(struct adv748x_csi2 *tx,
- struct v4l2_device *v4l2_dev,
- struct v4l2_subdev *src,
- unsigned int src_pad)
-{
- int enabled = MEDIA_LNK_FL_ENABLED;
- int ret;
-
- /*
- * Dynamic linking of the AFE is not supported.
- * Register the links as immutable.
- */
- enabled |= MEDIA_LNK_FL_IMMUTABLE;
-
- if (!src->v4l2_dev) {
- ret = v4l2_device_register_subdev(v4l2_dev, src);
- if (ret)
- return ret;
- }
-
- return media_create_pad_link(&src->entity, src_pad,
- &tx->sd.entity, ADV748X_CSI2_SINK,
- enabled);
-}
-
/* -----------------------------------------------------------------------------
* v4l2_subdev_internal_ops
*
@@ -72,25 +36,56 @@ static int adv748x_csi2_registered(struct v4l2_subdev *sd)
{
struct adv748x_csi2 *tx = adv748x_sd_to_csi2(sd);
struct adv748x_state *state = tx->state;
+ struct v4l2_subdev *src_sd;
+ unsigned int src_pad;
+ int ret;
adv_dbg(state, "Registered %s (%s)", is_txa(tx) ? "TXA":"TXB",
sd->name);
+ /* The first registered CSI-2 registers all input subdevices. */
+ src_sd = &state->hdmi.sd;
+ if (!src_sd->v4l2_dev && is_hdmi_enabled(state)) {
+ ret = v4l2_device_register_subdev(sd->v4l2_dev, src_sd);
+ if (ret)
+ return ret;
+ }
+
+ src_sd = &state->afe.sd;
+ if (!src_sd->v4l2_dev && is_afe_enabled(state)) {
+ ret = v4l2_device_register_subdev(sd->v4l2_dev, src_sd);
+ if (ret)
+ goto err_unregister_hdmi;
+ }
+
/*
* The adv748x hardware allows the AFE to route through the TXA, however
* this is not currently supported in this driver.
*
* Link HDMI->TXA, and AFE->TXB directly.
*/
- if (is_txa(tx) && is_hdmi_enabled(state))
- return adv748x_csi2_register_link(tx, sd->v4l2_dev,
- &state->hdmi.sd,
- ADV748X_HDMI_SOURCE);
- if (!is_txa(tx) && is_afe_enabled(state))
- return adv748x_csi2_register_link(tx, sd->v4l2_dev,
- &state->afe.sd,
- ADV748X_AFE_SOURCE);
- return 0;
+ if (is_txa(tx)) {
+ if (!is_hdmi_enabled(state))
+ return 0;
+
+ src_sd = &state->hdmi.sd;
+ src_pad = ADV748X_HDMI_SOURCE;
+ } else {
+ if (!is_afe_enabled(state))
+ return 0;
+
+ src_sd = &state->afe.sd;
+ src_pad = ADV748X_AFE_SOURCE;
+ }
+
+ /* Dynamic linking is not supported, register the links as immutable. */
+ return media_create_pad_link(&src_sd->entity, src_pad, &sd->entity,
+ ADV748X_CSI2_SINK, MEDIA_LNK_FL_ENABLED |
+ MEDIA_LNK_FL_IMMUTABLE);
+err_unregister_hdmi:
+ v4l2_device_unregister_subdev(&state->hdmi.sd);
+
+ return ret;
}
static const struct v4l2_subdev_internal_ops adv748x_csi2_internal_ops = {
--
2.7.4
^ permalink raw reply related [flat|nested] 12+ messages in thread* Re: [PATCH v2 5/5] media: i2c: adv748x: Register all input subdevices
2018-09-05 15:27 ` [PATCH v2 5/5] media: i2c: adv748x: Register all input subdevices Jacopo Mondi
@ 2018-09-16 14:39 ` Kieran Bingham
2018-09-17 11:17 ` jacopo mondi
0 siblings, 1 reply; 12+ messages in thread
From: Kieran Bingham @ 2018-09-16 14:39 UTC (permalink / raw)
To: Jacopo Mondi, laurent.pinchart, niklas.soderlund+renesas
Cc: linux-renesas-soc, linux-media
Hi Jacopo,
On 05/09/18 16:27, Jacopo Mondi wrote:
> The input subdevice registration, being the link between adv728x's inputs
s/adv728x/adv748x/
> and outputs fixed, happens at output subdevice registration time. In the
'are fixed, and happens at' ?
> current design the TXA output subdevice 'registered()' callback registers
> the HDMI input subdevice and the TXB output subdevice 'registered()' callback
> registers the AFE input subdevice instead. Media links are created
> accordingly to the fixed routing.
>
> As the adv748x driver can now probe with at least a single output port
> enabled an input subdevice linked to a disabled output is never registered
> to the media graph. Fix this by having the first registered output subdevice
> register all the available input subdevices.
>
If the input device can not be routed to the output device, then does it
matter that it's not registered?
> This change is necessary to have dynamic routing between the adv748x inputs
> and outputs implemented.
Indeed, unless it's necessary I feel like a this change or equivalent
should be part of a series which introduces dynamic routing.
(I.e., not that this patch should be discarded, but perhaps not
integrated quite yet)
> Signed-off-by: Jacopo Mondi <jacopo+renesas@jmondi.org>
> ---
> drivers/media/i2c/adv748x/adv748x-csi2.c | 85 +++++++++++++++-----------------
> 1 file changed, 40 insertions(+), 45 deletions(-)
>
> diff --git a/drivers/media/i2c/adv748x/adv748x-csi2.c b/drivers/media/i2c/adv748x/adv748x-csi2.c
> index 9e9df51..fd4aa9d 100644
> --- a/drivers/media/i2c/adv748x/adv748x-csi2.c
> +++ b/drivers/media/i2c/adv748x/adv748x-csi2.c
> @@ -24,42 +24,6 @@ static int adv748x_csi2_set_virtual_channel(struct adv748x_csi2 *tx,
> return tx_write(tx, ADV748X_CSI_VC_REF, vc << ADV748X_CSI_VC_REF_SHIFT);
> }
>
> -/**
> - * adv748x_csi2_register_link : Register and link internal entities
> - *
> - * @tx: CSI2 private entity
> - * @v4l2_dev: Video registration device
> - * @src: Source subdevice to establish link
> - * @src_pad: Pad number of source to link to this @tx
> - *
> - * Ensure that the subdevice is registered against the v4l2_device, and link the
> - * source pad to the sink pad of the CSI2 bus entity.
> - */
> -static int adv748x_csi2_register_link(struct adv748x_csi2 *tx,
> - struct v4l2_device *v4l2_dev,
> - struct v4l2_subdev *src,
> - unsigned int src_pad)
> -{
> - int enabled = MEDIA_LNK_FL_ENABLED;
> - int ret;
> -
> - /*
> - * Dynamic linking of the AFE is not supported.
> - * Register the links as immutable.
> - */
> - enabled |= MEDIA_LNK_FL_IMMUTABLE;
> -
> - if (!src->v4l2_dev) {
> - ret = v4l2_device_register_subdev(v4l2_dev, src);
> - if (ret)
> - return ret;
> - }
> -
> - return media_create_pad_link(&src->entity, src_pad,
> - &tx->sd.entity, ADV748X_CSI2_SINK,
> - enabled);
> -}
> -
> /* -----------------------------------------------------------------------------
> * v4l2_subdev_internal_ops
> *
> @@ -72,25 +36,56 @@ static int adv748x_csi2_registered(struct v4l2_subdev *sd)
> {
> struct adv748x_csi2 *tx = adv748x_sd_to_csi2(sd);
> struct adv748x_state *state = tx->state;
> + struct v4l2_subdev *src_sd;
> + unsigned int src_pad;
> + int ret;
>
> adv_dbg(state, "Registered %s (%s)", is_txa(tx) ? "TXA":"TXB",
> sd->name);
>
> + /* The first registered CSI-2 registers all input subdevices. */
> + src_sd = &state->hdmi.sd;
> + if (!src_sd->v4l2_dev && is_hdmi_enabled(state)) {
> + ret = v4l2_device_register_subdev(sd->v4l2_dev, src_sd);
> + if (ret)
> + return ret;
> + }
> +
> + src_sd = &state->afe.sd;
> + if (!src_sd->v4l2_dev && is_afe_enabled(state)) {
> + ret = v4l2_device_register_subdev(sd->v4l2_dev, src_sd);
> + if (ret)
> + goto err_unregister_hdmi;
> + }
> +
This registers both the AFE and HDMI as subdevices of the first v4l2_dev
... Is this an issue ? (in fact is the v4l2_dev for both CSI/TX devices
the same?)
What happens if we connect the TXA to one capture VIN device, and the
TXB to a completely different input device (this is probably a bit
hypothetical at the moment)
> /*
> * The adv748x hardware allows the AFE to route through the TXA, however
> * this is not currently supported in this driver.
> *
> * Link HDMI->TXA, and AFE->TXB directly.
> */
> - if (is_txa(tx) && is_hdmi_enabled(state))
> - return adv748x_csi2_register_link(tx, sd->v4l2_dev,
> - &state->hdmi.sd,
> - ADV748X_HDMI_SOURCE);
> - if (!is_txa(tx) && is_afe_enabled(state))
> - return adv748x_csi2_register_link(tx, sd->v4l2_dev,
> - &state->afe.sd,
> - ADV748X_AFE_SOURCE);
> - return 0;
> + if (is_txa(tx)) {
> + if (!is_hdmi_enabled(state))
> + return 0;
> +
> + src_sd = &state->hdmi.sd;
> + src_pad = ADV748X_HDMI_SOURCE;
> + } else {
> + if (!is_afe_enabled(state))
> + return 0;
> +
> + src_sd = &state->afe.sd;
> + src_pad = ADV748X_AFE_SOURCE;
> + }
> +
> + /* Dynamic linking is not supported, register the links as immutable. */
> + return media_create_pad_link(&src_sd->entity, src_pad, &sd->entity,
> + ADV748X_CSI2_SINK, MEDIA_LNK_FL_ENABLED |
> + MEDIA_LNK_FL_IMMUTABLE);
> +err_unregister_hdmi:
> + v4l2_device_unregister_subdev(&state->hdmi.sd);
> +
> + return ret;
> }
>
> static const struct v4l2_subdev_internal_ops adv748x_csi2_internal_ops = {
>
--
Kieran
^ permalink raw reply [flat|nested] 12+ messages in thread* Re: [PATCH v2 5/5] media: i2c: adv748x: Register all input subdevices
2018-09-16 14:39 ` Kieran Bingham
@ 2018-09-17 11:17 ` jacopo mondi
0 siblings, 0 replies; 12+ messages in thread
From: jacopo mondi @ 2018-09-17 11:17 UTC (permalink / raw)
To: Kieran Bingham
Cc: Jacopo Mondi, laurent.pinchart, niklas.soderlund+renesas,
linux-renesas-soc, linux-media
[-- Attachment #1: Type: text/plain, Size: 6572 bytes --]
Hi Kieran,
On Sun, Sep 16, 2018 at 03:39:17PM +0100, Kieran Bingham wrote:
> Hi Jacopo,
>
> On 05/09/18 16:27, Jacopo Mondi wrote:
> > The input subdevice registration, being the link between adv728x's inputs
>
> s/adv728x/adv748x/
>
> > and outputs fixed, happens at output subdevice registration time. In the
>
> 'are fixed, and happens at' ?
>
Are you sure? With your suggestions this would look like
The input subdevice registration, being the link between adv728x's
inputs are fixed, and happens at output subdevice registration time.
Not a native speaker, but this doesn't seems right to me :)
>
> > current design the TXA output subdevice 'registered()' callback registers
> > the HDMI input subdevice and the TXB output subdevice 'registered()' callback
> > registers the AFE input subdevice instead. Media links are created
> > accordingly to the fixed routing.
> >
> > As the adv748x driver can now probe with at least a single output port
> > enabled an input subdevice linked to a disabled output is never registered
> > to the media graph. Fix this by having the first registered output subdevice
> > register all the available input subdevices.
> >
>
> If the input device can not be routed to the output device, then does it
> matter that it's not registered?
>
> > This change is necessary to have dynamic routing between the adv748x inputs
> > and outputs implemented.
>
> Indeed, unless it's necessary I feel like a this change or equivalent
> should be part of a series which introduces dynamic routing.
>
> (I.e., not that this patch should be discarded, but perhaps not
> integrated quite yet)
>
You're right, without dynamic routing this patch can be safely
postponed. In the Ebisu case, the AFE frontend, won't simply show up.
Furthermore, this patch registers the subdevice only, does not create
links, so it has been useful just to make me happy because of seeing
all subdevices part of the media graph.
Let's post-pone this to dynamic routing implementation, which is not
far I guess.
Thanks for comments on all other patches, I incorporated your
suggestions, and will send v3 now.
Thanks
j
> > Signed-off-by: Jacopo Mondi <jacopo+renesas@jmondi.org>
> > ---
> > drivers/media/i2c/adv748x/adv748x-csi2.c | 85 +++++++++++++++-----------------
> > 1 file changed, 40 insertions(+), 45 deletions(-)
> >
> > diff --git a/drivers/media/i2c/adv748x/adv748x-csi2.c b/drivers/media/i2c/adv748x/adv748x-csi2.c
> > index 9e9df51..fd4aa9d 100644
> > --- a/drivers/media/i2c/adv748x/adv748x-csi2.c
> > +++ b/drivers/media/i2c/adv748x/adv748x-csi2.c
> > @@ -24,42 +24,6 @@ static int adv748x_csi2_set_virtual_channel(struct adv748x_csi2 *tx,
> > return tx_write(tx, ADV748X_CSI_VC_REF, vc << ADV748X_CSI_VC_REF_SHIFT);
> > }
> >
> > -/**
> > - * adv748x_csi2_register_link : Register and link internal entities
> > - *
> > - * @tx: CSI2 private entity
> > - * @v4l2_dev: Video registration device
> > - * @src: Source subdevice to establish link
> > - * @src_pad: Pad number of source to link to this @tx
> > - *
> > - * Ensure that the subdevice is registered against the v4l2_device, and link the
> > - * source pad to the sink pad of the CSI2 bus entity.
> > - */
> > -static int adv748x_csi2_register_link(struct adv748x_csi2 *tx,
> > - struct v4l2_device *v4l2_dev,
> > - struct v4l2_subdev *src,
> > - unsigned int src_pad)
> > -{
> > - int enabled = MEDIA_LNK_FL_ENABLED;
> > - int ret;
> > -
> > - /*
> > - * Dynamic linking of the AFE is not supported.
> > - * Register the links as immutable.
> > - */
> > - enabled |= MEDIA_LNK_FL_IMMUTABLE;
> > -
> > - if (!src->v4l2_dev) {
> > - ret = v4l2_device_register_subdev(v4l2_dev, src);
> > - if (ret)
> > - return ret;
> > - }
> > -
> > - return media_create_pad_link(&src->entity, src_pad,
> > - &tx->sd.entity, ADV748X_CSI2_SINK,
> > - enabled);
> > -}
> > -
> > /* -----------------------------------------------------------------------------
> > * v4l2_subdev_internal_ops
> > *
> > @@ -72,25 +36,56 @@ static int adv748x_csi2_registered(struct v4l2_subdev *sd)
> > {
> > struct adv748x_csi2 *tx = adv748x_sd_to_csi2(sd);
> > struct adv748x_state *state = tx->state;
> > + struct v4l2_subdev *src_sd;
> > + unsigned int src_pad;
> > + int ret;
> >
> > adv_dbg(state, "Registered %s (%s)", is_txa(tx) ? "TXA":"TXB",
> > sd->name);
> >
> > + /* The first registered CSI-2 registers all input subdevices. */
> > + src_sd = &state->hdmi.sd;
> > + if (!src_sd->v4l2_dev && is_hdmi_enabled(state)) {
> > + ret = v4l2_device_register_subdev(sd->v4l2_dev, src_sd);
> > + if (ret)
> > + return ret;
> > + }
> > +
> > + src_sd = &state->afe.sd;
> > + if (!src_sd->v4l2_dev && is_afe_enabled(state)) {
> > + ret = v4l2_device_register_subdev(sd->v4l2_dev, src_sd);
> > + if (ret)
> > + goto err_unregister_hdmi;
> > + }
> > +
>
>
> This registers both the AFE and HDMI as subdevices of the first v4l2_dev
> ... Is this an issue ? (in fact is the v4l2_dev for both CSI/TX devices
> the same?)
>
> What happens if we connect the TXA to one capture VIN device, and the
> TXB to a completely different input device (this is probably a bit
> hypothetical at the moment)
>
>
>
> > /*
> > * The adv748x hardware allows the AFE to route through the TXA, however
> > * this is not currently supported in this driver.
> > *
> > * Link HDMI->TXA, and AFE->TXB directly.
> > */
> > - if (is_txa(tx) && is_hdmi_enabled(state))
> > - return adv748x_csi2_register_link(tx, sd->v4l2_dev,
> > - &state->hdmi.sd,
> > - ADV748X_HDMI_SOURCE);
> > - if (!is_txa(tx) && is_afe_enabled(state))
> > - return adv748x_csi2_register_link(tx, sd->v4l2_dev,
> > - &state->afe.sd,
> > - ADV748X_AFE_SOURCE);
> > - return 0;
> > + if (is_txa(tx)) {
> > + if (!is_hdmi_enabled(state))
> > + return 0;
> > +
> > + src_sd = &state->hdmi.sd;
> > + src_pad = ADV748X_HDMI_SOURCE;
> > + } else {
> > + if (!is_afe_enabled(state))
> > + return 0;
> > +
> > + src_sd = &state->afe.sd;
> > + src_pad = ADV748X_AFE_SOURCE;
> > + }
> > +
> > + /* Dynamic linking is not supported, register the links as immutable. */
> > + return media_create_pad_link(&src_sd->entity, src_pad, &sd->entity,
> > + ADV748X_CSI2_SINK, MEDIA_LNK_FL_ENABLED |
> > + MEDIA_LNK_FL_IMMUTABLE);
> > +err_unregister_hdmi:
> > + v4l2_device_unregister_subdev(&state->hdmi.sd);
> > +
> > + return ret;
> > }
> >
> > static const struct v4l2_subdev_internal_ops adv748x_csi2_internal_ops = {
> >
>
> --
> Kieran
>
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 819 bytes --]
^ permalink raw reply [flat|nested] 12+ messages in thread