diff mbox series

[3/5] media: adv748x: Store the source subdevice in TX

Message ID 1544541373-30044-4-git-send-email-jacopo+renesas@jmondi.org (mailing list archive)
State Changes Requested
Delegated to: Kieran Bingham
Headers show
Series media: adv748x: Implement dynamic routing support | expand

Commit Message

Jacopo Mondi Dec. 11, 2018, 3:16 p.m. UTC
The power_up_tx() procedure needs to set a few registers conditionally to
the selected video source, but it currently checks for the provided tx to
be either TXA or TXB.

With the introduction of dynamic routing between HDMI and AFE entities to
TXA, checking which TX the function is operating on is not meaningful anymore.

To fix this, store the subdevice of the source providing video data to the
CSI-2 TX in the 'struct adv748x_csi2' representing the TX and check on it.

Signed-off-by: Jacopo Mondi <jacopo+renesas@jmondi.org>
---
 drivers/media/i2c/adv748x/adv748x-core.c | 2 +-
 drivers/media/i2c/adv748x/adv748x-csi2.c | 3 +++
 drivers/media/i2c/adv748x/adv748x.h      | 1 +
 3 files changed, 5 insertions(+), 1 deletion(-)

Comments

Kieran Bingham Dec. 11, 2018, 11:19 p.m. UTC | #1
Hi Jacopo,

On 11/12/2018 15:16, Jacopo Mondi wrote:
> The power_up_tx() procedure needs to set a few registers conditionally to
> the selected video source, but it currently checks for the provided tx to
> be either TXA or TXB.
> 
> With the introduction of dynamic routing between HDMI and AFE entities to
> TXA, checking which TX the function is operating on is not meaningful anymore.
> 
> To fix this, store the subdevice of the source providing video data to the
> CSI-2 TX in the 'struct adv748x_csi2' representing the TX and check on it.
> 
> Signed-off-by: Jacopo Mondi <jacopo+renesas@jmondi.org>
> ---
>  drivers/media/i2c/adv748x/adv748x-core.c | 2 +-
>  drivers/media/i2c/adv748x/adv748x-csi2.c | 3 +++
>  drivers/media/i2c/adv748x/adv748x.h      | 1 +
>  3 files changed, 5 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/media/i2c/adv748x/adv748x-core.c b/drivers/media/i2c/adv748x/adv748x-core.c
> index 5495dc7891e8..f3aabbccdfb5 100644
> --- a/drivers/media/i2c/adv748x/adv748x-core.c
> +++ b/drivers/media/i2c/adv748x/adv748x-core.c
> @@ -254,7 +254,7 @@ static int adv748x_power_up_tx(struct adv748x_csi2 *tx)
>  	adv748x_write_check(state, page, 0x00, 0xa0 | tx->num_lanes, &ret);
>  
>  	/* ADI Required Write */
> -	if (is_txa(tx)) {
> +	if (tx->rsd == &state->hdmi.sd) {

rsd? I presume this means 'remote subdevice' here?

I feel like 'src' is more meaningful here ... perhaps this could read
  (tx->src == &state->hdmi.sd)

Is that more understandable?


>  		adv748x_write_check(state, page, 0xdb, 0x10, &ret);
>  		adv748x_write_check(state, page, 0xd6, 0x07, &ret);
>  	} else {
> diff --git a/drivers/media/i2c/adv748x/adv748x-csi2.c b/drivers/media/i2c/adv748x/adv748x-csi2.c
> index 4d1aefc2c8d0..307966f4c736 100644
> --- a/drivers/media/i2c/adv748x/adv748x-csi2.c
> +++ b/drivers/media/i2c/adv748x/adv748x-csi2.c
> @@ -46,6 +46,9 @@ static int adv748x_csi2_register_link(struct adv748x_csi2 *tx,
>  			return ret;
>  	}
>  
> +	if (flags & MEDIA_LNK_FL_ENABLED)
> +		tx->rsd = src;

Is this easy to clear when the link changes?

(I'll find out in a later patch I guess)

> +
>  	return media_create_pad_link(&src->entity, src_pad,
>  				     &tx->sd.entity, ADV748X_CSI2_SINK,
>  				     flags);
> diff --git a/drivers/media/i2c/adv748x/adv748x.h b/drivers/media/i2c/adv748x/adv748x.h
> index b482c7fe6957..387002d6da65 100644
> --- a/drivers/media/i2c/adv748x/adv748x.h
> +++ b/drivers/media/i2c/adv748x/adv748x.h
> @@ -85,6 +85,7 @@ struct adv748x_csi2 {
>  	struct v4l2_ctrl_handler ctrl_hdl;
>  	struct v4l2_ctrl *pixel_rate;
>  	struct v4l2_subdev sd;
> +	struct v4l2_subdev *rsd;
>  };
>  
>  #define notifier_to_csi2(n) container_of(n, struct adv748x_csi2, notifier)
>
Laurent Pinchart Dec. 13, 2018, 9:29 a.m. UTC | #2
Hi Jacopo,

Thank you for the patch.

On Tuesday, 11 December 2018 17:16:11 EET Jacopo Mondi wrote:
> The power_up_tx() procedure needs to set a few registers conditionally to
> the selected video source, but it currently checks for the provided tx to
> be either TXA or TXB.
> 
> With the introduction of dynamic routing between HDMI and AFE entities to
> TXA, checking which TX the function is operating on is not meaningful
> anymore.
> 
> To fix this, store the subdevice of the source providing video data to the
> CSI-2 TX in the 'struct adv748x_csi2' representing the TX and check on it.
> 
> Signed-off-by: Jacopo Mondi <jacopo+renesas@jmondi.org>
> ---
>  drivers/media/i2c/adv748x/adv748x-core.c | 2 +-
>  drivers/media/i2c/adv748x/adv748x-csi2.c | 3 +++
>  drivers/media/i2c/adv748x/adv748x.h      | 1 +
>  3 files changed, 5 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/media/i2c/adv748x/adv748x-core.c
> b/drivers/media/i2c/adv748x/adv748x-core.c index 5495dc7891e8..f3aabbccdfb5
> 100644
> --- a/drivers/media/i2c/adv748x/adv748x-core.c
> +++ b/drivers/media/i2c/adv748x/adv748x-core.c
> @@ -254,7 +254,7 @@ static int adv748x_power_up_tx(struct adv748x_csi2 *tx)
>  	adv748x_write_check(state, page, 0x00, 0xa0 | tx->num_lanes, &ret);
> 
>  	/* ADI Required Write */
> -	if (is_txa(tx)) {
> +	if (tx->rsd == &state->hdmi.sd) {
>  		adv748x_write_check(state, page, 0xdb, 0x10, &ret);
>  		adv748x_write_check(state, page, 0xd6, 0x07, &ret);
>  	} else {
> diff --git a/drivers/media/i2c/adv748x/adv748x-csi2.c
> b/drivers/media/i2c/adv748x/adv748x-csi2.c index 4d1aefc2c8d0..307966f4c736
> 100644
> --- a/drivers/media/i2c/adv748x/adv748x-csi2.c
> +++ b/drivers/media/i2c/adv748x/adv748x-csi2.c
> @@ -46,6 +46,9 @@ static int adv748x_csi2_register_link(struct adv748x_csi2
> *tx, return ret;
>  	}
> 
> +	if (flags & MEDIA_LNK_FL_ENABLED)
> +		tx->rsd = src;
> +
>  	return media_create_pad_link(&src->entity, src_pad,
>  				     &tx->sd.entity, ADV748X_CSI2_SINK,
>  				     flags);
> diff --git a/drivers/media/i2c/adv748x/adv748x.h
> b/drivers/media/i2c/adv748x/adv748x.h index b482c7fe6957..387002d6da65
> 100644
> --- a/drivers/media/i2c/adv748x/adv748x.h
> +++ b/drivers/media/i2c/adv748x/adv748x.h
> @@ -85,6 +85,7 @@ struct adv748x_csi2 {
>  	struct v4l2_ctrl_handler ctrl_hdl;
>  	struct v4l2_ctrl *pixel_rate;
>  	struct v4l2_subdev sd;
> +	struct v4l2_subdev *rsd;

How about naming this source instead of rsd ? rsd is a bit cryptic.

With that change,

Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>

>  };
> 
>  #define notifier_to_csi2(n) container_of(n, struct adv748x_csi2, notifier)
diff mbox series

Patch

diff --git a/drivers/media/i2c/adv748x/adv748x-core.c b/drivers/media/i2c/adv748x/adv748x-core.c
index 5495dc7891e8..f3aabbccdfb5 100644
--- a/drivers/media/i2c/adv748x/adv748x-core.c
+++ b/drivers/media/i2c/adv748x/adv748x-core.c
@@ -254,7 +254,7 @@  static int adv748x_power_up_tx(struct adv748x_csi2 *tx)
 	adv748x_write_check(state, page, 0x00, 0xa0 | tx->num_lanes, &ret);
 
 	/* ADI Required Write */
-	if (is_txa(tx)) {
+	if (tx->rsd == &state->hdmi.sd) {
 		adv748x_write_check(state, page, 0xdb, 0x10, &ret);
 		adv748x_write_check(state, page, 0xd6, 0x07, &ret);
 	} else {
diff --git a/drivers/media/i2c/adv748x/adv748x-csi2.c b/drivers/media/i2c/adv748x/adv748x-csi2.c
index 4d1aefc2c8d0..307966f4c736 100644
--- a/drivers/media/i2c/adv748x/adv748x-csi2.c
+++ b/drivers/media/i2c/adv748x/adv748x-csi2.c
@@ -46,6 +46,9 @@  static int adv748x_csi2_register_link(struct adv748x_csi2 *tx,
 			return ret;
 	}
 
+	if (flags & MEDIA_LNK_FL_ENABLED)
+		tx->rsd = src;
+
 	return media_create_pad_link(&src->entity, src_pad,
 				     &tx->sd.entity, ADV748X_CSI2_SINK,
 				     flags);
diff --git a/drivers/media/i2c/adv748x/adv748x.h b/drivers/media/i2c/adv748x/adv748x.h
index b482c7fe6957..387002d6da65 100644
--- a/drivers/media/i2c/adv748x/adv748x.h
+++ b/drivers/media/i2c/adv748x/adv748x.h
@@ -85,6 +85,7 @@  struct adv748x_csi2 {
 	struct v4l2_ctrl_handler ctrl_hdl;
 	struct v4l2_ctrl *pixel_rate;
 	struct v4l2_subdev sd;
+	struct v4l2_subdev *rsd;
 };
 
 #define notifier_to_csi2(n) container_of(n, struct adv748x_csi2, notifier)